Re: [Outreachy PATCH v2 1/2] gpg-interface: replace strbuf_split_max() with string_list_split()
Olamide Caleb Bello <belkid98@gmail.com> writes:
Show 16 quoted lines
> @@ -821,7 +822,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key)
> struct child_process ssh_keygen = CHILD_PROCESS_INIT;
> int ret = -1;
> struct strbuf fingerprint_stdout = STRBUF_INIT;
> - struct strbuf **fingerprint;
> + struct string_list split = STRING_LIST_INIT_DUP;
> char *fingerprint_ret;
> const char *literal_key = NULL;
>
> @@ -845,13 +846,12 @@ static char *get_ssh_key_fingerprint(const char *signing_key)
> die_errno(_("failed to get the ssh fingerprint for key '%s'"),
> signing_key);
>
> - fingerprint = strbuf_split_max(&fingerprint_stdout, ' ', 3);
> - if (!fingerprint[1])
> + if (string_list_split(&split, fingerprint_stdout.buf, " ", 2) != 3)The original splits the thing into upto 3 pieces, but only complains if the second piece is NULL (i.e. we said "up to 3", but there was not even one place to split, and the request to split_max gave the one thing as one piece). IOW, the original code will happily accept if the finterprint_stdout is split only into two, not three.
The updated code asks to split at at most two places (yes, it is a confusing API, but if you split at two places, you will end up with three pieces), and insists that the split results in three pieces.
So the rewrite tightens the error condition.
Was the original code too loose in detecting an error, and does this patch tightens the condition "while at it"? Or was the original code correct to expect that there are legitimate cases where the payload in finterprint_stdout only contains two pieces, and it was the right thing to do to accept when fingerprint[1] is not NULL but fingerprint[2] is NULL?
This is a genuine question. I haven't studied the code path to reach this point in the code flow, I don't know what the data in fingerprint_stdout is supposed to look like, so I do not know the answer to the question (in other words, it cannot be an oblique way to point out that the updated code is wrong or anything like that).
Show 8 quoted lines
> die_errno(_("failed to get the ssh fingerprint for key '%s'"),
> signing_key);
>
> - fingerprint_ret = strbuf_detach(fingerprint[1], NULL);
> - strbuf_list_free(fingerprint);
> + fingerprint_ret = xstrdup(split.items[1].string);
> + string_list_clear(&split, 0);
> strbuf_release(&fingerprint_stdout);Since this code releases fingerprint_stdout before leaving, and returns a fresh copy of a split piece, it may make more sense to use string_list_split_in_place(), which does not have to allocate extra strings while it does its work, unlike string_list_split().
> return fingerprint_ret;
> }
Olamide Caleb Bello
