[PATCH 0/2] Fix imap-send with reordered message headers

[PATCH 0/2] Fix imap-send with reordered message headers

[Harald Nordgren via GitGitGadget]

git imap-send rejected patches sent with git send-email, such as ones
fetched from lore with b4, because it expected Date before Subject.

Harald Nordgren (2):
  imap-send: avoid leaking the tunnel command
  imap-send: accept messages with Subject before Date

 imap-send.c           | 38 ++++++++++++++++++--------------------
 t/t9001-send-email.sh | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 20 deletions(-)


base-commit: c51408889c3b372814bc20b9431c3b74d0d268eb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2456%2FHaraldNordgren%2Ffix%2Fimap-send-header-order-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2456/HaraldNordgren/fix/imap-send-header-order-v1
Pull-Request: https://github.com/git/git/pull/2456
-- 
gitgitgadget

[PATCH 1/2] imap-send: avoid leaking the tunnel command

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

When imap.tunnel is set, imap_open_store() starts the tunnel but never
releases the child_process used for it, which leaks its argument list.
No test sets imap.tunnel, so the leak checker has not caught this.

Only the pipes to the tunnel are needed once it is running, so release
the rest right after starting it.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 imap-send.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/imap-send.c b/imap-send.c
index bf1d2cf74d..6f76bade35 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1176,6 +1176,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
 
 		imap->buf.sock.fd[0] = tunnel.out;
 		imap->buf.sock.fd[1] = tunnel.in;
+		child_process_clear(&tunnel);
 
 		imap_info("OK\n");
 	} else {
-- 
gitgitgadget

[PATCH 2/2] imap-send: accept messages with Subject before Date

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

Patches sent with git send-email cannot be fed back into git imap-send
after fetching them from the mailing list archive, for example with

    b4 mbox --single-message -o - <url> | git imap-send

which fails with "no messages found to send". imap-send only counts a
message when its From, Date and Subject headers come in that order,
and git send-email writes Subject before Date.

Accept the three headers in any order within the header block of each
message. The copy that git send-email --imap-sent-folder pipes to
imap-send then passes the same check, so the special case added for it
in 04133f5bc4 (send-email: add ability to send a copy of sent emails to
an IMAP folder, 2025-08-12) is no longer needed.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 imap-send.c           | 37 +++++++++++++++++--------------------
 t/t9001-send-email.sh | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 20 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 6f76bade35..6563c8f693 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1453,32 +1453,29 @@ static void wrap_in_html(struct strbuf *msg)
 	*msg = buf;
 }
 
