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