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

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

[Kristofer Karlsson via GitGitGadget]

From: Kristofer Karlsson <krka@spotify.com>

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.

Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
    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

diff --git a/odb.c b/odb.c
index 2c93a9490c..0cda333c4e 100644
--- a/odb.c
+++ b/odb.c
@@ -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;
 }
 
diff --git a/t/perf/p5552-push-hidden-refs.sh b/t/perf/p5552-push-hidden-refs.sh
new file mode 100755
index 0000000000..e9aa206298
--- /dev/null
+++ b/t/perf/p5552-push-hidden-refs.sh
@@ -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
diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
index 1b7097179e..855a1d2c0a 100755
--- a/t/t5547-push-quarantine.sh
+++ b/t/t5547-push-quarantine.sh
@@ -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

Re: [PATCH] odb: do not list our own refs as alternate refs in the quarantine

[Justin Tobler]

On 26/10/28 03:18PM, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
> 
> 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 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.

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

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

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

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

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.

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

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

Re: [PATCH] odb: do not list our own refs as alternate refs in the quarantine

[Junio C Hamano]

"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:

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



> @@ -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.
> +	 */

In other words, if we tell our quarantine child where our real
object directory is, say, via the 'GIT_QUARANTINE_OBJECT_DIRECTORY'
environment variable, can the child notice the variable and identify
which of the passed alternates is the real object directory, so that
it knows not to use the refs associated with it?

