do not use strbuf_split*()

The patch series by Junio Hamano with link below,
https://public-inbox.org/git/[email protected]/,
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 <[email protected]>

## 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 <[email protected]>
- Reported-by: Junio Hamano <[email protected]>
+ Reported-by: Junio Hamano <[email protected]>

+ Helped-by: Junio Hamano <[email protected]>
+ Helped-by: Krisoffer Haughsbakk
+ Signed-off-by: Olamide Caleb Bello <[email protected]>

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

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.

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

index 2f4f0e32cb..1d793a56d2 100644
@@ -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

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

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.

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

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.

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.

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

[...]

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.

Okay noted.

Sorry about that. I'll fix it.

Okay thank you.

Okay noted.

Okay.

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

Ahh sorry I was supposed to use it here

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

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

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.

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.

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.

---
 gpg-interface.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

index 1d793a56d2..420e3a6646 100644
@@ -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

On Tue, Oct 21, 2025 at 12:57 AM Olamide Caleb Bello <[email protected]> wrote:

[...]

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.

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

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

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?

[...]

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/[email protected]/#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.

Okay thank you.

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

Okay. Noted.

Thank you very much for your time and review Christian.

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

Bello

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

The patch series by Junio Hamano with link below,
https://public-inbox.org/git/[email protected]/,
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.

- Helped-by: Junio Hamano <[email protected]>
- Helped-by: Krisoffer Haughsbakk
+ Helped-by: Kristoffer Haugsbakk <[email protected]>

## 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 <[email protected]>

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

- Helped-by: Junio Hamano <[email protected]>
- Helped-by: Krisoffer Haughsbakk
+ Helped-by: Kristoffer Haugsbakk <[email protected]>

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

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.

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

index 2f4f0e32cb..917081abac 100644
@@ -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

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

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.

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

index 917081abac..ad6ce58da8 100644
@@ -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

On Wed, Oct 22, 2025 at 2:40 PM Olamide Caleb Bello <[email protected]> wrote:

[...]

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

[...]

               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.

Okay

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

Bello

The patch series by Junio Hamano with link below,
https://public-inbox.org/git/[email protected]/,
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 <[email protected]>
         Helped-by: Christian Couder <[email protected]>
    @@ 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

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.

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

index 2f4f0e32cb..917081abac 100644
@@ -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

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.

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

index 917081abac..d1e88da8c1 100644
@@ -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

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

Thanks.

Yeah, v6 looks good to me too. Acked.

Thanks.