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

From: Jeff King <peff@peff.net>
Date: May 9, 2017, 22:16 UTC
Message-ID: <20170509221629.3z35qcz36oiix3kh@sigill.intra.peff.net>
In-Reply-To: <20170509182042.28389-1-jonathantanmy@google.com>

On Tue, May 09, 2017 at 11:20:42AM -0700, Jonathan Tan wrote:

Show 8 quoted lines

> fetch-pack, when fetching a literal SHA-1 from a server that is not
> configured with uploadpack.allowtipsha1inwant (or similar), always
> returns an error message of the form "Server does not allow request for
> unadvertised object %s". However, it is sometimes the case that such
> object is advertised.
> 
> Teach fetch-pack to also check the SHA-1s of the refs in the received
> ref advertisement if a literal SHA-1 was given by the user.

Hmm. That makes sense generally, as the request should succeed. But it seems like we're creating a client that will sometimes succeed and sometimes fail, and the reasoning will be somewhat opaque to the user. I have a feeling I'm missing some context on when you'd expect this to kick in.

Show 7 quoted lines

> +static int is_literal_sha1(const struct ref *ref)
> +{
> +	struct object_id oid;
> +	return !get_oid_hex(ref->name, &oid) &&
> +	       !ref->name[40] &&
> +	       !oidcmp(&oid, &ref->old_oid);
> +}

I think the preferred method these days is to avoid the bare "40":

  struct object_oid oid;
  const char *end;
  return !parse_oid_hex(ref->name, &oid, &end) &&
         !*end &&
	 !oidcmp(&oid, &ref->old_oid);

I was confused at first why we need this oidcmp() and the one below. But this one is checking "does the name parse to itself", and the other is checking "does this parse to our sought ref?". So both checks are needed.

Show 10 quoted lines

> +			for (i = 0; i < nr_sought; i++) {
> +				struct ref *s = sought[i];
> +				if (!strcmp(ref->name, s->name) ||
> +				    (is_literal_sha1(s) &&
> +				     !oidcmp(&ref->old_oid, &s->old_oid))) {
> +					keep = 1;
> +					s->match_status = REF_MATCHED;
>  				}
> -				i++;
>  			}

This will reparse ref->name as an oid via is_literal_sha1() for each pass through the loop. Should it be hoisted out? Maybe that is just premature optimization, though.

Other than those minor nits, the code itself looks fine to me.

-Peff