[Outreachy PATCH v3 0/2] gpg-interface.c: use string_list_split*() instead of strbuf_split*()

[Outreachy PATCH v3 0/2] gpg-interface.c: use string_list_split*() instead of strbuf_split*()

[Olamide Caleb Bello]

The patch series by Junio Hamano with link below,
https://public-inbox.org/git/20250731225433.4028872-1-gitster@poddbox.com/,
notices that the array of strbufs that calls to strbuf_split*() provides
are merely used to store the strings gotten from the split and no edit are
done on these resulting strings making the strbuf_split*() unideal
for this usecase, with the string_list_split*() being a more suitable
option.

Commit 2efe707054 (wt-status: avoid strbuf_split*(), 2025-07-31) for example,
in the series, notes that abbrev_oid_in_line() takes one line of rebase
todo list and splits tokens out of this line using strbuf_split_max().
However, no simultanous edits that take advantage of the strbuf API take
place but the tokens are merely used as pieces of strings.

This series continues on this, by replacing instances of
strbuf_split_max() with string_list_split_in_place() where the string from
the split is merely returned as char * and no edits are done on them.

Tests have also been performed on the commits on Github CI. The link is shown below
https://github.com/git/git/pull/2074

Changes in v3:
==============
 - Use string_list_split_in_place() instead of string_list_split() used in v2
 - Change the test for the return value of string_list_split_in_place()
   in get_ssh_key_fingerprint() patch 1 to at least 2 tokens in the split
   list to allow a successful test where the key owner's identity is missing.
 - Change the test for the return value of string_list_split_in_place() in
   get_default_ssh_signing_key() in patch 2 to at least 1 token to allow for
   a successful test.
 - Modify commit messages to indicate the switch to string_list_split_in_place()
   from string_list_split() used in v2
 - Add explanation in commit message about the test for the return value of
   string_list_split_in_place() in both patches
 - Add Helped-by and Reported-by tags

Olamide Caleb Bello (2):
  gpg-interface: replace strbuf_split*() with string_list_split*()
  gpg-interface: use string_list_split*() instead of strbuf_split*()

 gpg-interface.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

Range-diff versus v2:

