Re: [PATCH v2] fetch-pack: always allow fetching of literal SHA1s
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- May 10, 2017, 18:01 UTC
- Message-ID
- <20170510180130.GY28740@aiede.svl.corp.google.com>
- In-Reply-To
- <20170510164432.5447-1-jonathantanmy@google.com>
Hi,
Jonathan Tan wrote:
Show 5 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.
Thanks for fixing it.
[...]
Show 13 quoted lines
> --- 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.
Show 25 quoted lines
> +}
> +
> 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.
[...]
Show 11 quoted lines
> --- 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