On Wed, 13 May 2020 at 02:57, brian m. carlson
<sandals@xxxxxxxxxxxxxxxxxxxx> 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.
> 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`.
> 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
