gpg-interface.c: use string_list_split*() instead of strbuf_split*()

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

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 <[email protected]>

Reported-by Junio Hamano <[email protected]>

Helped-by Christian Couder <[email protected]>

Helped-by Junio Hamano <[email protected]>

---
 gpg-interface.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

gpg-interface.c +2 -1

index 2f4f0e32cb..cb182f4c11 100644
@@ -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;
 

gpg-interface.c +0 -0

 	strbuf_release(&fingerprint_stdout);
 	return fingerprint_ret;
 }

Quoted 2 lines · Olamide Caleb Bello

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.

and tweak the copy I received locally before applying.

Quoted 1 line

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

Quoted 12 lines

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.

Quoted 3 lines

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.

Quoted 35 lines

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 <[email protected]>

Reported-by Junio Hamano <[email protected]>

Helped-by Christian Couder <[email protected]>

Helped-by Junio Hamano <[email protected]>

---
 gpg-interface.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

gpg-interface.c +2 -1

index 2f4f0e32cb..cb182f4c11 100644
@@ -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;
 

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

Quoted 6 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_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 ;-).

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

Quoted 7 lines

gpg-interface.c +2 -2

              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?

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.

Quoted 3 lines

gpg-interface.c +0 -0

      strbuf_release(&fingerprint_stdout);
      return fingerprint_ret;
 }

Quoted 13 lines

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

Quoted 4 lines

gpg-interface.c +2 -2

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

Quoted 7 lines

    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++;

What I was missing was that we use fingerprint[1], not fingerprint[0]. So we need to do the strchr() twice, i.e.

     char *begin = fingerprint_stdout.buf;
     char *delim = strchr(begin, ' ');
     if (!delim)
         die_errno("Barf!");
     begin = delim + 1
     delim = strchr(begin, ' ');
     if (!delim)
         die_errno("Barf!");
     fingerprint_ret = xmemdupz(begin, end - begin);

Quoted 12 lines · Junio C Hamano

What I was missing was that we use fingerprint[1], not fingerprint[0]. So we need to do the strchr() twice, i.e.

     char *begin = fingerprint_stdout.buf;
     char *delim = strchr(begin, ' ');
     if (!delim)
         die_errno("Barf!");
     begin = delim + 1
     delim = strchr(begin, ' ');
     if (!delim)
         die_errno("Barf!");
     fingerprint_ret = xmemdupz(begin, end - begin);

Ouch, of course "end" is not declared anywhere and it is an obvious typo of delim. Sorry for not proofreading enough.

Quoted 13 lines · Bello Olamide

Quoted 13 lines

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

Quoted 4 lines

gpg-interface.c +2 -2

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

Quoted 7 lines

Quoted 7 lines

    char *begin = fingerprint_stdout.buf;
    char *delim = strchr(begin, ' ');
    if (!delim)
        die_errno("Barf!");
    fingerprint_ret = xmemdupz(begin, end - begin);

Am I missing something?

What I was missing was that we use fingerprint[1], not fingerprint[0]. So we need to do the strchr() twice, i.e.

     char *begin = fingerprint_stdout.buf;
     char *delim = strchr(begin, ' ');
     if (!delim)
         die_errno("Barf!");
     begin = delim + 1
     delim = strchr(begin, ' ');
     if (!delim)
         die_errno("Barf!");
     fingerprint_ret = xmemdupz(begin, end - begin);

Quoted 8 lines

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;

Quoted 7 lines

   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++;

The original does not seem to care and uses the whole fingerprint[1].buf; do we really care about tabs? The same for looking at CR or LF.

Even if we cared, we shouldn't have to open code strcspn() like this ;-)

[snip]

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

Reported-by Junio Hamano <[email protected]>

Helped-by Christian Couder <[email protected]>

Helped-by Junio Hamano <[email protected]>

---
 gpg-interface.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

[snip]

Quoted 3 lines · Olamide Caleb Bello

[snip]

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

Signoff should go at the end of the commit message. You’re saying that you are signing off on the changes as well as what was written in the commit message before that line. If I later add mine:

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

Signed-off-by ME

I would be saying that I am signing off on the changes as well as the previous lines in the commit message, including that line that you wrote.

Quoted 4 lines

Reported-by Junio Hamano <[email protected]>

Helped-by Christian Couder <[email protected]>

Helped-by Junio Hamano <[email protected]>

Quoted 10 lines · Kristoffer Haugsbakk

Signoff should go at the end of the commit message. You’re saying that you are signing off on the changes as well as what was written in the commit message before that line. If I later add mine:

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

Signed-off-by ME

I would be saying that I am signing off on the changes as well as the previous lines in the commit message, including that line that you wrote.

Please just to make sure I get you correctly, you are saying the Signoff should be the last thing in the commit message.

Like I should put it below the other tags so that if you also want to add yours, It can easily go below mine and it would mean you are signing off on the changes including my own Signoff?

Quoted 1 line

Quoted 3 lines

Quoted 3 lines

Reported-by Junio Hamano <[email protected]>

Helped-by Christian Couder <[email protected]>

Helped-by Junio Hamano <[email protected]>

