From: Bello Olamide Date: Tue, 21 Oct 2025 11:18:36 GMT Subject: Re: [Outreachy PATCH v4 1/2] gpg-interface: do not use misdesigned strbuf_split*() Message-ID: In-Reply-To: On Tue, 21 Oct 2025 at 07:46, Christian Couder wrote: > > On Tue, Oct 21, 2025 at 12:56 AM Olamide Caleb Bello wrote: > > > > In get_ssh_finger_print(), the output of the `ssh-keygen` command is > > put into `fingerprint_stdout` strbuf. Okay noted. > > Nit: I think this sentence doesn't need to be in its own paragraph. It > could be at the start of the paragraph below. > > > The string in fingerprint_stdout is then split into up to 3 strbufs using > > Nit: above the variable `fingerprint_stdout` was quoted, but now it's > not quoted anymore. I think it would be more consistent to quote it > here too. Sorry about that. I'll fix it. > > > 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. > > Nit: this sentence is a bit long. Maybe "however they ..." and "hence > they ..." could start new sentences instead. Okay thank you. > > > Simplify the process of retrieving and returning the desired token by > > using strchr() to isolate the token and xmemdupz() to return a copy of the > > token. > > This removes the roundabout way of splitting the string into strbufs, just > > to return the token. > > Nit: this last sentence should either be in its own paragraph, in > which case there should be a blank line before it, or it should be > part of the previous paragraph. Okay noted. > > > Reported-by: Junio Hamano > > Helped-by: Christian Couder > > Helped-by: Junio Hamano > > Nit: Junio reviews all the patches and adds his own "Signed-off-by:" > to the patch that are accepted, so there is no need to also mention > him in an "Helped-by:" trailer like this. Okay. > > > Helped-by: Krisoffer Haughsbakk > > I think you mean "Kristoffer Haugsbakk". Please spell his name > correctly and provide his email address like for everyone else. Oh I'm so sorry about that his. I'll correct this. Apologies. > > [...] > > > @@ -845,13 +844,17 @@ 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]) > > - die_errno(_("failed to get the ssh fingerprint for key '%s'"), > > + begin = fingerprint_stdout.buf; > > `begin` is set here, but not used below... > > > + delim = strchr(fingerprint_stdout.buf, ' '); Ahh sorry I was supposed to use it here > > + if (!delim) > > + die_errno(_("failed to get the ssh fingerprint for key %s"), > > signing_key); > > (This might be an issue that already existed, but I wonder if using > die_errno() instead of just die() is the right thing to do here. > Shouldn't we check errno before splitting?) Okay sorry I'm a bit confused. I should have used die() instead since we have not split the string yet? > > > - fingerprint_ret = strbuf_detach(fingerprint[1], NULL); > > - strbuf_list_free(fingerprint); > > + begin = delim + 1; > > ... before here, where `begin` is set to something else. This means it > was useless to set it to `fingerprint_stdout.buf` before. Yes I should have used it in the first call to strchr () > > > + delim = strchr(begin, ' '); > > + if (!delim) > > + die_errno(_("failed to get the ssh fingerprint for key %s"), > > + signing_key); > > + fingerprint_ret = xmemdupz(begin, delim - begin); > > strbuf_release(&fingerprint_stdout); > > return fingerprint_ret; > > I think this could be `return xmemdupz(begin, delim - begin);`, so we > could get rid of `fingerprint_ret`. Yes I saw your response already. Thank you. Apologies for resending if you're getting the mail again. My first mail to the list was rejected.