From: Christian Couder Date: Fri, 17 Oct 2025 09:15:37 GMT Subject: Re: [Outreachy PATCH v2 1/2] gpg-interface: replace strbuf_split_max() with string_list_split() Message-ID: In-Reply-To: On Thu, Oct 16, 2025 at 7:27 PM Junio C Hamano wrote: > > Olamide Caleb Bello 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. > > die_errno(_("failed to get the ssh fingerprint for key '%s'"), > > signing_key); > >