odb: do not list our own refs as alternate refs in the quarantine

From: Kristofer Karlsson <[email protected]>

Since 722ff7f876 (receive-pack: quarantine objects until pre-receive accepts, 2016-10-03), receive-pack writes incoming objects to a temporary object directory. Child processes see it as their primary object directory, and tmp-objdir passes the repository's own object directory to them via GIT_ALTERNATE_OBJECT_DIRECTORIES.

The connectivity check passes --alternate-refs to rev-list since 39b44ba771 (check_everything_connected: assume alternate ref tips are valid, 2019-07-01). Inside the quarantine, the repository's own object directory is one of the alternates, so --alternate-refs runs for-each-ref on the repository itself and lists all of its refs.

That includes hidden refs. bcec6780b2 (receive-pack: only use visible refs for connectivity check, 2022-11-17) added --exclude-hidden=receive to the check so that repositories with many hidden refs do not have to load all of them on each push. The hidden refs are excluded from --all, but they all come back through --alternate-refs.

Skip an alternate whose repository is our own when listing alternate refs. This is done by comparing paths, which is a hack: it is mainly here to measure the cost of the bug, and a proper fix would have the quarantine tell its children which directory is ours. It is safe,
though: alternate refs only serve as negative tips that are assumed to be complete, so dropping some can only make the traversal walk further, never make it accept a missing object.

Add a test that our own refs are no longer listed, a test that the refs of a real alternate still are, and a perf test pushing a single commit into a repository with a visible branch and about 200,000 refs hidden by receive.hideRefs, and into a copy of it that hides nothing:

    Test                           HEAD~1            HEAD
    -----------------------------------------------------------------------
    5552.2: push, hidden refs      0.31(0.32+0.08)   0.10(0.06+0.03) -67.7%
    5552.3: push, no hidden refs   1.25(1.08+0.68)   1.03(0.84+0.62) -17.6%

With hidden refs, most of the time went to the for-each-ref child that listed the alternate refs. Without hidden refs, every ref was listed twice, once for --all and once for --alternate-refs.

---
receive-pack: connectivity check does not exclude hidden refs in the quarantine

This is mainly a bug report, in patch form so that it comes with a test and numbers. I don't expect it to be merged, at least not in this form.

When a repository hides refs with receive.hideRefs, every push still loads all of the hidden refs in the connectivity check, even though bcec6780b2 (receive-pack: only use visible refs for connectivity check, 2022-11-17) excludes them from it. With many hidden refs, that is most of the time spent on a small push.

I stumbled across this while working on the boundary walk of the connectivity check (the follow-up to kk/connectivity-check-incremental), when the walk loaded every ref during a push even though it should only have looked at a few of them.

The cause is that receive-pack runs the check inside the object quarantine, and there the repository's own object directory is passed to rev-list as an alternate. --alternate-refs then lists all of the repository's own refs, hidden ones included. Without hidden refs, every ref is listed twice instead. Setting core.alternateRefsPrefixes to the visible namespaces works around it, though that also limits which refs of real alternates are used.

The patch includes a small fix by comparing paths, mostly so that the perf test can show the difference (about 0.31s -> 0.10s for a push with 200,000 hidden refs). I am not really happy with it as a proper fix though.

I also tried resolving the object directories that --alternate-refs uses before the quarantine is created. That got a bit messy: it needed a new internal rev-list option, and receive-pack had to carry the list of directories through to each of its connectivity checks. And it only fixes the check itself; other processes running inside the quarantine, like pre-receive hooks, would still see the repository as its own alternate.

So the question is really where this should be fixed: is there a better way for processes running inside the quarantine to tell the repository's own object directory apart from real alternates?

I am likely not the best person to answer that. Patrick, maybe you could take a look, since you have recently reworked tmp-objdir and the odb sources?

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2285%2Fspkrka%2Fquarantine-altrefs-bugreport-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2285/spkrka/quarantine-altrefs-bugreport-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2285

 odb.c                            | 16 ++++++++++++++
 t/perf/p5552-push-hidden-refs.sh | 37 ++++++++++++++++++++++++++++++++
 t/t5547-push-quarantine.sh       | 25 +++++++++++++++++++++
 3 files changed, 78 insertions(+)
 create mode 100755 t/perf/p5552-push-hidden-refs.sh

index 2c93a9490c..0cda333c4e 100644
@@ -183,6 +183,7 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,
 				  void *payload)
 {
 	struct strbuf path = STRBUF_INIT;
+	struct strbuf self = STRBUF_INIT;
 	size_t base_len;
 	struct alternate_refs_data *cb = payload;
 

@@ -190,6 +191,20 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,
 		goto out;
 	if (!strbuf_strip_suffix(&path, "/objects"))
 		goto out;
+
+	/*
+	 * NEEDSWORK: this is a hack, mainly here to measure the cost of
+	 * the bug. Inside an object quarantine, tmp-objdir passes our own
+	 * object directory to child processes as an alternate, so we would
+	 * list our own refs as alternate refs, including the hidden refs
+	 * that the connectivity check of receive-pack deliberately skips.
+	 * Comparing paths papers over that; the quarantine should rather
+	 * tell its children which directory is ours.
+	 */
+	if (cb->repo->commondir &&
+	    strbuf_realpath(&self, cb->repo->commondir, 0) &&
+	    !strcmp(path.buf, self.buf))
+		goto out;
 	base_len = path.len;
 
 	/* Is this a git repository with refs? */

@@ -202,6 +217,7 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,
 
 out:
 	strbuf_release(&path);
+	strbuf_release(&self);
 	return 0;
 }
 

new file mode 100755
index 0000000000..e9aa206298
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='speed of push into a repository with many hidden refs'
+. ./perf-lib.sh
+
+test_perf_fresh_repo
+
+test_expect_success 'set up repository with many hidden refs' '
+	git init --bare dst.git &&
+	test_commit_bulk --ref=refs/hidden/base --id=hidden 1000 &&
+	git rev-list refs/hidden/base >commits &&
+	for i in $(test_seq 200)
+	do
+		sed "s|^\(.*\)$|create refs/hidden/$i/\1 \1|" commits || return 1
+	done >input &&
+	git push dst.git refs/hidden/base &&
+	git -C dst.git update-ref --stdin <input &&
+	git -C dst.git pack-refs --all &&
+	cp -R dst.git dst-visible.git &&
+	git -C dst.git config receive.hideRefs refs/hidden &&
+	git symbolic-ref HEAD refs/heads/main &&
+	test_commit_bulk --id=visible 1 &&
+	git push dst.git main &&
+	git push dst-visible.git main
+'
+
+test_perf 'push, hidden refs' '
+	git commit --allow-empty -m new &&
+	git push -f dst.git HEAD:refs/heads/new
+'
+
+test_perf 'push, no hidden refs' '
+	git commit --allow-empty -m new &&
+	git push -f dst-visible.git HEAD:refs/heads/new
+'
+
+test_done

index 1b7097179e..855a1d2c0a 100755
@@ -101,4 +101,29 @@ test_expect_success '.keep file is removed after push' '
 	test_path_is_missing "$keep"
 '
 
+test_expect_success 'quarantine does not list own refs as alternate refs' '
+	git init --bare hidden.git &&
+	git push hidden.git HEAD:refs/heads/main HEAD:refs/hidden/one &&
+	git -C hidden.git config receive.hideRefs refs/hidden &&
+	write_script hidden.git/alternate-refs <<-EOF &&
+	echo "\$1" >>"$(pwd)/alternate-refs.log"
+	EOF
+	git -C hidden.git config core.alternateRefsCommand ./alternate-refs &&
+	test_commit hidden-push &&
+	git push hidden.git HEAD:refs/heads/new &&
+	test_path_is_missing alternate-refs.log
+'
+
+test_expect_success 'quarantine lists refs of other alternates' '
+	test_when_finished "rm -f alternate-refs.log" &&
+	git init --bare other.git &&
+	git push other.git HEAD:refs/heads/other &&
+	echo "$(pwd)/other.git/objects" >hidden.git/objects/info/alternates &&
+	test_commit hidden-push-2 &&
+	git push hidden.git HEAD:refs/heads/new2 &&
+	(cd other.git && pwd -P) >expect &&
+	sort -u alternate-refs.log >actual &&
+	test_cmp expect actual
+'
+
 test_done

base-commit: 37b3475fb530c14109211faca33716a173db61ed

gitgitgadget

That is quite interesting. So IIUC, because the "real" ODB is an alternate, all the references from the main repository are marked as uninteresting even though they have already been processed. For the connectivity check performed by git-receive-pack(1), there isn't any reason to consider "alternate refs" from the "real" ODB that is configured as an alternate.

So we effectively consider all the references a second time, but always all of them including the previously excluded hidden refs. Yikes.

This indeed sounds like a bit of a hack. To me the underlying issue here is that downstream git processes don't really have any concept of an ODB transaction. We rely on the git alternates mechanism and overriding the primary object directory to get the unaware child processes to behave how we want. This typically means setting the quarantine directory as the main object directory and the "real" object directory as an alternate via environment variables. That way any writes that are performed by a child process are written to the quarantine.

