threads / patch / 51400

patchcheck_everything_connected: assume alternate ref tips are valid

Subject: [PATCH] check_everything_connected: assume alternate ref tips are valid

## tl;dr

20 messages between Jun 28, 2019 and Jul 3, 2019. Diffs are folded; open one to read it.

replies: 19people: 5as markdown or json

Jeff King· Jun 28, 2019, 10:11 UTC · lore

When we receive a remote ref update to sha1 "X", we want to check that we have all of the objects needed by "X". We can assume that our repository is not currently corrupted, and therefore if we have a ref pointing at "Y", we have all of its objects. So we can stop our traversal from "X" as soon as we hit "Y".

If we make the same non-corruption assumption about any repositories we use to store alternates, then we can also use their ref tips to shorten the traversal.

This is especially useful when cloning with "--reference", as we otherwise do not have any local refs to check against, and have to traverse the whole history, even though the other side may have sent us few or no objects. Here are results for the included perf test (which shows off more or less the maximal savings, getting one new commit and sharing the whole history):

Test HEAD^ HEAD -------------------------------------------------------------------- [on git.git] 5600.3: clone --reference 2.94(2.86+0.08) 0.09(0.08+0.01) -96.9% [on linux.git] 5600.3: clone --reference 45.74(45.34+0.41) 0.36(0.30+0.08) -99.2%

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/rev-list-options.txt |  8 ++++
 connected.c                        |  1 +
 revision.c                         | 30 +++++++++++++++
 t/perf/p5600-clone-reference.sh    | 27 ++++++++++++++
 t/t5618-alternate-refs.sh          | 60 ++++++++++++++++++++++++++++++
 5 files changed, 126 insertions(+)
 create mode 100755 t/perf/p5600-clone-reference.sh
 create mode 100755 t/t5618-alternate-refs.sh
Show changes to 5 files +126 −0

Documentation/rev-list-options.txt, connected.c, revision.c, t/perf/p5600-clone-reference.sh, t/t5618-alternate-refs.sh

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 71a1fcc093..90a2c027ea 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -182,6 +182,14 @@ explicitly.
 	Pretend as if all objects mentioned by reflogs are listed on the
 	command line as `<commit>`.
 
+--alternate-refs::
+	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
+	be modified by `core.alternateRefsCommand`, etc. See
+	linkgit:git-config[1].
+
 --single-worktree::
 	By default, all working trees will be examined by the
 	following options when there are more than one (see
diff --git a/connected.c b/connected.c
index 1ab481fed6..cd9b324afa 100644
--- a/connected.c
+++ b/connected.c
@@ -80,6 +80,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 		argv_array_push(&rev_list.args, "--all");
 	}
 	argv_array_push(&rev_list.args, "--quiet");
+	argv_array_push(&rev_list.args, "--alternate-refs");
 	if (opt->progress)
 		argv_array_pushf(&rev_list.args, "--progress=%s",
 				 _("Checking connectivity"));
diff --git a/revision.c b/revision.c
index 621feb9df7..b9be08e9df 100644
--- a/revision.c
+++ b/revision.c
@@ -28,6 +28,7 @@
 #include "commit-graph.h"
 #include "prio-queue.h"
 #include "hashmap.h"
+#include "transport.h"
 
 volatile show_early_output_fn_t show_early_output;
 
@@ -1554,6 +1555,32 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 	free_worktrees(worktrees);
 }
 
+struct add_alternate_refs_data {
+	struct rev_info *revs;
+	unsigned int flags;
+};
+
+static void add_one_alternate_ref(const struct object_id *oid,
+				  void *vdata)
+{
+	const char *name = ".alternate";
+	struct add_alternate_refs_data *data = vdata;
+	struct object *obj;
+
+	obj = get_reference(data->revs, name, oid, data->flags);
+	add_rev_cmdline(data->revs, obj, name, REV_CMD_REV, data->flags);
+	add_pending_object(data->revs, obj, name);
+}
+
+static void add_alternate_refs_to_pending(struct rev_info *revs,
+					  unsigned int flags)
+{
+	struct add_alternate_refs_data data;
+	data.revs = revs;
+	data.flags = flags;
+	for_each_alternate_ref(add_one_alternate_ref, &data);
+}
+
 static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
 			    int exclude_parent)
 {
@@ -1956,6 +1983,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	    !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk") ||
 	    !strcmp(arg, "--bisect") || starts_with(arg, "--glob=") ||
 	    !strcmp(arg, "--indexed-objects") ||
+	    !strcmp(arg, "--alternate-refs") ||
 	    starts_with(arg, "--exclude=") ||
 	    starts_with(arg, "--branches=") || starts_with(arg, "--tags=") ||
 	    starts_with(arg, "--remotes=") || starts_with(arg, "--no-walk="))
