Merge branch 'jk/fetch-reachability-error-fix'

Code clean-up and a fix for "git fetch" by an explicit object name
(as opposed to fetching refs by name).

* jk/fetch-reachability-error-fix:
  fetch: do not consider peeled tags as advertised tips
  remote.c: make singular free_ref() public
  fetch: use free_refs()
  pkt-line: prepare buffer before handling ERR packets
  upload-pack: send ERR packet for non-tip objects
  t5530: check protocol response for "not our ref"
  t5516: drop ok=sigpipe from unreachable-want tests

7 files changed, 59 insertions, 25 deletions

diff --git a/fetch-pack.c b/fetch-pack.c
index 0f15877..3f24d0c 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -573,9 +573,14 @@ static void filter_refs(struct fetch_pack_args *args,
 		next = ref->next;
 
 		if (starts_with(ref->name, "refs/") &&
-		    check_refname_format(ref->name, 0))
-			; /* trash */
-		else {
+		    check_refname_format(ref->name, 0)) {
+			/*
+			 * trash or a peeled value; do not even add it to
+			 * unmatched list
+			 */
+			free_one_ref(ref);
+			continue;
+		} else {
 			while (i < nr_sought) {
 				int cmp = strcmp(ref->name, sought[i]->name);
 				if (cmp < 0)
@@ -630,10 +635,7 @@ static void filter_refs(struct fetch_pack_args *args,
 	}
 
 	oidset_clear(&tip_oids);
-	for (ref = unmatched; ref; ref = next) {
-		next = ref->next;
-		free(ref);
-	}
+	free_refs(unmatched);
 
 	*refs = newlist;
 }
diff --git a/pkt-line.c b/pkt-line.c
index ffd7220..c9ed780 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -350,16 +350,17 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 		return PACKET_READ_EOF;
 	}
 
-	if ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&
-	    starts_with(buffer, "ERR "))
-		die(_("remote error: %s"), buffer + 4);
-
 	if ((options & PACKET_READ_CHOMP_NEWLINE) &&
 	    len && buffer[len-1] == '\n')
 		len--;
 
 	buffer[len] = 0;
 	packet_trace(buffer, len, 0);
+
+	if ((options & PACKET_READ_DIE_ON_ERR_PACKET) &&
+	    starts_with(buffer, "ERR "))
+		die(_("remote error: %s"), buffer + 4);
+
 	*pktlen = len;
 	return PACKET_READ_NORMAL;
 }
diff --git a/remote.c b/remote.c
index 9cc3b07..3fe34ea 100644
--- a/remote.c
+++ b/remote.c
@@ -820,11 +820,11 @@ struct ref *copy_ref_list(const struct ref *ref)
 	return ret;
 }
 
-static void free_ref(struct ref *ref)
+void free_one_ref(struct ref *ref)
 {
 	if (!ref)
 		return;
-	free_ref(ref->peer_ref);
+	free_one_ref(ref->peer_ref);
 	free(ref->remote_status);
 	free(ref->symref);
 	free(ref);
@@ -835,7 +835,7 @@ void free_refs(struct ref *ref)
 	struct ref *next;
 	while (ref) {
 		next = ref->next;
-		free_ref(ref);
+		free_one_ref(ref);
 		ref = next;
 	}
 }
diff --git a/remote.h b/remote.h
index da53ad5..f58332a 100644
--- a/remote.h
+++ b/remote.h
@@ -131,8 +131,10 @@ int ref_compare_name(const void *, const void *);
 int check_ref_type(const struct ref *ref, int flags);
 
 /*
- * Frees the entire list and peers of elements.
+ * Free a single ref and its peer, or an entire list of refs and their peers,
+ * respectively.
  */
+void free_one_ref(struct ref *ref);
 void free_refs(struct ref *ref);
 
 struct oid_array;
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 49bf428..c81ca36 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1241,9 +1241,9 @@ do
 			cd shallow &&
 			# Some protocol versions (e.g. 2) support fetching
 			# unadvertised objects, so restrict this test to v0.
-			test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \
+			test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
 				git fetch ../testrepo/.git $SHA1_3 &&
