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

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

[Olamide Caleb Bello]

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

Changes since v1
================
 - Added commit reference and link to patch series for previous work
 done on the subject


Olamide Caleb Bello (2):
  gpg-interface: replace strbuf_split_max() with string_list_split()
  gpg-interface: use string_list_split() instead of strbuf_split_max()

 gpg-interface.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

-- 
2.51.0.463.g79cf913ea9

[Outreachy PATCH v2 1/2] gpg-interface: replace strbuf_split_max() with string_list_split()

[Olamide Caleb Bello]

In get_ssh_finger_print(), the output of the `ssh-keygen` command is
put into `fingerprint_stdout

The string in fingerprint_stdout is then split into 3 strbufs using
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.

Use string_list_split() instead for simplicity.

Note that strbuf_split_max() uses 3 to specify the number of tokens to
extract from the string, while string_list_split() uses 2 because it specifies
the number of times the split will be done on the string, so 2 gives 3 tokens
as it is in the original instance.

Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
---
 Changes in v2
   - Reword the commit message for clarity
   - Add () to the function names
   - Change the test to ensure the number of tokens appended to the
    list by string_list_split() equals 3
 gpg-interface.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 2f4f0e32cb..989dca7d14 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -14,6 +14,7 @@
 #include "sigchain.h"
 #include "tempfile.h"
 #include "alias.h"
+#include "string-list.h"
 
 static int git_gpg_config(const char *, const char *,
 			  const struct config_context *, void *);
@@ -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)
 		die_errno(_("failed to get the ssh fingerprint for key '%s'"),
 			  signing_key);
 
-	fingerprint_ret = strbuf_detach(fingerprint[1], NULL);
-	strbuf_list_free(fingerprint);
+	fingerprint_ret = xstrdup(split.items[1].string);
+	string_list_clear(&split, 0);
 	strbuf_release(&fingerprint_stdout);
 	return fingerprint_ret;
 }
-- 
2.51.0.463.g79cf913ea9

Re: [Outreachy PATCH v2 1/2] gpg-interface: replace strbuf_split_max() with string_list_split()

[Junio C Hamano]

Olamide Caleb Bello <belkid98@gmail.com> 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).

>  		die_errno(_("failed to get the ssh fingerprint for key '%s'"),
>  			  signing_key);
>  
> -	fingerprint_ret = strbuf_detach(fingerprint[1], NULL);
> -	strbuf_list_free(fingerprint);
> +	fingerprint_ret = xstrdup(split.items[1].string);
> +	string_list_clear(&split, 0);
>  	strbuf_release(&fingerprint_stdout);

Since this code releases fingerprint_stdout before leaving, and
returns a fresh copy of a split piece, it may make more sense to use
string_list_split_in_place(), which does not have to allocate extra
strings while it does its work, unlike string_list_split().

>  	return fingerprint_ret;
>  }

Re: [Outreachy PATCH v2 1/2] gpg-interface: replace strbuf_split_max() with string_list_split()

[Christian Couder]

On Thu, Oct 16, 2025 at 7:27 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Olamide Caleb Bello <belkid98@gmail.com> 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().

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.

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.

> >               die_errno(_("failed to get the ssh fingerprint for key '%s'"),
> >                         signing_key);
> >

Re: [Outreachy PATCH v2 1/2] gpg-interface: replace strbuf_split_max() with string_list_split()

[Bello Olamide]

On Fri, 17 Oct 2025 at 10:15, Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Thu, Oct 16, 2025 at 7:27 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Olamide Caleb Bello <belkid98@gmail.com> 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

Re: [Outreachy PATCH v2 1/2] gpg-interface: replace strbuf_split_max() with string_list_split()

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

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

I do not know why you or anybody can think it is safe from what you
observed above, though.  Who or which page of what manual told us
that the key owner's identity part cannot be missing?

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

Yes, as a code clean-up topic, I think it is the *only* acceptable
thing to do.  Finding the source to justify tightening of the rule
to validate data to insist we have to see 3 pieces can be left to a
separate topic once this series settles.

Thanks.

Re: [Outreachy PATCH v2 1/2] gpg-interface: replace strbuf_split_max() with string_list_split()

[Bello Olamide]

On Thu, 16 Oct 2025 at 18:27, Junio C Hamano <gitster@pobox.com> wrote:
>
> Olamide Caleb Bello <belkid98@gmail.com> 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).
>
> >               die_errno(_("failed to get the ssh fingerprint for key '%s'"),
> >                         signing_key);
> >
> > -     fingerprint_ret = strbuf_detach(fingerprint[1], NULL);
> > -     strbuf_list_free(fingerprint);
> > +     fingerprint_ret = xstrdup(split.items[1].string);
> > +     string_list_clear(&split, 0);
> >       strbuf_release(&fingerprint_stdout);
>
> Since this code releases fingerprint_stdout before leaving, and
> returns a fresh copy of a split piece, it may make more sense to use
> string_list_split_in_place(), which does not have to allocate extra
> strings while it does its work, unlike string_list_split().

Thank you Junio for the review.

I will take a look at string_list_split_in_place().

Bello

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

[Olamide Caleb Bello]

In get_default_ssh_signing_key(), the default ssh signing key is
retrieved in `key_stdout`, which is then split using
strbuf_split_max() into two tokens

The string in `key_stdout` is then split using strbuf_split_max() into
two tokens at a new line and the first token is returned as a `char *`
and not a strbuf.
This makes the function lack the use of strbuf API as no edits are
performed on the split tokens.

Replace strbuf_split_max() with string_list_split() for simplicity.

Note that strbuf_split_max() uses `2` to indicate the number of tokens
to extract from the string, while string_list_split() uses `1` to specify
the number of times the split will be done on the string, so 1 gives 2
tokens as it is in the original instance.

Signed-off-by: Olamide Caleb Bello <belkid98@gmail.com>
---
Changes in v2
 - Reword the commit message for clarity
 - Replace the comment removed in v1
 - Change the test to ensure the number of tokens added to the list by
   string_list_split() equals 2
 - Add () to function names

 gpg-interface.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 989dca7d14..ab7b8b46e5 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -862,7 +862,7 @@ static char *get_default_ssh_signing_key(void)
 	struct child_process ssh_default_key = CHILD_PROCESS_INIT;
 	int ret = -1;
 	struct strbuf key_stdout = STRBUF_INIT, key_stderr = STRBUF_INIT;
-	struct strbuf **keys;
+	struct string_list keys = STRING_LIST_INIT_DUP;
 	char *key_command = NULL;
 	const char **argv;
 	int n;
@@ -884,19 +884,19 @@ static char *get_default_ssh_signing_key(void)
 			   &key_stderr, 0);
 
 	if (!ret) {
-		keys = strbuf_split_max(&key_stdout, '\n', 2);
-		if (keys[0] && is_literal_ssh_key(keys[0]->buf, &literal_key)) {
+		if (string_list_split(&keys, key_stdout.buf, "\n", 1) == 2 &&
+			is_literal_ssh_key(keys.items[0].string, &literal_key)) {
 			/*
 			 * We only use `is_literal_ssh_key` here to check validity
 			 * The prefix will be stripped when the key is used.
 			 */
-			default_key = strbuf_detach(keys[0], NULL);
+			default_key = xstrdup(keys.items[0].string);
 		} else {
 			warning(_("gpg.ssh.defaultKeyCommand succeeded but returned no keys: %s %s"),
 				key_stderr.buf, key_stdout.buf);
 		}
 
-		strbuf_list_free(keys);
+		string_list_clear(&keys, 0);
 	} else {
 		warning(_("gpg.ssh.defaultKeyCommand failed: %s %s"),
 			key_stderr.buf, key_stdout.buf);
-- 
2.51.0.463.g79cf913ea9

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

[Christian Couder]

On Thu, Oct 16, 2025 at 3:04 AM Olamide Caleb Bello <belkid98@gmail.com> 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.

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

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

[Bello Olamide]

On Fri, 17 Oct 2025 at 08:58, Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Thu, Oct 16, 2025 at 3:04 AM Olamide Caleb Bello <belkid98@gmail.com> 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.