From: Bello Olamide <belkid98@gmail.com>
Date: Mon, 20 Oct 2025 18:12:08 GMT
Subject: Re: [Outreachy PATCH v3 1/2] gpg-interface: replace strbuf_split*() with string_list_split*()
Message-ID: <CAD=f0L9u9=VAwO31qTS7GKXDrt9HjwVCHEFqDGtpv8HuWyP=nA@mail.gmail.com>
In-Reply-To: <xmqq5xc9390j.fsf@gitster.g>

On Mon, 20 Oct 2025 at 16:09, Junio C Hamano <gitster@pobox.com> wrote:
>
> Bello Olamide <belkid98@gmail.com> writes:
>
> >> > -     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?
>
> 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);

Okay thank you

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

Okay thank you very much for the guidance.

Belo