From: Bello Olamide <belkid98@gmail.com>
Date: Mon, 20 Oct 2025 08:15:00 GMT
Subject: Re: [Outreachy PATCH v3 2/2] gpg-interface: use string_list_split*() instead of strbuf_split*()
Message-ID: <CAD=f0L9Bu2xcOt98n_iB6Td2+pdniOP-wU_KyigJdt+3Oy3wxw@mail.gmail.com>
In-Reply-To: <xmqqikga3mqj.fsf@gitster.g>

On Sun, 19 Oct 2025 at 17:00, Junio C Hamano <gitster@pobox.com> wrote:
>
> Olamide Caleb Bello <belkid98@gmail.com> writes:
>
> > 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 string in `key_stdout` is then split using strbuf_split_max() into
> > two tokens at a new line and the first token is returned as a `char *`
> > and not a strbuf.
> > This makes the function lack the use of strbuf API as no edits are
> > performed on the split tokens.
> >
> > Replace strbuf_split_max() with string_list_split_in_place() for
> > simplicity
> >
> > Note that strbuf_split_max() uses `2` to indicate the number of tokens
> > to extract from the string, while string_list_split_in_place() uses `1`
> > to specify the number of times the split will be done on the string,
> > so 1 gives 2 tokens as it is in the original instance.
> >
> > string_list_split_in_place() returns the number of substrings added to the
> > list keys.items, so we check that at least one substring is added to the
> > list since we just want to return the first substring.
> >
> > Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
> > Reported-by: Junio Hamano <gister@pobox.com>
> > Helped-by: Christian Couder <christian.couder@gmail.com>
> > ---
> >  gpg-interface.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
>
> Exactly the same comment as [1/2] (including the part about the
> first paragraph seemingly missing something at the end ;-).
>
> Also, it may not be necessary to highlight the quirky way the
> string_list_split*() function counts numbers again, as it is done in
> the previous patch so readers have already been warned against it.

Okay noted.
>
> And the same comment applies about the round-about way the original
> was written in the first place.  Isn't it merely the matter of
> finding the first line-feed and making a copy of a string up to that
> point?

Yes that is the goal.
>
> Perhaps we would be better off if we revise the theme of the topic
> "use string_list_split*() to replace strbuf_split*()" to "do not use
> misdesigned strbuf_split*() function" and do the rewrite without
> using string_list_split*() after all?  It may result in a much
> cleaner and simpler code at the end.

Okay something like this?
       char *begin;
       char *end;
       char *new_line, *line_end, first_line;
       size_t line_len;

       if (!ret) {
         ...
       begin = key_stdout.buf;
       end = key_stdout.len;
       new_line = memchr(begin, '\n', key_stdout.len)
       line_end = new_line ? new_line : end;
       if (line_end > begin && *(line_end - 1) == '\r')
            line_end--;
       line_len = (size_t)(line_end - begin)
       if (line_len > 0) {
       firstline = xmemdupz(begin, line_len)
       }
       default_key = first_line
       ...
       return default_key

I am just asking to know if something like this
should be done within the respective functions or I will need
to write functions for each and just call here.

Thanks
Bello