From: Junio C Hamano Date: Mon, 20 Oct 2025 15:09:16 GMT Subject: Re: [Outreachy PATCH v3 1/2] gpg-interface: replace strbuf_split*() with string_list_split*() Message-ID: In-Reply-To: Bello Olamide 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 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 ;-)