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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Elijah Newren <newren@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 00:47:29 +0100	[thread overview]
Message-ID: <211222.86mtktphxl.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <CABPp-BHDJMecupic=ybVhzZbRC5=WmY616JQdbfaqw_1A5sMHA@mail.gmail.com>

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.