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

From: Sergey Organov <sorganov@gmail.com>
Date: May 3, 2021, 17:42 UTC
Message-ID: <87czu7u32v.fsf@osv.gnss.ru>
In-Reply-To: <xmqqzgxfb80r.fsf@gitster.g>

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

Show 15 quoted lines

> Sergey Organov <sorganov@gmail.com> 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

I thought (apparently wrong) that the idea was to special-case "-m", and only "-m". I.e., if -m is alone, let it imply -p, otherwise not. That was the thing I was in opposition to.

>
>  * "--diff-merges=<kind>" enables some kind of diff output
>    automatially (for both merges and non-merges), and

No, I don't think this is OK, sorry. I fail to see why --diff-merges should affect non-merge commits. I believe it shouldn't.

>
>  * when there is no user preference given as to what kind of diff is
>    desired, we default to "-p".

What kind of diff "-p" gives for merge commits, exactly? As far as I can tell, it's "none".

Show 5 quoted lines

>
> 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.

In general, I hate dependencies between options. The only thing that is actually needed for convenience is an ability for an option to imply other options. What's currently there is already too complex for my personal taste, and I'd hate to add even more complexity on top.

Right now --diff-merges is pretty simple and straightforward: it specifies diff output for merge commits, and only for merge commits. If any other option disables diff machinery altogether, it will disable diff for merges as well.

OTOH, we have --patch that deals with non-merge commits.

Personally, I don't like the resulting interface very much, but it's historical, and can't be easily changed.

> So I would have expected you to call this kind of "special casing" a
> good thing.

Well, even though I was originally commenting about -m only, I must admit I'm in general against any "special casing", unless there is extremely strong reason to consider it.

>
>>> 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,

I fail to see why --diff-merges should ever affect non-merge commits. It'd be at least counter-intuitive, not to say directly opposite to the original design goal.

>>> but the good part is that it does not blindly imply "-p".

Yep.

Show 30 quoted lines

>>>
>>> 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

As I see it, it only defines the way they are to be represented by the diff machinery once passed to it, though it obviously depends on where we put the margin of "diff machinery".

>
>  * "--patch", "--stat", "--cc" etc are to specify if we use the diff
>    machinery and what kind of output is desired.

So, in your view, --cc output is not a product of "diff machinery"?

Show 8 quoted lines

> 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.

No, as far as I understand it, "--patch" output is for non-merge commits only. One can't sensibly use patch utility to pick merge commits anyway, so "--patch" makes no sense for merge commits and doesn't affect them, at least for now.

>
>     $ git log --diff-merges=first-parent master..next
>
> would give patches only for merge commits, but

It will give the output similar to what "--patch" would give for non-merge commits, yes, but in fact it's not "--patch" output, I think, so I doubt it should be called "give patches". It's just happens to be the same diff format.

Show 5 quoted lines

>
>     $ git log --stat --diff-merges=first-parent master..next
>
> would give us diffstat for all commits, including merges (against
> their first parents).

Yep, but I think it just matches the old behavior that has been always there, see below.

I'd start from the behavior even before my patches. Let's see:

  git log -n1 -p <merge_commit>
  git log -n1 --stat <merge_commit>
  git log -n1 --stat -p <merge_commit>

all give no diff no stat. No surprise for diff, though not that sure about stat, but it could be argued either way.

  git log -n1 -c <merge_commit>

does give diff output in particular format, nice!

  git log -n1 --stat -c <merge_commit>

gives stat output, but no diff! That's not what I expected at all. Effectively, this looks like --stat *disables* -c/-cc output?

Finally, the way to get both diff and stat for merge commits is... who'd guess, adding -p to the command, and that provided -p is already supposedly implied by -c (!):

  git log -n1 --stat -c -p <merge_commit>

In particular, this means that contrary to documentation, -c does not imply -p in the common sense of the word "imply", and interdependencies between all these options are already too complex to easily grok for a human being.

As for newer --diff-merges, they behave similar to -c here that seems reasonable. Overall, I still don't see any breakage introduced by --diff-merges, and it seems to behave according to its documentation, so shouldn't break any expectations either.

Getting back to the original question of letting -m imply -p, it shouldn't behave differently than -c/-cc, that do imply -p, so I don't see any significant problem that'd be added to the current status.

Right now the following two give exactly the same output:

  git log -n1 --stat -c <merge_commit>
  git log -n1 --stat -m <merge_commit>

the stat to the first parent, and it shouldn't change if we let -m "imply" -p the same way -c "implies" -p, whatever it actually means.

Best Regards,

-- Sergey Organov