[BUG?] Remote helper's wrong assumption of unchanged ref

[BUG?] Remote helper's wrong assumption of unchanged ref

[Jonathan Tan]

Commit f8ec916 ("Allow helpers to report in "list" command that the ref
is unchanged", 2009-11-17) allowed a remote helper to report that a ref
is unchanged even if it did not know the contents of a ref. However,
that commit also made Git assume that such a remote ref has the contents
of the local ref of the same name. If I'm not missing anything, this
assumption seems wrong; the attached test illustrates one case of local
edits being made after cloning with default parameters.

The original e-mail thread [1] seems to indicate that this feature is
meant for a remote helper with no Git-specific code (which is possible
if it supports "import" but not "fetch" - in this case, it would not
deal with SHA-1s at all) to nevertheless indicate "unchanged", most
likely to support optimizations on the client side.

But it seems to me that Git cannot perform this optimization. In other
words, it should just ignore "unchanged". If this makes sense, I'll
prepare a patch to do this.

[1] "[PATCH 00/13] Native and foreign helpers"
    <alpine.LNX.2.00.0908050052390.2147@iabervon.org>
---
 git-remote-testgit.sh     |  8 +++++++-
 t/t5801-remote-helpers.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/git-remote-testgit.sh b/git-remote-testgit.sh
index 752c763..6357868 100755
--- a/git-remote-testgit.sh
+++ b/git-remote-testgit.sh
@@ -52,7 +52,13 @@ do
 		echo
 		;;
 	list)
-		git for-each-ref --format='? %(refname)' 'refs/heads/'
+		if test -n "$GIT_REMOTE_TESTGIT_UNCHANGED_BRANCH_REGEX"
+		then
+			git for-each-ref --format='? %(refname)' 'refs/heads/' |
+				sed "/${GIT_REMOTE_TESTGIT_UNCHANGED_BRANCH_REGEX}/s/$/ unchanged/"
+		else
+			git for-each-ref --format='? %(refname)' 'refs/heads/'
+		fi
 		head=$(git symbolic-ref HEAD)
 		echo "@$head HEAD"
 		echo
diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 362b158..4a48f2b 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -301,4 +301,44 @@ test_expect_success 'fetch url' '
 	compare_refs server HEAD local FETCH_HEAD
 '
 
+test_expect_success 'setup remote repository and divergent clone' '
+	git init s2 &&
+	(
+		cd s2 &&
+		test_commit M1 &&
+		git checkout -b mybranch &&
+		test_commit B1
+	) &&
+	git clone "testgit::${PWD}/s2" divergent &&
+
+	(
+		cd divergent &&
+		git checkout master &&
+		test_commit M2 &&
+		git checkout mybranch &&
+		test_commit B2
+	)
+'
+
+test_expect_success 'fetch with unchanged claims' '
+	rm -rf local &&
+	cp -r divergent local &&
+
+	# No unchanged branches
+
+	GIT_REMOTE_TESTGIT_UNCHANGED_BRANCH_REGEX=ABCDE git -C local fetch &&
+	compare_refs s2 M1 local refs/remotes/origin/master &&
+	compare_refs s2 B1 local refs/remotes/origin/mybranch &&
+
+	# One unchanged branch
+
+	GIT_REMOTE_TESTGIT_UNCHANGED_BRANCH_REGEX=mybranch git -C local fetch &&
+	compare_refs s2 M1 local refs/remotes/origin/master &&
+
+	# I (Jonathan Tan) would expect refs/remotes/origin/mybranch to be B1,
+	# but it is B2.
+	test_must_fail compare_refs s2 B1 local refs/remotes/origin/mybranch &&
+	compare_refs local B2 local refs/remotes/origin/mybranch
+'
+
 test_done
-- 
2.8.0.rc3.226.g39d4020

[PATCH] transport-helper: drop broken "unchanged" feature

[Jonathan Tan]