1:  818ca6b104 ! 1:  7da4fded53 gpg-interface: replace strbuf_split_max() with string_list_split()
    @@ Metadata
     Author: Olamide Caleb Bello <belkid98@gmail.com>

      ## Commit message ##
    -    gpg-interface: replace strbuf_split_max() with string_list_split()
    +    gpg-interface: replace strbuf_split*() with string_list_split*()

         In get_ssh_finger_print(), the output of the `ssh-keygen` command is
         put into `fingerprint_stdout
    @@ Commit message
         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 as a char * and not a strbuf, hence they do not need to be strbufs.
    +    returned as a char * and not a strbuf, hence they do not need to be
    +    strbufs.

    -    Use string_list_split() instead for simplicity.
    +    Use string_list_split_in_place() instead for simplicity.

         Note that strbuf_split_max() uses 3 to specify the number of tokens to
    -    extract from the string, while string_list_split() uses 2 because it specifies
    -    the number of times the split will be done on the string, so 2 gives 3 tokens
    -    as it is in the original instance.
    +    extract from the string, while string_list_split_in_place() uses 2
    +    because it specifies the number of times the split will be done on
    +    the string, so 2 gives 3 tokens as it is in the original instance.
    +
    +    string_list_split_in_place() returns the number of substrings added to
    +    the `split.items` so for a successful split of the string in
    +    fingerprint_stdout, at least two items should be added to split.items
    +    so we can always be certain that the substring at index 1 is the ssh
    +    fingerprint even if the key owner's identity part is missing from the
    +    string in fingerprint_stdout.

         Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
    +    Reported-by: Junio Hamano <gitster@pobox.com>
    +    Helped-by: Christian Couder <christian.couder@gmail.com>
    +    Helped-by: Junio Hamano <gitster@pobox.com>

      ## gpg-interface.c ##
     @@
    @@ gpg-interface.c: static char *get_ssh_key_fingerprint(const char *signing_key)
      	int ret = -1;
      	struct strbuf fingerprint_stdout = STRBUF_INIT;
     -	struct strbuf **fingerprint;
    -+	struct string_list split = STRING_LIST_INIT_DUP;
    ++	struct string_list split = STRING_LIST_INIT_NODUP;
      	char *fingerprint_ret;
      	const char *literal_key = NULL;

    @@ gpg-interface.c: static char *get_ssh_key_fingerprint(const char *signing_key)

     -	fingerprint = strbuf_split_max(&fingerprint_stdout, ' ', 3);
     -	if (!fingerprint[1])
    -+	if (string_list_split(&split, fingerprint_stdout.buf, " ", 2) != 3)
    ++	if (string_list_split_in_place(&split, fingerprint_stdout.buf, " ", 2) <= 1)
      		die_errno(_("failed to get the ssh fingerprint for key '%s'"),
      			  signing_key);

2:  024a44a242 ! 2:  9a6eb6ff8b gpg-interface: use string_list_split() instead of strbuf_split_max()
    @@ Metadata
     Author: Olamide Caleb Bello <belkid98@gmail.com>

      ## Commit message ##
    -    gpg-interface: use string_list_split() instead of strbuf_split_max()
    +    gpg-interface: use string_list_split*() instead of strbuf_split*()

         In get_default_ssh_signing_key(), the default ssh signing key is
         retrieved in `key_stdout`, which is then split using
    @@ Commit message
         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() for simplicity.
    +    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() 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.
    +    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 ##
     @@ gpg-interface.c: static char *get_default_ssh_signing_key(void)
    @@ gpg-interface.c: static char *get_default_ssh_signing_key(void)
      	int ret = -1;
      	struct strbuf key_stdout = STRBUF_INIT, key_stderr = STRBUF_INIT;
     -	struct strbuf **keys;
    -+	struct string_list keys = STRING_LIST_INIT_DUP;
    ++	struct string_list keys = STRING_LIST_INIT_NODUP;
      	char *key_command = NULL;
      	const char **argv;
      	int n;
    @@ gpg-interface.c: static char *get_default_ssh_signing_key(void)
      	if (!ret) {
     -		keys = strbuf_split_max(&key_stdout, '\n', 2);
     -		if (keys[0] && is_literal_ssh_key(keys[0]->buf, &literal_key)) {
    -+		if (string_list_split(&keys, key_stdout.buf, "\n", 1) == 2 &&
    ++		if (string_list_split_in_place(&keys, key_stdout.buf, "\n", 1) > 0 &&
     +			is_literal_ssh_key(keys.items[0].string, &literal_key)) {
      			/*
      			 * We only use `is_literal_ssh_key` here to check validity


-- 
2.51.0.463.g79cf913ea9

[Outreachy PATCH v3 1/2] gpg-interface: replace strbuf_split*() with string_list_split*()

[Olamide Caleb Bello]

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 as a char * and not a strbuf, hence they do not need to be
strbufs.

Use string_list_split_in_place() instead for simplicity.

Note that strbuf_split_max() uses 3 to specify the number of tokens to
extract from the string, while string_list_split_in_place() uses 2
because it specifies the number of times the split will be done on
the string, so 2 gives 3 tokens as it is in the original instance.

string_list_split_in_place() returns the number of substrings added to
the `split.items` so for a successful split of the string in
fingerprint_stdout, at least two items should be added to split.items
so we can always be certain that the substring at index 1 is the ssh
fingerprint even if the key owner's identity part is missing from the
string in fingerprint_stdout.

Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
Reported-by: Junio Hamano <gitster@pobox.com>
Helped-by: Christian Couder <christian.couder@gmail.com>
Helped-by: Junio Hamano <gitster@pobox.com>
---
 gpg-interface.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 2f4f0e32cb..cb182f4c11 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -14,6 +14,7 @@
 #include "sigchain.h"
 #include "tempfile.h"
 #include "alias.h"
+#include "string-list.h"
 
 static int git_gpg_config(const char *, const char *,
 			  const struct config_context *, void *);
@@ -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_NODUP;
 	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_in_place(&split, fingerprint_stdout.buf, " ", 2) <= 1)
 		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);
 	return fingerprint_ret;
 }
-- 
2.51.0.463.g79cf913ea9

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

[Junio C Hamano]

Olamide Caleb Bello <belkid98@gmail.com> writes:

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

Something lost at the end?  I'd assume

	... into `fingerpritn_stdout` strbuf.

and tweak the copy I received locally before applying.

> The string in fingerprint_stdout is then split into 3 strbufs using

"into up to 3 strbufs", I think.  If we do not say so here, ...

> 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 as a char * and not a strbuf, hence they do not need to be
> strbufs.
>
> Use string_list_split_in_place() instead for simplicity.
>
> Note that strbuf_split_max() uses 3 to specify the number of tokens to
> extract from the string, while string_list_split_in_place() uses 2
> because it specifies the number of times the split will be done on
> the string, so 2 gives 3 tokens as it is in the original instance.
>
> string_list_split_in_place() returns the number of substrings added to
> the `split.items` so for a successful split of the string in
> fingerprint_stdout, at least two items should be added to split.items

... this "at least two items" would become contradictory.

> so we can always be certain that the substring at index 1 is the ssh
> fingerprint even if the key owner's identity part is missing from the
> string in fingerprint_stdout.
>
> Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
> Reported-by: Junio Hamano <gitster@pobox.com>
> Helped-by: Christian Couder <christian.couder@gmail.com>
> Helped-by: Junio Hamano <gitster@pobox.com>
> ---
>  gpg-interface.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 2f4f0e32cb..cb182f4c11 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -14,6 +14,7 @@
>  #include "sigchain.h"
>  #include "tempfile.h"
>  #include "alias.h"
> +#include "string-list.h"
>  
>  static int git_gpg_config(const char *, const char *,
>  			  const struct config_context *, void *);
> @@ -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_NODUP;
>  	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_in_place(&split, fingerprint_stdout.buf, " ", 2) <= 1)

This may be just me, but when we expect at least 2, I would find it
more natural if we said "if (count < 2) then error", rather "if
(count <= 1) then error".  I'll let it pass, as there is nothing
mathematically incorrect here ;-).

>  		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);

OK.  This is a straight-forward rewrite that is fairly faithful to
the original.

But I wonder why the original was written in such a convoluted way
to just extract the first part of a string that is space delimited
tokens.  It is obviously not your fault that the original is written
that way, bit I would have expected it to be done more like this:

    char *begin = fingerprint_stdout.buf;
    char *delim = strchr(begin, ' ');
    if (!delim)
	die_errno("Barf!");
    fingerprint_ret = xmemdupz(begin, end - begin);

Am I missing something?

That may or may not be outside the scope of this topic, which is to
reduce the calls to a misdesigned strbuf_split*() API functions.

Thanks.

>  	strbuf_release(&fingerprint_stdout);
>  	return fingerprint_ret;
>  }

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

[Bello Olamide]

On Sun, 19 Oct 2025 at 16:52, Junio C Hamano <gitster@pobox.com> wrote:
>
> Olamide Caleb Bello <belkid98@gmail.com> writes:
>
> > In get_ssh_finger_print(), the output of the `ssh-keygen` command is
> > put into `fingerprint_stdout
>
> Something lost at the end?  I'd assume
>
>         ... into `fingerpritn_stdout` strbuf.
Yes, thank you.
>
> and tweak the copy I received locally before applying.
>
> > The string in fingerprint_stdout is then split into 3 strbufs using
>
> "into up to 3 strbufs", I think.  If we do not say so here, ...
>
> > 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 as a char * and not a strbuf, hence they do not need to be
> > strbufs.
> >
> > Use string_list_split_in_place() instead for simplicity.
> >
> > Note that strbuf_split_max() uses 3 to specify the number of tokens to
> > extract from the string, while string_list_split_in_place() uses 2
> > because it specifies the number of times the split will be done on
> > the string, so 2 gives 3 tokens as it is in the original instance.
> >
> > string_list_split_in_place() returns the number of substrings added to
> > the `split.items` so for a successful split of the string in
> > fingerprint_stdout, at least two items should be added to split.items
>
> ... this "at least two items" would become contradictory.