-			test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \
+			test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
 				git fetch ../testrepo/.git $SHA1_1 &&
 			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
 			git fetch ../testrepo/.git $SHA1_1 &&
@@ -1251,8 +1251,9 @@ do
 			test_must_fail git cat-file commit $SHA1_2 &&
 			git fetch ../testrepo/.git $SHA1_2 &&
 			git cat-file commit $SHA1_2 &&
-			test_must_fail ok=sigpipe env GIT_TEST_PROTOCOL_VERSION= \
-				git fetch ../testrepo/.git $SHA1_3
+			test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+				git fetch ../testrepo/.git $SHA1_3 2>err &&
+			test_i18ngrep "remote error:.*not our ref.*$SHA1_3\$" err
 		)
 	'
 done
@@ -1284,6 +1285,17 @@ test_expect_success 'fetch follows tags by default' '
 	test_cmp expect actual
 '
 
+test_expect_success 'peeled advertisements are not considered ref tips' '
+	mk_empty testrepo &&
+	git -C testrepo commit --allow-empty -m one &&
+	git -C testrepo commit --allow-empty -m two &&
+	git -C testrepo tag -m foo mytag HEAD^ &&
+	oid=$(git -C testrepo rev-parse mytag^{commit}) &&
+	test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+		git fetch testrepo $oid 2>err &&
+	test_i18ngrep "Server does not allow request for unadvertised object" err
+'
+
 test_expect_success 'pushing a specific ref applies remote.$name.push as refmap' '
 	mk_test testrepo heads/master &&
 	rm -fr src dst &&
diff --git a/t/t5530-upload-pack-error.sh b/t/t5530-upload-pack-error.sh
index 4f6e32b..a1d3031 100755
--- a/t/t5530-upload-pack-error.sh
+++ b/t/t5530-upload-pack-error.sh
@@ -57,13 +57,25 @@ test_expect_success 'upload-pack fails due to error in rev-list' '
 	grep "bad tree object" output.err
 '
 
-test_expect_success 'upload-pack error message when bad ref requested' '
+test_expect_success 'upload-pack fails due to bad want (no object)' '
 
 	printf "0045want %s multi_ack_detailed\n00000009done\n0000" \
 		"deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" >input &&
 	test_must_fail git upload-pack . <input >output 2>output.err &&
-	grep -q "not our ref" output.err &&
-	! grep -q multi_ack_detailed output.err
+	grep "not our ref" output.err &&
+	grep "ERR" output &&
+	! grep multi_ack_detailed output.err
+'
+
+test_expect_success 'upload-pack fails due to bad want (not tip)' '
+
+	oid=$(echo an object we have | git hash-object -w --stdin) &&
+	printf "0045want %s multi_ack_detailed\n00000009done\n0000" \
+		"$oid" >input &&
+	test_must_fail git upload-pack . <input >output 2>output.err &&
+	grep "not our ref" output.err &&
+	grep "ERR" output &&
+	! grep multi_ack_detailed output.err
 '
 
 test_expect_success 'upload-pack fails due to error in pack-objects enumeration' '
diff --git a/upload-pack.c b/upload-pack.c
index d098ef5..cb603a6 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -592,7 +592,8 @@ error:
 	return 1;
 }
 
-static void check_non_tip(struct object_array *want_obj)
+static void check_non_tip(struct object_array *want_obj,
+			  struct packet_writer *writer)
 {
 	int i;
 
@@ -611,9 +612,13 @@ error:
 	/* Pick one of them (we know there at least is one) */
 	for (i = 0; i < want_obj->nr; i++) {
 		struct object *o = want_obj->objects[i].item;
-		if (!is_our_ref(o))
+		if (!is_our_ref(o)) {
+			packet_writer_error(writer,
+					    "upload-pack: not our ref %s",
+					    oid_to_hex(&o->oid));
 			die("git upload-pack: not our ref %s",
 			    oid_to_hex(&o->oid));
+		}
 	}
 }
 
@@ -936,7 +941,7 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
 	 * by another process that handled the initial request.
 	 */
 	if (has_non_tip)
-		check_non_tip(want_obj);
+		check_non_tip(want_obj, &writer);
 
 	if (!use_sideband && daemon_mode)
 		no_progress = 1;