From: Bello Olamide Date: Fri, 17 Oct 2025 10:07:34 GMT Subject: Re: [Outreachy PATCH v2 1/2] gpg-interface: replace strbuf_split_max() with string_list_split() Message-ID: In-Reply-To: On Fri, 17 Oct 2025 at 10:15, Christian Couder wrote: > > 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(). Yes, I will add this to the commit message too. > > 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. Yes, in this case fingerprint_stdout will contain something like: 2048 SHA256:BoQYpZX7zNZFOc9BSprkHoIgPhMTT1D5d5tqbwXZ1hA no comment (RSA) > > 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. Okay this is noted. Thank you. Bello