Yes, noted.

>
> > so we can always be certain that the substring at index 1 is the ssh
> > fingerprint even if the key owner's identity part is missing from the
> > string in fingerprint_stdout.
> >
> > Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
> > Reported-by: Junio Hamano <gitster@pobox.com>
> > Helped-by: Christian Couder <christian.couder@gmail.com>
> > Helped-by: Junio Hamano <gitster@pobox.com>
> > ---
> >  gpg-interface.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index 2f4f0e32cb..cb182f4c11 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -14,6 +14,7 @@
> >  #include "sigchain.h"
> >  #include "tempfile.h"
> >  #include "alias.h"
> > +#include "string-list.h"
> >
> >  static int git_gpg_config(const char *, const char *,
> >                         const struct config_context *, void *);
> > @@ -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_NODUP;
> >       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_in_place(&split, fingerprint_stdout.buf, " ", 2) <= 1)
>
> This may be just me, but when we expect at least 2, I would find it
> more natural if we said "if (count < 2) then error", rather "if
> (count <= 1) then error".  I'll let it pass, as there is nothing
> mathematically incorrect here ;-).
>
> >               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);
>
> OK.  This is a straight-forward rewrite that is fairly faithful to
> the original.
>
> But I wonder why the original was written in such a convoluted way
> to just extract the first part of a string that is space delimited
> tokens.  It is obviously not your fault that the original is written
> that way, bit I would have expected it to be done more like this:
>
>     char *begin = fingerprint_stdout.buf;
>     char *delim = strchr(begin, ' ');
>     if (!delim)
>         die_errno("Barf!");
>     fingerprint_ret = xmemdupz(begin, end - begin);
>
> Am I missing something?

