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

From: Christian Couder <christian.couder@gmail.com>
Date: Oct 21, 2025, 06:46 UTC
Message-ID: <CAP8UFD1J_B9W62bv=0yccQNGahkv2vco3arQOs0oe0DccdTeYg@mail.gmail.com>
In-Reply-To: <2879d9be3659a9c1ea554fff7814507caae24b65.1760997183.git.belkid98@gmail.com>

On Tue, Oct 21, 2025 at 12:56 AM Olamide Caleb Bello <belkid98@gmail.com> 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.

Show 5 quoted lines

> 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 <gitster@pobox.com>
> Helped-by: Christian Couder <christian.couder@gmail.com>
> Helped-by: Junio Hamano <gitster@pobox.com>

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.

[...]

Show 8 quoted lines

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

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

Show 7 quoted lines

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