Re: [PATCH] send-pack: Filter unknown commits from alternates of the remote

Björn Steinbrink [off-list ref] writes:

Actually I changed my mind.

We have the exactly the same issue for the real refs the target repository
has, not just borrowed phantom refs, in the code from day one of git-push.
In other words, this issue predates the ".have" extension, and your update
is in line with how the codepath for the real refs does its thing.  So
your fix is not worse than the existing code.

It can be argued that at least in the "real ref" case you are in control
of both ends and if you have a disconnected chain in your local repository
that you do not have a ref for, you are screwing yourself, and it is your
problem.  But when you forked your repository from somebody else on a
hosting site like github, you do not have much control over the other end
(because it is a closed site you cannot ssh in to diagnose what is really
going on), and if you do not exactly know from whom your hosted repository
is borrowing, it is more likely that you will get into a situation where
you may have objects near the tip without having the full chain after an
aborted transfer, and the insufficient check of doing only has_sha1_file()
may become a larger issue in such a settings.

But still, let's take the approach I labeled as *wrong* as an interim
solution for the immediate future.

I'd prefer a small helper function to consolidate the duplicated code,
like the attached patch, though.  How about doing it like this?

 builtin-send-pack.c |   46 ++++++++++++++++++++++++----------------------
 1 files changed, 24 insertions(+), 22 deletions(-)

diff --git c/builtin-send-pack.c w/builtin-send-pack.c
index a9fdbf9..2d24cf2 100644
--- c/builtin-send-pack.c
+++ w/builtin-send-pack.c

@@ -15,6 +15,23 @@ static struct send_pack_args args = {
 	/* .receivepack = */ "git-receive-pack",
 };
 
+static int feed_object(const unsigned char *theirs, int fd, int negative)
+{
+	char buf[42];
+
+	if (!has_sha1_file(theirs))
+		return 1;
+	/*
+	 * NEEDSWORK: we should not be satisfied by simply having
+	 * theirs, but should be making sure it is reachable from
+	 * some of our refs.
+	 */
+	memcpy(buf + negative, sha1_to_hex(theirs), 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 +52,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 +67,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;
 	}
 

.

On 2009.01.27 19:33:11 -0800, Junio C Hamano wrote:

It can be argued that at least in the "real ref" case you are in control
of both ends and if you have a disconnected chain in your local repository
that you do not have a ref for, you are screwing yourself, and it is your
problem.  But when you forked your repository from somebody else on a
hosting site like github, you do not have much control over the other end
(because it is a closed site you cannot ssh in to diagnose what is really
going on), and if you do not exactly know from whom your hosted repository
is borrowing, it is more likely that you will get into a situation where
you may have objects near the tip without having the full chain after an
aborted transfer, and the insufficient check of doing only has_sha1_file()
may become a larger issue in such a settings.

Uhm, it might be obvious, but what exactly could go wrong? Do we need to
fetch from multiple repos when alternates are involved? Or how would we
end up with a broken chain? I mean, it starts to make some sense to me
why we would need the connectivity check, but how do we end up with a
"partial" fetch at all?

I'd prefer a small helper function to consolidate the duplicated code,
like the attached patch, though.  How about doing it like this?

Yeah, that looks a lot nicer :-)

Björn