Okay something like this which just finds the desired token and
returns a copy?
    char *begin = fingerprint_stdout.buf;
    char *end = begin + fingerprint_stdout.len;
    char *space, *start, *endtok;

   space = memchr(begin, ' ', end-begin);
    if (!space)
        die_errno(_("failed to get the ssh fingerprint for key '%s'"),
            signing_key);
    start = space + 1;
     while (start < end && (*start = ' ' || *start == '\t'))
            start++;

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

    endtok = start;
    while (endtok < end && *endtok != ' ' && *endtok != '\t' &&
             *endtok !== '\r' && *endtok != '\n')
        endtok++;

    if (endtok == start)
        die_errno(_("failed to get the ssh fingerprint for key '%s'"),
            signing_key);

    fingerprint_ret = xmemdupz(start, endtok - start);
    return fingerptint_ret


>
> That may or may not be outside the scope of this topic, which is to
> reduce the calls to a misdesigned strbuf_split*() API functions.
>
> Thanks.
>
> >       strbuf_release(&fingerprint_stdout);
> >       return fingerprint_ret;
> >  }

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

[Junio C Hamano]

Bello Olamide <belkid98@gmail.com> writes:

>> > -     fingerprint_ret = strbuf_detach(fingerprint[1], NULL);
>> > -     strbuf_list_free(fingerprint);
>> > +     fingerprint_ret = xstrdup(split.items[1].string);
>> > +     string_list_clear(&split, 0);
>>
>> OK.  This is a straight-forward rewrite that is fairly faithful to
>> the original.
>>
>> But I wonder why the original was written in such a convoluted way
>> to just extract the first part of a string that is space delimited
>> tokens.  It is obviously not your fault that the original is written
>> that way, bit I would have expected it to be done more like this:
>>
>>     char *begin = fingerprint_stdout.buf;
>>     char *delim = strchr(begin, ' ');
>>     if (!delim)
>>         die_errno("Barf!");
>>     fingerprint_ret = xmemdupz(begin, end - begin);
>>
>> Am I missing something?

What I was missing was that we use fingerprint[1], not
fingerprint[0].  So we need to do the strchr() twice, i.e.

     char *begin = fingerprint_stdout.buf;
     char *delim = strchr(begin, ' ');
     if (!delim)
         die_errno("Barf!");
     begin = delim + 1
     delim = strchr(begin, ' ');
     if (!delim)
         die_errno("Barf!");
     fingerprint_ret = xmemdupz(begin, end - begin);

> Okay something like this which just finds the desired token and
> returns a copy?

>     char *begin = fingerprint_stdout.buf;
>     char *end = begin + fingerprint_stdout.len;
>     char *space, *start, *endtok;
>
>    space = memchr(begin, ' ', end-begin);
>     if (!space)
>         die_errno(_("failed to get the ssh fingerprint for key '%s'"),
>             signing_key);
>     start = space + 1;
>      while (start < end && (*start = ' ' || *start == '\t'))
>             start++;

