Re: [PATCH v3 2/7] refs: teach arbitrary repo support to iterators

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 8, 2021, 20:12 UTC
Message-ID: <20211008201235.1324349-1-jonathantanmy@google.com>
In-Reply-To: <kl6ly274y6l6.fsf@chooglen-macbookpro.roam.corp.google.com>

Show 18 quoted lines

> > @@ -1139,7 +1142,7 @@ static int should_pack_ref(const char *refname,
> >  		return 0;
> >  
> >  	/* Do not pack broken refs: */
> > -	if (!ref_resolves_to_object(refname, oid, ref_flags))
> > +	if (!ref_resolves_to_object(refname, the_repository, oid, ref_flags))
> >  		return 0;
> 
> Nit: It would be nice to have a NEEDSWORK the explain why the_repository is
> hardcoded instead of the commit message. Without having something in the
> code, it is a little surprising to see the_repository when we are so
> close to removing the_repository from refs/ altogether.
> 
> This in the spirit of your check in patch 3, where we explicitly warn
> readers when non-the_repository is not supported.
> 
>   +	if (iter->repo != the_repository)
>   +		BUG("peeling for non-the_repository is not supported");

This doesn't bring us closer or further from removing the_repository from refs/ - originally, this call to ref_resolves_to_object() implicitly referenced the_repository, and now it explicitly does, so there's no increase or decrease in usage of the_repository. I think that the true NEEDSWORK is that the entire ref writing system operates only on the_repository, and this is probably not the place to note it.