It is a bit of a shame that tmp_objdir_create() exports the same
value as DB_ENVIRONMENT in GIT_QUARANTINE_ENVIRONMENT, as the only
existing use of the latter is to check if the variable is exported
or not---in other words, to carry a single bit of information ("are
we in quarantine or not").  I wonder if we can change its value to
name the parent process's real object directory instead?  Then we
would not need to export yet another variable to carry the
information we need.

Re: [PATCH] odb: do not list our own refs as alternate refs in the quarantine

[Kristofer Karlsson]

On Thu, 29 Oct 2026 at 17:57, Justin Tobler <jltobler@gmail.com> wrote:
>
> 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.

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.

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

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

Re: [PATCH] odb: do not list our own refs as alternate refs in the quarantine

[Kristofer Karlsson]

On Thu, 29 Oct 2026 at 18:24, Junio C Hamano <gitster@pobox.com> wrote:
>
> In other words, if we tell our quarantine child where our real
> object directory is, say, via the 'GIT_QUARANTINE_OBJECT_DIRECTORY'
> environment variable, can the child notice the variable and identify
> which of the passed alternates is the real object directory, so that
> it knows not to use the refs associated with it?
>
> It is a bit of a shame that tmp_objdir_create() exports the same
> value as DB_ENVIRONMENT in GIT_QUARANTINE_ENVIRONMENT, as the only
> existing use of the latter is to check if the variable is exported
> or not---in other words, to carry a single bit of information ("are
> we in quarantine or not").  I wonder if we can change its value to
> name the parent process's real object directory instead?  Then we
> would not need to export yet another variable to carry the
> information we need.

Agreed, I think that is a reasonable way forward to a concrete fix.

I started working on a v2 based on your suggestion there.
First I tried tweaking GIT_QUARANTINE_PATH to
point to the real object dir, but I realized that might be
unsafe since some hooks may depend on the semantics of
that env variable, so it would be a breaking change.

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

Re: [PATCH] odb: do not list our own refs as alternate refs in the quarantine

[Junio C Hamano]

Kristofer Karlsson <krka@spotify.com> writes:

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

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.

Re: [PATCH] odb: do not list our own refs as alternate refs in the quarantine

[Jeff King]

On Fri, Oct 30, 2026 at 12:51:43PM +0100, Kristofer Karlsson wrote:

> > It is a bit of a shame that tmp_objdir_create() exports the same
> > value as DB_ENVIRONMENT in GIT_QUARANTINE_ENVIRONMENT, as the only
> > existing use of the latter is to check if the variable is exported
> > or not---in other words, to carry a single bit of information ("are
> > we in quarantine or not").  I wonder if we can change its value to
> > name the parent process's real object directory instead?  Then we
> > would not need to export yet another variable to carry the
> > information we need.
> 
> Agreed, I think that is a reasonable way forward to a concrete fix.
> 
> I started working on a v2 based on your suggestion there.
> First I tried tweaking GIT_QUARANTINE_PATH to
> point to the real object dir, but I realized that might be
> unsafe since some hooks may depend on the semantics of
> that env variable, so it would be a breaking change.

Yes, the reason it's a path and not a single bit is exactly for this
idea. ;)

I never upstreamed this final bit of the quarantine code because it
always felt a bit ugly. But how it worked at GitHub at the time (and
possibly still today? I don't know) was this.

The goal is that we want to reject excessively large objects on pushes,
for security (integer overflows, etc) and performance (pushing up 5GB
blobs is mostly an anti-pattern). It's nice to do it in index-pack for
two reasons. One is that we already have the data there, so it's a free
check (as opposed to a hook running a separate scan over the objects).
And two is that it's our first look at the objects, so it reduces attack
surface to reject the big ones early.

Dying unceremoniously for large trees and commits is fine. Nobody is
sending you 100MB trees unless they're an attacker. But with blobs, it's
almost always a simple user mistake, so you'd want a more gentle message
(and possibly even to do things like finding which paths point to the
blob).

Our solution was for index-pack to write the set of large blobs to
$GIT_QUARANTINE_PATH/large-objects. And then a hook is able to read that
and produce whatever nice message you want, but without going to the
expensive of running rev-list or similar just to find out that all of
the blobs are fine.

I had envisioned a world where we might use it for more information, or
two hooks could even use it as a rendezvous point, basically as "here's
an atomic tempdir for this whole push". But I don't remember if it was
ever used for that (the most likely candidate, from memory, was pushing
some secret-scanning code into index-pack, since it already has the
object bytes in memory, and then letting it generate a report).

> Then I started adding a second env variable.  That worked
> and it was not _too_ messy though I realized it was a bit
> redundant.  We can simply deduce the real object directory
> from the quarantine directory -- it's the parent, based on
> how tmp_objdir_create() works today -- so I built my preliminary
> v2 on that.
> 
> It feels like a slightly icky assumption to make, but it
> feels safe since I added tests to cover it, and does not seem
> possible to override outside of git itself.

I don't know, it actually sounds quite sensible: we do not show
--alternate-refs from an alternate source that is ourselves, because
we'd already have looked at those refs. The quarantine and tmp-objdir
code sets up this slightly odd alternate situation, but somebody could
also do it externally by setting up a few environment variables.

-Peff

[PATCH v2] odb: do not list our own refs as alternate refs in the quarantine

[Kristofer Karlsson via GitGitGadget]

From: Kristofer Karlsson <krka@spotify.com>

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.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
    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 <peff@peff.net>
          Signed-off-by: Kristofer Karlsson <krka@spotify.com>
      
     + ## 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

diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
index 285fb3809f..6e4c487d1e 100644
--- a/Documentation/rev-list-options.adoc
+++ b/Documentation/rev-list-options.adoc
@@ -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].
 
diff --git a/odb.c b/odb.c
index 2c93a9490c..646a498052 100644
--- a/odb.c
+++ b/odb.c
@@ -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"))
diff --git a/odb/source-files.c b/odb/source-files.c
index 5b4277fbfd..223af45dd9 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -3,6 +3,8 @@
 #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"
@@ -73,6 +75,7 @@ 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);
@@ -348,6 +351,45 @@ 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)
@@ -359,6 +401,7 @@ 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;
@@ -1256,15 +1299,19 @@ 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);
diff --git a/odb/source-files.h b/odb/source-files.h
index 837e890304..b404937fa1 100644
--- a/odb/source-files.h
+++ b/odb/source-files.h
@@ -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. */
diff --git a/t/perf/p5552-push-hidden-refs.sh b/t/perf/p5552-push-hidden-refs.sh
new file mode 100755
index 0000000000..e9aa206298
--- /dev/null
+++ b/t/perf/p5552-push-hidden-refs.sh
@@ -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
diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
index 6c14dfc42b..3426aec531 100755
--- a/t/t5547-push-quarantine.sh
+++ b/t/t5547-push-quarantine.sh
@@ -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
diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
index 2fb6d549d3..d52e426db1 100755
--- a/t/t5618-alternate-refs.sh
+++ b/t/t5618-alternate-refs.sh
@@ -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

Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine

