From: Christian Couder Date: Wed, 15 Oct 2025 15:01:30 GMT Subject: Re: [PATCH 1/2] [Outreachy] gpg-interface: replace strbuf_split with string_list_split Message-ID: In-Reply-To: On Wed, Oct 15, 2025 at 4:19 AM Olamide Caleb Bello wrote: > > get_ssh_finger_print() accepts a signing key and then uses pipe_command "pipe_command" is a function too so it's better to use "()" when talking about it like in "get_ssh_finger_print()". In the commit message subject, if it doesn't make it too long, I think it might be better to also use "()" when talking about functions. > to execute the ssh-keygen command, gets its output and sets it in > fingerprint_stdout. Anyway I am not sure we need so many details about what get_ssh_finger_print() does before the split. Maybe saying something like the following is enough: "In get_ssh_finger_print(), the output of the `ssh-keygen` command is put into `fingerprint_stdout`." > The string in fingerprint_stdout is then split into 3 strbufs using > 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, hence they do not need to be strbufs. It might be interesting to say that the fingerprint token is returned as a `char *` not a strbuf. > Use string_list_split instead for simplicity. Here also using "()" could make it clearer that "string_list_split" is a function. > Note that strbuf_split_max uses 3 to specify the number of tokens to Here also using "()" could help a bit. > extract from the string, while string_list_split uses 2 because it specifies Here also using "()" could help a bit. > the number of times the split will be done on the string, so 2 gives 3 tokens > as it is in the initial instance. Maybe: s/initial/original/ > Signed-off-by: Olamide Caleb Bello [...] > @@ -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) <= 1) According to its doc, string_list_split() returns the the number of substrings appended to the list. And you said in the commit message that it should give 3 tokens, so I think the above line should be: if (string_list_split(&split, fingerprint_stdout.buf, " ", 2) < 3) or even: if (string_list_split(&split, fingerprint_stdout.buf, " ", 2) != 3) > die_errno(_("failed to get the ssh fingerprint for key '%s'"), > signing_key); Except for the above points, your patch look good to me. Thanks.