gpg-interface.c: use string_list_split instead of strbuf_split_max

get_ssh_finger_print() accepts a signing key and then uses pipe_command

to execute the ssh-keygen command, gets its output and sets it in 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.

Use string_list_split 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 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 initial instance.

Signed-off-by Olamide Caleb Bello <[email protected]>

get_default_ssh_signing_key() gets the signing key via the pipe_command and

stores the output in key_stdout.

The output string is then split using strbuf_split_max into two tokens at a new line and the first token is returned.

This makes the function lack the
use of strbuf API as no edits was performed on the split tokens.

Replace strbuf_split_max with string_list_split for simplicity.

The array of strbufs that calls to strbuf_split_max provides are merely used to store the list of tokens gotten from the split and no edit are done on these resulting splits making the strbuf_split_max unideal for this usecase.

This patchset replaces these instances with the modern string_list_split

Quoted 2 lines · Olamide Caleb Bello

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.

Quoted 2 lines

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."

Quoted 4 lines

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.

Quoted 1 line

Use string_list_split instead for simplicity.

Here also using "()" could make it clearer that "string_list_split" is a function.

Quoted 1 line

Note that strbuf_split_max uses 3 to specify the number of tokens to

Here also using "()" could help a bit.

Quoted 1 line

extract from the string, while string_list_split uses 2 because it specifies

Here also using "()" could help a bit.

Quoted 2 lines

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/

Quoted 2 lines

Signed-off-by Olamide Caleb Bello <[email protected]>

[...]

gpg-interface.c static char *get_ssh_key_fingerprint(const char *signing_key) · line 849

Quoted 7 lines

gpg-interface.c +1 -2

@@ -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)

Review static char *get_ssh_key_fingerprint(const char *signing_key) · line 851

Quoted 2 lines

(quoted diff) +0 -0

                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.

Quoted 2 lines · Olamide Caleb Bello

get_default_ssh_signing_key() gets the signing key via the pipe_command and

s/the pipe_command/pipe_command()/

Quoted 1 line

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."

Quoted 3 lines

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.

Quoted 2 lines

This makes the function lack the
use of strbuf API as no edits was performed on the split tokens.

s/was performed/are performed/

Quoted 1 line

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.

gpg-interface.c static char *get_default_ssh_signing_key(void) · line 892

Quoted 12 lines

Signed-off-by Olamide Caleb Bello <[email protected]>

gpg-interface.c +0 -6

@@ -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.

gpg-interface.c static char *get_default_ssh_signing_key(void) · line 887

Quoted 2 lines

gpg-interface.c +1 -1

-                       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.

gpg-interface.c static char *get_default_ssh_signing_key(void) · line 889

Quoted 3 lines

gpg-interface.c +2 -0

+                       is_literal_ssh_key(keys.items[0].string, &literal_key)) {
+                               default_key = xstrdup(keys.items[0].string);

Thanks.

Quoted 7 lines · Olamide Caleb Bello

The array of strbufs that calls to strbuf_split_max provides are merely used to store the list of tokens gotten from the split and no edit are done on these resulting splits making the strbuf_split_max unideal for this usecase.

This patchset replaces these instances with the modern string_list_split

A cover letter like this should be used to provide broad context for the changes. So if there are there patch series or merged commits that started doing something similar, it would be a good idea to mention them or give a link. (To properly mention a commit, you can use something like: git show -s --pretty="tformat:%h (%s, %ad)" --date=short <commit>.) If there are mailing list discussions were the topic was discussed, it could be interesting to mention and link them too.

Thanks.