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

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

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

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

Something lost at the end?  I'd assume

	... into `fingerpritn_stdout` strbuf.

and tweak the copy I received locally before applying.

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

"into up to 3 strbufs", I think.  If we do not say so here, ...

> 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.
>
> Use string_list_split_in_place() instead for simplicity.
>
> Note that strbuf_split_max() uses 3 to specify the number of tokens to
> extract from the string, while string_list_split_in_place() uses 2
> because it specifies the number of times the split will be done on
> the string, so 2 gives 3 tokens as it is in the original instance.
>
> string_list_split_in_place() returns the number of substrings added to
> the `split.items` so for a successful split of the string in
> fingerprint_stdout, at least two items should be added to split.items

... this "at least two items" would become contradictory.

> so we can always be certain that the substring at index 1 is the ssh
> fingerprint even if the key owner's identity part is missing from the
> string in fingerprint_stdout.
>
> Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
> Reported-by: Junio Hamano <gitster@pobox.com>
> Helped-by: Christian Couder <christian.couder@gmail.com>
> Helped-by: Junio Hamano <gitster@pobox.com>
> ---
>  gpg-interface.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 2f4f0e32cb..cb182f4c11 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -14,6 +14,7 @@
>  #include "sigchain.h"
>  #include "tempfile.h"
>  #include "alias.h"
> +#include "string-list.h"
>  
>  static int git_gpg_config(const char *, const char *,
>  			  const struct config_context *, void *);
> @@ -821,7 +822,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key)
>  	struct child_process ssh_keygen = CHILD_PROCESS_INIT;
>  	int ret = -1;
>  	struct strbuf fingerprint_stdout = STRBUF_INIT;
> -	struct strbuf **fingerprint;
> +	struct string_list split = STRING_LIST_INIT_NODUP;
>  	char *fingerprint_ret;
>  	const char *literal_key = NULL;
>  
> @@ -845,13 +846,12 @@ static char *get_ssh_key_fingerprint(const char *signing_key)
>  		die_errno(_("failed to get the ssh fingerprint for key '%s'"),
>  			  signing_key);
>  
> -	fingerprint = strbuf_split_max(&fingerprint_stdout, ' ', 3);
> -	if (!fingerprint[1])
> +	if (string_list_split_in_place(&split, fingerprint_stdout.buf, " ", 2) <= 1)

This may be just me, but when we expect at least 2, I would find it
more natural if we said "if (count < 2) then error", rather "if
(count <= 1) then error".  I'll let it pass, as there is nothing
mathematically incorrect here ;-).

>  		die_errno(_("failed to get the ssh fingerprint for key '%s'"),
>  			  signing_key);
>  
> -	fingerprint_ret = strbuf_detach(fingerprint[1], NULL);
> -	strbuf_list_free(fingerprint);
> +	fingerprint_ret = xstrdup(split.items[1].string);
> +	string_list_clear(&split, 0);

OK.  This is a straight-forward rewrite that is fairly faithful to
the original.

But I wonder why the original was written in such a convoluted way
to just extract the first part of a string that is space delimited
tokens.  It is obviously not your fault that the original is written
that way, bit I would have expected it to be done more like this:

    char *begin = fingerprint_stdout.buf;
    char *delim = strchr(begin, ' ');
    if (!delim)
	die_errno("Barf!");
    fingerprint_ret = xmemdupz(begin, end - begin);

Am I missing something?

That may or may not be outside the scope of this topic, which is to
reduce the calls to a misdesigned strbuf_split*() API functions.

Thanks.

>  	strbuf_release(&fingerprint_stdout);
>  	return fingerprint_ret;
>  }