@@ -2442,6 +2470,8 @@ static int handle_revision_pseudo_opt(const char *submodule,
 		add_reflogs_to_pending(revs, *flags);
 	} else if (!strcmp(arg, "--indexed-objects")) {
 		add_index_objects_to_pending(revs, *flags);
+	} else if (!strcmp(arg, "--alternate-refs")) {
+		add_alternate_refs_to_pending(revs, *flags);
 	} else if (!strcmp(arg, "--not")) {
 		*flags ^= UNINTERESTING | BOTTOM;
 	} else if (!strcmp(arg, "--no-walk")) {
diff --git a/t/perf/p5600-clone-reference.sh b/t/perf/p5600-clone-reference.sh
new file mode 100755
index 0000000000..68fed66347
--- /dev/null
+++ b/t/perf/p5600-clone-reference.sh
@@ -0,0 +1,27 @@
+#!/bin/sh
+
+test_description='speed of clone --reference'
+. ./perf-lib.sh
+
+test_perf_default_repo
+
+test_expect_success 'create shareable repository' '
+	git clone --bare . shared.git
+'
+
+test_expect_success 'advance base repository' '
+	# Do not use test_commit here; its test_tick will
+	# use some ancient hard-coded date. The resulting clock
+	# skew will cause pack-objects to traverse in a very
+	# sub-optimal order, skewing the results.
+	echo content >new-file-that-does-not-exist &&
+	git add new-file-that-does-not-exist &&
+	git commit -m "new commit"
+'
+
+test_perf 'clone --reference' '
+	rm -rf dst.git &&
+	git clone --no-local --bare --reference shared.git . dst.git
+'
+
+test_done
diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
new file mode 100755
index 0000000000..3353216f09
--- /dev/null
+++ b/t/t5618-alternate-refs.sh
@@ -0,0 +1,60 @@
+#!/bin/sh
+
+test_description='test handling of --alternate-refs traversal'
+. ./test-lib.sh
+
+# Avoid test_commit because we want a specific and known set of refs:
+#
+#  base -- one
+#      \      \
+#       two -- merged
+#
+# where "one" and "two" are on separate refs, and "merged" is available only in
+# the dependent child repository.
+test_expect_success 'set up local refs' '
+	git checkout -b one &&
+	test_tick &&
+	git commit --allow-empty -m base &&
+	test_tick &&
+	git commit --allow-empty -m one &&
+	git checkout -b two HEAD^ &&
+	test_tick &&
+	git commit --allow-empty -m two
+'
+
+# We'll enter the child repository after it's set up since that's where
+# all of the subsequent tests will want to run (and it's easy to forget a
+# "-C child" and get nonsense results).
+test_expect_success 'set up shared clone' '
+	git clone -s . child &&
+	cd child &&
+	git merge origin/one
+'
+
+test_expect_success 'rev-list --alternate-refs' '
+	git rev-list --remotes=origin >expect &&
+	git rev-list --alternate-refs >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rev-list --not --alternate-refs' '
+	git rev-parse HEAD >expect &&
+	git rev-list HEAD --not --alternate-refs >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'limiting with alternateRefsPrefixes' '
+	test_config core.alternateRefsPrefixes refs/heads/one &&
+	git rev-list origin/one >expect &&
+	git rev-list --alternate-refs >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --source shows .alternate marker' '
+	git log --oneline --source --remotes=origin >expect.orig &&
+	sed "s/origin.* /.alternate /" <expect.orig >expect &&
+	git log --oneline --source --alternate-refs >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
2.22.0.775.g4ba9815492
Jeff King· Jun 28, 2019, 10:18 UTC · re: Jeff King · lore

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

On Fri, Jun 28, 2019 at 06:11:31AM -0400, Jeff King wrote:
> Subject: [PATCH] check_everything_connected: assume alternate ref tips...

Heh. This subject should just be "check_connected" these days. I wrote this patch originally many years ago, before 7043c7071c (check_everything_connected: use a struct with named options, 2016-07-15) gave the function a shorter name.

But the patch has been updated recently to match other updates to the alternates code. In particular, in the original "rev-list --alternate-refs" behaved as if all of the refs from the alternate were mentioned on the command line, and you could use --exclude, etc as if they were real refs. But we've since simplified the alternates interface to just get a list of oids, so now it behaves more like "--indexed-objects" in that it acts as if those oids were mentioned on the command line.

I think "--alternate-refs" is still the right name, though, since we're just giving the oids at the ref tips. And I think the included documentation makes this clear.

-Peff
Derrick Stolee· Jun 28, 2019, 12:51 UTC · re: Jeff King · lore

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

On 6/28/2019 6:11 AM, Jeff King wrote:
Show 9 quoted lines
> When we receive a remote ref update to sha1 "X", we want to check that
> we have all of the objects needed by "X". We can assume that our
> repository is not currently corrupted, and therefore if we have a ref
> pointing at "Y", we have all of its objects. So we can stop our
> traversal from "X" as soon as we hit "Y".
> 
> If we make the same non-corruption assumption about any repositories we
> use to store alternates, then we can also use their ref tips to shorten
> the traversal.

I was confused by this paragraph, because I didn't know about for_each_alternate_ref() and how refs_From_alternate_cb() will strip the "/objects" and append "/refs" to check refs if they exist. All of that logic is in transport.c but used by fetch-pack.c and builtin/receive-pack.c. But now we are adding to revision.c, so the restriction to "this helps data transfer" is getting murkier.

Is this something that should be extracted to the object-store layer? Or is it so tricky to use that we shouldn't make it too easy to fall into a bad pattern?

Show 13 quoted lines
> This is especially useful when cloning with "--reference", as we
> otherwise do not have any local refs to check against, and have to
> traverse the whole history, even though the other side may have sent us
> few or no objects. Here are results for the included perf test (which
> shows off more or less the maximal savings, getting one new commit and
> sharing the whole history):
> 
> Test                        HEAD^             HEAD
> --------------------------------------------------------------------
> [on git.git]
> 5600.3: clone --reference   2.94(2.86+0.08)   0.09(0.08+0.01) -96.9%
> [on linux.git]
> 5600.3: clone --reference   45.74(45.34+0.41)   0.36(0.30+0.08) -99.2%
It's really hard to argue with numbers like these. Kudos!
Show 8 quoted lines
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Documentation/rev-list-options.txt |  8 ++++
>  connected.c                        |  1 +
>  revision.c                         | 30 +++++++++++++++
>  t/perf/p5600-clone-reference.sh    | 27 ++++++++++++++
>  t/t5618-alternate-refs.sh          | 60 ++++++++++++++++++++++++++++++

Other than the high-level questions above, the code and tests look good to me.

Thanks, -Stolee

Jeff King· Jun 29, 2019, 07:43 UTC · re: Derrick Stolee · lore

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

On Fri, Jun 28, 2019 at 08:51:04AM -0400, Derrick Stolee wrote:
Show 18 quoted lines
> On 6/28/2019 6:11 AM, Jeff King wrote:
> > When we receive a remote ref update to sha1 "X", we want to check that
> > we have all of the objects needed by "X". We can assume that our
> > repository is not currently corrupted, and therefore if we have a ref
> > pointing at "Y", we have all of its objects. So we can stop our
> > traversal from "X" as soon as we hit "Y".
> > 
> > If we make the same non-corruption assumption about any repositories we
> > use to store alternates, then we can also use their ref tips to shorten
> > the traversal.
> 
> I was confused by this paragraph, because I didn't know about
> for_each_alternate_ref() and how refs_From_alternate_cb() will
> strip the "/objects" and append "/refs" to check refs if they
> exist. All of that logic is in transport.c but used by
> fetch-pack.c and builtin/receive-pack.c. But now we are adding
> to revision.c, so the restriction to "this helps data transfer"
> is getting murkier.

Using it for data transfer is still the main thing for our internal calls, but I think it's worth exposing it for general use via rev-list. I imagine it would mostly be for poking around and debugging, but it should allow things like:

  # what do we have that our alternate does not
  git rev-list --all --not --alternate-refs
> Is this something that should be extracted to the object-store
> layer? Or is it so tricky to use that we shouldn't make it too
> easy to fall into a bad pattern?

I'm not sure what you have in mind, exactly. If you are asking whether there are more places that alternate refs could be used, I can't think of any. If you are asking whether this is in the wrong place, no, I think it's the right place. :)

-Peff
Derrick Stolee· Jul 1, 2019, 12:25 UTC · re: Jeff King · lore

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

On 6/29/2019 3:43 AM, Jeff King wrote:
Show 28 quoted lines
> On Fri, Jun 28, 2019 at 08:51:04AM -0400, Derrick Stolee wrote:
> 
>> On 6/28/2019 6:11 AM, Jeff King wrote:
>>> When we receive a remote ref update to sha1 "X", we want to check that
>>> we have all of the objects needed by "X". We can assume that our
>>> repository is not currently corrupted, and therefore if we have a ref
>>> pointing at "Y", we have all of its objects. So we can stop our
>>> traversal from "X" as soon as we hit "Y".
>>>
>>> If we make the same non-corruption assumption about any repositories we
>>> use to store alternates, then we can also use their ref tips to shorten
>>> the traversal.
>>
>> I was confused by this paragraph, because I didn't know about
>> for_each_alternate_ref() and how refs_From_alternate_cb() will
>> strip the "/objects" and append "/refs" to check refs if they
>> exist. All of that logic is in transport.c but used by
>> fetch-pack.c and builtin/receive-pack.c. But now we are adding
>> to revision.c, so the restriction to "this helps data transfer"
>> is getting murkier.
> 
> Using it for data transfer is still the main thing for our internal
> calls, but I think it's worth exposing it for general use via rev-list.
> I imagine it would mostly be for poking around and debugging, but it
> should allow things like:
> 
>   # what do we have that our alternate does not
>   git rev-list --all --not --alternate-refs

So this is an example where the alternate refs are being used without any network activity.

Show 8 quoted lines
>> Is this something that should be extracted to the object-store
>> layer? Or is it so tricky to use that we shouldn't make it too
>> easy to fall into a bad pattern?
> 
> I'm not sure what you have in mind, exactly. If you are asking whether
> there are more places that alternate refs could be used, I can't think
> of any. If you are asking whether this is in the wrong place, no, I
> think it's the right place. :)

Just double-checking that it is appropriate for revision.c to take dependence on transport.h instead of moving the alternate ref stuff into a different header file. I trust your opinion.

-Stolee
Jeff King· Jul 1, 2019, 12:59 UTC · re: Derrick Stolee · lore

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

On Mon, Jul 01, 2019 at 08:25:09AM -0400, Derrick Stolee wrote:
Show 8 quoted lines
> > I'm not sure what you have in mind, exactly. If you are asking whether
> > there are more places that alternate refs could be used, I can't think
> > of any. If you are asking whether this is in the wrong place, no, I
> > think it's the right place. :)
> 
> Just double-checking that it is appropriate for revision.c to take
> dependence on transport.h instead of moving the alternate ref stuff
> into a different header file. I trust your opinion.
Ah, I see. I misunderstood you before.

Yes, this is weakening the ties of the feature to the transport code. Traditionally transport-oriented code was the only user, but it also used the upload-pack transport under the hood to access the alternate (that was changed a while ago to for-each-ref for speed).

I don't think there's any functional difference in having it there, but it could be moved to live alongside foreach_alt_odb() in sha1-file.c.

-Peff
Jeff King· Jul 1, 2019, 13:17 UTC · re: Jeff King · lore

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

On Mon, Jul 01, 2019 at 08:59:45AM -0400, Jeff King wrote:
Show 7 quoted lines
> Yes, this is weakening the ties of the feature to the transport code.
> Traditionally transport-oriented code was the only user, but it also
> used the upload-pack transport under the hood to access the alternate
> (that was changed a while ago to for-each-ref for speed).
> 
> I don't think there's any functional difference in having it there, but
> it could be moved to live alongside foreach_alt_odb() in sha1-file.c.

Looks like this hasn't quite hit 'next' yet, so perhaps we can reorganize it as a preparatory patch.

  [1/2]: object-store.h: move for_each_alternate_ref() from transport.h
  [2/2]: check_everything_connected: assume alternate ref tips are valid
 Documentation/rev-list-options.txt |  8 +++
 builtin/receive-pack.c             |  1 -
 connected.c                        |  1 +
 object-store.h                     |  2 +
 revision.c                         | 29 +++++++++
 sha1-file.c                        | 97 ++++++++++++++++++++++++++++++
 t/perf/p5600-clone-reference.sh    | 27 +++++++++
 t/t5618-alternate-refs.sh          | 60 ++++++++++++++++++
 transport.c                        | 97 ------------------------------
 transport.h                        |  2 -
 10 files changed, 224 insertions(+), 100 deletions(-)
 create mode 100755 t/perf/p5600-clone-reference.sh
 create mode 100755 t/t5618-alternate-refs.sh
-Peff
Jeff King· Jul 1, 2019, 13:17 UTC · re: Jeff King · lore

[PATCH 1/2] object-store.h: move for_each_alternate_ref() from transport.h

There's nothing inherently transport-related about enumerating the alternate ref tips. The code has lived in transport.[ch] because the only use so far had been advertising available tips during transport. But it could be used for more, and a future patch will teach rev-list to access these refs.

Let's move it alongside the other alt-odb code, declaring it in object-store.h with the implementation in sha1-file.c.

This lets us drop the inclusion of transport.h from receive-pack, which perhaps shows how it was misplaced (though receive-pack is about transporting objects, transport.h is mostly about the client side).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/receive-pack.c |  1 -
 object-store.h         |  2 +
 sha1-file.c            | 97 ++++++++++++++++++++++++++++++++++++++++++
 transport.c            | 97 ------------------------------------------
 transport.h            |  2 -
 5 files changed, 99 insertions(+), 100 deletions(-)
Show changes to 5 files +99 −100

builtin/receive-pack.c, object-store.h, sha1-file.c, transport.c, transport.h

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 77b7122456..557e686d5d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -12,7 +12,6 @@
 #include "object.h"
 #include "remote.h"
 #include "connect.h"
-#include "transport.h"
 #include "string-list.h"
 #include "sha1-array.h"
 #include "connected.h"
diff --git a/object-store.h b/object-store.h
index 49f56ab8d9..7f7b3cdd80 100644
--- a/object-store.h
+++ b/object-store.h
@@ -33,6 +33,8 @@ void prepare_alt_odb(struct repository *r);
 char *compute_alternate_path(const char *path, struct strbuf *err);
 typedef int alt_odb_fn(struct object_directory *, void *);
 int foreach_alt_odb(alt_odb_fn, void*);
+typedef void alternate_ref_fn(const struct object_id *oid, void *);
+void for_each_alternate_ref(alternate_ref_fn, void *);
 
 /*
  * Add the directory to the on-disk alternates file; the new entry will also
diff --git a/sha1-file.c b/sha1-file.c
index 888b6024d5..3c273c076a 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -743,6 +743,103 @@ char *compute_alternate_path(const char *path, struct strbuf *err)
 	return ref_git;
 }
 
+static void fill_alternate_refs_command(struct child_process *cmd,
+					const char *repo_path)
+{
+	const char *value;
+
+	if (!git_config_get_value("core.alternateRefsCommand", &value)) {
+		cmd->use_shell = 1;
+
+		argv_array_push(&cmd->args, value);
+		argv_array_push(&cmd->args, repo_path);
+	} else {
+		cmd->git_cmd = 1;
+
+		argv_array_pushf(&cmd->args, "--git-dir=%s", repo_path);
+		argv_array_push(&cmd->args, "for-each-ref");
+		argv_array_push(&cmd->args, "--format=%(objectname)");
+
+		if (!git_config_get_value("core.alternateRefsPrefixes", &value)) {
+			argv_array_push(&cmd->args, "--");
+			argv_array_split(&cmd->args, value);
+		}
+	}
+
+	cmd->env = local_repo_env;
+	cmd->out = -1;
+}
+
+static void read_alternate_refs(const char *path,
+				alternate_ref_fn *cb,
+				void *data)
+{
+	struct child_process cmd = CHILD_PROCESS_INIT;
+	struct strbuf line = STRBUF_INIT;
+	FILE *fh;
+
+	fill_alternate_refs_command(&cmd, path);
+
+	if (start_command(&cmd))
+		return;
+
+	fh = xfdopen(cmd.out, "r");
+	while (strbuf_getline_lf(&line, fh) != EOF) {
+		struct object_id oid;
+		const char *p;
+
+		if (parse_oid_hex(line.buf, &oid, &p) || *p) {
+			warning(_("invalid line while parsing alternate refs: %s"),
+				line.buf);
+			break;
+		}
+
+		cb(&oid, data);
+	}
+
+	fclose(fh);
+	finish_command(&cmd);
+}
+
+struct alternate_refs_data {
+	alternate_ref_fn *fn;
+	void *data;
+};
+
+static int refs_from_alternate_cb(struct object_directory *e,
+				  void *data)
+{
+	struct strbuf path = STRBUF_INIT;
+	size_t base_len;
+	struct alternate_refs_data *cb = data;
+
+	if (!strbuf_realpath(&path, e->path, 0))
+		goto out;
+	if (!strbuf_strip_suffix(&path, "/objects"))
+		goto out;
+	base_len = path.len;
+
+	/* Is this a git repository with refs? */
+	strbuf_addstr(&path, "/refs");
+	if (!is_directory(path.buf))
+		goto out;
+	strbuf_setlen(&path, base_len);
+
+	read_alternate_refs(path.buf, cb->fn, cb->data);
+
+out:
+	strbuf_release(&path);
+	return 0;
+}
+
+void for_each_alternate_ref(alternate_ref_fn fn, void *data)
+{
+	struct alternate_refs_data cb;
+	cb.fn = fn;
+	cb.data = data;
+	foreach_alt_odb(refs_from_alternate_cb, &cb);
+}
+
 int foreach_alt_odb(alt_odb_fn fn, void *cb)
 {
 	struct object_directory *ent;
diff --git a/transport.c b/transport.c
index f1fcd2c4b0..2def5a0c35 100644
--- a/transport.c
+++ b/transport.c
@@ -1380,100 +1380,3 @@ char *transport_anonymize_url(const char *url)
 literal_copy:
 	return xstrdup(url);
 }
