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

[Jonathan Tan <jonathantanmy@google.com>]

On Fri, May 12, 2017 at 1:14 AM, Jeff King <peff@peff.net> wrote:
> diff --git a/fetch-pack.c b/fetch-pack.c
> index afb8b0502..e167213c0 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;
> @@ -592,13 +593,27 @@ 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);
> +}
> +
>  static void filter_refs(struct fetch_pack_args *args,
>                         struct ref **refs,
>                         struct ref **sought, int nr_sought)
>  {
>         struct ref *newlist = NULL;
>         struct ref **newtail = &newlist;
> +       struct ref *unmatched = NULL;
>         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 seeking_raw_oid = 0;
>         int i;
>
>         i = 0;
> @@ -617,7 +632,8 @@ static void filter_refs(struct fetch_pack_args *args,
>                                 else if (cmp == 0) {
>                                         keep = 1; /* definitely have it */
>                                         sought[i]->match_status = REF_MATCHED;
> -                               }
> +                               } else if (is_literal_sha1(sought[i]))
> +                                       seeking_raw_oid = 1;

As far as I can tell, this seems to coincidentally work because SHA-1s
as strings compare less than any ref name (HEAD or refs/...) - if this
weren't the case, the "break" statement above might cause this line to
never be executed. I'm not sure if we want to rely on that.

>                                 i++;
>                         }
>                 }
> @@ -631,24 +647,27 @@ static void filter_refs(struct fetch_pack_args *args,
>                         ref->next = NULL;
>                         newtail = &ref->next;
>                 } else {
> -                       free(ref);
> +                       ref->next = unmatched;
> +                       unmatched = ref;
>                 }
>         }
>
> +       if (seeking_raw_oid && !send_raw_oids) {
> +               for (ref = newlist; ref; ref = ref->next)
> +                       oidset_insert(&tip_oids, &ref->old_oid);
> +               for (ref = unmatched; ref; ref = ref->next)
> +                       oidset_insert(&tip_oids, &ref->old_oid);
> +       }
> +
>         /* Append unmatched requests to the list */
>         for (i = 0; i < nr_sought; i++) {
> -               unsigned char sha1[20];
> -
>                 ref = sought[i];
>                 if (ref->match_status != REF_NOT_MATCHED)
>                         continue;
> -               if (get_sha1_hex(ref->name, sha1) ||
> -                   ref->name[40] != '\0' ||
> -                   hashcmp(sha1, ref->old_oid.hash))
> +               if (!is_literal_sha1(ref))
>                         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;
> @@ -656,6 +675,13 @@ static void filter_refs(struct fetch_pack_args *args,
>                         ref->match_status = REF_UNADVERTISED_NOT_ALLOWED;
>                 }
>         }
> +
> +       oidset_clear(&tip_oids);
> +       for (ref = unmatched; ref; ref = next) {
> +               next = ref->next;
> +               free(ref);
> +       }
> +
>         *refs = newlist;
>  }
>