[PATCH 0/2] [Outreachy] gpg-interface.c: use string_list_split instead of strbuf_split_max

[PATCH 0/2] [Outreachy] gpg-interface.c: use string_list_split instead of strbuf_split_max

[Olamide Caleb Bello]

The array of strbufs that calls to strbuf_split_max provides are merely
used to store the list of tokens gotten from the split and no edit are
done on these resulting splits making the strbuf_split_max unideal
for this usecase.

This patchset replaces these instances with the modern string_list_split

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 | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

-- 
2.51.0.463.g79cf913ea9

[PATCH 1/2] [Outreachy] gpg-interface: replace strbuf_split with string_list_split

[Olamide Caleb Bello]

get_ssh_finger_print() accepts a signing key and then uses pipe_command
to execute the ssh-keygen command, gets its output and sets it in
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, hence they do not need to be strbufs.

Use string_list_split 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 initial instance.

Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
---
 gpg-interface.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 2f4f0e32cb..043a808577 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_DUP;
 	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(&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: [PATCH 1/2] [Outreachy] gpg-interface: replace strbuf_split with string_list_split

[Christian Couder]

On Wed, Oct 15, 2025 at 4:19 AM Olamide Caleb Bello <belkid98@gmail.com> wrote:
>
> get_ssh_finger_print() accepts a signing key and then uses pipe_command

"pipe_command" is a function too so it's better to use "()" when
talking about it like in "get_ssh_finger_print()".

In the commit message subject, if it doesn't make it too long, I think
it might be better to also use "()" when talking about functions.

> to execute the ssh-keygen command, gets its output and sets it in
> fingerprint_stdout.

Anyway I am not sure we need so many details about what
get_ssh_finger_print() does before the split.

Maybe saying something like the following is enough:

"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, hence they do not need to be strbufs.

It might be interesting to say that the fingerprint token is returned
as a `char *` not a strbuf.

> Use string_list_split instead for simplicity.

Here also using "()" could make it clearer that "string_list_split" is
a function.

> Note that strbuf_split_max uses 3 to specify the number of tokens to

Here also using "()" could help a bit.

> extract from the string, while string_list_split uses 2 because it specifies

Here also using "()" could help a bit.

> the number of times the split will be done on the string, so 2 gives 3 tokens
> as it is in the initial instance.

Maybe: s/initial/original/

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

[...]

> @@ -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(&split, fingerprint_stdout.buf, " ", 2) <= 1)

According to its doc, string_list_split() returns the the number of
substrings appended to the list. And you said in the commit message
that it should give 3 tokens, so I think the above line should be:

if (string_list_split(&split, fingerprint_stdout.buf, " ", 2) < 3)

or even:

if (string_list_split(&split, fingerprint_stdout.buf, " ", 2) != 3)

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

Except for the above points, your patch look good to me. Thanks.

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

[Bello Olamide]

On Wed, 15 Oct 2025 at 16:01, Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Wed, Oct 15, 2025 at 4:19 AM Olamide Caleb Bello <belkid98@gmail.com> wrote:
> >
> > get_ssh_finger_print() accepts a signing key and then uses pipe_command
>
> "pipe_command" is a function too so it's better to use "()" when
> talking about it like in "get_ssh_finger_print()".
>
> In the commit message subject, if it doesn't make it too long, I think
> it might be better to also use "()" when talking about functions.
>
> > to execute the ssh-keygen command, gets its output and sets it in
> > fingerprint_stdout.
>
> Anyway I am not sure we need so many details about what
> get_ssh_finger_print() does before the split.
>
> Maybe saying something like the following is enough:
>
> "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, hence they do not need to be strbufs.
>
> It might be interesting to say that the fingerprint token is returned
> as a `char *` not a strbuf.
>
> > Use string_list_split instead for simplicity.
>
> Here also using "()" could make it clearer that "string_list_split" is
> a function.
>
> > Note that strbuf_split_max uses 3 to specify the number of tokens to
>
> Here also using "()" could help a bit.
>
> > extract from the string, while string_list_split uses 2 because it specifies
>
> Here also using "()" could help a bit.
>
> > the number of times the split will be done on the string, so 2 gives 3 tokens
> > as it is in the initial instance.
>
> Maybe: s/initial/original/

