Re: [PATCH 3/4] replace-objects: evaluate replacement refs without using the object store
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
> >
> > 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/3afabef75c627b8943bcae86837abc7c32fe 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
Re: [PATCH 3/4] replace-objects: evaluate replacement refs without using the object store
On 09/12/2017 07:31 PM, Jonathan Nieder wrote: > From: Stefan Beller > > 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/3afabef75c627b8943bcae86837abc7c32fe 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`). Michael
[PATCH 3/4] replace-objects: evaluate replacement refs without using the object store
From: Stefan Beller 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/3afabef75c627b8943bcae86837abc7c32fe 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. Signed-off-by: Stefan Beller Signed-off-by: Jonathan Nieder --- refs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs.c b/refs.c index b0106b8162..cd84ed9710 100644 --- a/refs.c +++ b/refs.c @@ -1394,7 +1394,7 @@ int for_each_replace_ref(each_ref_fn fn, void *cb_data) return do_for_each_ref(get_main_ref_store(), git_replace_ref_base, fn, strlen(git_replace_ref_base), - 0, cb_data); + DO_FOR_EACH_INCLUDE_BROKEN, cb_data); } int for_each_namespaced_ref(each_ref_fn fn, void *cb_data) -- 2.14.1.690.gbb1197296e
