Re: [Outreachy PATCH v3 1/2] gpg-interface: replace strbuf_split*() with string_list_split*()

>
> Olamide Caleb Bello <belkid98@gmail.com> writes:
>
> > In get_ssh_finger_print(), the output of the `ssh-keygen` command is
> > put into `fingerprint_stdout
>
> Something lost at the end?  I'd assume
>
>         ... into `fingerpritn_stdout` strbuf.

>
> and tweak the copy I received locally before applying.
>
> > The string in fingerprint_stdout is then split into 3 strbufs using
>
> "into up to 3 strbufs", I think.  If we do not say so here, ...
>
> > 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.
> >
> > Use string_list_split_in_place() instead for simplicity.
> >
> > Note that strbuf_split_max() uses 3 to specify the number of tokens to
> > extract from the string, while string_list_split_in_place() uses 2
> > because it specifies the number of times the split will be done on
> > the string, so 2 gives 3 tokens as it is in the original instance.
> >
> > string_list_split_in_place() returns the number of substrings added to
> > the `split.items` so for a successful split of the string in
> > fingerprint_stdout, at least two items should be added to split.items
>
> ... this "at least two items" would become contradictory.

>
> > so we can always be certain that the substring at index 1 is the ssh
> > fingerprint even if the key owner's identity part is missing from the
> > string in fingerprint_stdout.
> >
> > Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
> > Reported-by: Junio Hamano <gitster@pobox.com>
> > Helped-by: Christian Couder <christian.couder@gmail.com>
> > Helped-by: Junio Hamano <gitster@pobox.com>
> > ---
> >  gpg-interface.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index 2f4f0e32cb..cb182f4c11 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -14,6 +14,7 @@
> >  #include "sigchain.h"
> >  #include "tempfile.h"
> >  #include "alias.h"
> > +#include "string-list.h"
> >
> >  static int git_gpg_config(const char *, const char *,
> >                         const struct config_context *, void *);
> > @@ -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_NODUP;
> >       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_in_place(&split, fingerprint_stdout.buf, " ", 2) <= 1)
>
> This may be just me, but when we expect at least 2, I would find it
> more natural if we said "if (count < 2) then error", rather "if
> (count <= 1) then error".  I'll let it pass, as there is nothing
> mathematically incorrect here ;-).
>
> >               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);
>
> 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?

>
> That may or may not be outside the scope of this topic, which is to
> reduce the calls to a misdesigned strbuf_split*() API functions.
>
> Thanks.
>
> >       strbuf_release(&fingerprint_stdout);
> >       return fingerprint_ret;
> >  }