On Tue, Dec 21, 2021 at 5:06 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > > On Tue, Dec 21 2021, Elijah Newren wrote: > > > On Tue, Dec 21, 2021 at 1:29 PM Ævar Arnfjörð Bjarmason > > <avarab@xxxxxxxxx> wrote: > >> > >> > >> On Tue, Dec 21 2021, Elijah Newren via GitGitGadget wrote: > >> > >> > From: Elijah Newren <newren@xxxxxxxxx> > >> > > >> > Signed-off-by: Elijah Newren <newren@xxxxxxxxx> > >> > --- > >> > 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.
