From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 21 Oct 2026 08:25:02 GMT
Subject: Re: [PATCH v2 2/2] imap-send: accept messages with Subject before Date
Message-ID: <ath23vYy0n8UFLUB@pks.im>
In-Reply-To: <c42827a1225e49db2e0855638e4e76a06f7d9eba.1792566469.git.gitgitgadget@gmail.com>

On Wed, Oct 21, 2026 at 07:07:49AM +0000, Harald Nordgren via GitGitGadget wrote:
> 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, 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.

> diff --git a/imap-send.c b/imap-send.c
> index 6f76bade35..7ae2eeddc9 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 (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++;

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.

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

Quite weird that every single test carries this prerequisite. Makes me
wonder why we don't just skip all tests at once.

Patrick