For the connectivity check though, we don't really expect git-rev-list(1) to write any objects. Consequently, I don't think we neccessary need to set the quarantine directory as the primary object directory here. That would avoid git-rev-list(1) processing the "real" object directory references a second time.

[snip]

For the connectivity check performed via git-rev-list(1) it certainly matters, but I do wonder though for other processes if this really changes anything for them in practice. It could potentially be nice if there was a mechanism to more explicitly propagate a pending transaction accross process boundaries, but I'm not sure there are many usecases for that at present.

Building off of my previous thoughts, maybe it would make sense to make the connectivity check a more explicit part of the ODB transaction interface? Maybe for the "files" backend we could introduce a odb_transaction_files_check_connected() callback that configures git-rev-list(1) with the appropriate environment to validate the quarantined objects and not cause this bug? Happy to explore this path further if it seems reasonable.

-Justin

The connectivity check explicitly disables lazy fetching so it should be safe in practice, but it's hard to reason about potential regressions and having a more strict flow for this would be nice perhaps.

I am somewhat of a novice here, but I had a similar thought, that this should ideally be fixed by tweaking the API somehow, although I do not know exactly. I am not sure if we should introduce odb_transaction_files_check_connected() or resolve the alternate-refs into fixed directories earlier in the chain, or enforce better read-only mode (and add quarantine as a pure alternate for the connectivity check).

So, I am not sure what the long term fix is, but I think your idea makes sense for that. For the short term, I think something closer to what Junio suggested in the other reply is a reasonable way forward.

Thanks,
Kristofer

"Kristofer Karlsson via GitGitGadget" <[email protected]>

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.

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.

That said, if you think this is over-simplifying it, I can pivot back to introducing a new environment variable instead.

Thanks!
Kristofer

Well, as Patrick said elsewhere recently, the entire "since we are borrowing from this alternate object store, it would be nice if we can assume there is some ref storage that tells us which objects in it are 'complete', but because the alternate object store interface does not define such a set of refs associated with it, let's just assume the refs/ directory sitting immediately next to the objects/ directory is such a hierarchy" is an icky hack that is crumbling, given that the alternate object store may be a part of a repository that uses reftable. It also is troublesome to "borrow" from a shallow clone where their grafts are part of the mechanism they use to avoid stepping outside the set of available objects, which the alternate object store mechanism does not "borrow", as far as I recall.

So until we figure out how to fix these icky points, it probably is not a huge deal to pile on another icky assumption as a short-term measure.

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

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

From: Kristofer Karlsson <[email protected]>

Since 722ff7f876 (receive-pack: quarantine objects until pre-receive accepts, 2016-10-03), receive-pack writes incoming objects to a temporary object directory. Child processes see it as their primary object directory, and tmp-objdir passes the repository's own object directory to them via GIT_ALTERNATE_OBJECT_DIRECTORIES.

The connectivity check passes --alternate-refs to rev-list since 39b44ba771 (check_everything_connected: assume alternate ref tips are valid, 2019-07-01). Inside the quarantine, the repository's own object directory is one of the alternates, so --alternate-refs runs for-each-ref on the repository itself and lists all of its refs.

That includes hidden refs. bcec6780b2 (receive-pack: only use visible refs for connectivity check, 2022-11-17) added --exclude-hidden=receive to the check so that repositories with many hidden refs do not have to load all of them on each push. The hidden refs are excluded from --all, but they all come back through --alternate-refs.

An alternate that is ourselves is not a source of alternate refs: its refs are our own refs, which the caller already lists as it sees fit. Mark each object directory added via alternates with whether it is borrowed from another repository, and list alternate refs only from those. Within one process this needs no guessing, as tmp-objdir links our own object directory in itself. But a child process rebuilds its object store from the environment, so when an alternate is added, recognize ourselves in two ways:

- as our <commondir>/objects, which also covers an environment that makes our own object directory an alternate by hand;

- as the parent of GIT_QUARANTINE_PATH, as the quarantine is created directly inside our object directory (see "QUARANTINE ENVIRONMENT" in git-receive-pack(1)); this also works when the object directory is not under the commondir.

Document in --alternate-refs that our own object directory does not count as an alternate repository.

This is safe: alternate refs only serve as negative tips that are assumed to be complete, so dropping some can only make the traversal walk further, never make it accept a missing object.

Add tests that our own refs are no longer listed, by the connectivity check, by rev-list run from a pre-receive hook, by rev-list with our own object directory passed in as an alternate by hand, and by rev-list in a quarantine whose parent is not under the commondir, and that the refs of a real alternate still are. Add a perf test pushing a single commit into a repository with a visible branch and about 200,000 refs hidden by receive.hideRefs, and into a copy of it that hides nothing:

    Test                           HEAD~1            HEAD
    -----------------------------------------------------------------------
    5552.2: push, hidden refs      0.31(0.33+0.08)   0.09(0.06+0.03) -71.0%
    5552.3: push, no hidden refs   1.23(1.06+0.68)   1.01(0.83+0.61) -17.9%

With hidden refs, most of the time went to the for-each-ref child that listed the alternate refs. Without hidden refs, every ref was listed twice, once for --all and once for --alternate-refs.

---
odb: do not list our own refs as alternate refs in the quarantine

When a repository hides refs with receive.hideRefs, every push still loads all of the hidden refs in the connectivity check, even though bcec6780b2 (receive-pack: only use visible refs for connectivity check, 2022-11-17) excludes them from it. With many hidden refs, that is most of the time spent on a small push.

I stumbled across this while working on the boundary walk of the connectivity check (the follow-up to kk/connectivity-check-incremental), when the walk loaded every ref during a push even though it should only have looked at a few of them.

Changes since v1:

* Changed the rule to "an alternate that is ourselves is not a source of alternate refs", as Peff described it.

* Added a "borrowed" flag to alternates, set when they are loaded, which says whether they come from another repository, and used it for --alternate-refs. Our own object directory is recognized either as /objects or as the parent of GIT_QUARANTINE_PATH.

* Documented this for --alternate-refs.

* Added tests for rev-list --alternate-refs in a pre-receive hook, for our own object directory set up as an alternate by hand, and for a quarantine outside of the commondir.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2285%2Fspkrka%2Fquarantine-altrefs-bugreport-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2285/spkrka/quarantine-altrefs-bugreport-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2285

Range-diff vs v1:

1: 1b1de6eee1 ! 1: bac15042c0 odb: do not list our own refs as alternate refs in the quarantine
@@ Commit message
load all of them on each push. The hidden refs are excluded from --all, but they all come back through --alternate-refs.

- Skip an alternate whose repository is our own when listing alternate
- refs. This is done by comparing paths, which is a hack: it is mainly
- here to measure the cost of the bug, and a proper fix would have the
- quarantine tell its children which directory is ours. It is safe,
- though: alternate refs only serve as negative tips that are assumed to
- be complete, so dropping some can only make the traversal walk further,
- never make it accept a missing object.
-
- Add a test that our own refs are no longer listed, a test that the refs
- of a real alternate still are, and a perf test pushing a single commit
- into a repository with a visible branch and about 200,000 refs hidden
- by receive.hideRefs, and into a copy of it that hides nothing:
+ An alternate that is ourselves is not a source of alternate refs: its
+ refs are our own refs, which the caller already lists as it sees fit.
+ Mark each object directory added via alternates with whether it is
+ borrowed from another repository, and list alternate refs only from
+ those. Within one process this needs no guessing, as tmp-objdir
+ links our own object directory in itself. But a child process rebuilds
+ its object store from the environment, so when an alternate is added,
+ recognize ourselves in two ways:
+
+ - as our <commondir>/objects, which also covers an environment that
+ makes our own object directory an alternate by hand;
+
+ - as the parent of GIT_QUARANTINE_PATH, as the quarantine is created
+ directly inside our object directory (see "QUARANTINE ENVIRONMENT"
+ in git-receive-pack(1)); this also works when the object directory
+ is not under the commondir.
+
+ Document in --alternate-refs that our own object directory does not
+ count as an alternate repository.
+
+ This is safe: alternate refs only serve as negative tips that are
+ assumed to be complete, so dropping some can only make the traversal
+ walk further, never make it accept a missing object.
+
+ Add tests that our own refs are no longer listed, by the connectivity
+ check, by rev-list run from a pre-receive hook, by rev-list with our
+ own object directory passed in as an alternate by hand, and by rev-list
+ in a quarantine whose parent is not under the commondir, and that the
+ refs of a real alternate still are. Add a perf test pushing a
+ single commit into a repository with a visible branch and about
+ 200,000 refs hidden by receive.hideRefs, and into a copy of it that
+ hides nothing:

