Re: [Outreachy PATCH v3 2/2] gpg-interface: use string_list_split*() instead of strbuf_split*()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 19, 2025, 16:00 UTC
- Message-ID
- <xmqqikga3mqj.fsf@gitster.g>
- In-Reply-To
- <9a6eb6ff8b92a94de990303cc78026029a669cf8.1760869186.git.belkid98@gmail.com>
Olamide Caleb Bello <belkid98@gmail.com> writes:
Show 28 quoted lines
> In get_default_ssh_signing_key(), the default ssh signing key is > retrieved in `key_stdout`, which is then split using > strbuf_split_max() into two tokens > > The string in `key_stdout` is then split using strbuf_split_max() into > two tokens at a new line and the first token is returned as a `char *` > and not a strbuf. > This makes the function lack the use of strbuf API as no edits are > performed on the split tokens. > > Replace strbuf_split_max() with string_list_split_in_place() for > simplicity > > Note that strbuf_split_max() uses `2` to indicate the number of tokens > to extract from the string, while string_list_split_in_place() uses `1` > to specify the number of times the split will be done on the string, > so 1 gives 2 tokens as it is in the original instance. > > string_list_split_in_place() returns the number of substrings added to the > list keys.items, so we check that at least one substring is added to the > list since we just want to return the first substring. > > Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com> > Reported-by: Junio Hamano <gister@pobox.com> > Helped-by: Christian Couder <christian.couder@gmail.com> > --- > gpg-interface.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-)
Exactly the same comment as [1/2] (including the part about the first paragraph seemingly missing something at the end ;-).
Also, it may not be necessary to highlight the quirky way the string_list_split*() function counts numbers again, as it is done in the previous patch so readers have already been warned against it.
And the same comment applies about the round-about way the original was written in the first place. Isn't it merely the matter of finding the first line-feed and making a copy of a string up to that point?
Perhaps we would be better off if we revise the theme of the topic "use string_list_split*() to replace strbuf_split*()" to "do not use misdesigned strbuf_split*() function" and do the rewrite without using string_list_split*() after all? It may result in a much cleaner and simpler code at the end.
Thanks.
Show 37 quoted lines
> diff --git a/gpg-interface.c b/gpg-interface.c
> index cb182f4c11..3b9d85a5ef 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -862,7 +862,7 @@ static char *get_default_ssh_signing_key(void)
> struct child_process ssh_default_key = CHILD_PROCESS_INIT;
> int ret = -1;
> struct strbuf key_stdout = STRBUF_INIT, key_stderr = STRBUF_INIT;
> - struct strbuf **keys;
> + struct string_list keys = STRING_LIST_INIT_NODUP;
> char *key_command = NULL;
> const char **argv;
> int n;
> @@ -884,19 +884,19 @@ static char *get_default_ssh_signing_key(void)
> &key_stderr, 0);
>
> if (!ret) {
> - keys = strbuf_split_max(&key_stdout, '\n', 2);
> - if (keys[0] && is_literal_ssh_key(keys[0]->buf, &literal_key)) {
> + if (string_list_split_in_place(&keys, key_stdout.buf, "\n", 1) > 0 &&
> + is_literal_ssh_key(keys.items[0].string, &literal_key)) {
> /*
> * We only use `is_literal_ssh_key` here to check validity
> * The prefix will be stripped when the key is used.
> */
> - default_key = strbuf_detach(keys[0], NULL);
> + default_key = xstrdup(keys.items[0].string);
> } else {
> warning(_("gpg.ssh.defaultKeyCommand succeeded but returned no keys: %s %s"),
> key_stderr.buf, key_stdout.buf);
> }
>
> - strbuf_list_free(keys);
> + string_list_clear(&keys, 0);
> } else {
> warning(_("gpg.ssh.defaultKeyCommand failed: %s %s"),
> key_stderr.buf, key_stdout.buf);