Re: [PATCH 1/2] connected: allow supplying different view of reachable objects

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 31, 2022, 13:10 UTC
Message-ID: <Y1/JOGihHtMWv240@ncase>
In-Reply-To: <xmqqr0yrizqm.fsf@gitster.g>

On Fri, Oct 28, 2022 at 11:12:33AM -0700, Junio C Hamano wrote:

Show 32 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > diff --git a/connected.c b/connected.c
> > index 74a20cb32e..2a4c4e0025 100644
> > --- a/connected.c
> > +++ b/connected.c
> > @@ -98,7 +98,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
> >  	strvec_push(&rev_list.args, "--stdin");
> >  	if (has_promisor_remote())
> >  		strvec_push(&rev_list.args, "--exclude-promisor-objects");
> > -	if (!opt->is_deepening_fetch) {
> > +	if (!opt->is_deepening_fetch && !opt->reachable_oids_fn) {
> >  		strvec_push(&rev_list.args, "--not");
> >  		strvec_push(&rev_list.args, "--all");
> >  	}
> > @@ -125,6 +125,13 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
> >  
> >  	rev_list_in = xfdopen(rev_list.in, "w");
> >  
> > +	if (opt->reachable_oids_fn) {
> > +		const struct object_id *reachable_oid;
> > +		while ((reachable_oid = opt->reachable_oids_fn(opt->reachable_oids_data)) != NULL)
> > +			if (fprintf(rev_list_in, "^%s\n", oid_to_hex(reachable_oid)) < 0)
> > +				break;
> > +	}
> 
> It is good that these individual negative references are fed from
> the standard input, not on the command line, as they can be many.
> 
> In the original code without the reachable_oids_fn, we refrain from
> excluding when the is_deepening_fetch bit is set, but here we do not
> pay attention to the bit at all.  Is that sensible, and if so why?

Hm, good point. On a deepening fetch the commits that were the previous boundary will likely get replaced by new commits that are at a deeper point in history, so they cannot be used as a well-defined boundary. Instead, we do a complete graph-walk that doesn't stop at any previously known commits at all. At least that's how I understand the code, the explanation is likely a bit fuzzy.

I guess we should thus also pay attention to `is_deepening_fetch` here. As this means that `is_deepening_fetch` and `reachable_oids_fn` are mutually exclusive I'm inclined to go even further and `die()` if both are set at the same time. We only adapt git-receive-pack(1) anyway, so we should never run into this situation for now.

Patrick