From: Bello Olamide Date: Wed, 15 Oct 2025 21:56:09 GMT Subject: Re: [PATCH 1/2] [Outreachy] gpg-interface: replace strbuf_split with string_list_split Message-ID: In-Reply-To: On Wed, 15 Oct 2025 at 16:01, Christian Couder wrote: > > 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/ Thank you very much for your review, I make the adjustments. > > > 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) > Okay, thank you very much Christian. My thinking here is that after the split, it is expected that the fingerprint token will be at index 1 in the list so string_list_split() must append at least 2 tokens. so if it appends 1 token or none, I assume the fingerprint was not set in the list. I agree that < 3or != 3 is a better approach. Thank you. > > 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.