| Message ID | 20200513005424.81369-15-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SHA-256 part 2/3: protocol functionality | expand |
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. > 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
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.
diff --git a/connect.c b/connect.c index 511a069304..a39b843589 100644 --- a/connect.c +++ b/connect.c @@ -220,12 +220,24 @@ static void annotate_refs_with_symref_info(struct ref *ref) static void process_capabilities(struct packet_reader *reader, int *len) { + const char *feat_val; + int feat_len; + int 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); + } } static int process_dummy_ref(const struct packet_reader *reader) @@ -234,7 +246,7 @@ static int process_dummy_ref(const struct packet_reader *reader) struct object_id oid; const char *name; - if (parse_oid_hex(line, &oid, &name)) + if (parse_oid_hex_algop(line, &oid, &name, reader->hash_algo)) return 0; if (*name != ' ') return 0; @@ -258,7 +270,7 @@ static int process_ref(const struct packet_reader *reader, int len, struct object_id old_oid; const char *name; - if (parse_oid_hex(line, &old_oid, &name)) + if (parse_oid_hex_algop(line, &old_oid, &name, reader->hash_algo)) return 0; if (*name != ' ') return 0; @@ -270,7 +282,7 @@ static int process_ref(const struct packet_reader *reader, int len, die(_("protocol error: unexpected capabilities^{}")); } else if (check_ref(name, flags)) { struct ref *ref = alloc_ref(name); - oidcpy(&ref->old_oid, &old_oid); + memcpy(ref->old_oid.hash, old_oid.hash, reader->hash_algo->rawsz); **list = ref; *list = &ref->next; } @@ -288,7 +300,7 @@ static int process_shallow(const struct packet_reader *reader, int len, if (!skip_prefix(line, "shallow ", &arg)) return 0; - if (get_oid_hex(arg, &old_oid)) + if (get_oid_hex_algop(arg, &old_oid, reader->hash_algo)) die(_("protocol error: expected shallow sha-1, got '%s'"), arg); if (!shallow_points) die(_("repository on the other end cannot be shallow"));
If we're fetching refs, detect the hash algorithm and parse the refs using that algorithm. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- connect.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)