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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

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.

Thanks for fixing it.

[...]
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -592,6 +592,15 @@ 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);

When would a ref have its name be a oid and its value be a different
oid?

Ah, this check comes from

	commit b7916422c741b50925d454819b1977449fccc111
	Author: Jeff King <peff@peff.net>
	Date:   Thu Mar 19 16:34:51 2015 -0400

	    filter_ref: avoid overwriting ref->old_sha1 with garbage

which explains that the answer is "never" (leaving me even more
confused).  Anyway, that's orthogonal to this patch.

> +}
> +
>  static void filter_refs(struct fetch_pack_args *args,
>  			struct ref **refs,
>  			struct ref **sought, int nr_sought)
> @@ -601,7 +610,6 @@ static void filter_refs(struct fetch_pack_args *args,
>  	struct ref *ref, *next;
>  	int i;
>  
> -	i = 0;
>  	for (ref = *refs; ref; ref = next) {
>  		int keep = 0;
>  		next = ref->next;
> @@ -610,15 +618,14 @@ static void filter_refs(struct fetch_pack_args *args,
>  		    check_refname_format(ref->name, 0))
>  			; /* trash */
>  		else {
> -			while (i < nr_sought) {
> -				int cmp = strcmp(ref->name, sought[i]->name);
> -				if (cmp < 0)
> -					break; /* definitely do not have it */
> -				else if (cmp == 0) {
> -					keep = 1; /* definitely have it */
> -					sought[i]->match_status = REF_MATCHED;
> +			for (i = 0; i < nr_sought; i++) {

This resets i each time this code path is encountered.  Doesn't that
make this more expensive e.g. in the case where there are no sha1s
among the refs to be searched for?

Should this be conditional on whether there are any sha1-shaped refs in
'sought' to avoid that performance regression?

It's tempting to split 'sought' into two lists --- sha1-shaped refs
that have to be processed in full every time and the others that can
be processed in a single pass because they are in sorted order.

[...]
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -547,6 +547,12 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '
>  	git fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)
>  '
>  
> +test_expect_success 'fetch-pack can fetch a raw sha1 that is advertised as a ref' '
> +	git init server &&
> +	test_commit -C server 4 &&
> +	git fetch-pack server $(git -C server rev-parse refs/heads/master)
> +'

Is there test for the error case, too? (I.e. the server does not advertise
fetch-by-sha1 and the sha1 is not among the advertised refs)

Thanks,
Jonathan