Commit f8ec916 ("Allow helpers to report in "list" command that the ref
is unchanged", 2009-11-17) allowed a remote helper to report that a ref
is unchanged even if it did not know the contents of a ref. However,
that commit also made Git wrongly assume that such a remote ref always
has the contents of the local ref of the same name.

(Git also cannot assume that the remote ref has the value of the current
destination local ref, or any other ref, since the previous import/fetch
could have been made using a different refspec.)

Drop that assumption.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---

Here is a patch that would remove the assumption (if it is indeed
wrong).

 Documentation/gitremote-helpers.txt |  3 +++
 transport-helper.c                  | 43 -------------------------------------
 2 files changed, 3 insertions(+), 43 deletions(-)

diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index 9e8681f..c862339 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -393,6 +393,9 @@ attributes are defined.
 'unchanged'::
 	This ref is unchanged since the last import or fetch, although
 	the helper cannot necessarily determine what value that produced.
+	Git may import or fetch this ref anyway, because it does not
+	keep a record of the last values corresponding to the refs of a
+	specific remote.
 
 OPTIONS
 -------
diff --git a/transport-helper.c b/transport-helper.c
index 91aed35..6ab8e2f 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -392,9 +392,6 @@ static int fetch_with_fetch(struct transport *transport,
 
 	for (i = 0; i < nr_heads; i++) {
 		const struct ref *posn = to_fetch[i];
-		if (posn->status & REF_STATUS_UPTODATE)
-			continue;
-
 		strbuf_addf(&buf, "fetch %s %s\n",
 			    oid_to_hex(&posn->old_oid),
 			    posn->symref ? posn->symref : posn->name);
@@ -492,9 +489,6 @@ static int fetch_with_import(struct transport *transport,
 
 	for (i = 0; i < nr_heads; i++) {
 		posn = to_fetch[i];
-		if (posn->status & REF_STATUS_UPTODATE)
-			continue;
-
 		strbuf_addf(&buf, "import %s\n",
 			    posn->symref ? posn->symref : posn->name);
 		sendline(data, &buf);
@@ -531,8 +525,6 @@ static int fetch_with_import(struct transport *transport,
 	for (i = 0; i < nr_heads; i++) {
 		char *private, *name;
 		posn = to_fetch[i];
-		if (posn->status & REF_STATUS_UPTODATE)
-			continue;
 		name = posn->symref ? posn->symref : posn->name;
 		if (data->refspecs)
 			private = apply_refspecs(data->refspecs, data->refspec_nr, name);
@@ -649,21 +641,12 @@ static int fetch(struct transport *transport,
 		 int nr_heads, struct ref **to_fetch)
 {
 	struct helper_data *data = transport->data;
-	int i, count;
 
 	if (process_connect(transport, 0)) {
 		do_take_over(transport);
 		return transport->fetch(transport, nr_heads, to_fetch);
 	}
 
-	count = 0;
-	for (i = 0; i < nr_heads; i++)
-		if (!(to_fetch[i]->status & REF_STATUS_UPTODATE))
-			count++;
-
-	if (!count)
-		return 0;
-
 	if (data->check_connectivity &&
 	    data->transport_options.check_self_contained_and_connected)
 		set_helper_option(transport, "check-connectivity", "true");
@@ -1009,23 +992,6 @@ static int push_refs(struct transport *transport,
 	return -1;
 }
 
-
-static int has_attribute(const char *attrs, const char *attr) {
-	int len;
-	if (!attrs)
-		return 0;
-
-	len = strlen(attr);
-	for (;;) {
-		const char *space = strchrnul(attrs, ' ');
-		if (len == space - attrs && !strncmp(attrs, attr, len))
-			return 1;
-		if (!*space)
-			return 0;
-		attrs = space + 1;
-	}
-}
-
 static struct ref *get_refs_list(struct transport *transport, int for_push)
 {
 	struct helper_data *data = transport->data;
@@ -1067,15 +1033,6 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 			(*tail)->symref = xstrdup(buf.buf + 1);
 		else if (buf.buf[0] != '?')
 			get_oid_hex(buf.buf, &(*tail)->old_oid);
-		if (eon) {
-			if (has_attribute(eon + 1, "unchanged")) {
-				(*tail)->status |= REF_STATUS_UPTODATE;
-				if (read_ref((*tail)->name,
-					     (*tail)->old_oid.hash) < 0)
-					die(_("Could not read ref %s"),
-					    (*tail)->name);
-			}
-		}
 		tail = &((*tail)->next);
 	}
 	if (debug)
-- 
2.8.0.rc3.226.g39d4020