Thank you very much for your review, I make the adjustments.
>
> > Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
>
> [...]
>
> > @@ -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(&split, fingerprint_stdout.buf, " ", 2) <= 1)
>
> According to its doc, string_list_split() returns the the number of
> substrings appended to the list. And you said in the commit message
> that it should give 3 tokens, so I think the above line should be:
>
> if (string_list_split(&split, fingerprint_stdout.buf, " ", 2) < 3)
>
> or even:
>
> if (string_list_split(&split, fingerprint_stdout.buf, " ", 2) != 3)
>

Okay, thank you very much Christian.
My thinking here is that after the split, it is expected that the
fingerprint token
will be at index 1 in the list so string_list_split() must append at
least 2 tokens.
so if it appends 1 token or none, I assume the fingerprint was not set
in the list.

I agree that < 3or != 3 is a better approach.
Thank you.
> >                 die_errno(_("failed to get the ssh fingerprint for key '%s'"),
> >                           signing_key);
>
> Except for the above points, your patch look good to me. Thanks.

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

[Olamide Caleb Bello]

get_default_ssh_signing_key() gets the signing key via the pipe_command and
stores the output in key_stdout.
The output string is then split using strbuf_split_max into two tokens at a
new line and the first token is returned. This makes the function lack the
use of strbuf API as no edits was performed on the split tokens.

Replace strbuf_split_max with string_list_split for simplicity.

Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
---
 gpg-interface.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 043a808577..7a78cbd8b2 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_DUP;
 	char *key_command = NULL;
 	const char **argv;
 	int n;
@@ -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.
-			 */
-			default_key = strbuf_detach(keys[0], NULL);
+		if (string_list_split(&keys, key_stdout.buf, "\n", 1) > 0 &&
+			is_literal_ssh_key(keys.items[0].string, &literal_key)) {
+				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: [PATCH 2/2] [Outreachy] gpg-interface: use string_list_split instead of strbuf_split

[Christian Couder]

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>

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

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

[Bello Olamide]

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.

Re: [PATCH 0/2] [Outreachy] gpg-interface.c: use string_list_split instead of strbuf_split_max

[Christian Couder]

On Wed, Oct 15, 2025 at 4:19 AM Olamide Caleb Bello <belkid98@gmail.com> wrote:
>
> The array of strbufs that calls to strbuf_split_max provides are merely
> used to store the list of tokens gotten from the split and no edit are
> done on these resulting splits making the strbuf_split_max unideal
> for this usecase.
>
> This patchset replaces these instances with the modern string_list_split

A cover letter like this should be used to provide broad context for
the changes. So if there are there patch series or merged commits that
started doing something similar, it would be a good idea to mention
them or give a link. (To properly mention a commit, you can use
something like: `git show -s --pretty="tformat:%h (%s, %ad)"
--date=short <commit>`.) If there are mailing list discussions were
the topic was discussed, it could be interesting to mention and link
them too.

Thanks.

Re: [PATCH 0/2] [Outreachy] gpg-interface.c: use string_list_split instead of strbuf_split_max

[Bello Olamide]

On Wed, 15 Oct 2025 at 16:40, Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Wed, Oct 15, 2025 at 4:19 AM Olamide Caleb Bello <belkid98@gmail.com> wrote:
> >
> > The array of strbufs that calls to strbuf_split_max provides are merely
> > used to store the list of tokens gotten from the split and no edit are
> > done on these resulting splits making the strbuf_split_max unideal
> > for this usecase.
> >
> > This patchset replaces these instances with the modern string_list_split
>
> A cover letter like this should be used to provide broad context for
> the changes. So if there are there patch series or merged commits that
> started doing something similar, it would be a good idea to mention
> them or give a link. (To properly mention a commit, you can use
> something like: `git show -s --pretty="tformat:%h (%s, %ad)"
> --date=short <commit>`.) If there are mailing list discussions were
> the topic was discussed, it could be interesting to mention and link
> them too.
>
> Thanks.

Okay this is noted too.
Thank you very much for the feedback.

Bello