The original does not seem to care and uses the whole
fingerprint[1].buf; do we really care about tabs?  The same for
looking at CR or LF.

Even if we cared, we shouldn't have to open code strcspn() like this
;-)

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> What I was missing was that we use fingerprint[1], not
> fingerprint[0].  So we need to do the strchr() twice, i.e.
>
>      char *begin = fingerprint_stdout.buf;
>      char *delim = strchr(begin, ' ');
>      if (!delim)
>          die_errno("Barf!");
>      begin = delim + 1
>      delim = strchr(begin, ' ');
>      if (!delim)
>          die_errno("Barf!");
>      fingerprint_ret = xmemdupz(begin, end - begin);

Ouch, of course "end" is not declared anywhere and it is an obvious
typo of delim.  Sorry for not proofreading enough.

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

[Bello Olamide]

On Mon, 20 Oct 2025 at 17:31, Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > What I was missing was that we use fingerprint[1], not
> > fingerprint[0].  So we need to do the strchr() twice, i.e.
> >
> >      char *begin = fingerprint_stdout.buf;
> >      char *delim = strchr(begin, ' ');
> >      if (!delim)
> >          die_errno("Barf!");
> >      begin = delim + 1
> >      delim = strchr(begin, ' ');
> >      if (!delim)
> >          die_errno("Barf!");
> >      fingerprint_ret = xmemdupz(begin, end - begin);
>
> Ouch, of course "end" is not declared anywhere and it is an obvious
> typo of delim.  Sorry for not proofreading enough.

Okay thank you.

I will prepare the patch and resend it.
Thanks

Bello

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

[Bello Olamide]

On Mon, 20 Oct 2025 at 16:09, Junio C Hamano <gitster@pobox.com> wrote:
>
> Bello Olamide <belkid98@gmail.com> writes:
>
> >> > -     fingerprint_ret = strbuf_detach(fingerprint[1], NULL);
> >> > -     strbuf_list_free(fingerprint);
> >> > +     fingerprint_ret = xstrdup(split.items[1].string);
> >> > +     string_list_clear(&split, 0);
> >>
> >> OK.  This is a straight-forward rewrite that is fairly faithful to
> >> the original.
> >>
> >> But I wonder why the original was written in such a convoluted way
> >> to just extract the first part of a string that is space delimited
> >> tokens.  It is obviously not your fault that the original is written
> >> that way, bit I would have expected it to be done more like this:
> >>
> >>     char *begin = fingerprint_stdout.buf;
> >>     char *delim = strchr(begin, ' ');
> >>     if (!delim)
> >>         die_errno("Barf!");
> >>     fingerprint_ret = xmemdupz(begin, end - begin);
> >>
> >> Am I missing something?
>
> What I was missing was that we use fingerprint[1], not
> fingerprint[0].  So we need to do the strchr() twice, i.e.
>
>      char *begin = fingerprint_stdout.buf;
>      char *delim = strchr(begin, ' ');
>      if (!delim)
>          die_errno("Barf!");
>      begin = delim + 1
>      delim = strchr(begin, ' ');
>      if (!delim)
>          die_errno("Barf!");
>      fingerprint_ret = xmemdupz(begin, end - begin);

Okay thank you

>
> > Okay something like this which just finds the desired token and
> > returns a copy?
>
> >     char *begin = fingerprint_stdout.buf;
> >     char *end = begin + fingerprint_stdout.len;
> >     char *space, *start, *endtok;
> >
> >    space = memchr(begin, ' ', end-begin);
> >     if (!space)
> >         die_errno(_("failed to get the ssh fingerprint for key '%s'"),
> >             signing_key);
> >     start = space + 1;
> >      while (start < end && (*start = ' ' || *start == '\t'))
> >             start++;
>
> The original does not seem to care and uses the whole
> fingerprint[1].buf; do we really care about tabs?  The same for
> looking at CR or LF.
>
> Even if we cared, we shouldn't have to open code strcspn() like this
> ;-)

Okay thank you very much for the guidance.

Belo

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

[Kristoffer Haugsbakk]

On Sun, Oct 19, 2025, at 14:07, Olamide Caleb Bello wrote:
>[snip]
>
> Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>

