Re: [PATCH 14/44] connect: detect algorithm when fetching refs

From: Martin Ågren <martin.agren@gmail.com>
Date: May 16, 2020, 10:40 UTC
Message-ID: <CAN0heSo=xUGwb8FLXYpcFXO+e9dqvpzR=h2Bs+HC_ubQdTQuWw@mail.gmail.com>
In-Reply-To: <20200513005424.81369-15-sandals@crustytoothpaste.net>

On Wed, 13 May 2020 at 02:57, brian m. carlson <sandals@crustytoothpaste.net> wrote:

>
> If we're fetching refs, detect the hash algorithm and parse the refs
> using that algorithm.

As the added documentation from patch 2 says, if there are multiple "object-format" capabilities, "the first one given is the one used in the ref advertisement". And that's what you implement below.

Explaining that in this commit message and/or referring to "a recent commit" (patch 2) and/or adding that documentation here, not back then, would have avoided some confusion on my part, and perhaps also for future readers.

I don't have a strong opinion on which of those is better, I just think you could somehow make that a bit clearer here.

Show 5 quoted lines

>  static void process_capabilities(struct packet_reader *reader, int *len)
>  {
> +       const char *feat_val;
> +       int feat_len;
> +       int hash_algo;

You could reduce the scope of `hash_algo`.

Show 16 quoted lines

>         const char *line = reader->line;
>         int nul_location = strlen(line);
>         if (nul_location == *len)
>                 return;
>         server_capabilities_v1 = xstrdup(line + nul_location + 1);
>         *len = nul_location;
> +
> +       feat_val = server_feature_value("object-format", &feat_len);
> +       if (feat_val) {
> +               char *hash_name = xstrndup(feat_val, feat_len);
> +               hash_algo = hash_algo_by_name(hash_name);
> +               if (hash_algo != GIT_HASH_UNKNOWN)
> +                       reader->hash_algo = &hash_algos[hash_algo];
> +               free(hash_name);
> +       }
>  }

xstrndup is needed because we're not guaranteed a terminating NUL. You remember to call free afterwards. Ok.

If we don't get any "object-format", we do basically nothing here and `reader->hash_algo` will remain as whatever it already is. The docs from patch 2 promise that this will be handled as "SHA-1" -- would it be more robust if we did a similar fallback dance as you do elsewhere?

  feat_val = ...;
  if (!feat_val) {
          feat_val = hash_algos[GIT_HASH_SHA1].name;
          feat_len = strlen(feat_val);
  }
  char *hash_name = ...
  ...

You do initialize `reader->hash_algo` in patch 8, so I don't think this changes anything now. Maybe it's just premature future-proofing (if such a thing exists).

Martin