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

[Jeff King <peff@peff.net>]

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

> 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.

> +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.

> +			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