Fix imap-send with reordered message headers
cover Fix imap-send with reordered message headers
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
Signature · 1 line
gitgitgadget
PATCH 1/2 imap-send: avoid leaking the tunnel command
From: Harald Nordgren <[email protected]>
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.
--- imap-send.c | 1 + 1 file changed, 1 insertion(+)
imap-send.c +1 -0
@@ -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 {
Signature · 1 line
gitgitgadget
PATCH 2/2 imap-send: accept messages with Subject before Date
In reply to Harald Nordgren via GitGitGadgetFrom: Harald Nordgren <[email protected]>
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.
--- imap-send.c | 37 +++++++++++++++++-------------------- t/t9001-send-email.sh | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 20 deletions(-)
imap-send.c +17 -20
@@ -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 ;
t/t9001-send-email.sh +35 -0
@@ -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 <[email protected]>" \ + [email protected] \ + --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 <[email protected]>" \ + [email protected] \ + --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
Signature · 1 line
gitgitgadget
Re: [PATCH 2/2] imap-send: accept messages with Subject before Date
Quoted 3 lines · Harald Nordgren via GitGitGadget
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.
Quoted 4 lines
imap-send.c +4
-0+static bool has_required_headers (const char *line) +{ +bool from = false, date = false, subject = false; +
imap-send.c static void wrap_in_html(struct strbuf *msg) · line 1469
Quoted 10 lines
imap-send.c +10
-0+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 ...
Quoted 75 lines
static int
count_messages(struct strbuf *all_msgs){imap-send.c +2
-20int 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 ;t/t9001-send-email.sh +35
-0@@ -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 <[email protected]>" \ + [email protected] \ + --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 <[email protected]>" \ + [email protected] \ + --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
Quoted 4 lines
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
v2 cover Fix imap-send with reordered message headers
In reply to Harald Nordgren via GitGitGadgetgit imap-send rejected patches sent with git send-email, such as ones fetched from lore with b4, because it expected Date before Subject.
Changes in v2:
* Case insensitive header match according to RFC 822/2822/5322.
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: 47fdf2208c5b94c8cc01c106b8f7c85efb91c926
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2456%2FHaraldNordgren%2Ffix%2Fimap-send-header-order-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2456/HaraldNordgren/fix/imap-send-header-order-v2
Pull-Request: https://github.com/git/git/pull/2456
Range-diff vs v1:
1: 0784c16134 = 1: b170b1f6d3 imap-send: avoid leaking the tunnel command
2: 94129fb718 ! 2: c42827a122 imap-send: accept messages with Subject before Date
@@ Commit message
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
+ message, and match their names case insensitively as RFC 5322
+ specifies. 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.
@@ imap-send.c: static void wrap_in_html(struct strbuf *msg)
+ bool from = false, date = false, subject = false;
+
+ while ((line = strchr(line, '\n')) && *++line != '\n') {
-+ if (starts_with(line, "From: "))
++ if (istarts_with(line, "From: "))
+ from = true;
-+ else if (starts_with(line, "Date: "))
++ else if (istarts_with(line, "Date: "))
+ date = true;
-+ else if (starts_with(line, "Subject: "))
++ else if (istarts_with(line, "Subject: "))
+ subject = true;
+ }
+ return from && date && subject;Signature · 1 line
gitgitgadget
PATCH v2 1/2 imap-send: avoid leaking the tunnel command
From: Harald Nordgren <[email protected]>
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.
--- imap-send.c | 1 + 1 file changed, 1 insertion(+)
imap-send.c +1 -0
@@ -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 {
Signature · 1 line
gitgitgadget
Re: [PATCH v2 1/2] imap-send: avoid leaking the tunnel command
imap-send.c static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c · line 1179
Quoted 9 lines · Harald Nordgren via GitGitGadget
imap-send.c +1
-0@@ -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);
This change plugs the memory leak, but the bigger question to me is why we never call finish_command() on the child process at all. Is there a reason for that?
Patrick
Re: [PATCH v2 1/2] imap-send: avoid leaking the tunnel command
Quoted 7 lines
Quoted 3 lines
imap->buf.sock.fd[0] = tunnel.out; imap->buf.sock.fd[1] = tunnel.in;+
child_process_clear(&tunnel);This change plugs the memory leak, but the bigger question to me is why we never call
finish_command()on the child process at all. Is there a reason for that?
Yeah, that's curious. I'll take a look at handling this better.
Harald
PATCH v2 2/2 imap-send: accept messages with Subject before Date
In reply to Harald Nordgren via GitGitGadgetFrom: Harald Nordgren <[email protected]>
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, and match their names case insensitively as RFC 5322 specifies. 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.
--- imap-send.c | 37 +++++++++++++++++-------------------- t/t9001-send-email.sh | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 20 deletions(-)
imap-send.c +17 -20
@@ -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 (istarts_with (line,"From: " )) + from = true; +else if (istarts_with (line,"Date: " )) + date = true; +else if (istarts_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 ;
t/t9001-send-email.sh +35 -0
@@ -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 <[email protected]>" \ + [email protected] \ + --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 <[email protected]>" \ + [email protected] \ + --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
Signature · 1 line
gitgitgadget
Re: [PATCH v2 2/2] imap-send: accept messages with Subject before Date
Quoted 11 lines · Harald Nordgren via GitGitGadget
From: Harald Nordgren <[email protected]>
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-sendwhich 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.
Quoted 6 lines
Accept the three headers in any order within the header block of each message, and match their names case insensitively as RFC 5322 specifies. 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.
Aren't you fixing two different issues at once? I'd argue that this should be done in separate commits.
Quoted 42 lines
imap-send.c +16
-12@@ -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 (istarts_with (line,"From: " )) + from = true; +else if (istarts_with (line,"Date: " )) + date = true; +else if (istarts_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 ;
imap-send.c static void wrap_in_html(struct strbuf *msg) · line 1479
Quoted 10 lines
imap-send.c +2
-8- 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++;
In the preimage we special-cased when the line starts with "From git-send-email", and in that case we didn't recognize "Subject:" lines. So you're not only changing that we now accept the headers in any arbitrary order, but you also make us accept "Subject:" lines when we saw "From git-send-email". You don't really explain why that change is okay to do though.
t/t9001-send-email.sh test_expect_success $PREREQ '--compose handles to headers' ' · line 2866
Quoted 9 lines
t/t9001-send-email.sh +1
-0@@ -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' '
Quite weird that every single test carries this prerequisite. Makes me wonder why we don't just skip all tests at once.
Patrick
Re: [PATCH v2 2/2] imap-send: accept messages with Subject before Date
Quoted 16 lines
Quoted 9 lines
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-sendwhich 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.
Quoted 6 lines
Accept the three headers in any order within the header block of each message, and match their names case insensitively as RFC 5322 specifies. 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.
Quoted 2 lines
Aren't you fixing two different issues at once? I'd argue that this should be done in separate commits.
Good point.
Quoted 8 lines
t/t9001-send-email.sh test_expect_success $PREREQ '--compose handles to headers' ' · line 2866
Quoted 5 lines
t/t9001-send-email.sh +1
-0@@ -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' 'Quite weird that every single test carries this prerequisite. Makes me wonder why we don't just skip all tests at once.
What do you mean here, do you want to remove the tests?
Harald
Re: [PATCH v2 2/2] imap-send: accept messages with Subject before Date
Quoted 10 lines · Harald Nordgren
Quoted 8 lines
t/t9001-send-email.sh test_expect_success $PREREQ '--compose handles to headers' ' · line 2866
Quoted 5 lines
t/t9001-send-email.sh +1
-0@@ -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' 'Quite weird that every single test carries this prerequisite. Makes me wonder why we don't just skip all tests at once.
What do you mean here, do you want to remove the tests?
No, I rather think that we should have this at the beginning of the test file:
if ! test_have_prereq PERL
then
skip_all='skipping send-email tests; Perl not available' test_done
fi
Then we can drop the $PREREQ from all tests. But I just spotted that we do some more gymanstics with another prereq that we optionally add later, so this doesn't quite work as-is.
Overall though this is fine, we don't have to fix it as part of this patch series.
Patrick
Re: [PATCH v2 2/2] imap-send: accept messages with Subject before Date
Quoted 11 lines
Quoted 1 line
What do you mean here, do you want to remove the tests?
No, I rather think that we should have this at the beginning of the test file:
if !
test_have_prereqPERL
then
skip_all='skipping send-email tests; Perl not available'test_done
fi
Quoted 6 lines
Then we can drop the $PREREQ from all tests. But I just spotted that we do some more gymanstics with another prereq that we optionally add later, so this doesn't quite work as-is.
Overall though this is fine, we don't have to fix it as part of this patch series.
Alright, maybe better to have this as #leftoverbits ?
Harald
v3 cover Fix imap-send with reordered message headers
In reply to Harald Nordgren via GitGitGadgetgit imap-send rejected patches sent with git send-email, such as ones fetched from lore with b4, because it expected Date before Subject.
Changes in v3:
* Split concerns into different commits, commit message explains what they do.
* Handle tunnel leak in the proper place.
Changes in v2:
* Case insensitive header match according to RFC 822/2822/5322.
Harald Nordgren (4):
imap-send: wait for the tunnel command to finish
imap-send: accept messages with Subject before Date
imap-send: check git send-email copies like other messages
imap-send: match header names case insensitively
imap-send.c | 57 ++++++++++++++++++++++--------------------- t/t9001-send-email.sh | 35 ++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 28 deletions(-)
base-commit: 47fdf2208c5b94c8cc01c106b8f7c85efb91c926
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2456%2FHaraldNordgren%2Ffix%2Fimap-send-header-order-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2456/HaraldNordgren/fix/imap-send-header-order-v3
Pull-Request: https://github.com/git/git/pull/2456
Range-diff vs v2:
1: b170b1f6d3 ! 1: 0f446c3c12 imap-send: avoid leaking the tunnel command
@@ Metadata
Author: Harald Nordgren <[email protected]>
## Commit message ##
- imap-send: avoid leaking the tunnel command
+ imap-send: wait for the tunnel command to finish
- 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.
+ When imap.tunnel is set, git imap-send starts the tunnel command but
+ never waits for it to exit, and leaks the argument list it was started
+ with. The isync code that imap-send was derived from forked the tunnel
+ without ever reaping it, and c94d2dd080 (imap-send: use run-command
+ API for tunneling, 2009-10-19) carried that over.
- Only the pipes to the tunnel are needed once it is running, so release
- the rest right after starting it.
+ Wait for the tunnel when closing the connection. By then imap-send has
+ closed its end of the pipes, so the tunnel sees the end of its input
+ and exits. No test sets imap.tunnel, so the leak checker has not
+ caught this.
## imap-send.c ##
+@@ imap-send.c: struct imap {
+ /* command queue */
+ int nexttag, num_in_progress, literal_pending;
+ struct imap_cmd *in_progress, **in_progress_append;
++ struct child_process tunnel;
+ struct imap_buffer buf; /* this is BIG, so put it last */
+ };
+
+@@ imap-send.c: static void imap_close_server(struct imap_store *ictx)
+ imap_exec(ictx, NULL, "LOGOUT");
+ socket_shutdown(&imap->buf.sock);
+ }
++ if (ictx->cfg->tunnel)
++ finish_command(&imap->tunnel);
+ free(imap);
+ }
+
@@ imap-send.c: static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
+ /* open connection to IMAP server */
+
+ if (srvc->tunnel) {
+- struct child_process tunnel = CHILD_PROCESS_INIT;
++ struct child_process *tunnel = &imap->tunnel;
+
+ imap_info("Starting tunnel '%s'... ", srvc->tunnel);
+
+- strvec_push(&tunnel.args, srvc->tunnel);
+- tunnel.use_shell = 1;
+- tunnel.in = -1;
+- tunnel.out = -1;
+- if (start_command(&tunnel))
++ child_process_init(tunnel);
++ strvec_push(&tunnel->args, srvc->tunnel);
++ tunnel->use_shell = 1;
++ tunnel->in = -1;
++ tunnel->out = -1;
++ if (start_command(tunnel))
+ die("cannot start proxy %s", srvc->tunnel);
- imap->buf.sock.fd[0] = tunnel.out;
- imap->buf.sock.fd[1] = tunnel.in;
-+ child_process_clear(&tunnel);
+- imap->buf.sock.fd[0] = tunnel.out;
+- imap->buf.sock.fd[1] = tunnel.in;
++ imap->buf.sock.fd[0] = tunnel->out;
++ imap->buf.sock.fd[1] = tunnel->in;
imap_info("OK\n");
} else {
2: c42827a122 ! 2: 38214d9abf imap-send: accept messages with Subject before Date
@@ Commit message
and git send-email writes Subject before Date.
Accept the three headers in any order within the header block of each
- message, and match their names case insensitively as RFC 5322
- specifies. 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.
+ message.
Signed-off-by: Harald Nordgren <[email protected]>
@@ imap-send.c: static void wrap_in_html(struct strbuf *msg)
+ bool from = false, date = false, subject = false;
+
+ while ((line = strchr(line, '\n')) && *++line != '\n') {
-+ if (istarts_with(line, "From: "))
++ if (starts_with(line, "From: "))
+ from = true;
-+ else if (istarts_with(line, "Date: "))
++ else if (starts_with(line, "Date: "))
+ date = true;
-+ else if (istarts_with(line, "Subject: "))
++ else if (starts_with(line, "Subject: "))
+ subject = true;
+ }
+ return from && date && subject;
@@ imap-send.c: static void wrap_in_html(struct strbuf *msg)
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++;
+@@ imap-send.c: static int count_messages(struct strbuf *all_msgs)
+ if (!p) break;
+ p += 5;
+ count++;
- } else {
- p = strstr(p+5, "\nFrom: ");
- if (!p) break;
@@ imap-send.c: static void wrap_in_html(struct strbuf *msg)
- 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;
++ } else if (has_required_headers(p)) {
+ count++;
+ }
+ }
## t/t9001-send-email.sh ##
@@ t/t9001-send-email.sh: test_expect_success $PREREQ '--compose handles to headers' '
@@ t/t9001-send-email.sh: test_expect_success $PREREQ '--compose handles to headers
+ 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 <[email protected]>" \
-+ [email protected] \
-+ --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
-: ---------- > 3: 213c4950e0 imap-send: check git send-email copies like other messages
-: ---------- > 4: 01add80af2 imap-send: match header names case insensitivelySignature · 1 line
gitgitgadget
PATCH v3 1/4 imap-send: wait for the tunnel command to finish
From: Harald Nordgren <[email protected]>
When imap.tunnel is set, git imap-send starts the tunnel command but never waits for it to exit, and leaks the argument list it was started with. The isync code that imap-send was derived from forked the tunnel without ever reaping it, and c94d2dd080 (imap-send: use run-command API for tunneling, 2009-10-19) carried that over.
Wait for the tunnel when closing the connection. By then imap-send has closed its end of the pipes, so the tunnel sees the end of its input and exits. No test sets imap.tunnel, so the leak checker has not caught this.
--- imap-send.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
imap-send.c +12 -8
@@ -118,6 +118,7 @@ struct imap {/* command queue */ int nexttag, num_in_progress, literal_pending;struct imap_cmd *in_progress, **in_progress_append; +struct child_process tunnel;struct imap_buffer buf;/* this is BIG, so put it last */ };
@@ -858,6 +859,8 @@ static void imap_close_server(struct imap_store *ictx)imap_exec (ictx,NULL ,"LOGOUT" );socket_shutdown (&imap->buf.sock); } +if (ictx->cfg->tunnel) +finish_command (&imap->tunnel);free (imap); }
@@ -1163,19 +1166,20 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c/* open connection to IMAP server */ if (srvc->tunnel) { -struct child_process tunnel =CHILD_PROCESS_INIT ; +struct child_process *tunnel = &imap->tunnel;imap_info ("Starting tunnel '%s'... " , srvc->tunnel); -strvec_push (&tunnel.args, srvc->tunnel); - tunnel.use_shell =1 ; - tunnel.in =-1 ; - tunnel.out =-1 ; -if (start_command (&tunnel)) +child_process_init (tunnel); +strvec_push (&tunnel->args, srvc->tunnel); + tunnel->use_shell =1 ; + tunnel->in =-1 ; + tunnel->out =-1 ; +if (start_command (tunnel))die ("cannot start proxy %s" , srvc->tunnel); - imap->buf.sock.fd[0 ] = tunnel.out; - imap->buf.sock.fd[1 ] = tunnel.in; + imap->buf.sock.fd[0 ] = tunnel->out; + imap->buf.sock.fd[1 ] = tunnel->in;imap_info ("OK\n" ); }else {
Signature · 1 line
gitgitgadget
Re: [PATCH v3 1/4] imap-send: wait for the tunnel command to finish
Quoted 13 lines · Harald Nordgren via GitGitGadget
imap-send.c +1
-0@@ -118,6 +118,7 @@ struct imap {/* command queue */ int nexttag, num_in_progress, literal_pending;struct imap_cmd *in_progress, **in_progress_append; +struct child_process tunnel;struct imap_buffer buf;/* this is BIG, so put it last */ };
imap-send.c static void imap_close_server(struct imap_store *ictx) · line 863
Quoted 5 lines
imap-send.c +2
-0@@ -858,6 +859,8 @@ static void imap_close_server(struct imap_store *ictx)imap_exec (ictx,NULL ,"LOGOUT" );socket_shutdown (&imap->buf.sock); } +if (ictx->cfg->tunnel) +finish_command (&imap->tunnel);
Hmm, okay. Do we _know_ that ictx->cfg->tunnel is always properly initialized in case we call imap_close_server()? Or are there error paths where it might not be initialized yet? If the latter I think we might see weird behaviour here as the function does not seem to be safe when called on an uninitialized command.
Maybe we should make imap->tunnel an allocated pointer instead so that we know to only finish it when allocated and initialized?
imap-send.c static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c · line 1169
Quoted 6 lines
imap-send.c +1
-1@@ -1163,19 +1166,20 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c/* open connection to IMAP server */ if (srvc->tunnel) { -struct child_process tunnel =CHILD_PROCESS_INIT ; +struct child_process *tunnel = &imap->tunnel;
Nit: I would have avoided this local variable, it doesn't help us anyway to shrink the diff.
Patrick
Re: [PATCH v3 1/4] imap-send: wait for the tunnel command to finish
Quoted 11 lines
imap-send.c static void imap_close_server(struct imap_store *ictx) · line 863
Quoted 2 lines
imap-send.c +2
-0+if (ictx->cfg->tunnel) +finish_command (&imap->tunnel);Hmm, okay. Do we _know_ that
ictx->cfg->tunnelis always properly initialized in case we callimap_close_server()? Or are there error paths where it might not be initialized yet? If the latter I think we might see weird behaviour here as the function does not seem to be safe when called on an uninitialized command.Maybe we should make
imap->tunnelan allocated pointer instead so that we know to only finish it when allocated and initialized?
Good idea.
Quoted 9 lines
imap-send.c static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c · line 1169
Quoted 6 lines
imap-send.c +1
-1@@ -1163,19 +1166,20 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c/* open connection to IMAP server */ if (srvc->tunnel) { -struct child_process tunnel =CHILD_PROCESS_INIT ; +struct child_process *tunnel = &imap->tunnel;Nit: I would have avoided this local variable, it doesn't help us anyway to shrink the diff.
Ok.
Harald
PATCH v3 2/4 imap-send: accept messages with Subject before Date
In reply to Harald Nordgren via GitGitGadgetFrom: Harald Nordgren <[email protected]>
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.
--- imap-send.c | 24 ++++++++++++++++-------- t/t9001-send-email.sh | 22 ++++++++++++++++++++++ 2 files changed, 38 insertions(+), 8 deletions(-)
imap-send.c +16 -8
@@ -1456,6 +1456,21 @@ 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 ;
@@ -1471,14 +1486,7 @@ static int count_messages(struct strbuf *all_msgs)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 ; + }else if (has_required_headers (p)) { count++; } }
t/t9001-send-email.sh +22 -0
@@ -2863,4 +2863,26 @@ 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 <[email protected]>" \ + [email protected] \ + --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_done
Signature · 1 line
gitgitgadget
PATCH v3 3/4 imap-send: check git send-email copies like other messages
In reply to Harald Nordgren via GitGitGadgetFrom: Harald Nordgren <[email protected]>
The copy that git send-email --imap-sent-folder pipes to imap-send starts with a "From git-send-email" line. 04133f5bc4 (send-email: add ability to send a copy of sent emails to an IMAP folder, 2025-08-12) made imap-send count such a message when it has From and To headers, without looking for Date and Subject. Without that special case the copy would not be counted at all, because git send-email writes Subject before Date.
Now that the headers are accepted in any order, the special case is no longer needed. The copy carries the same headers git send-email sends over SMTP, which always include From, Subject and Date, so it passes the check for other messages. A To header is no longer required, the same as for any other message.
--- imap-send.c | 15 ++------------- t/t9001-send-email.sh | 13 +++++++++++++ 2 files changed, 15 insertions(+), 13 deletions(-)
imap-send.c +2 -13
@@ -1477,19 +1477,8 @@ static int count_messages(struct strbuf *all_msgs)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 if (has_required_headers (p)) { - count++; - } - } +if (starts_with (p,"From " ) &&has_required_headers (p)) + count++; p =strstr (p+5 ,"\nFrom " );if (!p)break ;
t/t9001-send-email.sh +13 -0
@@ -2885,4 +2885,17 @@ test_expect_success $PREREQ 'imap-send accepts a sent patch from the list archiv 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 <[email protected]>" \ + [email protected] \ + --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
Signature · 1 line
gitgitgadget
Re: [PATCH v3 3/4] imap-send: check git send-email copies like other messages
Quoted 10 lines · Harald Nordgren via GitGitGadget
From: Harald Nordgren <[email protected]>
The copy that git send-email --imap-sent-folder pipes to imap-send starts with a "From git-send-email" line.
04133f5bc4(send-email: add ability to send a copy of sent emails to an IMAP folder, 2025-08-12) made imap-send count such a message when it has From and To headers, without looking for Date and Subject. Without that special case the copy would not be counted at all, because git send-email writes Subject before Date.
Quoted 5 lines
Now that the headers are accepted in any order, the special case is no longer needed. The copy carries the same headers git send-email sends over SMTP, which always include From, Subject and Date, so it passes the check for other messages. A To header is no longer required, the same as for any other message.
You argue from the perspective of git-send-email(1) generating the mbox. But what if somebody writes an mbox via a different script that doesn't write a Date or Subject line? Based on RFC 5322, the Date header is mandatory, so I think it's safe to assume it always exists. But the Subject header is only specified as "SHOULD exist".
Now we probably don't care about such edge cases, so tightening the rules is likely going to be fine. But I don't think that's sufficiently explained.
Patrick
Re: [PATCH v3 3/4] imap-send: check git send-email copies like other messages
Quoted 20 lines
Quoted 8 lines
The copy that git send-email --imap-sent-folder pipes to imap-send starts with a "From git-send-email" line.
04133f5bc4(send-email: add ability to send a copy of sent emails to an IMAP folder, 2025-08-12) made imap-send count such a message when it has From and To headers, without looking for Date and Subject. Without that special case the copy would not be counted at all, because git send-email writes Subject before Date.Quoted 5 lines
Now that the headers are accepted in any order, the special case is no longer needed. The copy carries the same headers git send-email sends over SMTP, which always include From, Subject and Date, so it passes the check for other messages. A To header is no longer required, the same as for any other message.
You argue from the perspective of git-send-email(1) generating the mbox. But what if somebody writes an mbox via a different script that doesn't write a Date or Subject line? Based on RFC 5322, the Date header is mandatory, so I think it's safe to assume it always exists. But the Subject header is only specified as "SHOULD exist".
Quoted 3 lines
Now we probably don't care about such edge cases, so tightening the rules is likely going to be fine. But I don't think that's sufficiently explained.
Thanks, very valuable to have another set of eyes here!
Harald
PATCH v3 4/4 imap-send: match header names case insensitively
In reply to Harald Nordgren via GitGitGadgetFrom: Harald Nordgren <[email protected]>
imap-send says "no messages found to send" for a message whose headers are spelled "from:", "date:" or "subject:", although RFC 5322 treats header field names as case insensitive. Match the three names in any case.
--- imap-send.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
imap-send.c +3 -3
@@ -1461,11 +1461,11 @@ 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: " )) +if (istarts_with (line,"From: " )) from = true; -else if (starts_with (line,"Date: " )) +else if (istarts_with (line,"Date: " )) date = true; -else if (starts_with (line,"Subject: " )) +else if (istarts_with (line,"Subject: " )) subject = true; }return from && date && subject;
Signature · 1 line
gitgitgadget
Re: [PATCH v3 4/4] imap-send: match header names case insensitively
Quoted 6 lines · Harald Nordgren via GitGitGadget
From: Harald Nordgren <[email protected]>
imap-send says "no messages found to send" for a message whose headers are spelled "from:", "date:" or "subject:", although RFC 5322 treats header field names as case insensitive. Match the three names in any case.
Should we have a test for this, as well?
Patrick
v4 cover Fix imap-send with reordered message headers
In reply to Harald Nordgren via GitGitGadgetgit imap-send rejected patches sent with git send-email, such as ones fetched from lore with b4, because it expected Date before Subject.
Changes in v4:
* Store the tunnel as an allocated pointer, so it is only finished when it was started, and drop the local tunnel variable
* Add a test for lowercase from:, date: and subject: headers
* Explain in the commit message why dropping the From git-send-email special case is fine for mboxes written by other scripts
Changes in v3:
* Split concerns into different commits, commit message explains what they do.
* Handle tunnel leak in the proper place.
Changes in v2:
* Case insensitive header match according to RFC 822/2822/5322.
Harald Nordgren (4):
imap-send: wait for the tunnel command to finish
imap-send: accept messages with Subject before Date
imap-send: check git send-email copies like other messages
imap-send: match header names case insensitively
imap-send.c | 60 ++++++++++++++++++++++--------------------- t/t9001-send-email.sh | 50 ++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 29 deletions(-)
base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2456%2FHaraldNordgren%2Ffix%2Fimap-send-header-order-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2456/HaraldNordgren/fix/imap-send-header-order-v4
Pull-Request: https://github.com/git/git/pull/2456
Range-diff vs v3:
1: 0f446c3c12 ! 1: 8941ae12bb imap-send: wait for the tunnel command to finish
@@ imap-send.c: struct imap {
/* command queue */
int nexttag, num_in_progress, literal_pending;
struct imap_cmd *in_progress, **in_progress_append;
-+ struct child_process tunnel;
++ struct child_process *tunnel;
struct imap_buffer buf; /* this is BIG, so put it last */
};
@@ imap-send.c: static void imap_close_server(struct imap_store *ictx)
imap_exec(ictx, NULL, "LOGOUT");
socket_shutdown(&imap->buf.sock);
}
-+ if (ictx->cfg->tunnel)
-+ finish_command(&imap->tunnel);
++ if (imap->tunnel) {
++ finish_command(imap->tunnel);
++ free(imap->tunnel);
++ }
free(imap);
}
@@ imap-send.c: static struct imap_store *imap_open_store(struct imap_server_conf *
if (srvc->tunnel) {
- struct child_process tunnel = CHILD_PROCESS_INIT;
-+ struct child_process *tunnel = &imap->tunnel;
-
+-
imap_info("Starting tunnel '%s'... ", srvc->tunnel);
- strvec_push(&tunnel.args, srvc->tunnel);
@@ imap-send.c: static struct imap_store *imap_open_store(struct imap_server_conf *
- tunnel.in = -1;
- tunnel.out = -1;
- if (start_command(&tunnel))
-+ child_process_init(tunnel);
-+ strvec_push(&tunnel->args, srvc->tunnel);
-+ tunnel->use_shell = 1;
-+ tunnel->in = -1;
-+ tunnel->out = -1;
-+ if (start_command(tunnel))
++ imap->tunnel = xmalloc(sizeof(*imap->tunnel));
++ child_process_init(imap->tunnel);
++ strvec_push(&imap->tunnel->args, srvc->tunnel);
++ imap->tunnel->use_shell = 1;
++ imap->tunnel->in = -1;
++ imap->tunnel->out = -1;
++ if (start_command(imap->tunnel))
die("cannot start proxy %s", srvc->tunnel);
- imap->buf.sock.fd[0] = tunnel.out;
- imap->buf.sock.fd[1] = tunnel.in;
-+ imap->buf.sock.fd[0] = tunnel->out;
-+ imap->buf.sock.fd[1] = tunnel->in;
++ imap->buf.sock.fd[0] = imap->tunnel->out;
++ imap->buf.sock.fd[1] = imap->tunnel->in;
imap_info("OK\n");
} else {
2: 38214d9abf = 2: ad3158b336 imap-send: accept messages with Subject before Date
3: 213c4950e0 ! 3: 919e38c31d imap-send: check git send-email copies like other messages @@ Commit message
the check for other messages. A To header is no longer required, the same as for any other message.
+ This makes the check stricter when some other script writes an mbox
+ with a "From git-send-email" line. RFC 5322 requires a Date header but
+ only recommends Subject, so a valid message from such a script may
+ lack a Subject and would no longer be counted. imap-send has always
+ required a Subject under any other "From " line, though, and only
+ refuses an mbox when none of its messages are counted, so treating
+ these copies like the rest should not affect anyone in practice.
+
## imap-send.c ##
4: 01add80af2 ! 4: 5b4a08bbda imap-send: match header names case insensitively
@@ imap-send.c: static bool has_required_headers(const char *line)
subject = true;
}
return from && date && subject;
+
+ ## t/t9001-send-email.sh ##
+@@ t/t9001-send-email.sh: test_expect_success $PREREQ '--imap-sent-folder stores a copy of the sent patch'
+ test_grep "(1/1) done" err
+ '
+
++test_expect_success $PREREQ 'imap-send matches header names in any case' '
++ test_config imap.tunnel ./fake-imap-server &&
++ cat >lowercase.mbox <<-\EOF &&
++ From mboxrd@z Thu Jan 1 00:00:00 1970
++ from: Example <[email protected]>
++ date: Thu, 1 Jan 1970 00:00:00 +0000
++ subject: [PATCH] lowercase headers
++
++ body
++ EOF
++ git imap-send --folder=INBOX <lowercase.mbox 2>err &&
++ test_grep "^Sending 1 message to INBOX folder" err &&
++ test_grep "(1/1) done" err
++'
++
+ test_doneSignature · 1 line
gitgitgadget
PATCH v4 1/4 imap-send: wait for the tunnel command to finish
From: Harald Nordgren <[email protected]>
When imap.tunnel is set, git imap-send starts the tunnel command but never waits for it to exit, and leaks the argument list it was started with. The isync code that imap-send was derived from forked the tunnel without ever reaping it, and c94d2dd080 (imap-send: use run-command API for tunneling, 2009-10-19) carried that over.
Wait for the tunnel when closing the connection. By then imap-send has closed its end of the pipes, so the tunnel sees the end of its input and exits. No test sets imap.tunnel, so the leak checker has not caught this.
--- imap-send.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
imap-send.c +14 -9
@@ -118,6 +118,7 @@ struct imap {/* command queue */ int nexttag, num_in_progress, literal_pending;struct imap_cmd *in_progress, **in_progress_append; +struct child_process *tunnel;struct imap_buffer buf;/* this is BIG, so put it last */ };
@@ -858,6 +859,10 @@ static void imap_close_server(struct imap_store *ictx)imap_exec (ictx,NULL ,"LOGOUT" );socket_shutdown (&imap->buf.sock); } +if (imap->tunnel) { +finish_command (imap->tunnel); +free (imap->tunnel); + }free (imap); }
@@ -1163,19 +1168,19 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c/* open connection to IMAP server */ if (srvc->tunnel) { -struct child_process tunnel =CHILD_PROCESS_INIT ; -imap_info ("Starting tunnel '%s'... " , srvc->tunnel); -strvec_push (&tunnel.args, srvc->tunnel); - tunnel.use_shell =1 ; - tunnel.in =-1 ; - tunnel.out =-1 ; -if (start_command (&tunnel)) + imap->tunnel =xmalloc (sizeof (*imap->tunnel)); +child_process_init (imap->tunnel); +strvec_push (&imap->tunnel->args, srvc->tunnel); + imap->tunnel->use_shell =1 ; + imap->tunnel->in =-1 ; + imap->tunnel->out =-1 ; +if (start_command (imap->tunnel))die ("cannot start proxy %s" , srvc->tunnel); - imap->buf.sock.fd[0 ] = tunnel.out; - imap->buf.sock.fd[1 ] = tunnel.in; + imap->buf.sock.fd[0 ] = imap->tunnel->out; + imap->buf.sock.fd[1 ] = imap->tunnel->in;imap_info ("OK\n" ); }else {
Signature · 1 line
gitgitgadget
PATCH v4 2/4 imap-send: accept messages with Subject before Date
In reply to Harald Nordgren via GitGitGadgetFrom: Harald Nordgren <[email protected]>
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.
--- imap-send.c | 24 ++++++++++++++++-------- t/t9001-send-email.sh | 22 ++++++++++++++++++++++ 2 files changed, 38 insertions(+), 8 deletions(-)
imap-send.c +16 -8
@@ -1457,6 +1457,21 @@ 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 ;
@@ -1472,14 +1487,7 @@ static int count_messages(struct strbuf *all_msgs)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 ; + }else if (has_required_headers (p)) { count++; } }
t/t9001-send-email.sh +22 -0
@@ -2863,4 +2863,26 @@ 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 <[email protected]>" \ + [email protected] \ + --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_done
Signature · 1 line
gitgitgadget
Re: [PATCH v4 2/4] imap-send: accept messages with Subject before Date
Quoted 4 lines · Harald Nordgren via GitGitGadget
imap-send.c +4
-0+static bool has_required_headers (const char *line) +{ +bool from = false, date = false, subject = false; +
imap-send.c static void wrap_in_html(struct strbuf *msg) · line 1473
Quoted 10 lines
imap-send.c +10
-0+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; +}
The above assumes that an empty line that denotes the end of the header can be detected by looking for '\n\n' in the line[]. So this may be scanning thru to the end of the message including the body, because each and every line ends with CRLF and there is no two consecutive LF appearing in the line.
I admit that it was eons ago I read RFCs on IMAP (like RFC 2060), but I recall that its definition explicitly called for CRLF line endings, and existing code in imap-send.c before these patches seems to expect that it is reading CRLF delimited lines.
I didn't look at other parts of the series, but this one stood out.
Re: [PATCH v4 2/4] imap-send: accept messages with Subject before Date
Quoted 21 lines
Quoted 4 lines
imap-send.c +4
-0+static bool has_required_headers (const char *line) +{ +bool from = false, date = false, subject = false; +imap-send.c static void wrap_in_html(struct strbuf *msg) · line 1473
Quoted 10 lines
imap-send.c +10
-0+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; +}The above assumes that an empty line that denotes the end of the header can be detected by looking for '\n\n' in the line[]. So this may be scanning thru to the end of the message including the body, because each and every line ends with CRLF and there is no two consecutive LF appearing in the line.
Quoted 4 lines
I admit that it was eons ago I read RFCs on IMAP (like RFC 2060), but I recall that its definition explicitly called for CRLF line endings, and existing code in
imap-send.cbefore these patches seems to expect that it is reading CRLF delimited lines.
Thanks for pointing this out, I'll take a look.
Harald
PATCH v4 3/4 imap-send: check git send-email copies like other messages
In reply to Harald Nordgren via GitGitGadgetFrom: Harald Nordgren <[email protected]>
The copy that git send-email --imap-sent-folder pipes to imap-send starts with a "From git-send-email" line. 04133f5bc4 (send-email: add ability to send a copy of sent emails to an IMAP folder, 2025-08-12) made imap-send count such a message when it has From and To headers, without looking for Date and Subject. Without that special case the copy would not be counted at all, because git send-email writes Subject before Date.
Now that the headers are accepted in any order, the special case is no longer needed. The copy carries the same headers git send-email sends over SMTP, which always include From, Subject and Date, so it passes the check for other messages. A To header is no longer required, the same as for any other message.
This makes the check stricter when some other script writes an mbox with a "From git-send-email" line. RFC 5322 requires a Date header but only recommends Subject, so a valid message from such a script may lack a Subject and would no longer be counted. imap-send has always required a Subject under any other "From " line, though, and only refuses an mbox when none of its messages are counted, so treating these copies like the rest should not affect anyone in practice.
--- imap-send.c | 15 ++------------- t/t9001-send-email.sh | 13 +++++++++++++ 2 files changed, 15 insertions(+), 13 deletions(-)
imap-send.c +2 -13
@@ -1478,19 +1478,8 @@ static int count_messages(struct strbuf *all_msgs)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 if (has_required_headers (p)) { - count++; - } - } +if (starts_with (p,"From " ) &&has_required_headers (p)) + count++; p =strstr (p+5 ,"\nFrom " );if (!p)break ;
t/t9001-send-email.sh +13 -0
@@ -2885,4 +2885,17 @@ test_expect_success $PREREQ 'imap-send accepts a sent patch from the list archiv 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 <[email protected]>" \ + [email protected] \ + --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
Signature · 1 line
gitgitgadget
Re: [PATCH v4 3/4] imap-send: check git send-email copies like other messages
Quoted 7 lines · Harald Nordgren via GitGitGadget
This makes the check stricter when some other script writes an mbox with a "From git-send-email" line. RFC 5322 requires a Date header but only recommends Subject, so a valid message from such a script may lack a Subject and would no longer be counted. imap-send has always required a Subject under any other "From " line, though, and only refuses an mbox when none of its messages are counted, so treating these copies like the rest should not affect anyone in practice.
Makes sense. I think it's highly unlikely that anyone ever formats patch series without a Subject and then expects everything to "just work". So even though we are tightening checks, I don't see that this will have any negative consequences.
Patrick
PATCH v4 4/4 imap-send: match header names case insensitively
In reply to Harald Nordgren via GitGitGadgetFrom: Harald Nordgren <[email protected]>
imap-send says "no messages found to send" for a message whose headers are spelled "from:", "date:" or "subject:", although RFC 5322 treats header field names as case insensitive. Match the three names in any case.
--- imap-send.c | 6 +++--- t/t9001-send-email.sh | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-)
imap-send.c +3 -3
@@ -1462,11 +1462,11 @@ 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: " )) +if (istarts_with (line,"From: " )) from = true; -else if (starts_with (line,"Date: " )) +else if (istarts_with (line,"Date: " )) date = true; -else if (starts_with (line,"Subject: " )) +else if (istarts_with (line,"Subject: " )) subject = true; }return from && date && subject;
t/t9001-send-email.sh +15 -0
@@ -2898,4 +2898,19 @@ test_expect_success $PREREQ '--imap-sent-folder stores a copy of the sent patch' test_grep "(1/1) done" err ' +test_expect_success $PREREQ 'imap-send matches header names in any case' ' + test_config imap.tunnel ./fake-imap-server && + cat >lowercase.mbox <<-\EOF && + From mboxrd@z Thu Jan 1 00:00:00 1970 + from: Example <[email protected]> + date: Thu, 1 Jan 1970 00:00:00 +0000 + subject: [PATCH] lowercase headers + + body + EOF + git imap-send --folder=INBOX <lowercase.mbox 2>err && + test_grep "^Sending 1 message to INBOX folder" err && + test_grep "(1/1) done" err +' + test_done
Signature · 1 line
gitgitgadget
Quoted 10 lines · 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.
Changes in v4:
* Store the tunnel as an allocated pointer, so it is only finished when it was started, and drop the local tunnel variable
* Add a test for lowercase from:, date: and subject: headers
* Explain in the commit message why dropping the From git-send-email special case is fine for mboxes written by other scripts
Thanks, I'm happy with this version.
Patrick
Quoted 12 lines · Patrick Steinhardt
Quoted 10 lines · 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.
Changes in v4:
* Store the tunnel as an allocated pointer, so it is only finished when it was started, and drop the local tunnel variable
* Add a test for lowercase from:, date: and subject: headers
* Explain in the commit message why dropping the From git-send-email special case is fine for mboxes written by other scripts
Quoted 3 lines
Thanks, I'm happy with this version.
Patrick
Thanks.
v5 cover imap-send: fix with reordered message headers
In reply to Harald Nordgren via GitGitGadgetgit imap-send rejected patches sent with git send-email, such as ones fetched from lore with b4, because it expected Date before Subject.
Changes in v5:
* End the header block at an empty line in a CRLF mbox as well, so headers in the body or in later messages no longer count. Add a test.
Changes in v4:
* Store the tunnel as an allocated pointer, so it is only finished when it was started, and drop the local tunnel variable
* Add a test for lowercase from:, date: and subject: headers
* Explain in the commit message why dropping the From git-send-email special case is fine for mboxes written by other scripts
Changes in v3:
* Split concerns into different commits, commit message explains what they do.
* Handle tunnel leak in the proper place.
Changes in v2:
* Case insensitive header match according to RFC 822/2822/5322.
Harald Nordgren (4):
imap-send: wait for the tunnel command to finish
imap-send: accept messages with Subject before Date
imap-send: check git send-email copies like other messages
imap-send: match header names case insensitively
imap-send.c | 61 ++++++++++++++++++++------------------- t/t9001-send-email.sh | 66 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 29 deletions(-)
base-commit: 50fe5cc63f9cd6e89b87a2c9ccf96a444f4cfdb7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2456%2FHaraldNordgren%2Ffix%2Fimap-send-header-order-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2456/HaraldNordgren/fix/imap-send-header-order-v5
Pull-Request: https://github.com/git/git/pull/2456
Range-diff vs v4:
1: 8941ae12bb = 1: cb27ac3a27 imap-send: wait for the tunnel command to finish
2: ad3158b336 ! 2: fa90c64a5c imap-send: accept messages with Subject before Date
@@ Commit message
and git send-email writes Subject before Date.
Accept the three headers in any order within the header block of each
- message.
+ message, which ends at the first empty line whether the mbox uses LF or
+ CRLF line endings.
Signed-off-by: Harald Nordgren <[email protected]>
@@ imap-send.c: static void wrap_in_html(struct strbuf *msg)
+{
+ bool from = false, date = false, subject = false;
+
-+ while ((line = strchr(line, '\n')) && *++line != '\n') {
++ while ((line = strchr(line, '\n')) &&
++ *++line != '\n' && !starts_with(line, "\r\n")) {
+ if (starts_with(line, "From: "))
+ from = true;
+ else if (starts_with(line, "Date: "))
@@ t/t9001-send-email.sh: test_expect_success $PREREQ '--compose handles to headers
+ test_grep "^Sending 1 message to INBOX folder" err &&
+ test_grep "(1/1) done" err
+'
++
++test_expect_success $PREREQ 'imap-send ignores headers in the body of a CRLF mbox' '
++ test_config imap.tunnel ./fake-imap-server &&
++ q_to_cr >crlf.mbox <<-\EOF &&
++ From mboxrd@z Thu Jan 1 00:00:00 1970Q
++ From: Example <[email protected]>Q
++ Subject: Fwd: originalQ
++ Q
++ ---------- Forwarded message ---------Q
++ From: Someone <[email protected]>Q
++ Date: Thu, 1 Jan 1970 00:00:00 +0000Q
++ Subject: originalQ
++ EOF
++ test_must_fail git imap-send --folder=INBOX <crlf.mbox 2>err &&
++ test_grep "no messages found to send" err
++'
+
test_done 3: 919e38c31d ! 3: 8646185486 imap-send: check git send-email copies like other messages
@@ imap-send.c: static int count_messages(struct strbuf *all_msgs)
break;
## t/t9001-send-email.sh ##
-@@ t/t9001-send-email.sh: test_expect_success $PREREQ 'imap-send accepts a sent patch from the list archiv
- test_grep "(1/1) done" err
+@@ t/t9001-send-email.sh: test_expect_success $PREREQ 'imap-send ignores headers in the body of a CRLF mbo
+ test_grep "no messages found to send" err
'
+test_expect_success $PREREQ '--imap-sent-folder stores a copy of the sent patch' '
4: 5b4a08bbda ! 4: e7d1d7cd72 imap-send: match header names case insensitively
@@ Commit message
## imap-send.c ##
@@ imap-send.c: static bool has_required_headers(const char *line)
- bool from = false, date = false, subject = false;
- while ((line = strchr(line, '\n')) && *++line != '\n') {
+ while ((line = strchr(line, '\n')) &&
+ *++line != '\n' && !starts_with(line, "\r\n")) {
- if (starts_with(line, "From: "))
+ if (istarts_with(line, "From: "))
from = true;
Signature · 1 line
gitgitgadget
PATCH v5 1/4 imap-send: wait for the tunnel command to finish
From: Harald Nordgren <[email protected]>
When imap.tunnel is set, git imap-send starts the tunnel command but never waits for it to exit, and leaks the argument list it was started with. The isync code that imap-send was derived from forked the tunnel without ever reaping it, and c94d2dd080 (imap-send: use run-command API for tunneling, 2009-10-19) carried that over.
Wait for the tunnel when closing the connection. By then imap-send has closed its end of the pipes, so the tunnel sees the end of its input and exits. No test sets imap.tunnel, so the leak checker has not caught this.
--- imap-send.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
imap-send.c +14 -9
@@ -118,6 +118,7 @@ struct imap {/* command queue */ int nexttag, num_in_progress, literal_pending;struct imap_cmd *in_progress, **in_progress_append; +struct child_process *tunnel;struct imap_buffer buf;/* this is BIG, so put it last */ };
@@ -858,6 +859,10 @@ static void imap_close_server(struct imap_store *ictx)imap_exec (ictx,NULL ,"LOGOUT" );socket_shutdown (&imap->buf.sock); } +if (imap->tunnel) { +finish_command (imap->tunnel); +free (imap->tunnel); + }free (imap); }
@@ -1163,19 +1168,19 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c/* open connection to IMAP server */ if (srvc->tunnel) { -struct child_process tunnel =CHILD_PROCESS_INIT ; -imap_info ("Starting tunnel '%s'... " , srvc->tunnel); -strvec_push (&tunnel.args, srvc->tunnel); - tunnel.use_shell =1 ; - tunnel.in =-1 ; - tunnel.out =-1 ; -if (start_command (&tunnel)) + imap->tunnel =xmalloc (sizeof (*imap->tunnel)); +child_process_init (imap->tunnel); +strvec_push (&imap->tunnel->args, srvc->tunnel); + imap->tunnel->use_shell =1 ; + imap->tunnel->in =-1 ; + imap->tunnel->out =-1 ; +if (start_command (imap->tunnel))die ("cannot start proxy %s" , srvc->tunnel); - imap->buf.sock.fd[0 ] = tunnel.out; - imap->buf.sock.fd[1 ] = tunnel.in; + imap->buf.sock.fd[0 ] = imap->tunnel->out; + imap->buf.sock.fd[1 ] = imap->tunnel->in;imap_info ("OK\n" ); }else {
Signature · 1 line
gitgitgadget
PATCH v5 2/4 imap-send: accept messages with Subject before Date
In reply to Harald Nordgren via GitGitGadgetFrom: Harald Nordgren <[email protected]>
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, which ends at the first empty line whether the mbox uses LF or CRLF line endings.
--- imap-send.c | 25 +++++++++++++++++-------- t/t9001-send-email.sh | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 8 deletions(-)
imap-send.c +17 -8
@@ -1457,6 +1457,22 @@ 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' && !starts_with (line,"\r\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 ;
@@ -1472,14 +1488,7 @@ static int count_messages(struct strbuf *all_msgs)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 ; + }else if (has_required_headers (p)) { count++; } }
t/t9001-send-email.sh +38 -0
@@ -2863,4 +2863,42 @@ 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 <[email protected]>" \ + [email protected] \ + --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-send ignores headers in the body of a CRLF mbox' ' + test_config imap.tunnel ./fake-imap-server && + q_to_cr >crlf.mbox <<-\EOF && + From mboxrd@z Thu Jan 1 00:00:00 1970Q + From: Example <[email protected]>Q + Subject: Fwd: originalQ + Q + ---------- Forwarded message ---------Q + From: Someone <[email protected]>Q + Date: Thu, 1 Jan 1970 00:00:00 +0000Q + Subject: originalQ + EOF + test_must_fail git imap-send --folder=INBOX <crlf.mbox 2>err && + test_grep "no messages found to send" err +' + test_done
Signature · 1 line
gitgitgadget
PATCH v5 3/4 imap-send: check git send-email copies like other messages
In reply to Harald Nordgren via GitGitGadgetFrom: Harald Nordgren <[email protected]>
The copy that git send-email --imap-sent-folder pipes to imap-send starts with a "From git-send-email" line. 04133f5bc4 (send-email: add ability to send a copy of sent emails to an IMAP folder, 2025-08-12) made imap-send count such a message when it has From and To headers, without looking for Date and Subject. Without that special case the copy would not be counted at all, because git send-email writes Subject before Date.
Now that the headers are accepted in any order, the special case is no longer needed. The copy carries the same headers git send-email sends over SMTP, which always include From, Subject and Date, so it passes the check for other messages. A To header is no longer required, the same as for any other message.
This makes the check stricter when some other script writes an mbox with a "From git-send-email" line. RFC 5322 requires a Date header but only recommends Subject, so a valid message from such a script may lack a Subject and would no longer be counted. imap-send has always required a Subject under any other "From " line, though, and only refuses an mbox when none of its messages are counted, so treating these copies like the rest should not affect anyone in practice.
--- imap-send.c | 15 ++------------- t/t9001-send-email.sh | 13 +++++++++++++ 2 files changed, 15 insertions(+), 13 deletions(-)
imap-send.c +2 -13
@@ -1479,19 +1479,8 @@ static int count_messages(struct strbuf *all_msgs)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 if (has_required_headers (p)) { - count++; - } - } +if (starts_with (p,"From " ) &&has_required_headers (p)) + count++; p =strstr (p+5 ,"\nFrom " );if (!p)break ;
t/t9001-send-email.sh +13 -0
@@ -2901,4 +2901,17 @@ test_expect_success $PREREQ 'imap-send ignores headers in the body of a CRLF mbo test_grep "no messages found to send" 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 <[email protected]>" \ + [email protected] \ + --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
Signature · 1 line
gitgitgadget
PATCH v5 4/4 imap-send: match header names case insensitively
In reply to Harald Nordgren via GitGitGadgetFrom: Harald Nordgren <[email protected]>
imap-send says "no messages found to send" for a message whose headers are spelled "from:", "date:" or "subject:", although RFC 5322 treats header field names as case insensitive. Match the three names in any case.
--- imap-send.c | 6 +++--- t/t9001-send-email.sh | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-)
imap-send.c +3 -3
@@ -1463,11 +1463,11 @@ static bool has_required_headers(const char *line)while ((line =strchr (line,'\n' )) && *++line !='\n' && !starts_with (line,"\r\n" )) { -if (starts_with (line,"From: " )) +if (istarts_with (line,"From: " )) from = true; -else if (starts_with (line,"Date: " )) +else if (istarts_with (line,"Date: " )) date = true; -else if (starts_with (line,"Subject: " )) +else if (istarts_with (line,"Subject: " )) subject = true; }return from && date && subject;
t/t9001-send-email.sh +15 -0
@@ -2914,4 +2914,19 @@ test_expect_success $PREREQ '--imap-sent-folder stores a copy of the sent patch' test_grep "(1/1) done" err ' +test_expect_success $PREREQ 'imap-send matches header names in any case' ' + test_config imap.tunnel ./fake-imap-server && + cat >lowercase.mbox <<-\EOF && + From mboxrd@z Thu Jan 1 00:00:00 1970 + from: Example <[email protected]> + date: Thu, 1 Jan 1970 00:00:00 +0000 + subject: [PATCH] lowercase headers + + body + EOF + git imap-send --folder=INBOX <lowercase.mbox 2>err && + test_grep "^Sending 1 message to INBOX folder" err && + test_grep "(1/1) done" err +' + test_done
Signature · 1 line
gitgitgadget
Re: [PATCH v5 0/4] imap-send: fix with reordered message headers
In reply to Harald Nordgren via GitGitGadgetHi Patrick!
Is this ready to be merged?
Harald