+static bool has_required_headers(const char *line)
+{
+	bool from = false, date = false, subject = false;
+
+	while ((line = strchr(line, '\n')) && *++line != '\n') {
+		if (starts_with(line, "From: "))
+			from = true;
+		else if (starts_with(line, "Date: "))
+			date = true;
+		else if (starts_with(line, "Subject: "))
+			subject = true;
+	}
+	return from && date && subject;
+}
+
 static int count_messages(struct strbuf *all_msgs)
 {
 	int count = 0;
 	char *p = all_msgs->buf;
 
 	while (1) {
-		if (starts_with(p, "From ")) {
-			if (starts_with(p, "From git-send-email")) {
-				p = strstr(p+5, "\nFrom: ");
-				if (!p) break;
-				p += 7;
-				p = strstr(p, "\nTo: ");
-				if (!p) break;
-				p += 5;
-				count++;
-			} else {
-				p = strstr(p+5, "\nFrom: ");
-				if (!p) break;
-				p = strstr(p+7, "\nDate: ");
-				if (!p) break;
-				p = strstr(p+7, "\nSubject: ");
-				if (!p) break;
-				p += 10;
-				count++;
-			}
-		}
+		if (starts_with(p, "From ") && has_required_headers(p))
+			count++;
 		p = strstr(p+5, "\nFrom ");
 		if (!p)
 			break;
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index d1393ef197..1f77c14315 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -2863,4 +2863,39 @@ test_expect_success $PREREQ '--compose handles to headers' '
 	test_cmp expect msgtxt2.to
 '
 
+test_expect_success $PREREQ 'imap-send accepts a sent patch from the list archive' '
+	write_script fake-imap-server <<-\EOF &&
+	printf "* PREAUTH [CAPABILITY IMAP4rev1 LITERAL+] ready\r\n"
+	printf "1 OK\r\n2 OK\r\n3 OK\r\n"
+	cat >/dev/null
+	EOF
+	test_config imap.tunnel ./fake-imap-server &&
+	clean_fake_sendmail &&
+	git send-email \
+		--from="Example <from@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		HEAD^ &&
+	{
+		echo "From mboxrd@z Thu Jan  1 00:00:00 1970" &&
+		cat msgtxt1
+	} >archive.mbox &&
+	git imap-send --folder=INBOX <archive.mbox 2>err &&
+	test_grep "^Sending 1 message to INBOX folder" err &&
+	test_grep "(1/1) done" err
+'
+
+test_expect_success $PREREQ '--imap-sent-folder stores a copy of the sent patch' '
+	test_config imap.tunnel ./fake-imap-server &&
+	clean_fake_sendmail &&
+	git send-email \
+		--from="Example <from@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		--imap-sent-folder=Sent \
+		HEAD^ 2>err &&
+	test_grep "^Sending 1 message to Sent folder" err &&
+	test_grep "(1/1) done" err
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH 2/2] imap-send: accept messages with Subject before Date

[Junio C Hamano]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> which fails with "no messages found to send". imap-send only counts a
> message when its From, Date and Subject headers come in that order,
> and git send-email writes Subject before Date.

Nicely analysed.

> +static bool has_required_headers(const char *line)
> +{
> +	bool from = false, date = false, subject = false;
> +
> +	while ((line = strchr(line, '\n')) && *++line != '\n') {
> +		if (starts_with(line, "From: "))
> +			from = true;
> +		else if (starts_with(line, "Date: "))
> +			date = true;
> +		else if (starts_with(line, "Subject: "))
> +			subject = true;
> +	}
> +	return from && date && subject;
> +}

So the caller is responsible for placing the cursor at the beginning
of a message (e.g., a "From " line) and this one inspects the lines
starting at the point looking for the headers we want to see.  The
cursor is local so our incrementing will not break the caller.

RFC 822/2822/5322 specifies these headers as case insensitive, so
you'd want to use istarts_with() instead, though.  I can see that
the original did not get it right, but we are making the wrongs in
the original right, so ...

>  static int count_messages(struct strbuf *all_msgs)
>  {
>  	int count = 0;
>  	char *p = all_msgs->buf;
>  
>  	while (1) {
> -		if (starts_with(p, "From ")) {
> -			if (starts_with(p, "From git-send-email")) {
> -				p = strstr(p+5, "\nFrom: ");
> -				if (!p) break;
> -				p += 7;
> -				p = strstr(p, "\nTo: ");
> -				if (!p) break;
> -				p += 5;
> -				count++;
> -			} else {
> -				p = strstr(p+5, "\nFrom: ");
> -				if (!p) break;
> -				p = strstr(p+7, "\nDate: ");
> -				if (!p) break;
> -				p = strstr(p+7, "\nSubject: ");
> -				if (!p) break;
> -				p += 10;
> -				count++;
> -			}
> -		}
> +		if (starts_with(p, "From ") && has_required_headers(p))
> +			count++;
>  		p = strstr(p+5, "\nFrom ");
>  		if (!p)
>  			break;
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index d1393ef197..1f77c14315 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -2863,4 +2863,39 @@ test_expect_success $PREREQ '--compose handles to headers' '
>  	test_cmp expect msgtxt2.to
>  '
>  
> +test_expect_success $PREREQ 'imap-send accepts a sent patch from the list archive' '
> +	write_script fake-imap-server <<-\EOF &&
> +	printf "* PREAUTH [CAPABILITY IMAP4rev1 LITERAL+] ready\r\n"
> +	printf "1 OK\r\n2 OK\r\n3 OK\r\n"
> +	cat >/dev/null
> +	EOF
> +	test_config imap.tunnel ./fake-imap-server &&
> +	clean_fake_sendmail &&
> +	git send-email \
> +		--from="Example <from@example.com>" \
> +		--to=nobody@example.com \
> +		--smtp-server="$(pwd)/fake.sendmail" \
> +		HEAD^ &&
> +	{
> +		echo "From mboxrd@z Thu Jan  1 00:00:00 1970" &&
> +		cat msgtxt1
> +	} >archive.mbox &&
> +	git imap-send --folder=INBOX <archive.mbox 2>err &&
> +	test_grep "^Sending 1 message to INBOX folder" err &&
> +	test_grep "(1/1) done" err
> +'
> +
> +test_expect_success $PREREQ '--imap-sent-folder stores a copy of the sent patch' '
> +	test_config imap.tunnel ./fake-imap-server &&
> +	clean_fake_sendmail &&
> +	git send-email \
> +		--from="Example <from@example.com>" \
> +		--to=nobody@example.com \
> +		--smtp-server="$(pwd)/fake.sendmail" \
> +		--imap-sent-folder=Sent \
> +		HEAD^ 2>err &&
> +	test_grep "^Sending 1 message to Sent folder" err &&
> +	test_grep "(1/1) done" err
> +'
> +
>  test_done

Re: [PATCH 2/2] imap-send: accept messages with Subject before Date

[Harald Nordgren]

> RFC 822/2822/5322 specifies these headers as case insensitive, so
> you'd want to use istarts_with() instead, though.  I can see that
> the original did not get it right, but we are making the wrongs in
> the original right, so ...

Thanks!

Would you like to see a test for case insensitive? Personally, I would
skip it since I trust 'istarts_with'.


Harald