From: Christian Couder Date: Wed, 15 Oct 2025 15:28:34 GMT Subject: Re: [PATCH 2/2] [Outreachy] gpg-interface: use string_list_split instead of strbuf_split Message-ID: In-Reply-To: On Wed, Oct 15, 2025 at 4:19 AM Olamide Caleb Bello wrote: > > get_default_ssh_signing_key() gets the signing key via the pipe_command and s/the pipe_command/pipe_command()/ > stores the output in key_stdout. It's not very clear from the sentence if the signing key is the output or not. Maybe something like: "In get_default_ssh_signing_key(), the default ssh signing key is retrieved in `key_stdout`, which is then split using strbuf_split_max() into two tokens." > The output string is then split using strbuf_split_max into two tokens at a > new line and the first token is returned. Here also it might be interesting to know that the first token is returned as a `char *`, not a strbuf. > This makes the function lack the > use of strbuf API as no edits was performed on the split tokens. s/was performed/are performed/ > Replace strbuf_split_max with string_list_split for simplicity. Here also, using "()" could help a bit as it would make it clear that "strbuf_split_max" and "string_list_split" are functions. > Signed-off-by: Olamide Caleb Bello > @@ -884,19 +884,15 @@ static char *get_default_ssh_signing_key(void) > &key_stderr, 0); > > if (!ret) { > - keys = strbuf_split_max(&key_stdout, '\n', 2); > - if (keys[0] && is_literal_ssh_key(keys[0]->buf, &literal_key)) { > - /* > - * We only use `is_literal_ssh_key` here to check validity > - * The prefix will be stripped when the key is used. > - */ Why is this comment removed? It's not clear to me that it's not valid anymore. > - default_key = strbuf_detach(keys[0], NULL); > + if (string_list_split(&keys, key_stdout.buf, "\n", 1) > 0 && In the commit message you should explain, like you did for the previous commit, why "1" is passed to string_list_split() while "2" was passed to strbuf_split_max(). Also I think that, instead of "> 0", the tests should be ">=2" or "== 2". Or, if an output that contains no new line is valid, then that should be explained in the commit message. > + is_literal_ssh_key(keys.items[0].string, &literal_key)) { > + default_key = xstrdup(keys.items[0].string); Thanks.