{"thread":{"id":"48662","subject":"[PATCH 0/2] Fix protocol v2 tag following with CLI refspec","startedAt":"2018-06-05T21:16:48Z","lastAt":"2018-07-09T20:35:53Z","messageCount":23,"participants":["Jonathan Tan","Brandon Williams","Stefan Beller","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"349407","messageId":"cover.1528233146.git.jonathantanmy@google.com","threadId":"48662","inReplyTo":null,"subject":"[PATCH 0/2] Fix protocol v2 tag following with CLI refspec","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-05T21:16:33Z","receivedAt":"2018-06-05T21:16:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"After the events that led up to Jonathan Nieder's patch fixing fetch by\nSHA-1 in protocol v2 [1], while expanding protocol v2's test coverage, I\nfound a bug with tag following. Patch 2 is a patch that fixes that bug\n(and patch 1 is a related but independent test that I had written\nbeforehand).\n\n[1] https://public-inbox.org/git/20180531072339.GA43435@aiede.svl.corp.google.com/\n\nJonathan Tan (2):\n t5702: test fetch with multiple refspecs at a time\n fetch: send \"refs/tags/\" prefix upon CLI refspecs\n\n builtin/fetch.c | 2 +-\n t/t5702-protocol-v2.sh | 65 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 66 insertions(+), 1 deletion(-)\n\n-- \n2.17.0.768.g1526ddbba1.dirty\n\n"},{"id":"349408","messageId":"2bc7df55a6451afbd35638b31a1551c648129b22.1528233146.git.jonathantanmy@google.com","threadId":"48662","inReplyTo":"cover.1528233146.git.jonathantanmy@google.com","subject":"[PATCH 1/2] t5702: test fetch with multiple refspecs at a time","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-05T21:16:34Z","receivedAt":"2018-06-05T21:16:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Extend the protocol v2 tests to also test fetches with multiple refspecs\nspecified. This also covers the previously uncovered cases of fetching\nwith prefix matching and fetching by SHA-1.\n\nSigned-off-by: Jonathan Tan \n---\n t/t5702-protocol-v2.sh | 47 ++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 47 insertions(+)\n\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex a4fe6508b..261e82b0f 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -359,6 +359,53 @@ test_expect_success 'default refspec is used to filter ref when fetchcing' '\n \tgrep \"ref-prefix refs/tags/\" log\n '\n \n+test_expect_success 'fetch supports various ways of have lines' '\n+\trm -rf server client trace &&\n+\tgit init server &&\n+\ttest_commit -C server dwim &&\n+\tTREE=$(git -C server rev-parse HEAD^{tree}) &&\n+\tgit -C server tag exact \\\n+\t\t$(git -C server commit-tree -m a \"$TREE\") &&\n+\tgit -C server tag dwim-unwanted \\\n+\t\t$(git -C server commit-tree -m b \"$TREE\") &&\n+\tgit -C server tag exact-unwanted \\\n+\t\t$(git -C server commit-tree -m c \"$TREE\") &&\n+\tgit -C server tag prefix1 \\\n+\t\t$(git -C server commit-tree -m d \"$TREE\") &&\n+\tgit -C server tag prefix2 \\\n+\t\t$(git -C server commit-tree -m e \"$TREE\") &&\n+\tgit -C server tag fetch-by-sha1 \\\n+\t\t$(git -C server commit-tree -m f \"$TREE\") &&\n+\tgit -C server tag completely-unrelated \\\n+\t\t$(git -C server commit-tree -m g \"$TREE\") &&\n+\t\n+\tgit init client &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n+\t\tfetch \"file://$(pwd)/server\" \\\n+\t\tdwim \\\n+\t\trefs/tags/exact \\\n+\t\trefs/tags/prefix*:refs/tags/prefix* \\\n+\t\t\"$(git -C server rev-parse fetch-by-sha1)\" &&\n+\n+\t# Ensure that the appropriate prefixes are sent (using a sample)\n+\tgrep \"fetch> ref-prefix dwim\" trace &&\n+\tgrep \"fetch> ref-prefix refs/heads/dwim\" trace &&\n+\tgrep \"fetch> ref-prefix refs/tags/prefix\" trace &&\n+\n+\t# Ensure that the correct objects are returned\n+\tgit -C client cat-file -e $(git -C server rev-parse dwim) &&\n+\tgit -C client cat-file -e $(git -C server rev-parse exact) &&\n+\tgit -C client cat-file -e $(git -C server rev-parse prefix1) &&\n+\tgit -C client cat-file -e $(git -C server rev-parse prefix2) &&\n+\tgit -C client cat-file -e $(git -C server rev-parse fetch-by-sha1) &&\n+\ttest_must_fail git -C client cat-file -e \\\n+\t\t$(git -C server rev-parse dwim-unwanted) &&\n+\ttest_must_fail git -C client cat-file -e \\\n+\t\t$(git -C server rev-parse exact-unwanted) &&\n+\ttest_must_fail git -C client cat-file -e \\\n+\t\t$(git -C server rev-parse completely-unrelated)\n+'\n+\n # Test protocol v2 with 'http://' transport\n #\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n-- \n2.17.0.768.g1526ddbba1.dirty\n\n"},{"id":"349409","messageId":"4d69aa14dadbbc53defa6d7d6c9a62f2b02a0bfb.1528233146.git.jonathantanmy@google.com","threadId":"48662","inReplyTo":"cover.1528233146.git.jonathantanmy@google.com","subject":"[PATCH 2/2] fetch: send \"refs/tags/\" prefix upon CLI refspecs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-05T21:16:35Z","receivedAt":"2018-06-05T21:16:52Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When performing tag following, in addition to using the server's\n\"include-tag\" capability to send tag objects (and emulating it if the\nserver does not support that capability), \"git fetch\" relies upon the\npresence of refs/tags/* entries in the initial ref advertisement to\nlocally create refs pointing to the aforementioned tag objects. When\nusing protocol v2, refs/tags/* entries in the initial ref advertisement\nmay be suppressed by a ref-prefix argument, leading to the tag object\nbeing downloaded, but the ref not being created.\n\nCommit dcc73cf7ff (\"fetch: generate ref-prefixes when using a configured\nrefspec\", 2018-05-18) ensured that \"refs/tags/\" is always sent as a ref\nprefix when \"git fetch\" is invoked with no refspecs, but not when \"git\nfetch\" is invoked with refspecs. Extend that functionality to make it\nwork in both situations.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/fetch.c | 2 +-\n t/t5702-protocol-v2.sh | 18 ++++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ea5b9669a..1f447f1e8 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,\n \t\trefspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);\n \n \tif (ref_prefixes.argc &&\n-\t (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {\n+\t (tags == TAGS_SET || tags == TAGS_DEFAULT)) {\n \t\targv_array_push(&ref_prefixes, \"refs/tags/\");\n \t}\n \ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 261e82b0f..6733579c1 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -406,6 +406,24 @@ test_expect_success 'fetch supports various ways of have lines' '\n \t\t$(git -C server rev-parse completely-unrelated)\n '\n \n+test_expect_success 'fetch supports include-tag and tag following' '\n+\trm -rf server client trace &&\n+\tgit init server &&\n+\n+\ttest_commit -C server to_fetch &&\n+\tgit -C server tag -a annotated_tag -m message &&\n+\n+\tgit init client &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n+\t\tfetch \"$(pwd)/server\" to_fetch:to_fetch &&\n+\n+\tgrep \"fetch> ref-prefix to_fetch\" trace &&\n+\tgrep \"fetch> ref-prefix refs/tags/\" trace &&\n+\tgrep \"fetch> include-tag\" trace &&\n+\n+\tgit -C client cat-file -e $(git -C client rev-parse annotated_tag)\n+'\n+\n # Test protocol v2 with 'http://' transport\n #\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n-- \n2.17.0.768.g1526ddbba1.dirty\n\n"},{"id":"349410","messageId":"20180605212537.GF158365@google.com","threadId":"48662","inReplyTo":"4d69aa14dadbbc53defa6d7d6c9a62f2b02a0bfb.1528233146.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] fetch: send \"refs/tags/\" prefix upon CLI refspecs","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-06-05T21:25:37Z","receivedAt":"2018-06-05T21:25:43Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/05, Jonathan Tan wrote:\n> When performing tag following, in addition to using the server's\n> \"include-tag\" capability to send tag objects (and emulating it if the\n> server does not support that capability), \"git fetch\" relies upon the\n> presence of refs/tags/* entries in the initial ref advertisement to\n> locally create refs pointing to the aforementioned tag objects. When\n> using protocol v2, refs/tags/* entries in the initial ref advertisement\n> may be suppressed by a ref-prefix argument, leading to the tag object\n> being downloaded, but the ref not being created.\n> \n> Commit dcc73cf7ff (\"fetch: generate ref-prefixes when using a configured\n> refspec\", 2018-05-18) ensured that \"refs/tags/\" is always sent as a ref\n> prefix when \"git fetch\" is invoked with no refspecs, but not when \"git\n> fetch\" is invoked with refspecs. Extend that functionality to make it\n> work in both situations.\n> \n> Signed-off-by: Jonathan Tan \n\nTest t5702-protocol-v2.sh doesn't pass with this patch.\n\n> ---\n> builtin/fetch.c | 2 +-\n> t/t5702-protocol-v2.sh | 18 ++++++++++++++++++\n> 2 files changed, 19 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index ea5b9669a..1f447f1e8 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,\n> \t\trefspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);\n> \n> \tif (ref_prefixes.argc &&\n> -\t (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {\n> +\t (tags == TAGS_SET || tags == TAGS_DEFAULT)) {\n> \t\targv_array_push(&ref_prefixes, \"refs/tags/\");\n> \t}\n> \n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index 261e82b0f..6733579c1 100755\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -406,6 +406,24 @@ test_expect_success 'fetch supports various ways of have lines' '\n> \t\t$(git -C server rev-parse completely-unrelated)\n> '\n> \n> +test_expect_success 'fetch supports include-tag and tag following' '\n> +\trm -rf server client trace &&\n> +\tgit init server &&\n> +\n> +\ttest_commit -C server to_fetch &&\n> +\tgit -C server tag -a annotated_tag -m message &&\n> +\n> +\tgit init client &&\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n> +\t\tfetch \"$(pwd)/server\" to_fetch:to_fetch &&\n> +\n> +\tgrep \"fetch> ref-prefix to_fetch\" trace &&\n> +\tgrep \"fetch> ref-prefix refs/tags/\" trace &&\n> +\tgrep \"fetch> include-tag\" trace &&\n> +\n> +\tgit -C client cat-file -e $(git -C client rev-parse annotated_tag)\n> +'\n> +\n> # Test protocol v2 with 'http://' transport\n> #\n> . \"$TEST_DIRECTORY\"/lib-httpd.sh\n> -- \n> 2.17.0.768.g1526ddbba1.dirty\n> \n\n-- \nBrandon Williams\n"},{"id":"349411","messageId":"20180605212829.GG158365@google.com","threadId":"48662","inReplyTo":"4d69aa14dadbbc53defa6d7d6c9a62f2b02a0bfb.1528233146.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] fetch: send \"refs/tags/\" prefix upon CLI refspecs","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-06-05T21:28:29Z","receivedAt":"2018-06-05T21:28:36Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/05, Jonathan Tan wrote:\n> When performing tag following, in addition to using the server's\n> \"include-tag\" capability to send tag objects (and emulating it if the\n> server does not support that capability), \"git fetch\" relies upon the\n> presence of refs/tags/* entries in the initial ref advertisement to\n> locally create refs pointing to the aforementioned tag objects. When\n> using protocol v2, refs/tags/* entries in the initial ref advertisement\n> may be suppressed by a ref-prefix argument, leading to the tag object\n> being downloaded, but the ref not being created.\n> \n> Commit dcc73cf7ff (\"fetch: generate ref-prefixes when using a configured\n> refspec\", 2018-05-18) ensured that \"refs/tags/\" is always sent as a ref\n> prefix when \"git fetch\" is invoked with no refspecs, but not when \"git\n> fetch\" is invoked with refspecs. Extend that functionality to make it\n> work in both situations.\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> builtin/fetch.c | 2 +-\n> t/t5702-protocol-v2.sh | 18 ++++++++++++++++++\n> 2 files changed, 19 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index ea5b9669a..1f447f1e8 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,\n> \t\trefspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);\n> \n> \tif (ref_prefixes.argc &&\n> -\t (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {\n> +\t (tags == TAGS_SET || tags == TAGS_DEFAULT)) {\n> \t\targv_array_push(&ref_prefixes, \"refs/tags/\");\n> \t}\n\nThis is difficult...Really I don't think the default should be to follow\ntags. Mostly because this defeats the purpose of ref filtering when a\nuser only requests the master branch. Now instead of the server only\nsending the master branch, you get the whole tags namespace as well.\n\n> \n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index 261e82b0f..6733579c1 100755\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -406,6 +406,24 @@ test_expect_success 'fetch supports various ways of have lines' '\n> \t\t$(git -C server rev-parse completely-unrelated)\n> '\n> \n> +test_expect_success 'fetch supports include-tag and tag following' '\n> +\trm -rf server client trace &&\n> +\tgit init server &&\n> +\n> +\ttest_commit -C server to_fetch &&\n> +\tgit -C server tag -a annotated_tag -m message &&\n> +\n> +\tgit init client &&\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n> +\t\tfetch \"$(pwd)/server\" to_fetch:to_fetch &&\n> +\n> +\tgrep \"fetch> ref-prefix to_fetch\" trace &&\n> +\tgrep \"fetch> ref-prefix refs/tags/\" trace &&\n> +\tgrep \"fetch> include-tag\" trace &&\n> +\n> +\tgit -C client cat-file -e $(git -C client rev-parse annotated_tag)\n> +'\n> +\n> # Test protocol v2 with 'http://' transport\n> #\n> . \"$TEST_DIRECTORY\"/lib-httpd.sh\n> -- \n> 2.17.0.768.g1526ddbba1.dirty\n> \n\n-- \nBrandon Williams\n"},{"id":"349413","messageId":"cover.1528234587.git.jonathantanmy@google.com","threadId":"48662","inReplyTo":"cover.1528233146.git.jonathantanmy@google.com","subject":"[PATCH v2 0/2] Fix protocol v2 tag following with CLI refspec","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-05T21:40:34Z","receivedAt":"2018-06-05T21:40:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Test t5702-protocol-v2.sh doesn't pass with this patch.\n\nGood catch - I've fixed that.\n\n> This is difficult...Really I don't think the default should be to follow\n> tags. Mostly because this defeats the purpose of ref filtering when a\n> user only requests the master branch. Now instead of the server only\n> sending the master branch, you get the whole tags namespace as well.\n\nIt's true that there is now a performance drop. My instinctive reaction\nis to be conservative and preserve the existing behavior, but I'll see\nwhat others on this list think.\n\nIt's worth nothing that with ref-in-want (for example, in your latest\nseries [1]) we might be able to restore performance, because the server\ncan send refs/tags/X with the packfile instead of sending all\nrefs/tags/X refs initially to the client.\n\n[1] https://public-inbox.org/git/20180605175144.4225-1-bmwill@google.com/\n\nJonathan Tan (2):\n t5702: test fetch with multiple refspecs at a time\n fetch: send \"refs/tags/\" prefix upon CLI refspecs\n\n builtin/fetch.c | 2 +-\n t/t5702-protocol-v2.sh | 71 ++++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 69 insertions(+), 4 deletions(-)\n\n-- \n2.17.0.768.g1526ddbba1.dirty\n\n"},{"id":"349414","messageId":"2bc7df55a6451afbd35638b31a1551c648129b22.1528234587.git.jonathantanmy@google.com","threadId":"48662","inReplyTo":"cover.1528234587.git.jonathantanmy@google.com","subject":"[PATCH v2 1/2] t5702: test fetch with multiple refspecs at a time","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-05T21:40:35Z","receivedAt":"2018-06-05T21:40:55Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Extend the protocol v2 tests to also test fetches with multiple refspecs\nspecified. This also covers the previously uncovered cases of fetching\nwith prefix matching and fetching by SHA-1.\n\nSigned-off-by: Jonathan Tan \n---\n t/t5702-protocol-v2.sh | 47 ++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 47 insertions(+)\n\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex a4fe6508b..261e82b0f 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -359,6 +359,53 @@ test_expect_success 'default refspec is used to filter ref when fetchcing' '\n \tgrep \"ref-prefix refs/tags/\" log\n '\n \n+test_expect_success 'fetch supports various ways of have lines' '\n+\trm -rf server client trace &&\n+\tgit init server &&\n+\ttest_commit -C server dwim &&\n+\tTREE=$(git -C server rev-parse HEAD^{tree}) &&\n+\tgit -C server tag exact \\\n+\t\t$(git -C server commit-tree -m a \"$TREE\") &&\n+\tgit -C server tag dwim-unwanted \\\n+\t\t$(git -C server commit-tree -m b \"$TREE\") &&\n+\tgit -C server tag exact-unwanted \\\n+\t\t$(git -C server commit-tree -m c \"$TREE\") &&\n+\tgit -C server tag prefix1 \\\n+\t\t$(git -C server commit-tree -m d \"$TREE\") &&\n+\tgit -C server tag prefix2 \\\n+\t\t$(git -C server commit-tree -m e \"$TREE\") &&\n+\tgit -C server tag fetch-by-sha1 \\\n+\t\t$(git -C server commit-tree -m f \"$TREE\") &&\n+\tgit -C server tag completely-unrelated \\\n+\t\t$(git -C server commit-tree -m g \"$TREE\") &&\n+\t\n+\tgit init client &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n+\t\tfetch \"file://$(pwd)/server\" \\\n+\t\tdwim \\\n+\t\trefs/tags/exact \\\n+\t\trefs/tags/prefix*:refs/tags/prefix* \\\n+\t\t\"$(git -C server rev-parse fetch-by-sha1)\" &&\n+\n+\t# Ensure that the appropriate prefixes are sent (using a sample)\n+\tgrep \"fetch> ref-prefix dwim\" trace &&\n+\tgrep \"fetch> ref-prefix refs/heads/dwim\" trace &&\n+\tgrep \"fetch> ref-prefix refs/tags/prefix\" trace &&\n+\n+\t# Ensure that the correct objects are returned\n+\tgit -C client cat-file -e $(git -C server rev-parse dwim) &&\n+\tgit -C client cat-file -e $(git -C server rev-parse exact) &&\n+\tgit -C client cat-file -e $(git -C server rev-parse prefix1) &&\n+\tgit -C client cat-file -e $(git -C server rev-parse prefix2) &&\n+\tgit -C client cat-file -e $(git -C server rev-parse fetch-by-sha1) &&\n+\ttest_must_fail git -C client cat-file -e \\\n+\t\t$(git -C server rev-parse dwim-unwanted) &&\n+\ttest_must_fail git -C client cat-file -e \\\n+\t\t$(git -C server rev-parse exact-unwanted) &&\n+\ttest_must_fail git -C client cat-file -e \\\n+\t\t$(git -C server rev-parse completely-unrelated)\n+'\n+\n # Test protocol v2 with 'http://' transport\n #\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n-- \n2.17.0.768.g1526ddbba1.dirty\n\n"},{"id":"349415","messageId":"c6910161aab1f383b5721bdc91969baad8c10a66.1528234587.git.jonathantanmy@google.com","threadId":"48662","inReplyTo":"cover.1528234587.git.jonathantanmy@google.com","subject":"[PATCH v2 2/2] fetch: send \"refs/tags/\" prefix upon CLI refspecs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-05T21:40:36Z","receivedAt":"2018-06-05T21:40:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When performing tag following, in addition to using the server's\n\"include-tag\" capability to send tag objects (and emulating it if the\nserver does not support that capability), \"git fetch\" relies upon the\npresence of refs/tags/* entries in the initial ref advertisement to\nlocally create refs pointing to the aforementioned tag objects. When\nusing protocol v2, refs/tags/* entries in the initial ref advertisement\nmay be suppressed by a ref-prefix argument, leading to the tag object\nbeing downloaded, but the ref not being created.\n\nCommit dcc73cf7ff (\"fetch: generate ref-prefixes when using a configured\nrefspec\", 2018-05-18) ensured that \"refs/tags/\" is always sent as a ref\nprefix when \"git fetch\" is invoked with no refspecs, but not when \"git\nfetch\" is invoked with refspecs. Extend that functionality to make it\nwork in both situations.\n\nThis also necessitates a change another test which tested ref\nadvertisement filtering using tag refs - since tag refs are sent by\ndefault now, the test has been switched to using branch refs instead.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/fetch.c | 2 +-\n t/t5702-protocol-v2.sh | 24 +++++++++++++++++++++---\n 2 files changed, 22 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ea5b9669a..1f447f1e8 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,\n \t\trefspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);\n \n \tif (ref_prefixes.argc &&\n-\t (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {\n+\t (tags == TAGS_SET || tags == TAGS_DEFAULT)) {\n \t\targv_array_push(&ref_prefixes, \"refs/tags/\");\n \t}\n \ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 261e82b0f..b31b6d8d3 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -204,6 +204,7 @@ test_expect_success 'ref advertisment is filtered during fetch using protocol v2\n \ttest_when_finished \"rm -f log\" &&\n \n \ttest_commit -C file_parent three &&\n+\tgit -C file_parent branch unwanted-branch three &&\n \n \tGIT_TRACE_PACKET=\"$(pwd)/log\" git -C file_child -c protocol.version=2 \\\n \t\tfetch origin master &&\n@@ -212,9 +213,8 @@ test_expect_success 'ref advertisment is filtered during fetch using protocol v2\n \tgit -C file_parent log -1 --format=%s >expect &&\n \ttest_cmp expect actual &&\n \n-\t! grep \"refs/tags/one\" log &&\n-\t! grep \"refs/tags/two\" log &&\n-\t! grep \"refs/tags/three\" log\n+\tgrep \"refs/heads/master\" log &&\n+\t! grep \"refs/heads/unwanted-branch\" log\n '\n \n test_expect_success 'server-options are sent when fetching' '\n@@ -406,6 +406,24 @@ test_expect_success 'fetch supports various ways of have lines' '\n \t\t$(git -C server rev-parse completely-unrelated)\n '\n \n+test_expect_success 'fetch supports include-tag and tag following' '\n+\trm -rf server client trace &&\n+\tgit init server &&\n+\n+\ttest_commit -C server to_fetch &&\n+\tgit -C server tag -a annotated_tag -m message &&\n+\n+\tgit init client &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n+\t\tfetch \"$(pwd)/server\" to_fetch:to_fetch &&\n+\n+\tgrep \"fetch> ref-prefix to_fetch\" trace &&\n+\tgrep \"fetch> ref-prefix refs/tags/\" trace &&\n+\tgrep \"fetch> include-tag\" trace &&\n+\n+\tgit -C client cat-file -e $(git -C client rev-parse annotated_tag)\n+'\n+\n # Test protocol v2 with 'http://' transport\n #\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n-- \n2.17.0.768.g1526ddbba1.dirty\n\n"},{"id":"350407","messageId":"CAGZ79kbVsV390C0p3dAs3Ddp=K7mLnzfBhFC2jS4oon3Uy_nvw@mail.gmail.com","threadId":"48662","inReplyTo":"2bc7df55a6451afbd35638b31a1551c648129b22.1528234587.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 1/2] t5702: test fetch with multiple refspecs at a time","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-18T18:30:53Z","receivedAt":"2018-06-18T18:31:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jun 5, 2018 at 2:40 PM Jonathan Tan wrote:\n>\n> Extend the protocol v2 tests to also test fetches with multiple refspecs\n> specified. This also covers the previously uncovered cases of fetching\n> with prefix matching and fetching by SHA-1.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> t/t5702-protocol-v2.sh | 47 ++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 47 insertions(+)\n>\n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index a4fe6508b..261e82b0f 100755\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -359,6 +359,53 @@ test_expect_success 'default refspec is used to filter ref when fetchcing' '\n> grep \"ref-prefix refs/tags/\" log\n> '\n>\n> +test_expect_success 'fetch supports various ways of have lines' '\n> + rm -rf server client trace &&\n\nCan we move these deletions to test_when_finished of the previous(?) test\nas well as have them here in a test_when_finished line?\n\n> + git init server &&\n> + test_commit -C server dwim &&\n> + TREE=$(git -C server rev-parse HEAD^{tree}) &&\n> + git -C server tag exact \\\n> + $(git -C server commit-tree -m a \"$TREE\") &&\n> + git -C server tag dwim-unwanted \\\n> + $(git -C server commit-tree -m b \"$TREE\") &&\n> + git -C server tag exact-unwanted \\\n> + $(git -C server commit-tree -m c \"$TREE\") &&\n> + git -C server tag prefix1 \\\n> + $(git -C server commit-tree -m d \"$TREE\") &&\n> + git -C server tag prefix2 \\\n> + $(git -C server commit-tree -m e \"$TREE\") &&\n> + git -C server tag fetch-by-sha1 \\\n> + $(git -C server commit-tree -m f \"$TREE\") &&\n> + git -C server tag completely-unrelated \\\n> + $(git -C server commit-tree -m g \"$TREE\") &&\n\nSo you create different commits all with the same tree; no parents;\nthis seems to convey nicely that this is about testing tags.\n\n> + git init client &&\n> + GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n> + fetch \"file://$(pwd)/server\" \\\n> + dwim \\\n> + refs/tags/exact \\\n> + refs/tags/prefix*:refs/tags/prefix* \\\n> + \"$(git -C server rev-parse fetch-by-sha1)\" &&\n> +\n> + # Ensure that the appropriate prefixes are sent (using a sample)\n> + grep \"fetch> ref-prefix dwim\" trace &&\n> + grep \"fetch> ref-prefix refs/heads/dwim\" trace &&\n> + grep \"fetch> ref-prefix refs/tags/prefix\" trace &&\n> +\n> + # Ensure that the correct objects are returned\n> + git -C client cat-file -e $(git -C server rev-parse dwim) &&\n> + git -C client cat-file -e $(git -C server rev-parse exact) &&\n> + git -C client cat-file -e $(git -C server rev-parse prefix1) &&\n> + git -C client cat-file -e $(git -C server rev-parse prefix2) &&\n> + git -C client cat-file -e $(git -C server rev-parse fetch-by-sha1) &&\n> + test_must_fail git -C client cat-file -e \\\n> + $(git -C server rev-parse dwim-unwanted) &&\n> + test_must_fail git -C client cat-file -e \\\n> + $(git -C server rev-parse exact-unwanted) &&\n> + test_must_fail git -C client cat-file -e \\\n> + $(git -C server rev-parse completely-unrelated)\n> +'\n\nThis test is precise and easy to understand; the patch is\nReviewed-by: Stefan Beller \n(considering the test_when_finished comment as\nan optional nit)\n"},{"id":"350413","messageId":"CAGf8dgLXi95ZUin+EWQ5Q9MfNT5Qxs82=F5w_ohDw9-cK7OfFg@mail.gmail.com","threadId":"48662","inReplyTo":"CAGZ79kbVsV390C0p3dAs3Ddp=K7mLnzfBhFC2jS4oon3Uy_nvw@mail.gmail.com","subject":"Re: [PATCH v2 1/2] t5702: test fetch with multiple refspecs at a time","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-18T19:15:43Z","receivedAt":"2018-06-18T19:15:50Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Mon, Jun 18, 2018 at 11:30 AM, Stefan Beller wrote:\n>> +test_expect_success 'fetch supports various ways of have lines' '\n>> + rm -rf server client trace &&\n>\n> Can we move these deletions to test_when_finished of the previous(?) test\n> as well as have them here in a test_when_finished line?\n\nI think that deleting them when necessary makes it more explicit, and\nthis also supports tests where a repository is set up and then used\nover multiple tests (e.g. having separate \"setup X\" and \"use X\"\ntests).\n\n> This test is precise and easy to understand; the patch is\n> Reviewed-by: Stefan Beller \n> (considering the test_when_finished comment as\n> an optional nit)\n\nThanks. Do you have any comments about the performance issue that\nBrandon brought up?\n"},{"id":"350414","messageId":"CAGZ79kbB0Tv8wb_7j0=OdQqGU78KHp3JzuCeD01JTF4EHwOH0w@mail.gmail.com","threadId":"48662","inReplyTo":"c6910161aab1f383b5721bdc91969baad8c10a66.1528234587.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] fetch: send \"refs/tags/\" prefix upon CLI refspecs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-18T19:22:27Z","receivedAt":"2018-06-18T19:22:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jun 5, 2018 at 2:41 PM Jonathan Tan wrote:\n>\n> When performing tag following, in addition to using the server's\n> \"include-tag\" capability to send tag objects (and emulating it if the\n> server does not support that capability), \"git fetch\" relies upon the\n> presence of refs/tags/* entries in the initial ref advertisement to\n> locally create refs pointing to the aforementioned tag objects. When\n> using protocol v2, refs/tags/* entries in the initial ref advertisement\n> may be suppressed by a ref-prefix argument, leading to the tag object\n> being downloaded, but the ref not being created.\n>\n> Commit dcc73cf7ff (\"fetch: generate ref-prefixes when using a configured\n> refspec\", 2018-05-18) ensured that \"refs/tags/\" is always sent as a ref\n> prefix when \"git fetch\" is invoked with no refspecs, but not when \"git\n> fetch\" is invoked with refspecs. Extend that functionality to make it\n> work in both situations.\n\nokay. Thinking long term, we may want to introduce a capability that\ncan filter the tag space, e.g. \"want-refs-since refs/tags/*\"\nas a client directive as then the client only asks for new (newly\ncreated/appearing) tags instead of all tags.\n\n> This also necessitates a change another test which tested ref\n> advertisement filtering using tag refs -\n\nsounds plausible.\n\n> since tag refs are sent by\n> default now, the test has been switched to using branch refs instead.\n\nThat mismatches what I would expect to read below.\nI would expect the client to always ask for refs/tags/* now and\ninstead of the server just giving them.\n\nOh, the problem is in that other test to restrict the refs/tags\nto *not* be sent?\n\nMaybe we can only ask for refs/tags/* if we do not have any\n\"refs/tags/\" on the CLI: if I invoke \"git fetch refs/tags/v1\"\nI would not get an advertisement for refs/tags/v2 but if I omit\nall tags from the refspec, I'd get all its advertisements (v1+v2)\n\n\n\n> Signed-off-by: Jonathan Tan \n> ---\n> builtin/fetch.c | 2 +-\n> t/t5702-protocol-v2.sh | 24 +++++++++++++++++++++---\n> 2 files changed, 22 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index ea5b9669a..1f447f1e8 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,\n> refspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);\n>\n> if (ref_prefixes.argc &&\n> - (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {\n> + (tags == TAGS_SET || tags == TAGS_DEFAULT)) {\n\nOh, I see. This always asks for refs/tags/ whereas before we only\nasked for them if there were *no* refspec given. Maybe we can\nchange this to\n\n refspec_any_item_contains(\"refs/tags/\")\n\ninstead of always asking for the tags?\n(that function would need to be written; I guess for a short term bugfix\nthis is good enough)\n\nHow is the tag following documented (i.e. when is the user least\nsurprised that we do not tag-follow all and unconditionally)?\n\n\n> argv_array_push(&ref_prefixes, \"refs/tags/\");\n> }\n>\n> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\n> index 261e82b0f..b31b6d8d3 100755\n> --- a/t/t5702-protocol-v2.sh\n> +++ b/t/t5702-protocol-v2.sh\n> @@ -204,6 +204,7 @@ test_expect_success 'ref advertisment is filtered during fetch using protocol v2\n> test_when_finished \"rm -f log\" &&\n>\n> test_commit -C file_parent three &&\n> + git -C file_parent branch unwanted-branch three &&\n>\n> GIT_TRACE_PACKET=\"$(pwd)/log\" git -C file_child -c protocol.version=2 \\\n> fetch origin master &&\n> @@ -212,9 +213,8 @@ test_expect_success 'ref advertisment is filtered during fetch using protocol v2\n> git -C file_parent log -1 --format=%s >expect &&\n> test_cmp expect actual &&\n>\n> - ! grep \"refs/tags/one\" log &&\n> - ! grep \"refs/tags/two\" log &&\n> - ! grep \"refs/tags/three\" log\n> + grep \"refs/heads/master\" log &&\n> + ! grep \"refs/heads/unwanted-branch\" log\n> '\n\nThat makes sense so far.\n\n>\n> test_expect_success 'server-options are sent when fetching' '\n> @@ -406,6 +406,24 @@ test_expect_success 'fetch supports various ways of have lines' '\n> $(git -C server rev-parse completely-unrelated)\n> '\n>\n> +test_expect_success 'fetch supports include-tag and tag following' '\n> + rm -rf server client trace &&\n\ntest_when_finished ;)\n\n> + git init server &&\n> +\n> + test_commit -C server to_fetch &&\n> + git -C server tag -a annotated_tag -m message &&\n> +\n> + git init client &&\n> + GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n> + fetch \"$(pwd)/server\" to_fetch:to_fetch &&\n> +\n> + grep \"fetch> ref-prefix to_fetch\" trace &&\n> + grep \"fetch> ref-prefix refs/tags/\" trace &&\n> + grep \"fetch> include-tag\" trace &&\n> +\n> + git -C client cat-file -e $(git -C client rev-parse annotated_tag)\n> +'\n\nMakes sense.\n\nThanks,\nStefan\n"},{"id":"350417","messageId":"CAGZ79kZ0Y6Xu=2ikuejfEAvrsKc7uzyFdhErUO9hydv3aQuBjw@mail.gmail.com","threadId":"48662","inReplyTo":"CAGf8dgLXi95ZUin+EWQ5Q9MfNT5Qxs82=F5w_ohDw9-cK7OfFg@mail.gmail.com","subject":"Re: [PATCH v2 1/2] t5702: test fetch with multiple refspecs at a time","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-18T19:32:23Z","receivedAt":"2018-06-18T19:32:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jun 18, 2018 at 12:15 PM Jonathan Tan wrote:\n>\n> On Mon, Jun 18, 2018 at 11:30 AM, Stefan Beller wrote:\n> >> +test_expect_success 'fetch supports various ways of have lines' '\n> >> + rm -rf server client trace &&\n> >\n> > Can we move these deletions to test_when_finished of the previous(?) test\n> > as well as have them here in a test_when_finished line?\n>\n> I think that deleting them when necessary makes it more explicit, and\n> this also supports tests where a repository is set up and then used\n> over multiple tests (e.g. having separate \"setup X\" and \"use X\"\n> tests).\n\nand it is necessary at the end of each test, so that we minimize\nthe dependencies between tests. In an ideal world you could run\nany one test in the file alone and would still pass. :)\n\n>\n> > This test is precise and easy to understand; the patch is\n> > Reviewed-by: Stefan Beller \n> > (considering the test_when_finished comment as\n> > an optional nit)\n>\n> Thanks. Do you have any comments about the performance issue that\n> Brandon brought up?\n\nIs that https://public-inbox.org/git/20180605212829.GG158365@google.com/ ?\n(otherwise I'd be missing the performance issue)\n\nFor now I'd filter out tags only if a specific tag is requested\n(i.e. the user given refspec contains \"refs/tags/\" -> no tag following)\nand then later I'd try out a date(?) based capability that asks for\nnew tags only.\n\nNote that you can create old tags by backdating them, so we'd have\nto ask the server for any update in its reflog for refs/tags...\n"},{"id":"350420","messageId":"20180618194750.137341-1-jonathantanmy@google.com","threadId":"48662","inReplyTo":"CAGZ79kbB0Tv8wb_7j0=OdQqGU78KHp3JzuCeD01JTF4EHwOH0w@mail.gmail.com","subject":"Re: [PATCH v2 2/2] fetch: send \"refs/tags/\" prefix upon CLI refspecs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-18T19:47:50Z","receivedAt":"2018-06-18T19:47:58Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> okay. Thinking long term, we may want to introduce a capability that\n> can filter the tag space, e.g. \"want-refs-since refs/tags/*\"\n> as a client directive as then the client only asks for new (newly\n> created/appearing) tags instead of all tags.\n\nI don't think refs normally have a created/modified time.\n\n> > since tag refs are sent by\n> > default now, the test has been switched to using branch refs instead.\n> \n> That mismatches what I would expect to read below.\n> I would expect the client to always ask for refs/tags/* now and\n> instead of the server just giving them.\n\nI don't understand why there is a mismatch, so I'll just answer all your\nquestions. Yes, the client indeed always asks for refs/tags/* now.\n\nBy the server just giving them, do you mean (1) giving them based on\nwhat is being fetched, or (2) giving them as if refs/tags/* was sent? If\n(1), this is possible, but requires the server to precompute all the\ncommits that would be sent (and this would be an overestimate, since\nnegotiation has not yet taken place). If (2), I think it's better to let\nthe client control this (since the client could just as easily want\n--no-tags, then sending tags would be wasted).\n\n> Oh, the problem is in that other test to restrict the refs/tags\n> to *not* be sent?\n\nThe problem is that refs/tags/* is sent by default. Maybe adding\n--no-tags would work (I haven't tried it, though), but I think it's\nclearer for the test to just operate on branches, than to have --no-tags\nand let the reader wonder what --no-tags has to do with what's being\ntested.\n\n> Maybe we can only ask for refs/tags/* if we do not have any\n> \"refs/tags/\" on the CLI: if I invoke \"git fetch refs/tags/v1\"\n> I would not get an advertisement for refs/tags/v2 but if I omit\n> all tags from the refspec, I'd get all its advertisements (v1+v2)\n\nThis would cause different behavior. For example, if I run \"git fetch\nrefs/heads/master refs/tags/foo\", I would expect tag following to work\neven if the tag's name does not start with refs/tags/foo.\n\n> > @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,\n> > refspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);\n> >\n> > if (ref_prefixes.argc &&\n> > - (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {\n> > + (tags == TAGS_SET || tags == TAGS_DEFAULT)) {\n> \n> Oh, I see. This always asks for refs/tags/ whereas before we only\n> asked for them if there were *no* refspec given. Maybe we can\n> change this to\n> \n> refspec_any_item_contains(\"refs/tags/\")\n> \n> instead of always asking for the tags?\n> (that function would need to be written; I guess for a short term bugfix\n> this is good enough)\n\nSame answer as above.\n\n> How is the tag following documented (i.e. when is the user least\n> surprised that we do not tag-follow all and unconditionally)?\n\nIt's documented under the --no-tags option in the man page for git\nfetch. I'm not sure what you mean by the user being surprised.\n"},{"id":"350421","messageId":"20180618195336.GE73085@google.com","threadId":"48662","inReplyTo":"20180618194750.137341-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] fetch: send \"refs/tags/\" prefix upon CLI refspecs","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-06-18T19:53:36Z","receivedAt":"2018-06-18T19:53:42Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/18, Jonathan Tan wrote:\n> \n> This would cause different behavior. For example, if I run \"git fetch\n> refs/heads/master refs/tags/foo\", I would expect tag following to work\n> even if the tag's name does not start with refs/tags/foo.\n\nI understand that some people *may* want tag following, but really its\nconfusing from an end user's standpoint. You can fetch a particular ref\nand end up with a bunch of tags that you didn't want. Aside from that\nmy biggest issue is with performance. The ref filtering was added to\ncut down on the number of refs sent from the server, now we're basically\nadding them all back no matter what (unless a user goes and flips the\ndefault to not fetch tags).\n\nI think we're probably going to need some people other than us to\ncomment on how this should be handled.\n\n> \n> > > @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,\n> > > refspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);\n> > >\n> > > if (ref_prefixes.argc &&\n> > > - (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {\n> > > + (tags == TAGS_SET || tags == TAGS_DEFAULT)) {\n> > \n> > Oh, I see. This always asks for refs/tags/ whereas before we only\n> > asked for them if there were *no* refspec given. Maybe we can\n> > change this to\n> > \n> > refspec_any_item_contains(\"refs/tags/\")\n> > \n> > instead of always asking for the tags?\n> > (that function would need to be written; I guess for a short term bugfix\n> > this is good enough)\n> \n> Same answer as above.\n> \n> > How is the tag following documented (i.e. when is the user least\n> > surprised that we do not tag-follow all and unconditionally)?\n> \n> It's documented under the --no-tags option in the man page for git\n> fetch. I'm not sure what you mean by the user being surprised.\n\n-- \nBrandon Williams\n"},{"id":"350424","messageId":"xmqq1sd3yii2.fsf@gitster-ct.c.googlers.com","threadId":"48662","inReplyTo":"c6910161aab1f383b5721bdc91969baad8c10a66.1528234587.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] fetch: send \"refs/tags/\" prefix upon CLI refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-18T19:58:13Z","receivedAt":"2018-06-18T19:58:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> When performing tag following, in addition to using the server's\n> \"include-tag\" capability to send tag objects (and emulating it if the\n> server does not support that capability), \"git fetch\" relies upon the\n> presence of refs/tags/* entries in the initial ref advertisement to\n> locally create refs pointing to the aforementioned tag objects. When\n> using protocol v2, refs/tags/* entries in the initial ref advertisement\n> may be suppressed by a ref-prefix argument, leading to the tag object\n> being downloaded, but the ref not being created.\n\nI wonder if it is reasonable to define the protocol v2 semantics of\n\"include-tag\" request a bit differently.\n\nUnlike v0 protocol where the client iterates though the advertised\nrefs and see if objects at the tip of them, even if they weren't\nwhat the client initially wanted to fetch, to find these unsolicited\nfollowed tag objects, allow the server to say, \"I've included some\nobjects you did not explicitly ask for, and here are the refs/tags/*\nnames you should place on them\", somewhat similar to the way how the\nref-in-want thing would work (i.e. the client does not really ask\nfor just objects and decides what name to place on them---instead it\nlets the server to make part of the decision).\n\nWouldn't that allow us not having to advertise the whole tags\nnamespace only to implement the tag following?\n"},{"id":"350425","messageId":"20180618210731.152140-1-jonathantanmy@google.com","threadId":"48662","inReplyTo":"xmqq1sd3yii2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/2] fetch: send \"refs/tags/\" prefix upon CLI refspecs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-18T21:07:31Z","receivedAt":"2018-06-18T21:07:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> > When performing tag following, in addition to using the server's\n> > \"include-tag\" capability to send tag objects (and emulating it if the\n> > server does not support that capability), \"git fetch\" relies upon the\n> > presence of refs/tags/* entries in the initial ref advertisement to\n> > locally create refs pointing to the aforementioned tag objects. When\n> > using protocol v2, refs/tags/* entries in the initial ref advertisement\n> > may be suppressed by a ref-prefix argument, leading to the tag object\n> > being downloaded, but the ref not being created.\n> \n> I wonder if it is reasonable to define the protocol v2 semantics of\n> \"include-tag\" request a bit differently.\n> \n> Unlike v0 protocol where the client iterates though the advertised\n> refs and see if objects at the tip of them, even if they weren't\n> what the client initially wanted to fetch, to find these unsolicited\n> followed tag objects, allow the server to say, \"I've included some\n> objects you did not explicitly ask for, and here are the refs/tags/*\n> names you should place on them\", somewhat similar to the way how the\n> ref-in-want thing would work (i.e. the client does not really ask\n> for just objects and decides what name to place on them---instead it\n> lets the server to make part of the decision).\n> \n> Wouldn't that allow us not having to advertise the whole tags\n> namespace only to implement the tag following?\n\nYes, it would, but as far as I can tell, it would add an extra burden on\nthe server to walk all refs requested in the ls-refs call (in order to\ndetermine which tags to send back in the response). Also, this walk must\nbe done before any negotiation (since this is a ls-refs call), so the\nwalk is done all the way to the commits without any parents (and so an\noverestimate of tags might be sent).\n\nIt might be better to have this functionality with ref-in-want, not only\nbecause of its conceptual similarity, but because ref-in-want provides a\nway for us to send refs at packfile generation time. So it is more\nefficient (since the server has to iterate through all the refs anyway\nto include objects for include-tag, it can probably just remember which\nrefs caused objects to be added and return them), and also, the list of\ntags will not include any tags that point to non-sent objects.\n\nSo my current inclination is to let v2 \"include-tag\" have the same\nsemantics as v0 \"include-tag\", and only provide automatic sending of the\nref itself when we have ref-in-want.\n"},{"id":"350427","messageId":"xmqqwouvwzsz.fsf@gitster-ct.c.googlers.com","threadId":"48662","inReplyTo":"20180618210731.152140-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] fetch: send \"refs/tags/\" prefix upon CLI refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-18T21:27:24Z","receivedAt":"2018-06-18T21:27:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> Wouldn't that allow us not having to advertise the whole tags\n>> namespace only to implement the tag following?\n>\n> Yes, it would, but as far as I can tell, it would add an extra burden on\n> the server to walk all refs requested in the ls-refs call (in order to\n> determine which tags to send back in the response). Also, this walk must\n> be done before any negotiation (since this is a ls-refs call),...\n\nMy comment was that I doubt the \"must be done\" part of the above.\nHow would refs-in-want be responded where client-supplied \"I want\n'master' branch---I am not asking for the exact object the first\nserver I contacted said where the 'master' is at\" gets turned into\n\"So the final value of 'master' among these servers that are not\nquite in sync is this\" by the one that gives you the pack, not\nnecessarily the one that responds to ls-refs upon initial contact?\nCan't we do something similar, i.e. let the client say \"I want tags\nthat refer to new objects you are going to send me, I do not know\nwhat they are offhand\" and the server that actually gives you the\npack to say \"here are the tags I ended up including\"? The\n\"include-tag\" process to generate pack with extra objects (i.e. the\ntags that point at packed objects) has to involve walking for\nreachabliity anyway, so as long as the feature is supported,\nsomebody has to do the work, and if you want to cut down the\ntransfer cost of the refs/tags/* enumeration, it needs to happen on\nthe server end, no?\n\nOr perhaps v2 fetch should implement the automated tag following\nwithout using include-tag and instead as an extended feature of\nref-in-want. I think that is merely giving a different name to the\nsame idea outlined above, though ;-)\n"},{"id":"350434","messageId":"20180618231642.174650-1-jonathantanmy@google.com","threadId":"48662","inReplyTo":"xmqqwouvwzsz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/2] fetch: send \"refs/tags/\" prefix upon CLI refspecs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-18T23:16:42Z","receivedAt":"2018-06-18T23:16:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> >> Wouldn't that allow us not having to advertise the whole tags\n> >> namespace only to implement the tag following?\n> >\n> > Yes, it would, but as far as I can tell, it would add an extra burden on\n> > the server to walk all refs requested in the ls-refs call (in order to\n> > determine which tags to send back in the response). Also, this walk must\n> > be done before any negotiation (since this is a ls-refs call),...\n> \n> My comment was that I doubt the \"must be done\" part of the above.\n> How would refs-in-want be responded where client-supplied \"I want\n> 'master' branch---I am not asking for the exact object the first\n> server I contacted said where the 'master' is at\" gets turned into\n> \"So the final value of 'master' among these servers that are not\n> quite in sync is this\" by the one that gives you the pack, not\n> necessarily the one that responds to ls-refs upon initial contact?\n> Can't we do something similar, i.e. let the client say \"I want tags\n> that refer to new objects you are going to send me, I do not know\n> what they are offhand\" and the server that actually gives you the\n> pack to say \"here are the tags I ended up including\"? The\n> \"include-tag\" process to generate pack with extra objects (i.e. the\n> tags that point at packed objects) has to involve walking for\n> reachabliity anyway, so as long as the feature is supported,\n> somebody has to do the work, and if you want to cut down the\n> transfer cost of the refs/tags/* enumeration, it needs to happen on\n> the server end, no?\n\nAh, I think I see. There are these possible worlds:\n (1) the current world\n (2) no ref-in-want, and upload-pack sends tag information as part of\n its response to ls-refs\n (3) no ref-in-want, but upload-pack can send ref information right\n before the packfile\n (4) ref-in-want, and upload-pack will send ref information right before\n the packfile\n\nI was only thinking about (2) and (4), but I think you are talking about\n(3). Yes, that would work, although I don't think it's worth the\nprotocol churn to do (3) then (4), especially since we already have\nref-in-want patches sent to the mailing list - but I should have\ndiscussed this option in my previous e-mails too.\n\n> Or perhaps v2 fetch should implement the automated tag following\n> without using include-tag and instead as an extended feature of\n> ref-in-want. I think that is merely giving a different name to the\n> same idea outlined above, though ;-)\n\nInstead of not using include-tag, I would define include-tag in\nthe presence of want-refs to also include the refs, but I agree with\nthis solution.\n"},{"id":"351966","messageId":"20180709173813.GA14196@aiede.svl.corp.google.com","threadId":"48662","inReplyTo":"c6910161aab1f383b5721bdc91969baad8c10a66.1528234587.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] fetch: send \"refs/tags/\" prefix upon CLI refspecs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-07-09T17:38:13Z","receivedAt":"2018-07-09T17:38:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,\n> \t\trefspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);\n> \n> \tif (ref_prefixes.argc &&\n> -\t (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {\n> +\t (tags == TAGS_SET || tags == TAGS_DEFAULT)) {\n\nMakes a lot of sense.\n\nThis means that if I run\n\n\tgit fetch origin master\n\nthen the ls-refs step will now include refs/tags/* in addition to\nrefs/heads/master, erasing some of the gain that protocol v2 brought.\nBut since the user didn't pass --no-tags, that is what the user asked\nfor.\n\nOne could argue that the user didn't *mean* to ask for that. In other\nwords, I wonder if the following would better match the user intent:\n\n - introduce a new tagopt value, --tags=auto or something, that means\n that tags are only followed when no refspec is supplied. In other\n words,\n\n\tgit fetch --tags=auto \n\n would perform tag auto-following, while\n\n \tgit fetch --tags=auto \n\n would act like fetch --no-tags.\n\n - make --tags=auto the default for new clones.\n\nWhat do you think?\n\nSome related thoughts on tag auto-following:\n\nIt would be tempting to not use tag auto-following at all in the\ndefault configuration and just include refs/tags/*:refs/tags/* in the\ndefault clone refspec. Unfortunately, that would be a pretty\nsignificant behavior change from the present behavior, since\n\n - it would fetch tags pointing to objects unrelated to the fetched\n history\n - if we have ref-in-want *with* pattern support, it would mean we\n try to fetch all tags on every fetch. As discussed in the\n thread [1], this is expensive and difficult to get right.\n - if an unannotated tag is fast-forwarded, it would allow existing\n tags to be updated\n - it interacts poorly with --prune\n - ...\n\nI actually suspect it's a good direction in the long term, but until\nwe address those downsides (see also the occasional discussions on tag\nnamespaces), we're not ready for it. The --tags=auto described above\nis a sort of half approximation.\n\nAnyway, this is all a long way of saying that despite the performance\nregression I believe this patch heads in the right direction. The\nperformance regression is inevitable in what the user is\n(unintentionally) requesting, and we can address it separately by\nchanging the defaults to better match what the user intends to\nrequest.\n\nThanks,\nJonathan\n\n[1] https://public-inbox.org/git/20180605175144.4225-1-bmwill@google.com/\n"},{"id":"351970","messageId":"20180709175939.GC81741@google.com","threadId":"48662","inReplyTo":"20180709173813.GA14196@aiede.svl.corp.google.com","subject":"Re: [PATCH v2 2/2] fetch: send \"refs/tags/\" prefix upon CLI refspecs","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-07-09T17:59:39Z","receivedAt":"2018-07-09T17:59:45Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/09, Jonathan Nieder wrote:\n> Hi,\n> \n> Jonathan Tan wrote:\n> \n> > --- a/builtin/fetch.c\n> > +++ b/builtin/fetch.c\n> > @@ -359,7 +359,7 @@ static struct ref *get_ref_map(struct transport *transport,\n> > \t\trefspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);\n> > \n> > \tif (ref_prefixes.argc &&\n> > -\t (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {\n> > +\t (tags == TAGS_SET || tags == TAGS_DEFAULT)) {\n> \n> Makes a lot of sense.\n> \n> This means that if I run\n> \n> \tgit fetch origin master\n> \n> then the ls-refs step will now include refs/tags/* in addition to\n> refs/heads/master, erasing some of the gain that protocol v2 brought.\n> But since the user didn't pass --no-tags, that is what the user asked\n> for.\n> \n> One could argue that the user didn't *mean* to ask for that. In other\n> words, I wonder if the following would better match the user intent:\n> \n> - introduce a new tagopt value, --tags=auto or something, that means\n> that tags are only followed when no refspec is supplied. In other\n> words,\n> \n> \tgit fetch --tags=auto \n> \n> would perform tag auto-following, while\n> \n> \tgit fetch --tags=auto \n> \n> would act like fetch --no-tags.\n> \n> - make --tags=auto the default for new clones.\n> \n> What do you think?\n> \n> Some related thoughts on tag auto-following:\n> \n> It would be tempting to not use tag auto-following at all in the\n> default configuration and just include refs/tags/*:refs/tags/* in the\n> default clone refspec. Unfortunately, that would be a pretty\n> significant behavior change from the present behavior, since\n> \n> - it would fetch tags pointing to objects unrelated to the fetched\n> history\n> - if we have ref-in-want *with* pattern support, it would mean we\n> try to fetch all tags on every fetch. As discussed in the\n> thread [1], this is expensive and difficult to get right.\n> - if an unannotated tag is fast-forwarded, it would allow existing\n> tags to be updated\n> - it interacts poorly with --prune\n> - ...\n> \n> I actually suspect it's a good direction in the long term, but until\n> we address those downsides (see also the occasional discussions on tag\n> namespaces), we're not ready for it. The --tags=auto described above\n> is a sort of half approximation.\n> \n> Anyway, this is all a long way of saying that despite the performance\n> regression I believe this patch heads in the right direction. The\n> performance regression is inevitable in what the user is\n> (unintentionally) requesting, and we can address it separately by\n> changing the defaults to better match what the user intends to\n> request.\n\nI agree with this observation, though I'm a bit sad about it. I think\nthat having tag auto-following the default is a little confusing (and\nhurts perf[1] when using proto v2) but since thats the way its always been\nwe'll have to live with it for now. I think exploring changing the\ndefaults might be a good thing to do in the future. But for now we've\nhad enough people comment on this lacking functionality that we should\nfix it.\n\n[1] Thankfully it doesn't completely undo what protocol v2 did, as we\nstill are able to eliminate refs/changes or refs/pull or other various\nrefs which significantly add to the number of refs advertised during\nfetch.\n\n-- \nBrandon Williams\n"},{"id":"351976","messageId":"xmqq8t6ke07x.fsf@gitster-ct.c.googlers.com","threadId":"48662","inReplyTo":"20180709175939.GC81741@google.com","subject":"Re: [PATCH v2 2/2] fetch: send \"refs/tags/\" prefix upon CLI refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-09T18:24:34Z","receivedAt":"2018-07-09T18:24:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> I agree with this observation, though I'm a bit sad about it. I think\n> that having tag auto-following the default is a little confusing (and\n> hurts perf[1] when using proto v2) but since thats the way its always been\n> we'll have to live with it for now. I think exploring changing the\n> defaults might be a good thing to do in the future. But for now we've\n> had enough people comment on this lacking functionality that we should\n> fix it.\n>\n> [1] Thankfully it doesn't completely undo what protocol v2 did, as we\n> still are able to eliminate refs/changes or refs/pull or other various\n> refs which significantly add to the number of refs advertised during\n> fetch.\n\nI thought JTan's <20180618231642.174650-1-jonathantanmy@google.com>\nshowed us a way forward to reduce the overhead even further without\nhaving to be sad ;-). Am I mistaken?\n\n\n\n"},{"id":"351977","messageId":"20180709183328.GD81741@google.com","threadId":"48662","inReplyTo":"xmqq8t6ke07x.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/2] fetch: send \"refs/tags/\" prefix upon CLI refspecs","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-07-09T18:33:28Z","receivedAt":"2018-07-09T18:33:35Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/09, Junio C Hamano wrote:\n> Brandon Williams writes:\n> \n> > I agree with this observation, though I'm a bit sad about it. I think\n> > that having tag auto-following the default is a little confusing (and\n> > hurts perf[1] when using proto v2) but since thats the way its always been\n> > we'll have to live with it for now. I think exploring changing the\n> > defaults might be a good thing to do in the future. But for now we've\n> > had enough people comment on this lacking functionality that we should\n> > fix it.\n> >\n> > [1] Thankfully it doesn't completely undo what protocol v2 did, as we\n> > still are able to eliminate refs/changes or refs/pull or other various\n> > refs which significantly add to the number of refs advertised during\n> > fetch.\n> \n> I thought JTan's <20180618231642.174650-1-jonathantanmy@google.com>\n> showed us a way forward to reduce the overhead even further without\n> having to be sad ;-). Am I mistaken?\n\nThat's true, what Jonathan mentioned there would avoid having to send\n\"refs/tags/*\" when requesting the refs. The question is do we wait on\nimplementing that functionality (as another feature to fetch) or do we\nfix this now?\n\n-- \nBrandon Williams\n"},{"id":"352013","messageId":"xmqqd0vwcfkr.fsf@gitster-ct.c.googlers.com","threadId":"48662","inReplyTo":"20180709183328.GD81741@google.com","subject":"Re: [PATCH v2 2/2] fetch: send \"refs/tags/\" prefix upon CLI refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-09T20:35:48Z","receivedAt":"2018-07-09T20:35:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> On 07/09, Junio C Hamano wrote:\n>> Brandon Williams writes:\n>> \n>> > I agree with this observation, though I'm a bit sad about it. I think\n>> > that having tag auto-following the default is a little confusing (and\n>> > hurts perf[1] when using proto v2) but since thats the way its always been\n>> > we'll have to live with it for now. I think exploring changing the\n>> > defaults might be a good thing to do in the future. But for now we've\n>> > had enough people comment on this lacking functionality that we should\n>> > fix it.\n>> >\n>> > [1] Thankfully it doesn't completely undo what protocol v2 did, as we\n>> > still are able to eliminate refs/changes or refs/pull or other various\n>> > refs which significantly add to the number of refs advertised during\n>> > fetch.\n>> \n>> I thought JTan's <20180618231642.174650-1-jonathantanmy@google.com>\n>> showed us a way forward to reduce the overhead even further without\n>> having to be sad ;-). Am I mistaken?\n>\n> That's true, what Jonathan mentioned there would avoid having to send\n> \"refs/tags/*\" when requesting the refs. The question is do we wait on\n> implementing that functionality (as another feature to fetch) or do we\n> fix this now?\n\nIt's not like the earlier v2 protocol used to be super efficient and\ncorrect, whose performance benefit is destroyed with this \"fix\"\nirreversibly. It was a fast but sometimes incorrect implementation,\nand we'd protect users of the still-under-development feature with\nthese two patches while updating the protocol further in order to\nbecome truly efficient and correct, so I do not see it a wrong move\nto take a hit like this patch does in the meantime.\n\nWhat I would see a wrong move would be to leave it very long as-is\nafter we apply this fix, and declare to flip the default not to\nfollow tags, using the performance hit as an excuse.\n\nSo I do not care too deeply either way, whether we wait to regain\nefficiency or taking this safe fix as the first step, as long as it\nis fixed in the longer term. I had an impression that the sentiment\nin the thread was that it was OK to accept the inefficiency for now\nand fix it later, and I am personally fine with that approach.\n"}]}