Re: [Outreachy PATCH v4 1/2] gpg-interface: do not use misdesigned strbuf_split*()

On Tue, 21 Oct 2025 at 07:46, Christian Couder
[off-list ref] wrote:

On Tue, Oct 21, 2025 at 12:56 AM Olamide Caleb Bello [off-list ref] wrote:

quoted

In get_ssh_finger_print(), the output of the `ssh-keygen` command is
put into `fingerprint_stdout` strbuf.

Okay noted.

Nit: I think this sentence doesn't need to be in its own paragraph. It
could be at the start of the paragraph below.

quoted

The string in fingerprint_stdout is then split into up to 3 strbufs using

Nit: above the variable `fingerprint_stdout` was quoted, but now it's
not quoted anymore. I think it would be more consistent to quote it
here too.

Sorry about that. I'll fix it.

quoted

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.

Nit: this sentence is a bit long. Maybe "however they ..." and "hence
they ..." could start new sentences instead.

Okay thank you.

quoted

Simplify the process of retrieving and returning the desired token by
using strchr() to isolate the token and xmemdupz() to return a copy of the
token.
This removes the roundabout way of splitting the string into strbufs, just
to return the token.

Nit: this last sentence should either be in its own paragraph, in
which case there should be a blank line before it, or it should be
part of the previous paragraph.

Okay noted.

quoted

Reported-by: Junio Hamano <redacted>
Helped-by: Christian Couder [off-list ref]
Helped-by: Junio Hamano [off-list ref]

Nit: Junio reviews all the patches and adds his own "Signed-off-by:"
to the patch that are accepted, so there is no need to also mention
him in an "Helped-by:" trailer like this.

Okay.

quoted

Helped-by: Krisoffer Haughsbakk

I think you mean "Kristoffer Haugsbakk". Please spell his name
correctly and provide his email address like for everyone else.

Oh I'm so sorry about that his.
I'll correct this.
Apologies.

[...]

quoted

@@ -845,13 +844,17 @@ 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])
-               die_errno(_("failed to get the ssh fingerprint for key '%s'"),
+       begin = fingerprint_stdout.buf;

`begin` is set here, but not used below...

quoted

+       delim = strchr(fingerprint_stdout.buf, ' ');

Ahh sorry I was supposed to use it here

quoted

+       if (!delim)
+               die_errno(_("failed to get the ssh fingerprint for key %s"),
                          signing_key);

(This might be an issue that already existed, but I wonder if using
die_errno() instead of just die() is the right thing to do here.
Shouldn't we check errno before splitting?)

Okay sorry I'm a bit confused.
I should have used die() instead since we have not split the string yet?

quoted

-       fingerprint_ret = strbuf_detach(fingerprint[1], NULL);
-       strbuf_list_free(fingerprint);
+       begin = delim + 1;

... before here, where `begin` is set to something else. This means it
was useless to set it to `fingerprint_stdout.buf` before.

Yes I should have used it in the first call to strchr ()

quoted

+       delim = strchr(begin, ' ');
+       if (!delim)
+           die_errno(_("failed to get the ssh fingerprint for key %s"),
+                         signing_key);
+       fingerprint_ret = xmemdupz(begin, delim - begin);
        strbuf_release(&fingerprint_stdout);
        return fingerprint_ret;

I think this could be `return xmemdupz(begin, delim - begin);`, so we
could get rid of `fingerprint_ret`.

Yes I saw your response already.

Thank you.

Apologies for resending if you're getting the mail again.
My first mail to the list was rejected.