From: Christian Couder Date: Tue, 21 Oct 2025 06:46:22 GMT Subject: Re: [Outreachy PATCH v4 1/2] gpg-interface: do not use misdesigned strbuf_split*() Message-ID: In-Reply-To: <2879d9be3659a9c1ea554fff7814507caae24b65.1760997183.git.belkid98@gmail.com> 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. 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. > 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. > 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. > 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. > Helped-by: Krisoffer Haughsbakk I think you mean "Kristoffer Haugsbakk". Please spell his name correctly and provide his email address like for everyone else. [...] > @@ -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, ' '); > + 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?) > - 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. > + 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`. Thanks.