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

[Elijah Newren <newren@gmail.com>]

On Fri, Dec 18, 2020 at 2:11 PM Sergey Organov <sorganov@gmail.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > On Fri, Dec 18, 2020 at 8:05 AM Sergey Organov <sorganov@gmail.com> wrote:
> >>
> >> Elijah Newren <newren@gmail.com> writes:
> >>
> >> > On Wed, Dec 16, 2020 at 10:50 AM Sergey Organov <sorganov@gmail.com> 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 <sorganov@gmail.com>
> >> >> ---
> >> >>  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.

Sounds good.

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

When you said "`-m` is different" it was not clear what you were
contrasting to.  The fact that you were contrasting to the other
values in the set of three being described by this paragraph may seem
obvious to you, and I suspect many other readers may catch on to that,
but that really honestly did not occur to me while reading it.  Maybe
I'm weird.  Maybe some subset of users will be like me and we should
tighten up the wording.  Hard to say.

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

Works for me.  :-)

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

:-)

I didn't say the former documentation was correct, or that the
behavior logically flowed from the documentation's claims.  And you do
bring up good points that the past documentation should have been
updated when -c and --cc were made to imply -p.  I only claimed that
-c and --cc provide logical behaviors (I want diffs for everything,
for merges do it this way...all expressed in a nice compact bundle).

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

Works for me.  --cc needs a similar change, obviously.