-
-static void fill_alternate_refs_command(struct child_process *cmd,
-					const char *repo_path)
-{
-	const char *value;
-
-	if (!git_config_get_value("core.alternateRefsCommand", &value)) {
-		cmd->use_shell = 1;
-
-		argv_array_push(&cmd->args, value);
-		argv_array_push(&cmd->args, repo_path);
-	} else {
-		cmd->git_cmd = 1;
-
-		argv_array_pushf(&cmd->args, "--git-dir=%s", repo_path);
-		argv_array_push(&cmd->args, "for-each-ref");
-		argv_array_push(&cmd->args, "--format=%(objectname)");
-
-		if (!git_config_get_value("core.alternateRefsPrefixes", &value)) {
-			argv_array_push(&cmd->args, "--");
-			argv_array_split(&cmd->args, value);
-		}
-	}
-
-	cmd->env = local_repo_env;
-	cmd->out = -1;
-}
-
-static void read_alternate_refs(const char *path,
-				alternate_ref_fn *cb,
-				void *data)
-{
-	struct child_process cmd = CHILD_PROCESS_INIT;
-	struct strbuf line = STRBUF_INIT;
-	FILE *fh;
-
-	fill_alternate_refs_command(&cmd, path);
-
-	if (start_command(&cmd))
-		return;
-
-	fh = xfdopen(cmd.out, "r");
-	while (strbuf_getline_lf(&line, fh) != EOF) {
-		struct object_id oid;
-		const char *p;
-
-		if (parse_oid_hex(line.buf, &oid, &p) || *p) {
-			warning(_("invalid line while parsing alternate refs: %s"),
-				line.buf);
-			break;
-		}
-
-		cb(&oid, data);
-	}
-
-	fclose(fh);
-	finish_command(&cmd);
-}
-
-struct alternate_refs_data {
-	alternate_ref_fn *fn;
-	void *data;
-};
-
-static int refs_from_alternate_cb(struct object_directory *e,
-				  void *data)
-{
-	struct strbuf path = STRBUF_INIT;
-	size_t base_len;
-	struct alternate_refs_data *cb = data;
-
-	if (!strbuf_realpath(&path, e->path, 0))
-		goto out;
-	if (!strbuf_strip_suffix(&path, "/objects"))
-		goto out;
-	base_len = path.len;
-
-	/* Is this a git repository with refs? */
-	strbuf_addstr(&path, "/refs");
-	if (!is_directory(path.buf))
-		goto out;
-	strbuf_setlen(&path, base_len);
-
-	read_alternate_refs(path.buf, cb->fn, cb->data);
-
-out:
-	strbuf_release(&path);
-	return 0;
-}
-
-void for_each_alternate_ref(alternate_ref_fn fn, void *data)
-{
-	struct alternate_refs_data cb;
-	cb.fn = fn;
-	cb.data = data;
-	foreach_alt_odb(refs_from_alternate_cb, &cb);
-}
diff --git a/transport.h b/transport.h
index 06e06d3d89..0b5f7806f6 100644
--- a/transport.h
+++ b/transport.h
@@ -262,6 +262,4 @@ int transport_refs_pushed(struct ref *ref);
 void transport_print_push_status(const char *dest, struct ref *refs,
 		  int verbose, int porcelain, unsigned int *reject_reasons);
 
