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

[Junio C Hamano <gitster@pobox.com>]

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

> In get_default_ssh_signing_key(), the default ssh signing key is
> retrieved in `key_stdout`, which is then split using
> strbuf_split_max() into two tokens
>
> The string in `key_stdout` is then split using strbuf_split_max() into
> two tokens at a new line and the first token is returned as a `char *`
> and not a strbuf.
> This makes the function lack the use of strbuf API as no edits are
> performed on the split tokens.
>
> Replace strbuf_split_max() with string_list_split_in_place() for
> simplicity
>
> Note that strbuf_split_max() uses `2` to indicate the number of tokens
> to extract from the string, while string_list_split_in_place() uses `1`
> to specify the number of times the split will be done on the string,
> so 1 gives 2 tokens as it is in the original instance.
>
> string_list_split_in_place() returns the number of substrings added to the
> list keys.items, so we check that at least one substring is added to the
> list since we just want to return the first substring.
>
> Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
> Reported-by: Junio Hamano <gister@pobox.com>
> Helped-by: Christian Couder <christian.couder@gmail.com>
> ---
>  gpg-interface.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Exactly the same comment as [1/2] (including the part about the
first paragraph seemingly missing something at the end ;-).

Also, it may not be necessary to highlight the quirky way the
string_list_split*() function counts numbers again, as it is done in
the previous patch so readers have already been warned against it.

And the same comment applies about the round-about way the original
was written in the first place.  Isn't it merely the matter of
finding the first line-feed and making a copy of a string up to that
point?

Perhaps we would be better off if we revise the theme of the topic
"use string_list_split*() to replace strbuf_split*()" to "do not use
misdesigned strbuf_split*() function" and do the rewrite without
using string_list_split*() after all?  It may result in a much
cleaner and simpler code at the end.

Thanks.

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