Re: [PATCH] upload-pack: disable commit graph more gently for shallow traversal

From: Jeff King <peff@peff.net>
Date: Sep 12, 2019, 13:59 UTC
Message-ID: <20190912135952.GB23031@sigill.intra.peff.net>
In-Reply-To: <20190912020748.GA76228@syl.lan>

On Wed, Sep 11, 2019 at 10:07:48PM -0400, Taylor Blau wrote:

Show 10 quoted lines

> > The new test in t5500 triggers this segfault, but see the comments there
> > for how horribly intimate it has to be with how both upload-pack and
> > commit graphs work.
> 
> Thanks for the comment, too. I agree that protocol-level hacking is
> somewhat smelly, at least as far as t5500 is concerned, but for this
> particular case it seems the only sensible option.
> 
> I'm still left scratching my (sore) head about how someone triggered
> this in production, but maybe that's a riddle for another day.

I'll admit that part of my motivation was my inability to generate a working case just using Git commands (so my justification is that if it's so hard to do, then the test is inherently fragile, but you can also just accuse me of being lazy).

I think the key is something to do with the shape of history related to the requests, such that we walk over a commit during the have negotiation, but then also need to walk over it during the deepen-since. So maybe I am just missing something obvious, or maybe it just needs to be a deeper history. I do like that the case I showed is the minimal history, at least.

Show 10 quoted lines

> > +# A few subtle things about the request in this test:
> > +#
> > +#  - the server must have commit-graphs present and enabled
> 
> I think "enabled" may be somewhat redundant, at least since some recent
> changes to enable this by default. (As an aside, I tried to dig up the
> patches that Stolee sent to actually enable this and back up my claim,
> but I couldn't find them on 'master'. I'm not sure if that's my poor use
> of 'git log', or misremembering the fact that these were enabled by
> default.)

Yeah, it is redundant these days. I figured this might go to 'maint', though, and 31b1de6a09 (commit-graph: turn on commit-graph by default, 2019-08-13) is only in master.

> > +#    latter will try to lod it lazily.
> 
> s/lod/load

Thanks, fixed.

Show 10 quoted lines

> > +#
> > +#  - we must use protocol v2, because it handles the "have" negotiation before
> > +#    processing the shallow direectives
> > +#
> > +test_expect_success 'shallow since with commit graph and already-seen commit' '
> > +	test_create_repo shallow-since-graph &&
> > +	(
> 
> I'm not sure if this same-level indentation is common, or you're missing
> an extra tab here. Either way.

It's not common, but I was matching the surrounding tests for style (and use of a separate repo, and non-use of test_config).

Show 15 quoted lines

> > diff --git a/upload-pack.c b/upload-pack.c
> > index 222cd3ad89..a260b6b50d 100644
> > --- a/upload-pack.c
> > +++ b/upload-pack.c
> > @@ -722,7 +722,7 @@ static void deepen_by_rev_list(struct packet_writer *writer, int ac,
> >  {
> >  	struct commit_list *result;
> >
> > -	close_commit_graph(the_repository->objects);
> > +	disable_commit_graph();
> 
> This line made me think to check if we could remove 'close_commit_graph'
> all together, but there is a remaining callsite in packfile.c, and
> closing the commit-graph _is_ the right thing to do there, so I think we
> ought to keep it around.

Yeah, it's sort of inherently dangerous, as it doesn't scrub any in-memory commit structs that are in this "have a graph_pos but not maybe_tree" state.

The call in close_object_store() is probably fine, though, as the whole point there is getting rid of everything related to the object store. And since 14ba97f81c (alloc: allow arbitrary repositories for alloc functions, 2018-05-15) or thereabouts, that includes the object structs.

There's another call in write_commit_graph_file(), right before we rename a new file into place. This generally happens in a separate "commit-graph write" process, so I think it's OK. But it could eventually happen as part of another process, which would maybe be a problem. I'm actually not convinced the in-memory one needs to be closed here, but maybe it's a Windows thing (closing the file before renaming over it)?

-Peff