Re: Git v2.11.0 breaks max depth nested alternates

From: Kyle J. McKay <mackyle@gmail.com>
Date: Dec 4, 2016, 09:37 UTC
Message-ID: <E3C2AF2A-FE07-4C94-B549-3BDAF9B3DB5D@gmail.com>
In-Reply-To: <20161204045554.advzvylytdmt2bh2@sigill.intra.peff.net>

On Dec 3, 2016, at 20:55, Jeff King wrote:

Show 6 quoted lines

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

No we check for the limit. Anything at the limit gets broken by the quarantine change though.

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

Well, if we left the current limit in place, but as you say:

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

Yes. That's not nice, hence the patch. Without the fix, pushing might work sometimes until you actually need to access cut-off objects at pre-receive time. So you might be able to push sometimes and sometimes it breaks.

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

The patch is a step on that road. It doesn't go that far but all it would take is connecting the introduced variable to a config item. But you still need to bump it by 1 during quarantine operations. Such support would even allow alternates to be disallowed (except during quarantine). I wonder if there's an opportunity for further pack operation optimizations in such a case (you know there are no alternates because they're not allowed)?

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

All true. And I had similar thoughts. Perhaps we should add your comments to the patch description? There seems to be a trend towards having longer patch descriptions these days... ;)

Show 7 quoted lines

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

You took the words right out of my mouth... I guess I need to work on doing a better job of dumping my stream-of-thoughts that go into a patch into the emails to the list.

Most all of your comments could be dumped into the patch description as-is to pimp it out some. I have no objection to that, even adding an "Additional-analysis-by:" (or similar) credit line too. :)

--Kyle