{"thread":{"id":"66692","subject":"[PATCH] odb: do not list our own refs as alternate refs in the quarantine","startedAt":"2026-10-28T15:18:24Z","lastAt":"2026-11-04T18:05:48Z","messageCount":20,"participants":["Kristofer Karlsson via GitGitGadget","Justin Tobler","Junio C Hamano","Kristofer Karlsson","Jeff King","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"556674","messageId":"pull.2285.git.1793200705017.gitgitgadget@gmail.com","threadId":"66692","inReplyTo":null,"subject":"[PATCH] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Kristofer Karlsson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-10-28T15:18:24Z","receivedAt":"2026-10-28T15:18:24Z","isPatch":true,"sender":{"key":"krka@spotify.com","avatar":"https://avatars.githubusercontent.com/u/5349266?v=4"},"body":"From: Kristofer Karlsson \n\nSince 722ff7f876 (receive-pack: quarantine objects until pre-receive\naccepts, 2016-10-03), receive-pack writes incoming objects to a\ntemporary object directory. Child processes see it as their primary\nobject directory, and tmp-objdir passes the repository's own object\ndirectory to them via GIT_ALTERNATE_OBJECT_DIRECTORIES.\n\nThe connectivity check passes --alternate-refs to rev-list since\n39b44ba771 (check_everything_connected: assume alternate ref tips are\nvalid, 2019-07-01). Inside the quarantine, the repository's own object\ndirectory is one of the alternates, so --alternate-refs runs\nfor-each-ref on the repository itself and lists all of its refs.\n\nThat includes hidden refs. bcec6780b2 (receive-pack: only use visible\nrefs for connectivity check, 2022-11-17) added --exclude-hidden=receive\nto the check so that repositories with many hidden refs do not have to\nload all of them on each push. The hidden refs are excluded from --all,\nbut they all come back through --alternate-refs.\n\nSkip an alternate whose repository is our own when listing alternate\nrefs. This is done by comparing paths, which is a hack: it is mainly\nhere to measure the cost of the bug, and a proper fix would have the\nquarantine tell its children which directory is ours. It is safe,\nthough: alternate refs only serve as negative tips that are assumed to\nbe complete, so dropping some can only make the traversal walk further,\nnever make it accept a missing object.\n\nAdd a test that our own refs are no longer listed, a test that the refs\nof a real alternate still are, and a perf test pushing a single commit\ninto a repository with a visible branch and about 200,000 refs hidden\nby receive.hideRefs, and into a copy of it that hides nothing:\n\n Test HEAD~1 HEAD\n -----------------------------------------------------------------------\n 5552.2: push, hidden refs 0.31(0.32+0.08) 0.10(0.06+0.03) -67.7%\n 5552.3: push, no hidden refs 1.25(1.08+0.68) 1.03(0.84+0.62) -17.6%\n\nWith hidden refs, most of the time went to the for-each-ref child that\nlisted the alternate refs. Without hidden refs, every ref was listed\ntwice, once for --all and once for --alternate-refs.\n\nSigned-off-by: Kristofer Karlsson \n---\n receive-pack: connectivity check does not exclude hidden refs in the\n quarantine\n \n This is mainly a bug report, in patch form so that it comes with a test\n and numbers. I don't expect it to be merged, at least not in this form.\n \n When a repository hides refs with receive.hideRefs, every push still\n loads all of the hidden refs in the connectivity check, even though\n bcec6780b2 (receive-pack: only use visible refs for connectivity check,\n 2022-11-17) excludes them from it. With many hidden refs, that is most\n of the time spent on a small push.\n \n I stumbled across this while working on the boundary walk of the\n connectivity check (the follow-up to kk/connectivity-check-incremental),\n when the walk loaded every ref during a push even though it should only\n have looked at a few of them.\n \n The cause is that receive-pack runs the check inside the object\n quarantine, and there the repository's own object directory is passed to\n rev-list as an alternate. --alternate-refs then lists all of the\n repository's own refs, hidden ones included. Without hidden refs, every\n ref is listed twice instead. Setting core.alternateRefsPrefixes to the\n visible namespaces works around it, though that also limits which refs\n of real alternates are used.\n \n The patch includes a small fix by comparing paths, mostly so that the\n perf test can show the difference (about 0.31s -> 0.10s for a push with\n 200,000 hidden refs). I am not really happy with it as a proper fix\n though.\n \n I also tried resolving the object directories that --alternate-refs uses\n before the quarantine is created. That got a bit messy: it needed a new\n internal rev-list option, and receive-pack had to carry the list of\n directories through to each of its connectivity checks. And it only\n fixes the check itself; other processes running inside the quarantine,\n like pre-receive hooks, would still see the repository as its own\n alternate.\n \n So the question is really where this should be fixed: is there a better\n way for processes running inside the quarantine to tell the repository's\n own object directory apart from real alternates?\n \n I am likely not the best person to answer that. Patrick, maybe you could\n take a look, since you have recently reworked tmp-objdir and the odb\n sources?\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2285%2Fspkrka%2Fquarantine-altrefs-bugreport-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2285/spkrka/quarantine-altrefs-bugreport-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2285\n\n odb.c | 16 ++++++++++++++\n t/perf/p5552-push-hidden-refs.sh | 37 ++++++++++++++++++++++++++++++++\n t/t5547-push-quarantine.sh | 25 +++++++++++++++++++++\n 3 files changed, 78 insertions(+)\n create mode 100755 t/perf/p5552-push-hidden-refs.sh\n\ndiff --git a/odb.c b/odb.c\nindex 2c93a9490c..0cda333c4e 100644\n--- a/odb.c\n+++ b/odb.c\n@@ -183,6 +183,7 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,\n \t\t\t\t void *payload)\n {\n \tstruct strbuf path = STRBUF_INIT;\n+\tstruct strbuf self = STRBUF_INIT;\n \tsize_t base_len;\n \tstruct alternate_refs_data *cb = payload;\n \n@@ -190,6 +191,20 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,\n \t\tgoto out;\n \tif (!strbuf_strip_suffix(&path, \"/objects\"))\n \t\tgoto out;\n+\n+\t/*\n+\t * NEEDSWORK: this is a hack, mainly here to measure the cost of\n+\t * the bug. Inside an object quarantine, tmp-objdir passes our own\n+\t * object directory to child processes as an alternate, so we would\n+\t * list our own refs as alternate refs, including the hidden refs\n+\t * that the connectivity check of receive-pack deliberately skips.\n+\t * Comparing paths papers over that; the quarantine should rather\n+\t * tell its children which directory is ours.\n+\t */\n+\tif (cb->repo->commondir &&\n+\t strbuf_realpath(&self, cb->repo->commondir, 0) &&\n+\t !strcmp(path.buf, self.buf))\n+\t\tgoto out;\n \tbase_len = path.len;\n \n \t/* Is this a git repository with refs? */\n@@ -202,6 +217,7 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,\n \n out:\n \tstrbuf_release(&path);\n+\tstrbuf_release(&self);\n \treturn 0;\n }\n \ndiff --git a/t/perf/p5552-push-hidden-refs.sh b/t/perf/p5552-push-hidden-refs.sh\nnew file mode 100755\nindex 0000000000..e9aa206298\n--- /dev/null\n+++ b/t/perf/p5552-push-hidden-refs.sh\n@@ -0,0 +1,37 @@\n+#!/bin/sh\n+\n+test_description='speed of push into a repository with many hidden refs'\n+. ./perf-lib.sh\n+\n+test_perf_fresh_repo\n+\n+test_expect_success 'set up repository with many hidden refs' '\n+\tgit init --bare dst.git &&\n+\ttest_commit_bulk --ref=refs/hidden/base --id=hidden 1000 &&\n+\tgit rev-list refs/hidden/base >commits &&\n+\tfor i in $(test_seq 200)\n+\tdo\n+\t\tsed \"s|^\\(.*\\)$|create refs/hidden/$i/\\1 \\1|\" commits || return 1\n+\tdone >input &&\n+\tgit push dst.git refs/hidden/base &&\n+\tgit -C dst.git update-ref --stdin >\"$(pwd)/alternate-refs.log\"\n+\tEOF\n+\tgit -C hidden.git config core.alternateRefsCommand ./alternate-refs &&\n+\ttest_commit hidden-push &&\n+\tgit push hidden.git HEAD:refs/heads/new &&\n+\ttest_path_is_missing alternate-refs.log\n+'\n+\n+test_expect_success 'quarantine lists refs of other alternates' '\n+\ttest_when_finished \"rm -f alternate-refs.log\" &&\n+\tgit init --bare other.git &&\n+\tgit push other.git HEAD:refs/heads/other &&\n+\techo \"$(pwd)/other.git/objects\" >hidden.git/objects/info/alternates &&\n+\ttest_commit hidden-push-2 &&\n+\tgit push hidden.git HEAD:refs/heads/new2 &&\n+\t(cd other.git && pwd -P) >expect &&\n+\tsort -u alternate-refs.log >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\nbase-commit: 37b3475fb530c14109211faca33716a173db61ed\n-- \ngitgitgadget\n\n"},{"id":"556803","messageId":"auNvMUNxmeJe31Bq@denethor","threadId":"66692","inReplyTo":"pull.2285.git.1793200705017.gitgitgadget@gmail.com","subject":"Re: [PATCH] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-10-29T16:57:42Z","receivedAt":"2026-10-29T16:57:42Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 26/10/28 03:18PM, Kristofer Karlsson via GitGitGadget wrote:\n> From: Kristofer Karlsson \n> \n> Since 722ff7f876 (receive-pack: quarantine objects until pre-receive\n> accepts, 2016-10-03), receive-pack writes incoming objects to a\n> temporary object directory. Child processes see it as their primary\n> object directory, and tmp-objdir passes the repository's own object\n> directory to them via GIT_ALTERNATE_OBJECT_DIRECTORIES.\n> \n> The connectivity check passes --alternate-refs to rev-list since\n> 39b44ba771 (check_everything_connected: assume alternate ref tips are\n> valid, 2019-07-01). Inside the quarantine, the repository's own object\n> directory is one of the alternates, so --alternate-refs runs\n> for-each-ref on the repository itself and lists all of its refs.\n\nThat is quite interesting. So IIUC, because the \"real\" ODB is an\nalternate, all the references from the main repository are marked as\nuninteresting even though they have already been processed. For the\nconnectivity check performed by git-receive-pack(1), there isn't any\nreason to consider \"alternate refs\" from the \"real\" ODB that is\nconfigured as an alternate.\n\n> That includes hidden refs. bcec6780b2 (receive-pack: only use visible\n> refs for connectivity check, 2022-11-17) added --exclude-hidden=receive\n> to the check so that repositories with many hidden refs do not have to\n> load all of them on each push. The hidden refs are excluded from --all,\n> but they all come back through --alternate-refs.\n\nSo we effectively consider all the references a second time, but always\nall of them including the previously excluded hidden refs. Yikes.\n\n> Skip an alternate whose repository is our own when listing alternate\n> refs. This is done by comparing paths, which is a hack: it is mainly\n> here to measure the cost of the bug, and a proper fix would have the\n> quarantine tell its children which directory is ours. It is safe,\n> though: alternate refs only serve as negative tips that are assumed to\n> be complete, so dropping some can only make the traversal walk further,\n> never make it accept a missing object.\n\nThis indeed sounds like a bit of a hack. To me the underlying issue here\nis that downstream git processes don't really have any concept of an ODB\ntransaction. We rely on the git alternates mechanism and overriding the\nprimary object directory to get the unaware child processes to behave\nhow we want. This typically means setting the quarantine directory as\nthe main object directory and the \"real\" object directory as an\nalternate via environment variables. That way any writes that are\nperformed by a child process are written to the quarantine.\n\nFor the connectivity check though, we don't really expect\ngit-rev-list(1) to write any objects. Consequently, I don't think we\nneccessary need to set the quarantine directory as the primary object\ndirectory here. That would avoid git-rev-list(1) processing the \"real\"\nobject directory references a second time.\n\n[snip]\n> I also tried resolving the object directories that --alternate-refs uses\n> before the quarantine is created. That got a bit messy: it needed a new\n> internal rev-list option, and receive-pack had to carry the list of\n> directories through to each of its connectivity checks. And it only\n> fixes the check itself; other processes running inside the quarantine,\n> like pre-receive hooks, would still see the repository as its own\n> alternate.\n\nFor the connectivity check performed via git-rev-list(1) it certainly\nmatters, but I do wonder though for other processes if this really\nchanges anything for them in practice. It could potentially be nice if\nthere was a mechanism to more explicitly propagate a pending transaction\naccross process boundaries, but I'm not sure there are many usecases for\nthat at present.\n\n> So the question is really where this should be fixed: is there a better\n> way for processes running inside the quarantine to tell the repository's\n> own object directory apart from real alternates?\n\nBuilding off of my previous thoughts, maybe it would make sense to make\nthe connectivity check a more explicit part of the ODB transaction\ninterface? Maybe for the \"files\" backend we could introduce a\n`odb_transaction_files_check_connected()` callback that configures\ngit-rev-list(1) with the appropriate environment to validate the\nquarantined objects and not cause this bug? Happy to explore this path\nfurther if it seems reasonable.\n\n-Justin\n\n"},{"id":"556804","messageId":"xmqqfqxo4dy1.fsf@gitster.g","threadId":"66692","inReplyTo":"pull.2285.git.1793200705017.gitgitgadget@gmail.com","subject":"Re: [PATCH] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-29T17:24:54Z","receivedAt":"2026-10-29T17:24:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristofer Karlsson via GitGitGadget\" \nwrites:\n\n> I also tried resolving the object directories that --alternate-refs uses\n> before the quarantine is created. That got a bit messy: it needed a new\n> internal rev-list option, and receive-pack had to carry the list of\n> directories through to each of its connectivity checks. And it only\n> fixes the check itself; other processes running inside the quarantine,\n> like pre-receive hooks, would still see the repository as its own\n> alternate.\n> \n> So the question is really where this should be fixed: is there a better\n> way for processes running inside the quarantine to tell the repository's\n> own object directory apart from real alternates?\n\n\n\n> @@ -190,6 +191,20 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,\n> \t\tgoto out;\n> \tif (!strbuf_strip_suffix(&path, \"/objects\"))\n> \t\tgoto out;\n> +\n> +\t/*\n> +\t * NEEDSWORK: this is a hack, mainly here to measure the cost of\n> +\t * the bug. Inside an object quarantine, tmp-objdir passes our own\n> +\t * object directory to child processes as an alternate, so we would\n> +\t * list our own refs as alternate refs, including the hidden refs\n> +\t * that the connectivity check of receive-pack deliberately skips.\n> +\t * Comparing paths papers over that; the quarantine should rather\n> +\t * tell its children which directory is ours.\n> +\t */\n\nIn other words, if we tell our quarantine child where our real\nobject directory is, say, via the 'GIT_QUARANTINE_OBJECT_DIRECTORY'\nenvironment variable, can the child notice the variable and identify\nwhich of the passed alternates is the real object directory, so that\nit knows not to use the refs associated with it?\n\nIt is a bit of a shame that tmp_objdir_create() exports the same\nvalue as DB_ENVIRONMENT in GIT_QUARANTINE_ENVIRONMENT, as the only\nexisting use of the latter is to check if the variable is exported\nor not---in other words, to carry a single bit of information (\"are\nwe in quarantine or not\"). I wonder if we can change its value to\nname the parent process's real object directory instead? Then we\nwould not need to export yet another variable to carry the\ninformation we need.\n\n"},{"id":"556910","messageId":"CAL71e4P9LauUqjKTx1VeqehGVrbOpb3PXC8z1iQ6Bh7CFihr8w@mail.gmail.com","threadId":"66692","inReplyTo":"auNvMUNxmeJe31Bq@denethor","subject":"Re: [PATCH] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Kristofer Karlsson","fromEmail":"krka@spotify.com","sentAt":"2026-10-30T11:29:12Z","receivedAt":"2026-10-30T11:29:12Z","isPatch":true,"sender":{"key":"krka@spotify.com","avatar":"https://avatars.githubusercontent.com/u/5349266?v=4"},"body":"On Thu, 29 Oct 2026 at 17:57, Justin Tobler wrote:\n>\n> This indeed sounds like a bit of a hack. To me the underlying issue here\n> is that downstream git processes don't really have any concept of an ODB\n> transaction. We rely on the git alternates mechanism and overriding the\n> primary object directory to get the unaware child processes to behave\n> how we want. This typically means setting the quarantine directory as\n> the main object directory and the \"real\" object directory as an\n> alternate via environment variables. That way any writes that are\n> performed by a child process are written to the quarantine.\n>\n> For the connectivity check though, we don't really expect\n> git-rev-list(1) to write any objects. Consequently, I don't think we\n> neccessary need to set the quarantine directory as the primary object\n> directory here. That would avoid git-rev-list(1) processing the \"real\"\n> object directory references a second time.\n\nThe connectivity check explicitly disables lazy fetching so it\nshould be safe in practice, but it's hard to reason about\npotential regressions and having a more strict flow for this\nwould be nice perhaps.\n\n> > So the question is really where this should be fixed: is there a better\n> > way for processes running inside the quarantine to tell the repository's\n> > own object directory apart from real alternates?\n>\n> Building off of my previous thoughts, maybe it would make sense to make\n> the connectivity check a more explicit part of the ODB transaction\n> interface? Maybe for the \"files\" backend we could introduce a\n> `odb_transaction_files_check_connected()` callback that configures\n> git-rev-list(1) with the appropriate environment to validate the\n> quarantined objects and not cause this bug? Happy to explore this path\n> further if it seems reasonable.\n>\n> -Justin\n\nI am somewhat of a novice here, but I had a similar thought,\nthat this should ideally be fixed by tweaking the API somehow,\nalthough I do not know exactly. I am not sure if we should\nintroduce odb_transaction_files_check_connected() or resolve\nthe alternate-refs into fixed directories earlier in the chain,\nor enforce better read-only mode (and add quarantine as a pure\nalternate for the connectivity check).\n\nSo, I am not sure what the long term fix is, but I think your\nidea makes sense for that. For the short term, I think\nsomething closer to what Junio suggested in the other reply\nis a reasonable way forward.\n\nThanks,\nKristofer\n\n"},{"id":"556913","messageId":"CAL71e4PXZ8-T3RgLQQLtL2ASQMLbRrChfV6wAqxFSd_Tw7-2Sg@mail.gmail.com","threadId":"66692","inReplyTo":"xmqqfqxo4dy1.fsf@gitster.g","subject":"Re: [PATCH] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Kristofer Karlsson","fromEmail":"krka@spotify.com","sentAt":"2026-10-30T11:51:43Z","receivedAt":"2026-10-30T11:51:43Z","isPatch":true,"sender":{"key":"krka@spotify.com","avatar":"https://avatars.githubusercontent.com/u/5349266?v=4"},"body":"On Thu, 29 Oct 2026 at 18:24, Junio C Hamano wrote:\n>\n> In other words, if we tell our quarantine child where our real\n> object directory is, say, via the 'GIT_QUARANTINE_OBJECT_DIRECTORY'\n> environment variable, can the child notice the variable and identify\n> which of the passed alternates is the real object directory, so that\n> it knows not to use the refs associated with it?\n>\n> It is a bit of a shame that tmp_objdir_create() exports the same\n> value as DB_ENVIRONMENT in GIT_QUARANTINE_ENVIRONMENT, as the only\n> existing use of the latter is to check if the variable is exported\n> or not---in other words, to carry a single bit of information (\"are\n> we in quarantine or not\"). I wonder if we can change its value to\n> name the parent process's real object directory instead? Then we\n> would not need to export yet another variable to carry the\n> information we need.\n\nAgreed, I think that is a reasonable way forward to a concrete fix.\n\nI started working on a v2 based on your suggestion there.\nFirst I tried tweaking GIT_QUARANTINE_PATH to\npoint to the real object dir, but I realized that might be\nunsafe since some hooks may depend on the semantics of\nthat env variable, so it would be a breaking change.\n\nThen I started adding a second env variable. That worked\nand it was not _too_ messy though I realized it was a bit\nredundant. We can simply deduce the real object directory\nfrom the quarantine directory -- it's the parent, based on\nhow tmp_objdir_create() works today -- so I built my preliminary\nv2 on that.\n\nIt feels like a slightly icky assumption to make, but it\nfeels safe since I added tests to cover it, and does not seem\npossible to override outside of git itself.\n\nThat said, if you think this is over-simplifying it, I can\npivot back to introducing a new environment variable instead.\n\nThanks!\nKristofer\n\n"},{"id":"556930","messageId":"xmqqjymzxjgz.fsf@gitster.g","threadId":"66692","inReplyTo":"CAL71e4PXZ8-T3RgLQQLtL2ASQMLbRrChfV6wAqxFSd_Tw7-2Sg@mail.gmail.com","subject":"Re: [PATCH] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-30T16:05:00Z","receivedAt":"2026-10-30T16:05:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristofer Karlsson writes:\n\n> Then I started adding a second env variable. That worked\n> and it was not _too_ messy though I realized it was a bit\n> redundant. We can simply deduce the real object directory\n> from the quarantine directory -- it's the parent, based on\n> how tmp_objdir_create() works today -- so I built my preliminary\n> v2 on that.\n>\n> It feels like a slightly icky assumption to make, but it\n> feels safe since I added tests to cover it, and does not seem\n> possible to override outside of git itself.\n\nWell, as Patrick said elsewhere recently, the entire \"since we are\nborrowing from this alternate object store, it would be nice if we\ncan assume there is some ref storage that tells us which objects in\nit are 'complete', but because the alternate object store interface\ndoes not define such a set of refs associated with it, let's just\nassume the refs/ directory sitting immediately next to the objects/\ndirectory is such a hierarchy\" is an icky hack that is crumbling,\ngiven that the alternate object store may be a part of a repository\nthat uses reftable. It also is troublesome to \"borrow\" from a\nshallow clone where their grafts are part of the mechanism they use\nto avoid stepping outside the set of available objects, which the\nalternate object store mechanism does not \"borrow\", as far as I\nrecall.\n\nSo until we figure out how to fix these icky points, it probably is\nnot a huge deal to pile on another icky assumption as a short-term\nmeasure.\n\n"},{"id":"556948","messageId":"20261030181728.GB7582@coredump.intra.peff.net","threadId":"66692","inReplyTo":"CAL71e4PXZ8-T3RgLQQLtL2ASQMLbRrChfV6wAqxFSd_Tw7-2Sg@mail.gmail.com","subject":"Re: [PATCH] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-10-30T18:17:28Z","receivedAt":"2026-10-30T18:17:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 30, 2026 at 12:51:43PM +0100, Kristofer Karlsson wrote:\n\n> > It is a bit of a shame that tmp_objdir_create() exports the same\n> > value as DB_ENVIRONMENT in GIT_QUARANTINE_ENVIRONMENT, as the only\n> > existing use of the latter is to check if the variable is exported\n> > or not---in other words, to carry a single bit of information (\"are\n> > we in quarantine or not\"). I wonder if we can change its value to\n> > name the parent process's real object directory instead? Then we\n> > would not need to export yet another variable to carry the\n> > information we need.\n> \n> Agreed, I think that is a reasonable way forward to a concrete fix.\n> \n> I started working on a v2 based on your suggestion there.\n> First I tried tweaking GIT_QUARANTINE_PATH to\n> point to the real object dir, but I realized that might be\n> unsafe since some hooks may depend on the semantics of\n> that env variable, so it would be a breaking change.\n\nYes, the reason it's a path and not a single bit is exactly for this\nidea. ;)\n\nI never upstreamed this final bit of the quarantine code because it\nalways felt a bit ugly. But how it worked at GitHub at the time (and\npossibly still today? I don't know) was this.\n\nThe goal is that we want to reject excessively large objects on pushes,\nfor security (integer overflows, etc) and performance (pushing up 5GB\nblobs is mostly an anti-pattern). It's nice to do it in index-pack for\ntwo reasons. One is that we already have the data there, so it's a free\ncheck (as opposed to a hook running a separate scan over the objects).\nAnd two is that it's our first look at the objects, so it reduces attack\nsurface to reject the big ones early.\n\nDying unceremoniously for large trees and commits is fine. Nobody is\nsending you 100MB trees unless they're an attacker. But with blobs, it's\nalmost always a simple user mistake, so you'd want a more gentle message\n(and possibly even to do things like finding which paths point to the\nblob).\n\nOur solution was for index-pack to write the set of large blobs to\n$GIT_QUARANTINE_PATH/large-objects. And then a hook is able to read that\nand produce whatever nice message you want, but without going to the\nexpensive of running rev-list or similar just to find out that all of\nthe blobs are fine.\n\nI had envisioned a world where we might use it for more information, or\ntwo hooks could even use it as a rendezvous point, basically as \"here's\nan atomic tempdir for this whole push\". But I don't remember if it was\never used for that (the most likely candidate, from memory, was pushing\nsome secret-scanning code into index-pack, since it already has the\nobject bytes in memory, and then letting it generate a report).\n\n> Then I started adding a second env variable. That worked\n> and it was not _too_ messy though I realized it was a bit\n> redundant. We can simply deduce the real object directory\n> from the quarantine directory -- it's the parent, based on\n> how tmp_objdir_create() works today -- so I built my preliminary\n> v2 on that.\n> \n> It feels like a slightly icky assumption to make, but it\n> feels safe since I added tests to cover it, and does not seem\n> possible to override outside of git itself.\n\nI don't know, it actually sounds quite sensible: we do not show\n--alternate-refs from an alternate source that is ourselves, because\nwe'd already have looked at those refs. The quarantine and tmp-objdir\ncode sets up this slightly odd alternate situation, but somebody could\nalso do it externally by setting up a few environment variables.\n\n-Peff\n\n"},{"id":"557167","messageId":"pull.2285.v2.git.1793619933300.gitgitgadget@gmail.com","threadId":"66692","inReplyTo":"pull.2285.git.1793200705017.gitgitgadget@gmail.com","subject":"[PATCH v2] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Kristofer Karlsson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-11-02T11:45:33Z","receivedAt":"2026-11-02T11:45:33Z","isPatch":true,"sender":{"key":"krka@spotify.com","avatar":"https://avatars.githubusercontent.com/u/5349266?v=4"},"body":"From: Kristofer Karlsson \n\nSince 722ff7f876 (receive-pack: quarantine objects until pre-receive\naccepts, 2016-10-03), receive-pack writes incoming objects to a\ntemporary object directory. Child processes see it as their primary\nobject directory, and tmp-objdir passes the repository's own object\ndirectory to them via GIT_ALTERNATE_OBJECT_DIRECTORIES.\n\nThe connectivity check passes --alternate-refs to rev-list since\n39b44ba771 (check_everything_connected: assume alternate ref tips are\nvalid, 2019-07-01). Inside the quarantine, the repository's own object\ndirectory is one of the alternates, so --alternate-refs runs\nfor-each-ref on the repository itself and lists all of its refs.\n\nThat includes hidden refs. bcec6780b2 (receive-pack: only use visible\nrefs for connectivity check, 2022-11-17) added --exclude-hidden=receive\nto the check so that repositories with many hidden refs do not have to\nload all of them on each push. The hidden refs are excluded from --all,\nbut they all come back through --alternate-refs.\n\nAn alternate that is ourselves is not a source of alternate refs: its\nrefs are our own refs, which the caller already lists as it sees fit.\nMark each object directory added via alternates with whether it is\nborrowed from another repository, and list alternate refs only from\nthose. Within one process this needs no guessing, as tmp-objdir\nlinks our own object directory in itself. But a child process rebuilds\nits object store from the environment, so when an alternate is added,\nrecognize ourselves in two ways:\n\n - as our /objects, which also covers an environment that\n makes our own object directory an alternate by hand;\n\n - as the parent of GIT_QUARANTINE_PATH, as the quarantine is created\n directly inside our object directory (see \"QUARANTINE ENVIRONMENT\"\n in git-receive-pack(1)); this also works when the object directory\n is not under the commondir.\n\nDocument in --alternate-refs that our own object directory does not\ncount as an alternate repository.\n\nThis is safe: alternate refs only serve as negative tips that are\nassumed to be complete, so dropping some can only make the traversal\nwalk further, never make it accept a missing object.\n\nAdd tests that our own refs are no longer listed, by the connectivity\ncheck, by rev-list run from a pre-receive hook, by rev-list with our\nown object directory passed in as an alternate by hand, and by rev-list\nin a quarantine whose parent is not under the commondir, and that the\nrefs of a real alternate still are. Add a perf test pushing a\nsingle commit into a repository with a visible branch and about\n200,000 refs hidden by receive.hideRefs, and into a copy of it that\nhides nothing:\n\n Test HEAD~1 HEAD\n -----------------------------------------------------------------------\n 5552.2: push, hidden refs 0.31(0.33+0.08) 0.09(0.06+0.03) -71.0%\n 5552.3: push, no hidden refs 1.23(1.06+0.68) 1.01(0.83+0.61) -17.9%\n\nWith hidden refs, most of the time went to the for-each-ref child that\nlisted the alternate refs. Without hidden refs, every ref was listed\ntwice, once for --all and once for --alternate-refs.\n\nHelped-by: Jeff King \nSigned-off-by: Kristofer Karlsson \n---\n odb: do not list our own refs as alternate refs in the quarantine\n \n When a repository hides refs with receive.hideRefs, every push still\n loads all of the hidden refs in the connectivity check, even though\n bcec6780b2 (receive-pack: only use visible refs for connectivity check,\n 2022-11-17) excludes them from it. With many hidden refs, that is most\n of the time spent on a small push.\n \n I stumbled across this while working on the boundary walk of the\n connectivity check (the follow-up to kk/connectivity-check-incremental),\n when the walk loaded every ref during a push even though it should only\n have looked at a few of them.\n \n Changes since v1:\n \n * Changed the rule to \"an alternate that is ourselves is not a source\n of alternate refs\", as Peff described it.\n \n * Added a \"borrowed\" flag to alternates, set when they are loaded,\n which says whether they come from another repository, and used it for\n --alternate-refs. Our own object directory is recognized either as\n /objects or as the parent of GIT_QUARANTINE_PATH.\n \n * Documented this for --alternate-refs.\n \n * Added tests for rev-list --alternate-refs in a pre-receive hook, for\n our own object directory set up as an alternate by hand, and for a\n quarantine outside of the commondir.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2285%2Fspkrka%2Fquarantine-altrefs-bugreport-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2285/spkrka/quarantine-altrefs-bugreport-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/2285\n\nRange-diff vs v1:\n\n 1: 1b1de6eee1 ! 1: bac15042c0 odb: do not list our own refs as alternate refs in the quarantine\n @@ Commit message\n load all of them on each push. The hidden refs are excluded from --all,\n but they all come back through --alternate-refs.\n \n - Skip an alternate whose repository is our own when listing alternate\n - refs. This is done by comparing paths, which is a hack: it is mainly\n - here to measure the cost of the bug, and a proper fix would have the\n - quarantine tell its children which directory is ours. It is safe,\n - though: alternate refs only serve as negative tips that are assumed to\n - be complete, so dropping some can only make the traversal walk further,\n - never make it accept a missing object.\n -\n - Add a test that our own refs are no longer listed, a test that the refs\n - of a real alternate still are, and a perf test pushing a single commit\n - into a repository with a visible branch and about 200,000 refs hidden\n - by receive.hideRefs, and into a copy of it that hides nothing:\n + An alternate that is ourselves is not a source of alternate refs: its\n + refs are our own refs, which the caller already lists as it sees fit.\n + Mark each object directory added via alternates with whether it is\n + borrowed from another repository, and list alternate refs only from\n + those. Within one process this needs no guessing, as tmp-objdir\n + links our own object directory in itself. But a child process rebuilds\n + its object store from the environment, so when an alternate is added,\n + recognize ourselves in two ways:\n +\n + - as our /objects, which also covers an environment that\n + makes our own object directory an alternate by hand;\n +\n + - as the parent of GIT_QUARANTINE_PATH, as the quarantine is created\n + directly inside our object directory (see \"QUARANTINE ENVIRONMENT\"\n + in git-receive-pack(1)); this also works when the object directory\n + is not under the commondir.\n +\n + Document in --alternate-refs that our own object directory does not\n + count as an alternate repository.\n +\n + This is safe: alternate refs only serve as negative tips that are\n + assumed to be complete, so dropping some can only make the traversal\n + walk further, never make it accept a missing object.\n +\n + Add tests that our own refs are no longer listed, by the connectivity\n + check, by rev-list run from a pre-receive hook, by rev-list with our\n + own object directory passed in as an alternate by hand, and by rev-list\n + in a quarantine whose parent is not under the commondir, and that the\n + refs of a real alternate still are. Add a perf test pushing a\n + single commit into a repository with a visible branch and about\n + 200,000 refs hidden by receive.hideRefs, and into a copy of it that\n + hides nothing:\n \n Test HEAD~1 HEAD\n -----------------------------------------------------------------------\n - 5552.2: push, hidden refs 0.31(0.32+0.08) 0.10(0.06+0.03) -67.7%\n - 5552.3: push, no hidden refs 1.25(1.08+0.68) 1.03(0.84+0.62) -17.6%\n + 5552.2: push, hidden refs 0.31(0.33+0.08) 0.09(0.06+0.03) -71.0%\n + 5552.3: push, no hidden refs 1.23(1.06+0.68) 1.01(0.83+0.61) -17.9%\n \n With hidden refs, most of the time went to the for-each-ref child that\n listed the alternate refs. Without hidden refs, every ref was listed\n twice, once for --all and once for --alternate-refs.\n \n + Helped-by: Jeff King \n Signed-off-by: Kristofer Karlsson \n \n + ## Documentation/rev-list-options.adoc ##\n +@@ Documentation/rev-list-options.adoc: explicitly.\n + \tPretend as if all objects mentioned as ref tips of alternate\n + \trepositories were listed on the command line. An alternate\n + \trepository is any repository whose object directory is specified\n +-\tin `objects/info/alternates`. The set of included objects may\n ++\tin `objects/info/alternates`. The repository's own object\n ++\tdirectory does not count, even when it is passed in as an\n ++\talternate, as happens inside the object quarantine of\n ++\tlinkgit:git-receive-pack[1]. The set of included objects may\n + \tbe modified by `core.alternateRefsCommand`, etc. See\n + \tlinkgit:git-config[1].\n + \n +\n ## odb.c ##\n @@ odb.c: static int refs_from_alternate_cb(struct odb_files_dir *alternate,\n - \t\t\t\t void *payload)\n - {\n - \tstruct strbuf path = STRBUF_INIT;\n -+\tstruct strbuf self = STRBUF_INIT;\n \tsize_t base_len;\n \tstruct alternate_refs_data *cb = payload;\n \n -@@ odb.c: static int refs_from_alternate_cb(struct odb_files_dir *alternate,\n ++\tif (!alternate->borrowed)\n ++\t\tgoto out;\n ++\n + \tif (!strbuf_realpath(&path, alternate->abspath, 0))\n \t\tgoto out;\n \tif (!strbuf_strip_suffix(&path, \"/objects\"))\n +\n + ## odb/source-files.c ##\n +@@\n + #include \"blob.h\"\n + #include \"chdir-notify.h\"\n + #include \"config.h\"\n ++#include \"dir.h\"\n ++#include \"environment.h\"\n + #include \"gettext.h\"\n + #include \"hex.h\"\n + #include \"lockfile.h\"\n +@@ odb/source-files.c: static void odb_source_files_free(struct odb_source *source)\n + \t\tfiles->dirs = next;\n + \t}\n + \thashmap_clear(&files->dirs_by_path);\n ++\tfree(files->quarantine_path);\n + \n + \todb_source_release(&files->base);\n + \tfree(files);\n +@@ odb/source-files.c: static int read_alternates(const char *object_dir, struct strvec *out)\n + \treturn 0;\n + }\n + \n ++/*\n ++ * Our own object directory is /objects, or the parent of\n ++ * GIT_QUARANTINE_PATH, as an object quarantine is created directly\n ++ * inside it (see tmp_objdir_create()). Anything else, including a path\n ++ * that cannot be resolved, is borrowed.\n ++ */\n ++static bool is_borrowed_object_dir(struct odb_source_files *files,\n ++\t\t\t\t const char *path)\n ++{\n ++\tstruct strbuf real = STRBUF_INIT;\n ++\tstruct strbuf objdir = STRBUF_INIT;\n ++\tstruct strbuf self = STRBUF_INIT;\n ++\tbool ret = true;\n ++\n ++\tif (!strbuf_realpath(&real, path, 0))\n ++\t\tgoto out;\n ++\n ++\tstrbuf_addf(&objdir, \"%s/objects\", files->base.odb->repo->commondir);\n ++\tif (strbuf_realpath(&self, objdir.buf, 0) &&\n ++\t !fspathcmp(real.buf, self.buf)) {\n ++\t\tret = false;\n ++\t\tgoto out;\n ++\t}\n ++\n ++\tif (files->quarantine_path &&\n ++\t strbuf_realpath(&self, files->quarantine_path, 0)) {\n ++\t\tstrbuf_strip_file_from_path(&self);\n ++\t\tstrbuf_trim_trailing_dir_sep(&self);\n ++\t\tif (!fspathcmp(real.buf, self.buf))\n ++\t\t\tret = false;\n ++\t}\n ++\n ++out:\n ++\tstrbuf_release(&real);\n ++\tstrbuf_release(&objdir);\n ++\tstrbuf_release(&self);\n ++\treturn ret;\n ++}\n ++\n + static void odb_add_alternate_recursively(struct odb_source_files *files,\n + \t\t\t\t\t const char *path,\n + \t\t\t\t\t int depth)\n +@@ odb/source-files.c: static void odb_add_alternate_recursively(struct odb_source_files *files,\n \t\tgoto out;\n + \n + \talternate = odb_files_dir_new(files->base.odb, path, false);\n ++\talternate->borrowed = is_borrowed_object_dir(files, alternate->abspath);\n + \n + \t/* add the alternate entry */\n + \t*files->dirs_tail = alternate;\n +@@ odb/source-files.c: struct odb_source_files *odb_source_files_new(struct object_database *odb,\n + \tstruct odb_source_files *files;\n + \tchar *object_dir = NULL;\n + \tchar *alternates = NULL;\n ++\tchar *quarantine_path = NULL;\n + \n + \tif (flags & ODB_NEW_HONOR_ENV) {\n + \t\tobject_dir = xstrdup_or_null(getenv(DB_ENVIRONMENT));\n + \t\talternates = xstrdup_or_null(getenv(ALTERNATE_DB_ENVIRONMENT));\n ++\t\tquarantine_path =\n ++\t\t\txstrdup_or_null(getenv(GIT_QUARANTINE_ENVIRONMENT));\n + \t}\n + \tif (!object_dir)\n + \t\tobject_dir = xstrfmt(\"%s/objects\", odb->repo->commondir);\n + \n + \tCALLOC_ARRAY(files, 1);\n ++\tfiles->quarantine_path = quarantine_path;\n + \todb_source_init(&files->base, odb, ODB_SOURCE_FILES, object_dir);\n + \n + \thashmap_init(&files->dirs_by_path, odb_files_dir_by_path_cmp, files, 0);\n +\n + ## odb/source-files.h ##\n +@@ odb/source-files.h: struct odb_files_dir {\n + \t * repository. Directories added via alternates are not local.\n + \t */\n + \tbool local;\n +\n +\t/*\n -+\t * NEEDSWORK: this is a hack, mainly here to measure the cost of\n -+\t * the bug. Inside an object quarantine, tmp-objdir passes our own\n -+\t * object directory to child processes as an alternate, so we would\n -+\t * list our own refs as alternate refs, including the hidden refs\n -+\t * that the connectivity check of receive-pack deliberately skips.\n -+\t * Comparing paths papers over that; the quarantine should rather\n -+\t * tell its children which directory is ours.\n ++\t * Whether this directory is borrowed from another repository, so\n ++\t * that its refs are alternate refs. Not every directory added via\n ++\t * alternates is borrowed: inside an object quarantine, our own\n ++\t * object directory is passed to us as an alternate, too. Only set\n ++\t * for directories added via alternates.\n +\t */\n -+\tif (cb->repo->commondir &&\n -+\t strbuf_realpath(&self, cb->repo->commondir, 0) &&\n -+\t !strcmp(path.buf, self.buf))\n -+\t\tgoto out;\n - \tbase_len = path.len;\n - \n - \t/* Is this a git repository with refs? */\n -@@ odb.c: static int refs_from_alternate_cb(struct odb_files_dir *alternate,\n ++\tbool borrowed;\n + };\n \n - out:\n - \tstrbuf_release(&path);\n -+\tstrbuf_release(&self);\n - \treturn 0;\n - }\n + struct odb_files_dir *odb_files_dir_new(struct object_database *odb,\n +@@ odb/source-files.h: struct odb_source_files {\n + \t * determined by \"core.ignoreCase\".\n + \t */\n + \tint dirs_paths_icase;\n ++\n ++\t/*\n ++\t * The value of GIT_QUARANTINE_PATH when this source was set up from\n ++\t * the environment (see ODB_NEW_HONOR_ENV), or NULL.\n ++\t */\n ++\tchar *quarantine_path;\n + };\n \n + /* Allocate and initialize a new object source. */\n \n ## t/perf/p5552-push-hidden-refs.sh (new) ##\n @@\n @@ t/perf/p5552-push-hidden-refs.sh (new)\n +test_done\n \n ## t/t5547-push-quarantine.sh ##\n -@@ t/t5547-push-quarantine.sh: test_expect_success '.keep file is removed after push' '\n - \ttest_path_is_missing \"$keep\"\n +@@ t/t5547-push-quarantine.sh: test_expect_success '.keep file created outside of process is not removed' '\n + \ttest_path_is_missing \"${keep%.keep}.pack\"\n '\n \n +test_expect_success 'quarantine does not list own refs as alternate refs' '\n @@ t/t5547-push-quarantine.sh: test_expect_success '.keep file is removed after pus\n +\tsort -u alternate-refs.log >actual &&\n +\ttest_cmp expect actual\n +'\n ++\n ++test_expect_success 'rev-list --alternate-refs in a hook skips own refs' '\n ++\ttest_when_finished \"rm -rf altref.git\" &&\n ++\tgit init --bare altref.git &&\n ++\tgit push altref.git HEAD:refs/heads/main &&\n ++\ttest_hook -C altref.git pre-receive <<-\\EOF &&\n ++\tgit rev-list --no-walk --alternate-refs --stdin ../hook-alternate-refs\n ++\tEOF\n ++\ttest_commit altref-push &&\n ++\tgit push altref.git HEAD:refs/heads/new &&\n ++\ttest_must_be_empty hook-alternate-refs\n ++'\n ++\n + test_done\n +\n + ## t/t5618-alternate-refs.sh ##\n +@@ t/t5618-alternate-refs.sh: test_expect_success 'log --source shows .alternate marker' '\n + \ttest_cmp expect actual\n + '\n + \n ++# Our own object directory as an alternate is not a source of alternate\n ++# refs, but the alternates it brings in with it still are.\n ++test_expect_success 'own object directory as alternate lists only real alternates' '\n ++\ttest_when_finished \"rm -rf other-objects\" &&\n ++\tmkdir other-objects &&\n ++\tgit rev-list --remotes=origin >expect &&\n ++\tGIT_OBJECT_DIRECTORY=\"$(pwd)/other-objects\" \\\n ++\tGIT_ALTERNATE_OBJECT_DIRECTORIES=\"$(pwd)/.git/objects\" \\\n ++\t\tgit rev-list --alternate-refs >actual &&\n ++\ttest_cmp expect actual\n ++'\n ++\n ++# A quarantine is created inside our object directory, so its parent is\n ++# ourselves, even when that is not under our commondir.\n ++test_expect_success 'parent of the quarantine is not listed as an alternate' '\n ++\ttest_when_finished \"rm -rf store.git\" &&\n ++\tgit init --bare store.git &&\n ++\tgit push store.git refs/remotes/origin/one:refs/heads/one &&\n ++\tquarantine=\"$(pwd)/store.git/objects/tmp_objdir-test\" &&\n ++\tmkdir \"$quarantine\" &&\n ++\n ++\tgit rev-parse origin/one >expect &&\n ++\tGIT_OBJECT_DIRECTORY=\"$quarantine\" \\\n ++\tGIT_ALTERNATE_OBJECT_DIRECTORIES=\"$(pwd)/store.git/objects\" \\\n ++\t\tgit rev-list --no-walk --alternate-refs --stdin actual &&\n ++\ttest_cmp expect actual &&\n ++\n ++\tGIT_QUARANTINE_PATH=\"$quarantine\" \\\n ++\tGIT_OBJECT_DIRECTORY=\"$quarantine\" \\\n ++\tGIT_ALTERNATE_OBJECT_DIRECTORIES=\"$(pwd)/store.git/objects\" \\\n ++\t\tgit rev-list --no-walk --alternate-refs --stdin actual &&\n ++\ttest_must_be_empty actual\n ++'\n +\n test_done\n\n\n Documentation/rev-list-options.adoc | 5 ++-\n odb.c | 3 ++\n odb/source-files.c | 47 +++++++++++++++++++++++++++++\n odb/source-files.h | 15 +++++++++\n t/perf/p5552-push-hidden-refs.sh | 37 +++++++++++++++++++++++\n t/t5547-push-quarantine.sh | 38 +++++++++++++++++++++++\n t/t5618-alternate-refs.sh | 34 +++++++++++++++++++++\n 7 files changed, 178 insertions(+), 1 deletion(-)\n create mode 100755 t/perf/p5552-push-hidden-refs.sh\n\ndiff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc\nindex 285fb3809f..6e4c487d1e 100644\n--- a/Documentation/rev-list-options.adoc\n+++ b/Documentation/rev-list-options.adoc\n@@ -223,7 +223,10 @@ explicitly.\n \tPretend as if all objects mentioned as ref tips of alternate\n \trepositories were listed on the command line. An alternate\n \trepository is any repository whose object directory is specified\n-\tin `objects/info/alternates`. The set of included objects may\n+\tin `objects/info/alternates`. The repository's own object\n+\tdirectory does not count, even when it is passed in as an\n+\talternate, as happens inside the object quarantine of\n+\tlinkgit:git-receive-pack[1]. The set of included objects may\n \tbe modified by `core.alternateRefsCommand`, etc. See\n \tlinkgit:git-config[1].\n \ndiff --git a/odb.c b/odb.c\nindex 2c93a9490c..646a498052 100644\n--- a/odb.c\n+++ b/odb.c\n@@ -186,6 +186,9 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,\n \tsize_t base_len;\n \tstruct alternate_refs_data *cb = payload;\n \n+\tif (!alternate->borrowed)\n+\t\tgoto out;\n+\n \tif (!strbuf_realpath(&path, alternate->abspath, 0))\n \t\tgoto out;\n \tif (!strbuf_strip_suffix(&path, \"/objects\"))\ndiff --git a/odb/source-files.c b/odb/source-files.c\nindex 5b4277fbfd..223af45dd9 100644\n--- a/odb/source-files.c\n+++ b/odb/source-files.c\n@@ -3,6 +3,8 @@\n #include \"blob.h\"\n #include \"chdir-notify.h\"\n #include \"config.h\"\n+#include \"dir.h\"\n+#include \"environment.h\"\n #include \"gettext.h\"\n #include \"hex.h\"\n #include \"lockfile.h\"\n@@ -73,6 +75,7 @@ static void odb_source_files_free(struct odb_source *source)\n \t\tfiles->dirs = next;\n \t}\n \thashmap_clear(&files->dirs_by_path);\n+\tfree(files->quarantine_path);\n \n \todb_source_release(&files->base);\n \tfree(files);\n@@ -348,6 +351,45 @@ static int read_alternates(const char *object_dir, struct strvec *out)\n \treturn 0;\n }\n \n+/*\n+ * Our own object directory is /objects, or the parent of\n+ * GIT_QUARANTINE_PATH, as an object quarantine is created directly\n+ * inside it (see tmp_objdir_create()). Anything else, including a path\n+ * that cannot be resolved, is borrowed.\n+ */\n+static bool is_borrowed_object_dir(struct odb_source_files *files,\n+\t\t\t\t const char *path)\n+{\n+\tstruct strbuf real = STRBUF_INIT;\n+\tstruct strbuf objdir = STRBUF_INIT;\n+\tstruct strbuf self = STRBUF_INIT;\n+\tbool ret = true;\n+\n+\tif (!strbuf_realpath(&real, path, 0))\n+\t\tgoto out;\n+\n+\tstrbuf_addf(&objdir, \"%s/objects\", files->base.odb->repo->commondir);\n+\tif (strbuf_realpath(&self, objdir.buf, 0) &&\n+\t !fspathcmp(real.buf, self.buf)) {\n+\t\tret = false;\n+\t\tgoto out;\n+\t}\n+\n+\tif (files->quarantine_path &&\n+\t strbuf_realpath(&self, files->quarantine_path, 0)) {\n+\t\tstrbuf_strip_file_from_path(&self);\n+\t\tstrbuf_trim_trailing_dir_sep(&self);\n+\t\tif (!fspathcmp(real.buf, self.buf))\n+\t\t\tret = false;\n+\t}\n+\n+out:\n+\tstrbuf_release(&real);\n+\tstrbuf_release(&objdir);\n+\tstrbuf_release(&self);\n+\treturn ret;\n+}\n+\n static void odb_add_alternate_recursively(struct odb_source_files *files,\n \t\t\t\t\t const char *path,\n \t\t\t\t\t int depth)\n@@ -359,6 +401,7 @@ static void odb_add_alternate_recursively(struct odb_source_files *files,\n \t\tgoto out;\n \n \talternate = odb_files_dir_new(files->base.odb, path, false);\n+\talternate->borrowed = is_borrowed_object_dir(files, alternate->abspath);\n \n \t/* add the alternate entry */\n \t*files->dirs_tail = alternate;\n@@ -1256,15 +1299,19 @@ struct odb_source_files *odb_source_files_new(struct object_database *odb,\n \tstruct odb_source_files *files;\n \tchar *object_dir = NULL;\n \tchar *alternates = NULL;\n+\tchar *quarantine_path = NULL;\n \n \tif (flags & ODB_NEW_HONOR_ENV) {\n \t\tobject_dir = xstrdup_or_null(getenv(DB_ENVIRONMENT));\n \t\talternates = xstrdup_or_null(getenv(ALTERNATE_DB_ENVIRONMENT));\n+\t\tquarantine_path =\n+\t\t\txstrdup_or_null(getenv(GIT_QUARANTINE_ENVIRONMENT));\n \t}\n \tif (!object_dir)\n \t\tobject_dir = xstrfmt(\"%s/objects\", odb->repo->commondir);\n \n \tCALLOC_ARRAY(files, 1);\n+\tfiles->quarantine_path = quarantine_path;\n \todb_source_init(&files->base, odb, ODB_SOURCE_FILES, object_dir);\n \n \thashmap_init(&files->dirs_by_path, odb_files_dir_by_path_cmp, files, 0);\ndiff --git a/odb/source-files.h b/odb/source-files.h\nindex 837e890304..b404937fa1 100644\n--- a/odb/source-files.h\n+++ b/odb/source-files.h\n@@ -33,6 +33,15 @@ struct odb_files_dir {\n \t * repository. Directories added via alternates are not local.\n \t */\n \tbool local;\n+\n+\t/*\n+\t * Whether this directory is borrowed from another repository, so\n+\t * that its refs are alternate refs. Not every directory added via\n+\t * alternates is borrowed: inside an object quarantine, our own\n+\t * object directory is passed to us as an alternate, too. Only set\n+\t * for directories added via alternates.\n+\t */\n+\tbool borrowed;\n };\n \n struct odb_files_dir *odb_files_dir_new(struct object_database *odb,\n@@ -66,6 +75,12 @@ struct odb_source_files {\n \t * determined by \"core.ignoreCase\".\n \t */\n \tint dirs_paths_icase;\n+\n+\t/*\n+\t * The value of GIT_QUARANTINE_PATH when this source was set up from\n+\t * the environment (see ODB_NEW_HONOR_ENV), or NULL.\n+\t */\n+\tchar *quarantine_path;\n };\n \n /* Allocate and initialize a new object source. */\ndiff --git a/t/perf/p5552-push-hidden-refs.sh b/t/perf/p5552-push-hidden-refs.sh\nnew file mode 100755\nindex 0000000000..e9aa206298\n--- /dev/null\n+++ b/t/perf/p5552-push-hidden-refs.sh\n@@ -0,0 +1,37 @@\n+#!/bin/sh\n+\n+test_description='speed of push into a repository with many hidden refs'\n+. ./perf-lib.sh\n+\n+test_perf_fresh_repo\n+\n+test_expect_success 'set up repository with many hidden refs' '\n+\tgit init --bare dst.git &&\n+\ttest_commit_bulk --ref=refs/hidden/base --id=hidden 1000 &&\n+\tgit rev-list refs/hidden/base >commits &&\n+\tfor i in $(test_seq 200)\n+\tdo\n+\t\tsed \"s|^\\(.*\\)$|create refs/hidden/$i/\\1 \\1|\" commits || return 1\n+\tdone >input &&\n+\tgit push dst.git refs/hidden/base &&\n+\tgit -C dst.git update-ref --stdin >\"$(pwd)/alternate-refs.log\"\n+\tEOF\n+\tgit -C hidden.git config core.alternateRefsCommand ./alternate-refs &&\n+\ttest_commit hidden-push &&\n+\tgit push hidden.git HEAD:refs/heads/new &&\n+\ttest_path_is_missing alternate-refs.log\n+'\n+\n+test_expect_success 'quarantine lists refs of other alternates' '\n+\ttest_when_finished \"rm -f alternate-refs.log\" &&\n+\tgit init --bare other.git &&\n+\tgit push other.git HEAD:refs/heads/other &&\n+\techo \"$(pwd)/other.git/objects\" >hidden.git/objects/info/alternates &&\n+\ttest_commit hidden-push-2 &&\n+\tgit push hidden.git HEAD:refs/heads/new2 &&\n+\t(cd other.git && pwd -P) >expect &&\n+\tsort -u alternate-refs.log >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'rev-list --alternate-refs in a hook skips own refs' '\n+\ttest_when_finished \"rm -rf altref.git\" &&\n+\tgit init --bare altref.git &&\n+\tgit push altref.git HEAD:refs/heads/main &&\n+\ttest_hook -C altref.git pre-receive <<-\\EOF &&\n+\tgit rev-list --no-walk --alternate-refs --stdin ../hook-alternate-refs\n+\tEOF\n+\ttest_commit altref-push &&\n+\tgit push altref.git HEAD:refs/heads/new &&\n+\ttest_must_be_empty hook-alternate-refs\n+'\n+\n test_done\ndiff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\nindex 2fb6d549d3..d52e426db1 100755\n--- a/t/t5618-alternate-refs.sh\n+++ b/t/t5618-alternate-refs.sh\n@@ -58,4 +58,38 @@ test_expect_success 'log --source shows .alternate marker' '\n \ttest_cmp expect actual\n '\n \n+# Our own object directory as an alternate is not a source of alternate\n+# refs, but the alternates it brings in with it still are.\n+test_expect_success 'own object directory as alternate lists only real alternates' '\n+\ttest_when_finished \"rm -rf other-objects\" &&\n+\tmkdir other-objects &&\n+\tgit rev-list --remotes=origin >expect &&\n+\tGIT_OBJECT_DIRECTORY=\"$(pwd)/other-objects\" \\\n+\tGIT_ALTERNATE_OBJECT_DIRECTORIES=\"$(pwd)/.git/objects\" \\\n+\t\tgit rev-list --alternate-refs >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+# A quarantine is created inside our object directory, so its parent is\n+# ourselves, even when that is not under our commondir.\n+test_expect_success 'parent of the quarantine is not listed as an alternate' '\n+\ttest_when_finished \"rm -rf store.git\" &&\n+\tgit init --bare store.git &&\n+\tgit push store.git refs/remotes/origin/one:refs/heads/one &&\n+\tquarantine=\"$(pwd)/store.git/objects/tmp_objdir-test\" &&\n+\tmkdir \"$quarantine\" &&\n+\n+\tgit rev-parse origin/one >expect &&\n+\tGIT_OBJECT_DIRECTORY=\"$quarantine\" \\\n+\tGIT_ALTERNATE_OBJECT_DIRECTORIES=\"$(pwd)/store.git/objects\" \\\n+\t\tgit rev-list --no-walk --alternate-refs --stdin actual &&\n+\ttest_cmp expect actual &&\n+\n+\tGIT_QUARANTINE_PATH=\"$quarantine\" \\\n+\tGIT_OBJECT_DIRECTORY=\"$quarantine\" \\\n+\tGIT_ALTERNATE_OBJECT_DIRECTORIES=\"$(pwd)/store.git/objects\" \\\n+\t\tgit rev-list --no-walk --alternate-refs --stdin actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n\nbase-commit: ebdc087dc99d68bb340c282b2ba5f722def28d19\n-- \ngitgitgadget\n\n"},{"id":"557250","messageId":"xmqq1p92g375.fsf@gitster.g","threadId":"66692","inReplyTo":"pull.2285.v2.git.1793619933300.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-11-03T00:37:34Z","receivedAt":"2026-11-03T00:37:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristofer Karlsson via GitGitGadget\" \nwrites:\n\n> Changes since v1:\n> \n> * Changed the rule to \"an alternate that is ourselves is not a source\n> of alternate refs\", as Peff described it.\n> \n> * Added a \"borrowed\" flag to alternates, set when they are loaded,\n> which says whether they come from another repository, and used it for\n> --alternate-refs. Our own object directory is recognized either as\n> /objects or as the parent of GIT_QUARANTINE_PATH.\n> \n> * Documented this for --alternate-refs.\n> \n> * Added tests for rev-list --alternate-refs in a pre-receive hook, for\n> our own object directory set up as an alternate by hand, and for a\n> quarantine outside of the commondir.\n\nAll of the above changes make sense to me.\n\nThis unfortunately crashes with Patrick's odb-reload-alternates\nseries, but let me see if I can get a reasonable conflict resolution\nor if it is more reasonable to ask you to rebase this on the other\ntopic.\n\nThanks.\n\n"},{"id":"557253","messageId":"xmqqld7aen0n.fsf@gitster.g","threadId":"66692","inReplyTo":"xmqq1p92g375.fsf@gitster.g","subject":"Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-11-03T01:12:24Z","receivedAt":"2026-11-03T01:12:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Kristofer Karlsson via GitGitGadget\" \n> writes:\n>\n>> Changes since v1:\n>> ...\n>> quarantine outside of the commondir.\n>\n> All of the above changes make sense to me.\n>\n> This unfortunately crashes with Patrick's odb-reload-alternates\n> series, but let me see if I can get a reasonable conflict resolution\n> or if it is more reasonable to ask you to rebase this on the other\n> topic.\n\nI decided to queue it as-is instead of asking you to depend on the\nother topic. Here is what I got after making a trial merge to\n'seen'.\n\nI usually use \"git show\" to trigger the dense-combined diff format\n(i.e., \"git show --cc seen\"), but \"git show --remerge-diff seen\"\noutput is shown below, just to see how easy it is to read.\n\n\n\ncommit b7f4d1e66b034c2a64b7f3323f9cd156a028f080\nMerge: 1707829739 3f91b4bc4b\nAuthor: Junio C Hamano \nDate: Mon Nov 2 16:37:57 2026 -0800\n\n Merge branch 'kk/quarantine-alternate-refs' into seen\n \n The connectivity check in 'git-receive-pack(1)' has been optimized\n to not list the repository's own refs as alternate refs when running\n inside the object quarantine, avoiding redundant traversal of hidden\n refs.\n \n * kk/quarantine-alternate-refs:\n odb: do not list our own refs as alternate refs in the quarantine\n\ndiff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc\nremerge CONFLICT (content): Merge conflict in Documentation/rev-list-options.adoc\nindex 4e92cf6456..d20c5f6372 100644\n--- a/Documentation/rev-list-options.adoc\n+++ b/Documentation/rev-list-options.adoc\n@@ -223,23 +223,13 @@ explicitly.\n \tPretend as if all objects mentioned as ref tips of alternate\n \trepositories were listed on the command line. An alternate\n \trepository is any repository whose object directory is specified\n-<<<<<<< 1707829739 (Merge branch 'kk/connectivity-check-ignore-replace' into seen)\n \tin `objects/info/alternates`. The refs of shallow and grafted\n \talternate repositories are ignored, as the history behind them\n-\tmay be missing. The set of included objects may be modified by\n+\tmay be missing. The repository's own object directory does not\n+\tcount, even when it is passed in as an alternate, as happens\n+\tinside the object quarantine of\tlinkgit:git-receive-pack[1].\n+\tThe set of included objects may be modified by\n \t`core.alternateRefsCommand`, etc. See linkgit:git-config[1].\n-||||||| ebdc087dc9\n-\tin `objects/info/alternates`. The set of included objects may\n-\tbe modified by `core.alternateRefsCommand`, etc. See\n-\tlinkgit:git-config[1].\n-=======\n-\tin `objects/info/alternates`. The repository's own object\n-\tdirectory does not count, even when it is passed in as an\n-\talternate, as happens inside the object quarantine of\n-\tlinkgit:git-receive-pack[1]. The set of included objects may\n-\tbe modified by `core.alternateRefsCommand`, etc. See\n-\tlinkgit:git-config[1].\n->>>>>>> 3f91b4bc4b (odb: do not list our own refs as alternate refs in the quarantine)\n \n `--single-worktree`::\n \tBy default, all working trees will be examined by the\ndiff --git a/odb/source-files.c b/odb/source-files.c\nremerge CONFLICT (content): Merge conflict in odb/source-files.c\nindex 4239cb493c..8ce437fdc2 100644\n--- a/odb/source-files.c\n+++ b/odb/source-files.c\n@@ -75,15 +75,9 @@ static void odb_source_files_free(struct odb_source *source)\n \t\todb_files_dir_free(files->dirs);\n \t\tfiles->dirs = next;\n \t}\n-<<<<<<< 1707829739 (Merge branch 'kk/connectivity-check-ignore-replace' into seen)\n \tstrset_clear(&files->dir_paths);\n \tstat_validity_clear(&files->alternates_validity);\n-||||||| ebdc087dc9\n-\thashmap_clear(&files->dirs_by_path);\n-=======\n-\thashmap_clear(&files->dirs_by_path);\n \tfree(files->quarantine_path);\n->>>>>>> 3f91b4bc4b (odb: do not list our own refs as alternate refs in the quarantine)\n \n \todb_source_release(&files->base);\n \tfree(files);\n@@ -362,15 +356,6 @@ static void read_alternates(const char *object_dir, struct strvec *out,\n \tfree(path);\n }\n \n-<<<<<<< 1707829739 (Merge branch 'kk/connectivity-check-ignore-replace' into seen)\n-static void add_alternate_recursively(struct odb_source_files *files,\n-\t\t\t\t const char *path,\n-\t\t\t\t int depth)\n-||||||| ebdc087dc9\n-static void odb_add_alternate_recursively(struct odb_source_files *files,\n-\t\t\t\t\t const char *path,\n-\t\t\t\t\t int depth)\n-=======\n /*\n * Our own object directory is /objects, or the parent of\n * GIT_QUARANTINE_PATH, as an object quarantine is created directly\n@@ -410,10 +395,9 @@ static bool is_borrowed_object_dir(struct odb_source_files *files,\n \treturn ret;\n }\n \n-static void odb_add_alternate_recursively(struct odb_source_files *files,\n-\t\t\t\t\t const char *path,\n-\t\t\t\t\t int depth)\n->>>>>>> 3f91b4bc4b (odb: do not list our own refs as alternate refs in the quarantine)\n+static void add_alternate_recursively(struct odb_source_files *files,\n+\t\t\t\t const char *path,\n+\t\t\t\t int depth)\n {\n \tstruct odb_files_dir *alternate;\n \tstruct strvec alternates = STRVEC_INIT;\ndiff --git a/odb/source-files.h b/odb/source-files.h\nremerge CONFLICT (content): Merge conflict in odb/source-files.h\nindex 7afc2f28ad..9117dbea5b 100644\n--- a/odb/source-files.h\n+++ b/odb/source-files.h\n@@ -70,22 +70,16 @@ struct odb_source_files {\n \t * Whether directory paths shall be compared case-insensitively, as\n \t * determined by \"core.ignoreCase\".\n \t */\n-<<<<<<< 1707829739 (Merge branch 'kk/connectivity-check-ignore-replace' into seen)\n \tint dir_paths_icase;\n \n \t/* Stat data of the \"info/alternates\" file. */\n \tstruct stat_validity alternates_validity;\n-||||||| ebdc087dc9\n-\tint dirs_paths_icase;\n-=======\n-\tint dirs_paths_icase;\n \n \t/*\n \t * The value of GIT_QUARANTINE_PATH when this source was set up from\n \t * the environment (see ODB_NEW_HONOR_ENV), or NULL.\n \t */\n \tchar *quarantine_path;\n->>>>>>> 3f91b4bc4b (odb: do not list our own refs as alternate refs in the quarantine)\n };\n \n /* Allocate and initialize a new object source. */\ndiff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\nremerge CONFLICT (content): Merge conflict in t/t5618-alternate-refs.sh\nindex cefc37797c..048c7c63c1 100755\n--- a/t/t5618-alternate-refs.sh\n+++ b/t/t5618-alternate-refs.sh\n@@ -58,34 +58,6 @@ test_expect_success 'log --source shows .alternate marker' '\n \ttest_cmp expect actual\n '\n \n-<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 1707829739 (Merge branch 'kk/connectivity-check-ignore-replace' into seen)\n-test_expect_success 'connectivity check of a fetch excludes alternate refs' '\n-\tGIT_TRACE2_EVENT=\"$(pwd)/fetch.trace\" git fetch origin &&\n-\ttest_subcommand_flex git rev-list --objects --stdin --not --all \\\n-\t\t--alternate-refs --quiet >.git/objects/info/alternates &&\n-\tgit cat-file -e $shallow_tip &&\n-\tgit rev-list --remotes=origin >expect &&\n-\tgit rev-list --alternate-refs >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-|||||||||||||||||||||||||||||||| ebdc087dc9\n-================================\n # Our own object directory as an alternate is not a source of alternate\n # refs, but the alternates it brings in with it still are.\n test_expect_success 'own object directory as alternate lists only real alternates' '\n@@ -120,5 +92,29 @@ test_expect_success 'parent of the quarantine is not listed as an alternate' '\n \ttest_must_be_empty actual\n '\n \n->>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 3f91b4bc4b (odb: do not list our own refs as alternate refs in the quarantine)\n+test_expect_success 'connectivity check of a fetch excludes alternate refs' '\n+\tGIT_TRACE2_EVENT=\"$(pwd)/fetch.trace\" git fetch origin &&\n+\ttest_subcommand_flex git rev-list --objects --stdin --not --all \\\n+\t\t--alternate-refs --quiet >.git/objects/info/alternates &&\n+\tgit cat-file -e $shallow_tip &&\n+\tgit rev-list --remotes=origin >expect &&\n+\tgit rev-list --alternate-refs >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\n"},{"id":"557287","messageId":"CAL71e4PmDkhc_LX_efeS2RZuayjLX2ORiwfQBC4rxN7xSEgYvQ@mail.gmail.com","threadId":"66692","inReplyTo":"xmqqld7aen0n.fsf@gitster.g","subject":"Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Kristofer Karlsson","fromEmail":"krka@spotify.com","sentAt":"2026-11-03T10:19:06Z","receivedAt":"2026-11-03T10:19:06Z","isPatch":true,"sender":{"key":"krka@spotify.com","avatar":"https://avatars.githubusercontent.com/u/5349266?v=4"},"body":"On Tue, 3 Nov 2026 at 02:12, Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > \"Kristofer Karlsson via GitGitGadget\" \n> > writes:\n> >\n> >> Changes since v1:\n> >> ...\n> >> quarantine outside of the commondir.\n> >\n> > All of the above changes make sense to me.\n> >\n> > This unfortunately crashes with Patrick's odb-reload-alternates\n> > series, but let me see if I can get a reasonable conflict resolution\n> > or if it is more reasonable to ask you to rebase this on the other\n> > topic.\n>\n> I decided to queue it as-is instead of asking you to depend on the\n> other topic. Here is what I got after making a trial merge to\n> 'seen'.\n>\n\nThank you! I also read through Patrick's changes in\nps/odb-reload-alternates, and they appear to be logically\ncompatible. I couldn't spot any problematic interactions,\nand your conflict resolution looks correct to me.\n\nKristofer\n\n"},{"id":"557296","messageId":"aum-34I8KZvTfDRT@pks.im","threadId":"66692","inReplyTo":"pull.2285.v2.git.1793619933300.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-11-03T11:00:47Z","receivedAt":"2026-11-03T11:00:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Nov 02, 2026 at 11:45:33AM +0000, Kristofer Karlsson via GitGitGadget wrote:\n> diff --git a/odb/source-files.c b/odb/source-files.c\n> index 5b4277fbfd..223af45dd9 100644\n> --- a/odb/source-files.c\n> +++ b/odb/source-files.c\n> @@ -348,6 +351,45 @@ static int read_alternates(const char *object_dir, struct strvec *out)\n> \treturn 0;\n> }\n> \n> +/*\n> + * Our own object directory is /objects, or the parent of\n> + * GIT_QUARANTINE_PATH, as an object quarantine is created directly\n\nOr it may be derived from `files://` with the object storage\nformat extension.\n\n> + * inside it (see tmp_objdir_create()). Anything else, including a path\n> + * that cannot be resolved, is borrowed.\n> + */\n> +static bool is_borrowed_object_dir(struct odb_source_files *files,\n> +\t\t\t\t const char *path)\n> +{\n> +\tstruct strbuf real = STRBUF_INIT;\n> +\tstruct strbuf objdir = STRBUF_INIT;\n> +\tstruct strbuf self = STRBUF_INIT;\n> +\tbool ret = true;\n> +\n> +\tif (!strbuf_realpath(&real, path, 0))\n> +\t\tgoto out;\n> +\n> +\tstrbuf_addf(&objdir, \"%s/objects\", files->base.odb->repo->commondir);\n> +\tif (strbuf_realpath(&self, objdir.buf, 0) &&\n> +\t !fspathcmp(real.buf, self.buf)) {\n> +\t\tret = false;\n> +\t\tgoto out;\n> +\t}\n\nSo this here isn't fully correct as it does not account for all cases.\nWe'd have to read the object storage format extension to figure out\nwhere the primary object directory lives.\n\n> @@ -1256,15 +1299,19 @@ struct odb_source_files *odb_source_files_new(struct object_database *odb,\n> \tstruct odb_source_files *files;\n> \tchar *object_dir = NULL;\n> \tchar *alternates = NULL;\n> +\tchar *quarantine_path = NULL;\n> \n> \tif (flags & ODB_NEW_HONOR_ENV) {\n> \t\tobject_dir = xstrdup_or_null(getenv(DB_ENVIRONMENT));\n> \t\talternates = xstrdup_or_null(getenv(ALTERNATE_DB_ENVIRONMENT));\n> +\t\tquarantine_path =\n> +\t\t\txstrdup_or_null(getenv(GIT_QUARANTINE_ENVIRONMENT));\n> \t}\n> \tif (!object_dir)\n> \t\tobject_dir = xstrfmt(\"%s/objects\", odb->repo->commondir);\n> \n> \tCALLOC_ARRAY(files, 1);\n> +\tfiles->quarantine_path = quarantine_path;\n> \todb_source_init(&files->base, odb, ODB_SOURCE_FILES, object_dir);\n> \n> \thashmap_init(&files->dirs_by_path, odb_files_dir_by_path_cmp, files, 0);\n\nThis makes me wonder whether the better fix would be to stop modifying\nthe primary object directory and instead we simply append the quarantine\ndirectory to alternates. The presence of `GIT_QUARANTINE_ENVIRONMENT`\ncould then serve as an indicator that the child process is not allowed\nto write objects.\n\nThat's a much bigger change in semantics though, and the risk of causing\na regression would be quite a bit higher. But the upside is that we\nwouldn't have to juggle alternates anymore, and your case with alternate\nrefs would \"just work\".\n\nPatrick\n\n"},{"id":"557303","messageId":"CAL71e4McZzVJqFHnmpoQkC6+hUkhz9_HtEy7dqC5rDyXdV6oaQ@mail.gmail.com","threadId":"66692","inReplyTo":"aum-34I8KZvTfDRT@pks.im","subject":"Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Kristofer Karlsson","fromEmail":"krka@spotify.com","sentAt":"2026-11-03T12:05:29Z","receivedAt":"2026-11-03T12:05:29Z","isPatch":true,"sender":{"key":"krka@spotify.com","avatar":"https://avatars.githubusercontent.com/u/5349266?v=4"},"body":"On Tue, Nov 03, 2026 at 12:00 PM Patrick Steinhardt wrote:\n> So this here isn't fully correct as it does not account for all cases.\n> We'd have to read the object storage format extension to figure out\n> where the primary object directory lives.\n\nGood point. I can rework the patch to make it easier to integrate\nwith your ps/odb-object-storage-extension topic, by having the files\nsource remember its default object directory in\nfiles->default_object_dir where it decides it, and comparing against\nthat instead of hardcoding /objects in the check. Then\nyour topic only has to change how the default is decided, and the\ncheck follows. Something like this, if you want:\n\n--- a/odb/source-files.c\n+++ b/odb/source-files.c\n@@ static bool is_borrowed_object_dir(struct odb_source_files *files,\n- strbuf_addf(&objdir, \"%s/objects\", files->base.odb->repo->commondir);\n- if (strbuf_realpath(&self, objdir.buf, 0) &&\n+ if (strbuf_realpath(&self, files->default_object_dir, 0) &&\n !fspathcmp(real.buf, self.buf)) {\n@@ struct odb_source_files *odb_source_files_new(struct object_database *odb,\n+ default_object_dir = xstrfmt(\"%s/objects\", odb->repo->commondir);\n if (!object_dir)\n- object_dir = xstrfmt(\"%s/objects\", odb->repo->commondir);\n+ object_dir = xstrdup(default_object_dir);\n\n CALLOC_ARRAY(files, 1);\n+ files->default_object_dir = default_object_dir;\n\nEither way, the check should not make things worse where it is\nincomplete. A directory we fail to recognize as our own stays\nborrowed, so --alternate-refs lists its refs just like today. The\nopposite mistake would need a quarantine inside another repository's\nobject directory, which seems unlikely, and even then it would only\ncost some negative tips: the traversal walks further, but never\naccepts a missing object.\n\n> This makes me wonder whether the better fix would be to stop modifying\n> the primary object directory and instead we simply append the quarantine\n> directory to alternates.\n[...]\n\nThat does sound like the cleaner fix. Since you are already deep into\nthis area, I would be happy to drop the fix and wait for that instead.\nI expect very few users to be affected in practice: mostly git hosts\nwith many hidden refs, which is the case bcec6780b2 was about.\n\nIn that case I could send just the tests, with the hidden refs case\nmarked as test_expect_failure, so that it is covered once a long-term\nfix lands. Or, if you would rather have a short-term fix in the\nmeantime, I can send a v3 with the change above.\n\nThanks,\nKristofer\n\n"},{"id":"557305","messageId":"aunS6dE0B-JAJnBk@pks.im","threadId":"66692","inReplyTo":"CAL71e4McZzVJqFHnmpoQkC6+hUkhz9_HtEy7dqC5rDyXdV6oaQ@mail.gmail.com","subject":"Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-11-03T12:26:17Z","receivedAt":"2026-11-03T12:26:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Nov 03, 2026 at 01:05:29PM +0100, Kristofer Karlsson wrote:\n> On Tue, Nov 03, 2026 at 12:00 PM Patrick Steinhardt wrote:\n> > So this here isn't fully correct as it does not account for all cases.\n> > We'd have to read the object storage format extension to figure out\n> > where the primary object directory lives.\n> \n> Good point. I can rework the patch to make it easier to integrate\n> with your ps/odb-object-storage-extension topic, by having the files\n> source remember its default object directory in\n> files->default_object_dir where it decides it, and comparing against\n> that instead of hardcoding /objects in the check. Then\n> your topic only has to change how the default is decided, and the\n> check follows. Something like this, if you want:\n\nNote that this topic is already merged to \"next\", so in theory you could\nrebase your patch on top of it now.\n\n> > This makes me wonder whether the better fix would be to stop modifying\n> > the primary object directory and instead we simply append the quarantine\n> > directory to alternates.\n> [...]\n> \n> That does sound like the cleaner fix. Since you are already deep into\n> this area, I would be happy to drop the fix and wait for that instead.\n> I expect very few users to be affected in practice: mostly git hosts\n> with many hidden refs, which is the case bcec6780b2 was about.\n> \n> In that case I could send just the tests, with the hidden refs case\n> marked as test_expect_failure, so that it is covered once a long-term\n> fix lands. Or, if you would rather have a short-term fix in the\n> meantime, I can send a v3 with the change above.\n\nI certainly do care about this case and would like to see this fixed :)\nI'd like to get Justin's opinion on that proposal first though, as he's\nworked on transactions quite extensively over the last couple months.\n\nPatrick\n\n"},{"id":"557382","messageId":"aupPATAGBWH1RIe6@denethor","threadId":"66692","inReplyTo":"aunS6dE0B-JAJnBk@pks.im","subject":"Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-11-03T21:53:11Z","receivedAt":"2026-11-03T21:53:11Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 26/11/03 01:26PM, Patrick Steinhardt wrote:\n> On Tue, Nov 03, 2026 at 01:05:29PM +0100, Kristofer Karlsson wrote:\n> > That does sound like the cleaner fix. Since you are already deep into\n> > this area, I would be happy to drop the fix and wait for that instead.\n> > I expect very few users to be affected in practice: mostly git hosts\n> > with many hidden refs, which is the case bcec6780b2 was about.\n> > \n> > In that case I could send just the tests, with the hidden refs case\n> > marked as test_expect_failure, so that it is covered once a long-term\n> > fix lands. Or, if you would rather have a short-term fix in the\n> > meantime, I can send a v3 with the change above.\n> \n> I certainly do care about this case and would like to see this fixed :)\n> I'd like to get Justin's opinion on that proposal first though, as he's\n> worked on transactions quite extensively over the last couple months.\n\nRegarding the fix proposed in this patch, I'm not quite sure this is\nreally the right layer to address the problem. The root of the problem\nis that we are swapping around the \"real\" and quarantine object\ndirectories to facilitate writing to the \"files\" ODB transaction. IMO,\nthis is already rather fragile. So I do agree with Patrick that I think\nthe better long-term fix is to stop modifying the primary object\ndirectory altogether and instead propagate some context about the\npending transaction via environment variables to the child process that\nit can use to reconstruct the proper environment. This is certainly a\nmore involved change, but something that I've been wanting to do for a\nwhile now regardless.\n\nIf we are interested in a more simple fix for the short-term, we could\nadapt `odb_transaction_env()` to have a \"read-only\" mode flag that does\nnot modify the primary object directory and only appends the quarantine\ndirectory as an alternate. For the connectivity check in\ngit-receive-pack(1) we don't write any object anyways, so configuring\nthe environment in this manner should be sufficient to fix the issue.\n\n-Justin\n\n"},{"id":"557395","messageId":"20261104024806.GA1492379@coredump.intra.peff.net","threadId":"66692","inReplyTo":"pull.2285.v2.git.1793619933300.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-11-04T02:48:06Z","receivedAt":"2026-11-04T02:48:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 02, 2026 at 11:45:33AM +0000, Kristofer Karlsson via GitGitGadget wrote:\n\n> An alternate that is ourselves is not a source of alternate refs: its\n> refs are our own refs, which the caller already lists as it sees fit.\n> Mark each object directory added via alternates with whether it is\n> borrowed from another repository, and list alternate refs only from\n> those. Within one process this needs no guessing, as tmp-objdir\n> links our own object directory in itself. But a child process rebuilds\n> its object store from the environment, so when an alternate is added,\n> recognize ourselves in two ways:\n> \n> - as our /objects, which also covers an environment that\n> makes our own object directory an alternate by hand;\n> \n> - as the parent of GIT_QUARANTINE_PATH, as the quarantine is created\n> directly inside our object directory (see \"QUARANTINE ENVIRONMENT\"\n> in git-receive-pack(1)); this also works when the object directory\n> is not under the commondir.\n\nSo as I said before, I think this is a reasonable fix regardless of the\nhow quarantine issues work. We should not be pulling in alternate ref\nsources that duplicate each other or our main repo.\n\nBut I was surprised to see special handling of GIT_QUARANTINE_PATH here.\nI think this is a defect in the alternate-ref code. Yes, the common case\nis triggering it with our tmp-objdir shenanigans, but if we fix the\nunderlying issue, then the use of it here should fix the quarantine\nissue automatically.\n\nI.e., I'd expect the code part of the patch to just be:\n\ndiff --git a/odb.c b/odb.c\nindex 2c93a9490c..f9ab5c8e7f 100644\n--- a/odb.c\n+++ b/odb.c\n@@ -183,6 +183,7 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,\n \t\t\t\t void *payload)\n {\n \tstruct strbuf path = STRBUF_INIT;\n+\tstruct strbuf repo = STRBUF_INIT;\n \tsize_t base_len;\n \tstruct alternate_refs_data *cb = payload;\n \n@@ -198,10 +199,16 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,\n \t\tgoto out;\n \tstrbuf_setlen(&path, base_len);\n \n+\t/* XXX could realpath once in caller as a mild optimization */\n+\tif (strbuf_realpath(&repo, cb->repo->commondir, 0) &&\n+\t !strcmp(path.buf, repo.buf))\n+\t\tgoto out;\n+\n \tread_alternate_refs(cb->repo, path.buf, cb->fn, cb->payload);\n \n out:\n \tstrbuf_release(&path);\n+\tstrbuf_release(&repo);\n \treturn 0;\n }\n \n\nand keep it totally local to the path hackery in the alternate-refs\npath.\n\nOr am I missing something that requires more complication?\n\n> This is safe: alternate refs only serve as negative tips that are\n> assumed to be complete, so dropping some can only make the traversal\n> walk further, never make it accept a missing object.\n\nThat's how _we_ use them in the internal callers, but it's a rev-list\noption. So we can't say how they are used elsewhere. That said, I think\nthis is a totally reasonable tightening of the definition for general\nuse.\n\n-Peff\n\n"},{"id":"557421","messageId":"CAL71e4OreARF+dbCU3S8-_umrFEptPLPUiPDH3SYBx6OYjVpfg@mail.gmail.com","threadId":"66692","inReplyTo":"20261104024806.GA1492379@coredump.intra.peff.net","subject":"Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Kristofer Karlsson","fromEmail":"krka@spotify.com","sentAt":"2026-11-04T10:25:38Z","receivedAt":"2026-11-04T10:25:38Z","isPatch":true,"sender":{"key":"krka@spotify.com","avatar":"https://avatars.githubusercontent.com/u/5349266?v=4"},"body":"On Wed, 4 Nov 2026 at 03:48, Jeff King wrote:\n>\n> I.e., I'd expect the code part of the patch to just be:\n>\n> diff --git a/odb.c b/odb.c\n> index 2c93a9490c..f9ab5c8e7f 100644\n> --- a/odb.c\n> +++ b/odb.c\n> @@ -183,6 +183,7 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,\n> void *payload)\n> {\n> struct strbuf path = STRBUF_INIT;\n> + struct strbuf repo = STRBUF_INIT;\n> size_t base_len;\n> struct alternate_refs_data *cb = payload;\n>\n> @@ -198,10 +199,16 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,\n> goto out;\n> strbuf_setlen(&path, base_len);\n>\n> + /* XXX could realpath once in caller as a mild optimization */\n> + if (strbuf_realpath(&repo, cb->repo->commondir, 0) &&\n> + !strcmp(path.buf, repo.buf))\n> + goto out;\n> +\n> read_alternate_refs(cb->repo, path.buf, cb->fn, cb->payload);\n>\n> out:\n> strbuf_release(&path);\n> + strbuf_release(&repo);\n> return 0;\n> }\n>\n>\n> and keep it totally local to the path hackery in the alternate-refs\n> path.\n>\n> Or am I missing something that requires more complication?\n\nNo, I think you are right. The quarantine parent only matters when\nthe object directory is not under the commondir, which is rare. Your\nversion is essentially my v1 without the \"this is a hack\" framing\n(and also cleaner).\n\n> > This is safe: alternate refs only serve as negative tips that are\n> > assumed to be complete, so dropping some can only make the traversal\n> > walk further, never make it accept a missing object.\n>\n> That's how _we_ use them in the internal callers, but it's a rev-list\n> option. So we can't say how they are used elsewhere. That said, I think\n> this is a totally reasonable tightening of the definition for general\n> use.\n\nGood point. I will reword that part of the commit message so that it\ndoes not rely on how the connectivity check uses them. It also seems\nin line with what we already do when loading alternates, where\nlisting the object directory itself is ignored as a common mistake;\ninside the quarantine, that check just compares against the\nquarantine instead of our own object directory.\n\nJustin suggested fixing it at the transaction layer instead, so I will\nwait to see where that discussion lands before sending a v3.\n\nIt looks like the remaining options are:\n 1. Go with your proposed version.\n 2. Do nothing until the long term fix.\n 3. Just add a regression test for now.\n\nThanks,\nKristofer\n\n"},{"id":"557423","messageId":"CAL71e4M0t2ZgbqOd4MXeX3vH2W7pPRSMg+8i8epXYbh5-5t8Hw@mail.gmail.com","threadId":"66692","inReplyTo":"aupPATAGBWH1RIe6@denethor","subject":"Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Kristofer Karlsson","fromEmail":"krka@spotify.com","sentAt":"2026-11-04T10:32:53Z","receivedAt":"2026-11-04T10:32:53Z","isPatch":true,"sender":{"key":"krka@spotify.com","avatar":"https://avatars.githubusercontent.com/u/5349266?v=4"},"body":"On Tue, 3 Nov 2026 at 22:53, Justin Tobler wrote:\n>\n> Regarding the fix proposed in this patch, I'm not quite sure this is\n> really the right layer to address the problem. The root of the problem\n> is that we are swapping around the \"real\" and quarantine object\n> directories to facilitate writing to the \"files\" ODB transaction. IMO,\n> this is already rather fragile. So I do agree with Patrick that I think\n> the better long-term fix is to stop modifying the primary object\n> directory altogether and instead propagate some context about the\n> pending transaction via environment variables to the child process that\n> it can use to reconstruct the proper environment. This is certainly a\n> more involved change, but something that I've been wanting to do for a\n> while now regardless.\n>\n> If we are interested in a more simple fix for the short-term, we could\n> adapt `odb_transaction_env()` to have a \"read-only\" mode flag that does\n> not modify the primary object directory and only appends the quarantine\n> directory as an alternate. For the connectivity check in\n> git-receive-pack(1) we don't write any object anyways, so configuring\n> the environment in this manner should be sufficient to fix the issue.\n\nI agree with that long-term direction. What do you think we should\ndo for the short term? I see a few options:\n 1. A smaller, more focused patch in the alternate refs code, as Peff\n suggested in the other message.\n 2. Your read-only transaction environment for the connectivity check,\n (if you want to take that on, I would probably mess it up :-) ).\n 3. Only a regression test for now, with the hidden refs case marked as\n test_expect_failure, so that we do not lose track of it.\n 4. Or nothing, and wait for the long-term fix, as nobody seems to be\n hit hard by this at the moment.\n\nThanks,\nKristofer\n\n"},{"id":"557465","messageId":"autiszIPCf4Aes9T@denethor","threadId":"66692","inReplyTo":"CAL71e4M0t2ZgbqOd4MXeX3vH2W7pPRSMg+8i8epXYbh5-5t8Hw@mail.gmail.com","subject":"Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-11-04T16:58:12Z","receivedAt":"2026-11-04T16:58:12Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 26/11/04 11:32AM, Kristofer Karlsson wrote:\n> I agree with that long-term direction. What do you think we should\n> do for the short term? I see a few options:\n> 1. A smaller, more focused patch in the alternate refs code, as Peff\n> suggested in the other message.\n> 2. Your read-only transaction environment for the connectivity check,\n> (if you want to take that on, I would probably mess it up :-) ).\n> 3. Only a regression test for now, with the hidden refs case marked as\n> test_expect_failure, so that we do not lose track of it.\n> 4. Or nothing, and wait for the long-term fix, as nobody seems to be\n> hit hard by this at the moment.\n\nIMO, I think the patch suggested by Peff is a reasonably fix for this\nproblem. Later down the line I will probably also revisit how \"files\"\nODB transactions manage the object directory, but there are some other\ntransaction related topics I'm hoping to land first.\n\nThanks,\n-Justin\n\n"},{"id":"557479","messageId":"xmqq4idw320z.fsf@gitster.g","threadId":"66692","inReplyTo":"CAL71e4OreARF+dbCU3S8-_umrFEptPLPUiPDH3SYBx6OYjVpfg@mail.gmail.com","subject":"Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-11-04T18:05:48Z","receivedAt":"2026-11-04T18:05:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristofer Karlsson writes:\n\n>> That's how _we_ use them in the internal callers, but it's a rev-list\n>> option. So we can't say how they are used elsewhere. That said, I think\n>> this is a totally reasonable tightening of the definition for general\n>> use.\n>\n> Good point. I will reword that part of the commit message so that it\n> does not rely on how the connectivity check uses them. It also seems\n> in line with what we already do when loading alternates, where\n> listing the object directory itself is ignored as a common mistake;\n> inside the quarantine, that check just compares against the\n> quarantine instead of our own object directory.\n\nSounds very good.\nThanks for coming up with a good way forward.\n\n"}]}