Signoff should go at the end of the commit message.  You’re saying that
you are signing off on the changes as well as what was written in the
commit message before that line.  If I later add mine:

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

I would be saying that I am signing off on the changes as well as the
previous lines in the commit message, including that line that you
wrote.

> Reported-by: Junio Hamano <gitster@pobox.com>
> Helped-by: Christian Couder <christian.couder@gmail.com>
> Helped-by: Junio Hamano <gitster@pobox.com>
> ---
>  gpg-interface.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>[snip]

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

[Bello Olamide]

On Mon, 20 Oct 2025 at 17:46, Kristoffer Haugsbakk
<kristofferhaugsbakk@fastmail.com> wrote:
>
> On Sun, Oct 19, 2025, at 14:07, Olamide Caleb Bello wrote:
> >[snip]
> >
> > Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
>

Hello Kristoffer,
Thank you for your review.

> Signoff should go at the end of the commit message.  You’re saying that
> you are signing off on the changes as well as what was written in the
> commit message before that line.  If I later add mine:
>
>     Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
>     Signed-off-by: ME
>
> I would be saying that I am signing off on the changes as well as the
> previous lines in the commit message, including that line that you
> wrote.

Please just to make sure I get you correctly, you are saying the Signoff
should be the last thing in the commit message.

Like I should put it below the other tags so that if you also want to add yours,
It can easily go below mine and it would mean you are signing off on the
changes including my own Signoff?

>
> > Reported-by: Junio Hamano <gitster@pobox.com>
> > Helped-by: Christian Couder <christian.couder@gmail.com>
> > Helped-by: Junio Hamano <gitster@pobox.com>
       It should go below here instead?

Thank you.

Bello

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

[Kristoffer Haugsbakk]

On Mon, Oct 20, 2025, at 20:25, Bello Olamide wrote:
> On Mon, 20 Oct 2025 at 17:46, Kristoffer Haugsbakk
> <kristofferhaugsbakk@fastmail.com> wrote:
>> Signoff should go at the end of the commit message.  You’re saying that
>> you are signing off on the changes as well as what was written in the
>> commit message before that line.  If I later add mine:
>>
>>     Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
>>     Signed-off-by: ME
>>
>> I would be saying that I am signing off on the changes as well as the
>> previous lines in the commit message, including that line that you
>> wrote.
>
> Please just to make sure I get you correctly, you are saying the Signoff
> should be the last thing in the commit message.
>
> Like I should put it below the other tags so that if you also want to add yours,
> It can easily go below mine and it would mean you are signing off on the
> changes including my own Signoff?
>
>>
>> > Reported-by: Junio Hamano <gitster@pobox.com>
>> > Helped-by: Christian Couder <christian.couder@gmail.com>
>> > Helped-by: Junio Hamano <gitster@pobox.com>
>        It should go below here instead?

Yes, that is my understanding. :)

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

[Bello Olamide]

On Mon, 20 Oct 2025 at 19:37, Kristoffer Haugsbakk
<kristofferhaugsbakk@fastmail.com> wrote:
>
>
> On Mon, Oct 20, 2025, at 20:25, Bello Olamide wrote:
> > On Mon, 20 Oct 2025 at 17:46, Kristoffer Haugsbakk
> > <kristofferhaugsbakk@fastmail.com> wrote:
> >> Signoff should go at the end of the commit message.  You’re saying that
> >> you are signing off on the changes as well as what was written in the
> >> commit message before that line.  If I later add mine:
> >>
> >>     Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
> >>     Signed-off-by: ME
> >>
> >> I would be saying that I am signing off on the changes as well as the
> >> previous lines in the commit message, including that line that you
> >> wrote.
> >
> > Please just to make sure I get you correctly, you are saying the Signoff
> > should be the last thing in the commit message.
> >
> > Like I should put it below the other tags so that if you also want to add yours,
> > It can easily go below mine and it would mean you are signing off on the
> > changes including my own Signoff?
> >
> >>
> >> > Reported-by: Junio Hamano <gitster@pobox.com>
> >> > Helped-by: Christian Couder <christian.couder@gmail.com>
> >> > Helped-by: Junio Hamano <gitster@pobox.com>
> >        It should go below here instead?
>
> Yes, that is my understanding. :)

Okay noted.
Thank you very much Kristoffer

