From: Bello Olamide <belkid98@gmail.com>
Date: Fri, 17 Oct 2025 10:53:52 GMT
Subject: Re: [Outreachy PATCH v2 1/2] gpg-interface: replace strbuf_split_max() with string_list_split()
Message-ID: <CAD=f0L8-+Ut4eTbd4Mh-vPJV7KwNxcVUY1-xCrB+m8mT0xkkow@mail.gmail.com>
In-Reply-To: <xmqqms5q4v0r.fsf@gitster.g>

On Thu, 16 Oct 2025 at 18:27, Junio C Hamano <gitster@pobox.com> wrote:
>
> Olamide Caleb Bello <belkid98@gmail.com> writes:
>
> > @@ -821,7 +822,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key)
> >       struct child_process ssh_keygen = CHILD_PROCESS_INIT;
> >       int ret = -1;
> >       struct strbuf fingerprint_stdout = STRBUF_INIT;
> > -     struct strbuf **fingerprint;
> > +     struct string_list split = STRING_LIST_INIT_DUP;
> >       char *fingerprint_ret;
> >       const char *literal_key = NULL;
> >
> > @@ -845,13 +846,12 @@ 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])
> > +     if (string_list_split(&split, fingerprint_stdout.buf, " ", 2) != 3)
>
> The original splits the thing into upto 3 pieces, but only complains
> if the second piece is NULL (i.e. we said "up to 3", but there was
> not even one place to split, and the request to split_max gave the
> one thing as one piece).  IOW, the original code will happily accept
> if the finterprint_stdout is split only into two, not three.
>
> The updated code asks to split at at most two places (yes, it is a
> confusing API, but if you split at two places, you will end up with
> three pieces), and insists that the split results in three pieces.
>
> So the rewrite tightens the error condition.
>
> Was the original code too loose in detecting an error, and does this
> patch tightens the condition "while at it"?  Or was the original
> code correct to expect that there are legitimate cases where the
> payload in finterprint_stdout only contains two pieces, and it was
> the right thing to do to accept when fingerprint[1] is not NULL but
> fingerprint[2] is NULL?
>
> This is a genuine question.  I haven't studied the code path to
> reach this point in the code flow, I don't know what the data in
> fingerprint_stdout is supposed to look like, so I do not know the
> answer to the question (in other words, it cannot be an oblique way
> to point out that the updated code is wrong or anything like that).
>
> >               die_errno(_("failed to get the ssh fingerprint for key '%s'"),
> >                         signing_key);
> >
> > -     fingerprint_ret = strbuf_detach(fingerprint[1], NULL);
> > -     strbuf_list_free(fingerprint);
> > +     fingerprint_ret = xstrdup(split.items[1].string);
> > +     string_list_clear(&split, 0);
> >       strbuf_release(&fingerprint_stdout);
>
> Since this code releases fingerprint_stdout before leaving, and
> returns a fresh copy of a split piece, it may make more sense to use
> string_list_split_in_place(), which does not have to allocate extra
> strings while it does its work, unlike string_list_split().

Thank you Junio for the review.

I will take a look at string_list_split_in_place().

Bello