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