[Outreachy PATCH v4 0/2] do not use strbuf_split*()

[Outreachy PATCH v4 0/2] do not use 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 in those cases.

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 cleanup, by replacing instances of
strbuf_split_max() with strchr() to get the required token around the
delimiter where the token from the split is merely returned as char *
and not strbufs and no edits are done on them.
This makes the code cleaner, faster and more efficient.

Tests have also been performed on the commits on Github CI. The link is shown below

https://github.com/git/git/pull/2076

Changes in v4:
==============
 - Use strchr() to extract the required token over string_list_split_in_place
   used in v3.
 - Modify commit messages to indicate the switch to strchr() from
   string_list_split_in_place() used in v2 and reason for prefering strchr()

Olamide Caleb Bello (2):
  gpg-interface: do not use misdesigned strbuf_split*()
  gpg-interface: do not use misdesigned strbuf_split*() [Part 2]

 gpg-interface.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

Range diff versus v3
====================
1:  7da4fded53 < -:  ---------- gpg-interface: replace strbuf_split*() with string_list_split*()
-:  ---------- > 1:  2879d9be36 gpg-interface: do not use misdesigned strbuf_split*()
2:  9a6eb6ff8b ! 2:  a830de15ec gpg-interface: use string_list_split*() instead of strbuf_split*()
    @@ Metadata
     Author: Olamide Caleb Bello <belkid98@gmail.com>

      ## Commit message ##
    -    gpg-interface: use string_list_split*() instead of strbuf_split*()
    +    gpg-interface: do not use misdesigned strbuf_split*() [Part 2]

         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.
    +    retrieved in `key_stdout` buf, which is then split using
    +    strbuf_split_max() into up to two strbufs at a new line and the first
    +    strbuf 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.
    +    Simplify the process of retrieving and returning the desired line by
    +    using strchr() to isolate the line and xmemdupz() to return a copy of the
    +    line.
    +    This removes the roundabout way of splitting the string into strbufs, just
    +    to return the line.

    -    Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
    -    Reported-by: Junio Hamano <gister@pobox.com>
    +    Reported-by: Junio Hamano <gitster@pobox.com>
         Helped-by: Christian Couder <christian.couder@gmail.com>
    +    Helped-by: Junio Hamano <gitster@pobox.com>
    +    Helped-by: Krisoffer Haughsbakk
    +    Signed-off-by: Olamide Caleb Bello <belkid98@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_NODUP;
      	char *key_command = NULL;
      	const char **argv;
      	int n;
    + 	char *default_key = NULL;
    + 	const char *literal_key = NULL;
    ++	char *begin, *new_line, *first_line;
    +
    + 	if (!ssh_default_key_command)
    + 		die(_("either user.signingkey or gpg.ssh.defaultKeyCommand needs to be configured"));
     @@ gpg-interface.c: 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)) {
    ++		begin = key_stdout.buf;
    ++		new_line = strchr(begin, '\n');
    ++		first_line = xmemdupz(begin, new_line - begin);
    ++		if (is_literal_ssh_key(first_line, &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);
    ++			default_key = first_line;
      		} else {
    ++			free(first_line);
      			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

[Outreachy PATCH v4 1/2] gpg-interface: do not use misdesigned strbuf_split*()

[Olamide Caleb Bello]

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

The string in fingerprint_stdout is then split into up to 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.

Simplify the process of retrieving and returning the desired token by
using strchr() to isolate the token and xmemdupz() to return a copy of the
token.
This removes the roundabout way of splitting the string into strbufs, just
to return the token.

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

diff --git a/gpg-interface.c b/gpg-interface.c
index 2f4f0e32cb..1d793a56d2 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -821,8 +821,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;
-	char *fingerprint_ret;
+	char *fingerprint_ret, *begin, *delim;
 	const char *literal_key = NULL;
 
 	/*
@@ -845,13 +844,17 @@ 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])
-		die_errno(_("failed to get the ssh fingerprint for key '%s'"),
+	begin = fingerprint_stdout.buf;
+	delim = strchr(fingerprint_stdout.buf, ' ');
+	if (!delim)
+		die_errno(_("failed to get the ssh fingerprint for key %s"),
 			  signing_key);
-
-	fingerprint_ret = strbuf_detach(fingerprint[1], NULL);
-	strbuf_list_free(fingerprint);
+	begin = delim + 1;
+	delim = strchr(begin, ' ');
+	if (!delim)
+	    die_errno(_("failed to get the ssh fingerprint for key %s"),
+			  signing_key);
+	fingerprint_ret = xmemdupz(begin, delim - begin);
 	strbuf_release(&fingerprint_stdout);
 	return fingerprint_ret;
 }
-- 
2.51.0.463.g79cf913ea9

[Outreachy PATCH v4 2/2] gpg-interface: do not use misdesigned strbuf_split*() [Part 2]

[Olamide Caleb Bello]

In get_default_ssh_signing_key(), the default ssh signing key is
retrieved in `key_stdout` buf, which is then split using
strbuf_split_max() into up to two strbufs at a new line and the first
strbuf 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.

Simplify the process of retrieving and returning the desired line by
using strchr() to isolate the line and xmemdupz() to return a copy of the
line.
This removes the roundabout way of splitting the string into strbufs, just
to return the line.

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

diff --git a/gpg-interface.c b/gpg-interface.c
index 1d793a56d2..420e3a6646 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -865,12 +865,12 @@ 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;
 	char *key_command = NULL;
 	const char **argv;
 	int n;
 	char *default_key = NULL;
 	const char *literal_key = NULL;
+	char *begin, *new_line, *first_line;
 
 	if (!ssh_default_key_command)
 		die(_("either user.signingkey or gpg.ssh.defaultKeyCommand needs to be configured"));
@@ -887,19 +887,21 @@ 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)) {
+		begin = key_stdout.buf;
+		new_line = strchr(begin, '\n');
+		first_line = xmemdupz(begin, new_line - begin);
+		if (is_literal_ssh_key(first_line, &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 = first_line;
 		} else {
+			free(first_line);
 			warning(_("gpg.ssh.defaultKeyCommand succeeded but returned no keys: %s %s"),
 				key_stderr.buf, key_stdout.buf);
 		}
 
-		strbuf_list_free(keys);
 	} else {
 		warning(_("gpg.ssh.defaultKeyCommand failed: %s %s"),
 			key_stderr.buf, key_stdout.buf);
-- 
2.51.0.463.g79cf913ea9

Re: [Outreachy PATCH v4 1/2] gpg-interface: do not use misdesigned strbuf_split*()

[Christian Couder]

On Tue, Oct 21, 2025 at 12:56 AM Olamide Caleb Bello <belkid98@gmail.com> wrote:
>
> In get_ssh_finger_print(), the output of the `ssh-keygen` command is
> put into `fingerprint_stdout` strbuf.

Nit: I think this sentence doesn't need to be in its own paragraph. It
could be at the start of the paragraph below.

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

Nit: above the variable `fingerprint_stdout` was quoted, but now it's
not quoted anymore. I think it would be more consistent to quote it
here too.

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

Nit: this sentence is a bit long. Maybe "however they ..." and "hence
they ..." could start new sentences instead.

> Simplify the process of retrieving and returning the desired token by
> using strchr() to isolate the token and xmemdupz() to return a copy of the
> token.
> This removes the roundabout way of splitting the string into strbufs, just
> to return the token.

Nit: this last sentence should either be in its own paragraph, in
which case there should be a blank line before it, or it should be
part of the previous paragraph.

> Reported-by: Junio Hamano <gitster@pobox.com>
> Helped-by: Christian Couder <christian.couder@gmail.com>
> Helped-by: Junio Hamano <gitster@pobox.com>

Nit: Junio reviews all the patches and adds his own "Signed-off-by:"
to the patch that are accepted, so there is no need to also mention
him in an "Helped-by:" trailer like this.

> Helped-by: Krisoffer Haughsbakk

I think you mean "Kristoffer Haugsbakk". Please spell his name
correctly and provide his email address like for everyone else.

[...]

> @@ -845,13 +844,17 @@ 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])
> -               die_errno(_("failed to get the ssh fingerprint for key '%s'"),
> +       begin = fingerprint_stdout.buf;

`begin` is set here, but not used below...

> +       delim = strchr(fingerprint_stdout.buf, ' ');
> +       if (!delim)
> +               die_errno(_("failed to get the ssh fingerprint for key %s"),
>                           signing_key);

(This might be an issue that already existed, but I wonder if using
die_errno() instead of just die() is the right thing to do here.
Shouldn't we check errno before splitting?)

> -       fingerprint_ret = strbuf_detach(fingerprint[1], NULL);
> -       strbuf_list_free(fingerprint);
> +       begin = delim + 1;

... before here, where `begin` is set to something else. This means it
was useless to set it to `fingerprint_stdout.buf` before.

> +       delim = strchr(begin, ' ');
> +       if (!delim)
> +           die_errno(_("failed to get the ssh fingerprint for key %s"),
> +                         signing_key);
> +       fingerprint_ret = xmemdupz(begin, delim - begin);
>         strbuf_release(&fingerprint_stdout);
>         return fingerprint_ret;

I think this could be `return xmemdupz(begin, delim - begin);`, so we
could get rid of `fingerprint_ret`.

Thanks.

Re: [Outreachy PATCH v4 1/2] gpg-interface: do not use misdesigned strbuf_split*()

[Christian Couder]

On Tue, Oct 21, 2025 at 8:46 AM Christian Couder
<christian.couder@gmail.com> wrote:

> > +       delim = strchr(begin, ' ');
> > +       if (!delim)
> > +           die_errno(_("failed to get the ssh fingerprint for key %s"),
> > +                         signing_key);
> > +       fingerprint_ret = xmemdupz(begin, delim - begin);
> >         strbuf_release(&fingerprint_stdout);
> >         return fingerprint_ret;
>
> I think this could be `return xmemdupz(begin, delim - begin);`, so we
> could get rid of `fingerprint_ret`.

No, actually I think we need `fingerprint_ret` because we need to call
`xmemdupz(begin, delim - begin)` before releasing
`fingerprint_stdout`. Sorry for the noise.

Re: [Outreachy PATCH v4 2/2] gpg-interface: do not use misdesigned strbuf_split*() [Part 2]

[Christian Couder]

On Tue, Oct 21, 2025 at 12:57 AM Olamide Caleb Bello <belkid98@gmail.com> wrote:

[...]

> Reported-by: Junio Hamano <gitster@pobox.com>
> Helped-by: Christian Couder <christian.couder@gmail.com>
> Helped-by: Junio Hamano <gitster@pobox.com>
> Helped-by: Krisoffer Haughsbakk

I won't repeat the issues that are the same as in patch 1/2, but
please correct them.

[...]

> @@ -887,19 +887,21 @@ 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)) {
> +               begin = key_stdout.buf;
> +               new_line = strchr(begin, '\n');
> +               first_line = xmemdupz(begin, new_line - begin);

What if no \n character is found by strchr()?

Thanks.

Re: [Outreachy PATCH v4 0/2] do not use strbuf_split*()

[Christian Couder]

On Tue, Oct 21, 2025 at 12:56 AM Olamide Caleb Bello <belkid98@gmail.com> wrote:
>
> 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 in those cases.

Now that the string_list_split*() functions are not used in your
series anymore, I think you can remove "with the string_list_split*()
being a more suitable option in those cases".

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

I am not sure taking this commit as an example is really useful now
that the string_list_split*() functions are not used in your series
anymore. Maybe you can find a more relevant example commit in Junio's
series?

[...]

> Olamide Caleb Bello (2):
>   gpg-interface: do not use misdesigned strbuf_split*()
>   gpg-interface: do not use misdesigned strbuf_split*() [Part 2]

I don't think having "[Part 2]" is a good idea if there is no "[Part
1]". And maybe using "part 1/2" and "part 2/2" is even better if you
want to go this way (so that would be for example "gpg-interface: do
not use misdesigned strbuf_split*(), part 1/2"). Otherwise, I think
it's Ok if both commits have exactly the same subject.

Also please start to use the `--in-reply-to=<...>` option of `git
send-email` so that your patch series are all in the same thread on
the mailing list archive. For example right now if you look at
https://lore.kernel.org/git/cover.1760997183.git.belkid98@gmail.com/#r,
you will see:

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-10-20 22:55 Olamide Caleb Bello [this message]
2025-10-20 22:55 ` [Outreachy PATCH v4 1/2] gpg-interface: do not use
misdesigned strbuf_split*() Olamide Caleb Bello
2025-10-21  6:46   ` Christian Couder
2025-10-21  6:51     ` Christian Couder
2025-10-20 22:55 ` [Outreachy PATCH v4 2/2] gpg-interface: do not use
misdesigned strbuf_split*() [Part 2] Olamide Caleb Bello

So we don't see the previous patches and messages related to v1, v2 and v3.

If the tutorials and documentation are not clear enough, and you can't
make it work, then please ask for help and say what you tried so that
we can help you with this.

Thanks.

Re: [Outreachy PATCH v4 0/2] do not use strbuf_split*()

[Bello Olamide]

On Tue, 21 Oct 2025 at 08:19, Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Tue, Oct 21, 2025 at 12:56 AM Olamide Caleb Bello <belkid98@gmail.com> wrote:
> >
> > 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 in those cases.
>
> Now that the string_list_split*() functions are not used in your
> series anymore, I think you can remove "with the string_list_split*()
> being a more suitable option in those cases".

Okay thank you.

> > 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.
>
> I am not sure taking this commit as an example is really useful now
> that the string_list_split*() functions are not used in your series
> anymore. Maybe you can find a more relevant example commit in Junio's
> series?
>
> [...]

Okay. Thank you. I will take a closer look at the series and look for
a more suitable
reference.

>
> > Olamide Caleb Bello (2):
> >   gpg-interface: do not use misdesigned strbuf_split*()
> >   gpg-interface: do not use misdesigned strbuf_split*() [Part 2]
>
> I don't think having "[Part 2]" is a good idea if there is no "[Part
> 1]". And maybe using "part 1/2" and "part 2/2" is even better if you
> want to go this way (so that would be for example "gpg-interface: do
> not use misdesigned strbuf_split*(), part 1/2"). Otherwise, I think
> it's Ok if both commits have exactly the same subject.

Okay. Noted.
>
> Also please start to use the `--in-reply-to=<...>` option of `git
> send-email` so that your patch series are all in the same thread on
> the mailing list archive. For example right now if you look at
> https://lore.kernel.org/git/cover.1760997183.git.belkid98@gmail.com/#r,
> you will see:
>
> Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
> 2025-10-20 22:55 Olamide Caleb Bello [this message]
> 2025-10-20 22:55 ` [Outreachy PATCH v4 1/2] gpg-interface: do not use
> misdesigned strbuf_split*() Olamide Caleb Bello
> 2025-10-21  6:46   ` Christian Couder
> 2025-10-21  6:51     ` Christian Couder
> 2025-10-20 22:55 ` [Outreachy PATCH v4 2/2] gpg-interface: do not use
> misdesigned strbuf_split*() [Part 2] Olamide Caleb Bello
>
> So we don't see the previous patches and messages related to v1, v2 and v3.
>
> If the tutorials and documentation are not clear enough, and you can't
> make it work, then please ask for help and say what you tried so that
> we can help you with this.

Thank you very much for your time and review Christian.

I will study the documentation to see how to do this.

Bello

Re: [Outreachy PATCH v4 1/2] gpg-interface: do not use misdesigned strbuf_split*()

[Bello Olamide]

On Tue, 21 Oct 2025 at 07:46, Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Tue, Oct 21, 2025 at 12:56 AM Olamide Caleb Bello <belkid98@gmail.com> wrote:
> >
> > In get_ssh_finger_print(), the output of the `ssh-keygen` command is
> > put into `fingerprint_stdout` strbuf.

Okay noted.
>
> Nit: I think this sentence doesn't need to be in its own paragraph. It
> could be at the start of the paragraph below.
>
> > The string in fingerprint_stdout is then split into up to 3 strbufs using
>
> Nit: above the variable `fingerprint_stdout` was quoted, but now it's
> not quoted anymore. I think it would be more consistent to quote it
> here too.

Sorry about that. I'll fix it.

>
> > 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.
>
> Nit: this sentence is a bit long. Maybe "however they ..." and "hence
> they ..." could start new sentences instead.

Okay thank you.
>
> > Simplify the process of retrieving and returning the desired token by
> > using strchr() to isolate the token and xmemdupz() to return a copy of the
> > token.
> > This removes the roundabout way of splitting the string into strbufs, just
> > to return the token.
>
> Nit: this last sentence should either be in its own paragraph, in
> which case there should be a blank line before it, or it should be
> part of the previous paragraph.

Okay noted.
>
> > Reported-by: Junio Hamano <gitster@pobox.com>
> > Helped-by: Christian Couder <christian.couder@gmail.com>
> > Helped-by: Junio Hamano <gitster@pobox.com>
>
> Nit: Junio reviews all the patches and adds his own "Signed-off-by:"
> to the patch that are accepted, so there is no need to also mention
> him in an "Helped-by:" trailer like this.

Okay.

>
> > Helped-by: Krisoffer Haughsbakk
>
> I think you mean "Kristoffer Haugsbakk". Please spell his name
> correctly and provide his email address like for everyone else.

Oh I'm so sorry about that his.
I'll correct this.
Apologies.

>
> [...]
>
> > @@ -845,13 +844,17 @@ 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])
> > -               die_errno(_("failed to get the ssh fingerprint for key '%s'"),
> > +       begin = fingerprint_stdout.buf;
>
> `begin` is set here, but not used below...
>
> > +       delim = strchr(fingerprint_stdout.buf, ' ');

Ahh sorry I was supposed to use it here


> > +       if (!delim)
> > +               die_errno(_("failed to get the ssh fingerprint for key %s"),
> >                           signing_key);
>
> (This might be an issue that already existed, but I wonder if using
> die_errno() instead of just die() is the right thing to do here.
> Shouldn't we check errno before splitting?)

Okay sorry I'm a bit confused.
I should have used die() instead since we have not split the string yet?

>
> > -       fingerprint_ret = strbuf_detach(fingerprint[1], NULL);
> > -       strbuf_list_free(fingerprint);
> > +       begin = delim + 1;
>
> ... before here, where `begin` is set to something else. This means it
> was useless to set it to `fingerprint_stdout.buf` before.

Yes I should have used it in the first call to strchr ()

>
> > +       delim = strchr(begin, ' ');
> > +       if (!delim)
> > +           die_errno(_("failed to get the ssh fingerprint for key %s"),
> > +                         signing_key);
> > +       fingerprint_ret = xmemdupz(begin, delim - begin);
> >         strbuf_release(&fingerprint_stdout);
> >         return fingerprint_ret;
>
> I think this could be `return xmemdupz(begin, delim - begin);`, so we
> could get rid of `fingerprint_ret`.

Yes I saw your response already.

Thank you.

Apologies for resending if you're getting the mail again.
My first mail to the list was rejected.

Re: [Outreachy PATCH v4 2/2] gpg-interface: do not use misdesigned strbuf_split*() [Part 2]

[Bello Olamide]

On Tue, 21 Oct 2025 at 08:01, Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Tue, Oct 21, 2025 at 12:57 AM Olamide Caleb Bello <belkid98@gmail.com> wrote:
>
> [...]
>
> > Reported-by: Junio Hamano <gitster@pobox.com>
> > Helped-by: Christian Couder <christian.couder@gmail.com>
> > Helped-by: Junio Hamano <gitster@pobox.com>
> > Helped-by: Krisoffer Haughsbakk
>
> I won't repeat the issues that are the same as in patch 1/2, but
> please correct them.
>
> [...]

Yes, thank you.

I will fix them.
>
> > @@ -887,19 +887,21 @@ 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)) {
> > +               begin = key_stdout.buf;
> > +               new_line = strchr(begin, '\n');
> > +               first_line = xmemdupz(begin, new_line - begin);
>
> What if no \n character is found by strchr()?
In the original code, just the first line of a possible two lines
is returned.
So since we need just the first and if no new line is found,
I can do
                char *end = new_line  ? new_line : strchr(begin, '\0');
                first_line = xmemdupz(begin, end);

Does this work?

Thanks
Bello

Re: [Outreachy PATCH v4 1/2] gpg-interface: do not use misdesigned strbuf_split*()

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

>> Reported-by: Junio Hamano <gitster@pobox.com>
>> Helped-by: Christian Couder <christian.couder@gmail.com>
>> Helped-by: Junio Hamano <gitster@pobox.com>
>
> Nit: Junio reviews all the patches and adds his own "Signed-off-by:"
> to the patch that are accepted, so there is no need to also mention
> him in an "Helped-by:" trailer like this.

Just this point.

Somebody is later expected to sign-off on the patch has little to do
with who is on Helped-by: lines.  The provenance of whatever help by
others the author incorporated into the patch is covered by the
author's sign-off.  The sign-off I would give to this patch later is
only to certify that I received a signed-off patch and commited
verbatim, or with my own changes that can be shared under the same
DCO.

Not that I think the amount of help I gave is substantial enough to
deserve a "Helped-by" credit, though.

For everything else in your review, I would very much appreciate you
for helping the author of the patch.  Thanks.

Re: [Outreachy PATCH v4 0/2] do not use strbuf_split*()

[Junio C Hamano]

Bello Olamide <belkid98@gmail.com> writes:

>> > 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.
>>
>> I am not sure taking this commit as an example is really useful now
>> that the string_list_split*() functions are not used in your series
>> anymore. Maybe you can find a more relevant example commit in Junio's
>> series?
>>
>> [...]
>
> Okay. Thank you. I will take a closer look at the series and look for
> a more suitable
> reference.

Thanks Christian for lending us very sharp eyes.

What we do in these patches now is closer in spirit to d6fd08bd
(sub-process: do not use strbuf_split*(), 2025-07-31), I think, in
that we do not split things into an array of strbuf, and instead
parse things out in place as much as possible.

Re: [Outreachy PATCH v4 0/2] do not use strbuf_split*()

[Bello Olamide]

On Tue, 21 Oct 2025 at 18:13, Junio C Hamano <gitster@pobox.com> wrote:
>
> Bello Olamide <belkid98@gmail.com> writes:
>
> >> > 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.
> >>
> >> I am not sure taking this commit as an example is really useful now
> >> that the string_list_split*() functions are not used in your series
> >> anymore. Maybe you can find a more relevant example commit in Junio's
> >> series?
> >>
> >> [...]
> >
> > Okay. Thank you. I will take a closer look at the series and look for
> > a more suitable
> > reference.
>
> Thanks Christian for lending us very sharp eyes.
>
> What we do in these patches now is closer in spirit to d6fd08bd
> (sub-process: do not use strbuf_split*(), 2025-07-31), I think, in
> that we do not split things into an array of strbuf, and instead
> parse things out in place as much as possible.

Thank you very much Junio for pointing me to a suitable reference.
Makes my work easier :)

Bello

[Outreachy PATCH v5 0/2] do not use misdesigned 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.

Commit d6fd08bd (sub-process: do not use strbuf_split*(), 2025-07-31) for
example, in the series, observes that the subprocess_read_status() reads
one packet line and tries to find "status=<foo>" by splitting the line
into two strbufs which is an overkill to extract <foo>.

This series continues on this cleanup, by replacing instances of
strbuf_split_max() with strchr() to get the required token around the
delimiter where the token from the split is merely returned as char *
and not strbufs and no edits are done on them.
This makes the code cleaner, faster and more efficient.

Tests have also been performed on the commits on Github CI. The link is shown below

https://github.com/git/git/pull/2080

Changes in v5:
==============
 - Modify commit messages to provide proper context for commit reference
 - Correct reviewer's name and email address
 - correct code logic by assigning `fingerprint_stdout.buf` to `begin` in
   first call to strchr in patch 1
 - Modify logic in call to strchr() when no '\n' is found by passing '\0'
   to get the end of first line.
 - use die() in place of die_errno() in failed calls to strchr(), retaining
   die_errno() before the string is split


Olamide Caleb Bello (2):
  gpg-interface: do not use misdesigned strbuf_split*()
  gpg-interface: do not use misdesigned strbuf_split*()

 gpg-interface.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

Range diff versus v4
====================

1:  2879d9be36 ! 1:  df8fbbd3a5 gpg-interface: do not use misdesigned strbuf_split*()
    @@ Commit message
     
         In get_ssh_finger_print(), the output of the `ssh-keygen` command is
         put into `fingerprint_stdout` strbuf.
    -
    -    The string in fingerprint_stdout is then split into up to 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
    +    The string in `fingerprint_stdout` is then split into up to 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.
     
         Simplify the process of retrieving and returning the desired token by
         using strchr() to isolate the token and xmemdupz() to return a copy of the
    -    token.
    -    This removes the roundabout way of splitting the string into strbufs, just
    -    to return the token.
    +    token. This removes the roundabout way of splitting the string into
    +    strbufs just to return the token.
     
         Reported-by: Junio Hamano <gitster@pobox.com>
         Helped-by: Christian Couder <christian.couder@gmail.com>
    -    Helped-by: Junio Hamano <gitster@pobox.com>
    -    Helped-by: Krisoffer Haughsbakk
    +    Helped-by: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>
         Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
     
      ## gpg-interface.c ##
    @@ gpg-interface.c: static char *get_ssh_key_fingerprint(const char *signing_key)
     -	if (!fingerprint[1])
     -		die_errno(_("failed to get the ssh fingerprint for key '%s'"),
     +	begin = fingerprint_stdout.buf;
    -+	delim = strchr(fingerprint_stdout.buf, ' ');
    ++	delim = strchr(begin, ' ');
     +	if (!delim)
    -+		die_errno(_("failed to get the ssh fingerprint for key %s"),
    ++		die(_("failed to get the ssh fingerprint for key %s"),
      			  signing_key);
     -
     -	fingerprint_ret = strbuf_detach(fingerprint[1], NULL);
    @@ gpg-interface.c: static char *get_ssh_key_fingerprint(const char *signing_key)
     +	begin = delim + 1;
     +	delim = strchr(begin, ' ');
     +	if (!delim)
    -+	    die_errno(_("failed to get the ssh fingerprint for key %s"),
    ++	    die(_("failed to get the ssh fingerprint for key %s"),
     +			  signing_key);
     +	fingerprint_ret = xmemdupz(begin, delim - begin);
      	strbuf_release(&fingerprint_stdout);
2:  a830de15ec ! 2:  5df667227b gpg-interface: do not use misdesigned strbuf_split*() [Part 2]
    @@ Metadata
     Author: Olamide Caleb Bello <belkid98@gmail.com>
     
      ## Commit message ##
    -    gpg-interface: do not use misdesigned strbuf_split*() [Part 2]
    +    gpg-interface: do not use misdesigned strbuf_split*()
     
         In get_default_ssh_signing_key(), the default ssh signing key is
         retrieved in `key_stdout` buf, which is then split using
    @@ Commit message
     
         Reported-by: Junio Hamano <gitster@pobox.com>
         Helped-by: Christian Couder <christian.couder@gmail.com>
    -    Helped-by: Junio Hamano <gitster@pobox.com>
    -    Helped-by: Krisoffer Haughsbakk
    +    Helped-by: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>
         Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
     
      ## gpg-interface.c ##
    @@ gpg-interface.c: static char *get_default_ssh_signing_key(void)
      	int n;
      	char *default_key = NULL;
      	const char *literal_key = NULL;
    -+	char *begin, *new_line, *first_line;
    ++	char *begin, *new_line, *first_line, *end;
      
      	if (!ssh_default_key_command)
      		die(_("either user.signingkey or gpg.ssh.defaultKeyCommand needs to be configured"));
    @@ gpg-interface.c: static char *get_default_ssh_signing_key(void)
     -		if (keys[0] && is_literal_ssh_key(keys[0]->buf, &literal_key)) {
     +		begin = key_stdout.buf;
     +		new_line = strchr(begin, '\n');
    -+		first_line = xmemdupz(begin, new_line - begin);
    ++		end = new_line ? new_line : strchr(begin, '\0');
    ++		first_line = xmemdupz(begin, end - begin);
     +		if (is_literal_ssh_key(first_line, &literal_key)) {
      			/*
      			 * We only use `is_literal_ssh_key` here to check validity

-- 
2.51.0.463.g79cf913ea9

[Outreachy PATCH v5 1/2] gpg-interface: do not use misdesigned strbuf_split*()

[Olamide Caleb Bello]

In get_ssh_finger_print(), the output of the `ssh-keygen` command is
put into `fingerprint_stdout` strbuf.
The string in `fingerprint_stdout` is then split into up to 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.

Simplify the process of retrieving and returning the desired token by
using strchr() to isolate the token and xmemdupz() to return a copy of the
token. This removes the roundabout way of splitting the string into
strbufs just to return the token.

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

diff --git a/gpg-interface.c b/gpg-interface.c
index 2f4f0e32cb..917081abac 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -821,8 +821,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;
-	char *fingerprint_ret;
+	char *fingerprint_ret, *begin, *delim;
 	const char *literal_key = NULL;
 
 	/*
@@ -845,13 +844,17 @@ 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])
-		die_errno(_("failed to get the ssh fingerprint for key '%s'"),
+	begin = fingerprint_stdout.buf;
+	delim = strchr(begin, ' ');
+	if (!delim)
+		die(_("failed to get the ssh fingerprint for key %s"),
 			  signing_key);
-
-	fingerprint_ret = strbuf_detach(fingerprint[1], NULL);
-	strbuf_list_free(fingerprint);
+	begin = delim + 1;
+	delim = strchr(begin, ' ');
+	if (!delim)
+	    die(_("failed to get the ssh fingerprint for key %s"),
+			  signing_key);
+	fingerprint_ret = xmemdupz(begin, delim - begin);
 	strbuf_release(&fingerprint_stdout);
 	return fingerprint_ret;
 }
-- 
2.51.0.463.g79cf913ea9

[Outreachy PATCH v5 2/2] gpg-interface: do not use misdesigned strbuf_split*()

[Olamide Caleb Bello]

In get_default_ssh_signing_key(), the default ssh signing key is
retrieved in `key_stdout` buf, which is then split using
strbuf_split_max() into up to two strbufs at a new line and the first
strbuf 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.

Simplify the process of retrieving and returning the desired line by
using strchr() to isolate the line and xmemdupz() to return a copy of the
line.
This removes the roundabout way of splitting the string into strbufs, just
to return the line.

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

diff --git a/gpg-interface.c b/gpg-interface.c
index 917081abac..ad6ce58da8 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -865,12 +865,12 @@ 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;
 	char *key_command = NULL;
 	const char **argv;
 	int n;
 	char *default_key = NULL;
 	const char *literal_key = NULL;
+	char *begin, *new_line, *first_line, *end;
 
 	if (!ssh_default_key_command)
 		die(_("either user.signingkey or gpg.ssh.defaultKeyCommand needs to be configured"));
@@ -887,19 +887,22 @@ 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)) {
+		begin = key_stdout.buf;
+		new_line = strchr(begin, '\n');
+		end = new_line ? new_line : strchr(begin, '\0');
+		first_line = xmemdupz(begin, end - begin);
+		if (is_literal_ssh_key(first_line, &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 = first_line;
 		} else {
+			free(first_line);
 			warning(_("gpg.ssh.defaultKeyCommand succeeded but returned no keys: %s %s"),
 				key_stderr.buf, key_stdout.buf);
 		}
 
-		strbuf_list_free(keys);
 	} else {
 		warning(_("gpg.ssh.defaultKeyCommand failed: %s %s"),
 			key_stderr.buf, key_stdout.buf);
-- 
2.51.0.463.g79cf913ea9

Re: [Outreachy PATCH v5 1/2] gpg-interface: do not use misdesigned strbuf_split*()

[Christian Couder]

On Wed, Oct 22, 2025 at 2:40 PM Olamide Caleb Bello <belkid98@gmail.com> wrote:
>
> In get_ssh_finger_print(), the output of the `ssh-keygen` command is
> put into `fingerprint_stdout` strbuf.
> The string in `fingerprint_stdout` is then split into up to 3 strbufs

Nit: it's not clear if the first sentence of this commit message is
part of the same paragraph as the second sentence or not. If you
reroll this patch, I would suggest making it clearly part of the same
paragraph like this:

"In get_ssh_finger_print(), the output of the `ssh-keygen` command is
put into `fingerprint_stdout` strbuf. The string in `fingerprint_stdout` is
then split into up to 3 strbufs using strbuf_split_max(). However..."

Otherwise this patch looks fine to me.

Thanks.

Re: [Outreachy PATCH v5 2/2] gpg-interface: do not use misdesigned strbuf_split*()

[Christian Couder]

On Wed, Oct 22, 2025 at 2:40 PM Olamide Caleb Bello <belkid98@gmail.com> wrote:

[...]

> Simplify the process of retrieving and returning the desired line by
> using strchr() to isolate the line and xmemdupz() to return a copy of the
> line.
> This removes the roundabout way of splitting the string into strbufs, just
> to return the line.

Nit: here also I think it should be clear that these last two
sentences are in the same paragraph.

[...]

> @@ -887,19 +887,22 @@ 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)) {
> +               begin = key_stdout.buf;
> +               new_line = strchr(begin, '\n');
> +               end = new_line ? new_line : strchr(begin, '\0');
> +               first_line = xmemdupz(begin, end - begin);

That works but I wonder if something like the following is not a bit better:

               if (new_line)
                       first_line = xmemdupz(begin, new_line - begin);
               else
                       first_line = xstrdup(begin);

Thanks.

Re: [Outreachy PATCH v5 2/2] gpg-interface: do not use misdesigned strbuf_split*()

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

>> @@ -887,19 +887,22 @@ 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)) {
>> +               begin = key_stdout.buf;
>> +               new_line = strchr(begin, '\n');
>> +               end = new_line ? new_line : strchr(begin, '\0');
>> +               first_line = xmemdupz(begin, end - begin);
>
> That works but I wonder if something like the following is not a bit better:
>
>                if (new_line)
>                        first_line = xmemdupz(begin, new_line - begin);
>                else
>                        first_line = xstrdup(begin);

Yeah, that is certainly much easier to understand without even
reading and thinking.

Thanks.

Re: [Outreachy PATCH v5 1/2] gpg-interface: do not use misdesigned strbuf_split*()

[Bello Olamide]

On Wed, 22 Oct 2025 at 14:56, Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Wed, Oct 22, 2025 at 2:40 PM Olamide Caleb Bello <belkid98@gmail.com> wrote:
> >
> > In get_ssh_finger_print(), the output of the `ssh-keygen` command is
> > put into `fingerprint_stdout` strbuf.
> > The string in `fingerprint_stdout` is then split into up to 3 strbufs
>
> Nit: it's not clear if the first sentence of this commit message is
> part of the same paragraph as the second sentence or not. If you
> reroll this patch, I would suggest making it clearly part of the same
> paragraph like this:
>
> "In get_ssh_finger_print(), the output of the `ssh-keygen` command is
> put into `fingerprint_stdout` strbuf. The string in `fingerprint_stdout` is
> then split into up to 3 strbufs using strbuf_split_max(). However..."
>
> Otherwise this patch looks fine to me.
>
> Thanks.

Okay thank you very much

Bello

Re: [Outreachy PATCH v5 2/2] gpg-interface: do not use misdesigned strbuf_split*()

[Bello Olamide]

On Wed, 22 Oct 2025 at 15:04, Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Wed, Oct 22, 2025 at 2:40 PM Olamide Caleb Bello <belkid98@gmail.com> wrote:
>
> [...]
>
> > Simplify the process of retrieving and returning the desired line by
> > using strchr() to isolate the line and xmemdupz() to return a copy of the
> > line.
> > This removes the roundabout way of splitting the string into strbufs, just
> > to return the line.
>
> Nit: here also I think it should be clear that these last two
> sentences are in the same paragraph.

Okay

>
> [...]
>
> > @@ -887,19 +887,22 @@ 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)) {
> > +               begin = key_stdout.buf;
> > +               new_line = strchr(begin, '\n');
> > +               end = new_line ? new_line : strchr(begin, '\0');
> > +               first_line = xmemdupz(begin, end - begin);
>
> That works but I wonder if something like the following is not a bit better:
>
>                if (new_line)
>                        first_line = xmemdupz(begin, new_line - begin);
>                else
>                        first_line = xstrdup(begin);

Ah yes.
It is much better.
Thank you very much for your guide.
I have already learnt a lot in this series.

Bello

[Outreachy PATCH v6 0/2] do not use misdesigned 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.

Commit d6fd08bd (sub-process: do not use strbuf_split*(), 2025-07-31) for
example, in the series, observes that the subprocess_read_status() reads
one packet line and tries to find "status=<foo>" by splitting the line
into two strbufs which is an overkill to extract <foo>.

This series continues on this cleanup, by replacing instances of
strbuf_split_max() with strchr() to get the required token around the
delimiter where the token from the split is merely returned as char *
and not strbufs and no edits are done on them.
This makes the code cleaner, faster and more efficient.

Tests have also been performed on the commits on Github CI. The link is shown below

https://github.com/git/git/pull/2080

Changes in v6
=============
- Modify commit messages to have proper structure
- Changed logic in get_default_ssh_signing_key() to use xmemdupz() if
  key has '\n' and xstrdup() if not.

Olamide Caleb Bello (2):
  gpg-interface: do not use misdesigned strbuf_split*()
  gpg-interface: do not use misdesigned strbuf_split*()

 gpg-interface.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

Range diff versus v5
====================

1:  df8fbbd3a5 ! 1:  92fc78c203 gpg-interface: do not use misdesigned strbuf_split*()
    @@ Commit message
         gpg-interface: do not use misdesigned strbuf_split*()

         In get_ssh_finger_print(), the output of the `ssh-keygen` command is
    -    put into `fingerprint_stdout` strbuf.
    -    The string in `fingerprint_stdout` is then split into up to 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.
    +    put into `fingerprint_stdout` strbuf. The string in `fingerprint_stdout`
    +    is then split into up to 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.

         Simplify the process of retrieving and returning the desired token by
         using strchr() to isolate the token and xmemdupz() to return a copy of the
2:  5df667227b ! 2:  e52855242c gpg-interface: do not use misdesigned strbuf_split*()
    @@ Commit message

         Simplify the process of retrieving and returning the desired line by
         using strchr() to isolate the line and xmemdupz() to return a copy of the
    -    line.
    -    This removes the roundabout way of splitting the string into strbufs, just
    -    to return the line.
    +    line. This removes the roundabout way of splitting the string into
    +    strbufs, just to return the line.

         Reported-by: Junio Hamano <gitster@pobox.com>
         Helped-by: Christian Couder <christian.couder@gmail.com>
    @@ gpg-interface.c: static char *get_default_ssh_signing_key(void)
      	int n;
      	char *default_key = NULL;
      	const char *literal_key = NULL;
    -+	char *begin, *new_line, *first_line, *end;
    ++	char *begin, *new_line, *first_line;

      	if (!ssh_default_key_command)
      		die(_("either user.signingkey or gpg.ssh.defaultKeyCommand needs to be configured"));
    @@ gpg-interface.c: static char *get_default_ssh_signing_key(void)
     -		if (keys[0] && is_literal_ssh_key(keys[0]->buf, &literal_key)) {
     +		begin = key_stdout.buf;
     +		new_line = strchr(begin, '\n');
    -+		end = new_line ? new_line : strchr(begin, '\0');
    -+		first_line = xmemdupz(begin, end - begin);
    ++		if (new_line)
    ++			first_line = xmemdupz(begin, new_line - begin);
    ++		else
    ++			first_line = xstrdup(begin);
     +		if (is_literal_ssh_key(first_line, &literal_key)) {
      			/*
      			 * We only use `is_literal_ssh_key` here to check validity

--
2.51.0.463.g79cf913ea9

[Outreachy PATCH v6 1/2] gpg-interface: do not use misdesigned strbuf_split*()

[Olamide Caleb Bello]

In get_ssh_finger_print(), the output of the `ssh-keygen` command is
put into `fingerprint_stdout` strbuf. The string in `fingerprint_stdout`
is then split into up to 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.

Simplify the process of retrieving and returning the desired token by
using strchr() to isolate the token and xmemdupz() to return a copy of the
token. This removes the roundabout way of splitting the string into
strbufs just to return the token.

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

diff --git a/gpg-interface.c b/gpg-interface.c
index 2f4f0e32cb..917081abac 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -821,8 +821,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;
-	char *fingerprint_ret;
+	char *fingerprint_ret, *begin, *delim;
 	const char *literal_key = NULL;
 
 	/*
@@ -845,13 +844,17 @@ 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])
-		die_errno(_("failed to get the ssh fingerprint for key '%s'"),
+	begin = fingerprint_stdout.buf;
+	delim = strchr(begin, ' ');
+	if (!delim)
+		die(_("failed to get the ssh fingerprint for key %s"),
 			  signing_key);
-
-	fingerprint_ret = strbuf_detach(fingerprint[1], NULL);
-	strbuf_list_free(fingerprint);
+	begin = delim + 1;
+	delim = strchr(begin, ' ');
+	if (!delim)
+	    die(_("failed to get the ssh fingerprint for key %s"),
+			  signing_key);
+	fingerprint_ret = xmemdupz(begin, delim - begin);
 	strbuf_release(&fingerprint_stdout);
 	return fingerprint_ret;
 }
-- 
2.51.0.463.g79cf913ea9

[Outreachy PATCH v6 2/2] gpg-interface: do not use misdesigned strbuf_split*()

[Olamide Caleb Bello]

In get_default_ssh_signing_key(), the default ssh signing key is
retrieved in `key_stdout` buf, which is then split using
strbuf_split_max() into up to two strbufs at a new line and the first
strbuf 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.

Simplify the process of retrieving and returning the desired line by
using strchr() to isolate the line and xmemdupz() to return a copy of the
line. This removes the roundabout way of splitting the string into
strbufs, just to return the line.

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

diff --git a/gpg-interface.c b/gpg-interface.c
index 917081abac..d1e88da8c1 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -865,12 +865,12 @@ 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;
 	char *key_command = NULL;
 	const char **argv;
 	int n;
 	char *default_key = NULL;
 	const char *literal_key = NULL;
+	char *begin, *new_line, *first_line;
 
 	if (!ssh_default_key_command)
 		die(_("either user.signingkey or gpg.ssh.defaultKeyCommand needs to be configured"));
@@ -887,19 +887,24 @@ 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)) {
+		begin = key_stdout.buf;
+		new_line = strchr(begin, '\n');
+		if (new_line)
+			first_line = xmemdupz(begin, new_line - begin);
+		else
+			first_line = xstrdup(begin);
+		if (is_literal_ssh_key(first_line, &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 = first_line;
 		} else {
+			free(first_line);
 			warning(_("gpg.ssh.defaultKeyCommand succeeded but returned no keys: %s %s"),
 				key_stderr.buf, key_stdout.buf);
 		}
 
-		strbuf_list_free(keys);
 	} else {
 		warning(_("gpg.ssh.defaultKeyCommand failed: %s %s"),
 			key_stderr.buf, key_stdout.buf);
-- 
2.51.0.463.g79cf913ea9

Re: [Outreachy PATCH v6 0/2] do not use misdesigned strbuf_split*()

[Junio C Hamano]

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

> Changes in v6
> =============
> - Modify commit messages to have proper structure
> - Changed logic in get_default_ssh_signing_key() to use xmemdupz() if
>   key has '\n' and xstrdup() if not.

This round looks good to me.  Christian, should we declare victory
and mark it for 'next' now?

Thanks.

>
> Olamide Caleb Bello (2):
>   gpg-interface: do not use misdesigned strbuf_split*()
>   gpg-interface: do not use misdesigned strbuf_split*()
>
>  gpg-interface.c | 34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
>
> Range diff versus v5
> ====================
>
> 1:  df8fbbd3a5 ! 1:  92fc78c203 gpg-interface: do not use misdesigned strbuf_split*()
>     @@ Commit message
>          gpg-interface: do not use misdesigned strbuf_split*()
>
>          In get_ssh_finger_print(), the output of the `ssh-keygen` command is
>     -    put into `fingerprint_stdout` strbuf.
>     -    The string in `fingerprint_stdout` is then split into up to 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.
>     +    put into `fingerprint_stdout` strbuf. The string in `fingerprint_stdout`
>     +    is then split into up to 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.
>
>          Simplify the process of retrieving and returning the desired token by
>          using strchr() to isolate the token and xmemdupz() to return a copy of the
> 2:  5df667227b ! 2:  e52855242c gpg-interface: do not use misdesigned strbuf_split*()
>     @@ Commit message
>
>          Simplify the process of retrieving and returning the desired line by
>          using strchr() to isolate the line and xmemdupz() to return a copy of the
>     -    line.
>     -    This removes the roundabout way of splitting the string into strbufs, just
>     -    to return the line.
>     +    line. This removes the roundabout way of splitting the string into
>     +    strbufs, just to return the line.
>
>          Reported-by: Junio Hamano <gitster@pobox.com>
>          Helped-by: Christian Couder <christian.couder@gmail.com>
>     @@ gpg-interface.c: static char *get_default_ssh_signing_key(void)
>       	int n;
>       	char *default_key = NULL;
>       	const char *literal_key = NULL;
>     -+	char *begin, *new_line, *first_line, *end;
>     ++	char *begin, *new_line, *first_line;
>
>       	if (!ssh_default_key_command)
>       		die(_("either user.signingkey or gpg.ssh.defaultKeyCommand needs to be configured"));
>     @@ gpg-interface.c: static char *get_default_ssh_signing_key(void)
>      -		if (keys[0] && is_literal_ssh_key(keys[0]->buf, &literal_key)) {
>      +		begin = key_stdout.buf;
>      +		new_line = strchr(begin, '\n');
>     -+		end = new_line ? new_line : strchr(begin, '\0');
>     -+		first_line = xmemdupz(begin, end - begin);
>     ++		if (new_line)
>     ++			first_line = xmemdupz(begin, new_line - begin);
>     ++		else
>     ++			first_line = xstrdup(begin);
>      +		if (is_literal_ssh_key(first_line, &literal_key)) {
>       			/*
>       			 * We only use `is_literal_ssh_key` here to check validity
>
> --
> 2.51.0.463.g79cf913ea9

Re: [Outreachy PATCH v6 0/2] do not use misdesigned strbuf_split*()

[Christian Couder]

On Thu, Oct 23, 2025 at 6:27 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Olamide Caleb Bello <belkid98@gmail.com> writes:
>
> > Changes in v6
> > =============
> > - Modify commit messages to have proper structure
> > - Changed logic in get_default_ssh_signing_key() to use xmemdupz() if
> >   key has '\n' and xstrdup() if not.
>
> This round looks good to me.  Christian, should we declare victory
> and mark it for 'next' now?

Yeah, v6 looks good to me too. Acked.

Thanks.