Re: [Outreachy PATCH v2 1/2] gpg-interface: replace strbuf_split_max() with string_list_split()

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

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

> @@ -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_DUP;
>  	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(&split, fingerprint_stdout.buf, " ", 2) != 3)

The original splits the thing into upto 3 pieces, but only complains
if the second piece is NULL (i.e. we said "up to 3", but there was
not even one place to split, and the request to split_max gave the
one thing as one piece).  IOW, the original code will happily accept
if the finterprint_stdout is split only into two, not three.

The updated code asks to split at at most two places (yes, it is a
confusing API, but if you split at two places, you will end up with
three pieces), and insists that the split results in three pieces. 

So the rewrite tightens the error condition.

Was the original code too loose in detecting an error, and does this
patch tightens the condition "while at it"?  Or was the original
code correct to expect that there are legitimate cases where the
payload in finterprint_stdout only contains two pieces, and it was
the right thing to do to accept when fingerprint[1] is not NULL but
fingerprint[2] is NULL?

This is a genuine question.  I haven't studied the code path to
reach this point in the code flow, I don't know what the data in
fingerprint_stdout is supposed to look like, so I do not know the
answer to the question (in other words, it cannot be an oblique way
to point out that the updated code is wrong or anything like that).

>  		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);
>  	strbuf_release(&fingerprint_stdout);

Since this code releases fingerprint_stdout before leaving, and
returns a fresh copy of a split piece, it may make more sense to use
string_list_split_in_place(), which does not have to allocate extra
strings while it does its work, unlike string_list_split().

>  	return fingerprint_ret;
>  }