[Outreachy PATCH v3 2/2] gpg-interface: use string_list_split*() instead of strbuf_split*()

[Olamide Caleb Bello]

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(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index cb182f4c11..3b9d85a5ef 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -862,7 +862,7 @@ static char *get_default_ssh_signing_key(void)
 	struct child_process ssh_default_key = CHILD_PROCESS_INIT;
 	int ret = -1;
 	struct strbuf key_stdout = STRBUF_INIT, key_stderr = STRBUF_INIT;
-	struct strbuf **keys;
+	struct string_list keys = STRING_LIST_INIT_NODUP;
 	char *key_command = NULL;
 	const char **argv;
 	int n;
@@ -884,19 +884,19 @@ 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)) {
+		if (string_list_split_in_place(&keys, key_stdout.buf, "\n", 1) > 0 &&
+			is_literal_ssh_key(keys.items[0].string, &literal_key)) {
 			/*
 			 * We only use `is_literal_ssh_key` here to check validity
 			 * The prefix will be stripped when the key is used.
 			 */
-			default_key = strbuf_detach(keys[0], NULL);
+			default_key = xstrdup(keys.items[0].string);
 		} else {
 			warning(_("gpg.ssh.defaultKeyCommand succeeded but returned no keys: %s %s"),
 				key_stderr.buf, key_stdout.buf);
 		}
 
-		strbuf_list_free(keys);
+		string_list_clear(&keys, 0);
 	} else {
 		warning(_("gpg.ssh.defaultKeyCommand failed: %s %s"),
 			key_stderr.buf, key_stdout.buf);
-- 
2.51.0.463.g79cf913ea9

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

[Junio C Hamano]

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.

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?

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.

Thanks.

> diff --git a/gpg-interface.c b/gpg-interface.c
> index cb182f4c11..3b9d85a5ef 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -862,7 +862,7 @@ static char *get_default_ssh_signing_key(void)
>  	struct child_process ssh_default_key = CHILD_PROCESS_INIT;
>  	int ret = -1;
>  	struct strbuf key_stdout = STRBUF_INIT, key_stderr = STRBUF_INIT;
> -	struct strbuf **keys;
> +	struct string_list keys = STRING_LIST_INIT_NODUP;
>  	char *key_command = NULL;
>  	const char **argv;
>  	int n;
> @@ -884,19 +884,19 @@ 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)) {
> +		if (string_list_split_in_place(&keys, key_stdout.buf, "\n", 1) > 0 &&
> +			is_literal_ssh_key(keys.items[0].string, &literal_key)) {
>  			/*
>  			 * We only use `is_literal_ssh_key` here to check validity
>  			 * The prefix will be stripped when the key is used.
>  			 */
> -			default_key = strbuf_detach(keys[0], NULL);
> +			default_key = xstrdup(keys.items[0].string);
>  		} else {
>  			warning(_("gpg.ssh.defaultKeyCommand succeeded but returned no keys: %s %s"),
>  				key_stderr.buf, key_stdout.buf);
>  		}
>  
> -		strbuf_list_free(keys);
> +		string_list_clear(&keys, 0);
>  	} else {
>  		warning(_("gpg.ssh.defaultKeyCommand failed: %s %s"),
>  			key_stderr.buf, key_stdout.buf);

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

[Bello Olamide]

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

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

[Junio C Hamano]

Bello Olamide <belkid98@gmail.com> writes:

> 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.

Unlike [1/2] that asked for the second string, this one just wants
to discard everything after the first LF, so I am not sure if you
need any new helper or hand-rolled loop.  Wouldn't strchr() and
xmemdupz() that were used in my response to [1/2] sufficient for the
purpose of this step, too?

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

[Bello Olamide]

On Mon, 20 Oct 2025 at 16:18, Junio C Hamano <gitster@pobox.com> wrote:
>
> Bello Olamide <belkid98@gmail.com> writes:
>
> > 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.
>
> Unlike [1/2] that asked for the second string, this one just wants
> to discard everything after the first LF, so I am not sure if you
> need any new helper or hand-rolled loop.  Wouldn't strchr() and
> xmemdupz() that were used in my response to [1/2] sufficient for the
> purpose of this step, too?
>

Yes it will,
Thank you very much Junio.
I will send the new patch

Bello