Re: [PATCH 1/2] [Outreachy] gpg-interface: replace strbuf_split with string_list_split

From: Christian Couder <christian.couder@gmail.com>
Date: Oct 15, 2025, 15:01 UTC
Message-ID: <CAP8UFD1U07T8_nX+sSL6ZHhCQsQuy5PwfQfbHC5piC5W5T=7Cw@mail.gmail.com>
In-Reply-To: <aea3b80a8883c4dac4e05defdee3eb82b83fe8c9.1760490943.git.belkid98@gmail.com>

On Wed, Oct 15, 2025 at 4:19 AM Olamide Caleb Bello <belkid98@gmail.com> wrote:

>
> get_ssh_finger_print() accepts a signing key and then uses pipe_command

"pipe_command" is a function too so it's better to use "()" when talking about it like in "get_ssh_finger_print()".

In the commit message subject, if it doesn't make it too long, I think it might be better to also use "()" when talking about functions.

> to execute the ssh-keygen command, gets its output and sets it in
> fingerprint_stdout.

Anyway I am not sure we need so many details about what get_ssh_finger_print() does before the split.

Maybe saying something like the following is enough:

"In get_ssh_finger_print(), the output of the `ssh-keygen` command is put into `fingerprint_stdout`."

> The string in fingerprint_stdout is then split into 3 strbufs using
> strbuf_split_max(), however they are not modified after the split thereby
> not making use of the strbuf API as the fingerprint token is merely
> returned, hence they do not need to be strbufs.

It might be interesting to say that the fingerprint token is returned as a `char *` not a strbuf.

> Use string_list_split instead for simplicity.

Here also using "()" could make it clearer that "string_list_split" is a function.

> Note that strbuf_split_max uses 3 to specify the number of tokens to

Here also using "()" could help a bit.

> extract from the string, while string_list_split uses 2 because it specifies

Here also using "()" could help a bit.

> the number of times the split will be done on the string, so 2 gives 3 tokens
> as it is in the initial instance.

Maybe: s/initial/original/

> Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>

[...]

Show 7 quoted lines

> @@ -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) <= 1)

According to its doc, string_list_split() returns the the number of substrings appended to the list. And you said in the commit message that it should give 3 tokens, so I think the above line should be:

if (string_list_split(&split, fingerprint_stdout.buf, " ", 2) < 3)

or even:

if (string_list_split(&split, fingerprint_stdout.buf, " ", 2) != 3)

>                 die_errno(_("failed to get the ssh fingerprint for key '%s'"),
>                           signing_key);

Except for the above points, your patch look good to me. Thanks.