-typedef void alternate_ref_fn(const struct object_id *oid, void *);
-void for_each_alternate_ref(alternate_ref_fn, void *);
 #endif
-- 
2.22.0.776.g16867c022c
Derrick Stolee· Jul 1, 2019, 13:23 UTC · re: Jeff King · lore

Re: [PATCH 1/2] object-store.h: move for_each_alternate_ref() from transport.h

On 7/1/2019 9:17 AM, Jeff King wrote:
Show 8 quoted lines
> There's nothing inherently transport-related about enumerating the
> alternate ref tips. The code has lived in transport.[ch] because the
> only use so far had been advertising available tips during transport.
> But it could be used for more, and a future patch will teach rev-list to
> access these refs.
> 
> Let's move it alongside the other alt-odb code, declaring it in
> object-store.h with the implementation in sha1-file.c.
Thanks!
> This lets us drop the inclusion of transport.h from receive-pack, which
> perhaps shows how it was misplaced (though receive-pack is about
> transporting objects, transport.h is mostly about the client side).

That's an interesting de-coupling. Thanks for looking into reorganizing the code. This series looks good to me.

-Stolee
Jeff King· Jul 1, 2019, 13:18 UTC · re: Jeff King · lore

[PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid

When we receive a remote ref update to sha1 "X", we want to check that we have all of the objects needed by "X". We can assume that our repository is not currently corrupted, and therefore if we have a ref pointing at "Y", we have all of its objects. So we can stop our traversal from "X" as soon as we hit "Y".

If we make the same non-corruption assumption about any repositories we use to store alternates, then we can also use their ref tips to shorten the traversal.

This is especially useful when cloning with "--reference", as we otherwise do not have any local refs to check against, and have to traverse the whole history, even though the other side may have sent us few or no objects. Here are results for the included perf test (which shows off more or less the maximal savings, getting one new commit and sharing the whole history):

Test HEAD^ HEAD -------------------------------------------------------------------- [on git.git] 5600.3: clone --reference 2.94(2.86+0.08) 0.09(0.08+0.01) -96.9% [on linux.git] 5600.3: clone --reference 45.74(45.34+0.41) 0.36(0.30+0.08) -99.2%

Signed-off-by: Jeff King <peff@peff.net>
---
Identical to v1 except for dropping the transport.h include in
revision.c.
 Documentation/rev-list-options.txt |  8 ++++
 connected.c                        |  1 +
 revision.c                         | 29 +++++++++++++++
 t/perf/p5600-clone-reference.sh    | 27 ++++++++++++++
 t/t5618-alternate-refs.sh          | 60 ++++++++++++++++++++++++++++++
 5 files changed, 125 insertions(+)
 create mode 100755 t/perf/p5600-clone-reference.sh
 create mode 100755 t/t5618-alternate-refs.sh
Show changes to 5 files +125 −0

Documentation/rev-list-options.txt, connected.c, revision.c, t/perf/p5600-clone-reference.sh, t/t5618-alternate-refs.sh

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 71a1fcc093..90a2c027ea 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -182,6 +182,14 @@ explicitly.
 	Pretend as if all objects mentioned by reflogs are listed on the
 	command line as `<commit>`.
 
+--alternate-refs::
+	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
+	be modified by `core.alternateRefsCommand`, etc. See
+	linkgit:git-config[1].
+
 --single-worktree::
 	By default, all working trees will be examined by the
 	following options when there are more than one (see
diff --git a/connected.c b/connected.c
index 1ab481fed6..cd9b324afa 100644
--- a/connected.c
+++ b/connected.c
@@ -80,6 +80,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 		argv_array_push(&rev_list.args, "--all");
 	}
 	argv_array_push(&rev_list.args, "--quiet");
+	argv_array_push(&rev_list.args, "--alternate-refs");
 	if (opt->progress)
 		argv_array_pushf(&rev_list.args, "--progress=%s",
 				 _("Checking connectivity"));
diff --git a/revision.c b/revision.c
index 621feb9df7..07412297f0 100644
--- a/revision.c
+++ b/revision.c
@@ -1554,6 +1554,32 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 	free_worktrees(worktrees);
 }
 
