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

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

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

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

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.

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.

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.

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.

Helped-by: Krisoffer Haughsbakk

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

[...]

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

+       delim = strchr(fingerprint_stdout.buf, ' ');
+       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?)

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

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

Thanks.