Re: [PATCH] odb: do not list our own refs as alternate refs in the quarantine
- From
Jeff King <peff@peff.net>
- Date
- Oct 30, 2026, 18:17 UTC
- Message-ID
- <20261030181728.GB7582@coredump.intra.peff.net>
- In-Reply-To
- <CAL71e4PXZ8-T3RgLQQLtL2ASQMLbRrChfV6wAqxFSd_Tw7-2Sg@mail.gmail.com>
On Fri, Oct 30, 2026 at 12:51:43PM +0100, Kristofer Karlsson wrote:
Show 16 quoted lines
> > It is a bit of a shame that tmp_objdir_create() exports the same
> > value as DB_ENVIRONMENT in GIT_QUARANTINE_ENVIRONMENT, as the only
> > existing use of the latter is to check if the variable is exported
> > or not---in other words, to carry a single bit of information ("are
> > we in quarantine or not"). I wonder if we can change its value to
> > name the parent process's real object directory instead? Then we
> > would not need to export yet another variable to carry the
> > information we need.
>
> Agreed, I think that is a reasonable way forward to a concrete fix.
>
> I started working on a v2 based on your suggestion there.
> First I tried tweaking GIT_QUARANTINE_PATH to
> point to the real object dir, but I realized that might be
> unsafe since some hooks may depend on the semantics of
> that env variable, so it would be a breaking change.Yes, the reason it's a path and not a single bit is exactly for this idea. ;)
I never upstreamed this final bit of the quarantine code because it always felt a bit ugly. But how it worked at GitHub at the time (and possibly still today? I don't know) was this.
The goal is that we want to reject excessively large objects on pushes, for security (integer overflows, etc) and performance (pushing up 5GB blobs is mostly an anti-pattern). It's nice to do it in index-pack for two reasons. One is that we already have the data there, so it's a free check (as opposed to a hook running a separate scan over the objects). And two is that it's our first look at the objects, so it reduces attack surface to reject the big ones early.
Dying unceremoniously for large trees and commits is fine. Nobody is sending you 100MB trees unless they're an attacker. But with blobs, it's almost always a simple user mistake, so you'd want a more gentle message (and possibly even to do things like finding which paths point to the blob).
Our solution was for index-pack to write the set of large blobs to $GIT_QUARANTINE_PATH/large-objects. And then a hook is able to read that and produce whatever nice message you want, but without going to the expensive of running rev-list or similar just to find out that all of the blobs are fine.
I had envisioned a world where we might use it for more information, or two hooks could even use it as a rendezvous point, basically as "here's an atomic tempdir for this whole push". But I don't remember if it was ever used for that (the most likely candidate, from memory, was pushing some secret-scanning code into index-pack, since it already has the object bytes in memory, and then letting it generate a report).
Show 10 quoted lines
> Then I started adding a second env variable. That worked > and it was not _too_ messy though I realized it was a bit > redundant. We can simply deduce the real object directory > from the quarantine directory -- it's the parent, based on > how tmp_objdir_create() works today -- so I built my preliminary > v2 on that. > > It feels like a slightly icky assumption to make, but it > feels safe since I added tests to cover it, and does not seem > possible to override outside of git itself.
I don't know, it actually sounds quite sensible: we do not show --alternate-refs from an alternate source that is ourselves, because we'd already have looked at those refs. The quarantine and tmp-objdir code sets up this slightly odd alternate situation, but somebody could also do it externally by setting up a few environment variables.
-Peff