v3, 2 partsgpg-interface.c: use string_list_split*() instead of strbuf_split*()

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

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

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

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

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

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

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

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

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

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

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

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

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

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