Subject: Re: [PATCH v2 29/33] doc/git-log: describe new --diff-merges options

From: Sergey Organov <[email-masked]>
Date: Sat, 19 Dec 2020 01:11:28 +0300

Elijah Newren <[email-masked]> writes:

> On Fri, Dec 18, 2020 at 8:05 AM Sergey Organov <[email-masked]> wrote:
>>
>> Elijah Newren <[email-masked]> writes:
>>
>> > On Wed, Dec 16, 2020 at 10:50 AM Sergey Organov <[email-masked]> wrote:
>> >>
>> >> Describe all the new --diff-merges options in the git-log.txt and
>> >> adopt description of originals accordingly.
>> >
>> > You also took care to explain interactions of options with -p that
>> > were previously undocumented, which is a nice bonus.  That wording
>> > could still be improved a bit, though, as noted below.
>> >
>> >> Signed-off-by: Sergey Organov <[email-masked]>
>> >> ---
>> >>  Documentation/git-log.txt | 85 ++++++++++++++++++++++++---------------
>> >>  1 file changed, 52 insertions(+), 33 deletions(-)
>> >>
>> >> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
>> >> index 2b8ac5ff882a..27bc619490c6 100644
>> >> --- a/Documentation/git-log.txt
>> >> +++ b/Documentation/git-log.txt
>> >> @@ -120,45 +120,64 @@ DIFF FORMATTING
>> >>  By default, `git log` does not generate any diff output. The options
>> >>  below can be used to show the changes made by each commit.
>> >>
>> >> -Note that unless one of `-c`, `--cc`, or `-m` is given, merge commits
>> >> -will never show a diff, even if a diff format like `--patch` is
>> >> -selected, nor will they match search options like `-S`. The exception is
>> >> -when `--first-parent` is in use, in which merges are treated like normal
>> >> -single-parent commits (this can be overridden by providing a
>> >> -combined-diff option or with `--no-diff-merges`).
>> >> +Note that unless one of `--diff-merges` variants (including short
>> >> +`-m`, `-c`, and `--cc` options) is explicitly given, merge commits
>> >> +will not show a diff, even if a diff format like `--patch` is
>> >> +selected, nor will they match search options like `-S`. The exception
>> >> +is when `--first-parent` is in use, in which case `first-parent` is
>> >> +the default format.
>> >
>> > Thanks for fixing this up.  :-)
>>
>> Don't mention it :-)
>>
>> >
>> >>
>> >> --c::
>> >> -       With this option, diff output for a merge commit
>> >> -       shows the differences from each of the parents to the merge result
>> >> -       simultaneously instead of showing pairwise diff between a parent
>> >> -       and the result one at a time. Furthermore, it lists only files
>> >> -       which were modified from all parents.
>> >> -
>> >> ---cc::
>> >> -       This flag implies the `-c` option and further compresses the
>> >> -       patch output by omitting uninteresting hunks whose contents in
>> >> -       the parents have only two variants and the merge result picks
>> >> -       one of them without modification.
>> >> +--diff-merges=(off|none|first-parent|1|separate|m|combined|c|dense-combined|cc)::
>> >> +--no-diff-merges::
>> >> +       Specify diff format to be used for merge commits. Default is
>> >> +       `off` unless `--first-parent` is in use, in which case
>> >> +       `first-parent` is the default.
>> >> ++
>> >> +--diff-merges=(off|none):::
>> >> +--no-diff-merges:::
>> >> +       Disable output of diffs for merge commits. Useful to override
>> >> +       implied value.
>> >> ++
>> >> +--diff-merges=first-parent:::
>> >> +--diff-merges=1:::
>> >> +       This option makes merge commits show the full diff with
>> >> +       respect to the first parent only.
>> >
>> > Does it imply -p?
>>
>> No, none of --diff-merges options do. This one is not any special. Why
>> the question?
>
> The documentation on -m was vague enough that it made me wonder.
> Fixing it would probably prevent me from having asked this question.

OK, I see, so let's try to fix the cause, please see below.

>
>> >> ++
>> >> +--diff-merges=separate:::
>> >> +--diff-merges=m:::
>> >> +-m:::
>> >> +       This makes merge commits show the full diff with respect to
>> >> +       each of the parents. Separate log entry and diff is generated
>> >> +       for each parent. `-m` is different in that it doesn't produce
>> >> +       any output without `-p`.
>> >
>> > Different from what?  From --first-parent?  From flags that haven't
>> > been covered yet?  (-c and --cc show up below)
>>
>> Well, from --diff-merges=m and --diff-merges=separate, that, as any
>> other --diff-merge option, do produce output (for merge commits) even
>> without -p.
>
> That wasn't at all clear to me as the intent of the last sentence.

Well, actually, the right answer to your question is: -m is different
from *everything* ;-)

Maybe the confusion you got is not caused by the documentation, but
rather by some of your expectations? In particular, I wonder, how comes
--first-parent appeared in your question, that is entirely unrelated?

I mean I'm still unsure how to make this description more clear, maybe
this will do:

--diff-merges=separate:::
--diff-merges=m:::
-m:::
       This makes merge commits show the full diff with respect to
       each of the parents. Separate log entry and diff is generated
       for each parent. `-m` doesn't produce any output without `-p`.

>
>> >> ++
>> >> +--diff-merges=combined:::
>> >> +--diff-merges=c:::
>> >> +-c:::
>> >> +       With this option, diff output for a merge commit shows the
>> >> +       differences from each of the parents to the merge result
>> >> +       simultaneously instead of showing pairwise diff between a
>> >> +       parent and the result one at a time. Furthermore, it lists
>> >> +       only files which were modified from all parents. Historically,
>> >> +       `-c` enables diff output for non-merge commits as well.
>> >
>> > "Historically"?  Does that mean it doesn't anymore?
>>
>> Eh, I don't think "historically" means that, but I'm not sure, being
>> non-native English speaker.
>
> Sometimes non-native speakers use the language more accurately.  This
> might be such a case, but that sentence did make me think you might be
> attempting to document past behavior as a way of helping people adjust
> to current/new behavior.
>
>> > (Maybe, "The short form, `-c`, also enables diff output for non-merge
>> > commits as well." or something like that?)
>>
>> ... and then try to explain why this otherwise illogical behavior is
>> there? I thought "historically" would cover that.
>
> It doesn't seem like illogical behavior to me.   Perhaps the view of
> -c as a diff-merges option, reinforced by placing -c right next to
> --diff-merges in the documentation, is what causes you to think so?

-c, described as selecting specific format of representation of merge
commits, suddenly enabling diff output of non-merge commits, is logical?
No. Useful? Maybe. Logical? Not to me. Just saying, see below.

> If so, maybe we should document -c (and --cc) separately after all the
> --diff-merges options, using something like the following:
>
> -c:::
>     Implies both --patch and --diff-merges=combined, i.e. turn on
> patches for normal commits and show a combined diff format for merges.

The problem with this is that it'd then be preferable to describe -m
separately as well, and that's not that simple.

Overall, I'd rather change that in place, like this:

--diff-merges=combined:::
--diff-merges=c:::
-c:::
       With this option, diff output for a merge commit shows the
       differences from each of the parents to the merge result
       simultaneously instead of showing pairwise diff between a
       parent and the result one at a time. Furthermore, it lists
       only files which were modified from all parents. `-c` implies
       `--patch`.

Thanks,
-- Sergey