git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, "Derrick Stolee" <dstolee@microsoft.com>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Subject: Re: [PATCH] upload-pack: disable commit graph more gently for shallow traversal
Date: Thu, 12 Sep 2019 09:59:52 -0400	[thread overview]
Message-ID: <20190912135952.GB23031@sigill.intra.peff.net> (raw)
In-Reply-To: <20190912020748.GA76228@syl.lan>

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

> > 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.

> > +# 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.

> > +#
> > +#  - 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).

> > 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

  parent reply	other threads:[~2019-09-12 13:59 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-12  0:04 [PATCH] upload-pack: disable commit graph more gently for shallow traversal Jeff King
2019-09-12  0:18 ` Jeff King
2019-09-12  1:11   ` [PATCH] list-objects: don't queue root trees unless revs->tree_objects is set Jeff King
2019-09-12  1:19     ` Jeff King
2019-09-12 12:31     ` Derrick Stolee
2019-09-12 16:52     ` Junio C Hamano
2019-09-12 22:34       ` Jeff King
2019-09-13 18:05         ` Junio C Hamano
2019-09-12  2:08   ` [PATCH] upload-pack: disable commit graph more gently for shallow traversal Taylor Blau
2019-09-12 14:03     ` Jeff King
2019-09-12  2:07 ` Taylor Blau
2019-09-12 11:06   ` SZEDER Gábor
2019-09-12 14:01     ` Jeff King
2019-09-12 12:46   ` Derrick Stolee
2019-09-12 13:59   ` Jeff King [this message]
2019-09-12 12:23 ` Derrick Stolee
2019-09-12 14:23   ` Jeff King
2019-09-12 19:27     ` Derrick Stolee
2019-09-12 14:41 ` [PATCH v2] upload-pack commit graph segfault fix Jeff King
2019-09-12 14:43   ` Jeff King
2019-09-12 14:44   ` [PATCH v2 1/2] commit-graph: bump DIE_ON_LOAD check to actual load-time Jeff King
2019-09-12 19:30     ` Derrick Stolee
2019-09-12 14:44   ` [PATCH v2 2/2] upload-pack: disable commit graph more gently for shallow traversal Jeff King
2019-09-13 13:26     ` Derrick Stolee
2019-09-12 16:56   ` [PATCH v2] upload-pack commit graph segfault fix Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190912135952.GB23031@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=me@ttaylorr.com \
    --cc=pclouds@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).