Re: [PATCH v2] range-diff: optionally include merge commits' diffs in the analysis

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 11, 2024, 19:55 UTC
Message-ID: <c7c71d10-5795-6c93-6ff4-1effc534c6a0@gmx.de>
In-Reply-To: <CABPp-BGeSwDHNeDdEwf+mo5q33GO3f0UiFpVOpjiLF3msEvrfg@mail.gmail.com>

Hi Elijah,

On Fri, 8 Nov 2024, Elijah Newren wrote:

Show 24 quoted lines

> On Fri, Nov 8, 2024 at 5:43 AM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
> > index fbdbe0befeb..17a85957877 100644
> > --- a/Documentation/git-range-diff.txt
> > +++ b/Documentation/git-range-diff.txt
> > @@ -81,6 +81,15 @@ to revert to color all lines according to the outer diff markers
> >         Suppress commits that are missing from the second specified range
> >         (or the "right range" when using the `<rev1>...<rev2>` format).
> >
> > +--diff-merges=<format>::
> > +       Instead of ignoring merge commits, generate diffs for them using the
> > +       corresponding `--diff-merges=<format>` option of linkgit:git-log[1],
> > +       and include them in the comparison.
> > ++
> > +Note: In the common case, the `first-parent` mode will be the most natural one
> > +to use,
>
> I think we need more wording around "common case"; I believe this
> "common case" is when you are diffing against a merely transplanted
> series of commits (a series created by rebasing without inserting,
> removing, or minimally modifying those commits in the process) that
> `first-parent` makes sense.  And it only makes sense in that case.

I tried to reproduce some of the common cases (which were more along the lines of stacked PRs than transplanted series of commits), and you're right: with the remerge bug fix, the range-diffs are much more easily interpreted.

For example, I had to squash in a few fixes in https://github.com/git/git-scm.com/pull/1915, and I try to combine all of my currently-open git-scm.com PRs into the `gh-pages` branch in my fork.

With `--diff-merges=1`, all of those single-commit merges that update the various translations of the book had diffs identical to the diffs of the commits they merged. Which confused range-diff quite a bit, often picking a non-merge as matching a merge (which is of course suboptimal).

With `--diff-merges=remerge`, these trivial merges had no diffs at all, which made the review much easier.

Show 7 quoted lines

> I think `remerge-diff` generally makes sense here, both in the cases
> when `first-parent` makes sense and when `first-parent` does not.  It
> could be improved by suppressing the inclusion of short commit ids
> (and maybe also commit messages) in the labels of conflict markers.  I
> suspect that issue might make `remerge-diff` less useful than
> `first-parent` in simple common cases currently, but I think it's the
> right thing to build upon for what you are trying to view.

Interesting idea about the suppressed commit IDs. I'll play with that.

Ciao, Johannes