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

["Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com>]

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 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 <haraldnordgren@gmail.com>
      
       ## 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.
      
          Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
      
       ## 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 <haraldnordgren@gmail.com>
      
     @@ 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 <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
 -:  ---------- > 3:  213c4950e0 imap-send: check git send-email copies like other messages
 -:  ---------- > 4:  01add80af2 imap-send: match header names case insensitively

-- 
gitgitgadget