Sergey Organov <sorganov@xxxxxxxxx> writes:
>> 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.
I do not know if I get it. "log --diff-merges=<kind>" giving the
same output as "log" (i.e. no trace of any kind of diff) would be
puzzling to users, and to help them, it is OK to say that
* "--diff-merges=<kind>" enables some kind of diff output
automatially (for both merges and non-merges), and
* when there is no user preference given as to what kind of diff is
desired, we default to "-p".
As it is natural to expect "--stat --diff-merges=<kind> would give
only the diffstat without patch, we end up "special casing"
"--diff-merges=<kind>" that is given alone, without specifying what
kind of diff is desired, and behave as if "-p" was given. So I
would have expected you to call this kind of "special casing" a good
thing.
>> 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.
Because I view
* "--diff-merges" is a way to specify how merge commits are passed
to the diff machinery (e.g. pass nothing to the diff machinery,
compare only with the first parent, etc.), and
* "--patch", "--stat", "--cc" etc are to specify if we use the diff
machinery and what kind of output is desired.
but we are conflating the "enable diff" feature into the former to
match end-user expectation, if "--diff-merges" without any of the
"--patch", "--stat", etc. enables the "--patch" output for merge
commits, it would be confusing if we do not give the same "--patch"
output for single-parent commits, too.
But the current code gives "--patch" output only for merge commits,
doesn't it? E.g.
$ git log --diff-merges=first-parent master..next
would give patches only for merge commits, but
$ git log --stat --diff-merges=first-parent master..next
would give us diffstat for all commits, including merges (against
their first parents).
