From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 18 Sep 2025 05:49:31 GMT
Subject: Re: [PATCH v2] combine-diff: don't override recursive flag in diff_tree_combined()
Message-ID: <aMuda6sqEiwxxAgp@pks.im>
In-Reply-To: <87ecs6o21e.fsf@iotcl.com>

On Tue, Sep 16, 2025 at 05:22:21PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > Do we know to set `diffopt.flags.recursive` in that case already,
> > or do we handle recursion manually in that case?
> 
> git-last-modified sets that flag when `-r` or `--recursive` is given.
> That flag is passed down to diff_tree_combined(), which reuses that
> value (and no longer overrided it). Does that answer your question?

Yup, it does.

> >> diff --git a/combine-diff.c b/combine-diff.c
> >> index 3878faabe7bb2f7c80cffbf3add6123f17960627..305414efdf436d53fee8d79aa4219f6a4dd3445e 100644
> >> --- a/combine-diff.c
> >> +++ b/combine-diff.c
> >> @@ -1515,7 +1515,6 @@ void diff_tree_combined(const struct object_id *oid,
> >>  
> >>  	diffopts = *opt;
> >>  	copy_pathspec(&diffopts.pathspec, &opt->pathspec);
> >> -	diffopts.flags.recursive = 1;
> >>  	diffopts.flags.allow_external = 0;
> >>  
> >>  	/* find set of paths that everybody touches
> >
> > With the above I'm a bit worried that we're changing behaviour for
> > direct or indirect callers without noticing. Our tests may not detect
> > any regressions, but that doesn't really prove that there is none.
> 
> As I've been trying to explain in the first version[1] of my patch, I
> would say my change fixes a bug. Agreed, it's an ancient old bug, I
> still consider it a bug. I think in practice no one will notice this
> change in behaviour, because otherwise they would have complained about
> the buggy behaviour.
> 
> For the callsites we didn't address, I'm doubtful anyone will notice a
> change. Also, as I'm laying out above, we only see a bug in rare edge
> cases. That's also why we don't see any test failures.
> 
> So yes, there might be a change in behaviour, but it will be correct and
> barely anyone will notice.
> 
> Unfortunately I don't have any data to prove this.

The old code certainly feels fishy, that much I can say. Other than that
I'm not knowledgeable enough in this code area to have an informed
opinion. The old mailing list thread [1] doesn't surface any useful
info, either.

Patrick

[1]: https://lore.kernel.org/git/7vwtgqas0y.fsf@assigned-by-dhcp.cox.net/