Re: [Outreachy PATCH v2 0/2] gpg-interface.c: use string_list_split() instead of strbuf_split_max()

From: Christian Couder <christian.couder@gmail.com>
Date: Oct 17, 2025, 07:58 UTC
Message-ID: <CAP8UFD0Lzmt5ayQ3iSXtmJohDawmCEiu1r8Gq2mKTMy-1gX_fw@mail.gmail.com>
In-Reply-To: <cover.1760571220.git.belkid98@gmail.com>

On Thu, Oct 16, 2025 at 3:04 AM Olamide Caleb Bello <belkid98@gmail.com> wrote:

Show 10 quoted lines

>
> 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.

> 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.

> 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.

Thanks.