Re: Git v2.11.0 breaks max depth nested alternates

["Kyle J. McKay" <mackyle@gmail.com>]

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

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

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

> 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