send-pack: do not send unknown object name from ".have" to pack-objects

v1.6.1 introduced ".have" extension to the protocol to allow the receiving
side to advertise objects that are reachable from refs in the repositories
it borrows from.  This was meant to be used by the sending side to avoid
sending such objects; they are already available through the alternates
mechanism.

The client side implementation in v1.6.1, which was introduced with
40c155f (push: prepare sender to receive extended ref information from the
receiver, 2008-09-09) aka v1.6.1-rc1~203^2~1, were faulty in that it did
not consider the possiblity that the repository receiver borrows from
might have objects it does not know about.

This fixes it by refraining from passing missing commits to underlying
pack-objects.  Revision machinery may need to be tightened further to
treat missing uninteresting objects as non-error events, but this is an
obvious and safe fix for a maintenance release that is almost good enough.

Signed-off-by: Junio C Hamano <gitster@pobox.com>

2 files changed, 127 insertions, 22 deletions

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index a9fdbf9d45..d65d019692 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -15,6 +15,20 @@ static struct send_pack_args args = {
 	/* .receivepack = */ "git-receive-pack",
 };
 
+static int feed_object(const unsigned char *sha1, int fd, int negative)
+{
+	char buf[42];
+
+	if (negative && !has_sha1_file(sha1))
+		return 1;
+
+	memcpy(buf + negative, sha1_to_hex(sha1), 40);
+	if (negative)
+		buf[0] = '^';
+	buf[40 + negative] = '\n';
+	return write_or_whine(fd, buf, 41 + negative, "send-pack: send refs");
+}
+
 /*
  * Make a pack stream and spit it out into file descriptor fd
  */
@@ -35,7 +49,6 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 	};
 	struct child_process po;
 	int i;
-	char buf[42];
 
 	if (args.use_thin_pack)
 		argv[4] = "--thin";
@@ -51,31 +64,17 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 	 * We feed the pack-objects we just spawned with revision
 	 * parameters by writing to the pipe.
 	 */
-	for (i = 0; i < extra->nr; i++) {
-		memcpy(buf + 1, sha1_to_hex(&extra->array[i][0]), 40);
-		buf[0] = '^';
-		buf[41] = '\n';
-		if (!write_or_whine(po.in, buf, 42, "send-pack: send refs"))
+	for (i = 0; i < extra->nr; i++)
+		if (!feed_object(extra->array[i], po.in, 1))
 			break;
-	}
 
 	while (refs) {
 		if (!is_null_sha1(refs->old_sha1) &&
-		    has_sha1_file(refs->old_sha1)) {
-			memcpy(buf + 1, sha1_to_hex(refs->old_sha1), 40);
-			buf[0] = '^';
-			buf[41] = '\n';
-			if (!write_or_whine(po.in, buf, 42,
-						"send-pack: send refs"))
-				break;
-		}
-		if (!is_null_sha1(refs->new_sha1)) {
-			memcpy(buf, sha1_to_hex(refs->new_sha1), 40);
-			buf[40] = '\n';
-			if (!write_or_whine(po.in, buf, 41,
-						"send-pack: send refs"))
-				break;
-		}
+		    !feed_object(refs->old_sha1, po.in, 1))
+			break;
+		if (!is_null_sha1(refs->new_sha1) &&
+		    !feed_object(refs->new_sha1, po.in, 0))
+			break;
 		refs = refs->next;
 	}
 
diff --git a/t/t5519-push-alternates.sh b/t/t5519-push-alternates.sh
new file mode 100755
index 0000000000..6dfc55ad61
--- /dev/null
+++ b/t/t5519-push-alternates.sh
@@ -0,0 +1,106 @@
+#!/bin/sh
+
+test_description='push to a repository that borrows from elsewhere'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	mkdir alice-pub &&
+	(
+		cd alice-pub &&
+		GIT_DIR=. git init
+	) &&
+	mkdir alice-work &&
+	(
+		cd alice-work &&
+		git init &&
+		>file &&
+		git add . &&
+		git commit -m initial &&
+		git push ../alice-pub master
+	) &&
+
+	# Project Bob is a fork of project Alice
+	mkdir bob-pub &&
+	(
+		cd bob-pub &&
+		GIT_DIR=. git init &&
+		mkdir -p objects/info &&
+		echo ../../alice-pub/objects >objects/info/alternates
+	) &&
+	git clone alice-pub bob-work &&
+	(
+		cd bob-work &&
+		git push ../bob-pub master
+	)
+'
+
+test_expect_success 'alice works and pushes' '
+	(
+		cd alice-work &&
+		echo more >file &&
+		git commit -a -m second &&
+		git push ../alice-pub
+	)
+'
+
+test_expect_success 'bob fetches from alice, works and pushes' '
+	(
+		# Bob acquires what Alice did in his work tree first.
+		# Even though these objects are not directly in
+		# the public repository of Bob, this push does not
+		# need to send the commit Bob received from Alice
+		# to his public repository, as all the object Alice
+		# has at her public repository are available to it
+		# via its alternates.
+		cd bob-work &&
+		git pull ../alice-pub master &&
+		echo more bob >file &&
+		git commit -a -m third &&
+		git push ../bob-pub
+	) &&
+
+	# Check that the second commit by Alice is not sent
+	# to ../bob-pub
+	(
+		cd bob-pub &&
+		second=$(git rev-parse HEAD^) &&
+		rm -f objects/info/alternates &&
+		test_must_fail git cat-file -t $second &&
+		echo ../../alice-pub/objects >objects/info/alternates
+	)
+'
+
+test_expect_success 'clean-up in case the previous failed' '
+	(
+		cd bob-pub &&
+		echo ../../alice-pub/objects >objects/info/alternates
+	)
+'
+
+test_expect_success 'alice works and pushes again' '
+	(
+		# Alice does not care what Bob does.  She does not
+		# even have to be aware of his existence.  She just
+		# keeps working and pushing
+		cd alice-work &&
+		echo more alice >file &&
+		git commit -a -m fourth &&
+		git push ../alice-pub
+	)
+'
+
+test_expect_success 'bob works and pushes' '
+	(
+		# This time Bob does not pull from Alice, and
+		# the master branch at her public repository points
+		# at a commit Bob does not know about.  This should
+		# not prevent the push by Bob from succeeding.
+		cd bob-work &&
+		echo yet more bob >file &&
+		git commit -a -m fifth &&
+		git push ../bob-pub
+	)
+'
+
+test_done