+struct add_alternate_refs_data {
+	struct rev_info *revs;
+	unsigned int flags;
+};
+
+static void add_one_alternate_ref(const struct object_id *oid,
+				  void *vdata)
+{
+	const char *name = ".alternate";
+	struct add_alternate_refs_data *data = vdata;
+	struct object *obj;
+
+	obj = get_reference(data->revs, name, oid, data->flags);
+	add_rev_cmdline(data->revs, obj, name, REV_CMD_REV, data->flags);
+	add_pending_object(data->revs, obj, name);
+}
+
+static void add_alternate_refs_to_pending(struct rev_info *revs,
+					  unsigned int flags)
+{
+	struct add_alternate_refs_data data;
+	data.revs = revs;
+	data.flags = flags;
+	for_each_alternate_ref(add_one_alternate_ref, &data);
+}
+
 static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
 			    int exclude_parent)
 {
@@ -1956,6 +1982,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	    !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk") ||
 	    !strcmp(arg, "--bisect") || starts_with(arg, "--glob=") ||
 	    !strcmp(arg, "--indexed-objects") ||
+	    !strcmp(arg, "--alternate-refs") ||
 	    starts_with(arg, "--exclude=") ||
 	    starts_with(arg, "--branches=") || starts_with(arg, "--tags=") ||
 	    starts_with(arg, "--remotes=") || starts_with(arg, "--no-walk="))
