{"thread":{"id":"44579","subject":"[BUG?] Remote helper's wrong assumption of unchanged ref","startedAt":"2016-12-01T00:54:41Z","lastAt":"2016-12-01T01:14:19Z","messageCount":2,"participants":["Jonathan Tan"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"306653","messageId":"1480553664-12804-1-git-send-email-jonathantanmy@google.com","threadId":"44579","inReplyTo":null,"subject":"[BUG?] Remote helper's wrong assumption of unchanged ref","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-12-01T00:54:24Z","receivedAt":"2016-12-01T00:54:41Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit f8ec916 (\"Allow helpers to report in \"list\" command that the ref\nis unchanged\", 2009-11-17) allowed a remote helper to report that a ref\nis unchanged even if it did not know the contents of a ref. However,\nthat commit also made Git assume that such a remote ref has the contents\nof the local ref of the same name. If I'm not missing anything, this\nassumption seems wrong; the attached test illustrates one case of local\nedits being made after cloning with default parameters.\n\nThe original e-mail thread [1] seems to indicate that this feature is\nmeant for a remote helper with no Git-specific code (which is possible\nif it supports \"import\" but not \"fetch\" - in this case, it would not\ndeal with SHA-1s at all) to nevertheless indicate \"unchanged\", most\nlikely to support optimizations on the client side.\n\nBut it seems to me that Git cannot perform this optimization. In other\nwords, it should just ignore \"unchanged\". If this makes sense, I'll\nprepare a patch to do this.\n\n[1] \"[PATCH 00/13] Native and foreign helpers\"\n    <alpine.LNX.2.00.0908050052390.2147@iabervon.org>\n---\n git-remote-testgit.sh     |  8 +++++++-\n t/t5801-remote-helpers.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 47 insertions(+), 1 deletion(-)\n\ndiff --git a/git-remote-testgit.sh b/git-remote-testgit.sh\nindex 752c763..6357868 100755\n--- a/git-remote-testgit.sh\n+++ b/git-remote-testgit.sh\n@@ -52,7 +52,13 @@ do\n \t\techo\n \t\t;;\n \tlist)\n-\t\tgit for-each-ref --format='? %(refname)' 'refs/heads/'\n+\t\tif test -n \"$GIT_REMOTE_TESTGIT_UNCHANGED_BRANCH_REGEX\"\n+\t\tthen\n+\t\t\tgit for-each-ref --format='? %(refname)' 'refs/heads/' |\n+\t\t\t\tsed \"/${GIT_REMOTE_TESTGIT_UNCHANGED_BRANCH_REGEX}/s/$/ unchanged/\"\n+\t\telse\n+\t\t\tgit for-each-ref --format='? %(refname)' 'refs/heads/'\n+\t\tfi\n \t\thead=$(git symbolic-ref HEAD)\n \t\techo \"@$head HEAD\"\n \t\techo\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 362b158..4a48f2b 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -301,4 +301,44 @@ test_expect_success 'fetch url' '\n \tcompare_refs server HEAD local FETCH_HEAD\n '\n \n+test_expect_success 'setup remote repository and divergent clone' '\n+\tgit init s2 &&\n+\t(\n+\t\tcd s2 &&\n+\t\ttest_commit M1 &&\n+\t\tgit checkout -b mybranch &&\n+\t\ttest_commit B1\n+\t) &&\n+\tgit clone \"testgit::${PWD}/s2\" divergent &&\n+\n+\t(\n+\t\tcd divergent &&\n+\t\tgit checkout master &&\n+\t\ttest_commit M2 &&\n+\t\tgit checkout mybranch &&\n+\t\ttest_commit B2\n+\t)\n+'\n+\n+test_expect_success 'fetch with unchanged claims' '\n+\trm -rf local &&\n+\tcp -r divergent local &&\n+\n+\t# No unchanged branches\n+\n+\tGIT_REMOTE_TESTGIT_UNCHANGED_BRANCH_REGEX=ABCDE git -C local fetch &&\n+\tcompare_refs s2 M1 local refs/remotes/origin/master &&\n+\tcompare_refs s2 B1 local refs/remotes/origin/mybranch &&\n+\n+\t# One unchanged branch\n+\n+\tGIT_REMOTE_TESTGIT_UNCHANGED_BRANCH_REGEX=mybranch git -C local fetch &&\n+\tcompare_refs s2 M1 local refs/remotes/origin/master &&\n+\n+\t# I (Jonathan Tan) would expect refs/remotes/origin/mybranch to be B1,\n+\t# but it is B2.\n+\ttest_must_fail compare_refs s2 B1 local refs/remotes/origin/mybranch &&\n+\tcompare_refs local B2 local refs/remotes/origin/mybranch\n+'\n+\n test_done\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"306654","messageId":"1480554814-28596-1-git-send-email-jonathantanmy@google.com","threadId":"44579","inReplyTo":"1480553664-12804-1-git-send-email-jonathantanmy@google.com","subject":"[PATCH] transport-helper: drop broken \"unchanged\" feature","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-12-01T01:13:34Z","receivedAt":"2016-12-01T01:14:19Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit f8ec916 (\"Allow helpers to report in \"list\" command that the ref\nis unchanged\", 2009-11-17) allowed a remote helper to report that a ref\nis unchanged even if it did not know the contents of a ref. However,\nthat commit also made Git wrongly assume that such a remote ref always\nhas the contents of the local ref of the same name.\n\n(Git also cannot assume that the remote ref has the value of the current\ndestination local ref, or any other ref, since the previous import/fetch\ncould have been made using a different refspec.)\n\nDrop that assumption.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n\nHere is a patch that would remove the assumption (if it is indeed\nwrong).\n\n Documentation/gitremote-helpers.txt |  3 +++\n transport-helper.c                  | 43 -------------------------------------\n 2 files changed, 3 insertions(+), 43 deletions(-)\n\ndiff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\nindex 9e8681f..c862339 100644\n--- a/Documentation/gitremote-helpers.txt\n+++ b/Documentation/gitremote-helpers.txt\n@@ -393,6 +393,9 @@ attributes are defined.\n 'unchanged'::\n \tThis ref is unchanged since the last import or fetch, although\n \tthe helper cannot necessarily determine what value that produced.\n+\tGit may import or fetch this ref anyway, because it does not\n+\tkeep a record of the last values corresponding to the refs of a\n+\tspecific remote.\n \n OPTIONS\n -------\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 91aed35..6ab8e2f 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -392,9 +392,6 @@ static int fetch_with_fetch(struct transport *transport,\n \n \tfor (i = 0; i < nr_heads; i++) {\n \t\tconst struct ref *posn = to_fetch[i];\n-\t\tif (posn->status & REF_STATUS_UPTODATE)\n-\t\t\tcontinue;\n-\n \t\tstrbuf_addf(&buf, \"fetch %s %s\\n\",\n \t\t\t    oid_to_hex(&posn->old_oid),\n \t\t\t    posn->symref ? posn->symref : posn->name);\n@@ -492,9 +489,6 @@ static int fetch_with_import(struct transport *transport,\n \n \tfor (i = 0; i < nr_heads; i++) {\n \t\tposn = to_fetch[i];\n-\t\tif (posn->status & REF_STATUS_UPTODATE)\n-\t\t\tcontinue;\n-\n \t\tstrbuf_addf(&buf, \"import %s\\n\",\n \t\t\t    posn->symref ? posn->symref : posn->name);\n \t\tsendline(data, &buf);\n@@ -531,8 +525,6 @@ static int fetch_with_import(struct transport *transport,\n \tfor (i = 0; i < nr_heads; i++) {\n \t\tchar *private, *name;\n \t\tposn = to_fetch[i];\n-\t\tif (posn->status & REF_STATUS_UPTODATE)\n-\t\t\tcontinue;\n \t\tname = posn->symref ? posn->symref : posn->name;\n \t\tif (data->refspecs)\n \t\t\tprivate = apply_refspecs(data->refspecs, data->refspec_nr, name);\n@@ -649,21 +641,12 @@ static int fetch(struct transport *transport,\n \t\t int nr_heads, struct ref **to_fetch)\n {\n \tstruct helper_data *data = transport->data;\n-\tint i, count;\n \n \tif (process_connect(transport, 0)) {\n \t\tdo_take_over(transport);\n \t\treturn transport->fetch(transport, nr_heads, to_fetch);\n \t}\n \n-\tcount = 0;\n-\tfor (i = 0; i < nr_heads; i++)\n-\t\tif (!(to_fetch[i]->status & REF_STATUS_UPTODATE))\n-\t\t\tcount++;\n-\n-\tif (!count)\n-\t\treturn 0;\n-\n \tif (data->check_connectivity &&\n \t    data->transport_options.check_self_contained_and_connected)\n \t\tset_helper_option(transport, \"check-connectivity\", \"true\");\n@@ -1009,23 +992,6 @@ static int push_refs(struct transport *transport,\n \treturn -1;\n }\n \n-\n-static int has_attribute(const char *attrs, const char *attr) {\n-\tint len;\n-\tif (!attrs)\n-\t\treturn 0;\n-\n-\tlen = strlen(attr);\n-\tfor (;;) {\n-\t\tconst char *space = strchrnul(attrs, ' ');\n-\t\tif (len == space - attrs && !strncmp(attrs, attr, len))\n-\t\t\treturn 1;\n-\t\tif (!*space)\n-\t\t\treturn 0;\n-\t\tattrs = space + 1;\n-\t}\n-}\n-\n static struct ref *get_refs_list(struct transport *transport, int for_push)\n {\n \tstruct helper_data *data = transport->data;\n@@ -1067,15 +1033,6 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)\n \t\t\t(*tail)->symref = xstrdup(buf.buf + 1);\n \t\telse if (buf.buf[0] != '?')\n \t\t\tget_oid_hex(buf.buf, &(*tail)->old_oid);\n-\t\tif (eon) {\n-\t\t\tif (has_attribute(eon + 1, \"unchanged\")) {\n-\t\t\t\t(*tail)->status |= REF_STATUS_UPTODATE;\n-\t\t\t\tif (read_ref((*tail)->name,\n-\t\t\t\t\t     (*tail)->old_oid.hash) < 0)\n-\t\t\t\t\tdie(_(\"Could not read ref %s\"),\n-\t\t\t\t\t    (*tail)->name);\n-\t\t\t}\n-\t\t}\n \t\ttail = &((*tail)->next);\n \t}\n \tif (debug)\n-- \n2.8.0.rc3.226.g39d4020\n\n"}]}