It should go below here instead?

Quoted 21 lines · Bello Olamide

Quoted 10 lines · Kristoffer Haugsbakk

Signoff should go at the end of the commit message. You’re saying that you are signing off on the changes as well as what was written in the commit message before that line. If I later add mine:

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

Signed-off-by ME

I would be saying that I am signing off on the changes as well as the previous lines in the commit message, including that line that you wrote.

Please just to make sure I get you correctly, you are saying the Signoff should be the last thing in the commit message.

Like I should put it below the other tags so that if you also want to add yours, It can easily go below mine and it would mean you are signing off on the changes including my own Signoff?

Quoted 1 line

Quoted 4 lines

Quoted 3 lines

Quoted 3 lines

Reported-by Junio Hamano <[email protected]>

Helped-by Christian Couder <[email protected]>

Helped-by Junio Hamano <[email protected]>

It should go below here instead?

Yes, that is my understanding. :)

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 string in key_stdout is then split using strbuf_split_max() into two tokens at a new line and the first token is returned as a char * and not a strbuf.
This makes the function lack the use of strbuf API as no edits are performed on the split tokens.

Replace strbuf_split_max() with string_list_split_in_place() for simplicity

Note that strbuf_split_max() uses 2 to indicate the number of tokens to extract from the string, while string_list_split_in_place() uses 1 to specify the number of times the split will be done on the string, so 1 gives 2 tokens as it is in the original instance.

string_list_split_in_place() returns the number of substrings added to the list keys.items, so we check that at least one substring is added to the list since we just want to return the first substring.

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

Reported-by Junio Hamano <[email protected]>

Helped-by Christian Couder <[email protected]>

---
 gpg-interface.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

gpg-interface.c +5 -5

index cb182f4c11..3b9d85a5ef 100644
@@ -862,7 +862,7 @@ static char *get_default_ssh_signing_key(void)
 	struct child_process ssh_default_key = CHILD_PROCESS_INIT;
 	int ret = -1;
 	struct strbuf key_stdout = STRBUF_INIT, key_stderr = STRBUF_INIT;
-	struct strbuf **keys;
+	struct string_list keys = STRING_LIST_INIT_NODUP;
 	char *key_command = NULL;
 	const char **argv;
 	int n;

@@ -884,19 +884,19 @@ 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)) {
+		if (string_list_split_in_place(&keys, key_stdout.buf, "\n", 1) > 0 &&
+			is_literal_ssh_key(keys.items[0].string, &literal_key)) {
 			/*
 			 * We only use `is_literal_ssh_key` here to check validity
 			 * The prefix will be stripped when the key is used.
 			 */
-			default_key = strbuf_detach(keys[0], NULL);
+			default_key = xstrdup(keys.items[0].string);
 		} else {
 			warning(_("gpg.ssh.defaultKeyCommand succeeded but returned no keys: %s %s"),
 				key_stderr.buf, key_stdout.buf);
 		}
 
-		strbuf_list_free(keys);
+		string_list_clear(&keys, 0);
 	} else {
 		warning(_("gpg.ssh.defaultKeyCommand failed: %s %s"),
 			key_stderr.buf, key_stdout.buf);

Quoted 22 lines · Olamide Caleb Bello

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 string in key_stdout is then split using strbuf_split_max() into two tokens at a new line and the first token is returned as a char * and not a strbuf.
This makes the function lack the use of strbuf API as no edits are performed on the split tokens.

Replace strbuf_split_max() with string_list_split_in_place() for simplicity

Note that strbuf_split_max() uses 2 to indicate the number of tokens to extract from the string, while string_list_split_in_place() uses 1 to specify the number of times the split will be done on the string, so 1 gives 2 tokens as it is in the original instance.

string_list_split_in_place() returns the number of substrings added to the list keys.items, so we check that at least one substring is added to the list since we just want to return the first substring.

Quoted 6 lines

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

Reported-by Junio Hamano <[email protected]>

Helped-by Christian Couder <[email protected]>

---
 gpg-interface.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Exactly the same comment as [1/2] (including the part about the first paragraph seemingly missing something at the end ;-).

Also, it may not be necessary to highlight the quirky way the string_list_split*() function counts numbers again, as it is done in the previous patch so readers have already been warned against it.

And the same comment applies about the round-about way the original was written in the first place. Isn't it merely the matter of finding the first line-feed and making a copy of a string up to that point?

Perhaps we would be better off if we revise the theme of the topic "use string_list_split*() to replace strbuf_split*()" to "do not use misdesigned strbuf_split*() function" and do the rewrite without using string_list_split*() after all? It may result in a much cleaner and simpler code at the end.

I am just asking to know if something like this should be done within the respective functions or I will need to write functions for each and just call here.

Quoted 3 lines · Bello Olamide

I am just asking to know if something like this should be done within the respective functions or I will need to write functions for each and just call here.

Unlike [1/2] that asked for the second string, this one just wants to discard everything after the first LF, so I am not sure if you need any new helper or hand-rolled loop. Wouldn't strchr() and xmemdupz() that were used in my response to [1/2] sufficient for the purpose of this step, too?