From: Bello Olamide Date: Mon, 20 Oct 2025 06:32:18 GMT Subject: Re: [Outreachy PATCH v3 1/2] gpg-interface: replace strbuf_split*() with string_list_split*() Message-ID: In-Reply-To: On Sun, 19 Oct 2025 at 16:52, Junio C Hamano wrote: > > Olamide Caleb Bello 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. Yes, thank you. > > 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. Yes, noted. > > > 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 > > Reported-by: Junio Hamano > > Helped-by: Christian Couder > > Helped-by: Junio Hamano > > --- > > 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? Okay something like this which just finds the desired token and returns a copy? char *begin = fingerprint_stdout.buf; char *end = begin + fingerprint_stdout.len; char *space, *start, *endtok; space = memchr(begin, ' ', end-begin); if (!space) die_errno(_("failed to get the ssh fingerprint for key '%s'"), signing_key); start = space + 1; while (start < end && (*start = ' ' || *start == '\t')) start++; if (start >=end) die_errno(_("failed to get the ssh fingerprint for key '%s'"), signing_key); endtok = start; while (endtok < end && *endtok != ' ' && *endtok != '\t' && *endtok !== '\r' && *endtok != '\n') endtok++; if (endtok == start) die_errno(_("failed to get the ssh fingerprint for key '%s'"), signing_key); fingerprint_ret = xmemdupz(start, endtok - start); return fingerptint_ret > > 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; > > }