Re: [PATCH v5] fetch-pack: always allow fetching of literal SHA1s

From: Jeff King <peff@peff.net>
Date: May 12, 2017, 08:14 UTC
Message-ID: <20170512081417.w537fmd4o5rl4kja@sigill.intra.peff.net>
In-Reply-To: <20170512075931.umunxd72nj53snds@sigill.intra.peff.net>

On Fri, May 12, 2017 at 03:59:31AM -0400, Jeff King wrote:

Show 15 quoted lines

> It's hard to resolve that because the loop that chops up the lists is
> also the loop that is figuring out if there are leftover raw-sha1
> names. But you could probably structure it like:
> 
>   1. Loop and see if we have unmatched names that look like sha1s.
> 
>   2. Set up the oidset from the two chopped-up lists if there are
>      sha1 candidates (and if we aren't going to just send them
>      regardless due to server options).
> 
>   3. Loop over the unmatched candidates and check them against the
>      oidset.
> 
> That avoids the lazy-init, so pulling (2) out into a function results in
> a better interface.

Here that is for reference. It's a bit more complex than the other solutions, requiring the unmatched list, the is_literal_sha1() helper, and the oidset. But it avoids any extra allocation when it isn't necessary, and drops the laziness.

I mostly did this to give the discussion something concrete to look at. I'm actually OK with any of the options so far, as long as it does not have any quadratic behavior. :)

diff --git a/fetch-pack.c b/fetch-pack.c
index afb8b0502..e167213c0 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -15,6 +15,7 @@
 #include "version.h"
 #include "prio-queue.h"
 #include "sha1-array.h"
+#include "oidset.h"
 
 static int transfer_unpack_limit = -1;
 static int fetch_unpack_limit = -1;
@@ -592,13 +593,27 @@ static void mark_recent_complete_commits(struct fetch_pack_args *args,
 	}
 }
 
+static int is_literal_sha1(const struct ref *ref)
+{
+	struct object_id oid;
+	const char *end;
+	return !parse_oid_hex(ref->name, &oid, &end) &&
+	       !*end &&
+	       !oidcmp(&oid, &ref->old_oid);
+}
+
 static void filter_refs(struct fetch_pack_args *args,
 			struct ref **refs,
 			struct ref **sought, int nr_sought)
 {
 	struct ref *newlist = NULL;
 	struct ref **newtail = &newlist;
+	struct ref *unmatched = NULL;
 	struct ref *ref, *next;
+	struct oidset tip_oids = OIDSET_INIT;
+	int send_raw_oids = (allow_unadvertised_object_request &
+			     (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));
+	int seeking_raw_oid = 0;
 	int i;
 
 	i = 0;
@@ -617,7 +632,8 @@ static void filter_refs(struct fetch_pack_args *args,
 				else if (cmp == 0) {
 					keep = 1; /* definitely have it */
 					sought[i]->match_status = REF_MATCHED;
-				}
+				} else if (is_literal_sha1(sought[i]))
+					seeking_raw_oid = 1;
 				i++;
 			}
 		}
@@ -631,24 +647,27 @@ static void filter_refs(struct fetch_pack_args *args,
 			ref->next = NULL;
 			newtail = &ref->next;
 		} else {
-			free(ref);
+			ref->next = unmatched;
+			unmatched = ref;
 		}
 	}
 
+	if (seeking_raw_oid && !send_raw_oids) {
+		for (ref = newlist; ref; ref = ref->next)
+			oidset_insert(&tip_oids, &ref->old_oid);
+		for (ref = unmatched; ref; ref = ref->next)
+			oidset_insert(&tip_oids, &ref->old_oid);
+	}
+
 	/* Append unmatched requests to the list */
 	for (i = 0; i < nr_sought; i++) {
-		unsigned char sha1[20];
-
 		ref = sought[i];
 		if (ref->match_status != REF_NOT_MATCHED)
 			continue;
-		if (get_sha1_hex(ref->name, sha1) ||
-		    ref->name[40] != '\0' ||
-		    hashcmp(sha1, ref->old_oid.hash))
+		if (!is_literal_sha1(ref))
 			continue;
 
-		if ((allow_unadvertised_object_request &
-		    (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
+		if (send_raw_oids || oidset_contains(&tip_oids, &ref->old_oid)) {
 			ref->match_status = REF_MATCHED;
 			*newtail = copy_ref(ref);
 			newtail = &(*newtail)->next;
@@ -656,6 +675,13 @@ static void filter_refs(struct fetch_pack_args *args,
 			ref->match_status = REF_UNADVERTISED_NOT_ALLOWED;
 		}
 	}
+
+	oidset_clear(&tip_oids);
+	for (ref = unmatched; ref; ref = next) {
+		next = ref->next;
+		free(ref);
+	}
+
 	*refs = newlist;
 }