Re: [Outreachy PATCH v2 1/2] gpg-interface: replace strbuf_split_max() with string_list_split()

>
> On Thu, Oct 16, 2025 at 7:27 PM 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).
>
> Yeah, I think the problem is that the commit message should explain
> how this whole issue is dealt with. Currently it talks about passing
> "2" to string_list_split() instead of passing "3" to
> strbuf_split_max(), but it says nothing about dealing with the return
> value from string_list_split().

>
> It seems to me that fingerprint_stdout should contain something like:
>
> 4096 SHA256:PelI2esT2xZlv20wJJyYOkQsli5RMK79oJ2VxqYb2PA
> christian.couder@gmail.com (RSA)
>
> and it looks like the 'key_size fingerprint_hash:fingerprint comment
> (key_type)' format is the standard `ssh-keygen -l` output.
>
> So I think it's safe to say that we should expect
> `string_list_split(..., 2)` to return 3. That should work even if
> there is no comment in the key file.

>
> But on the other hand, I think it's also acceptable to say that we
> just want to keep the same behavior as the original code and check its
> return value with `> 1` since we only need the second element
> resulting from the split.