From: Bello Olamide <belkid98@gmail.com>
Date: Wed, 15 Oct 2025 22:02:59 GMT
Subject: Re: [PATCH 2/2] [Outreachy] gpg-interface: use string_list_split instead of strbuf_split
Message-ID: <CAD=f0L90FjPs1R_s70gvbOMqG=H45wVj9n6g9vGTq_Xvt90E3g@mail.gmail.com>
In-Reply-To: <CAP8UFD3zu6DJRRBcSMOQsPQDQcgzSOsqsrMWJH51=VeBkWs+SQ@mail.gmail.com>

On Wed, 15 Oct 2025 at 16:28, Christian Couder
<christian.couder@gmail.com> wrote:
>
> 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."

Thank you. This is much better. I struggled a bit to get the right wording.
>
> > 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>
>
> > @@ -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.

Thank you very much.
I will make corrections
>
> > +                       is_literal_ssh_key(keys.items[0].string, &literal_key)) {
> > +                               default_key = xstrdup(keys.items[0].string);
>
> Thanks.