Re: Git v2.11.0 breaks max depth nested alternates

From: Jeff King <peff@peff.net>
Date: Dec 4, 2016, 04:55 UTC
Message-ID: <20161204045554.advzvylytdmt2bh2@sigill.intra.peff.net>
In-Reply-To: <fe33de5b5f0b3da68b249cc4a49a6d7@3c843fe6ba8f3c586a21345a2783aa0>

On Sat, Dec 03, 2016 at 04:24:02PM -0800, Kyle J. McKay wrote:

Show 8 quoted lines

> When the incoming quarantine takes place the current objects directory  
> is demoted to an alternate thereby increasing its depth (and any  
> alternates it references) by one and causing any object store that was  
> previously at the maximum nesting depth to be ignored courtesy of the  
> above hard-coded maximum depth.
> 
> If the incoming push happens to need access to some of those objects  
> to perhaps "--fix-thin" its pack it will crash and burn.

Yep, that makes sense. I didn't really worry about this because the existing "5" is totally arbitrary, and meant to be so high that nobody reaches it (it's just there to break cycles).

So I do think this is worth dealing with, but I'm also curious why you're hitting the depth-5 limit. I'm guessing it has to do with hosting a hierarchy of related repos. But is your system then always in danger of busting the 5-limit if people create too deep a repository hierarchy?

Specifically, I'm wondering if it would be sufficient to just bump it to
6. Or 100.

Of course any static bump runs into the funny case where a repo _usually_ works, but fails when pushed to. Which is kind of nasty and unintuitive. And your patch fixes that, and we can leave the idea of bumping the static depth number as an orthogonal issue (that personally, I do not care about much about either way).

Show 12 quoted lines

> diff --git a/common-main.c b/common-main.c
> index c654f955..9f747491 100644
> --- a/common-main.c
> +++ b/common-main.c
> @@ -37,5 +37,8 @@ int main(int argc, const char **argv)
>  
>  	restore_sigpipe_to_default();
>  
> +	if (getenv(GIT_QUARANTINE_ENVIRONMENT))
> +		alt_odb_max_depth++;
> +
>  	return cmd_main(argc, argv);

After reading your problem description, my initial thought was to increment the counter when we allocate the tmp-objdir, and decrement when it is destroyed. Because the parent receive-pack process adds it to its alternates, too. But:

  1. Receive-pack doesn't care; it adds the tmp-objdir as an alternate,
     rather than adding it as its main object dir and bumping down the
     main one.

  2. There would have to be some way of communicating to sub-processes
     that they should bump their max-depth by one.

You've basically used the quarantine-path variable as the inter-process flag for (2). Which feels a little funny, because its value is unrelated to the alt-odb setup. But it is a reliable signal, so there's a certain elegance. It's probably the best option, given that the alternative is a specific variable to say "hey, bump your max-alt-odb-depth by one". That's pretty ugly, too. :)

-Peff