Re: [PATCH 2/2] [Outreachy] gpg-interface: use string_list_split instead of strbuf_split

From: Christian Couder <christian.couder@gmail.com>
Date: Oct 15, 2025, 15:28 UTC
Message-ID: <CAP8UFD3zu6DJRRBcSMOQsPQDQcgzSOsqsrMWJH51=VeBkWs+SQ@mail.gmail.com>
In-Reply-To: <d1c20c911637870c3cacfedf763ce508a641710f.1760490943.git.belkid98@gmail.com>

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

>
> get_default_ssh_signing_key() gets the signing key via the pipe_command and

s/the pipe_command/pipe_command()/

> stores the output in key_stdout.

It's not very clear from the sentence if the signing key is the output or not. Maybe something like:

"In get_default_ssh_signing_key(), the default ssh signing key is retrieved in `key_stdout`, which is then split using strbuf_split_max() into two tokens."

> The output string is then split using strbuf_split_max into two tokens at a
> new line and the first token is returned.

Here also it might be interesting to know that the first token is returned as a `char *`, not a strbuf.

> This makes the function lack the
> use of strbuf API as no edits was performed on the split tokens.

s/was performed/are performed/

> Replace strbuf_split_max with string_list_split for simplicity.

Here also, using "()" could help a bit as it would make it clear that "strbuf_split_max" and "string_list_split" are functions.

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

Show 10 quoted lines

> @@ -884,19 +884,15 @@ static char *get_default_ssh_signing_key(void)
>                            &key_stderr, 0);
>
>         if (!ret) {
> -               keys = strbuf_split_max(&key_stdout, '\n', 2);
> -               if (keys[0] && is_literal_ssh_key(keys[0]->buf, &literal_key)) {
> -                       /*
> -                        * We only use `is_literal_ssh_key` here to check validity
> -                        * The prefix will be stripped when the key is used.
> -                        */

Why is this comment removed? It's not clear to me that it's not valid anymore.

> -                       default_key = strbuf_detach(keys[0], NULL);
> +               if (string_list_split(&keys, key_stdout.buf, "\n", 1) > 0 &&

In the commit message you should explain, like you did for the previous commit, why "1" is passed to string_list_split() while "2" was passed to strbuf_split_max().

Also I think that, instead of "> 0", the tests should be ">=2" or "== 2". Or, if an output that contains no new line is valid, then that should be explained in the commit message.

> +                       is_literal_ssh_key(keys.items[0].string, &literal_key)) {
> +                               default_key = xstrdup(keys.items[0].string);

Thanks.