[PATCH v2 0/3] format-patch: teach `--header-cmd`

From: Kristoffer Haugsbakk <code@khaugsbakk.name>
To: git@vger.kernel.org
Cc: "Kristoffer Haugsbakk" <code@khaugsbakk.name>,
	"Jeff King" <peff@peff.net>,
	"Maxim Cournoyer" <maxim.cournoyer@gmail.com>,
	"Jean-Noël Avila" <avila.jn@gmail.com>
Subject: [PATCH v2 0/3] format-patch: teach `--header-cmd`
Date: Tue, 19 Mar 2024 19:35:35 +0100	[thread overview]
Message-ID: <cover.1710873210.git.code@khaugsbakk.name> (raw)
In-Reply-To: <cover.1709841147.git.code@khaugsbakk.name>

(most of this is from the main commit/patch with some elaboration in
parts)

Teach git-format-patch(1) `--header-cmd` (with negation) and the
accompanying config variable `format.headerCmd` which allows the user to
add extra headers per-patch.

§ Motivation

format-patch knows `--add-header`. However, that seems most useful for
series-wide headers; you cannot really control what the header is like
per patch or specifically for the cover letter. To that end, teach
format-patch a new option which runs a command that has access to the
hash of the current commit (if it is a code patch) and the patch count
which is used for the patch files that this command outputs. Also
include an environment variable which tells the version of this API so
that the command can detect and error out in case the API changes.

This is inspired by `--header-cmd` of git-send-email(1).[1]

🔗 1: https://lore.kernel.org/git/20230423122744.4865-1-maxim.cournoyer@gmail.com/

§ Discussion

One can already get the commit hash from the first line of the patch file:

    From 04967d53399ed2db09d224008f557ec1a5847cc1 Mon Sep 17 00:00:00 2001

However, the point of this option is to be able to calculate header
output based primarily on the commit hash without having to go back and
forth between commands (i.e. running format-patch and then parsing the
patch files).

As an example, the command could output a header for the current commit
as well as another header for the previously-published commits:

    X-Commit-Hash: 97b53c04894578b23d0c650f69885f734699afc7
    X-Previous-Commits:
        4ad5d4190649dcb5f26c73a6f15ab731891b9dfd
        d275d1d179b90592ddd7b5da2ae4573b3f7a37b7
        402b7937951073466bf4527caffd38175391c7da

One can imagine that (1) these previous commit hashes were stored on every
commit rewrite and (2) the commits that had been published previously
were also stored. Then the command just needed the current commit hash
in order to look up this information.

Now interested parties can use this information to track where the
patches come from.

This information could of course be given between the three-dash/three-
hyphen line and the patch proper. However, the hypothetical project in
question might prefer to use this part for extra patch information
written by the author and leave the above information for tooling; this
way the extra information does not need to disturb the reader.

§ Demonstration

The above current/previous hash example is taken from:

https://lore.kernel.org/git/97b53c04894578b23d0c650f69885f734699afc7.1709670287.git.code@khaugsbakk.name/

§ Changes in v2

Changes based on feedback from Jean-Noël and Peff.

Peff suggested changes to the strbuf handling in `log_tree`, removing
constness in order to facilitate freeing without casting, and fleshing
out the preliminary patch.

In detail:

• revision: add a per-email field to rev-info
  • Replaces (subject) “log-tree: take ownership of pointer”
  • Separate out changes for `rev.info.pe_headers`, changes to
    ownership, const, and strbuf handling in `log_tree`
  • Link: https://lore.kernel.org/git/20240313065454.GB125150@coredump.intra.peff.net/
• format-patch: teach `--header-cmd`
  • Simplify tests: use `true` and `false` as commands
  • Fix indentation in code
  • Use AsciiDoc definition list
    • Link: https://lore.kernel.org/git/53ea3745-205b-40c0-a4c5-9be26d9b88bf@gmail.com/
• format-patch: check if header output looks valid
  • (no changes)

§ CC

Cc: Jeff King <peff@peff.net>
Cc: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Cc: "Jean-Noël Avila" <avila.jn@gmail.com>

§ CI

https://github.com/LemmingAvalanche/git/actions/runs/8332742200/job/22802597793

Fails but looks similar to the failures for `master` at 3bd955d2691 (The
ninth batch, 2024-03-18):

