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