{"thread":{"id":"51400","subject":"[PATCH] check_everything_connected: assume alternate ref tips are valid","startedAt":"2019-06-28T10:11:35Z","lastAt":"2019-07-03T17:06:01Z","messageCount":20,"participants":["Jeff King","Derrick Stolee","Junio C Hamano","Taylor Blau","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"378254","messageId":"20190628101131.GA22862@sigill.intra.peff.net","threadId":"51400","inReplyTo":null,"subject":"[PATCH] check_everything_connected: assume alternate ref tips are valid","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-28T10:11:31Z","receivedAt":"2019-06-28T10:11:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we receive a remote ref update to sha1 \"X\", we want to check that\nwe have all of the objects needed by \"X\". We can assume that our\nrepository is not currently corrupted, and therefore if we have a ref\npointing at \"Y\", we have all of its objects. So we can stop our\ntraversal from \"X\" as soon as we hit \"Y\".\n\nIf we make the same non-corruption assumption about any repositories we\nuse to store alternates, then we can also use their ref tips to shorten\nthe traversal.\n\nThis is especially useful when cloning with \"--reference\", as we\notherwise do not have any local refs to check against, and have to\ntraverse the whole history, even though the other side may have sent us\nfew or no objects. Here are results for the included perf test (which\nshows off more or less the maximal savings, getting one new commit and\nsharing the whole history):\n\nTest HEAD^ HEAD\n--------------------------------------------------------------------\n[on git.git]\n5600.3: clone --reference 2.94(2.86+0.08) 0.09(0.08+0.01) -96.9%\n[on linux.git]\n5600.3: clone --reference 45.74(45.34+0.41) 0.36(0.30+0.08) -99.2%\n\nSigned-off-by: Jeff King \n---\n Documentation/rev-list-options.txt | 8 ++++\n connected.c | 1 +\n revision.c | 30 +++++++++++++++\n t/perf/p5600-clone-reference.sh | 27 ++++++++++++++\n t/t5618-alternate-refs.sh | 60 ++++++++++++++++++++++++++++++\n 5 files changed, 126 insertions(+)\n create mode 100755 t/perf/p5600-clone-reference.sh\n create mode 100755 t/t5618-alternate-refs.sh\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 71a1fcc093..90a2c027ea 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -182,6 +182,14 @@ explicitly.\n \tPretend as if all objects mentioned by reflogs are listed on the\n \tcommand line as ``.\n \n+--alternate-refs::\n+\tPretend as if all objects mentioned as ref tips of alternate\n+\trepositories were listed on the command line. An alternate\n+\trepository is any repository whose object directory is specified\n+\tin `objects/info/alternates`. The set of included objects may\n+\tbe modified by `core.alternateRefsCommand`, etc. See\n+\tlinkgit:git-config[1].\n+\n --single-worktree::\n \tBy default, all working trees will be examined by the\n \tfollowing options when there are more than one (see\ndiff --git a/connected.c b/connected.c\nindex 1ab481fed6..cd9b324afa 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -80,6 +80,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\targv_array_push(&rev_list.args, \"--all\");\n \t}\n \targv_array_push(&rev_list.args, \"--quiet\");\n+\targv_array_push(&rev_list.args, \"--alternate-refs\");\n \tif (opt->progress)\n \t\targv_array_pushf(&rev_list.args, \"--progress=%s\",\n \t\t\t\t _(\"Checking connectivity\"));\ndiff --git a/revision.c b/revision.c\nindex 621feb9df7..b9be08e9df 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -28,6 +28,7 @@\n #include \"commit-graph.h\"\n #include \"prio-queue.h\"\n #include \"hashmap.h\"\n+#include \"transport.h\"\n \n volatile show_early_output_fn_t show_early_output;\n \n@@ -1554,6 +1555,32 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)\n \tfree_worktrees(worktrees);\n }\n \n+struct add_alternate_refs_data {\n+\tstruct rev_info *revs;\n+\tunsigned int flags;\n+};\n+\n+static void add_one_alternate_ref(const struct object_id *oid,\n+\t\t\t\t void *vdata)\n+{\n+\tconst char *name = \".alternate\";\n+\tstruct add_alternate_refs_data *data = vdata;\n+\tstruct object *obj;\n+\n+\tobj = get_reference(data->revs, name, oid, data->flags);\n+\tadd_rev_cmdline(data->revs, obj, name, REV_CMD_REV, data->flags);\n+\tadd_pending_object(data->revs, obj, name);\n+}\n+\n+static void add_alternate_refs_to_pending(struct rev_info *revs,\n+\t\t\t\t\t unsigned int flags)\n+{\n+\tstruct add_alternate_refs_data data;\n+\tdata.revs = revs;\n+\tdata.flags = flags;\n+\tfor_each_alternate_ref(add_one_alternate_ref, &data);\n+}\n+\n static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n \t\t\t int exclude_parent)\n {\n@@ -1956,6 +1983,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t !strcmp(arg, \"--no-walk\") || !strcmp(arg, \"--do-walk\") ||\n \t !strcmp(arg, \"--bisect\") || starts_with(arg, \"--glob=\") ||\n \t !strcmp(arg, \"--indexed-objects\") ||\n+\t !strcmp(arg, \"--alternate-refs\") ||\n \t starts_with(arg, \"--exclude=\") ||\n \t starts_with(arg, \"--branches=\") || starts_with(arg, \"--tags=\") ||\n \t starts_with(arg, \"--remotes=\") || starts_with(arg, \"--no-walk=\"))\n@@ -2442,6 +2470,8 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t\tadd_reflogs_to_pending(revs, *flags);\n \t} else if (!strcmp(arg, \"--indexed-objects\")) {\n \t\tadd_index_objects_to_pending(revs, *flags);\n+\t} else if (!strcmp(arg, \"--alternate-refs\")) {\n+\t\tadd_alternate_refs_to_pending(revs, *flags);\n \t} else if (!strcmp(arg, \"--not\")) {\n \t\t*flags ^= UNINTERESTING | BOTTOM;\n \t} else if (!strcmp(arg, \"--no-walk\")) {\ndiff --git a/t/perf/p5600-clone-reference.sh b/t/perf/p5600-clone-reference.sh\nnew file mode 100755\nindex 0000000000..68fed66347\n--- /dev/null\n+++ b/t/perf/p5600-clone-reference.sh\n@@ -0,0 +1,27 @@\n+#!/bin/sh\n+\n+test_description='speed of clone --reference'\n+. ./perf-lib.sh\n+\n+test_perf_default_repo\n+\n+test_expect_success 'create shareable repository' '\n+\tgit clone --bare . shared.git\n+'\n+\n+test_expect_success 'advance base repository' '\n+\t# Do not use test_commit here; its test_tick will\n+\t# use some ancient hard-coded date. The resulting clock\n+\t# skew will cause pack-objects to traverse in a very\n+\t# sub-optimal order, skewing the results.\n+\techo content >new-file-that-does-not-exist &&\n+\tgit add new-file-that-does-not-exist &&\n+\tgit commit -m \"new commit\"\n+'\n+\n+test_perf 'clone --reference' '\n+\trm -rf dst.git &&\n+\tgit clone --no-local --bare --reference shared.git . dst.git\n+'\n+\n+test_done\ndiff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\nnew file mode 100755\nindex 0000000000..3353216f09\n--- /dev/null\n+++ b/t/t5618-alternate-refs.sh\n@@ -0,0 +1,60 @@\n+#!/bin/sh\n+\n+test_description='test handling of --alternate-refs traversal'\n+. ./test-lib.sh\n+\n+# Avoid test_commit because we want a specific and known set of refs:\n+#\n+# base -- one\n+# \\ \\\n+# two -- merged\n+#\n+# where \"one\" and \"two\" are on separate refs, and \"merged\" is available only in\n+# the dependent child repository.\n+test_expect_success 'set up local refs' '\n+\tgit checkout -b one &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m base &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m one &&\n+\tgit checkout -b two HEAD^ &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m two\n+'\n+\n+# We'll enter the child repository after it's set up since that's where\n+# all of the subsequent tests will want to run (and it's easy to forget a\n+# \"-C child\" and get nonsense results).\n+test_expect_success 'set up shared clone' '\n+\tgit clone -s . child &&\n+\tcd child &&\n+\tgit merge origin/one\n+'\n+\n+test_expect_success 'rev-list --alternate-refs' '\n+\tgit rev-list --remotes=origin >expect &&\n+\tgit rev-list --alternate-refs >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'rev-list --not --alternate-refs' '\n+\tgit rev-parse HEAD >expect &&\n+\tgit rev-list HEAD --not --alternate-refs >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'limiting with alternateRefsPrefixes' '\n+\ttest_config core.alternateRefsPrefixes refs/heads/one &&\n+\tgit rev-list origin/one >expect &&\n+\tgit rev-list --alternate-refs >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'log --source shows .alternate marker' '\n+\tgit log --oneline --source --remotes=origin >expect.orig &&\n+\tsed \"s/origin.* /.alternate /\" expect &&\n+\tgit log --oneline --source --alternate-refs >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.22.0.775.g4ba9815492\n"},{"id":"378255","messageId":"20190628101858.GA23052@sigill.intra.peff.net","threadId":"51400","inReplyTo":"20190628101131.GA22862@sigill.intra.peff.net","subject":"Re: [PATCH] check_everything_connected: assume alternate ref tips are valid","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-28T10:18:58Z","receivedAt":"2019-06-28T10:19:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 28, 2019 at 06:11:31AM -0400, Jeff King wrote:\n\n> Subject: [PATCH] check_everything_connected: assume alternate ref tips...\n\nHeh. This subject should just be \"check_connected\" these days. I wrote\nthis patch originally many years ago, before 7043c7071c\n(check_everything_connected: use a struct with named options,\n2016-07-15) gave the function a shorter name.\n\nBut the patch has been updated recently to match other updates to the\nalternates code. In particular, in the original \"rev-list\n--alternate-refs\" behaved as if all of the refs from the alternate were\nmentioned on the command line, and you could use --exclude, etc as if\nthey were real refs. But we've since simplified the alternates interface\nto just get a list of oids, so now it behaves more like\n\"--indexed-objects\" in that it acts as if those oids were mentioned on\nthe command line.\n\nI think \"--alternate-refs\" is still the right name, though, since we're\njust giving the oids at the ref tips. And I think the included\ndocumentation makes this clear.\n\n-Peff\n"},{"id":"378266","messageId":"601d8561-6e24-559c-6fbb-fa25a7389fa0@gmail.com","threadId":"51400","inReplyTo":"20190628101131.GA22862@sigill.intra.peff.net","subject":"Re: [PATCH] check_everything_connected: assume alternate ref tips are valid","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-06-28T12:51:04Z","receivedAt":"2019-06-28T12:51:07Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/28/2019 6:11 AM, Jeff King wrote:\n> When we receive a remote ref update to sha1 \"X\", we want to check that\n> we have all of the objects needed by \"X\". We can assume that our\n> repository is not currently corrupted, and therefore if we have a ref\n> pointing at \"Y\", we have all of its objects. So we can stop our\n> traversal from \"X\" as soon as we hit \"Y\".\n> \n> If we make the same non-corruption assumption about any repositories we\n> use to store alternates, then we can also use their ref tips to shorten\n> the traversal.\n\nI was confused by this paragraph, because I didn't know about\nfor_each_alternate_ref() and how refs_From_alternate_cb() will\nstrip the \"/objects\" and append \"/refs\" to check refs if they\nexist. All of that logic is in transport.c but used by\nfetch-pack.c and builtin/receive-pack.c. But now we are adding\nto revision.c, so the restriction to \"this helps data transfer\"\nis getting murkier.\n\nIs this something that should be extracted to the object-store\nlayer? Or is it so tricky to use that we shouldn't make it too\neasy to fall into a bad pattern?\n\n> This is especially useful when cloning with \"--reference\", as we\n> otherwise do not have any local refs to check against, and have to\n> traverse the whole history, even though the other side may have sent us\n> few or no objects. Here are results for the included perf test (which\n> shows off more or less the maximal savings, getting one new commit and\n> sharing the whole history):\n> \n> Test HEAD^ HEAD\n> --------------------------------------------------------------------\n> [on git.git]\n> 5600.3: clone --reference 2.94(2.86+0.08) 0.09(0.08+0.01) -96.9%\n> [on linux.git]\n> 5600.3: clone --reference 45.74(45.34+0.41) 0.36(0.30+0.08) -99.2%\n\nIt's really hard to argue with numbers like these. Kudos!\n\n> \n> Signed-off-by: Jeff King \n> ---\n> Documentation/rev-list-options.txt | 8 ++++\n> connected.c | 1 +\n> revision.c | 30 +++++++++++++++\n> t/perf/p5600-clone-reference.sh | 27 ++++++++++++++\n> t/t5618-alternate-refs.sh | 60 ++++++++++++++++++++++++++++++\n\nOther than the high-level questions above, the code and tests look\ngood to me.\n\nThanks,\n-Stolee \n\n"},{"id":"378276","messageId":"xmqqpnmx8ysf.fsf@gitster-ct.c.googlers.com","threadId":"51400","inReplyTo":"20190628101131.GA22862@sigill.intra.peff.net","subject":"Re: [PATCH] check_everything_connected: assume alternate ref tips are valid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-28T16:22:56Z","receivedAt":"2019-06-28T16:23:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When we receive a remote ref update to sha1 \"X\", we want to check that\n> we have all of the objects needed by \"X\". We can assume that our\n> repository is not currently corrupted, and therefore if we have a ref\n> pointing at \"Y\", we have all of its objects. So we can stop our\n> traversal from \"X\" as soon as we hit \"Y\".\n>\n> If we make the same non-corruption assumption about any repositories we\n> use to store alternates, then we can also use their ref tips to shorten\n> the traversal.\n> ...\n> diff --git a/connected.c b/connected.c\n> index 1ab481fed6..cd9b324afa 100644\n> --- a/connected.c\n> +++ b/connected.c\n> @@ -80,6 +80,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n> \t\targv_array_push(&rev_list.args, \"--all\");\n> \t}\n> \targv_array_push(&rev_list.args, \"--quiet\");\n> +\targv_array_push(&rev_list.args, \"--alternate-refs\");\n> \tif (opt->progress)\n> \t\targv_array_pushf(&rev_list.args, \"--progress=%s\",\n> \t\t\t\t _(\"Checking connectivity\"));\n\nQuite honestly, I am very surprised that we did not do this. The\nidea of alternate object store, as well as reducing transfer cost by\nadvertising their tips as '.have' phony refs, is almost as old as\nthe pack protocol itself.\n"},{"id":"378317","messageId":"20190629074348.GA5080@sigill.intra.peff.net","threadId":"51400","inReplyTo":"601d8561-6e24-559c-6fbb-fa25a7389fa0@gmail.com","subject":"Re: [PATCH] check_everything_connected: assume alternate ref tips are valid","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-29T07:43:49Z","receivedAt":"2019-06-29T07:43:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 28, 2019 at 08:51:04AM -0400, Derrick Stolee wrote:\n\n> On 6/28/2019 6:11 AM, Jeff King wrote:\n> > When we receive a remote ref update to sha1 \"X\", we want to check that\n> > we have all of the objects needed by \"X\". We can assume that our\n> > repository is not currently corrupted, and therefore if we have a ref\n> > pointing at \"Y\", we have all of its objects. So we can stop our\n> > traversal from \"X\" as soon as we hit \"Y\".\n> > \n> > If we make the same non-corruption assumption about any repositories we\n> > use to store alternates, then we can also use their ref tips to shorten\n> > the traversal.\n> \n> I was confused by this paragraph, because I didn't know about\n> for_each_alternate_ref() and how refs_From_alternate_cb() will\n> strip the \"/objects\" and append \"/refs\" to check refs if they\n> exist. All of that logic is in transport.c but used by\n> fetch-pack.c and builtin/receive-pack.c. But now we are adding\n> to revision.c, so the restriction to \"this helps data transfer\"\n> is getting murkier.\n\nUsing it for data transfer is still the main thing for our internal\ncalls, but I think it's worth exposing it for general use via rev-list.\nI imagine it would mostly be for poking around and debugging, but it\nshould allow things like:\n\n # what do we have that our alternate does not\n git rev-list --all --not --alternate-refs\n\n> Is this something that should be extracted to the object-store\n> layer? Or is it so tricky to use that we shouldn't make it too\n> easy to fall into a bad pattern?\n\nI'm not sure what you have in mind, exactly. If you are asking whether\nthere are more places that alternate refs could be used, I can't think\nof any. If you are asking whether this is in the wrong place, no, I\nthink it's the right place. :)\n\n-Peff\n"},{"id":"378318","messageId":"20190629075536.GB5080@sigill.intra.peff.net","threadId":"51400","inReplyTo":"xmqqpnmx8ysf.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] check_everything_connected: assume alternate ref tips are valid","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-29T07:55:36Z","receivedAt":"2019-06-29T07:55:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 28, 2019 at 09:22:56AM -0700, Junio C Hamano wrote:\n\n> > \targv_array_push(&rev_list.args, \"--quiet\");\n> > +\targv_array_push(&rev_list.args, \"--alternate-refs\");\n> > \tif (opt->progress)\n> > \t\targv_array_pushf(&rev_list.args, \"--progress=%s\",\n> > \t\t\t\t _(\"Checking connectivity\"));\n> \n> Quite honestly, I am very surprised that we did not do this. The\n> idea of alternate object store, as well as reducing transfer cost by\n> advertising their tips as '.have' phony refs, is almost as old as\n> the pack protocol itself.\n\nYeah, as you note we are already telling the other side of the push\n\"hey, we already have these objects\". So we are almost always just\nwalking over our own local objects in the connectivity check, which is\nsilly.\n\nI only did \"clone --reference\" in the perf test because it was the\nsimplest, but a push to a server with alternates should be similarly\nimproved. E.g., doing this in a clone of linux.git:\n\n git init --bare dst.git\n echo '../../.git/objects' >dst.git/objects/info/alternates\n time git push dst.git HEAD\n\ngoes from 40+ seconds to 100ms or so. Again, obviously that's the best\ncase, but it should also improve the normal case of somebody pulling\ndown \"torvalds/linux.git\" and pushing it back up to their own\n\"peff/linux.git\", too.\n\nI don't have real-world numbers yet from GitHub, because we're not\nactually advertising .haves on push right now. All of the Git pieces are\nnow in places to do so, but we still have to make some tweaks at our\nreplication layer. But soon. :)\n\n-Peff\n"},{"id":"378370","messageId":"418213f2-82d6-f7bd-7379-7f20f0e83084@gmail.com","threadId":"51400","inReplyTo":"20190629074348.GA5080@sigill.intra.peff.net","subject":"Re: [PATCH] check_everything_connected: assume alternate ref tips are valid","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-07-01T12:25:09Z","receivedAt":"2019-07-01T12:25:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/29/2019 3:43 AM, Jeff King wrote:\n> On Fri, Jun 28, 2019 at 08:51:04AM -0400, Derrick Stolee wrote:\n> \n>> On 6/28/2019 6:11 AM, Jeff King wrote:\n>>> When we receive a remote ref update to sha1 \"X\", we want to check that\n>>> we have all of the objects needed by \"X\". We can assume that our\n>>> repository is not currently corrupted, and therefore if we have a ref\n>>> pointing at \"Y\", we have all of its objects. So we can stop our\n>>> traversal from \"X\" as soon as we hit \"Y\".\n>>>\n>>> If we make the same non-corruption assumption about any repositories we\n>>> use to store alternates, then we can also use their ref tips to shorten\n>>> the traversal.\n>>\n>> I was confused by this paragraph, because I didn't know about\n>> for_each_alternate_ref() and how refs_From_alternate_cb() will\n>> strip the \"/objects\" and append \"/refs\" to check refs if they\n>> exist. All of that logic is in transport.c but used by\n>> fetch-pack.c and builtin/receive-pack.c. But now we are adding\n>> to revision.c, so the restriction to \"this helps data transfer\"\n>> is getting murkier.\n> \n> Using it for data transfer is still the main thing for our internal\n> calls, but I think it's worth exposing it for general use via rev-list.\n> I imagine it would mostly be for poking around and debugging, but it\n> should allow things like:\n> \n> # what do we have that our alternate does not\n> git rev-list --all --not --alternate-refs\n\nSo this is an example where the alternate refs are being used without\nany network activity.\n\n>> Is this something that should be extracted to the object-store\n>> layer? Or is it so tricky to use that we shouldn't make it too\n>> easy to fall into a bad pattern?\n> \n> I'm not sure what you have in mind, exactly. If you are asking whether\n> there are more places that alternate refs could be used, I can't think\n> of any. If you are asking whether this is in the wrong place, no, I\n> think it's the right place. :)\n\nJust double-checking that it is appropriate for revision.c to take\ndependence on transport.h instead of moving the alternate ref stuff\ninto a different header file. I trust your opinion.\n\n-Stolee\n"},{"id":"378371","messageId":"5c38cb79-bc04-db10-12b9-7e4f83f836c4@gmail.com","threadId":"51400","inReplyTo":"20190629075536.GB5080@sigill.intra.peff.net","subject":"Re: [PATCH] check_everything_connected: assume alternate ref tips are valid","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-07-01T12:26:23Z","receivedAt":"2019-07-01T12:26:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/29/2019 3:55 AM, Jeff King wrote:\n> On Fri, Jun 28, 2019 at 09:22:56AM -0700, Junio C Hamano wrote:\n> \n>>> \targv_array_push(&rev_list.args, \"--quiet\");\n>>> +\targv_array_push(&rev_list.args, \"--alternate-refs\");\n>>> \tif (opt->progress)\n>>> \t\targv_array_pushf(&rev_list.args, \"--progress=%s\",\n>>> \t\t\t\t _(\"Checking connectivity\"));\n>>\n>> Quite honestly, I am very surprised that we did not do this. The\n>> idea of alternate object store, as well as reducing transfer cost by\n>> advertising their tips as '.have' phony refs, is almost as old as\n>> the pack protocol itself.\n> \n> Yeah, as you note we are already telling the other side of the push\n> \"hey, we already have these objects\". So we are almost always just\n> walking over our own local objects in the connectivity check, which is\n> silly.\n> \n> I only did \"clone --reference\" in the perf test because it was the\n> simplest, but a push to a server with alternates should be similarly\n> improved. E.g., doing this in a clone of linux.git:\n> \n> git init --bare dst.git\n> echo '../../.git/objects' >dst.git/objects/info/alternates\n> time git push dst.git HEAD\n> \n> goes from 40+ seconds to 100ms or so. Again, obviously that's the best\n> case, but it should also improve the normal case of somebody pulling\n> down \"torvalds/linux.git\" and pushing it back up to their own\n> \"peff/linux.git\", too.\n> \n> I don't have real-world numbers yet from GitHub, because we're not\n> actually advertising .haves on push right now. All of the Git pieces are\n> now in places to do so, but we still have to make some tweaks at our\n> replication layer. But soon. :)\n\nExciting! Should improve the user's experience keeping their forks\nupdated!\n\n-Stolee\n"},{"id":"378376","messageId":"20190701125945.GB4704@sigill.intra.peff.net","threadId":"51400","inReplyTo":"418213f2-82d6-f7bd-7379-7f20f0e83084@gmail.com","subject":"Re: [PATCH] check_everything_connected: assume alternate ref tips are valid","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-01T12:59:46Z","receivedAt":"2019-07-01T12:59:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 01, 2019 at 08:25:09AM -0400, Derrick Stolee wrote:\n\n> > I'm not sure what you have in mind, exactly. If you are asking whether\n> > there are more places that alternate refs could be used, I can't think\n> > of any. If you are asking whether this is in the wrong place, no, I\n> > think it's the right place. :)\n> \n> Just double-checking that it is appropriate for revision.c to take\n> dependence on transport.h instead of moving the alternate ref stuff\n> into a different header file. I trust your opinion.\n\nAh, I see. I misunderstood you before.\n\nYes, this is weakening the ties of the feature to the transport code.\nTraditionally transport-oriented code was the only user, but it also\nused the upload-pack transport under the hood to access the alternate\n(that was changed a while ago to for-each-ref for speed).\n\nI don't think there's any functional difference in having it there, but\nit could be moved to live alongside foreach_alt_odb() in sha1-file.c.\n\n-Peff\n"},{"id":"378382","messageId":"20190701131713.GA25349@sigill.intra.peff.net","threadId":"51400","inReplyTo":"20190701125945.GB4704@sigill.intra.peff.net","subject":"Re: [PATCH] check_everything_connected: assume alternate ref tips are valid","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-01T13:17:13Z","receivedAt":"2019-07-01T13:17:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 01, 2019 at 08:59:45AM -0400, Jeff King wrote:\n\n> Yes, this is weakening the ties of the feature to the transport code.\n> Traditionally transport-oriented code was the only user, but it also\n> used the upload-pack transport under the hood to access the alternate\n> (that was changed a while ago to for-each-ref for speed).\n> \n> I don't think there's any functional difference in having it there, but\n> it could be moved to live alongside foreach_alt_odb() in sha1-file.c.\n\nLooks like this hasn't quite hit 'next' yet, so perhaps we can\nreorganize it as a preparatory patch.\n\n [1/2]: object-store.h: move for_each_alternate_ref() from transport.h\n [2/2]: check_everything_connected: assume alternate ref tips are valid\n\n Documentation/rev-list-options.txt | 8 +++\n builtin/receive-pack.c | 1 -\n connected.c | 1 +\n object-store.h | 2 +\n revision.c | 29 +++++++++\n sha1-file.c | 97 ++++++++++++++++++++++++++++++\n t/perf/p5600-clone-reference.sh | 27 +++++++++\n t/t5618-alternate-refs.sh | 60 ++++++++++++++++++\n transport.c | 97 ------------------------------\n transport.h | 2 -\n 10 files changed, 224 insertions(+), 100 deletions(-)\n create mode 100755 t/perf/p5600-clone-reference.sh\n create mode 100755 t/t5618-alternate-refs.sh\n\n-Peff\n"},{"id":"378383","messageId":"20190701131740.GA2584@sigill.intra.peff.net","threadId":"51400","inReplyTo":"20190701131713.GA25349@sigill.intra.peff.net","subject":"[PATCH 1/2] object-store.h: move for_each_alternate_ref() from transport.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-01T13:17:40Z","receivedAt":"2019-07-01T13:17:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's nothing inherently transport-related about enumerating the\nalternate ref tips. The code has lived in transport.[ch] because the\nonly use so far had been advertising available tips during transport.\nBut it could be used for more, and a future patch will teach rev-list to\naccess these refs.\n\nLet's move it alongside the other alt-odb code, declaring it in\nobject-store.h with the implementation in sha1-file.c.\n\nThis lets us drop the inclusion of transport.h from receive-pack, which\nperhaps shows how it was misplaced (though receive-pack is about\ntransporting objects, transport.h is mostly about the client side).\n\nSigned-off-by: Jeff King \n---\n builtin/receive-pack.c | 1 -\n object-store.h | 2 +\n sha1-file.c | 97 ++++++++++++++++++++++++++++++++++++++++++\n transport.c | 97 ------------------------------------------\n transport.h | 2 -\n 5 files changed, 99 insertions(+), 100 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 77b7122456..557e686d5d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -12,7 +12,6 @@\n #include \"object.h\"\n #include \"remote.h\"\n #include \"connect.h\"\n-#include \"transport.h\"\n #include \"string-list.h\"\n #include \"sha1-array.h\"\n #include \"connected.h\"\ndiff --git a/object-store.h b/object-store.h\nindex 49f56ab8d9..7f7b3cdd80 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -33,6 +33,8 @@ void prepare_alt_odb(struct repository *r);\n char *compute_alternate_path(const char *path, struct strbuf *err);\n typedef int alt_odb_fn(struct object_directory *, void *);\n int foreach_alt_odb(alt_odb_fn, void*);\n+typedef void alternate_ref_fn(const struct object_id *oid, void *);\n+void for_each_alternate_ref(alternate_ref_fn, void *);\n \n /*\n * Add the directory to the on-disk alternates file; the new entry will also\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 888b6024d5..3c273c076a 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -743,6 +743,103 @@ char *compute_alternate_path(const char *path, struct strbuf *err)\n \treturn ref_git;\n }\n \n+static void fill_alternate_refs_command(struct child_process *cmd,\n+\t\t\t\t\tconst char *repo_path)\n+{\n+\tconst char *value;\n+\n+\tif (!git_config_get_value(\"core.alternateRefsCommand\", &value)) {\n+\t\tcmd->use_shell = 1;\n+\n+\t\targv_array_push(&cmd->args, value);\n+\t\targv_array_push(&cmd->args, repo_path);\n+\t} else {\n+\t\tcmd->git_cmd = 1;\n+\n+\t\targv_array_pushf(&cmd->args, \"--git-dir=%s\", repo_path);\n+\t\targv_array_push(&cmd->args, \"for-each-ref\");\n+\t\targv_array_push(&cmd->args, \"--format=%(objectname)\");\n+\n+\t\tif (!git_config_get_value(\"core.alternateRefsPrefixes\", &value)) {\n+\t\t\targv_array_push(&cmd->args, \"--\");\n+\t\t\targv_array_split(&cmd->args, value);\n+\t\t}\n+\t}\n+\n+\tcmd->env = local_repo_env;\n+\tcmd->out = -1;\n+}\n+\n+static void read_alternate_refs(const char *path,\n+\t\t\t\talternate_ref_fn *cb,\n+\t\t\t\tvoid *data)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tstruct strbuf line = STRBUF_INIT;\n+\tFILE *fh;\n+\n+\tfill_alternate_refs_command(&cmd, path);\n+\n+\tif (start_command(&cmd))\n+\t\treturn;\n+\n+\tfh = xfdopen(cmd.out, \"r\");\n+\twhile (strbuf_getline_lf(&line, fh) != EOF) {\n+\t\tstruct object_id oid;\n+\t\tconst char *p;\n+\n+\t\tif (parse_oid_hex(line.buf, &oid, &p) || *p) {\n+\t\t\twarning(_(\"invalid line while parsing alternate refs: %s\"),\n+\t\t\t\tline.buf);\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tcb(&oid, data);\n+\t}\n+\n+\tfclose(fh);\n+\tfinish_command(&cmd);\n+}\n+\n+struct alternate_refs_data {\n+\talternate_ref_fn *fn;\n+\tvoid *data;\n+};\n+\n+static int refs_from_alternate_cb(struct object_directory *e,\n+\t\t\t\t void *data)\n+{\n+\tstruct strbuf path = STRBUF_INIT;\n+\tsize_t base_len;\n+\tstruct alternate_refs_data *cb = data;\n+\n+\tif (!strbuf_realpath(&path, e->path, 0))\n+\t\tgoto out;\n+\tif (!strbuf_strip_suffix(&path, \"/objects\"))\n+\t\tgoto out;\n+\tbase_len = path.len;\n+\n+\t/* Is this a git repository with refs? */\n+\tstrbuf_addstr(&path, \"/refs\");\n+\tif (!is_directory(path.buf))\n+\t\tgoto out;\n+\tstrbuf_setlen(&path, base_len);\n+\n+\tread_alternate_refs(path.buf, cb->fn, cb->data);\n+\n+out:\n+\tstrbuf_release(&path);\n+\treturn 0;\n+}\n+\n+void for_each_alternate_ref(alternate_ref_fn fn, void *data)\n+{\n+\tstruct alternate_refs_data cb;\n+\tcb.fn = fn;\n+\tcb.data = data;\n+\tforeach_alt_odb(refs_from_alternate_cb, &cb);\n+}\n+\n int foreach_alt_odb(alt_odb_fn fn, void *cb)\n {\n \tstruct object_directory *ent;\ndiff --git a/transport.c b/transport.c\nindex f1fcd2c4b0..2def5a0c35 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1380,100 +1380,3 @@ char *transport_anonymize_url(const char *url)\n literal_copy:\n \treturn xstrdup(url);\n }\n-\n-static void fill_alternate_refs_command(struct child_process *cmd,\n-\t\t\t\t\tconst char *repo_path)\n-{\n-\tconst char *value;\n-\n-\tif (!git_config_get_value(\"core.alternateRefsCommand\", &value)) {\n-\t\tcmd->use_shell = 1;\n-\n-\t\targv_array_push(&cmd->args, value);\n-\t\targv_array_push(&cmd->args, repo_path);\n-\t} else {\n-\t\tcmd->git_cmd = 1;\n-\n-\t\targv_array_pushf(&cmd->args, \"--git-dir=%s\", repo_path);\n-\t\targv_array_push(&cmd->args, \"for-each-ref\");\n-\t\targv_array_push(&cmd->args, \"--format=%(objectname)\");\n-\n-\t\tif (!git_config_get_value(\"core.alternateRefsPrefixes\", &value)) {\n-\t\t\targv_array_push(&cmd->args, \"--\");\n-\t\t\targv_array_split(&cmd->args, value);\n-\t\t}\n-\t}\n-\n-\tcmd->env = local_repo_env;\n-\tcmd->out = -1;\n-}\n-\n-static void read_alternate_refs(const char *path,\n-\t\t\t\talternate_ref_fn *cb,\n-\t\t\t\tvoid *data)\n-{\n-\tstruct child_process cmd = CHILD_PROCESS_INIT;\n-\tstruct strbuf line = STRBUF_INIT;\n-\tFILE *fh;\n-\n-\tfill_alternate_refs_command(&cmd, path);\n-\n-\tif (start_command(&cmd))\n-\t\treturn;\n-\n-\tfh = xfdopen(cmd.out, \"r\");\n-\twhile (strbuf_getline_lf(&line, fh) != EOF) {\n-\t\tstruct object_id oid;\n-\t\tconst char *p;\n-\n-\t\tif (parse_oid_hex(line.buf, &oid, &p) || *p) {\n-\t\t\twarning(_(\"invalid line while parsing alternate refs: %s\"),\n-\t\t\t\tline.buf);\n-\t\t\tbreak;\n-\t\t}\n-\n-\t\tcb(&oid, data);\n-\t}\n-\n-\tfclose(fh);\n-\tfinish_command(&cmd);\n-}\n-\n-struct alternate_refs_data {\n-\talternate_ref_fn *fn;\n-\tvoid *data;\n-};\n-\n-static int refs_from_alternate_cb(struct object_directory *e,\n-\t\t\t\t void *data)\n-{\n-\tstruct strbuf path = STRBUF_INIT;\n-\tsize_t base_len;\n-\tstruct alternate_refs_data *cb = data;\n-\n-\tif (!strbuf_realpath(&path, e->path, 0))\n-\t\tgoto out;\n-\tif (!strbuf_strip_suffix(&path, \"/objects\"))\n-\t\tgoto out;\n-\tbase_len = path.len;\n-\n-\t/* Is this a git repository with refs? */\n-\tstrbuf_addstr(&path, \"/refs\");\n-\tif (!is_directory(path.buf))\n-\t\tgoto out;\n-\tstrbuf_setlen(&path, base_len);\n-\n-\tread_alternate_refs(path.buf, cb->fn, cb->data);\n-\n-out:\n-\tstrbuf_release(&path);\n-\treturn 0;\n-}\n-\n-void for_each_alternate_ref(alternate_ref_fn fn, void *data)\n-{\n-\tstruct alternate_refs_data cb;\n-\tcb.fn = fn;\n-\tcb.data = data;\n-\tforeach_alt_odb(refs_from_alternate_cb, &cb);\n-}\ndiff --git a/transport.h b/transport.h\nindex 06e06d3d89..0b5f7806f6 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -262,6 +262,4 @@ int transport_refs_pushed(struct ref *ref);\n void transport_print_push_status(const char *dest, struct ref *refs,\n \t\t int verbose, int porcelain, unsigned int *reject_reasons);\n \n-typedef void alternate_ref_fn(const struct object_id *oid, void *);\n-void for_each_alternate_ref(alternate_ref_fn, void *);\n #endif\n-- \n2.22.0.776.g16867c022c\n\n"},{"id":"378384","messageId":"20190701131815.GB2584@sigill.intra.peff.net","threadId":"51400","inReplyTo":"20190701131713.GA25349@sigill.intra.peff.net","subject":"[PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-01T13:18:15Z","receivedAt":"2019-07-01T13:18:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we receive a remote ref update to sha1 \"X\", we want to check that\nwe have all of the objects needed by \"X\". We can assume that our\nrepository is not currently corrupted, and therefore if we have a ref\npointing at \"Y\", we have all of its objects. So we can stop our\ntraversal from \"X\" as soon as we hit \"Y\".\n\nIf we make the same non-corruption assumption about any repositories we\nuse to store alternates, then we can also use their ref tips to shorten\nthe traversal.\n\nThis is especially useful when cloning with \"--reference\", as we\notherwise do not have any local refs to check against, and have to\ntraverse the whole history, even though the other side may have sent us\nfew or no objects. Here are results for the included perf test (which\nshows off more or less the maximal savings, getting one new commit and\nsharing the whole history):\n\nTest HEAD^ HEAD\n--------------------------------------------------------------------\n[on git.git]\n5600.3: clone --reference 2.94(2.86+0.08) 0.09(0.08+0.01) -96.9%\n[on linux.git]\n5600.3: clone --reference 45.74(45.34+0.41) 0.36(0.30+0.08) -99.2%\n\nSigned-off-by: Jeff King \n---\nIdentical to v1 except for dropping the transport.h include in\nrevision.c.\n\n Documentation/rev-list-options.txt | 8 ++++\n connected.c | 1 +\n revision.c | 29 +++++++++++++++\n t/perf/p5600-clone-reference.sh | 27 ++++++++++++++\n t/t5618-alternate-refs.sh | 60 ++++++++++++++++++++++++++++++\n 5 files changed, 125 insertions(+)\n create mode 100755 t/perf/p5600-clone-reference.sh\n create mode 100755 t/t5618-alternate-refs.sh\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 71a1fcc093..90a2c027ea 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -182,6 +182,14 @@ explicitly.\n \tPretend as if all objects mentioned by reflogs are listed on the\n \tcommand line as ``.\n \n+--alternate-refs::\n+\tPretend as if all objects mentioned as ref tips of alternate\n+\trepositories were listed on the command line. An alternate\n+\trepository is any repository whose object directory is specified\n+\tin `objects/info/alternates`. The set of included objects may\n+\tbe modified by `core.alternateRefsCommand`, etc. See\n+\tlinkgit:git-config[1].\n+\n --single-worktree::\n \tBy default, all working trees will be examined by the\n \tfollowing options when there are more than one (see\ndiff --git a/connected.c b/connected.c\nindex 1ab481fed6..cd9b324afa 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -80,6 +80,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\targv_array_push(&rev_list.args, \"--all\");\n \t}\n \targv_array_push(&rev_list.args, \"--quiet\");\n+\targv_array_push(&rev_list.args, \"--alternate-refs\");\n \tif (opt->progress)\n \t\targv_array_pushf(&rev_list.args, \"--progress=%s\",\n \t\t\t\t _(\"Checking connectivity\"));\ndiff --git a/revision.c b/revision.c\nindex 621feb9df7..07412297f0 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1554,6 +1554,32 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)\n \tfree_worktrees(worktrees);\n }\n \n+struct add_alternate_refs_data {\n+\tstruct rev_info *revs;\n+\tunsigned int flags;\n+};\n+\n+static void add_one_alternate_ref(const struct object_id *oid,\n+\t\t\t\t void *vdata)\n+{\n+\tconst char *name = \".alternate\";\n+\tstruct add_alternate_refs_data *data = vdata;\n+\tstruct object *obj;\n+\n+\tobj = get_reference(data->revs, name, oid, data->flags);\n+\tadd_rev_cmdline(data->revs, obj, name, REV_CMD_REV, data->flags);\n+\tadd_pending_object(data->revs, obj, name);\n+}\n+\n+static void add_alternate_refs_to_pending(struct rev_info *revs,\n+\t\t\t\t\t unsigned int flags)\n+{\n+\tstruct add_alternate_refs_data data;\n+\tdata.revs = revs;\n+\tdata.flags = flags;\n+\tfor_each_alternate_ref(add_one_alternate_ref, &data);\n+}\n+\n static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,\n \t\t\t int exclude_parent)\n {\n@@ -1956,6 +1982,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t !strcmp(arg, \"--no-walk\") || !strcmp(arg, \"--do-walk\") ||\n \t !strcmp(arg, \"--bisect\") || starts_with(arg, \"--glob=\") ||\n \t !strcmp(arg, \"--indexed-objects\") ||\n+\t !strcmp(arg, \"--alternate-refs\") ||\n \t starts_with(arg, \"--exclude=\") ||\n \t starts_with(arg, \"--branches=\") || starts_with(arg, \"--tags=\") ||\n \t starts_with(arg, \"--remotes=\") || starts_with(arg, \"--no-walk=\"))\n@@ -2442,6 +2469,8 @@ static int handle_revision_pseudo_opt(const char *submodule,\n \t\tadd_reflogs_to_pending(revs, *flags);\n \t} else if (!strcmp(arg, \"--indexed-objects\")) {\n \t\tadd_index_objects_to_pending(revs, *flags);\n+\t} else if (!strcmp(arg, \"--alternate-refs\")) {\n+\t\tadd_alternate_refs_to_pending(revs, *flags);\n \t} else if (!strcmp(arg, \"--not\")) {\n \t\t*flags ^= UNINTERESTING | BOTTOM;\n \t} else if (!strcmp(arg, \"--no-walk\")) {\ndiff --git a/t/perf/p5600-clone-reference.sh b/t/perf/p5600-clone-reference.sh\nnew file mode 100755\nindex 0000000000..68fed66347\n--- /dev/null\n+++ b/t/perf/p5600-clone-reference.sh\n@@ -0,0 +1,27 @@\n+#!/bin/sh\n+\n+test_description='speed of clone --reference'\n+. ./perf-lib.sh\n+\n+test_perf_default_repo\n+\n+test_expect_success 'create shareable repository' '\n+\tgit clone --bare . shared.git\n+'\n+\n+test_expect_success 'advance base repository' '\n+\t# Do not use test_commit here; its test_tick will\n+\t# use some ancient hard-coded date. The resulting clock\n+\t# skew will cause pack-objects to traverse in a very\n+\t# sub-optimal order, skewing the results.\n+\techo content >new-file-that-does-not-exist &&\n+\tgit add new-file-that-does-not-exist &&\n+\tgit commit -m \"new commit\"\n+'\n+\n+test_perf 'clone --reference' '\n+\trm -rf dst.git &&\n+\tgit clone --no-local --bare --reference shared.git . dst.git\n+'\n+\n+test_done\ndiff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\nnew file mode 100755\nindex 0000000000..3353216f09\n--- /dev/null\n+++ b/t/t5618-alternate-refs.sh\n@@ -0,0 +1,60 @@\n+#!/bin/sh\n+\n+test_description='test handling of --alternate-refs traversal'\n+. ./test-lib.sh\n+\n+# Avoid test_commit because we want a specific and known set of refs:\n+#\n+# base -- one\n+# \\ \\\n+# two -- merged\n+#\n+# where \"one\" and \"two\" are on separate refs, and \"merged\" is available only in\n+# the dependent child repository.\n+test_expect_success 'set up local refs' '\n+\tgit checkout -b one &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m base &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m one &&\n+\tgit checkout -b two HEAD^ &&\n+\ttest_tick &&\n+\tgit commit --allow-empty -m two\n+'\n+\n+# We'll enter the child repository after it's set up since that's where\n+# all of the subsequent tests will want to run (and it's easy to forget a\n+# \"-C child\" and get nonsense results).\n+test_expect_success 'set up shared clone' '\n+\tgit clone -s . child &&\n+\tcd child &&\n+\tgit merge origin/one\n+'\n+\n+test_expect_success 'rev-list --alternate-refs' '\n+\tgit rev-list --remotes=origin >expect &&\n+\tgit rev-list --alternate-refs >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'rev-list --not --alternate-refs' '\n+\tgit rev-parse HEAD >expect &&\n+\tgit rev-list HEAD --not --alternate-refs >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'limiting with alternateRefsPrefixes' '\n+\ttest_config core.alternateRefsPrefixes refs/heads/one &&\n+\tgit rev-list origin/one >expect &&\n+\tgit rev-list --alternate-refs >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'log --source shows .alternate marker' '\n+\tgit log --oneline --source --remotes=origin >expect.orig &&\n+\tsed \"s/origin.* /.alternate /\" expect &&\n+\tgit log --oneline --source --alternate-refs >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.22.0.776.g16867c022c\n"},{"id":"378385","messageId":"877673cb-5b2b-6e65-9a03-a16de043990d@gmail.com","threadId":"51400","inReplyTo":"20190701131740.GA2584@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] object-store.h: move for_each_alternate_ref() from transport.h","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-07-01T13:23:41Z","receivedAt":"2019-07-01T13:23:47Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/1/2019 9:17 AM, Jeff King wrote:\n> There's nothing inherently transport-related about enumerating the\n> alternate ref tips. The code has lived in transport.[ch] because the\n> only use so far had been advertising available tips during transport.\n> But it could be used for more, and a future patch will teach rev-list to\n> access these refs.\n> \n> Let's move it alongside the other alt-odb code, declaring it in\n> object-store.h with the implementation in sha1-file.c.\n\nThanks!\n\n> This lets us drop the inclusion of transport.h from receive-pack, which\n> perhaps shows how it was misplaced (though receive-pack is about\n> transporting objects, transport.h is mostly about the client side).\n\nThat's an interesting de-coupling. Thanks for looking into reorganizing\nthe code. This series looks good to me.\n\n-Stolee\n"},{"id":"378414","messageId":"20190701170245.GA54693@TaylorsMBP5815.attlocal.net","threadId":"51400","inReplyTo":"20190701131713.GA25349@sigill.intra.peff.net","subject":"Re: [PATCH] check_everything_connected: assume alternate ref tips are valid","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-07-01T17:02:45Z","receivedAt":"2019-07-01T17:02:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Peff,\n\nOn Mon, Jul 01, 2019 at 09:17:13AM -0400, Jeff King wrote:\n> On Mon, Jul 01, 2019 at 08:59:45AM -0400, Jeff King wrote:\n>\n> > Yes, this is weakening the ties of the feature to the transport code.\n> > Traditionally transport-oriented code was the only user, but it also\n> > used the upload-pack transport under the hood to access the alternate\n> > (that was changed a while ago to for-each-ref for speed).\n> >\n> > I don't think there's any functional difference in having it there, but\n> > it could be moved to live alongside foreach_alt_odb() in sha1-file.c.\n>\n> Looks like this hasn't quite hit 'next' yet, so perhaps we can\n> reorganize it as a preparatory patch.\n>\n> [1/2]: object-store.h: move for_each_alternate_ref() from transport.h\n> [2/2]: check_everything_connected: assume alternate ref tips are valid\n>\n> Documentation/rev-list-options.txt | 8 +++\n> builtin/receive-pack.c | 1 -\n> connected.c | 1 +\n> object-store.h | 2 +\n> revision.c | 29 +++++++++\n> sha1-file.c | 97 ++++++++++++++++++++++++++++++\n> t/perf/p5600-clone-reference.sh | 27 +++++++++\n> t/t5618-alternate-refs.sh | 60 ++++++++++++++++++\n> transport.c | 97 ------------------------------\n> transport.h | 2 -\n> 10 files changed, 224 insertions(+), 100 deletions(-)\n> create mode 100755 t/perf/p5600-clone-reference.sh\n> create mode 100755 t/t5618-alternate-refs.sh\n\nThis looks good to me, too (and matches my recollection from our prior\noff-list review against GitHub's fork).\n\nOne thing that I didn't catch in my initial review that I am seeing now\nis the \".alternate\" marker. Why did you choose this? I was thinking that\n\".have\" would make more sense since it's consistent with what's shown in\nthe ref advertisement, but I think that actually \".alternate\" is a\n_better_ choice: the two really do refer to different things.\n\nEither way, I don't think that it matters much: this series is already\non 'next' and I think that either is fine (especially since \".have\"\nrefers to a nameless ref and \".alternate\" refers to a nameless\npseudo-remote).\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"378473","messageId":"20190702052938.GC16344@sigill.intra.peff.net","threadId":"51400","inReplyTo":"20190701170245.GA54693@TaylorsMBP5815.attlocal.net","subject":"Re: [PATCH] check_everything_connected: assume alternate ref tips are valid","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-02T05:29:39Z","receivedAt":"2019-07-02T05:29:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 01, 2019 at 12:02:45PM -0500, Taylor Blau wrote:\n\n> One thing that I didn't catch in my initial review that I am seeing now\n> is the \".alternate\" marker. Why did you choose this? I was thinking that\n> \".have\" would make more sense since it's consistent with what's shown in\n> the ref advertisement, but I think that actually \".alternate\" is a\n> _better_ choice: the two really do refer to different things.\n\nYeah, I had called these \".have\" originally, but decided that was too\ntied up with the current users, and not with the concept. I think\nkeeping the leading \".\" is worthwhile as that's an invalid refname.\n\nI also thought about an empty string, but it's probably more informative\nto show _something_. After all, the user would not see these unless they\nspecifically asked for them _and_ used something like --source, so\npresumably it's a useful piece of information at that point (I don't\nknow of any other way to show these names except for --source).\n\nI suppose one other option would be to name them after the oid itself.\nSo with --source you'd find out that 1234abcd came from 1234abcd (duh),\nbut also that its children came from 1234abcd. Maybe that has value. I\ndunno.\n\nIt would be easy to change, but I'd also be OK punting until somebody\ncomes up with a compelling use case.\n\n-Peff\n"},{"id":"378532","messageId":"20190703091225.GQ21574@szeder.dev","threadId":"51400","inReplyTo":"20190701131815.GB2584@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-07-03T09:12:25Z","receivedAt":"2019-07-03T09:12:30Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Jul 01, 2019 at 09:18:15AM -0400, Jeff King wrote:\n> diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\n> new file mode 100755\n> index 0000000000..3353216f09\n> --- /dev/null\n> +++ b/t/t5618-alternate-refs.sh\n> @@ -0,0 +1,60 @@\n\n> +test_expect_success 'log --source shows .alternate marker' '\n> +\tgit log --oneline --source --remotes=origin >expect.orig &&\n> +\tsed \"s/origin.* /.alternate /\" expect &&\n\nUnnecessary redirection, 'sed' can open that file on its own as well.\n\n> +\tgit log --oneline --source --alternate-refs >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_done\n> -- \n> 2.22.0.776.g16867c022c\n"},{"id":"378546","messageId":"20190703164116.GA23260@sigill.intra.peff.net","threadId":"51400","inReplyTo":"20190703091225.GQ21574@szeder.dev","subject":"Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-03T16:41:16Z","receivedAt":"2019-07-03T16:41:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 03, 2019 at 11:12:25AM +0200, SZEDER Gábor wrote:\n\n> On Mon, Jul 01, 2019 at 09:18:15AM -0400, Jeff King wrote:\n> > diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\n> > new file mode 100755\n> > index 0000000000..3353216f09\n> > --- /dev/null\n> > +++ b/t/t5618-alternate-refs.sh\n> > @@ -0,0 +1,60 @@\n> \n> > +test_expect_success 'log --source shows .alternate marker' '\n> > +\tgit log --oneline --source --remotes=origin >expect.orig &&\n> > +\tsed \"s/origin.* /.alternate /\" expect &&\n> \n> Unnecessary redirection, 'sed' can open that file on its own as well.\n\nSure, but is there a compelling reason not to feed it as stdin?\n\n-Peff\n"},{"id":"378547","messageId":"xmqq5zoj2hi6.fsf@gitster-ct.c.googlers.com","threadId":"51400","inReplyTo":"20190703164116.GA23260@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-03T16:46:25Z","receivedAt":"2019-07-03T16:46:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jul 03, 2019 at 11:12:25AM +0200, SZEDER Gábor wrote:\n>\n>> On Mon, Jul 01, 2019 at 09:18:15AM -0400, Jeff King wrote:\n>> > diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\n>> > new file mode 100755\n>> > index 0000000000..3353216f09\n>> > --- /dev/null\n>> > +++ b/t/t5618-alternate-refs.sh\n>> > @@ -0,0 +1,60 @@\n>> \n>> > +test_expect_success 'log --source shows .alternate marker' '\n>> > +\tgit log --oneline --source --remotes=origin >expect.orig &&\n>> > +\tsed \"s/origin.* /.alternate /\" expect &&\n>> \n>> Unnecessary redirection, 'sed' can open that file on its own as well.\n>\n> Sure, but is there a compelling reason not to feed it as stdin?\n\nFWIW I do not think it is bad to redirect into a command at all ;-)\n\n\n\n"},{"id":"378548","messageId":"20190703165036.GS21574@szeder.dev","threadId":"51400","inReplyTo":"20190703164116.GA23260@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-07-03T16:50:36Z","receivedAt":"2019-07-03T16:50:42Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Jul 03, 2019 at 12:41:16PM -0400, Jeff King wrote:\n> On Wed, Jul 03, 2019 at 11:12:25AM +0200, SZEDER Gábor wrote:\n> \n> > On Mon, Jul 01, 2019 at 09:18:15AM -0400, Jeff King wrote:\n> > > diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\n> > > new file mode 100755\n> > > index 0000000000..3353216f09\n> > > --- /dev/null\n> > > +++ b/t/t5618-alternate-refs.sh\n> > > @@ -0,0 +1,60 @@\n> > \n> > > +test_expect_success 'log --source shows .alternate marker' '\n> > > +\tgit log --oneline --source --remotes=origin >expect.orig &&\n> > > +\tsed \"s/origin.* /.alternate /\" expect &&\n> > \n> > Unnecessary redirection, 'sed' can open that file on its own as well.\n> \n> Sure, but is there a compelling reason not to feed it as stdin?\n\nNot really, other than there is no compelling reason to do so :)\n\n"},{"id":"378550","messageId":"xmqqwogz1217.fsf@gitster-ct.c.googlers.com","threadId":"51400","inReplyTo":"20190703165036.GS21574@szeder.dev","subject":"Re: [PATCH v2 2/2] check_everything_connected: assume alternate ref tips are valid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-03T17:05:56Z","receivedAt":"2019-07-03T17:06:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> On Wed, Jul 03, 2019 at 12:41:16PM -0400, Jeff King wrote:\n>> On Wed, Jul 03, 2019 at 11:12:25AM +0200, SZEDER Gábor wrote:\n>> \n>> > On Mon, Jul 01, 2019 at 09:18:15AM -0400, Jeff King wrote:\n>> > > diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh\n>> > > new file mode 100755\n>> > > index 0000000000..3353216f09\n>> > > --- /dev/null\n>> > > +++ b/t/t5618-alternate-refs.sh\n>> > > @@ -0,0 +1,60 @@\n>> > \n>> > > +test_expect_success 'log --source shows .alternate marker' '\n>> > > +\tgit log --oneline --source --remotes=origin >expect.orig &&\n>> > > +\tsed \"s/origin.* /.alternate /\" expect &&\n>> > \n>> > Unnecessary redirection, 'sed' can open that file on its own as well.\n>> \n>> Sure, but is there a compelling reason not to feed it as stdin?\n>\n> Not really, other than there is no compelling reason to do so :)\n\nFor this particular one, it would not make much difference, but when\nfeeding a single file to a command that can take many instructions\nas command line arguments, I tend to prefer\n\n\t$ cmd