https://github.com/git/git/actions/runs/8333455189/job/22804953254

Kristoffer Haugsbakk (3):
  revision: add a per-email field to rev-info
  format-patch: teach `--header-cmd`
  format-patch: check if header output looks valid

 Documentation/config/format.txt    |  5 ++
 Documentation/git-format-patch.txt | 23 +++++++++
 builtin/log.c                      | 76 ++++++++++++++++++++++++++++++
 log-tree.c                         | 21 +++++----
 log-tree.h                         |  2 +-
 pretty.h                           |  2 +-
 revision.h                         |  4 +-
 t/t4014-format-patch.sh            | 49 +++++++++++++++++++
 8 files changed, 169 insertions(+), 13 deletions(-)

Range-diff against v1:
1:  3b12a8cf393 < -:  ----------- log-tree: take ownership of pointer
-:  ----------- > 1:  9a7102b708e revision: add a per-email field to rev-info
2:  f405a0140b5 ! 2:  8c511797a47 format-patch: teach `--header-cmd`
    @@ Commit message

      ## Notes (series) ##
    -    Documentation/config/format.txt:
    -    • I get the impression that `_` is the convention for placeholders now:
    -      `_<cmd>_`
    +    v2:
    +    • Simplify tests: use `true` and `false` as commands
    +    • Fix indentation
    +    • Don’t use `const` for owned pointer (avoid cast when freeing)
    +      • Link: https://lore.kernel.org/git/cover.1709841147.git.code@khaugsbakk.name/T/#m12d104a5a769c7f6e02b1d0a75855142004e9206
    +    • Use AsciiDoc definition list
    +      • Link: https://lore.kernel.org/git/53ea3745-205b-40c0-a4c5-9be26d9b88bf@gmail.com/

      ## Documentation/config/format.txt ##
     @@ Documentation/config/format.txt: format.headers::
    @@ Documentation/git-format-patch.txt: feeding the result to `git send-email`.
     ++
     +_<cmd>_ has access to these environment variables:
     ++
    -+	GIT_FP_HEADER_CMD_VERSION
    -++
    -+The version of this API. Currently `1`. _<cmd>_ may return exit code
    -+`2` in order to signal that it does not support the given version.
    -++
    -+	GIT_FP_HEADER_CMD_HASH
    -++
    -+The hash of the commit corresponding to the current patch. Not set if
    -+the current patch is the cover letter.
    -++
    -+	GIT_FP_HEADER_CMD_COUNT
    -++
    -+The current patch count. Increments for each patch.
    ++--
    ++GIT_FP_HEADER_CMD_VERSION;;
    ++	The version of this API. Currently `1`. _<cmd>_ may return exit code
    ++	`2` in order to signal that it does not support the given version.
    ++GIT_FP_HEADER_CMD_HASH;;
    ++	The hash of the commit corresponding to the current patch. Not set if
    ++	the current patch is the cover letter.
    ++GIT_FP_HEADER_CMD_COUNT;;
    ++	The current patch count. Increments for each patch.
    ++--
     ++
     +`git format-patch` will error out if _<cmd>_ returns a non-zero exit
     +code.
    @@ builtin/log.c: static void make_cover_letter(struct rev_info *rev, int use_separ
      		show_range_diff(rev->rdiff1, rev->rdiff2, &range_diff_opts);
      		strvec_clear(&other_arg);
      	}
    -+	free((char *)pp.after_subject);
    ++	free(pp.after_subject);
      }

      static char *clean_message_id(const char *msg_id)
    @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
      		/* interdiff/range-diff in cover-letter; omit from patches */
      		rev.idiff_oid1 = NULL;
      		rev.rdiff1 = NULL;
    -+		free((char *)rev.pe_headers);
    ++		free(rev.pe_headers);
      	}
      	rev.add_signoff = do_signoff;

    @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
      		}
      		if (output_directory)
      			fclose(rev.diffopt.file);
    -+		free((char *)rev.pe_headers);
    ++		free(rev.pe_headers);
      	}
      	stop_progress(&progress);
      	free(list);

    - ## log-tree.c ##
    -@@ log-tree.c: void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)
    - 	}
    - }
    - 
    -+static char *extra_and_pe_headers(const char *extra_headers, const char *pe_headers) {
    -+	struct strbuf all_headers = STRBUF_INIT;
    -+
    -+	if (extra_headers)
    -+		strbuf_addstr(&all_headers, extra_headers);
    -+	if (pe_headers) {
    -+		strbuf_addstr(&all_headers, pe_headers);
    -+	}
    -+	return strbuf_detach(&all_headers, NULL);
    -+}
    -+
    - void log_write_email_headers(struct rev_info *opt, struct commit *commit,
    - 			     const char **extra_headers_p,
    - 			     int *need_8bit_cte_p,
    - 			     int maybe_multipart)
    - {
    --	const char *extra_headers = opt->extra_headers;
    -+	const char *extra_headers =
    -+		extra_and_pe_headers(opt->extra_headers, opt->pe_headers);
    - 	const char *name = oid_to_hex(opt->zero_commit ?
    - 				      null_oid() : &commit->object.oid);
    - 
    -@@ log-tree.c: void log_write_email_headers(struct rev_info *opt, struct commit *commit,
    - 			 extra_headers ? extra_headers : "",
    - 			 mime_boundary_leader, opt->mime_boundary,
    - 			 mime_boundary_leader, opt->mime_boundary);
    -+		free((char *)extra_headers);
    - 		extra_headers = strbuf_detach(&subject_buffer, NULL);
    - 
    - 		if (opt->numbered_files)
    -@@ log-tree.c: void show_log(struct rev_info *opt)
    - 
    - 	strbuf_release(&msgbuf);
    - 	free(ctx.notes_message);
    -+	free((char *)ctx.after_subject);
    - 
    - 	if (cmit_fmt_is_mail(ctx.fmt) && opt->idiff_oid1) {
    - 		struct diff_queue_struct dq;
    -
    - ## revision.h ##
    -@@ revision.h: struct rev_info {
    - 	struct string_list *ref_message_ids;
    - 	int		add_signoff;
    - 	const char	*extra_headers;
    -+	/* per-email headers */
    -+	const char	*pe_headers;
    - 	const char	*log_reencode;
    - 	const char	*subject_prefix;
    - 	int		patch_name_max;
    -
      ## t/t4014-format-patch.sh ##
     @@ t/t4014-format-patch.sh: test_expect_failure 'configuration To: header (rfc2047)' '
      	grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\$" hdrs9
    @@ t/t4014-format-patch.sh: test_expect_failure 'configuration To: header (rfc2047)
     +'
     +
     +test_expect_success '--header-cmd with no output works' '
    -+	write_script cmd <<-\EOF &&
    -+	exit 0
    -+	EOF
    -+	git format-patch --header-cmd=./cmd --stdout main..side
    ++	git format-patch --header-cmd=true --stdout main..side
     +'
     +
     +test_expect_success '--header-cmd reports failed command' '
    -+	write_script cmd <<-\EOF &&
    -+	exit 1
    -+	EOF
    -+		cat > expect <<-\EOF &&
    -+	fatal: header-cmd ./cmd: failed with exit code 1
    ++	cat > expect <<-\EOF &&
    ++	fatal: header-cmd false: failed with exit code 1
     +	EOF
    -+	test_must_fail git format-patch --header-cmd=./cmd --stdout main..side >actual 2>&1 &&
    ++	test_must_fail git format-patch --header-cmd=false --stdout main..side >actual 2>&1 &&
     +	test_cmp expect actual
     +'
     +
3:  0e8409227e4 ! 3:  c570467c8db format-patch: check if header output looks valid
    @@ builtin/log.c: static char *header_cmd_output(struct rev_info *rev, const struct

      ## t/t4014-format-patch.sh ##
     @@ t/t4014-format-patch.sh: test_expect_success '--header-cmd with no output works' '
    - 	git format-patch --header-cmd=./cmd --stdout main..side
    + 	git format-patch --header-cmd=true --stdout main..side
      '

     +test_expect_success '--header-cmd without headers-like output fails' '
    @@ t/t4014-format-patch.sh: test_expect_success '--header-cmd with no output works'
     +'
     +
      test_expect_success '--header-cmd reports failed command' '
    - 	write_script cmd <<-\EOF &&
    - 	exit 1
    + 	cat > expect <<-\EOF &&
    + 	fatal: header-cmd false: failed with exit code 1
-- 
2.44.0.144.g29ae9861142