[Junio C Hamano]

"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:

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

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.

Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>>     Changes since v1:
>>     ...
>>        quarantine outside of the commondir.
>
> 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.

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 <gitster@pobox.com>
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

diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
remerge CONFLICT (content): Merge conflict in Documentation/rev-list-options.adoc
index 4e92cf6456..d20c5f6372 100644
--- a/Documentation/rev-list-options.adoc
+++ b/Documentation/rev-list-options.adoc
@@ -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
diff --git a/odb/source-files.c b/odb/source-files.c
remerge CONFLICT (content): Merge conflict in odb/source-files.c
index 4239cb493c..8ce437fdc2 100644
--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ -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;
diff --git a/odb/source-files.h b/odb/source-files.h
remerge CONFLICT (content): Merge conflict in odb/source-files.h
index 7afc2f28ad..9117dbea5b 100644
--- a/odb/source-files.h
+++ b/odb/source-files.h
@@ -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. */
diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
remerge CONFLICT (content): Merge conflict in t/t5618-alternate-refs.sh
index cefc37797c..048c7c63c1 100755
--- a/t/t5618-alternate-refs.sh
+++ b/t/t5618-alternate-refs.sh
@@ -58,34 +58,6 @@ test_expect_success 'log --source shows .alternate marker' '
 	test_cmp expect actual
 '
 
-<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 1707829739 (Merge branch 'kk/connectivity-check-ignore-replace' into seen)
-test_expect_success 'connectivity check of a fetch excludes alternate refs' '
-	GIT_TRACE2_EVENT="$(pwd)/fetch.trace" git fetch origin &&
-	test_subcommand_flex git rev-list --objects --stdin --not --all \
-		--alternate-refs --quiet <fetch.trace
-'
-
-test_expect_success 'connectivity check of a deepening fetch excludes alternate refs' '
-	GIT_TRACE2_EVENT="$(pwd)/deepen.trace" git fetch --depth=1 origin one &&
-	test_subcommand_flex git rev-list --objects --stdin --not \
-		--alternate-refs --quiet <deepen.trace
-'
-
-test_expect_success 'refs of a shallow alternate are ignored' '
-	git clone --depth=1 --no-local . shallow &&
-	test_commit -C shallow shallow-only &&
-	shallow_tip=$(git -C shallow rev-parse HEAD) &&
-	cp .git/objects/info/alternates alternates.bak &&
-	test_when_finished "mv alternates.bak .git/objects/info/alternates" &&
-	echo "$(pwd)/shallow/.git/objects" >>.git/objects/info/alternates &&
-	git cat-file -e $shallow_tip &&
-	git rev-list --remotes=origin >expect &&
-	git rev-list --alternate-refs >actual &&
-	test_cmp expect actual
-'
-
-|||||||||||||||||||||||||||||||| ebdc087dc9
-================================
 # 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' '
@@ -120,5 +92,29 @@ test_expect_success 'parent of the quarantine is not listed as an alternate' '
 	test_must_be_empty actual
 '
 