Test HEAD~1 HEAD
-----------------------------------------------------------------------
- 5552.2: push, hidden refs 0.31(0.32+0.08) 0.10(0.06+0.03) -67.7%
- 5552.3: push, no hidden refs 1.25(1.08+0.68) 1.03(0.84+0.62) -17.6%
+ 5552.2: push, hidden refs 0.31(0.33+0.08) 0.09(0.06+0.03) -71.0%
+ 5552.3: push, no hidden refs 1.23(1.06+0.68) 1.01(0.83+0.61) -17.9%

With hidden refs, most of the time went to the for-each-ref child that listed the alternate refs. Without hidden refs, every ref was listed twice, once for --all and once for --alternate-refs.

+ Helped-by: Jeff King <[email protected]>

+ ## Documentation/rev-list-options.adoc ##
+@@ Documentation/rev-list-options.adoc: explicitly.
+ Pretend as if all objects mentioned as ref tips of alternate
+ repositories were listed on the command line. An alternate
+ repository is any repository whose object directory is specified
+- in objects/info/alternates. The set of included objects may
++ in objects/info/alternates. The repository's own object
++ directory does not count, even when it is passed in as an
++ alternate, as happens inside the object quarantine of
++ linkgit:git-receive-pack[1]. The set of included objects may
+ be modified by core.alternateRefsCommand, etc. See
+ linkgit:git-config[1].
+
+
## odb.c ##
@@ odb.c: static int refs_from_alternate_cb(struct odb_files_dir *alternate,
- void *payload)
- {
- struct strbuf path = STRBUF_INIT;
-+ struct strbuf self = STRBUF_INIT;
size_t base_len;
struct alternate_refs_data *cb = payload;

-@@ odb.c: static int refs_from_alternate_cb(struct odb_files_dir *alternate,
++ if (!alternate->borrowed)
++ goto out;
++
+ if (!strbuf_realpath(&path, alternate->abspath, 0))
goto out;
if (!strbuf_strip_suffix(&path, "/objects"))
+
+ ## odb/source-files.c ##
+@@
+ #include "blob.h"
+ #include "chdir-notify.h"
+ #include "config.h"
++#include "dir.h"
++#include "environment.h"
+ #include "gettext.h"
+ #include "hex.h"
+ #include "lockfile.h"
+@@ odb/source-files.c: static void odb_source_files_free(struct odb_source *source)
+ files->dirs = next;
+ }
+ hashmap_clear(&files->dirs_by_path);
++ free(files->quarantine_path);
+
+ odb_source_release(&files->base);
+ free(files);
+@@ odb/source-files.c: static int read_alternates(const char *object_dir, struct strvec *out)
+ return 0;
+ }
+
++/*
++ * Our own object directory is <commondir>/objects, or the parent of
++ * GIT_QUARANTINE_PATH, as an object quarantine is created directly
++ * inside it (see tmp_objdir_create()). Anything else, including a path
++ * that cannot be resolved, is borrowed.
++ */
++static bool is_borrowed_object_dir(struct odb_source_files *files,
++ const char *path)
++{
++ struct strbuf real = STRBUF_INIT;
++ struct strbuf objdir = STRBUF_INIT;
++ struct strbuf self = STRBUF_INIT;
++ bool ret = true;
++
++ if (!strbuf_realpath(&real, path, 0))
++ goto out;
++
++ strbuf_addf(&objdir, "%s/objects", files->base.odb->repo->commondir);
++ if (strbuf_realpath(&self, objdir.buf, 0) &&
++ !fspathcmp(real.buf, self.buf)) {
++ ret = false;
++ goto out;
++ }
++
++ if (files->quarantine_path &&
++ strbuf_realpath(&self, files->quarantine_path, 0)) {
++ strbuf_strip_file_from_path(&self);
++ strbuf_trim_trailing_dir_sep(&self);
++ if (!fspathcmp(real.buf, self.buf))
++ ret = false;
++ }
++
++out:
++ strbuf_release(&real);
++ strbuf_release(&objdir);
++ strbuf_release(&self);
++ return ret;
++}
++
+ static void odb_add_alternate_recursively(struct odb_source_files *files,
+ const char *path,
+ int depth)
+@@ odb/source-files.c: static void odb_add_alternate_recursively(struct odb_source_files *files,
goto out;
+
+ alternate = odb_files_dir_new(files->base.odb, path, false);
++ alternate->borrowed = is_borrowed_object_dir(files, alternate->abspath);
+
+ /* add the alternate entry */
+ *files->dirs_tail = alternate;
+@@ odb/source-files.c: struct odb_source_files *odb_source_files_new(struct object_database *odb,
+ struct odb_source_files *files;
+ char *object_dir = NULL;
+ char *alternates = NULL;
++ char *quarantine_path = NULL;
+
+ if (flags & ODB_NEW_HONOR_ENV) {
+ object_dir = xstrdup_or_null(getenv(DB_ENVIRONMENT));
+ alternates = xstrdup_or_null(getenv(ALTERNATE_DB_ENVIRONMENT));
++ quarantine_path =
++ xstrdup_or_null(getenv(GIT_QUARANTINE_ENVIRONMENT));
+ }
+ if (!object_dir)
+ object_dir = xstrfmt("%s/objects", odb->repo->commondir);
+
+ CALLOC_ARRAY(files, 1);
++ files->quarantine_path = quarantine_path;
+ odb_source_init(&files->base, odb, ODB_SOURCE_FILES, object_dir);
+
+ hashmap_init(&files->dirs_by_path, odb_files_dir_by_path_cmp, files, 0);
+
+ ## odb/source-files.h ##
+@@ odb/source-files.h: struct odb_files_dir {
+ * repository. Directories added via alternates are not local.
+ */
+ bool local;
+
+ /*
-+ * NEEDSWORK: this is a hack, mainly here to measure the cost of
-+ * the bug. Inside an object quarantine, tmp-objdir passes our own
-+ * object directory to child processes as an alternate, so we would
-+ * list our own refs as alternate refs, including the hidden refs
-+ * that the connectivity check of receive-pack deliberately skips.
-+ * Comparing paths papers over that; the quarantine should rather
-+ * tell its children which directory is ours.
++ * Whether this directory is borrowed from another repository, so
++ * that its refs are alternate refs. Not every directory added via
++ * alternates is borrowed: inside an object quarantine, our own
++ * object directory is passed to us as an alternate, too. Only set
++ * for directories added via alternates.
+ */
-+ if (cb->repo->commondir &&
-+ strbuf_realpath(&self, cb->repo->commondir, 0) &&
-+ !strcmp(path.buf, self.buf))
-+ goto out;
- base_len = path.len;
-
- /* Is this a git repository with refs? */
-@@ odb.c: static int refs_from_alternate_cb(struct odb_files_dir *alternate,
++ bool borrowed;
+ };

- out:
- strbuf_release(&path);
-+ strbuf_release(&self);
- return 0;
- }
+ struct odb_files_dir *odb_files_dir_new(struct object_database *odb, +@@ odb/source-files.h: struct odb_source_files {
+ * determined by "core.ignoreCase".
+ */
+ int dirs_paths_icase;
++
++ /*
++ * The value of GIT_QUARANTINE_PATH when this source was set up from
++ * the environment (see ODB_NEW_HONOR_ENV), or NULL.
++ */
++ char *quarantine_path;
+ };

+ /* Allocate and initialize a new object source. */

## t/perf/p5552-push-hidden-refs.sh (new) ##
@@
@@ t/perf/p5552-push-hidden-refs.sh (new)
+test_done

## t/t5547-push-quarantine.sh ##
-@@ t/t5547-push-quarantine.sh: test_expect_success '.keep file is removed after push' '
- test_path_is_missing "$keep"
+@@ t/t5547-push-quarantine.sh: test_expect_success '.keep file created outside of process is not removed' '
+ test_path_is_missing "${keep%.keep}.pack"
'

+test_expect_success 'quarantine does not list own refs as alternate refs' '
@@ t/t5547-push-quarantine.sh: test_expect_success '.keep file is removed after pus
+ sort -u alternate-refs.log >actual &&
+ test_cmp expect actual
+'
++
++test_expect_success 'rev-list --alternate-refs in a hook skips own refs' '
++ test_when_finished "rm -rf altref.git" &&
++ git init --bare altref.git &&
++ git push altref.git HEAD:refs/heads/main &&
++ test_hook -C altref.git pre-receive <<-\EOF &&
++ git rev-list --no-walk --alternate-refs --stdin </dev/null \
++ >../hook-alternate-refs
++ EOF
++ test_commit altref-push &&
++ git push altref.git HEAD:refs/heads/new &&
++ test_must_be_empty hook-alternate-refs
++'
++
+ test_done
+
+ ## t/t5618-alternate-refs.sh ##
+@@ t/t5618-alternate-refs.sh: test_expect_success 'log --source shows .alternate marker' '
+ test_cmp expect actual
+ '
+
++# Our own object directory as an alternate is not a source of alternate ++# refs, but the alternates it brings in with it still are. ++test_expect_success 'own object directory as alternate lists only real alternates' '
++ test_when_finished "rm -rf other-objects" &&
++ mkdir other-objects &&
++ git rev-list --remotes=origin >expect &&
++ GIT_OBJECT_DIRECTORY="$(pwd)/other-objects" \
++ GIT_ALTERNATE_OBJECT_DIRECTORIES="$(pwd)/.git/objects" \
++ git rev-list --alternate-refs >actual &&
++ test_cmp expect actual
++'
++
++# A quarantine is created inside our object directory, so its parent is ++# ourselves, even when that is not under our commondir. ++test_expect_success 'parent of the quarantine is not listed as an alternate' '
++ test_when_finished "rm -rf store.git" &&
++ git init --bare store.git &&
++ git push store.git refs/remotes/origin/one:refs/heads/one &&
++ quarantine="$(pwd)/store.git/objects/tmp_objdir-test" &&
++ mkdir "$quarantine" &&
++
++ git rev-parse origin/one >expect &&
++ GIT_OBJECT_DIRECTORY="$quarantine" \
++ GIT_ALTERNATE_OBJECT_DIRECTORIES="$(pwd)/store.git/objects" \
++ git rev-list --no-walk --alternate-refs --stdin </dev/null >actual &&
++ test_cmp expect actual &&
++
++ GIT_QUARANTINE_PATH="$quarantine" \
++ GIT_OBJECT_DIRECTORY="$quarantine" \
++ GIT_ALTERNATE_OBJECT_DIRECTORIES="$(pwd)/store.git/objects" \
++ git rev-list --no-walk --alternate-refs --stdin </dev/null >actual &&
++ test_must_be_empty actual
++'
+
test_done

 Documentation/rev-list-options.adoc |  5 ++-
 odb.c                               |  3 ++
 odb/source-files.c                  | 47 +++++++++++++++++++++++++++++
 odb/source-files.h                  | 15 +++++++++
 t/perf/p5552-push-hidden-refs.sh    | 37 +++++++++++++++++++++++
 t/t5547-push-quarantine.sh          | 38 +++++++++++++++++++++++
 t/t5618-alternate-refs.sh           | 34 +++++++++++++++++++++
 7 files changed, 178 insertions(+), 1 deletion(-)
 create mode 100755 t/perf/p5552-push-hidden-refs.sh

index 285fb3809f..6e4c487d1e 100644
@@ -223,7 +223,10 @@ explicitly.
 	Pretend as if all objects mentioned as ref tips of alternate
 	repositories were listed on the command line. An alternate
 	repository is any repository whose object directory is specified
-	in `objects/info/alternates`.  The set of included objects may
+	in `objects/info/alternates`.  The repository's own object
+	directory does not count, even when it is passed in as an
+	alternate, as happens inside the object quarantine of
+	linkgit:git-receive-pack[1].  The set of included objects may
 	be modified by `core.alternateRefsCommand`, etc. See
 	linkgit:git-config[1].
 

index 2c93a9490c..646a498052 100644
@@ -186,6 +186,9 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,
 	size_t base_len;
 	struct alternate_refs_data *cb = payload;
 
+	if (!alternate->borrowed)
+		goto out;
+
 	if (!strbuf_realpath(&path, alternate->abspath, 0))
 		goto out;
 	if (!strbuf_strip_suffix(&path, "/objects"))

index 837e890304..b404937fa1 100644
@@ -33,6 +33,15 @@ struct odb_files_dir {
 	 * repository. Directories added via alternates are not local.
 	 */
 	bool local;
+
+	/*
+	 * Whether this directory is borrowed from another repository, so
+	 * that its refs are alternate refs. Not every directory added via
+	 * alternates is borrowed: inside an object quarantine, our own
+	 * object directory is passed to us as an alternate, too. Only set
+	 * for directories added via alternates.
+	 */
+	bool borrowed;
 };
 
 struct odb_files_dir *odb_files_dir_new(struct object_database *odb,

@@ -66,6 +75,12 @@ struct odb_source_files {
 	 * determined by "core.ignoreCase".
 	 */
 	int dirs_paths_icase;
+
+	/*
+	 * The value of GIT_QUARANTINE_PATH when this source was set up from
+	 * the environment (see ODB_NEW_HONOR_ENV), or NULL.
+	 */
+	char *quarantine_path;
 };
 
 /* Allocate and initialize a new object source. */

new file mode 100755
index 0000000000..e9aa206298
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='speed of push into a repository with many hidden refs'
+. ./perf-lib.sh
+
+test_perf_fresh_repo
+
+test_expect_success 'set up repository with many hidden refs' '
+	git init --bare dst.git &&
+	test_commit_bulk --ref=refs/hidden/base --id=hidden 1000 &&
+	git rev-list refs/hidden/base >commits &&
+	for i in $(test_seq 200)
+	do
+		sed "s|^\(.*\)$|create refs/hidden/$i/\1 \1|" commits || return 1
+	done >input &&
+	git push dst.git refs/hidden/base &&
+	git -C dst.git update-ref --stdin <input &&
+	git -C dst.git pack-refs --all &&
+	cp -R dst.git dst-visible.git &&
+	git -C dst.git config receive.hideRefs refs/hidden &&
+	git symbolic-ref HEAD refs/heads/main &&
+	test_commit_bulk --id=visible 1 &&
+	git push dst.git main &&
+	git push dst-visible.git main
+'
+
+test_perf 'push, hidden refs' '
+	git commit --allow-empty -m new &&
+	git push -f dst.git HEAD:refs/heads/new
+'
+
+test_perf 'push, no hidden refs' '
+	git commit --allow-empty -m new &&
+	git push -f dst-visible.git HEAD:refs/heads/new
+'
+
+test_done

index 6c14dfc42b..3426aec531 100755
@@ -125,4 +125,42 @@ test_expect_success '.keep file created outside of process is not removed' '
 	test_path_is_missing "${keep%.keep}.pack"
 '
 
+test_expect_success 'quarantine does not list own refs as alternate refs' '
+	git init --bare hidden.git &&
+	git push hidden.git HEAD:refs/heads/main HEAD:refs/hidden/one &&
+	git -C hidden.git config receive.hideRefs refs/hidden &&
+	write_script hidden.git/alternate-refs <<-EOF &&
+	echo "\$1" >>"$(pwd)/alternate-refs.log"
+	EOF
+	git -C hidden.git config core.alternateRefsCommand ./alternate-refs &&
+	test_commit hidden-push &&
+	git push hidden.git HEAD:refs/heads/new &&
+	test_path_is_missing alternate-refs.log
+'
+
+test_expect_success 'quarantine lists refs of other alternates' '
+	test_when_finished "rm -f alternate-refs.log" &&
+	git init --bare other.git &&
+	git push other.git HEAD:refs/heads/other &&
+	echo "$(pwd)/other.git/objects" >hidden.git/objects/info/alternates &&
+	test_commit hidden-push-2 &&
+	git push hidden.git HEAD:refs/heads/new2 &&
+	(cd other.git && pwd -P) >expect &&
+	sort -u alternate-refs.log >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rev-list --alternate-refs in a hook skips own refs' '
+	test_when_finished "rm -rf altref.git" &&
+	git init --bare altref.git &&
+	git push altref.git HEAD:refs/heads/main &&
+	test_hook -C altref.git pre-receive <<-\EOF &&
+	git rev-list --no-walk --alternate-refs --stdin </dev/null \
+		>../hook-alternate-refs
+	EOF
+	test_commit altref-push &&
+	git push altref.git HEAD:refs/heads/new &&
+	test_must_be_empty hook-alternate-refs
+'
+
 test_done

index 2fb6d549d3..d52e426db1 100755
@@ -58,4 +58,38 @@ test_expect_success 'log --source shows .alternate marker' '
 	test_cmp expect actual
 '
 
+# Our own object directory as an alternate is not a source of alternate
+# refs, but the alternates it brings in with it still are.
+test_expect_success 'own object directory as alternate lists only real alternates' '
+	test_when_finished "rm -rf other-objects" &&
+	mkdir other-objects &&
+	git rev-list --remotes=origin >expect &&
+	GIT_OBJECT_DIRECTORY="$(pwd)/other-objects" \
+	GIT_ALTERNATE_OBJECT_DIRECTORIES="$(pwd)/.git/objects" \
+		git rev-list --alternate-refs >actual &&
+	test_cmp expect actual
+'
+
+# A quarantine is created inside our object directory, so its parent is
+# ourselves, even when that is not under our commondir.
+test_expect_success 'parent of the quarantine is not listed as an alternate' '
+	test_when_finished "rm -rf store.git" &&
+	git init --bare store.git &&
+	git push store.git refs/remotes/origin/one:refs/heads/one &&
+	quarantine="$(pwd)/store.git/objects/tmp_objdir-test" &&
+	mkdir "$quarantine" &&
+
+	git rev-parse origin/one >expect &&
+	GIT_OBJECT_DIRECTORY="$quarantine" \
+	GIT_ALTERNATE_OBJECT_DIRECTORIES="$(pwd)/store.git/objects" \
+		git rev-list --no-walk --alternate-refs --stdin </dev/null >actual &&
+	test_cmp expect actual &&
+
+	GIT_QUARANTINE_PATH="$quarantine" \
+	GIT_OBJECT_DIRECTORY="$quarantine" \
+	GIT_ALTERNATE_OBJECT_DIRECTORIES="$(pwd)/store.git/objects" \
+		git rev-list --no-walk --alternate-refs --stdin </dev/null >actual &&
+	test_must_be_empty actual
+'
+
 test_done

base-commit: ebdc087dc99d68bb340c282b2ba5f722def28d19

gitgitgadget

"Kristofer Karlsson via GitGitGadget" <[email protected]>

All of the above changes make sense to me.

This unfortunately crashes with Patrick's odb-reload-alternates series, but let me see if I can get a reasonable conflict resolution or if it is more reasonable to ask you to rebase this on the other topic.

Thanks.

I decided to queue it as-is instead of asking you to depend on the other topic. Here is what I got after making a trial merge to 'seen'.

I usually use "git show" to trigger the dense-combined diff format (i.e., "git show --cc seen"), but "git show --remerge-diff seen" output is shown below, just to see how easy it is to read.

commit b7f4d1e66b034c2a64b7f3323f9cd156a028f080
Merge: 1707829739 3f91b4bc4b
Author: Junio C Hamano <[email protected]>
Date: Mon Nov 2 16:37:57 2026 -0800

Merge branch 'kk/quarantine-alternate-refs' into seen

The connectivity check in 'git-receive-pack(1)' has been optimized to not list the repository's own refs as alternate refs when running inside the object quarantine, avoiding redundant traversal of hidden refs.

* kk/quarantine-alternate-refs:
odb: do not list our own refs as alternate refs in the quarantine

remerge CONFLICT (content): Merge conflict in Documentation/rev-list-options.adoc index 4e92cf6456..d20c5f6372 100644

@@ -223,23 +223,13 @@ explicitly.
 	Pretend as if all objects mentioned as ref tips of alternate
 	repositories were listed on the command line. An alternate
 	repository is any repository whose object directory is specified
-<<<<<<< 1707829739 (Merge branch 'kk/connectivity-check-ignore-replace' into seen)
 	in `objects/info/alternates`.  The refs of shallow and grafted
 	alternate repositories are ignored, as the history behind them
-	may be missing.  The set of included objects may be modified by
+	may be missing.  The repository's own object directory does not
+	count, even when it is passed in as an alternate, as happens
+	inside the object quarantine of	linkgit:git-receive-pack[1].
+	The set of included objects may be modified by
 	`core.alternateRefsCommand`, etc. See linkgit:git-config[1].
-||||||| ebdc087dc9
-	in `objects/info/alternates`.  The set of included objects may
-	be modified by `core.alternateRefsCommand`, etc. See
-	linkgit:git-config[1].
-=======
-	in `objects/info/alternates`.  The repository's own object
-	directory does not count, even when it is passed in as an
-	alternate, as happens inside the object quarantine of
-	linkgit:git-receive-pack[1].  The set of included objects may
-	be modified by `core.alternateRefsCommand`, etc. See
-	linkgit:git-config[1].
->>>>>>> 3f91b4bc4b (odb: do not list our own refs as alternate refs in the quarantine)
 
 `--single-worktree`::
 	By default, all working trees will be examined by the

remerge CONFLICT (content): Merge conflict in odb/source-files.c index 4239cb493c..8ce437fdc2 100644

@@ -75,15 +75,9 @@ static void odb_source_files_free(struct odb_source *source)
 		odb_files_dir_free(files->dirs);
 		files->dirs = next;
 	}
-<<<<<<< 1707829739 (Merge branch 'kk/connectivity-check-ignore-replace' into seen)
 	strset_clear(&files->dir_paths);
 	stat_validity_clear(&files->alternates_validity);
-||||||| ebdc087dc9
-	hashmap_clear(&files->dirs_by_path);
-=======
-	hashmap_clear(&files->dirs_by_path);
 	free(files->quarantine_path);
->>>>>>> 3f91b4bc4b (odb: do not list our own refs as alternate refs in the quarantine)
 
 	odb_source_release(&files->base);
 	free(files);

@@ -362,15 +356,6 @@ static void read_alternates(const char *object_dir, struct strvec *out,
 	free(path);
 }
 
-<<<<<<< 1707829739 (Merge branch 'kk/connectivity-check-ignore-replace' into seen)
-static void add_alternate_recursively(struct odb_source_files *files,
-				      const char *path,
-				      int depth)
-||||||| ebdc087dc9
-static void odb_add_alternate_recursively(struct odb_source_files *files,
-					  const char *path,
-					  int depth)
-=======
 /*
  * Our own object directory is <commondir>/objects, or the parent of
  * GIT_QUARANTINE_PATH, as an object quarantine is created directly

@@ -410,10 +395,9 @@ static bool is_borrowed_object_dir(struct odb_source_files *files,
 	return ret;
 }
 
-static void odb_add_alternate_recursively(struct odb_source_files *files,
-					  const char *path,
-					  int depth)
->>>>>>> 3f91b4bc4b (odb: do not list our own refs as alternate refs in the quarantine)
+static void add_alternate_recursively(struct odb_source_files *files,
+				      const char *path,
+				      int depth)
 {
 	struct odb_files_dir *alternate;
 	struct strvec alternates = STRVEC_INIT;

remerge CONFLICT (content): Merge conflict in odb/source-files.h index 7afc2f28ad..9117dbea5b 100644

@@ -70,22 +70,16 @@ struct odb_source_files {
 	 * Whether directory paths shall be compared case-insensitively, as
 	 * determined by "core.ignoreCase".
 	 */
-<<<<<<< 1707829739 (Merge branch 'kk/connectivity-check-ignore-replace' into seen)
 	int dir_paths_icase;
 
 	/* Stat data of the "info/alternates" file. */
 	struct stat_validity alternates_validity;
-||||||| ebdc087dc9
-	int dirs_paths_icase;
-=======
-	int dirs_paths_icase;
 
 	/*
 	 * The value of GIT_QUARANTINE_PATH when this source was set up from
 	 * the environment (see ODB_NEW_HONOR_ENV), or NULL.
 	 */
 	char *quarantine_path;
->>>>>>> 3f91b4bc4b (odb: do not list our own refs as alternate refs in the quarantine)
 };
 
 /* Allocate and initialize a new object source. */

remerge CONFLICT (content): Merge conflict in t/t5618-alternate-refs.sh index cefc37797c..048c7c63c1 100755

Thank you! I also read through Patrick's changes in ps/odb-reload-alternates, and they appear to be logically compatible. I couldn't spot any problematic interactions, and your conflict resolution looks correct to me.

Kristofer

Good point. I can rework the patch to make it easier to integrate with your ps/odb-object-storage-extension topic, by having the files source remember its default object directory in files->default_object_dir where it decides it, and comparing against that instead of hardcoding <commondir>/objects in the check. Then your topic only has to change how the default is decided, and the check follows. Something like this, if you want:

@@ static bool is_borrowed_object_dir(struct odb_source_files *files,
- strbuf_addf(&objdir, "%s/objects", files->base.odb->repo->commondir);
- if (strbuf_realpath(&self, objdir.buf, 0) &&
+ if (strbuf_realpath(&self, files->default_object_dir, 0) &&
     !fspathcmp(real.buf, self.buf)) {
@@ struct odb_source_files *odb_source_files_new(struct object_database *odb,
+ default_object_dir = xstrfmt("%s/objects", odb->repo->commondir);
  if (!object_dir)
- object_dir = xstrfmt("%s/objects", odb->repo->commondir);
+ object_dir = xstrdup(default_object_dir);

CALLOC_ARRAY(files, 1);
+ files->default_object_dir = default_object_dir;

Either way, the check should not make things worse where it is incomplete. A directory we fail to recognize as our own stays borrowed, so --alternate-refs lists its refs just like today. The opposite mistake would need a quarantine inside another repository's object directory, which seems unlikely, and even then it would only cost some negative tips: the traversal walks further, but never accepts a missing object.

[...]

That does sound like the cleaner fix. Since you are already deep into this area, I would be happy to drop the fix and wait for that instead. I expect very few users to be affected in practice: mostly git hosts with many hidden refs, which is the case bcec6780b2 was about.

In that case I could send just the tests, with the hidden refs case marked as test_expect_failure, so that it is covered once a long-term fix lands. Or, if you would rather have a short-term fix in the meantime, I can send a v3 with the change above.

Thanks,
Kristofer

Note that this topic is already merged to "next", so in theory you could rebase your patch on top of it now.

I certainly do care about this case and would like to see this fixed :) I'd like to get Justin's opinion on that proposal first though, as he's worked on transactions quite extensively over the last couple months.

Patrick

Regarding the fix proposed in this patch, I'm not quite sure this is really the right layer to address the problem. The root of the problem is that we are swapping around the "real" and quarantine object directories to facilitate writing to the "files" ODB transaction. IMO, this is already rather fragile. So I do agree with Patrick that I think the better long-term fix is to stop modifying the primary object directory altogether and instead propagate some context about the pending transaction via environment variables to the child process that it can use to reconstruct the proper environment. This is certainly a more involved change, but something that I've been wanting to do for a while now regardless.

If we are interested in a more simple fix for the short-term, we could adapt odb_transaction_env() to have a "read-only" mode flag that does not modify the primary object directory and only appends the quarantine directory as an alternate. For the connectivity check in git-receive-pack(1) we don't write any object anyways, so configuring the environment in this manner should be sufficient to fix the issue.

-Justin

I agree with that long-term direction. What do you think we should do for the short term? I see a few options:
1. A smaller, more focused patch in the alternate refs code, as Peff suggested in the other message.
2. Your read-only transaction environment for the connectivity check, (if you want to take that on, I would probably mess it up :-) ).
3. Only a regression test for now, with the hidden refs case marked as test_expect_failure, so that we do not lose track of it.
4. Or nothing, and wait for the long-term fix, as nobody seems to be hit hard by this at the moment.

Thanks,
Kristofer

IMO, I think the patch suggested by Peff is a reasonably fix for this problem. Later down the line I will probably also revisit how "files" ODB transactions manage the object directory, but there are some other transaction related topics I'm hoping to land first.

Thanks,
-Justin

So as I said before, I think this is a reasonable fix regardless of the how quarantine issues work. We should not be pulling in alternate ref sources that duplicate each other or our main repo.

But I was surprised to see special handling of GIT_QUARANTINE_PATH here. I think this is a defect in the alternate-ref code. Yes, the common case is triggering it with our tmp-objdir shenanigans, but if we fix the underlying issue, then the use of it here should fix the quarantine issue automatically.

I.e., I'd expect the code part of the patch to just be:

index 2c93a9490c..f9ab5c8e7f 100644
@@ -183,6 +183,7 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,
 				  void *payload)
 {
 	struct strbuf path = STRBUF_INIT;
+	struct strbuf repo = STRBUF_INIT;
 	size_t base_len;
 	struct alternate_refs_data *cb = payload;
 

@@ -198,10 +199,16 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,
 		goto out;
 	strbuf_setlen(&path, base_len);
 
+	/* XXX could realpath once in caller as a mild optimization */
+	if (strbuf_realpath(&repo, cb->repo->commondir, 0) &&
+	    !strcmp(path.buf, repo.buf))
+		goto out;
+
 	read_alternate_refs(cb->repo, path.buf, cb->fn, cb->payload);
 
 out:
 	strbuf_release(&path);
+	strbuf_release(&repo);
 	return 0;
 }
 

and keep it totally local to the path hackery in the alternate-refs path.

Or am I missing something that requires more complication?

That's how _we_ use them in the internal callers, but it's a rev-list option. So we can't say how they are used elsewhere. That said, I think this is a totally reasonable tightening of the definition for general use.

-Peff

No, I think you are right. The quarantine parent only matters when the object directory is not under the commondir, which is rare. Your version is essentially my v1 without the "this is a hack" framing (and also cleaner).

Good point. I will reword that part of the commit message so that it does not rely on how the connectivity check uses them. It also seems in line with what we already do when loading alternates, where listing the object directory itself is ignored as a common mistake; inside the quarantine, that check just compares against the quarantine instead of our own object directory.

Justin suggested fixing it at the transaction layer instead, so I will wait to see where that discussion lands before sending a v3.

It looks like the remaining options are:
1. Go with your proposed version.
2. Do nothing until the long term fix.
3. Just add a regression test for now.

Thanks,
Kristofer

Sounds very good.
Thanks for coming up with a good way forward.

What we really want is "where are we reading our local refs from" (to avoid reading them again). So even if you have an object directory not under the commondir, we would not find our repo again by looking at the parent of that directory.

But I also won't be surprised if there is some arcane working tree setup that I'm missing.

Anyway, what's in your v3 looks good to me. Thanks for fixing this!

-Peff

With those historic environment variables like GIT_DIR and GIT_WORK_TREE, and GIT_OBJECT_DIRECTORY, any random arrangement is possible, I am afraid. I do not know if it is worth worrying about, though. We've seen people use out-of-tree GIT_DIR by using the former two environment variables, and I can see why people find it attractive, but I do not recall people using creative uses of GIT_OBJECT_DIRECTORY to tear /objects directory out of GIT_DIR, and what the perceived advantage of doing so are..

Thanks, both. Let me mark the topic as ready for 'next'.

From: Kristofer Karlsson <[email protected]>

Since 722ff7f876 (receive-pack: quarantine objects until pre-receive accepts, 2016-10-03), receive-pack writes incoming objects to a temporary object directory. Child processes see it as their primary object directory, and tmp-objdir passes the repository's own object directory to them via GIT_ALTERNATE_OBJECT_DIRECTORIES.

The connectivity check passes --alternate-refs to rev-list since 39b44ba771 (check_everything_connected: assume alternate ref tips are valid, 2019-07-01). Inside the quarantine, the repository's own object directory is one of the alternates, so --alternate-refs runs for-each-ref on the repository itself and lists all of its refs.

That includes hidden refs. bcec6780b2 (receive-pack: only use visible refs for connectivity check, 2022-11-17) added --exclude-hidden=receive to the check so that repositories with many hidden refs do not have to load all of them on each push. The hidden refs are excluded from --all, but they all come back through --alternate-refs.

Our own repository is not a source of alternate refs: its refs are our own refs, which the caller already lists as it sees fit. So skip an alternate whose repository is our own when listing alternate refs. This does not depend on how the alternate came about, so it also covers an environment that makes our own object directory an alternate by hand.

Since --alternate-refs is a rev-list option, callers other than the connectivity check may use it too, so this tightens what it means rather than only fixing the connectivity check. It is in line with what we already do when loading alternates, where listing the object directory itself is ignored as a common mistake; inside the quarantine that check compares against the quarantine instead of our own object directory, so our own object directory slips through. Document the tightened meaning, too.

Add tests that our own refs are no longer listed, by the connectivity check, by rev-list run from a pre-receive hook, and by rev-list with our own object directory passed in as an alternate by hand, and that the refs of a real alternate still are. Add a perf test pushing a single commit into a repository with a visible branch and about 200,000 refs hidden by receive.hideRefs, and into a copy of it that hides nothing:

    Test                           HEAD~1            HEAD
    -----------------------------------------------------------------------
    5552.2: push, hidden refs      0.29(0.31+0.06)   0.10(0.07+0.03) -65.5%
    5552.3: push, no hidden refs   1.20(1.02+0.68)   1.00(0.83+0.59) -16.7%

With hidden refs, most of the time went to the for-each-ref child that listed the alternate refs. Without hidden refs, every ref was listed twice, once for --all and once for --alternate-refs.

---
odb: do not list our own refs as alternate refs

When a repository hides refs with receive.hideRefs, every push still loads all of the hidden refs in the connectivity check, even though bcec6780b2 (receive-pack: only use visible refs for connectivity check, 2022-11-17) excludes them from it. With many hidden refs, that is most of the time spent on a small push.

I stumbled across this while working on the boundary walk of the connectivity check (the follow-up to kk/connectivity-check-incremental), when the walk loaded every ref during a push even though it should only have looked at a few of them.

Changes since v2:

* Went back to the approach of v1, as Peff suggested: skip our own repository in the alternate refs code itself, with no special handling of the quarantine. The "borrowed" flag and the check for the parent of GIT_QUARANTINE_PATH are gone.

* The commit message now describes this as a tightening of what --alternate-refs means, in line with how loading alternates already ignores the object directory itself.

* Kept the documentation note and the tests from v2, except the one for a quarantine outside of the commondir.

Changes since v1:

* Changed the rule to "an alternate that is ourselves is not a source of alternate refs", as Peff described it.

* Added a "borrowed" flag to alternates, set when they are loaded, which says whether they come from another repository, and used it for --alternate-refs. Our own object directory is recognized either as commondir/objects or as the parent of GIT_QUARANTINE_PATH.

* Documented this for --alternate-refs.

* Added tests for rev-list --alternate-refs in a pre-receive hook, for our own object directory set up as an alternate by hand, and for a quarantine outside of the commondir.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2285%2Fspkrka%2Fquarantine-altrefs-bugreport-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2285/spkrka/quarantine-altrefs-bugreport-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2285

Range-diff vs v2:

1: bac15042c0 ! 1: 35ab373cd4 odb: do not list our own refs as alternate refs in the quarantine
@@ Metadata
Author: Kristofer Karlsson <[email protected]>

## Commit message ##
- odb: do not list our own refs as alternate refs in the quarantine
+ odb: do not list our own refs as alternate refs

Since 722ff7f876 (receive-pack: quarantine objects until pre-receive accepts, 2016-10-03), receive-pack writes incoming objects to a
- temporary object directory. Child processes see it as their primary
+ temporary object directory. Child processes see it as their primary
object directory, and tmp-objdir passes the repository's own object directory to them via GIT_ALTERNATE_OBJECT_DIRECTORIES.

The connectivity check passes --alternate-refs to rev-list since 39b44ba771 (check_everything_connected: assume alternate ref tips are
- valid, 2019-07-01). Inside the quarantine, the repository's own object
+ valid, 2019-07-01). Inside the quarantine, the repository's own object
directory is one of the alternates, so --alternate-refs runs for-each-ref on the repository itself and lists all of its refs.

- That includes hidden refs. bcec6780b2 (receive-pack: only use visible
+ That includes hidden refs. bcec6780b2 (receive-pack: only use visible
refs for connectivity check, 2022-11-17) added --exclude-hidden=receive to the check so that repositories with many hidden refs do not have to
- load all of them on each push. The hidden refs are excluded from --all,
+ load all of them on each push. The hidden refs are excluded from --all,
but they all come back through --alternate-refs.

- An alternate that is ourselves is not a source of alternate refs: its
- refs are our own refs, which the caller already lists as it sees fit.
- Mark each object directory added via alternates with whether it is
- borrowed from another repository, and list alternate refs only from
- those. Within one process this needs no guessing, as tmp-objdir
- links our own object directory in itself. But a child process rebuilds
- its object store from the environment, so when an alternate is added,
- recognize ourselves in two ways:
-
- - as our <commondir>/objects, which also covers an environment that
- makes our own object directory an alternate by hand;
-
- - as the parent of GIT_QUARANTINE_PATH, as the quarantine is created
- directly inside our object directory (see "QUARANTINE ENVIRONMENT"
- in git-receive-pack(1)); this also works when the object directory
- is not under the commondir.
-
- Document in --alternate-refs that our own object directory does not
- count as an alternate repository.
-
- This is safe: alternate refs only serve as negative tips that are
- assumed to be complete, so dropping some can only make the traversal
- walk further, never make it accept a missing object.
+ Our own repository is not a source of alternate refs: its refs are our
+ own refs, which the caller already lists as it sees fit. So skip an
+ alternate whose repository is our own when listing alternate refs.
+ This does not depend on how the alternate came about, so it also covers
+ an environment that makes our own object directory an alternate by
+ hand.
+
+ Since --alternate-refs is a rev-list option, callers other than the
+ connectivity check may use it too, so this tightens what it means
+ rather than only fixing the connectivity check. It is in line with
+ what we already do when loading alternates, where listing the object
+ directory itself is ignored as a common mistake; inside the quarantine
+ that check compares against the quarantine instead of our own object
+ directory, so our own object directory slips through. Document the
+ tightened meaning, too.

Add tests that our own refs are no longer listed, by the connectivity
- check, by rev-list run from a pre-receive hook, by rev-list with our
- own object directory passed in as an alternate by hand, and by rev-list
- in a quarantine whose parent is not under the commondir, and that the
- refs of a real alternate still are. Add a perf test pushing a
+ check, by rev-list run from a pre-receive hook, and by rev-list with
+ our own object directory passed in as an alternate by hand, and that
+ the refs of a real alternate still are. Add a perf test pushing a
single commit into a repository with a visible branch and about 200,000 refs hidden by receive.hideRefs, and into a copy of it that hides nothing:

Test HEAD~1 HEAD
-----------------------------------------------------------------------
- 5552.2: push, hidden refs 0.31(0.33+0.08) 0.09(0.06+0.03) -71.0%
- 5552.3: push, no hidden refs 1.23(1.06+0.68) 1.01(0.83+0.61) -17.9%
+ 5552.2: push, hidden refs 0.29(0.31+0.06) 0.10(0.07+0.03) -65.5%
+ 5552.3: push, no hidden refs 1.20(1.02+0.68) 1.00(0.83+0.59) -16.7%

With hidden refs, most of the time went to the for-each-ref child that
- listed the alternate refs. Without hidden refs, every ref was listed
+ listed the alternate refs. Without hidden refs, every ref was listed
twice, once for --all and once for --alternate-refs.

@@ Documentation/rev-list-options.adoc: explicitly.

## odb.c ##
-@@ odb.c: static int refs_from_alternate_cb(struct odb_files_dir *alternate,
- size_t base_len;
- struct alternate_refs_data *cb = payload;
-
-+ if (!alternate->borrowed)
-+ goto out;
-+
- if (!strbuf_realpath(&path, alternate->abspath, 0))
- goto out;
- if (!strbuf_strip_suffix(&path, "/objects"))
-
- ## odb/source-files.c ##
@@
- #include "blob.h"
- #include "chdir-notify.h"
+ #include "abspath.h"
+ #include "commit-graph.h"
#include "config.h"
+#include "dir.h"
-+#include "environment.h"
+ #include "environment.h"
#include "gettext.h"
- #include "hex.h"
- #include "lockfile.h"
-@@ odb/source-files.c: static void odb_source_files_free(struct odb_source *source)
- files->dirs = next;
- }
- hashmap_clear(&files->dirs_by_path);
-+ free(files->quarantine_path);
-
- odb_source_release(&files->base);
- free(files);
-@@ odb/source-files.c: static int read_alternates(const char *object_dir, struct strvec *out)
- return 0;
- }
+ #include "hashmap.h"
+@@ odb.c: static int refs_from_alternate_cb(struct odb_files_dir *alternate,
+ void *payload)
+ {
+ struct strbuf path = STRBUF_INIT;
++ struct strbuf repo = STRBUF_INIT;
+ size_t base_len;
+ struct alternate_refs_data *cb = payload;

-+/*
-+ * Our own object directory is <commondir>/objects, or the parent of
-+ * GIT_QUARANTINE_PATH, as an object quarantine is created directly
-+ * inside it (see tmp_objdir_create()). Anything else, including a path
-+ * that cannot be resolved, is borrowed.
-+ */
-+static bool is_borrowed_object_dir(struct odb_source_files *files,
-+ const char *path)
-+{
-+ struct strbuf real = STRBUF_INIT;
-+ struct strbuf objdir = STRBUF_INIT;
-+ struct strbuf self = STRBUF_INIT;
-+ bool ret = true;
-+
-+ if (!strbuf_realpath(&real, path, 0))
-+ goto out;
-+
-+ strbuf_addf(&objdir, "%s/objects", files->base.odb->repo->commondir);
-+ if (strbuf_realpath(&self, objdir.buf, 0) &&
-+ !fspathcmp(real.buf, self.buf)) {
-+ ret = false;
-+ goto out;
-+ }
-+
-+ if (files->quarantine_path &&
-+ strbuf_realpath(&self, files->quarantine_path, 0)) {
-+ strbuf_strip_file_from_path(&self);
-+ strbuf_trim_trailing_dir_sep(&self);
-+ if (!fspathcmp(real.buf, self.buf))
-+ ret = false;
-+ }
-+
-+out:
-+ strbuf_release(&real);
-+ strbuf_release(&objdir);
-+ strbuf_release(&self);
-+ return ret;
-+}
-+
- static void odb_add_alternate_recursively(struct odb_source_files *files,
- const char *path,
- int depth)
-@@ odb/source-files.c: static void odb_add_alternate_recursively(struct odb_source_files *files, +@@ odb.c: static int refs_from_alternate_cb(struct odb_files_dir *alternate,
goto out;
+ strbuf_setlen(&path, base_len);

- alternate = odb_files_dir_new(files->base.odb, path, false);
-+ alternate->borrowed = is_borrowed_object_dir(files, alternate->abspath);
-
- /* add the alternate entry */
- *files->dirs_tail = alternate;
-@@ odb/source-files.c: struct odb_source_files *odb_source_files_new(struct object_database *odb,
- struct odb_source_files *files;
- char *object_dir = NULL;
- char *alternates = NULL;
-+ char *quarantine_path = NULL;
-
- if (flags & ODB_NEW_HONOR_ENV) {
- object_dir = xstrdup_or_null(getenv(DB_ENVIRONMENT));
- alternates = xstrdup_or_null(getenv(ALTERNATE_DB_ENVIRONMENT));
-+ quarantine_path =
-+ xstrdup_or_null(getenv(GIT_QUARANTINE_ENVIRONMENT));
- }
- if (!object_dir)
- object_dir = xstrfmt("%s/objects", odb->repo->commondir);
-
- CALLOC_ARRAY(files, 1);
-+ files->quarantine_path = quarantine_path;
- odb_source_init(&files->base, odb, ODB_SOURCE_FILES, object_dir);
-
- hashmap_init(&files->dirs_by_path, odb_files_dir_by_path_cmp, files, 0);
-
- ## odb/source-files.h ##
-@@ odb/source-files.h: struct odb_files_dir {
- * repository. Directories added via alternates are not local.
- */
- bool local;
++ /* Our own repository is not a source of alternate refs. */
++ if (strbuf_realpath(&repo, cb->repo->commondir, 0) &&
++ !fspathcmp(path.buf, repo.buf))
++ goto out;
+
-+ /*
-+ * Whether this directory is borrowed from another repository, so
-+ * that its refs are alternate refs. Not every directory added via
-+ * alternates is borrowed: inside an object quarantine, our own
-+ * object directory is passed to us as an alternate, too. Only set
-+ * for directories added via alternates.
-+ */
-+ bool borrowed;
- };
+ read_alternate_refs(cb->repo, path.buf, cb->fn, cb->payload);

- struct odb_files_dir *odb_files_dir_new(struct object_database *odb, -@@ odb/source-files.h: struct odb_source_files {
- * determined by "core.ignoreCase".
- */
- int dirs_paths_icase;
-+
-+ /*
-+ * The value of GIT_QUARANTINE_PATH when this source was set up from
-+ * the environment (see ODB_NEW_HONOR_ENV), or NULL.
-+ */
-+ char *quarantine_path;
- };
+ out:
+ strbuf_release(&path);
++ strbuf_release(&repo);
+ return 0;
+ }

- /* Allocate and initialize a new object source. */

## t/perf/p5552-push-hidden-refs.sh (new) ##
@@
@@ t/t5618-alternate-refs.sh: test_expect_success 'log --source shows .alternate ma
+ git rev-list --alternate-refs >actual &&
+ test_cmp expect actual
+'
-+
-+# A quarantine is created inside our object directory, so its parent is -+# ourselves, even when that is not under our commondir. -+test_expect_success 'parent of the quarantine is not listed as an alternate' '
-+ test_when_finished "rm -rf store.git" &&
-+ git init --bare store.git &&
-+ git push store.git refs/remotes/origin/one:refs/heads/one &&
-+ quarantine="$(pwd)/store.git/objects/tmp_objdir-test" &&
-+ mkdir "$quarantine" &&
-+
-+ git rev-parse origin/one >expect &&
-+ GIT_OBJECT_DIRECTORY="$quarantine" \
-+ GIT_ALTERNATE_OBJECT_DIRECTORIES="$(pwd)/store.git/objects" \
-+ git rev-list --no-walk --alternate-refs --stdin </dev/null >actual &&
-+ test_cmp expect actual &&
-+
-+ GIT_QUARANTINE_PATH="$quarantine" \
-+ GIT_OBJECT_DIRECTORY="$quarantine" \
-+ GIT_ALTERNATE_OBJECT_DIRECTORIES="$(pwd)/store.git/objects" \
-+ git rev-list --no-walk --alternate-refs --stdin </dev/null >actual &&
-+ test_must_be_empty actual
-+'
+
test_done

 Documentation/rev-list-options.adoc |  5 +++-
 odb.c                               |  8 ++++++
 t/perf/p5552-push-hidden-refs.sh    | 37 ++++++++++++++++++++++++++++
 t/t5547-push-quarantine.sh          | 38 +++++++++++++++++++++++++++++
 t/t5618-alternate-refs.sh           | 12 +++++++++
 5 files changed, 99 insertions(+), 1 deletion(-)
 create mode 100755 t/perf/p5552-push-hidden-refs.sh

index 285fb3809f..6e4c487d1e 100644
@@ -223,7 +223,10 @@ explicitly.
 	Pretend as if all objects mentioned as ref tips of alternate
 	repositories were listed on the command line. An alternate
 	repository is any repository whose object directory is specified
-	in `objects/info/alternates`.  The set of included objects may
+	in `objects/info/alternates`.  The repository's own object
+	directory does not count, even when it is passed in as an
+	alternate, as happens inside the object quarantine of
+	linkgit:git-receive-pack[1].  The set of included objects may
 	be modified by `core.alternateRefsCommand`, etc. See
 	linkgit:git-config[1].
 

index 2c93a9490c..0f78ffed6a 100644
@@ -2,6 +2,7 @@
 #include "abspath.h"
 #include "commit-graph.h"
 #include "config.h"
+#include "dir.h"
 #include "environment.h"
 #include "gettext.h"
 #include "hashmap.h"

@@ -183,6 +184,7 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,
 				  void *payload)
 {
 	struct strbuf path = STRBUF_INIT;
+	struct strbuf repo = STRBUF_INIT;
 	size_t base_len;
 	struct alternate_refs_data *cb = payload;
 

@@ -198,10 +200,16 @@ static int refs_from_alternate_cb(struct odb_files_dir *alternate,
 		goto out;
 	strbuf_setlen(&path, base_len);
 
+	/* Our own repository is not a source of alternate refs. */
+	if (strbuf_realpath(&repo, cb->repo->commondir, 0) &&
+	    !fspathcmp(path.buf, repo.buf))
+		goto out;
+
 	read_alternate_refs(cb->repo, path.buf, cb->fn, cb->payload);
 
 out:
 	strbuf_release(&path);
+	strbuf_release(&repo);
 	return 0;
 }
 

new file mode 100755
index 0000000000..e9aa206298
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='speed of push into a repository with many hidden refs'
+. ./perf-lib.sh
+
+test_perf_fresh_repo
+
+test_expect_success 'set up repository with many hidden refs' '
+	git init --bare dst.git &&
+	test_commit_bulk --ref=refs/hidden/base --id=hidden 1000 &&
+	git rev-list refs/hidden/base >commits &&
+	for i in $(test_seq 200)
+	do
+		sed "s|^\(.*\)$|create refs/hidden/$i/\1 \1|" commits || return 1
+	done >input &&
+	git push dst.git refs/hidden/base &&
+	git -C dst.git update-ref --stdin <input &&
+	git -C dst.git pack-refs --all &&
+	cp -R dst.git dst-visible.git &&
+	git -C dst.git config receive.hideRefs refs/hidden &&
+	git symbolic-ref HEAD refs/heads/main &&
+	test_commit_bulk --id=visible 1 &&
+	git push dst.git main &&
+	git push dst-visible.git main
+'
+
+test_perf 'push, hidden refs' '
+	git commit --allow-empty -m new &&
+	git push -f dst.git HEAD:refs/heads/new
+'
+
+test_perf 'push, no hidden refs' '
+	git commit --allow-empty -m new &&
+	git push -f dst-visible.git HEAD:refs/heads/new
+'
+
+test_done

index 6c14dfc42b..3426aec531 100755
@@ -125,4 +125,42 @@ test_expect_success '.keep file created outside of process is not removed' '
 	test_path_is_missing "${keep%.keep}.pack"
 '
 
+test_expect_success 'quarantine does not list own refs as alternate refs' '
+	git init --bare hidden.git &&
+	git push hidden.git HEAD:refs/heads/main HEAD:refs/hidden/one &&
+	git -C hidden.git config receive.hideRefs refs/hidden &&
+	write_script hidden.git/alternate-refs <<-EOF &&
+	echo "\$1" >>"$(pwd)/alternate-refs.log"
+	EOF
+	git -C hidden.git config core.alternateRefsCommand ./alternate-refs &&
+	test_commit hidden-push &&
+	git push hidden.git HEAD:refs/heads/new &&
+	test_path_is_missing alternate-refs.log
+'
+
+test_expect_success 'quarantine lists refs of other alternates' '
+	test_when_finished "rm -f alternate-refs.log" &&
+	git init --bare other.git &&
+	git push other.git HEAD:refs/heads/other &&
+	echo "$(pwd)/other.git/objects" >hidden.git/objects/info/alternates &&
+	test_commit hidden-push-2 &&
+	git push hidden.git HEAD:refs/heads/new2 &&
+	(cd other.git && pwd -P) >expect &&
+	sort -u alternate-refs.log >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rev-list --alternate-refs in a hook skips own refs' '
+	test_when_finished "rm -rf altref.git" &&
+	git init --bare altref.git &&
+	git push altref.git HEAD:refs/heads/main &&
+	test_hook -C altref.git pre-receive <<-\EOF &&
+	git rev-list --no-walk --alternate-refs --stdin </dev/null \
+		>../hook-alternate-refs
+	EOF
+	test_commit altref-push &&
+	git push altref.git HEAD:refs/heads/new &&
+	test_must_be_empty hook-alternate-refs
+'
+
 test_done

index 2fb6d549d3..e64e190c48 100755
@@ -58,4 +58,16 @@ test_expect_success 'log --source shows .alternate marker' '
 	test_cmp expect actual
 '
 
+# Our own object directory as an alternate is not a source of alternate
+# refs, but the alternates it brings in with it still are.
+test_expect_success 'own object directory as alternate lists only real alternates' '
+	test_when_finished "rm -rf other-objects" &&
+	mkdir other-objects &&
+	git rev-list --remotes=origin >expect &&
+	GIT_OBJECT_DIRECTORY="$(pwd)/other-objects" \
+	GIT_ALTERNATE_OBJECT_DIRECTORIES="$(pwd)/.git/objects" \
+		git rev-list --alternate-refs >actual &&
+	test_cmp expect actual
+'
+
 test_done

base-commit: 750f5de7dfe9a4062f95088212b063f189d50e91

gitgitgadget

"Kristofer Karlsson via GitGitGadget" <[email protected]>

Thanks. Will replace.