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

[Elijah Newren <newren@gmail.com>]

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.

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

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

> >
> >> ++
> >> +--diff-merges=dense-combined:::
> >> +--diff-merges=cc:::
> >> +--cc:::
> >> +       With this option the output produced by
> >> +       `--diff-merges=combined` is further compressed by omitting
> >> +       uninteresting hunks whose contents in the parents have only
> >> +       two variants and the merge result picks one of them without
> >> +       modification.  Historically, `--c` enables diff output for
> >> +       non-merge commits as well.
> >
> > Same note as above.
>
> Yep.
>
> >
> >>  --combined-all-paths::
> >>         This flag causes combined diffs (used for merge commits) to
> >>         list the name of the file from all parents.  It thus only has
> >> -       effect when -c or --cc are specified, and is likely only
> >> -       useful if filename changes are detected (i.e. when either
> >> -       rename or copy detection have been requested).
> >> +       effect when `--diff-merges=[dense-]combined` is in use, and
> >> +       is likely only useful if filename changes are detected (i.e.
> >> +       when either rename or copy detection have been requested).
> >>
> >> --m::
> >> -       This flag makes the merge commits show the full diff like
> >> -       regular commits; for each merge parent, a separate log entry
> >> -       and diff is generated. An exception is that only diff against
> >> -       the first parent is shown when `--first-parent` option is given;
> >> -       in that case, the output represents the changes the merge
> >> -       brought _into_ the then-current branch.
> >> -
> >> ---diff-merges=off::
> >> ---no-diff-merges::
> >> -       Disable output of diffs for merge commits (default). Useful to
> >> -       override `-m`, `-c`, or `--cc`.
> >>
> >>  :git-log: 1
> >>  include::diff-options.txt[]
> >> --
> >> 2.25.1
> >
> > The rest looks good.
>
> Thanks,
> -- Sergey