Re: [PATCH 3/4] replace-objects: evaluate replacement refs without using the object store

[Jeff King <peff@peff.net>]

On Wed, Sep 13, 2017 at 10:03:58AM +0200, Michael Haggerty wrote:

> On 09/12/2017 07:31 PM, Jonathan Nieder wrote:
> > From: Stefan Beller <sbeller@google.com>
> > 
> > Pass DO_FOR_EACH_INCLUDE_BROKEN when iterating over replacement refs
> > so that the iteration does not require opening the named objects from
> > the object store. This avoids a dependency cycle between object access
> > and replace ref iteration.
> > 
> > Moreover the ref subsystem has not been migrated yet to access the
> > object store via passed in repository objects.  As a result, without
> > this patch, iterating over replace refs in a repository other than
> > the_repository it produces errors:
> > 
> >    error: refs/replace/3afabef75c627b894cccc3bcae86837abc7c32fe does not point to a valid object!
> > 
> > Noticed while adapting the object store (and in particular its
> > evaluation of replace refs) to handle arbitrary repositories.
> 
> Have you checked that consumers of this API can handle broken
> references? Aside from missing values, broken references can have
> illegal names (though hopefully not unsafe in the sense of causing
> filesystem traversal outside of `$GITDIR`).

It looks like there are only two callers. One complains about
names that aren't 40-hex. The other ("git replace -l") parses the 40-hex
in "long" mode, but will print anything in short mode (not that printing
is very dangerous).

I do have to wonder if for_each_replace_ref() should just do the 40-hex
syntactic check itself (and issue a warning for other refs). It seems
like that would be the point of calling for_each_replace_ref() instead
of just for_each_ref_in("refs/replace") yourself, and both of the
callers would benefit.

-Peff