Re: [PATCH 0/5] diff-merges: more features

[Sergey Organov <sorganov@gmail.com>]

Glen Choo <chooglen@google.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> The last time '-m' issue appeared on the list, it all started here:
>>
>> https://lore.kernel.org/git/CAMMLpeR-W35Qq6a343ifrxJ=mwBc_VcXZtVrBYDpJTySNBroFw@mail.gmail.com/
>>
>> In particular, the final patch and its revert is deeper down this tread:
>>
>> https://lore.kernel.org/git/20210520214703.27323-11-sorganov@gmail.com/#t
>>
>> and
>>
>> https://lore.kernel.org/git/YQyUM2uZdFBX8G0r@google.com/
>
> Thanks, these provide extremely helpful context :) In particular:
>
> - Junio describes this "do nothing unless -p" is given behavior as an
>   accident [1].

I rather read it as Junio saying that "-m does not imply -p" is
historical accident, and yes, it is, provided '-c/--cc' were fixed at
some point, and '-m' was not, so in fact I figure Junio meant: "-m
differs from -c/--cc in not implying '-p'" is historical accident. And
then he suggests to leave it as is, with which I disagree.

In addition, in all the discussions, I believe Junio at least once said
he does see valid usages for current hush '-m':

<quote>
But "stash list" example shows that "log --first-parent -m" without
"-p" in a script has a valid reason, and a change that hurts those
who correctly used a command and an option in a way they were
intended to do _is_ problematic.
</quote>

here:

https://lore.kernel.org/git/xmqqy29chim6.fsf@gitster.g/

> - Jonathan Nieder notes that this change accidentally broke scripts
>   where "-m" probably wasn't doing anything useful, but we wanted to
>   avoid breaking the scripts for backwards compatibility anyway [2].
>
> I got the sense that the closest thing to an intentional use case of
> "-m" is for users who thought that "-m" would affect path limiting [3],
> although it doesn't actually do that.

I don't think so. Dunno why you got such feeling. It's rather that for
some time "--first-parent -m" was the only way to produce *most* useful
form of "-m" format: show diff with respect to the *first* parent only,
whereas without "--first-parent" "-m" produced diff output for *every*
parent in turn(!) giving extremely confusing result. Please notice how
--first-parent appears in most of those discussions.

Overall, the (simplified) history of '-m' goes like this, as far as I
can tell:

0. Original '-m', documented only for 'diff-tree'. The diffs were
   produced to all the parents that was probably very logical for
   plumbing 'diff-tree', as it reflects symmetric nature of merge
   commits in the DAG, that is the core of git data model.

   However, while "git log -p" does not produce patches for merge
   commits (apparently to get rid of often large output), '-m' in fact
   enforces the output for merge commits, in the format produced by
   'diff-tree -m', i.e., diffs to all the parents in turn.

   [The latter was probably the first mistake, it should have rather
   produced the diff with respect to the first parent that is more
   suitable for "git log" being porcelain, to show changes introduced by
   the commit to the mainline, exactly as for non-merge commits]

1. '-c' is introduced, then '--cc' is introduced, with semantics similar
   to '-m' with respect to '-p', but different kinds of output.

   At this point we have consistent behavior of '-m', '-c', and '--cc'
   with respect to '-p', none of which produce any output unless '-p' is
   specified as well.

2. '-c' and '--cc' are changed to imply '-p'[0]. '-m' is left alone,
   supposedly forgotten as being undocumented for "git log" and of
   limited use, due to its large and surprising output.

   [I think that was the second mistake, forgetting to change '-m'
   accordingly]

3. '-m' is changed to produce diff with respect to the first parent only
   when '--first-parent' is specified [1]. '-m' finally starts to
   (sometimes) give useful output, and starts to be actually used,
   but only together with '--first-parent' most of times.

   BTW, this is the first time '-m' has been documented as part of "git
   log": "This patch properly documents the -m switch, which has so far
   been mentioned only as a fairly special diff-tree flag."

   [I think there are more mistakes here: not changing '-m' to imply
   '-p' at this point, and not producing single-parent diff even without
   '--first-parent']

4. "--first-parent" is suggested to imply "-m"(!) to let "--first-parent
   -p" to produce diff for merge commits[2]. That in turn needed an
   option that will negate implied "-m", and that's where
   --[no-]diff-merges was suggested.

   Please notice that if '-m' implied '-p' (as it should) at this point,
   there should be little needed for these patches, as just saying "git
   log --first-parent -m" would produce required result. So, mistakes
   above caused a need to fix them.

5. At this point, in the referenced discussion I suggested
   '--diff-merges=on|off' instead of '--[no-]diff-merges', to allow for
   further extensions.

6. '--diff-merges=' option actually born to provide some missing
   functionality and to get rid of inconsistencies[3].

7. '-m' format becomes configurable using new "log.diffMerges"
   configuration[4] so that we can make it conveniently useful even
   without "--first-parent". This was immediately implemented in
   generalized manner to allow to configure '-m' to produce not only
   single-parent diff, but any supported format.

8. As a reply to yet another request on the mailing list "why '-m'
   produces no output", I tried "-m imply -p" patch series[5], which
   were accepted, but then the last patch only(!) from the series, that
   actually introduced required behavior, has been reverted.

   This left me feel I got some unfinished job to do.

9. These patch series, trying "-m imply -p" again, now more carefully.

> So what I've reads so far suggests that "do nothing unless -p" (aka
> --diff-merges=hide) is not actually useful, and we should just drop
> it.

Again, I've tried exactly this before, and that was first accepted, and
then reverted, that's why --diff-merges=hide has been introduced in
these series, to address the issues raised during revert request.

References:

[0]: Showing merges easier with "git log":

https://lore.kernel.org/git/1440110591-12941-1-git-send-email-gitster@pobox.com/

[1]: git log -p -m: Document, honor --first-parent

https://lore.kernel.org/git/20100210011149.GR9553@machine.or.cz/

[2]: making --first-parent imply -m:

https://lore.kernel.org/git/20200728163617.GA2649887@coredump.intra.peff.net/

[3]: git-log: implement new --diff-merge options:

https://lore.kernel.org/git/20201221152000.13134-1-sorganov@gmail.com/

[4]: git log: configurable default format for merge diffs

https://lore.kernel.org/git/20210413114118.25693-1-sorganov@gmail.com/

[5]: diff-merges: let -m imply -p

https://lore.kernel.org/git/20210520214703.27323-1-sorganov@gmail.com/

Thanks,
-- Sergey Organov