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, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> 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".
>
> The reason why I found this exchange interesting is because I think
> it shows a noteworthy gap between end-user expectations and what the
> implementors know.
>
> Stepping back (or sideways) a bit, pretend for a while that there
> were no "pickaxe" feature in Git.  Instead there is the "patch-grep"
> tool whose design is roughly:
>
>    1. It reads "git log -p" output from its standard input, and
>       splits the lines into records, each of which consists of the
>       header part (i.e. starting at the "commit <object name>" line,
>       to the first blank line before the title), the log message
>       part, and the patch part.
>
>    2. It takes command line arguments, which are, like "git grep",
>       patterns to match and instructions on how to combine the match
>       result.
>
>    3. It applies the match criteria only to the patch part of each
>       record.  A record without any match in the patch part is
>       discarded.
>
>    4. It uses the surviving record's "commit <object name>" lines
>       to decide what commits to show.  It does the moral equivalent
>       of invoking "git show" on each of them, and perhaps lets you
>       affect how the commits are shown.
>
>       Or perhaps it just lists the commit object names chosen for
>       further processing by downstream tools that read from it.
>
>
> So the user would be able to say something like
>
> 	git log -Ux --since=6.months |
> 	git patch-grep \
> 		--commit-names-only \
> 		--all-match \
> 		-e '+.*devm_request_threaded_irq(IRQF_SHARED)' \
>                 -e '-.*devm_request_threaded_irq(IRQF_ONESHOT)' |
> 	xargs git show --oneline -s
>
> As an implementor, you know that is not how your -G<pattern> thing
> works, but coming from the end-user side, I think it is a reasonable
> mental model to expect a tool to work more like so.  And I think the
> expectation from combining --oneline with -Ux was that the -U option
> would apply to step 1, not step 4 (as --oneline is a clear
> indication that the user wants a very concise final result).
>
> Personally, I think the _best_ match for the original wish would be
> to have that hypothetical "git patch-grep" read from "git log -L"
> that is limited to the C function in the source the user is
> interested in.
>
> And until "git patch-grep" becomes reality, I would probably have
> done
>
> 	git log -L<function of interest> -U<x> | less
>
> and asked "less" to skip to a match with
>
> 	/(IRQF_SHARED|IRQF_ONESHOT)
>
> and then kept hitting 'n' until I find what replaces them, as a
> stop-gap measure.
>
> By the way, I think your thing is interesting regardless, even if it
> does not match the use case in the original thread (it actually may
> match---I didn't think it through).

Yeah it's definitely a bit orthagonal, should have sent it in reply to
something else and actually read the E-Mail, but I think it's useful.

> Because in the context of diff/log family, however, the word "raw"
> has a specific connotation about the "--raw" format (as opposed to
> "--patch"), I would not call this "grep the patch output itself,
> instead of grepping the source (guided by the patch output to tell
> what lines are near the lines that got replaced)" feature anything
> "raw", by the way.

I agree, brainfarted on not thinking about "raw". Do you or anyone have
a suggestion for a better CLI option name?

Maybe --pickaxe-patch or --pickaxe-patch-format (to go with git-diff's
-u aka --patch (i.e. not --raw) default format)? Or
--pickaxe-G-with-context or --pickaxe-with-context or
--with-pickaxe-context or --pickaxe-context ? All of these suck, but I'm
coming up blank on a better one :)

Probably the least shitty of those shitty options is --pickaxe-patch,
since we have --patch which triggers the same format, and we can
document that the default is a -G search through --no-pickaxe-patch, and
you can just tweak the format.

It also leaves the door open (unlike having *-G-* in the option) to
support this for -S if anyone cared...