On 2020-05-16 at 10:40:11, Martin Ågren wrote:
> 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'll try to reword to improve things.

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

Can do.

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

Yeah, I can do that.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204