Re: Git v2.11.0 breaks max depth nested alternates

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

quoted

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