@@ -2442,6 +2469,8 @@ static int handle_revision_pseudo_opt(const char *submodule,
 		add_reflogs_to_pending(revs, *flags);
 	} else if (!strcmp(arg, "--indexed-objects")) {
 		add_index_objects_to_pending(revs, *flags);
+	} else if (!strcmp(arg, "--alternate-refs")) {
+		add_alternate_refs_to_pending(revs, *flags);
 	} else if (!strcmp(arg, "--not")) {
 		*flags ^= UNINTERESTING | BOTTOM;
 	} else if (!strcmp(arg, "--no-walk")) {
diff --git a/t/perf/p5600-clone-reference.sh b/t/perf/p5600-clone-reference.sh
new file mode 100755
index 0000000000..68fed66347
--- /dev/null
+++ b/t/perf/p5600-clone-reference.sh
@@ -0,0 +1,27 @@
+#!/bin/sh
+
+test_description='speed of clone --reference'
+. ./perf-lib.sh
+
+test_perf_default_repo
+
+test_expect_success 'create shareable repository' '
+	git clone --bare . shared.git
+'
+
+test_expect_success 'advance base repository' '
+	# Do not use test_commit here; its test_tick will
+	# use some ancient hard-coded date. The resulting clock
+	# skew will cause pack-objects to traverse in a very
+	# sub-optimal order, skewing the results.
+	echo content >new-file-that-does-not-exist &&
+	git add new-file-that-does-not-exist &&
+	git commit -m "new commit"
+'
+
+test_perf 'clone --reference' '
+	rm -rf dst.git &&
+	git clone --no-local --bare --reference shared.git . dst.git
+'
+
+test_done
diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
new file mode 100755
index 0000000000..3353216f09
--- /dev/null
+++ b/t/t5618-alternate-refs.sh
@@ -0,0 +1,60 @@
+#!/bin/sh
+
+test_description='test handling of --alternate-refs traversal'
+. ./test-lib.sh
+
+# Avoid test_commit because we want a specific and known set of refs:
+#
+#  base -- one
+#      \      \
+#       two -- merged
+#
+# where "one" and "two" are on separate refs, and "merged" is available only in
+# the dependent child repository.
+test_expect_success 'set up local refs' '
+	git checkout -b one &&
+	test_tick &&
+	git commit --allow-empty -m base &&
+	test_tick &&
+	git commit --allow-empty -m one &&
+	git checkout -b two HEAD^ &&
+	test_tick &&
+	git commit --allow-empty -m two
+'
+
+# We'll enter the child repository after it's set up since that's where
+# all of the subsequent tests will want to run (and it's easy to forget a
+# "-C child" and get nonsense results).
+test_expect_success 'set up shared clone' '
+	git clone -s . child &&
+	cd child &&
+	git merge origin/one
+'
+
+test_expect_success 'rev-list --alternate-refs' '
+	git rev-list --remotes=origin >expect &&
+	git rev-list --alternate-refs >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rev-list --not --alternate-refs' '
+	git rev-parse HEAD >expect &&
+	git rev-list HEAD --not --alternate-refs >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'limiting with alternateRefsPrefixes' '
+	test_config core.alternateRefsPrefixes refs/heads/one &&
+	git rev-list origin/one >expect &&
+	git rev-list --alternate-refs >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --source shows .alternate marker' '
+	git log --oneline --source --remotes=origin >expect.orig &&
+	sed "s/origin.* /.alternate /" <expect.orig >expect &&
+	git log --oneline --source --alternate-refs >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
2.22.0.776.g16867c022c
SZEDER Gábor· Jul 3, 2019, 09:12 UTC · re: Jeff King · lore

Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid

On Mon, Jul 01, 2019 at 09:18:15AM -0400, Jeff King wrote:
Show 6 quoted lines
> diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
> new file mode 100755
> index 0000000000..3353216f09
> --- /dev/null
> +++ b/t/t5618-alternate-refs.sh
> @@ -0,0 +1,60 @@
> +test_expect_success 'log --source shows .alternate marker' '
> +	git log --oneline --source --remotes=origin >expect.orig &&
> +	sed "s/origin.* /.alternate /" <expect.orig >expect &&
Unnecessary redirection, 'sed' can open that file on its own as well.
Show 7 quoted lines
> +	git log --oneline --source --alternate-refs >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_done
> -- 
> 2.22.0.776.g16867c022c
Jeff King· Jul 3, 2019, 16:41 UTC · re: SZEDER Gábor · lore

Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid

On Wed, Jul 03, 2019 at 11:12:25AM +0200, SZEDER Gábor wrote:
Show 13 quoted lines
> On Mon, Jul 01, 2019 at 09:18:15AM -0400, Jeff King wrote:
> > diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
> > new file mode 100755
> > index 0000000000..3353216f09
> > --- /dev/null
> > +++ b/t/t5618-alternate-refs.sh
> > @@ -0,0 +1,60 @@
> 
> > +test_expect_success 'log --source shows .alternate marker' '
> > +	git log --oneline --source --remotes=origin >expect.orig &&
> > +	sed "s/origin.* /.alternate /" <expect.orig >expect &&
> 
> Unnecessary redirection, 'sed' can open that file on its own as well.
Sure, but is there a compelling reason not to feed it as stdin?
-Peff
Junio C Hamano· Jul 3, 2019, 16:46 UTC · re: Jeff King · lore

Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid

Jeff King <peff@peff.net> writes:
Show 17 quoted lines
> On Wed, Jul 03, 2019 at 11:12:25AM +0200, SZEDER Gábor wrote:
>
>> On Mon, Jul 01, 2019 at 09:18:15AM -0400, Jeff King wrote:
>> > diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
>> > new file mode 100755
>> > index 0000000000..3353216f09
>> > --- /dev/null
>> > +++ b/t/t5618-alternate-refs.sh
>> > @@ -0,0 +1,60 @@
>> 
>> > +test_expect_success 'log --source shows .alternate marker' '
>> > +	git log --oneline --source --remotes=origin >expect.orig &&
>> > +	sed "s/origin.* /.alternate /" <expect.orig >expect &&
>> 
>> Unnecessary redirection, 'sed' can open that file on its own as well.
>
> Sure, but is there a compelling reason not to feed it as stdin?
FWIW I do not think it is bad to redirect into a command at all ;-)
SZEDER Gábor· Jul 3, 2019, 16:50 UTC · re: Jeff King · lore

Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid

On Wed, Jul 03, 2019 at 12:41:16PM -0400, Jeff King wrote:
Show 17 quoted lines
> On Wed, Jul 03, 2019 at 11:12:25AM +0200, SZEDER Gábor wrote:
> 
> > On Mon, Jul 01, 2019 at 09:18:15AM -0400, Jeff King wrote:
> > > diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
> > > new file mode 100755
> > > index 0000000000..3353216f09
> > > --- /dev/null
> > > +++ b/t/t5618-alternate-refs.sh
> > > @@ -0,0 +1,60 @@
> > 
> > > +test_expect_success 'log --source shows .alternate marker' '
> > > +	git log --oneline --source --remotes=origin >expect.orig &&
> > > +	sed "s/origin.* /.alternate /" <expect.orig >expect &&
> > 
> > Unnecessary redirection, 'sed' can open that file on its own as well.
> 
> Sure, but is there a compelling reason not to feed it as stdin?
Not really, other than there is no compelling reason to do so :)
Junio C Hamano· Jul 3, 2019, 17:05 UTC · re: SZEDER Gábor · lore

Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid

SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 20 quoted lines
> On Wed, Jul 03, 2019 at 12:41:16PM -0400, Jeff King wrote:
>> On Wed, Jul 03, 2019 at 11:12:25AM +0200, SZEDER Gábor wrote:
>> 
>> > On Mon, Jul 01, 2019 at 09:18:15AM -0400, Jeff King wrote:
>> > > diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
>> > > new file mode 100755
>> > > index 0000000000..3353216f09
>> > > --- /dev/null
>> > > +++ b/t/t5618-alternate-refs.sh
>> > > @@ -0,0 +1,60 @@
>> > 
>> > > +test_expect_success 'log --source shows .alternate marker' '
>> > > +	git log --oneline --source --remotes=origin >expect.orig &&
>> > > +	sed "s/origin.* /.alternate /" <expect.orig >expect &&
>> > 
>> > Unnecessary redirection, 'sed' can open that file on its own as well.
>> 
>> Sure, but is there a compelling reason not to feed it as stdin?
>
> Not really, other than there is no compelling reason to do so :)

For this particular one, it would not make much difference, but when feeding a single file to a command that can take many instructions as command line arguments, I tend to prefer

	$ cmd <input \
		-e 's/foo/bar/' \
		-e 's/xyzzy/frotz/g' \
		...
which I find slightly easier to read than
	$ cmd \
		-e 's/foo/bar/' \
		-e 's/xyzzy/frotz/g' \
		... \
		input
Taylor Blau· Jul 1, 2019, 17:02 UTC · re: Jeff King · lore

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

Hi Peff,
On Mon, Jul 01, 2019 at 09:17:13AM -0400, Jeff King wrote:
Show 29 quoted lines
> On Mon, Jul 01, 2019 at 08:59:45AM -0400, Jeff King wrote:
>
> > Yes, this is weakening the ties of the feature to the transport code.
> > Traditionally transport-oriented code was the only user, but it also
> > used the upload-pack transport under the hood to access the alternate
> > (that was changed a while ago to for-each-ref for speed).
> >
> > I don't think there's any functional difference in having it there, but
> > it could be moved to live alongside foreach_alt_odb() in sha1-file.c.
>
> Looks like this hasn't quite hit 'next' yet, so perhaps we can
> reorganize it as a preparatory patch.
>
>   [1/2]: object-store.h: move for_each_alternate_ref() from transport.h
>   [2/2]: check_everything_connected: assume alternate ref tips are valid
>
>  Documentation/rev-list-options.txt |  8 +++
>  builtin/receive-pack.c             |  1 -
>  connected.c                        |  1 +
>  object-store.h                     |  2 +
>  revision.c                         | 29 +++++++++
>  sha1-file.c                        | 97 ++++++++++++++++++++++++++++++
>  t/perf/p5600-clone-reference.sh    | 27 +++++++++
>  t/t5618-alternate-refs.sh          | 60 ++++++++++++++++++
>  transport.c                        | 97 ------------------------------
>  transport.h                        |  2 -
>  10 files changed, 224 insertions(+), 100 deletions(-)
>  create mode 100755 t/perf/p5600-clone-reference.sh
>  create mode 100755 t/t5618-alternate-refs.sh

