Re: [PATCH 2/2] diffcore-pickaxe: add --pickaxe-raw-diff for use with -G

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Apr 25 2019, Eugeniu Rosca wrote:

> On Thu, Apr 25, 2019 at 01:24:56AM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Thu, Apr 25 2019, Eugeniu Rosca wrote:
>>
>> > Hi Ævar,
>> >
>> > Thanks for the amazingly fast reply and for the useful feature (yay!).
>> >
>> > On Wed, Apr 24, 2019 at 05:37:10PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> >>
>> >> On Wed, Apr 24 2019, Ævar Arnfjörð Bjarmason wrote:
>> >>
>> >> > Add the ability for the -G<regex> pickaxe to search only through added
>> >> > or removed lines in the diff, or even through an arbitrary amount of
>> >> > context lines when combined with -U<n>.
>> >> >
>> >> > This has been requested[1][2] a few times in the past, and isn't
>> >> > currently possible. Instead users need to do -G<regex> and then write
>> >> > their own post-parsing script to see if the <regex> matched added or
>> >> > removed lines, or both. There was no way to match the adjacent context
>> >> > lines other than running and grepping the equivalent of a "log -p -U<n>".
>> >> >
>> >> > 1. https://public-inbox.org/git/xmqqwoqrr8y2.fsf@gitster-ct.c.googlers.com/
>> >> > 2. https://public-inbox.org/git/20190424102609.GA19697@vmlxhi-102.adit-jv.com/
>> >>
>> >> I see now once I actually read Eugeniu Rosca's E-Mail upthread instead
>> >> of just knee-jerk sending out patches that this doesn't actually solve
>> >> his particular problem fully.
>> >>
>> >> I.e. if you want some AND/OR matching support this --pickaxe-raw-diff
>> >> won't give you that, but it *does* make it much easier to script up such
>> >> an option. Run it twice with -G"\+<regex>" and -G"-<regex>", "sort |
>> >> uniq -c" the commit list, and see which things occur once or twice.
>> >>
>> >> Of course that doesn't give you more complex nested and/or cases, but if
>> >> git-log grew support for that like git-grep has the -G option could use
>> >> that, although at that point we'd probably want to spend effort on
>> >> making the underlying machinery smarter to avoid duplicate work.
>> >
>> > Purely from user's standpoint, I feel more comfortable with `git grep`
>> > and `git log --grep` particularly b/c they support '--all-match' [2],
>> > allowing more flexible multi-line searches. Based on your feedback, it
>> > looks to me that `git log -G/-S` did not have a chance to develop their
>> > features to the same level.
>> >
>> >>
>> >> Furthermore, and quoting Eugeniu upthread:
>> >>
>> >>     In the context of [1], I would like to find all Linux commits which
>> >>     replaced:
>> >>     	'devm_request_threaded_irq(* IRQF_SHARED *)'
>> >>     by:
>> >>     	'devm_request_threaded_irq(* IRQF_ONESHOT *)'
>> >>
>> >> Such AND/OR machinery would give you what you wanted *most* of the time,
>> >> but it would also find removed/added pairs that were "unrelated" as well
>> >> as "related". Solving *that* problem is more complex, but something the
>> >> diff machinery could in principle expose.
>> >
>> > I expect some false positives, since git is agnostic on the language
>> > used to write the versioned files (the latter sounds like a research
>> > topic to me - I hope there is somebody willing to experiment with that
>> > in future).
>>
>> I was thinking of something where the added/removed could be filtered to
>> cases that occur in the same diff hunk.
>>
>> >>
>> >> But the "-G<regex> --pickaxe-raw-diff" feature I have as-is is very
>> >> useful,
>> >
>> > I agree. I am a bit bothered by the fact that
>> > `git log --oneline -Ux -G<regex> --pickaxe-raw-diff` outputs the
>> > contents/patch of a commit. My expectation is that we have the
>> > `log -p` knob for that?
>>
>> This is unrelated to --pickaxe-raw-diff, -U<n> just implies -p in
>> general. See e.g. "git log -U1".
>
> Oops. Since I use `-U<n>` mostly with `git show`, I missed the
> implication. You are right. Then, my question is how users are
> going to (quote from commit description):
>
>> >> > [..] search [..] through an arbitrary amount of
>> >> > context lines when combined with -U<n>.
>
> and achieve a `git log --oneline` report, given that -U<n> unfolds
> the commits?
>
> FTR, based on my quick experiments, --pickaxe-raw-diff does process
> several lines of context by default (it appears to default to -U3).

Yeah I should document this explicitly. We use the default diff context
so if you just -G'foo.*bar' you'll find things in the 6x lines of
context (3 before / 3 after), not just the "-" and "+" lines.

It's a "feature", but we should be really clear about it, i.e. you need
to anchor with "^[+-]" if you want the same thing that -G does for you
now.

I *do* find the default semantics really useful. Sometimes you can use
-L, but I've often done manual greps with -U<n> for "let's find code
changes anywhere in the project near places where we use some API",
maybe we should pick -U0 with --pickaxe-raw-diff by default to avoid
*that* particular surprise by default, but I think that would be even
more confusing...

>>
>> >> I've had at least two people off-list ask me about a problem
>> >> that would be solved by it just in the last 1/2 year (unrelated to them
>> >> having seen the WIP patch I sent last October).
>> >>
>> >> It's more general than Junio's suggested --pickaxe-ignore-{add,del}
>> >
>> > As a user, I would be happier to freely grep in the raw commit contents
>> > rather than learning a dozen of new options which provide small subsets
>> > of the same functionality. So, I personally vote for the approach taken
>> > by --pickaxe-raw-diff. This would also reduce the complexity of my
>> > current git aliases and/or allow dropping some of them altogether.
>> >
>> > Quite off topic, but I also needed to come up with a solution to get
>> > the C functions modified/touched by a git commit [3]. It is my
>> > understanding that --pickaxe-raw-diff can't help here and I still have
>> > to rely on parsing the output of `git log -p`?
>>
>> Yeah, it doesn't help with that. When it runs we haven't generated the
>> context line or the "@@" line yet, that's later. You can breakpoint on
>> xdl_format_hunk_hdr and diffgrep_consume to see it in action.
>>
>> It's a waste of CPU to generate that for all possible hunks, most of
>> which we won't show at all.
>>
>> But it's of course possible to do so by running the full diff machinery
>> over every commit and matching on the result, the current pickaxe is
>> just taking shortcuts and not doing that.
>>
>> >> options[1], but those could be implemented in terms of this underlying
>> >> code if anyone cared to have those as aliases. You'd just take the
>> >> -G<regex> and prefix the <regex> with "^\+" or "^-" as appropriate and
>> >> turn on the DIFF_PICKAXE_G_RAW_DIFF flag.
>> >>
>> >> 1. https://public-inbox.org/git/xmqqwoqrr8y2.fsf@gitster-ct.c.googlers.com/
>> >
>> > Thanks!
>> >
>> > [2] https://gitster.livejournal.com/30195.html
>> > [3] https://stackoverflow.com/questions/50707171/how-to-get-all-c-functions-modified-by-a-git-commit