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

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

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

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