This looks good to me, too (and matches my recollection from our prior off-list review against GitHub's fork).

One thing that I didn't catch in my initial review that I am seeing now is the ".alternate" marker. Why did you choose this? I was thinking that ".have" would make more sense since it's consistent with what's shown in the ref advertisement, but I think that actually ".alternate" is a _better_ choice: the two really do refer to different things.

Either way, I don't think that it matters much: this series is already on 'next' and I think that either is fine (especially since ".have" refers to a nameless ref and ".alternate" refers to a nameless pseudo-remote).

> -Peff

Thanks, Taylor

Jeff King· Jul 2, 2019, 05:29 UTC · re: Taylor Blau · lore

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

On Mon, Jul 01, 2019 at 12:02:45PM -0500, Taylor Blau wrote:
Show 5 quoted lines
> One thing that I didn't catch in my initial review that I am seeing now
> is the ".alternate" marker. Why did you choose this? I was thinking that
> ".have" would make more sense since it's consistent with what's shown in
> the ref advertisement, but I think that actually ".alternate" is a
> _better_ choice: the two really do refer to different things.

Yeah, I had called these ".have" originally, but decided that was too tied up with the current users, and not with the concept. I think keeping the leading "." is worthwhile as that's an invalid refname.

I also thought about an empty string, but it's probably more informative to show _something_. After all, the user would not see these unless they specifically asked for them _and_ used something like --source, so presumably it's a useful piece of information at that point (I don't know of any other way to show these names except for --source).

I suppose one other option would be to name them after the oid itself. So with --source you'd find out that 1234abcd came from 1234abcd (duh), but also that its children came from 1234abcd. Maybe that has value. I dunno.

It would be easy to change, but I'd also be OK punting until somebody comes up with a compelling use case.

-Peff
Junio C Hamano· Jun 28, 2019, 16:22 UTC · re: Jeff King · lore

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

Jeff King <peff@peff.net> writes:
Show 22 quoted lines
> When we receive a remote ref update to sha1 "X", we want to check that
> we have all of the objects needed by "X". We can assume that our
> repository is not currently corrupted, and therefore if we have a ref
> pointing at "Y", we have all of its objects. So we can stop our
> traversal from "X" as soon as we hit "Y".
>
> If we make the same non-corruption assumption about any repositories we
> use to store alternates, then we can also use their ref tips to shorten
> the traversal.
> ...
> diff --git a/connected.c b/connected.c
> index 1ab481fed6..cd9b324afa 100644
> --- a/connected.c
> +++ b/connected.c
> @@ -80,6 +80,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>  		argv_array_push(&rev_list.args, "--all");
>  	}
>  	argv_array_push(&rev_list.args, "--quiet");
> +	argv_array_push(&rev_list.args, "--alternate-refs");
>  	if (opt->progress)
>  		argv_array_pushf(&rev_list.args, "--progress=%s",
>  				 _("Checking connectivity"));

Quite honestly, I am very surprised that we did not do this. The idea of alternate object store, as well as reducing transfer cost by advertising their tips as '.have' phony refs, is almost as old as the pack protocol itself.

Jeff King· Jun 29, 2019, 07:55 UTC · re: Junio C Hamano · lore

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

On Fri, Jun 28, 2019 at 09:22:56AM -0700, Junio C Hamano wrote:
Show 10 quoted lines
> >  	argv_array_push(&rev_list.args, "--quiet");
> > +	argv_array_push(&rev_list.args, "--alternate-refs");
> >  	if (opt->progress)
> >  		argv_array_pushf(&rev_list.args, "--progress=%s",
> >  				 _("Checking connectivity"));
> 
> Quite honestly, I am very surprised that we did not do this.  The
> idea of alternate object store, as well as reducing transfer cost by
> advertising their tips as '.have' phony refs, is almost as old as
> the pack protocol itself.

Yeah, as you note we are already telling the other side of the push "hey, we already have these objects". So we are almost always just walking over our own local objects in the connectivity check, which is silly.

I only did "clone --reference" in the perf test because it was the simplest, but a push to a server with alternates should be similarly improved. E.g., doing this in a clone of linux.git:

  git init --bare dst.git
  echo '../../.git/objects' >dst.git/objects/info/alternates
  time git push dst.git HEAD

goes from 40+ seconds to 100ms or so. Again, obviously that's the best case, but it should also improve the normal case of somebody pulling down "torvalds/linux.git" and pushing it back up to their own "peff/linux.git", too.

I don't have real-world numbers yet from GitHub, because we're not actually advertising .haves on push right now. All of the Git pieces are now in places to do so, but we still have to make some tweaks at our replication layer. But soon. :)

-Peff
Derrick Stolee· Jul 1, 2019, 12:26 UTC · re: Jeff King · lore

Re: [PATCH] check_everything_connected: assume alternate ref tips are valid

On 6/29/2019 3:55 AM, Jeff King wrote:
Show 35 quoted lines
> On Fri, Jun 28, 2019 at 09:22:56AM -0700, Junio C Hamano wrote:
> 
>>>  	argv_array_push(&rev_list.args, "--quiet");
>>> +	argv_array_push(&rev_list.args, "--alternate-refs");
>>>  	if (opt->progress)
>>>  		argv_array_pushf(&rev_list.args, "--progress=%s",
>>>  				 _("Checking connectivity"));
>>
>> Quite honestly, I am very surprised that we did not do this.  The
>> idea of alternate object store, as well as reducing transfer cost by
>> advertising their tips as '.have' phony refs, is almost as old as
>> the pack protocol itself.
> 
> Yeah, as you note we are already telling the other side of the push
> "hey, we already have these objects". So we are almost always just
> walking over our own local objects in the connectivity check, which is
> silly.
> 
> I only did "clone --reference" in the perf test because it was the
> simplest, but a push to a server with alternates should be similarly
> improved. E.g., doing this in a clone of linux.git:
> 
>   git init --bare dst.git
>   echo '../../.git/objects' >dst.git/objects/info/alternates
>   time git push dst.git HEAD
> 
> goes from 40+ seconds to 100ms or so. Again, obviously that's the best
> case, but it should also improve the normal case of somebody pulling
> down "torvalds/linux.git" and pushing it back up to their own
> "peff/linux.git", too.
> 
> I don't have real-world numbers yet from GitHub, because we're not
> actually advertising .haves on push right now. All of the Git pieces are
> now in places to do so, but we still have to make some tweaks at our
> replication layer. But soon. :)

Exciting! Should improve the user's experience keeping their forks updated!

-Stolee

← back to recent threads