->>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 3f91b4bc4b (odb: do not list our own refs as alternate refs in the quarantine)
+test_expect_success 'connectivity check of a fetch excludes alternate refs' '
+	GIT_TRACE2_EVENT="$(pwd)/fetch.trace" git fetch origin &&
+	test_subcommand_flex git rev-list --objects --stdin --not --all \
+		--alternate-refs --quiet <fetch.trace
+'
+
+test_expect_success 'connectivity check of a deepening fetch excludes alternate refs' '
+	GIT_TRACE2_EVENT="$(pwd)/deepen.trace" git fetch --depth=1 origin one &&
+	test_subcommand_flex git rev-list --objects --stdin --not \
+		--alternate-refs --quiet <deepen.trace
+'
+
+test_expect_success 'refs of a shallow alternate are ignored' '
+	git clone --depth=1 --no-local . shallow &&
+	test_commit -C shallow shallow-only &&
+	shallow_tip=$(git -C shallow rev-parse HEAD) &&
+	cp .git/objects/info/alternates alternates.bak &&
+	test_when_finished "mv alternates.bak .git/objects/info/alternates" &&
+	echo "$(pwd)/shallow/.git/objects" >>.git/objects/info/alternates &&
+	git cat-file -e $shallow_tip &&
+	git rev-list --remotes=origin >expect &&
+	git rev-list --alternate-refs >actual &&
+	test_cmp expect actual
+'
+
 test_done

Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine

[Kristofer Karlsson]

On Tue, 3 Nov 2026 at 02:12, Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
> > writes:
> >
> >>     Changes since v1:
> >>     ...
> >>        quarantine outside of the commondir.
> >
> > 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.
>
> 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'.
>

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

Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine

[Patrick Steinhardt]

On Mon, Nov 02, 2026 at 11:45:33AM +0000, Kristofer Karlsson via GitGitGadget wrote:
> diff --git a/odb/source-files.c b/odb/source-files.c
> index 5b4277fbfd..223af45dd9 100644
> --- a/odb/source-files.c
> +++ b/odb/source-files.c
> @@ -348,6 +351,45 @@ 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

Or it may be derived from `files://<payload>` with the object storage
format extension.

> + * 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;
> +	}

So this here isn't fully correct as it does not account for all cases.
We'd have to read the object storage format extension to figure out
where the primary object directory lives.

> @@ -1256,15 +1299,19 @@ 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);

This makes me wonder whether the better fix would be to stop modifying
the primary object directory and instead we simply append the quarantine
directory to alternates. The presence of `GIT_QUARANTINE_ENVIRONMENT`
could then serve as an indicator that the child process is not allowed
to write objects.

That's a much bigger change in semantics though, and the risk of causing
a regression would be quite a bit higher. But the upside is that we
wouldn't have to juggle alternates anymore, and your case with alternate
refs would "just work".

Patrick

Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine

[Kristofer Karlsson]

On Tue, Nov 03, 2026 at 12:00 PM Patrick Steinhardt <ps@pks.im> wrote:
> So this here isn't fully correct as it does not account for all cases.
> We'd have to read the object storage format extension to figure out
> where the primary object directory lives.

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:

--- a/odb/source-files.c
+++ b/odb/source-files.c
@@ 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.

> This makes me wonder whether the better fix would be to stop modifying
> the primary object directory and instead we simply append the quarantine
> directory to alternates.
[...]

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

Re: [PATCH v2] odb: do not list our own refs as alternate refs in the quarantine

[Patrick Steinhardt]

On Tue, Nov 03, 2026 at 01:05:29PM +0100, Kristofer Karlsson wrote:
> On Tue, Nov 03, 2026 at 12:00 PM Patrick Steinhardt <ps@pks.im> wrote:
> > So this here isn't fully correct as it does not account for all cases.
> > We'd have to read the object storage format extension to figure out
> > where the primary object directory lives.
> 
> 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:

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

> > This makes me wonder whether the better fix would be to stop modifying
> > the primary object directory and instead we simply append the quarantine
> > directory to alternates.
> [...]
> 
> 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.

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