Re: [Outreachy PATCH v3 1/2] gpg-interface: replace strbuf_split*() with string_list_split*()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 19, 2025, 15:52 UTC
- Message-ID
- <xmqqqzuy3n3k.fsf@gitster.g>
- In-Reply-To
- <7da4fded535984faea52d5f88793d3c8e47c0091.1760869186.git.belkid98@gmail.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, ...
Show 15 quoted lines
> 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.
Show 40 quoted lines
> 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 ;-).
Show 7 quoted lines
> 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; > }