Re: Why doesn't `git log -m` imply `-p`?

[Sergey Organov <sorganov@gmail.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> Yep, but --diff-merges=m doesn't imply -p either, though it does produce
>>> diff output without -p, for merge commits only.
>>
>> I misspoke without thinking it through.  It is absolutely wrong for
>> the "-m" option (or "--diff-merges=m" for that matter) to imply
>> "-p".
>>
>> $ git log --stat --summary
>>
>> would show "diff", but the kind of "diff" requested is not a textual
>> patch but just diffstat and the summary of new/removed files, and
>> the "diff" is shown only for single-parent commits, and it omits
>> "diff" for merge commits.  Adding "-m" to this command line is *not*
>> a request to show the textual patch.  It is to ask "diff" to be
>> shown pairwise with each of the parent.
>>
>> $ git log -m --stat --summary
>>
>> It is probably OK to special case "-m" given alone without any other
>> option [*1*] that specifies what kind of "diff" is requested and
>> make it imply "-p".  But unconditionally flipping "-p" on only
>> because you saw "-m" (or "--diff-merges=m" for that matter) is just
>> wrong.
>
> Luckily,
>
>     $ git log [--stat] --diff-merges=first-parent master..seen
>
> seems to do almost the right thing, with respect to the "It is
> probably OK to special case" I gave above.

I believe any special-casing is to be a last resort, and definitely is
not the right thing to do in this particular case.

>
> It only "enables diff" for merge commits, which does not quite feel
> right and we may want to do the same "enable diff" for single parent
> commits, but the good part is that it does not blindly imply "-p".
>
> It seems to do the "enable diff" the right way by honoring other
> command line options that specify the format of the diff, so with
> "--stat" included in the sample command line above, we get the
> diffstat for single parent commits (because we ask for "--stat" from
> the command line to show it throughout the history) and also for
> merge commits (because --diff-merges=first-parent does *not* blindly
> turn the textual patch '-p' on).

Good to know! I must admit I did nothing special in this regard, just
paid attention to avoid breaking any existing logic, at least knowingly.

>
>> [Footnote]
>>
>> *1* They are not limited to "-p", "--stat" and "--summary", but
>> you'd need to also pay attention to "--raw", "--name-only", etc.)
>
> I've merged the so/log-diff-merge topic to 'master', with this
> (possibly) known breakage that it does not do anything for single
> parent commits.  We may want to fix this last mile before the
> release that is scheduled to happen around early June.

I have no idea what the breakage is or could be. Do we have any relevant
tests in place already, or can somebody suggest some to check for
possible breakage?

> Note that I didn't check if you are doing the right thing for all
> formats, or if I was lucky and --stat was only one of them you paid
> attention to, when you needed to notice all others that you don't.
> But if you used the same logic that allows "git show" to by default
> give "-p/--cc" output while "git show --stat" to squelch the patch
> output, you should be OK.

In fact I didn't do anything specific to --stat, nor to other options
you mention (--raw, --name-only, --summary), so I'd expect all of them
still work the same way they were before my --diff-merges series. Need
to be checked anyway, sure thing, and that gets us back to the questions
of tests. I personally don't know what expectations are, so it's hard
for me to implement needed tests.

Thanks,
-- Sergey Organov