Re: [PATCH 9/9] doc/diff-options: explain the new --remerge-diff option

From: Elijah Newren <newren@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>, Jeff King <peff@peff.net>,
	Jonathan Nieder <jrnieder@gmail.com>,
	Sergey Organov <sorganov@gmail.com>,
	Bagas Sanjaya <bagasdotme@gmail.com>,
	Neeraj Singh <nksingh85@gmail.com>
Subject: Re: [PATCH 9/9] doc/diff-options: explain the new --remerge-diff option
Date: Wed, 22 Dec 2021 11:05:36 -0800	[thread overview]
Message-ID: <CABPp-BFmMZUeJaOmyLHPR20XCKF=bj8ZYfZAw13vRLUNJy80-Q@mail.gmail.com> (raw)
In-Reply-To: <211222.86mtktphxl.gmgdl@evledraar.gmail.com>

On Tue, Dec 21, 2021 at 5:06 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Tue, Dec 21 2021, Elijah Newren wrote:
>
> > On Tue, Dec 21, 2021 at 1:29 PM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >>
> >>
> >> On Tue, Dec 21 2021, Elijah Newren via GitGitGadget wrote:
> >>
> >> > From: Elijah Newren <newren@gmail.com>
> >> >
> >> > Signed-off-by: Elijah Newren <newren@gmail.com>
> >> > ---
> >> >  Documentation/diff-options.txt | 8 ++++++++
> >> >  1 file changed, 8 insertions(+)
> >> >
> >> > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> >> > index c89d530d3d1..b05f1c9f1c9 100644
> >> > --- a/Documentation/diff-options.txt
> >> > +++ b/Documentation/diff-options.txt
> >> > @@ -64,6 +64,14 @@ ifdef::git-log[]
> >> >       each of the parents. Separate log entry and diff is generated
> >> >       for each parent.
> >> >  +
> >> > +--diff-merges=remerge:::
> >> > +--diff-merges=r:::
> >> > +--remerge-diff:::
> >> > +     With this option, two-parent merge commits are remerged to
> >> > +     create a temporary tree object -- potentially containing files
> >> > +     with conflict markers and such.  A diff is then shown between
> >> > +     that temporary tree and the actual merge commit.
> >> > ++
> >> >  --diff-merges=combined:::
> >> >  --diff-merges=c:::
> >> >  -c:::
> >>
> >> This & 5/9 would I think be better squashed into their respective "main"
> >> patches.
> >
> > I presume you mean the "main" patch for this one is 8/9.  I was trying
> > to find a way to break up that large patch, but this is pretty small
> > so...sure I'll squash it in.
> >
> > What are you referring to as the "main" patch for 5/9, though?  It
> > only seems related to 6/9 and 7/9 to me, but I very deliberately split
> > those patches off and don't want to confuse them with unrelated
> > changes.  I disagree with combining 5/9 with either of those.
>
> I just gave it a quick initial skim.
>
> I have sometimes found it a bit harder to review your patches due to
> over-splitting.
>
> E.g. (went back and looked) here tmp_objdir_discard_objects() is
> introduced in 1/9 but used in 8/9. "path_messages" is then introduced in
> 5/9 and used in 8/9, no?
>
> Anyway, just a bit of feedback. FWIW not just bikeshedding. I do find
> myself stopping at 1/9, paging to 2/9, searching for the function, not
> there, checking 3/9 etc.
>
> I realize this is a bit of a stones & glass houses comment, but I find
> it a bit easier to review things when a patch is larger v.s. having it
> split up in a way where preceding steps don't do anything yet except
> wait for use by a subsequent patch.
>
> 0.02 etc.

Oh, 8/9.  That one could make sense.

And thanks for the feedback.  Perhaps I could restructure this series
with a top-down design instead of bottom-up.  Doing that would mean
either adding functions with an instant-die() implementation in the
first step or just leaving a placeholder comment, and then filling
those things in for later steps.