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

From: Jeff King <peff@peff.net>
Date: May 12, 2017, 07:59 UTC
Message-ID: <20170512075931.umunxd72nj53snds@sigill.intra.peff.net>
In-Reply-To: <xmqq37ca7gw0.fsf@gitster.mtv.corp.google.com>

On Fri, May 12, 2017 at 03:01:35PM +0900, Junio C Hamano wrote:

Show 25 quoted lines

> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> >> +static void add_refs_to_oidset(struct oidset *oids, struct ref *refs)
> >> +{
> >> +	for (; refs; refs = refs->next)
> >> +		oidset_insert(oids, &refs->old_oid);
> >> +}
> >> +
> >> +static int tip_oids_contain(struct oidset *tip_oids,
> >> +			    struct ref *unmatched, struct ref *newlist,
> >> +			    const struct object_id *id)
> >> +{
> >> +	if (!tip_oids->map.cmpfn) {
> >
> > This feels like a layering violation.  Could it be e.g. a static inline
> > function oidset_is_initialized in oidset.h?
> 
> Surely it does.
> 
> Also, tip_oids_contain() uses unmatched and newlist only on the
> first call, but the internal API this patch establishes gives an
> illusion (confusion) that updating unmatched and newlist while
> making repeated to calls to this function may affect the outcome of
> tip_oids_contain() function.  In fact, doesn't the loop that calls
> this function extend "newlist" by extending the list at its tail?

It does, but only with elements whose oids were already in the set. So I don't think it's wrong, but I agree the interface makes it a bit muddy.

The whole thing is much easier to follow if you just create the oidset before chopping it up into two lists. But that loses the "lazy" property, and you pay for the oidset even if there are no raw sha1s requested. But it is nice and short. See the patch below (as a replacement for what we've been discussing, not on top).

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.

---

diff --git a/fetch-pack.c b/fetch-pack.c
index afb8b0502..4f3e8fa6b 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;
@@ -599,6 +600,9 @@ static void filter_refs(struct fetch_pack_args *args,
 	struct ref *newlist = NULL;
 	struct ref **newtail = &newlist;
 	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 i;
 
 	i = 0;
@@ -626,6 +630,9 @@ static void filter_refs(struct fetch_pack_args *args,
 		    (!args->deepen || !starts_with(ref->name, "refs/tags/")))
 			keep = 1;
 
+		if (!send_raw_oids)
+			oidset_insert(&tip_oids, &ref->old_oid);
+
 		if (keep) {
 			*newtail = ref;
 			ref->next = NULL;
@@ -647,8 +654,7 @@ static void filter_refs(struct fetch_pack_args *args,
 		    hashcmp(sha1, ref->old_oid.hash))
 			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;
@@ -657,6 +663,8 @@ static void filter_refs(struct fetch_pack_args *args,
 		}
 	}
 	*refs = newlist;
+
+	oidset_clear(&tip_oids);
 }
 
 static void mark_alternate_complete(struct object *obj)