From: Bello Olamide Date: Fri, 17 Oct 2025 09:54:05 GMT Subject: Re: [Outreachy PATCH v2 0/2] gpg-interface.c: use string_list_split() instead of strbuf_split_max() Message-ID: In-Reply-To: On Fri, 17 Oct 2025 at 08:58, Christian Couder wrote: > > On Thu, Oct 16, 2025 at 3:04 AM Olamide Caleb Bello wrote: > > > > Commit 2efe707054 (wt-status: avoid strbuf_split*(), 2025-07-31) noticed > > that the array of strbufs that calls to strbuf_split*() provides are merely > > used to store the strings gotten from the split and no edit are > > done on these resulting strings making the strbuf_split*() unideal > > for this usecase, with the string_list_split*() being a more suitable option. > > > > The patch series by Junio Hamano can be seen in the link below. > > > > https://public-inbox.org/git/20250731225433.4028872-1-gitster@pobox.com/ > > This description is probably good enough as-is, but here are some > comments that might help improve it if you want if you send a v3. > > The way you explain things up to here, it might seem only one commit > in that series is about avoiding strbuf_split*(). But in fact the > commit you mention is the first one in that series which is named "do > not overuse strbuf_split*()" and contains 11 patches. > > So I think it would be a bit better if, instead of speaking about that > commit first, this cover letter started with a link to that patch > series and explained the purpose of the whole series. You may then > mention one or more commits in the series as examples of commits where > strbuf_split*() is replaced with string_list_split*() though if you > want. Yes, this approach would be better. Thank you > > > This series continues on this path by replacing instances of > > strbuf_split_max() with string_list_split() where the string from the > > split is merely returned as char * and no edits are done on them. > > Yeah, some commits in the series do that, but not all. Yes I will reword this too. > > > Changes since v1 > > ================ > > - Added commit reference and link to patch series for previous work > > done on the subject > > In a cover letter we are interested in the changes in the patch series > since the previous version, not the changes in the cover letter. This > is because the cover letter itself is not merged (except perhaps its > first few sentences that might be reused in the merge commit, but this > is more advanced, so don't take this into account for now) when a > patch series is merged. So a cover letter is more about giving context > to reviewers and inviting them to review the patch series. > > So here it would be nice if there were things like: > > - a summary of the changes in the patch series since v1, > - a range diff between v1 and v2, > - a link to a CI platform where the v2 has been pushed and the CI > tests have been performed. > > This would help make reviewer confident that the series is in a much > better shape compared to v1 and reviewers' comments on v1 have been > taken into account. > Thank you very much for the review and suggestions. I will reorganise and send a corrected v3. Bello.