{"thread":{"id":"61071","subject":"[PATCH 0/3] format-patch: teach `--header-cmd`","startedAt":"2024-03-07T20:00:18Z","lastAt":"2024-03-22T22:31:31Z","messageCount":34,"participants":["Kristoffer Haugsbakk","Jean-Noël Avila","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"490206","messageId":"cover.1709841147.git.code@khaugsbakk.name","threadId":"61071","inReplyTo":null,"subject":"[PATCH 0/3] format-patch: teach `--header-cmd`","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-07T19:59:34Z","receivedAt":"2024-03-07T20:00:18Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"(most of this is from the main commit with some elaboration in parts)\n\nTeach git-format-patch(1) `--header-cmd` (with negation) and the\naccompanying config variable `format.headerCmd` which allows the user to\nadd extra headers per-patch.\n\n§ Motivation\n\nformat-patch knows `--add-header`. However, that seems most useful for\nseries-wide headers; you cannot really control what the header is like\nper patch or specifically for the cover letter. To that end, teach\nformat-patch a new option which runs a command that has access to the\nhash of the current commit (if it is a code patch) and the patch count\nwhich is used for the patch files that this command outputs. Also\ninclude an environment variable which tells the version of this API so\nthat the command can detect and error out in case the API changes.\n\nThis is inspired by `--header-cmd` of git-send-email(1).\n\n§ Discussion\n\nOne may already get the commit hash from the commit by looking at the message id created by format-patch:\n\n Message-ID: <48c517ffb3dce2188aba5f0a2c1f4f9dc8df59f0.1709840255.git.code@khaugsbakk.name>\n\nHowever that does not seem to be documented behavior, and relying on the\nmessage id from various tools seems to have backfired before.[1]\n\nIt’s also more convenient to not have to parse any output from\nformat-patch.\n\nOne may also be interested in finding some information based on the\ncommit hash, not just simply to output the hash itself.\n\n🔗 1: https://lore.kernel.org/git/20231106153214.s5abourejkuiwk64@pengutronix.de/\n\nFor example, the command could output a header for the current commit as\nwell as another header for the previously-published commits:\n\n X-Commit-Hash: 97b53c04894578b23d0c650f69885f734699afc7\n X-Previous-Commits:\n 4ad5d4190649dcb5f26c73a6f15ab731891b9dfd\n d275d1d179b90592ddd7b5da2ae4573b3f7a37b7\n 402b7937951073466bf4527caffd38175391c7da\n\nOne can imagine that (1) these previous commit hashes were stored on every\ncommit rewrite and (2) the commits that had been published previously\nwere also stored. Then the command just needed the current commit hash\nin order to look up this information.\n\nNow interested parties can use this information to track where the\npatches come from.\n\nThis information could of course be given between the\nthree-dash/three-hyphen line and the patch proper. However, the\nhypoethetical project in question might prefer to use this part for\nextra patch information written by the author and leave the above\ninformation for tooling; this way the extra information does not need to\ndisturb the reader.\n\n§ Demonstration\n\nThe above current/previous hash example is taken from:\n\nhttps://lore.kernel.org/git/97b53c04894578b23d0c650f69885f734699afc7.1709670287.git.code@khaugsbakk.name/\n\n§ CC\n\nFor patch “log-tree: take ownership of pointer”:\n\nCc: Jeff King \n\nFor the git-send-email(1) `--header-cmd` topic:[1]\n\nCc: Maxim Cournoyer \n\n🔗 1: https://lore.kernel.org/git/20230423122744.4865-1-maxim.cournoyer@gmail.com/\n\nKristoffer Haugsbakk (3):\n log-tree: take ownership of pointer\n format-patch: teach `--header-cmd`\n format-patch: check if header output looks valid\n\n Documentation/config/format.txt | 5 ++\n Documentation/git-format-patch.txt | 26 ++++++++++\n builtin/log.c | 76 ++++++++++++++++++++++++++++++\n log-tree.c | 18 ++++++-\n revision.h | 2 +\n t/t4014-format-patch.sh | 55 +++++++++++++++++++++\n 6 files changed, 180 insertions(+), 2 deletions(-)\n\n-- \n2.44.0.169.gd259cac85a8\n\n"},{"id":"490207","messageId":"3b12a8cf393b6d8f0877fd7d87173c565d7d5a90.1709841147.git.code@khaugsbakk.name","threadId":"61071","inReplyTo":"cover.1709841147.git.code@khaugsbakk.name","subject":"[PATCH 1/3] log-tree: take ownership of pointer","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-07T19:59:35Z","receivedAt":"2024-03-07T20:00:19Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"The MIME header handling started using string buffers in\nd50b69b868d (log_write_email_headers: use strbufs, 2018-05-18). The\nsubject buffer is given to `extra_headers` without that variable taking\nownership; the commit “punts on that ownership” (in general, not just\nfor this buffer).\n\nIn an upcoming commit we will first assign `extra_headers` to the owned\npointer from another `strbuf`. In turn we need this variable to always\ncontain an owned pointer so that we can free it in the calling\nfunction.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n log-tree.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 337b9334cdb..2eabd19962b 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -519,7 +519,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t extra_headers ? extra_headers : \"\",\n \t\t\t mime_boundary_leader, opt->mime_boundary,\n \t\t\t mime_boundary_leader, opt->mime_boundary);\n-\t\textra_headers = subject_buffer.buf;\n+\t\textra_headers = strbuf_detach(&subject_buffer, NULL);\n \n \t\tif (opt->numbered_files)\n \t\t\tstrbuf_addf(&filename, \"%d\", opt->nr);\n-- \n2.44.0.169.gd259cac85a8\n\n"},{"id":"490208","messageId":"f405a0140b5655bc66a0a7a603517a421d7669cf.1709841147.git.code@khaugsbakk.name","threadId":"61071","inReplyTo":"cover.1709841147.git.code@khaugsbakk.name","subject":"[PATCH 2/3] format-patch: teach `--header-cmd`","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-07T19:59:36Z","receivedAt":"2024-03-07T20:00:20Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Teach git-format-patch(1) `--header-cmd` (with negation) and the\naccompanying config variable `format.headerCmd` which allows the user to\nadd extra headers per-patch.\n\nformat-patch knows `--add-header`. However, that seems most useful for\nseries-wide headers; you cannot really control what the header is like\nper patch or specifically for the cover letter. To that end, teach\nformat-patch a new option which runs a command that has access to the\nhash of the current commit (if it is a code patch) and the patch count\nwhich is used for the patch files that this command outputs. Also\ninclude an environment variable which tells the version of this API so\nthat the command can detect and error out in case the API changes.\n\nThis is inspired by `--header-cmd` of git-send-email(1).\n\n§ Discussion\n\nThe command can use the provided commit hash to provide relevant\ninformation in the header. For example, the command could output a\nheader for the current commit as well as the previously-published\ncommits:\n\n X-Commit-Hash: 97b53c04894578b23d0c650f69885f734699afc7\n X-Previous-Commits:\n 4ad5d4190649dcb5f26c73a6f15ab731891b9dfd\n d275d1d179b90592ddd7b5da2ae4573b3f7a37b7\n 402b7937951073466bf4527caffd38175391c7da\n\nNow interested parties can use this information to track where the\npatches come from.\n\nThis information could of course be given between the\nthree-dash/three-hyphen line and the patch proper. However, the project\nmight prefer to use this part for extra patch information written by the\nauthor and leave the above information for tooling; this way the extra\ninformation does not need to disturb the reader.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n Documentation/config/format.txt:\n • I get the impression that `_` is the convention for placeholders now:\n `__`\n\n Documentation/config/format.txt | 5 ++++\n Documentation/git-format-patch.txt | 26 ++++++++++++++++++\n builtin/log.c | 43 ++++++++++++++++++++++++++++++\n log-tree.c | 16 ++++++++++-\n revision.h | 2 ++\n t/t4014-format-patch.sh | 42 +++++++++++++++++++++++++++++\n 6 files changed, 133 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/format.txt b/Documentation/config/format.txt\nindex 7410e930e53..c184b865824 100644\n--- a/Documentation/config/format.txt\n+++ b/Documentation/config/format.txt\n@@ -31,6 +31,11 @@ format.headers::\n \tAdditional email headers to include in a patch to be submitted\n \tby mail. See linkgit:git-format-patch[1].\n \n+format.headerCmd::\n+\tCommand to run for each patch that should output RFC 2822 email\n+\theaders. Has access to some information per patch via\n+\tenvironment variables. See linkgit:git-format-patch[1].\n+\n format.to::\n format.cc::\n \tAdditional recipients to include in a patch to be submitted\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 728bb3821c1..41c344902e9 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -303,6 +303,32 @@ feeding the result to `git send-email`.\n \t`Cc:`, and custom) headers added so far from config or command\n \tline.\n \n+--[no-]header-cmd=::\n+\tRun __ for each patch. __ should output valid RFC 2822\n+\temail headers. This can also be configured with\n+\tthe configuration variable `format.headerCmd`. Can be turned off\n+\twith `--no-header-cmd`. This works independently of\n+\t`--[no-]add-header`.\n++\n+__ has access to these environment variables:\n++\n+\tGIT_FP_HEADER_CMD_VERSION\n++\n+The version of this API. Currently `1`. __ may return exit code\n+`2` in order to signal that it does not support the given version.\n++\n+\tGIT_FP_HEADER_CMD_HASH\n++\n+The hash of the commit corresponding to the current patch. Not set if\n+the current patch is the cover letter.\n++\n+\tGIT_FP_HEADER_CMD_COUNT\n++\n+The current patch count. Increments for each patch.\n++\n+`git format-patch` will error out if __ returns a non-zero exit\n+code.\n+\n --[no-]cover-letter::\n \tIn addition to the patches, generate a cover letter file\n \tcontaining the branch description, shortlog and the overall diffstat. You can\ndiff --git a/builtin/log.c b/builtin/log.c\nindex db1808d7c13..eecbcdf1d6d 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -43,10 +43,13 @@\n #include \"tmp-objdir.h\"\n #include \"tree.h\"\n #include \"write-or-die.h\"\n+#include \"run-command.h\"\n \n #define MAIL_DEFAULT_WRAP 72\n #define COVER_FROM_AUTO_MAX_SUBJECT_LEN 100\n #define FORMAT_PATCH_NAME_MAX_DEFAULT 64\n+#define HC_VERSION \"1\"\n+#define HC_NOT_SUPPORTED 2\n \n /* Set a default date-time format for git log (\"log.date\" config variable) */\n static const char *default_date_mode = NULL;\n@@ -902,6 +905,7 @@ static int auto_number = 1;\n \n static char *default_attach = NULL;\n \n+static const char *header_cmd = NULL;\n static struct string_list extra_hdr = STRING_LIST_INIT_NODUP;\n static struct string_list extra_to = STRING_LIST_INIT_NODUP;\n static struct string_list extra_cc = STRING_LIST_INIT_NODUP;\n@@ -1100,6 +1104,8 @@ static int git_format_config(const char *var, const char *value,\n \t\tformat_no_prefix = 1;\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"format.headercmd\"))\n+\t\treturn git_config_string(&header_cmd, var, value);\n \n \t/*\n \t * ignore some porcelain config which would otherwise be parsed by\n@@ -1419,6 +1425,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \t\tshow_range_diff(rev->rdiff1, rev->rdiff2, &range_diff_opts);\n \t\tstrvec_clear(&other_arg);\n \t}\n+\tfree((char *)pp.after_subject);\n }\n \n static char *clean_message_id(const char *msg_id)\n@@ -1865,6 +1872,35 @@ static void infer_range_diff_ranges(struct strbuf *r1,\n \t}\n }\n \n+/* Returns an owned pointer */\n+static char *header_cmd_output(struct rev_info *rev, const struct commit *cmit)\n+{\n+\tstruct child_process header_cmd_proc = CHILD_PROCESS_INIT;\n+\tstruct strbuf output = STRBUF_INIT;\n+\tint res;\n+\n+\tstrvec_pushl(&header_cmd_proc.args, header_cmd, NULL);\n+\tif (cmit)\n+\t\tstrvec_pushf(&header_cmd_proc.env, \"GIT_FP_HEADER_CMD_HASH=%s\",\n+\t\t\t oid_to_hex(&cmit->object.oid));\n+\tstrvec_pushl(&header_cmd_proc.env,\n+\t\t \"GIT_FP_HEADER_CMD_VERSION=\" HC_VERSION, NULL);\n+\tstrvec_pushf(&header_cmd_proc.env, \"GIT_FP_HEADER_CMD_COUNT=%\" PRIuMAX,\n+\t\t (uintmax_t)rev->nr);\n+\tres = capture_command(&header_cmd_proc, &output, 0);\n+\tif (res) {\n+\t\tif (res == HC_NOT_SUPPORTED)\n+\t\t\tdie(_(\"header-cmd %s: returned exit \"\n+\t\t\t \"code %d; the command does not support \"\n+\t\t\t \"version \" HC_VERSION),\n+\t\t\t header_cmd, HC_NOT_SUPPORTED);\n+\t\telse\n+\t\t\tdie(_(\"header-cmd %s: failed with exit code %d\"),\n+\t\t\t header_cmd, res);\n+\t}\n+\treturn strbuf_detach(&output, NULL);\n+}\n+\n int cmd_format_patch(int argc, const char **argv, const char *prefix)\n {\n \tstruct commit *commit;\n@@ -1955,6 +1991,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tOPT_GROUP(N_(\"Messaging\")),\n \t\tOPT_CALLBACK(0, \"add-header\", NULL, N_(\"header\"),\n \t\t\t N_(\"add email header\"), header_callback),\n+\t\tOPT_STRING(0, \"header-cmd\", &header_cmd, N_(\"email\"), N_(\"command that will be run to generate headers\")),\n \t\tOPT_STRING_LIST(0, \"to\", &extra_to, N_(\"email\"), N_(\"add To: header\")),\n \t\tOPT_STRING_LIST(0, \"cc\", &extra_cc, N_(\"email\"), N_(\"add Cc: header\")),\n \t\tOPT_CALLBACK_F(0, \"from\", &from, N_(\"ident\"),\n@@ -2321,6 +2358,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (cover_letter) {\n \t\tif (thread)\n \t\t\tgen_message_id(&rev, \"cover\");\n+\t\tif (header_cmd)\n+\t\t\trev.pe_headers = header_cmd_output(&rev, NULL);\n \t\tmake_cover_letter(&rev, !!output_directory,\n \t\t\t\t origin, nr, list, description_file, branch_name, quiet);\n \t\tprint_bases(&bases, rev.diffopt.file);\n@@ -2330,6 +2369,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t/* interdiff/range-diff in cover-letter; omit from patches */\n \t\trev.idiff_oid1 = NULL;\n \t\trev.rdiff1 = NULL;\n+\t\tfree((char *)rev.pe_headers);\n \t}\n \trev.add_signoff = do_signoff;\n \n@@ -2376,6 +2416,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\tgen_message_id(&rev, oid_to_hex(&commit->object.oid));\n \t\t}\n \n+\t\tif (header_cmd)\n+\t\t\trev.pe_headers = header_cmd_output(&rev, commit);\n \t\tif (output_directory &&\n \t\t open_next_file(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))\n \t\t\tdie(_(\"failed to create output files\"));\n@@ -2402,6 +2444,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tif (output_directory)\n \t\t\tfclose(rev.diffopt.file);\n+\t\tfree((char *)rev.pe_headers);\n \t}\n \tstop_progress(&progress);\n \tfree(list);\ndiff --git a/log-tree.c b/log-tree.c\nindex 2eabd19962b..3ca383d099f 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -469,12 +469,24 @@ void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)\n \t}\n }\n \n+static char *extra_and_pe_headers(const char *extra_headers, const char *pe_headers) {\n+\tstruct strbuf all_headers = STRBUF_INIT;\n+\n+\tif (extra_headers)\n+\t\tstrbuf_addstr(&all_headers, extra_headers);\n+\tif (pe_headers) {\n+\t\tstrbuf_addstr(&all_headers, pe_headers);\n+\t}\n+\treturn strbuf_detach(&all_headers, NULL);\n+}\n+\n void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t const char **extra_headers_p,\n \t\t\t int *need_8bit_cte_p,\n \t\t\t int maybe_multipart)\n {\n-\tconst char *extra_headers = opt->extra_headers;\n+\tconst char *extra_headers =\n+\t\textra_and_pe_headers(opt->extra_headers, opt->pe_headers);\n \tconst char *name = oid_to_hex(opt->zero_commit ?\n \t\t\t\t null_oid() : &commit->object.oid);\n \n@@ -519,6 +531,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t extra_headers ? extra_headers : \"\",\n \t\t\t mime_boundary_leader, opt->mime_boundary,\n \t\t\t mime_boundary_leader, opt->mime_boundary);\n+\t\tfree((char *)extra_headers);\n \t\textra_headers = strbuf_detach(&subject_buffer, NULL);\n \n \t\tif (opt->numbered_files)\n@@ -857,6 +870,7 @@ void show_log(struct rev_info *opt)\n \n \tstrbuf_release(&msgbuf);\n \tfree(ctx.notes_message);\n+\tfree((char *)ctx.after_subject);\n \n \tif (cmit_fmt_is_mail(ctx.fmt) && opt->idiff_oid1) {\n \t\tstruct diff_queue_struct dq;\ndiff --git a/revision.h b/revision.h\nindex 94c43138bc3..eb36bdea36e 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -291,6 +291,8 @@ struct rev_info {\n \tstruct string_list *ref_message_ids;\n \tint\t\tadd_signoff;\n \tconst char\t*extra_headers;\n+\t/* per-email headers */\n+\tconst char\t*pe_headers;\n \tconst char\t*log_reencode;\n \tconst char\t*subject_prefix;\n \tint\t\tpatch_name_max;\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex e37a1411ee2..dfda21d4b2b 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -238,6 +238,48 @@ test_expect_failure 'configuration To: header (rfc2047)' '\n \tgrep \"^To: =?UTF-8?q?R=20=C3=84=20Cipient?= \\$\" hdrs9\n '\n \n+test_expect_success '--header-cmd' '\n+\twrite_script cmd <<-\\EOF &&\n+\tprintf \"X-S: $GIT_FP_HEADER_CMD_HASH\\n\"\n+\tprintf \"X-V: $GIT_FP_HEADER_CMD_VERSION\\n\"\n+\tprintf \"X-C: $GIT_FP_HEADER_CMD_COUNT\\n\"\n+\tEOF\n+\texpect_sha1=$(git rev-parse side) &&\n+\tgit format-patch --header-cmd=./cmd --stdout main..side >patch &&\n+\tgrep \"^X-S: $expect_sha1\" patch &&\n+\tgrep \"^X-V: 1\" patch &&\n+\tgrep \"^X-C: 3\" patch\n+'\n+\n+test_expect_success '--header-cmd with no output works' '\n+\twrite_script cmd <<-\\EOF &&\n+\texit 0\n+\tEOF\n+\tgit format-patch --header-cmd=./cmd --stdout main..side\n+'\n+\n+test_expect_success '--header-cmd reports failed command' '\n+\twrite_script cmd <<-\\EOF &&\n+\texit 1\n+\tEOF\n+\t\tcat > expect <<-\\EOF &&\n+\tfatal: header-cmd ./cmd: failed with exit code 1\n+\tEOF\n+\ttest_must_fail git format-patch --header-cmd=./cmd --stdout main..side >actual 2>&1 &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--header-cmd reports exit code 2' '\n+\twrite_script cmd <<-\\EOF &&\n+\texit 2\n+\tEOF\n+\tcat > expect <<-\\EOF &&\n+\tfatal: header-cmd ./cmd: returned exit code 2; the command does not support version 1\n+\tEOF\n+\ttest_must_fail git format-patch --header-cmd=./cmd --stdout main..side >actual 2>&1 &&\n+\ttest_cmp expect actual\n+'\n+\n # check_patch : Verify that looks like a half-sane\n # patch email to avoid a false positive with !grep\n check_patch () {\n-- \n2.44.0.169.gd259cac85a8\n\n"},{"id":"490209","messageId":"0e8409227e4e8eb73bac7dff97e3f53584b4e283.1709841147.git.code@khaugsbakk.name","threadId":"61071","inReplyTo":"cover.1709841147.git.code@khaugsbakk.name","subject":"[PATCH 3/3] format-patch: check if header output looks valid","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-07T19:59:37Z","receivedAt":"2024-03-07T20:00:22Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Implement a function based on `mailinfo.c:is_mail`.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n Isolating this for review as its own commit so that I can point out the\n provenance. May well be squashed into the main patch eventually.\n\n builtin/log.c | 33 +++++++++++++++++++++++++++++++++\n t/t4014-format-patch.sh | 13 +++++++++++++\n 2 files changed, 46 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex eecbcdf1d6d..27e1a66dd03 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1872,6 +1872,35 @@ static void infer_range_diff_ranges(struct strbuf *r1,\n \t}\n }\n \n+static int is_mail(struct strbuf *sb)\n+{\n+\tconst char *header_regex = \"^[!-9;-~]+:\";\n+\tregex_t regex;\n+\tint ret = 1, i;\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\n+\tif (regcomp(®ex, header_regex, REG_NOSUB | REG_EXTENDED))\n+\t\tdie(\"invalid pattern: %s\", header_regex);\n+\tstring_list_split(&list, sb->buf, '\\n', -1);\n+\tfor (i = 0; i < list.nr; i++) {\n+\t\t/* End of header */\n+\t\tif (!*list.items[i].string && i == (list.nr - 1))\n+\t\t\tbreak;\n+\t\t/* Ignore indented folded lines */\n+\t\tif (*list.items[i].string == '\\t' ||\n+\t\t *list.items[i].string == ' ')\n+\t\t\tcontinue;\n+\t\t/* It's a header if it matches header_regex */\n+\t\tif (regexec(®ex, list.items[i].string, 0, NULL, 0)) {\n+\t\t\tret = 0;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\tstring_list_clear(&list, 1);\n+\tregfree(®ex);\n+\treturn ret;\n+}\n+\n /* Returns an owned pointer */\n static char *header_cmd_output(struct rev_info *rev, const struct commit *cmit)\n {\n@@ -1898,6 +1927,10 @@ static char *header_cmd_output(struct rev_info *rev, const struct commit *cmit)\n \t\t\tdie(_(\"header-cmd %s: failed with exit code %d\"),\n \t\t\t header_cmd, res);\n \t}\n+\tif (!is_mail(&output))\n+\t\tdie(_(\"header-cmd %s: returned output which was \"\n+\t\t \"not recognized as valid RFC 2822 headers\"),\n+\t\t header_cmd);\n \treturn strbuf_detach(&output, NULL);\n }\n \ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex dfda21d4b2b..98e0eb706e6 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -258,6 +258,19 @@ test_expect_success '--header-cmd with no output works' '\n \tgit format-patch --header-cmd=./cmd --stdout main..side\n '\n \n+test_expect_success '--header-cmd without headers-like output fails' '\n+\twrite_script cmd <<-\\EOF &&\n+\tprintf \"X-S: $GIT_FP_HEADER_CMD_HASH\\n\"\n+\tprintf \"\\n\"\n+\tprintf \"X-C: $GIT_FP_HEADER_CMD_COUNT\\n\"\n+\tEOF\n+\tcat > expect <<-\\EOF &&\n+\tfatal: header-cmd ./cmd: returned output which was not recognized as valid RFC 2822 headers\n+\tEOF\n+\ttest_must_fail git format-patch --header-cmd=./cmd --stdout main..side >actual 2>&1 &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--header-cmd reports failed command' '\n \twrite_script cmd <<-\\EOF &&\n \texit 1\n-- \n2.44.0.169.gd259cac85a8\n\n"},{"id":"490263","messageId":"108afd9c-e158-4366-853e-3a384c77a452@app.fastmail.com","threadId":"61071","inReplyTo":"f405a0140b5655bc66a0a7a603517a421d7669cf.1709841147.git.code@khaugsbakk.name","subject":"Re: [PATCH 2/3] format-patch: teach `--header-cmd`","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-08T18:30:24Z","receivedAt":"2024-03-08T18:30:47Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Thu, Mar 7, 2024, at 20:59, Kristoffer Haugsbakk wrote:\n> +test_expect_success '--header-cmd with no output works' '\n> +\twrite_script cmd <<-\\EOF &&\n> +\texit 0\n> +\tEOF\n> +\tgit format-patch --header-cmd=./cmd --stdout main..side\n> +'\n\nThis can be simplified to `--header-cmd=true`.\n"},{"id":"490398","messageId":"53ea3745-205b-40c0-a4c5-9be26d9b88bf@gmail.com","threadId":"61071","inReplyTo":"f405a0140b5655bc66a0a7a603517a421d7669cf.1709841147.git.code@khaugsbakk.name","subject":"Re: [PATCH 2/3] format-patch: teach `--header-cmd`","fromName":"Jean-Noël Avila","fromEmail":"avila.jn@gmail.com","sentAt":"2024-03-11T21:29:53Z","receivedAt":"2024-03-11T21:29:59Z","isPatch":true,"sender":{"key":"jn.avila@free.fr","avatar":"https://avatars.githubusercontent.com/u/156172?v=4"},"body":"Le 07/03/2024 à 20:59, Kristoffer Haugsbakk a écrit :\n> \n> +format.headerCmd::\n> +\tCommand to run for each patch that should output RFC 2822 email\n> +\theaders. Has access to some information per patch via\n> +\tenvironment variables. See linkgit:git-format-patch[1].\n> +\n> format.to::\n> format.cc::\n> \tAdditional recipients to include in a patch to be submitted\n> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\n> index 728bb3821c1..41c344902e9 100644\n> --- a/Documentation/git-format-patch.txt\n> +++ b/Documentation/git-format-patch.txt\n> @@ -303,6 +303,32 @@ feeding the result to `git send-email`.\n> \t`Cc:`, and custom) headers added so far from config or command\n> \tline.\n> \n> +--[no-]header-cmd=::\n> +\tRun __ for each patch. __ should output valid RFC 2822\n> +\temail headers. This can also be configured with\n> +\tthe configuration variable `format.headerCmd`. Can be turned off\n> +\twith `--no-header-cmd`. This works independently of\n> +\t`--[no-]add-header`.\n> ++\n> +__ has access to these environment variables:\n> ++\n> +\tGIT_FP_HEADER_CMD_VERSION\n\nBetter use a nested description list like this:\n\nGIT_FP_HEADER_CMD_VERSION;;\n The version of this API. Currently `1`. __ may return exit code\n `2` in order to signal that it does not support the given version.\n\n> ++\n> +The version of this API. Currently `1`. __ may return exit code\n> +`2` in order to signal that it does not support the given version.\n> ++\n> +\tGIT_FP_HEADER_CMD_HASH\n> ++\n> +The hash of the commit corresponding to the current patch. Not set if\n> +the current patch is the cover letter.\n> ++\n> +\tGIT_FP_HEADER_CMD_COUNT\n> ++\n> +The current patch count. Increments for each patch.\n> ++\n> +`git format-patch` will error out if __ returns a non-zero exit\n> +code.\n> +\n> --[no-]cover-letter::\n> \tIn addition to the patches, generate a cover letter file\n> \tcontaining the branch description, shortlog and the overall diffstat. You can\n\n\nOverall, thank you for correctly marking up placeholders and options.\n\n"},{"id":"490452","messageId":"d677ed9c-0f93-4fb9-a878-62711f9d6fdf@app.fastmail.com","threadId":"61071","inReplyTo":"53ea3745-205b-40c0-a4c5-9be26d9b88bf@gmail.com","subject":"Re: [PATCH 2/3] format-patch: teach `--header-cmd`","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-12T08:13:40Z","receivedAt":"2024-03-12T08:14:02Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Mon, Mar 11, 2024, at 22:29, Jean-Noël Avila wrote:\n>> +--[no-]header-cmd=::\n>> +\tRun __ for each patch. __ should output valid RFC 2822\n>> +\temail headers. This can also be configured with\n>> +\tthe configuration variable `format.headerCmd`. Can be turned off\n>> +\twith `--no-header-cmd`. This works independently of\n>> +\t`--[no-]add-header`.\n>> ++\n>> +__ has access to these environment variables:\n>> ++\n>> +\tGIT_FP_HEADER_CMD_VERSION\n>\n> Better use a nested description list like this:\n>\n> GIT_FP_HEADER_CMD_VERSION;;\n> The version of this API. Currently `1`. __ may return exit code\n> `2` in order to signal that it does not support the given version.\n>\n\nThanks, I’ll do that in the next version.\n\n>> ++\n>> +The version of this API. Currently `1`. __ may return exit code\n>> +`2` in order to signal that it does not support the given version.\n>> ++\n>> +\tGIT_FP_HEADER_CMD_HASH\n>> ++\n>> +The hash of the commit corresponding to the current patch. Not set if\n>> +the current patch is the cover letter.\n>> ++\n>> +\tGIT_FP_HEADER_CMD_COUNT\n>> ++\n>> +The current patch count. Increments for each patch.\n>> ++\n>> +`git format-patch` will error out if __ returns a non-zero exit\n>> +code.\n>> +\n>> --[no-]cover-letter::\n>> \tIn addition to the patches, generate a cover letter file\n>> \tcontaining the branch description, shortlog and the overall diffstat. You can\n>\n>\n> Overall, thank you for correctly marking up placeholders and options.\n\nThanks for reviewing!\n\n--\nKristoffer Haugsbakk\n"},{"id":"490475","messageId":"20240312092959.GA96171@coredump.intra.peff.net","threadId":"61071","inReplyTo":"3b12a8cf393b6d8f0877fd7d87173c565d7d5a90.1709841147.git.code@khaugsbakk.name","subject":"Re: [PATCH 1/3] log-tree: take ownership of pointer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-12T09:29:59Z","receivedAt":"2024-03-12T09:30:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 07, 2024 at 08:59:35PM +0100, Kristoffer Haugsbakk wrote:\n\n> The MIME header handling started using string buffers in\n> d50b69b868d (log_write_email_headers: use strbufs, 2018-05-18). The\n> subject buffer is given to `extra_headers` without that variable taking\n> ownership; the commit “punts on that ownership” (in general, not just\n> for this buffer).\n> \n> In an upcoming commit we will first assign `extra_headers` to the owned\n> pointer from another `strbuf`. In turn we need this variable to always\n> contain an owned pointer so that we can free it in the calling\n> function.\n\nHmm, OK. This patch by itself introduces a memory leak. It would be nice\nif we could couple it with the matching free() so that we can see that\nthe issue is fixed. It sounds like your patch 2 is going to introduce\nsuch a free, but I'm not sure it's complete. It frees the old\nextra_headers before reassigning it, but nobody cleans it up after\nhandling the final commit.\n\nWe should also drop the \"static\" from subject_buffer, if it is no longer\nneeded. Likewise, any strings that start owning memory (here or in patch\n2) should probably drop their \"const\". That makes the ownership more\nclear, and avoids ugly casts when freeing.\n\n-Peff\n"},{"id":"490495","messageId":"73a4cb87-2800-4ad1-b7a2-33c6465fcc50@app.fastmail.com","threadId":"61071","inReplyTo":"20240312092959.GA96171@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] log-tree: take ownership of pointer","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-12T17:43:55Z","receivedAt":"2024-03-12T17:44:19Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Hi Jeff and thanks for taking a look\n\nOn Tue, Mar 12, 2024, at 10:29, Jeff King wrote:\n> On Thu, Mar 07, 2024 at 08:59:35PM +0100, Kristoffer Haugsbakk wrote:\n>\n>> The MIME header handling started using string buffers in\n>> d50b69b868d (log_write_email_headers: use strbufs, 2018-05-18). The\n>> subject buffer is given to `extra_headers` without that variable taking\n>> ownership; the commit “punts on that ownership” (in general, not just\n>> for this buffer).\n>>\n>> In an upcoming commit we will first assign `extra_headers` to the owned\n>> pointer from another `strbuf`. In turn we need this variable to always\n>> contain an owned pointer so that we can free it in the calling\n>> function.\n>\n> Hmm, OK. This patch by itself introduces a memory leak. It would be nice\n> if we could couple it with the matching free() so that we can see that\n> the issue is fixed. It sounds like your patch 2 is going to introduce\n> such a free, but I'm not sure it's complete.\n\nIs it okay if it is done in patch 2?\n\n> It frees the old extra_headers before reassigning it, but nobody\n> cleans it up after handling the final commit.\n\nI didn’t get any leak errors from the CI. `extra_headers` in `show_log`\nis populated by calling `log_write_email_headers`. Then later it is\nassigned to\n\n ctx.after_subject = extra_headers;\n\nThen `ctx.after_subject is freed later\n\n free((char *)ctx.after_subject);\n\nAm I missing something?\n\n> We should also drop the \"static\" from subject_buffer, if it is no longer\n> needed. Likewise, any strings that start owning memory (here or in patch\n> 2) should probably drop their \"const\". That makes the ownership more\n> clear, and avoids ugly casts when freeing.\n\nOkay, I’ll do that.\n\nThanks\n"},{"id":"490530","messageId":"20240313065454.GB125150@coredump.intra.peff.net","threadId":"61071","inReplyTo":"73a4cb87-2800-4ad1-b7a2-33c6465fcc50@app.fastmail.com","subject":"Re: [PATCH 1/3] log-tree: take ownership of pointer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-13T06:54:54Z","receivedAt":"2024-03-13T06:54:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 12, 2024 at 06:43:55PM +0100, Kristoffer Haugsbakk wrote:\n\n> > Hmm, OK. This patch by itself introduces a memory leak. It would be nice\n> > if we could couple it with the matching free() so that we can see that\n> > the issue is fixed. It sounds like your patch 2 is going to introduce\n> > such a free, but I'm not sure it's complete.\n> \n> Is it okay if it is done in patch 2?\n\nI don't think it's the end of the world to do it in patch 2, as long as\nwe end up in a good spot. But IMHO it's really hard for reviewers to\nunderstand what is going on, because it's intermingled with so many\nother changes. It would be much easier to read if we had a preparatory\npatch that switched the memory ownership of the field, and then built on\ntop of that.\n\nBut I recognize that sometimes that's hard to do, because the state is\nso tangled that the functional change is what untangles it. I'm not sure\nif that's the case here or not; you'd probably have a better idea as\nsomebody who looked carefully at it recently.\n\n> > It frees the old extra_headers before reassigning it, but nobody\n> > cleans it up after handling the final commit.\n> \n> I didn’t get any leak errors from the CI. `extra_headers` in `show_log`\n> is populated by calling `log_write_email_headers`. Then later it is\n> assigned to\n> \n> ctx.after_subject = extra_headers;\n> \n> Then `ctx.after_subject is freed later\n> \n> free((char *)ctx.after_subject);\n> \n> Am I missing something?\n\nAh, I see. I was confused by looking for a free of an extra_headers\nfield. We have rev_info.extra_headers, and that is _not_ owned by\nrev_info. We used to assign that to a variable in\nlog_write_email_headers(), but now we actually make a copy of it. And so\nthe copy is freed in that function when we replace it with a version\ncontaining extra mime headers here:\n\n strbuf_addf(&subject_buffer,\n \"%s\"\n \"MIME-Version: 1.0\\n\"\n \"Content-Type: multipart/mixed;\"\n \" boundary=\\\"%s%s\\\"\\n\"\n \"\\n\"\n \"This is a multi-part message in MIME \"\n \"format.\\n\"\n \"--%s%s\\n\"\n \"Content-Type: text/plain; \"\n \"charset=UTF-8; format=fixed\\n\"\n \"Content-Transfer-Encoding: 8bit\\n\\n\",\n extra_headers ? extra_headers : \"\",\n mime_boundary_leader, opt->mime_boundary,\n mime_boundary_leader, opt->mime_boundary);\n free((char *)extra_headers);\n extra_headers = strbuf_detach(&subject_buffer, NULL);\n\nBut the actual ownership is passed out via the extra_headers_p variable,\nand that is what is assigned to ctx.after_subject (which now takes\nownership).\n\nI think in the snippet I quoted above that extra_headers could never be\nNULL now, right? We'll always return at least an empty string. But\nmoreover, we are formatting it into a strbuf, only to potentially copy\nit it another strbuf. Couldn't we just do it all in one strbuf?\n\nSomething like this:\n\n log-tree.c | 29 ++++++++---------------------\n 1 file changed, 8 insertions(+), 21 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 9196b4f1d4..0a703a0303 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -469,29 +469,22 @@ void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)\n \t}\n }\n \n-static char *extra_and_pe_headers(const char *extra_headers, const char *pe_headers) {\n-\tstruct strbuf all_headers = STRBUF_INIT;\n-\n-\tif (extra_headers)\n-\t\tstrbuf_addstr(&all_headers, extra_headers);\n-\tif (pe_headers) {\n-\t\tstrbuf_addstr(&all_headers, pe_headers);\n-\t}\n-\treturn strbuf_detach(&all_headers, NULL);\n-}\n-\n void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t const char **extra_headers_p,\n \t\t\t int *need_8bit_cte_p,\n \t\t\t int maybe_multipart)\n {\n-\tconst char *extra_headers =\n-\t\textra_and_pe_headers(opt->extra_headers, opt->pe_headers);\n+\tstruct strbuf headers = STRBUF_INIT;\n \tconst char *name = oid_to_hex(opt->zero_commit ?\n \t\t\t\t null_oid() : &commit->object.oid);\n \n \t*need_8bit_cte_p = 0; /* unknown */\n \n+\tif (opt->extra_headers)\n+\t\tstrbuf_addstr(&headers, opt->extra_headers);\n+\tif (opt->pe_headers)\n+\t\tstrbuf_addstr(&headers, opt->pe_headers);\n+\n \tfprintf(opt->diffopt.file, \"From %s Mon Sep 17 00:00:00 2001\\n\", name);\n \tgraph_show_oneline(opt->graph);\n \tif (opt->message_id) {\n@@ -508,16 +501,13 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\tgraph_show_oneline(opt->graph);\n \t}\n \tif (opt->mime_boundary && maybe_multipart) {\n-\t\tstatic struct strbuf subject_buffer = STRBUF_INIT;\n \t\tstatic struct strbuf buffer = STRBUF_INIT;\n \t\tstruct strbuf filename = STRBUF_INIT;\n \t\t*need_8bit_cte_p = -1; /* NEVER */\n \n-\t\tstrbuf_reset(&subject_buffer);\n \t\tstrbuf_reset(&buffer);\n \n-\t\tstrbuf_addf(&subject_buffer,\n-\t\t\t \"%s\"\n+\t\tstrbuf_addf(&headers,\n \t\t\t \"MIME-Version: 1.0\\n\"\n \t\t\t \"Content-Type: multipart/mixed;\"\n \t\t\t \" boundary=\\\"%s%s\\\"\\n\"\n@@ -528,11 +518,8 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t \"Content-Type: text/plain; \"\n \t\t\t \"charset=UTF-8; format=fixed\\n\"\n \t\t\t \"Content-Transfer-Encoding: 8bit\\n\\n\",\n-\t\t\t extra_headers ? extra_headers : \"\",\n \t\t\t mime_boundary_leader, opt->mime_boundary,\n \t\t\t mime_boundary_leader, opt->mime_boundary);\n-\t\tfree((char *)extra_headers);\n-\t\textra_headers = strbuf_detach(&subject_buffer, NULL);\n \n \t\tif (opt->numbered_files)\n \t\t\tstrbuf_addf(&filename, \"%d\", opt->nr);\n@@ -552,7 +539,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\topt->diffopt.stat_sep = buffer.buf;\n \t\tstrbuf_release(&filename);\n \t}\n-\t*extra_headers_p = extra_headers;\n+\t*extra_headers_p = headers.len ? strbuf_detach(&headers, NULL) : NULL;\n }\n \n static void show_sig_lines(struct rev_info *opt, int status, const char *bol)\n\n\nThe resulting code is shorter and (IMHO) easier to understand. It\navoids an extra allocation and copy when using mime. It also avoids the\nallocation of an empty string when opt->extra_headers and\nopt->pe_headers are both NULL. It does make an extra copy when\nextra_headers is non-NULL but pe_headers is NULL (and you're not using\nMIME), as we could just use opt->extra_headers as-is, then. But since\nthe caller needs to take ownership, we can't avoid that copy.\n\nI think you could even do this cleanup before adding pe_headers,\nespecially if it was coupled with cleaning up the memory ownership\nissues.\n\n-Peff\n"},{"id":"490562","messageId":"929692bc-c5f5-4dca-a96c-5b95603c3d26@app.fastmail.com","threadId":"61071","inReplyTo":"20240313065454.GB125150@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] log-tree: take ownership of pointer","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-13T17:49:52Z","receivedAt":"2024-03-13T17:51:13Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Wed, Mar 13, 2024, at 07:54, Jeff King wrote:\n> On Tue, Mar 12, 2024 at 06:43:55PM +0100, Kristoffer Haugsbakk wrote:\n>\n>> > Hmm, OK. This patch by itself introduces a memory leak. It would be nice\n>> > if we could couple it with the matching free() so that we can see that\n>> > the issue is fixed. It sounds like your patch 2 is going to introduce\n>> > such a free, but I'm not sure it's complete.\n>>\n>> Is it okay if it is done in patch 2?\n>\n> I don't think it's the end of the world to do it in patch 2, as long as\n> we end up in a good spot. But IMHO it's really hard for reviewers to\n> understand what is going on, because it's intermingled with so many\n> other changes. It would be much easier to read if we had a preparatory\n> patch that switched the memory ownership of the field, and then built on\n> top of that.\n\nSounds good. I’ll do that.\n\n> But I recognize that sometimes that's hard to do, because the state is\n> so tangled that the functional change is what untangles it. I'm not sure\n> if that's the case here or not; you'd probably have a better idea as\n> somebody who looked carefully at it recently.\n\nSeems doable in this case.\n\nBy the way. I pretty much just elbowed in the changes I needed (like in\n`revision.h`) in order to add this per-patch/cover letter headers\nvariable. Let me know if there are better ways to do it.\n\n>> > It frees the old extra_headers before reassigning it, but nobody\n>> > cleans it up after handling the final commit.\n>>\n>> I didn’t get any leak errors from the CI. `extra_headers` in `show_log`\n>> is populated by calling `log_write_email_headers`. Then later it is\n>> assigned to\n>>\n>> ctx.after_subject = extra_headers;\n>>\n>> Then `ctx.after_subject is freed later\n>>\n>> free((char *)ctx.after_subject);\n>>\n>> Am I missing something?\n>\n> Ah, I see. I was confused by looking for a free of an extra_headers\n> field. We have rev_info.extra_headers, and that is _not_ owned by\n> rev_info. We used to assign that to a variable in\n> log_write_email_headers(), but now we actually make a copy of it. And so\n> the copy is freed in that function when we replace it with a version\n> containing extra mime headers here:\n>\n> [snip]\n>\n> But the actual ownership is passed out via the extra_headers_p variable,\n> and that is what is assigned to ctx.after_subject (which now takes\n> ownership).\n>\n> I think in the snippet I quoted above that extra_headers could never be\n> NULL now, right? We'll always return at least an empty string. But\n> moreover, we are formatting it into a strbuf, only to potentially copy\n> it it another strbuf. Couldn't we just do it all in one strbuf?\n>\n> Something like this:\n>\n> [snip]\n>\n>\n> The resulting code is shorter and (IMHO) easier to understand. It\n> avoids an extra allocation and copy when using mime. It also avoids the\n> allocation of an empty string when opt->extra_headers and\n> opt->pe_headers are both NULL. It does make an extra copy when\n> extra_headers is non-NULL but pe_headers is NULL (and you're not using\n> MIME), as we could just use opt->extra_headers as-is, then. But since\n> the caller needs to take ownership, we can't avoid that copy.\n>\n> I think you could even do this cleanup before adding pe_headers,\n> especially if it was coupled with cleaning up the memory ownership\n> issues.\n>\n> -Peff\n\nI haven’t tried yet but this seems like a good plan. It was getting a\ngetting a bit too back and forth with my changes. So I’ll try to use\nyour patch and see if I can get a clean preparatory patch/commit before\nthe main change.\n\nCheers\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"490941","messageId":"cover.1710873210.git.code@khaugsbakk.name","threadId":"61071","inReplyTo":"cover.1709841147.git.code@khaugsbakk.name","subject":"[PATCH v2 0/3] format-patch: teach `--header-cmd`","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-19T18:35:35Z","receivedAt":"2024-03-19T18:37:01Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"(most of this is from the main commit/patch with some elaboration in\nparts)\n\nTeach git-format-patch(1) `--header-cmd` (with negation) and the\naccompanying config variable `format.headerCmd` which allows the user to\nadd extra headers per-patch.\n\n§ Motivation\n\nformat-patch knows `--add-header`. However, that seems most useful for\nseries-wide headers; you cannot really control what the header is like\nper patch or specifically for the cover letter. To that end, teach\nformat-patch a new option which runs a command that has access to the\nhash of the current commit (if it is a code patch) and the patch count\nwhich is used for the patch files that this command outputs. Also\ninclude an environment variable which tells the version of this API so\nthat the command can detect and error out in case the API changes.\n\nThis is inspired by `--header-cmd` of git-send-email(1).[1]\n\n🔗 1: https://lore.kernel.org/git/20230423122744.4865-1-maxim.cournoyer@gmail.com/\n\n§ Discussion\n\nOne can already get the commit hash from the first line of the patch file:\n\n From 04967d53399ed2db09d224008f557ec1a5847cc1 Mon Sep 17 00:00:00 2001\n\nHowever, the point of this option is to be able to calculate header\noutput based primarily on the commit hash without having to go back and\nforth between commands (i.e. running format-patch and then parsing the\npatch files).\n\nAs an example, the command could output a header for the current commit\nas well as another header for the previously-published commits:\n\n X-Commit-Hash: 97b53c04894578b23d0c650f69885f734699afc7\n X-Previous-Commits:\n 4ad5d4190649dcb5f26c73a6f15ab731891b9dfd\n d275d1d179b90592ddd7b5da2ae4573b3f7a37b7\n 402b7937951073466bf4527caffd38175391c7da\n\nOne can imagine that (1) these previous commit hashes were stored on every\ncommit rewrite and (2) the commits that had been published previously\nwere also stored. Then the command just needed the current commit hash\nin order to look up this information.\n\nNow interested parties can use this information to track where the\npatches come from.\n\nThis information could of course be given between the three-dash/three-\nhyphen line and the patch proper. However, the hypothetical project in\nquestion might prefer to use this part for extra patch information\nwritten by the author and leave the above information for tooling; this\nway the extra information does not need to disturb the reader.\n\n§ Demonstration\n\nThe above current/previous hash example is taken from:\n\nhttps://lore.kernel.org/git/97b53c04894578b23d0c650f69885f734699afc7.1709670287.git.code@khaugsbakk.name/\n\n§ Changes in v2\n\nChanges based on feedback from Jean-Noël and Peff.\n\nPeff suggested changes to the strbuf handling in `log_tree`, removing\nconstness in order to facilitate freeing without casting, and fleshing\nout the preliminary patch.\n\nIn detail:\n\n• revision: add a per-email field to rev-info\n • Replaces (subject) “log-tree: take ownership of pointer”\n • Separate out changes for `rev.info.pe_headers`, changes to\n ownership, const, and strbuf handling in `log_tree`\n • Link: https://lore.kernel.org/git/20240313065454.GB125150@coredump.intra.peff.net/\n• format-patch: teach `--header-cmd`\n • Simplify tests: use `true` and `false` as commands\n • Fix indentation in code\n • Use AsciiDoc definition list\n • Link: https://lore.kernel.org/git/53ea3745-205b-40c0-a4c5-9be26d9b88bf@gmail.com/\n• format-patch: check if header output looks valid\n • (no changes)\n\n§ CC\n\nCc: Jeff King \nCc: Maxim Cournoyer \nCc: \"Jean-Noël Avila\" \n\n§ CI\n\nhttps://github.com/LemmingAvalanche/git/actions/runs/8332742200/job/22802597793\n\nFails but looks similar to the failures for `master` at 3bd955d2691 (The\nninth batch, 2024-03-18):\n\nhttps://github.com/git/git/actions/runs/8333455189/job/22804953254\n\nKristoffer Haugsbakk (3):\n revision: add a per-email field to rev-info\n format-patch: teach `--header-cmd`\n format-patch: check if header output looks valid\n\n Documentation/config/format.txt | 5 ++\n Documentation/git-format-patch.txt | 23 +++++++++\n builtin/log.c | 76 ++++++++++++++++++++++++++++++\n log-tree.c | 21 +++++----\n log-tree.h | 2 +-\n pretty.h | 2 +-\n revision.h | 4 +-\n t/t4014-format-patch.sh | 49 +++++++++++++++++++\n 8 files changed, 169 insertions(+), 13 deletions(-)\n\nRange-diff against v1:\n1: 3b12a8cf393 < -: ----------- log-tree: take ownership of pointer\n-: ----------- > 1: 9a7102b708e revision: add a per-email field to rev-info\n2: f405a0140b5 ! 2: 8c511797a47 format-patch: teach `--header-cmd`\n @@ Commit message\n \n \n ## Notes (series) ##\n - Documentation/config/format.txt:\n - • I get the impression that `_` is the convention for placeholders now:\n - `__`\n + v2:\n + • Simplify tests: use `true` and `false` as commands\n + • Fix indentation\n + • Don’t use `const` for owned pointer (avoid cast when freeing)\n + • Link: https://lore.kernel.org/git/cover.1709841147.git.code@khaugsbakk.name/T/#m12d104a5a769c7f6e02b1d0a75855142004e9206\n + • Use AsciiDoc definition list\n + • Link: https://lore.kernel.org/git/53ea3745-205b-40c0-a4c5-9be26d9b88bf@gmail.com/\n \n ## Documentation/config/format.txt ##\n @@ Documentation/config/format.txt: format.headers::\n @@ Documentation/git-format-patch.txt: feeding the result to `git send-email`.\n ++\n +__ has access to these environment variables:\n ++\n -+\tGIT_FP_HEADER_CMD_VERSION\n -++\n -+The version of this API. Currently `1`. __ may return exit code\n -+`2` in order to signal that it does not support the given version.\n -++\n -+\tGIT_FP_HEADER_CMD_HASH\n -++\n -+The hash of the commit corresponding to the current patch. Not set if\n -+the current patch is the cover letter.\n -++\n -+\tGIT_FP_HEADER_CMD_COUNT\n -++\n -+The current patch count. Increments for each patch.\n ++--\n ++GIT_FP_HEADER_CMD_VERSION;;\n ++\tThe version of this API. Currently `1`. __ may return exit code\n ++\t`2` in order to signal that it does not support the given version.\n ++GIT_FP_HEADER_CMD_HASH;;\n ++\tThe hash of the commit corresponding to the current patch. Not set if\n ++\tthe current patch is the cover letter.\n ++GIT_FP_HEADER_CMD_COUNT;;\n ++\tThe current patch count. Increments for each patch.\n ++--\n ++\n +`git format-patch` will error out if __ returns a non-zero exit\n +code.\n @@ builtin/log.c: static void make_cover_letter(struct rev_info *rev, int use_separ\n \t\tshow_range_diff(rev->rdiff1, rev->rdiff2, &range_diff_opts);\n \t\tstrvec_clear(&other_arg);\n \t}\n -+\tfree((char *)pp.after_subject);\n ++\tfree(pp.after_subject);\n }\n \n static char *clean_message_id(const char *msg_id)\n @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre\n \t\t/* interdiff/range-diff in cover-letter; omit from patches */\n \t\trev.idiff_oid1 = NULL;\n \t\trev.rdiff1 = NULL;\n -+\t\tfree((char *)rev.pe_headers);\n ++\t\tfree(rev.pe_headers);\n \t}\n \trev.add_signoff = do_signoff;\n \n @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre\n \t\t}\n \t\tif (output_directory)\n \t\t\tfclose(rev.diffopt.file);\n -+\t\tfree((char *)rev.pe_headers);\n ++\t\tfree(rev.pe_headers);\n \t}\n \tstop_progress(&progress);\n \tfree(list);\n \n - ## log-tree.c ##\n -@@ log-tree.c: void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)\n - \t}\n - }\n - \n -+static char *extra_and_pe_headers(const char *extra_headers, const char *pe_headers) {\n -+\tstruct strbuf all_headers = STRBUF_INIT;\n -+\n -+\tif (extra_headers)\n -+\t\tstrbuf_addstr(&all_headers, extra_headers);\n -+\tif (pe_headers) {\n -+\t\tstrbuf_addstr(&all_headers, pe_headers);\n -+\t}\n -+\treturn strbuf_detach(&all_headers, NULL);\n -+}\n -+\n - void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n - \t\t\t const char **extra_headers_p,\n - \t\t\t int *need_8bit_cte_p,\n - \t\t\t int maybe_multipart)\n - {\n --\tconst char *extra_headers = opt->extra_headers;\n -+\tconst char *extra_headers =\n -+\t\textra_and_pe_headers(opt->extra_headers, opt->pe_headers);\n - \tconst char *name = oid_to_hex(opt->zero_commit ?\n - \t\t\t\t null_oid() : &commit->object.oid);\n - \n -@@ log-tree.c: void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n - \t\t\t extra_headers ? extra_headers : \"\",\n - \t\t\t mime_boundary_leader, opt->mime_boundary,\n - \t\t\t mime_boundary_leader, opt->mime_boundary);\n -+\t\tfree((char *)extra_headers);\n - \t\textra_headers = strbuf_detach(&subject_buffer, NULL);\n - \n - \t\tif (opt->numbered_files)\n -@@ log-tree.c: void show_log(struct rev_info *opt)\n - \n - \tstrbuf_release(&msgbuf);\n - \tfree(ctx.notes_message);\n -+\tfree((char *)ctx.after_subject);\n - \n - \tif (cmit_fmt_is_mail(ctx.fmt) && opt->idiff_oid1) {\n - \t\tstruct diff_queue_struct dq;\n -\n - ## revision.h ##\n -@@ revision.h: struct rev_info {\n - \tstruct string_list *ref_message_ids;\n - \tint\t\tadd_signoff;\n - \tconst char\t*extra_headers;\n -+\t/* per-email headers */\n -+\tconst char\t*pe_headers;\n - \tconst char\t*log_reencode;\n - \tconst char\t*subject_prefix;\n - \tint\t\tpatch_name_max;\n -\n ## t/t4014-format-patch.sh ##\n @@ t/t4014-format-patch.sh: test_expect_failure 'configuration To: header (rfc2047)' '\n \tgrep \"^To: =?UTF-8?q?R=20=C3=84=20Cipient?= \\$\" hdrs9\n @@ t/t4014-format-patch.sh: test_expect_failure 'configuration To: header (rfc2047)\n +'\n +\n +test_expect_success '--header-cmd with no output works' '\n -+\twrite_script cmd <<-\\EOF &&\n -+\texit 0\n -+\tEOF\n -+\tgit format-patch --header-cmd=./cmd --stdout main..side\n ++\tgit format-patch --header-cmd=true --stdout main..side\n +'\n +\n +test_expect_success '--header-cmd reports failed command' '\n -+\twrite_script cmd <<-\\EOF &&\n -+\texit 1\n -+\tEOF\n -+\t\tcat > expect <<-\\EOF &&\n -+\tfatal: header-cmd ./cmd: failed with exit code 1\n ++\tcat > expect <<-\\EOF &&\n ++\tfatal: header-cmd false: failed with exit code 1\n +\tEOF\n -+\ttest_must_fail git format-patch --header-cmd=./cmd --stdout main..side >actual 2>&1 &&\n ++\ttest_must_fail git format-patch --header-cmd=false --stdout main..side >actual 2>&1 &&\n +\ttest_cmp expect actual\n +'\n +\n3: 0e8409227e4 ! 3: c570467c8db format-patch: check if header output looks valid\n @@ builtin/log.c: static char *header_cmd_output(struct rev_info *rev, const struct\n \n ## t/t4014-format-patch.sh ##\n @@ t/t4014-format-patch.sh: test_expect_success '--header-cmd with no output works' '\n - \tgit format-patch --header-cmd=./cmd --stdout main..side\n + \tgit format-patch --header-cmd=true --stdout main..side\n '\n \n +test_expect_success '--header-cmd without headers-like output fails' '\n @@ t/t4014-format-patch.sh: test_expect_success '--header-cmd with no output works'\n +'\n +\n test_expect_success '--header-cmd reports failed command' '\n - \twrite_script cmd <<-\\EOF &&\n - \texit 1\n + \tcat > expect <<-\\EOF &&\n + \tfatal: header-cmd false: failed with exit code 1\n-- \n2.44.0.144.g29ae9861142\n\n"},{"id":"490942","messageId":"9a7102b708e4afe78447e48e4baf5b6d66ca50d1.1710873210.git.code@khaugsbakk.name","threadId":"61071","inReplyTo":"cover.1710873210.git.code@khaugsbakk.name","subject":"[PATCH v2 1/3] revision: add a per-email field to rev-info","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-19T18:35:36Z","receivedAt":"2024-03-19T18:37:03Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Add `pe_header` to `rev_info` to store per-email headers.\n\nThe next commit will add an option to `format-patch` which will allow\nthe user to store headers per-email; a complement to options like\n`--add-header`.\n\nTo make this possible we need a new field to store these headers. We\nalso need to take ownership of `extra_headers_p` in\n`log_write_email_headers`; facilitate this by removing constness from\nthe relevant pointers.\n\nHelped-by: Jeff King \nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n v2:\n • Replaces “log-tree: take ownership of pointer”\n • Link: https://lore.kernel.org/git/3b12a8cf393b6d8f0877fd7d87173c565d7d5a90.1709841147.git.code@khaugsbakk.name/\n • More preliminary work\n • Link: https://lore.kernel.org/git/20240313065454.GB125150@coredump.intra.peff.net/\n\n log-tree.c | 21 +++++++++++----------\n log-tree.h | 2 +-\n pretty.h | 2 +-\n revision.h | 4 +++-\n 4 files changed, 16 insertions(+), 13 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex e5438b029d9..f6cdde6e8f3 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -470,16 +470,21 @@ void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)\n }\n \n void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n-\t\t\t const char **extra_headers_p,\n+\t\t\t char **extra_headers_p,\n \t\t\t int *need_8bit_cte_p,\n \t\t\t int maybe_multipart)\n {\n-\tconst char *extra_headers = opt->extra_headers;\n+\tstruct strbuf headers = STRBUF_INIT;\n \tconst char *name = oid_to_hex(opt->zero_commit ?\n \t\t\t\t null_oid() : &commit->object.oid);\n \n \t*need_8bit_cte_p = 0; /* unknown */\n \n+\tif (opt->extra_headers)\n+\t\tstrbuf_addstr(&headers, opt->extra_headers);\n+\tif (opt->pe_headers)\n+\t\tstrbuf_addstr(&headers, opt->pe_headers);\n+\n \tfprintf(opt->diffopt.file, \"From %s Mon Sep 17 00:00:00 2001\\n\", name);\n \tgraph_show_oneline(opt->graph);\n \tif (opt->message_id) {\n@@ -496,16 +501,13 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\tgraph_show_oneline(opt->graph);\n \t}\n \tif (opt->mime_boundary && maybe_multipart) {\n-\t\tstatic struct strbuf subject_buffer = STRBUF_INIT;\n \t\tstatic struct strbuf buffer = STRBUF_INIT;\n \t\tstruct strbuf filename = STRBUF_INIT;\n \t\t*need_8bit_cte_p = -1; /* NEVER */\n \n-\t\tstrbuf_reset(&subject_buffer);\n \t\tstrbuf_reset(&buffer);\n \n-\t\tstrbuf_addf(&subject_buffer,\n-\t\t\t \"%s\"\n+\t\tstrbuf_addf(&headers,\n \t\t\t \"MIME-Version: 1.0\\n\"\n \t\t\t \"Content-Type: multipart/mixed;\"\n \t\t\t \" boundary=\\\"%s%s\\\"\\n\"\n@@ -516,10 +518,8 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t \"Content-Type: text/plain; \"\n \t\t\t \"charset=UTF-8; format=fixed\\n\"\n \t\t\t \"Content-Transfer-Encoding: 8bit\\n\\n\",\n-\t\t\t extra_headers ? extra_headers : \"\",\n \t\t\t mime_boundary_leader, opt->mime_boundary,\n \t\t\t mime_boundary_leader, opt->mime_boundary);\n-\t\textra_headers = subject_buffer.buf;\n \n \t\tif (opt->numbered_files)\n \t\t\tstrbuf_addf(&filename, \"%d\", opt->nr);\n@@ -539,7 +539,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\topt->diffopt.stat_sep = buffer.buf;\n \t\tstrbuf_release(&filename);\n \t}\n-\t*extra_headers_p = extra_headers;\n+\t*extra_headers_p = headers.len ? strbuf_detach(&headers, NULL) : NULL;\n }\n \n static void show_sig_lines(struct rev_info *opt, int status, const char *bol)\n@@ -678,7 +678,7 @@ void show_log(struct rev_info *opt)\n \tstruct log_info *log = opt->loginfo;\n \tstruct commit *commit = log->commit, *parent = log->parent;\n \tint abbrev_commit = opt->abbrev_commit ? opt->abbrev : the_hash_algo->hexsz;\n-\tconst char *extra_headers = opt->extra_headers;\n+\tchar *extra_headers = opt->extra_headers;\n \tstruct pretty_print_context ctx = {0};\n \n \topt->loginfo = NULL;\n@@ -857,6 +857,7 @@ void show_log(struct rev_info *opt)\n \n \tstrbuf_release(&msgbuf);\n \tfree(ctx.notes_message);\n+\tfree(ctx.after_subject);\n \n \tif (cmit_fmt_is_mail(ctx.fmt) && opt->idiff_oid1) {\n \t\tstruct diff_queue_struct dq;\ndiff --git a/log-tree.h b/log-tree.h\nindex 41c776fea52..94978e2c838 100644\n--- a/log-tree.h\n+++ b/log-tree.h\n@@ -29,7 +29,7 @@ void format_decorations(struct strbuf *sb, const struct commit *commit,\n \t\t\tint use_color, const struct decoration_options *opts);\n void show_decorations(struct rev_info *opt, struct commit *commit);\n void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n-\t\t\t const char **extra_headers_p,\n+\t\t\t char **extra_headers_p,\n \t\t\t int *need_8bit_cte_p,\n \t\t\t int maybe_multipart);\n void load_ref_decorations(struct decoration_filter *filter, int flags);\ndiff --git a/pretty.h b/pretty.h\nindex 421209e9ec2..bdce3191875 100644\n--- a/pretty.h\n+++ b/pretty.h\n@@ -35,7 +35,7 @@ struct pretty_print_context {\n \t */\n \tenum cmit_fmt fmt;\n \tint abbrev;\n-\tconst char *after_subject;\n+\tchar *after_subject;\n \tint preserve_subject;\n \tstruct date_mode date_mode;\n \tunsigned date_mode_explicit:1;\ndiff --git a/revision.h b/revision.h\nindex 94c43138bc3..95e92397a7a 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -290,7 +290,9 @@ struct rev_info {\n \tstruct ident_split from_ident;\n \tstruct string_list *ref_message_ids;\n \tint\t\tadd_signoff;\n-\tconst char\t*extra_headers;\n+\tchar\t\t*extra_headers;\n+\t/* per-email headers */\n+\tchar\t\t*pe_headers;\n \tconst char\t*log_reencode;\n \tconst char\t*subject_prefix;\n \tint\t\tpatch_name_max;\n-- \n2.44.0.144.g29ae9861142\n\n"},{"id":"490943","messageId":"8c511797a476a86bf231595d6d7f5db790f12731.1710873210.git.code@khaugsbakk.name","threadId":"61071","inReplyTo":"cover.1710873210.git.code@khaugsbakk.name","subject":"[PATCH v2 2/3] format-patch: teach `--header-cmd`","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-19T18:35:37Z","receivedAt":"2024-03-19T18:37:05Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Teach git-format-patch(1) `--header-cmd` (with negation) and the\naccompanying config variable `format.headerCmd` which allows the user to\nadd extra headers per-patch.\n\nformat-patch knows `--add-header`. However, that seems most useful for\nseries-wide headers; you cannot really control what the header is like\nper patch or specifically for the cover letter. To that end, teach\nformat-patch a new option which runs a command that has access to the\nhash of the current commit (if it is a code patch) and the patch count\nwhich is used for the patch files that this command outputs. Also\ninclude an environment variable which tells the version of this API so\nthat the command can detect and error out in case the API changes.\n\nThis is inspired by `--header-cmd` of git-send-email(1).\n\n§ Discussion\n\nThe command can use the provided commit hash to provide relevant\ninformation in the header. For example, the command could output a\nheader for the current commit as well as the previously-published\ncommits:\n\n X-Commit-Hash: 97b53c04894578b23d0c650f69885f734699afc7\n X-Previous-Commits:\n 4ad5d4190649dcb5f26c73a6f15ab731891b9dfd\n d275d1d179b90592ddd7b5da2ae4573b3f7a37b7\n 402b7937951073466bf4527caffd38175391c7da\n\nNow interested parties can use this information to track where the\npatches come from.\n\nThis information could of course be given between the\nthree-dash/three-hyphen line and the patch proper. However, the project\nmight prefer to use this part for extra patch information written by the\nauthor and leave the above information for tooling; this way the extra\ninformation does not need to disturb the reader.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n v2:\n • Simplify tests: use `true` and `false` as commands\n • Fix indentation\n • Don’t use `const` for owned pointer (avoid cast when freeing)\n • Link: https://lore.kernel.org/git/cover.1709841147.git.code@khaugsbakk.name/T/#m12d104a5a769c7f6e02b1d0a75855142004e9206\n • Use AsciiDoc definition list\n • Link: https://lore.kernel.org/git/53ea3745-205b-40c0-a4c5-9be26d9b88bf@gmail.com/\n\n Documentation/config/format.txt | 5 ++++\n Documentation/git-format-patch.txt | 23 ++++++++++++++++\n builtin/log.c | 43 ++++++++++++++++++++++++++++++\n t/t4014-format-patch.sh | 36 +++++++++++++++++++++++++\n 4 files changed, 107 insertions(+)\n\ndiff --git a/Documentation/config/format.txt b/Documentation/config/format.txt\nindex 7410e930e53..c184b865824 100644\n--- a/Documentation/config/format.txt\n+++ b/Documentation/config/format.txt\n@@ -31,6 +31,11 @@ format.headers::\n \tAdditional email headers to include in a patch to be submitted\n \tby mail. See linkgit:git-format-patch[1].\n \n+format.headerCmd::\n+\tCommand to run for each patch that should output RFC 2822 email\n+\theaders. Has access to some information per patch via\n+\tenvironment variables. See linkgit:git-format-patch[1].\n+\n format.to::\n format.cc::\n \tAdditional recipients to include in a patch to be submitted\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 728bb3821c1..4f87fd25db9 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -303,6 +303,29 @@ feeding the result to `git send-email`.\n \t`Cc:`, and custom) headers added so far from config or command\n \tline.\n \n+--[no-]header-cmd=::\n+\tRun __ for each patch. __ should output valid RFC 2822\n+\temail headers. This can also be configured with\n+\tthe configuration variable `format.headerCmd`. Can be turned off\n+\twith `--no-header-cmd`. This works independently of\n+\t`--[no-]add-header`.\n++\n+__ has access to these environment variables:\n++\n+--\n+GIT_FP_HEADER_CMD_VERSION;;\n+\tThe version of this API. Currently `1`. __ may return exit code\n+\t`2` in order to signal that it does not support the given version.\n+GIT_FP_HEADER_CMD_HASH;;\n+\tThe hash of the commit corresponding to the current patch. Not set if\n+\tthe current patch is the cover letter.\n+GIT_FP_HEADER_CMD_COUNT;;\n+\tThe current patch count. Increments for each patch.\n+--\n++\n+`git format-patch` will error out if __ returns a non-zero exit\n+code.\n+\n --[no-]cover-letter::\n \tIn addition to the patches, generate a cover letter file\n \tcontaining the branch description, shortlog and the overall diffstat. You can\ndiff --git a/builtin/log.c b/builtin/log.c\nindex e5da0d10434..bc656b5e0f8 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -43,10 +43,13 @@\n #include \"tmp-objdir.h\"\n #include \"tree.h\"\n #include \"write-or-die.h\"\n+#include \"run-command.h\"\n \n #define MAIL_DEFAULT_WRAP 72\n #define COVER_FROM_AUTO_MAX_SUBJECT_LEN 100\n #define FORMAT_PATCH_NAME_MAX_DEFAULT 64\n+#define HC_VERSION \"1\"\n+#define HC_NOT_SUPPORTED 2\n \n /* Set a default date-time format for git log (\"log.date\" config variable) */\n static const char *default_date_mode = NULL;\n@@ -902,6 +905,7 @@ static int auto_number = 1;\n \n static char *default_attach = NULL;\n \n+static const char *header_cmd = NULL;\n static struct string_list extra_hdr = STRING_LIST_INIT_NODUP;\n static struct string_list extra_to = STRING_LIST_INIT_NODUP;\n static struct string_list extra_cc = STRING_LIST_INIT_NODUP;\n@@ -1100,6 +1104,8 @@ static int git_format_config(const char *var, const char *value,\n \t\tformat_no_prefix = 1;\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"format.headercmd\"))\n+\t\treturn git_config_string(&header_cmd, var, value);\n \n \t/*\n \t * ignore some porcelain config which would otherwise be parsed by\n@@ -1419,6 +1425,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \t\tshow_range_diff(rev->rdiff1, rev->rdiff2, &range_diff_opts);\n \t\tstrvec_clear(&other_arg);\n \t}\n+\tfree(pp.after_subject);\n }\n \n static char *clean_message_id(const char *msg_id)\n@@ -1869,6 +1876,35 @@ static void infer_range_diff_ranges(struct strbuf *r1,\n \t}\n }\n \n+/* Returns an owned pointer */\n+static char *header_cmd_output(struct rev_info *rev, const struct commit *cmit)\n+{\n+\tstruct child_process header_cmd_proc = CHILD_PROCESS_INIT;\n+\tstruct strbuf output = STRBUF_INIT;\n+\tint res;\n+\n+\tstrvec_pushl(&header_cmd_proc.args, header_cmd, NULL);\n+\tif (cmit)\n+\t\tstrvec_pushf(&header_cmd_proc.env, \"GIT_FP_HEADER_CMD_HASH=%s\",\n+\t\t\t oid_to_hex(&cmit->object.oid));\n+\tstrvec_pushl(&header_cmd_proc.env,\n+\t\t \"GIT_FP_HEADER_CMD_VERSION=\" HC_VERSION, NULL);\n+\tstrvec_pushf(&header_cmd_proc.env, \"GIT_FP_HEADER_CMD_COUNT=%\" PRIuMAX,\n+\t\t (uintmax_t)rev->nr);\n+\tres = capture_command(&header_cmd_proc, &output, 0);\n+\tif (res) {\n+\t\tif (res == HC_NOT_SUPPORTED)\n+\t\t\tdie(_(\"header-cmd %s: returned exit \"\n+\t\t\t \"code %d; the command does not support \"\n+\t\t\t \"version \" HC_VERSION),\n+\t\t\t header_cmd, HC_NOT_SUPPORTED);\n+\t\telse\n+\t\t\tdie(_(\"header-cmd %s: failed with exit code %d\"),\n+\t\t\t header_cmd, res);\n+\t}\n+\treturn strbuf_detach(&output, NULL);\n+}\n+\n int cmd_format_patch(int argc, const char **argv, const char *prefix)\n {\n \tstruct commit *commit;\n@@ -1959,6 +1995,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tOPT_GROUP(N_(\"Messaging\")),\n \t\tOPT_CALLBACK(0, \"add-header\", NULL, N_(\"header\"),\n \t\t\t N_(\"add email header\"), header_callback),\n+\t\tOPT_STRING(0, \"header-cmd\", &header_cmd, N_(\"email\"), N_(\"command that will be run to generate headers\")),\n \t\tOPT_STRING_LIST(0, \"to\", &extra_to, N_(\"email\"), N_(\"add To: header\")),\n \t\tOPT_STRING_LIST(0, \"cc\", &extra_cc, N_(\"email\"), N_(\"add Cc: header\")),\n \t\tOPT_CALLBACK_F(0, \"from\", &from, N_(\"ident\"),\n@@ -2325,6 +2362,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (cover_letter) {\n \t\tif (thread)\n \t\t\tgen_message_id(&rev, \"cover\");\n+\t\tif (header_cmd)\n+\t\t\trev.pe_headers = header_cmd_output(&rev, NULL);\n \t\tmake_cover_letter(&rev, !!output_directory,\n \t\t\t\t origin, nr, list, description_file, branch_name, quiet);\n \t\tprint_bases(&bases, rev.diffopt.file);\n@@ -2334,6 +2373,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t/* interdiff/range-diff in cover-letter; omit from patches */\n \t\trev.idiff_oid1 = NULL;\n \t\trev.rdiff1 = NULL;\n+\t\tfree(rev.pe_headers);\n \t}\n \trev.add_signoff = do_signoff;\n \n@@ -2380,6 +2420,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\tgen_message_id(&rev, oid_to_hex(&commit->object.oid));\n \t\t}\n \n+\t\tif (header_cmd)\n+\t\t\trev.pe_headers = header_cmd_output(&rev, commit);\n \t\tif (output_directory &&\n \t\t open_next_file(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))\n \t\t\tdie(_(\"failed to create output files\"));\n@@ -2406,6 +2448,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tif (output_directory)\n \t\t\tfclose(rev.diffopt.file);\n+\t\tfree(rev.pe_headers);\n \t}\n \tstop_progress(&progress);\n \tfree(list);\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex e37a1411ee2..dc85c4c28fe 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -238,6 +238,42 @@ test_expect_failure 'configuration To: header (rfc2047)' '\n \tgrep \"^To: =?UTF-8?q?R=20=C3=84=20Cipient?= \\$\" hdrs9\n '\n \n+test_expect_success '--header-cmd' '\n+\twrite_script cmd <<-\\EOF &&\n+\tprintf \"X-S: $GIT_FP_HEADER_CMD_HASH\\n\"\n+\tprintf \"X-V: $GIT_FP_HEADER_CMD_VERSION\\n\"\n+\tprintf \"X-C: $GIT_FP_HEADER_CMD_COUNT\\n\"\n+\tEOF\n+\texpect_sha1=$(git rev-parse side) &&\n+\tgit format-patch --header-cmd=./cmd --stdout main..side >patch &&\n+\tgrep \"^X-S: $expect_sha1\" patch &&\n+\tgrep \"^X-V: 1\" patch &&\n+\tgrep \"^X-C: 3\" patch\n+'\n+\n+test_expect_success '--header-cmd with no output works' '\n+\tgit format-patch --header-cmd=true --stdout main..side\n+'\n+\n+test_expect_success '--header-cmd reports failed command' '\n+\tcat > expect <<-\\EOF &&\n+\tfatal: header-cmd false: failed with exit code 1\n+\tEOF\n+\ttest_must_fail git format-patch --header-cmd=false --stdout main..side >actual 2>&1 &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--header-cmd reports exit code 2' '\n+\twrite_script cmd <<-\\EOF &&\n+\texit 2\n+\tEOF\n+\tcat > expect <<-\\EOF &&\n+\tfatal: header-cmd ./cmd: returned exit code 2; the command does not support version 1\n+\tEOF\n+\ttest_must_fail git format-patch --header-cmd=./cmd --stdout main..side >actual 2>&1 &&\n+\ttest_cmp expect actual\n+'\n+\n # check_patch : Verify that looks like a half-sane\n # patch email to avoid a false positive with !grep\n check_patch () {\n-- \n2.44.0.144.g29ae9861142\n\n"},{"id":"490944","messageId":"c570467c8db35d96e4262857658fcc64328d810c.1710873210.git.code@khaugsbakk.name","threadId":"61071","inReplyTo":"cover.1710873210.git.code@khaugsbakk.name","subject":"[PATCH v2 3/3] format-patch: check if header output looks valid","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-19T18:35:38Z","receivedAt":"2024-03-19T18:37:07Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Implement a function based on `mailinfo.c:is_mail`.\n\nSigned-off-by: Kristoffer Haugsbakk \n---\n\nNotes (series):\n Isolating this for review as its own commit so that I can point out the\n provenance. May well be squashed into the main patch eventually.\n\n builtin/log.c | 33 +++++++++++++++++++++++++++++++++\n t/t4014-format-patch.sh | 13 +++++++++++++\n 2 files changed, 46 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex bc656b5e0f8..2902c2bf6fe 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1876,6 +1876,35 @@ static void infer_range_diff_ranges(struct strbuf *r1,\n \t}\n }\n \n+static int is_mail(struct strbuf *sb)\n+{\n+\tconst char *header_regex = \"^[!-9;-~]+:\";\n+\tregex_t regex;\n+\tint ret = 1, i;\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\n+\tif (regcomp(®ex, header_regex, REG_NOSUB | REG_EXTENDED))\n+\t\tdie(\"invalid pattern: %s\", header_regex);\n+\tstring_list_split(&list, sb->buf, '\\n', -1);\n+\tfor (i = 0; i < list.nr; i++) {\n+\t\t/* End of header */\n+\t\tif (!*list.items[i].string && i == (list.nr - 1))\n+\t\t\tbreak;\n+\t\t/* Ignore indented folded lines */\n+\t\tif (*list.items[i].string == '\\t' ||\n+\t\t *list.items[i].string == ' ')\n+\t\t\tcontinue;\n+\t\t/* It's a header if it matches header_regex */\n+\t\tif (regexec(®ex, list.items[i].string, 0, NULL, 0)) {\n+\t\t\tret = 0;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\tstring_list_clear(&list, 1);\n+\tregfree(®ex);\n+\treturn ret;\n+}\n+\n /* Returns an owned pointer */\n static char *header_cmd_output(struct rev_info *rev, const struct commit *cmit)\n {\n@@ -1902,6 +1931,10 @@ static char *header_cmd_output(struct rev_info *rev, const struct commit *cmit)\n \t\t\tdie(_(\"header-cmd %s: failed with exit code %d\"),\n \t\t\t header_cmd, res);\n \t}\n+\tif (!is_mail(&output))\n+\t\tdie(_(\"header-cmd %s: returned output which was \"\n+\t\t \"not recognized as valid RFC 2822 headers\"),\n+\t\t header_cmd);\n \treturn strbuf_detach(&output, NULL);\n }\n \ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex dc85c4c28fe..533a5b246e5 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -255,6 +255,19 @@ test_expect_success '--header-cmd with no output works' '\n \tgit format-patch --header-cmd=true --stdout main..side\n '\n \n+test_expect_success '--header-cmd without headers-like output fails' '\n+\twrite_script cmd <<-\\EOF &&\n+\tprintf \"X-S: $GIT_FP_HEADER_CMD_HASH\\n\"\n+\tprintf \"\\n\"\n+\tprintf \"X-C: $GIT_FP_HEADER_CMD_COUNT\\n\"\n+\tEOF\n+\tcat > expect <<-\\EOF &&\n+\tfatal: header-cmd ./cmd: returned output which was not recognized as valid RFC 2822 headers\n+\tEOF\n+\ttest_must_fail git format-patch --header-cmd=./cmd --stdout main..side >actual 2>&1 &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--header-cmd reports failed command' '\n \tcat > expect <<-\\EOF &&\n \tfatal: header-cmd false: failed with exit code 1\n-- \n2.44.0.144.g29ae9861142\n\n"},{"id":"490967","messageId":"20240319212940.GE1159535@coredump.intra.peff.net","threadId":"61071","inReplyTo":"9a7102b708e4afe78447e48e4baf5b6d66ca50d1.1710873210.git.code@khaugsbakk.name","subject":"Re: [PATCH v2 1/3] revision: add a per-email field to rev-info","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-19T21:29:40Z","receivedAt":"2024-03-19T21:29:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 19, 2024 at 07:35:36PM +0100, Kristoffer Haugsbakk wrote:\n\n> Add `pe_header` to `rev_info` to store per-email headers.\n\nIt is only just now that I realized that \"pe\" stands for per-email\n(though to be fair I was not really focused on the intent of the series\nwhen reading v1). Can we just call it per_email_headers or something?\n\n> The next commit will add an option to `format-patch` which will allow\n> the user to store headers per-email; a complement to options like\n> `--add-header`.\n> \n> To make this possible we need a new field to store these headers. We\n> also need to take ownership of `extra_headers_p` in\n> `log_write_email_headers`; facilitate this by removing constness from\n> the relevant pointers.\n\nThere are three pointers at play here:\n\n - ctx.after_subject has its const removed, since it will now always be\n allocated by log_write_email_headers(), and then freed by the\n caller. Makes sense Though it looks like we only free in show_log(),\n and the free in make_cover_letter() is not added until patch 2?\n\n - rev_info.extra_headers has its const removed here, but I don't think\n that is helping anything. We only use it to write into the \"headers\"\n strbuf in log_write_email_headers(), which always returns\n headers.buf (or NULL).\n\n - rev.pe_headers is introduced as non-const because it is allocated\n and freed for each email. That makes some sense, though if we\n followed the pattern of rev.extra_headers, then the pointer is\n conceptually \"const\" within the rev_info struct, and it is the\n caller who keeps track of the allocation (using a to_free variable).\n Possibly we should do the same here?\n\nI do still think this could be split in a more obvious way, leaving the\npe_headers bits until they are actually needed. Let me see if I can\nsketch it up.\n\n-Peff\n"},{"id":"490970","messageId":"9cf0dfba-355b-4670-baee-a30d53976832@app.fastmail.com","threadId":"61071","inReplyTo":"20240319212940.GE1159535@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/3] revision: add a per-email field to rev-info","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-19T21:41:28Z","receivedAt":"2024-03-19T21:41:50Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Tue, Mar 19, 2024, at 22:29, Jeff King wrote:\n> On Tue, Mar 19, 2024 at 07:35:36PM +0100, Kristoffer Haugsbakk wrote:\n>\n>> Add `pe_header` to `rev_info` to store per-email headers.\n>\n> It is only just now that I realized that \"pe\" stands for per-email\n> (though to be fair I was not really focused on the intent of the series\n> when reading v1). Can we just call it per_email_headers or something?\n\nFor sure.\n\n>> The next commit will add an option to `format-patch` which will allow\n>> the user to store headers per-email; a complement to options like\n>> `--add-header`.\n>>\n>> To make this possible we need a new field to store these headers. We\n>> also need to take ownership of `extra_headers_p` in\n>> `log_write_email_headers`; facilitate this by removing constness from\n>> the relevant pointers.\n>\n> There are three pointers at play here:\n>\n> - ctx.after_subject has its const removed, since it will now always be\n> allocated by log_write_email_headers(), and then freed by the\n> caller. Makes sense Though it looks like we only free in show_log(),\n> and the free in make_cover_letter() is not added until patch 2?\n>\n> - rev_info.extra_headers has its const removed here, but I don't think\n> that is helping anything. We only use it to write into the \"headers\"\n> strbuf in log_write_email_headers(), which always returns\n> headers.buf (or NULL).\n>\n> - rev.pe_headers is introduced as non-const because it is allocated\n> and freed for each email. That makes some sense, though if we\n> followed the pattern of rev.extra_headers, then the pointer is\n> conceptually \"const\" within the rev_info struct, and it is the\n> caller who keeps track of the allocation (using a to_free variable).\n> Possibly we should do the same here?\n>\n> I do still think this could be split in a more obvious way, leaving the\n> pe_headers bits until they are actually needed. Let me see if I can\n> sketch it up.\n\nNice :)\n"},{"id":"490980","messageId":"20240320002555.GB903718@coredump.intra.peff.net","threadId":"61071","inReplyTo":"20240319212940.GE1159535@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/3] revision: add a per-email field to rev-info","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-20T00:25:55Z","receivedAt":"2024-03-20T00:25:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 19, 2024 at 05:29:40PM -0400, Jeff King wrote:\n\n> There are three pointers at play here:\n> \n> - ctx.after_subject has its const removed, since it will now always be\n> allocated by log_write_email_headers(), and then freed by the\n> caller. Makes sense Though it looks like we only free in show_log(),\n> and the free in make_cover_letter() is not added until patch 2?\n> \n> - rev_info.extra_headers has its const removed here, but I don't think\n> that is helping anything. We only use it to write into the \"headers\"\n> strbuf in log_write_email_headers(), which always returns\n> headers.buf (or NULL).\n> \n> - rev.pe_headers is introduced as non-const because it is allocated\n> and freed for each email. That makes some sense, though if we\n> followed the pattern of rev.extra_headers, then the pointer is\n> conceptually \"const\" within the rev_info struct, and it is the\n> caller who keeps track of the allocation (using a to_free variable).\n> Possibly we should do the same here?\n> \n> I do still think this could be split in a more obvious way, leaving the\n> pe_headers bits until they are actually needed. Let me see if I can\n> sketch it up.\n\nOK, this rabbit hole went much deeper than I expected. ;)\n\nI see why you wanted to drop the const from rev_info.extra_headers here.\nWe need the local extra_headers variable in show_log() to be non-const\n(since it receives the output of log_write_email_headers). But we also\nassign rev_info.extra_headers to that variable, and if it is const, the\ncompiler will complain.\n\nBut as it turns out, that assignment is not really necessary at all! It\nis only used when you have extra headers along with a non-email format.\nIn most cases we simply ignore the headers for those formats, and in the\none case where we do respect them, I think it is doing the wrong thing.\n\nSo here are some patches which clean things up. They would make a\nsuitable base for your changes, I think, but IMHO they also stand on\ntheir own as cleanups.\n\nHaving now stared at this code for a bit, I do think there's another,\nmuch simpler option for your series: keep the same ugly static-strbuf\nallocation pattern in log_write_email_headers(), but extend it further.\nI'll show that in a moment, too.\n\n [1/6]: shortlog: stop setting pp.print_email_subject\n [2/6]: pretty: split oneline and email subject printing\n [3/6]: pretty: drop print_email_subject flag\n [4/6]: log: do not set up extra_headers for non-email formats\n [5/6]: format-patch: return an allocated string from log_write_email_headers()\n [6/6]: format-patch: simplify after-subject MIME header handling\n\n builtin/log.c | 4 ++--\n builtin/rev-list.c | 1 +\n builtin/shortlog.c | 1 -\n log-tree.c | 22 +++++++++-------------\n log-tree.h | 2 +-\n pretty.c | 43 ++++++++++++++++++++-----------------------\n pretty.h | 11 +++++------\n 7 files changed, 38 insertions(+), 46 deletions(-)\n\n-Peff\n"},{"id":"490981","messageId":"20240320002716.GA904136@coredump.intra.peff.net","threadId":"61071","inReplyTo":"20240320002555.GB903718@coredump.intra.peff.net","subject":"[PATCH 1/6] shortlog: stop setting pp.print_email_subject","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-20T00:27:16Z","receivedAt":"2024-03-20T00:27:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When shortlog processes a commit using its internal traversal, it may\npretty-print the subject line for the summary view. When we do so, we\nset the \"print_email_subject\" flag in the pretty-print context. But this\nflag does nothing! Since we are using CMIT_FMT_USERFORMAT, we skip most\nof the usual formatting code entirely.\n\nThis flag is there due to commit 6d167fd7cc (pretty: use\nfmt_output_email_subject(), 2017-03-01). But that just switched us away\nfrom setting an empty \"subject\" header field, which was similarly\nuseless. That was added by dd2e794a21 (Refactor pretty_print_commit\narguments into a struct, 2009-10-19). Before using the struct, we had to\npass _something_ as the argument, so we passed the empty string (a NULL\nwould have worked equally well).\n\nSo this setting has never done anything, and we can drop the line. That\nshortens the code, but more importantly, makes it easier to reason about\nand refactor the other users of this flag.\n\nSigned-off-by: Jeff King \n---\n builtin/shortlog.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 1307ed2b88..3c7cd2d6ef 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -245,7 +245,6 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \n \tctx.fmt = CMIT_FMT_USERFORMAT;\n \tctx.abbrev = log->abbrev;\n-\tctx.print_email_subject = 1;\n \tctx.date_mode = log->date_mode;\n \tctx.output_encoding = get_log_output_encoding();\n \n-- \n2.44.0.643.g35f318e502\n\n"},{"id":"490982","messageId":"20240320002851.GB904136@coredump.intra.peff.net","threadId":"61071","inReplyTo":"20240320002555.GB903718@coredump.intra.peff.net","subject":"[PATCH 2/6] pretty: split oneline and email subject printing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-20T00:28:51Z","receivedAt":"2024-03-20T00:28:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The pp_title_line() function is used for two formats: the oneline format\nand the subject line of the email format. But most of the logic in the\nfunction does not make any sense for oneline; it is about special\nformatting of email headers.\n\nLumping the two formats together made sense long ago in 4234a76167\n(Extend --pretty=oneline to cover the first paragraph, 2007-06-11), when\nthere was a lot of manual logic to paste lines together. But later,\n88c44735ab (pretty: factor out format_subject(), 2008-12-27) pulled that\nlogic into its own function.\n\nWe can implement the oneline format by just calling that one function.\nThis makes the intention of the code much more clear, as we know we only\nneed to worry about those extra email options when dealing with actual\nemail.\n\nWhile the intent here is cleanup, it is possible to trigger these cases\nin practice by running format-patch with an explicit --oneline option.\nBut if you did, the results are basically nonsense. For example, with\nthe preserve_subject flag:\n\n $ printf \"%s\\n\" one two three | git commit --allow-empty -F -\n $ git format-patch -1 --stdout -k | grep ^Subject\n Subject: =?UTF-8?q?one=0Atwo=0Athree?=\n $ git format-patch -1 --stdout -k --oneline --no-signature\n 2af7fbe one\n two\n three\n\nOr with extra headers:\n\n $ git format-patch -1 --stdout --cc=me --oneline --no-signature\n 2af7fbe one two three\n Cc: me\n\nSo I'd actually consider this to be an improvement, though you are\nprobably crazy to use other formats with format-patch in the first place\n(arguably it should forbid non-email formats entirely, but that's a\nbigger change).\n\nAs a bonus, it eliminates some pointless extra allocations for the\noneline output. The email code, since it has to deal with wrapping,\nformats into an extra auxiliary buffer. The speedup is tiny, though like\n\"rev-list --no-abbrev --format=oneline\" seems to improve by a consistent\n1-2% for me.\n\nSigned-off-by: Jeff King \n---\n builtin/log.c | 2 +-\n pretty.c | 22 ++++++++++++----------\n pretty.h | 8 ++++----\n 3 files changed, 17 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex e5da0d1043..89cce9c29d 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1297,7 +1297,7 @@ static void prepare_cover_text(struct pretty_print_context *pp,\n \t\tsubject = subject_sb.buf;\n \n do_pp:\n-\tpp_title_line(pp, &subject, sb, encoding, need_8bit_cte);\n+\tpp_email_subject(pp, &subject, sb, encoding, need_8bit_cte);\n \tpp_remainder(pp, &body, sb, 0);\n \n \tstrbuf_release(&description_sb);\ndiff --git a/pretty.c b/pretty.c\nindex bdbed4295a..be0f2f566d 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -2077,11 +2077,11 @@ static void pp_header(struct pretty_print_context *pp,\n \t}\n }\n \n-void pp_title_line(struct pretty_print_context *pp,\n-\t\t const char **msg_p,\n-\t\t struct strbuf *sb,\n-\t\t const char *encoding,\n-\t\t int need_8bit_cte)\n+void pp_email_subject(struct pretty_print_context *pp,\n+\t\t const char **msg_p,\n+\t\t struct strbuf *sb,\n+\t\t const char *encoding,\n+\t\t int need_8bit_cte)\n {\n \tstatic const int max_length = 78; /* per rfc2047 */\n \tstruct strbuf title;\n@@ -2126,9 +2126,8 @@ void pp_title_line(struct pretty_print_context *pp,\n \tif (pp->after_subject) {\n \t\tstrbuf_addstr(sb, pp->after_subject);\n \t}\n-\tif (cmit_fmt_is_mail(pp->fmt)) {\n-\t\tstrbuf_addch(sb, '\\n');\n-\t}\n+\n+\tstrbuf_addch(sb, '\\n');\n \n \tif (pp->in_body_headers.nr) {\n \t\tint i;\n@@ -2328,8 +2327,11 @@ void pretty_print_commit(struct pretty_print_context *pp,\n \tmsg = skip_blank_lines(msg);\n \n \t/* These formats treat the title line specially. */\n-\tif (pp->fmt == CMIT_FMT_ONELINE || cmit_fmt_is_mail(pp->fmt))\n-\t\tpp_title_line(pp, &msg, sb, encoding, need_8bit_cte);\n+\tif (pp->fmt == CMIT_FMT_ONELINE) {\n+\t\tmsg = format_subject(sb, msg, \" \");\n+\t\tstrbuf_addch(sb, '\\n');\n+\t} else if (cmit_fmt_is_mail(pp->fmt))\n+\t\tpp_email_subject(pp, &msg, sb, encoding, need_8bit_cte);\n \n \tbeginning_of_body = sb->len;\n \tif (pp->fmt != CMIT_FMT_ONELINE)\ndiff --git a/pretty.h b/pretty.h\nindex 421209e9ec..d4ff79deb3 100644\n--- a/pretty.h\n+++ b/pretty.h\n@@ -96,13 +96,13 @@ void pp_user_info(struct pretty_print_context *pp, const char *what,\n \t\t\tconst char *encoding);\n \n /*\n- * Format title line of commit message taken from \"msg_p\" and\n+ * Format subject line of commit message taken from \"msg_p\" and\n * put it into \"sb\".\n * First line of \"msg_p\" is also affected.\n */\n-void pp_title_line(struct pretty_print_context *pp, const char **msg_p,\n-\t\t\tstruct strbuf *sb, const char *encoding,\n-\t\t\tint need_8bit_cte);\n+void pp_email_subject(struct pretty_print_context *pp, const char **msg_p,\n+\t\t struct strbuf *sb, const char *encoding,\n+\t\t int need_8bit_cte);\n \n /*\n * Get current state of commit message from \"msg_p\" and continue formatting\n-- \n2.44.0.643.g35f318e502\n\n"},{"id":"490983","messageId":"20240320003044.GC904136@coredump.intra.peff.net","threadId":"61071","inReplyTo":"20240320002555.GB903718@coredump.intra.peff.net","subject":"[PATCH 3/6] pretty: drop print_email_subject flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-20T00:30:44Z","receivedAt":"2024-03-20T00:30:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"With one exception, the print_email_subject flag is set if and only if\nthe commit format is email based:\n\n - in make_cover_letter() we set it along with CMIT_FMT_EMAIL\n explicitly\n\n - in show_log(), we set it if cmit_fmt_is_mail() is true. That covers\n format-patch as well as \"git log --format=email\" (or mboxrd).\n\nThe one exception is \"rev-list --format=email\", which somewhat\nnonsensically prints the author and date as email headers, but no\nsubject, like:\n\n $ git rev-list --format=email HEAD\n commit 64fc4c2cdd4db2645eaabb47aa4bac820b03cdba\n From: Jeff King \n Date: Tue, 19 Mar 2024 19:39:26 -0400\n\n this is the subject\n\n this is the body\n\nIt's doubtful that this is a useful format at all (the \"commit\" lines\nreplace the \"From\" lines that would make it work as an actual mbox).\nBut I think that printing the subject as a header (like this patch does)\nis the least surprising thing to do.\n\nSo let's drop this field, making the code a little simpler and easier to\nreason about. Note that we do need to set the \"rev\" field of the\npretty_print_context in rev-list, since that is used to check for\nsubject_prefix, etc. It's not possible to set those fields via rev-list,\nso we'll always just print \"Subject: \". But unless we pass in our\nrev_info, fmt_output_email_subject() would segfault trying to figure it\nout.\n\nSigned-off-by: Jeff King \n---\nThis one is not strictly necessary for building your series, but it\nseemed like a useful simplification/cleanup made possible by the\nprevious commits.\n\nI didn't bother with tests here or in the previous commit, because I\nthink the commands I've shown are basically nonsense. So while there are\nuser-visible changes, to me they are more \"this is slightly less\nnonsensical than the previous behavior\", and the main motivation is the\ncleanup.\n\n builtin/log.c | 1 -\n builtin/rev-list.c | 1 +\n log-tree.c | 1 -\n pretty.c | 21 ++++++++-------------\n pretty.h | 1 -\n 5 files changed, 9 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 89cce9c29d..071a7f3131 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1364,7 +1364,6 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \tpp.fmt = CMIT_FMT_EMAIL;\n \tpp.date_mode.type = DATE_RFC2822;\n \tpp.rev = rev;\n-\tpp.print_email_subject = 1;\n \tpp.encode_email_headers = rev->encode_email_headers;\n \tpp_user_info(&pp, NULL, &sb, committer, encoding);\n \tprepare_cover_text(&pp, description_file, branch_name, &sb,\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ec455aa972..77803727e0 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -219,6 +219,7 @@ static void show_commit(struct commit *commit, void *data)\n \t\tctx.fmt = revs->commit_format;\n \t\tctx.output_encoding = get_log_output_encoding();\n \t\tctx.color = revs->diffopt.use_color;\n+\t\tctx.rev = revs;\n \t\tpretty_print_commit(&ctx, commit, &buf);\n \t\tif (buf.len) {\n \t\t\tif (revs->commit_format != CMIT_FMT_ONELINE)\ndiff --git a/log-tree.c b/log-tree.c\nindex e5438b029d..c27240a533 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -742,7 +742,6 @@ void show_log(struct rev_info *opt)\n \t\tlog_write_email_headers(opt, commit, &extra_headers,\n \t\t\t\t\t&ctx.need_8bit_cte, 1);\n \t\tctx.rev = opt;\n-\t\tctx.print_email_subject = 1;\n \t} else if (opt->commit_format != CMIT_FMT_USERFORMAT) {\n \t\tfputs(diff_get_color_opt(&opt->diffopt, DIFF_COMMIT), opt->diffopt.file);\n \t\tif (opt->commit_format != CMIT_FMT_ONELINE)\ndiff --git a/pretty.c b/pretty.c\nindex be0f2f566d..eecbce82cf 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -2091,19 +2091,14 @@ void pp_email_subject(struct pretty_print_context *pp,\n \t\t\t\tpp->preserve_subject ? \"\\n\" : \" \");\n \n \tstrbuf_grow(sb, title.len + 1024);\n-\tif (pp->print_email_subject) {\n-\t\tif (pp->rev)\n-\t\t\tfmt_output_email_subject(sb, pp->rev);\n-\t\tif (pp->encode_email_headers &&\n-\t\t needs_rfc2047_encoding(title.buf, title.len))\n-\t\t\tadd_rfc2047(sb, title.buf, title.len,\n-\t\t\t\t\t\tencoding, RFC2047_SUBJECT);\n-\t\telse\n-\t\t\tstrbuf_add_wrapped_bytes(sb, title.buf, title.len,\n+\tfmt_output_email_subject(sb, pp->rev);\n+\tif (pp->encode_email_headers &&\n+\t needs_rfc2047_encoding(title.buf, title.len))\n+\t\tadd_rfc2047(sb, title.buf, title.len,\n+\t\t\t encoding, RFC2047_SUBJECT);\n+\telse\n+\t\tstrbuf_add_wrapped_bytes(sb, title.buf, title.len,\n \t\t\t\t\t -last_line_length(sb), 1, max_length);\n-\t} else {\n-\t\tstrbuf_addbuf(sb, &title);\n-\t}\n \tstrbuf_addch(sb, '\\n');\n \n \tif (need_8bit_cte == 0) {\n@@ -2319,7 +2314,7 @@ void pretty_print_commit(struct pretty_print_context *pp,\n \t}\n \n \tpp_header(pp, encoding, commit, &msg, sb);\n-\tif (pp->fmt != CMIT_FMT_ONELINE && !pp->print_email_subject) {\n+\tif (pp->fmt != CMIT_FMT_ONELINE && !cmit_fmt_is_mail(pp->fmt)) {\n \t\tstrbuf_addch(sb, '\\n');\n \t}\n \ndiff --git a/pretty.h b/pretty.h\nindex d4ff79deb3..021bc1d658 100644\n--- a/pretty.h\n+++ b/pretty.h\n@@ -39,7 +39,6 @@ struct pretty_print_context {\n \tint preserve_subject;\n \tstruct date_mode date_mode;\n \tunsigned date_mode_explicit:1;\n-\tint print_email_subject;\n \tint expand_tabs_in_log;\n \tint need_8bit_cte;\n \tchar *notes_message;\n-- \n2.44.0.643.g35f318e502\n\n"},{"id":"490984","messageId":"20240320003139.GD904136@coredump.intra.peff.net","threadId":"61071","inReplyTo":"20240320002555.GB903718@coredump.intra.peff.net","subject":"[PATCH 4/6] log: do not set up extra_headers for non-email formats","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-20T00:31:39Z","receivedAt":"2024-03-20T00:31:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The commit pretty-printer code has an \"after_subject\" parameter which it\nuses to insert extra headers into the email format. In show_log() we set\nthis by calling log_write_email_headers() if we are using an email\nformat, but otherwise default the variable to the rev_info.extra_headers\nvariable.\n\nSince the pretty-printer code will ignore after_subject unless we are\nusing an email format, this default is pointless. We can just set\nafter_subject directly, eliminating an extra variable.\n\nSigned-off-by: Jeff King \n---\nThis one is enabled by the previous commits. And after this now both\ncallers of log_write_email_headers() directly pass in \"after_subject\",\nwhich makes the next steps easy.\n\n log-tree.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex c27240a533..a50f79ec60 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -678,7 +678,6 @@ void show_log(struct rev_info *opt)\n \tstruct log_info *log = opt->loginfo;\n \tstruct commit *commit = log->commit, *parent = log->parent;\n \tint abbrev_commit = opt->abbrev_commit ? opt->abbrev : the_hash_algo->hexsz;\n-\tconst char *extra_headers = opt->extra_headers;\n \tstruct pretty_print_context ctx = {0};\n \n \topt->loginfo = NULL;\n@@ -739,7 +738,7 @@ void show_log(struct rev_info *opt)\n \t */\n \n \tif (cmit_fmt_is_mail(opt->commit_format)) {\n-\t\tlog_write_email_headers(opt, commit, &extra_headers,\n+\t\tlog_write_email_headers(opt, commit, &ctx.after_subject,\n \t\t\t\t\t&ctx.need_8bit_cte, 1);\n \t\tctx.rev = opt;\n \t} else if (opt->commit_format != CMIT_FMT_USERFORMAT) {\n@@ -807,7 +806,6 @@ void show_log(struct rev_info *opt)\n \tctx.date_mode = opt->date_mode;\n \tctx.date_mode_explicit = opt->date_mode_explicit;\n \tctx.abbrev = opt->diffopt.abbrev;\n-\tctx.after_subject = extra_headers;\n \tctx.preserve_subject = opt->preserve_subject;\n \tctx.encode_email_headers = opt->encode_email_headers;\n \tctx.reflog_info = opt->reflog_info;\n-- \n2.44.0.643.g35f318e502\n\n"},{"id":"490985","messageId":"20240320003533.GE904136@coredump.intra.peff.net","threadId":"61071","inReplyTo":"20240320002555.GB903718@coredump.intra.peff.net","subject":"[PATCH 5/6] format-patch: return an allocated string from log_write_email_headers()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-20T00:35:33Z","receivedAt":"2024-03-20T00:35:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When pretty-printing a commit in the email format, we have to fill in\nthe \"after subject\" field of the pretty_print_context with any extra\nheaders the user provided (e.g., from \"--to\" or \"--cc\" options) plus any\nspecial MIME headers.\n\nWe return an out-pointer that sometimes points to a newly heap-allocated\nstring and sometimes not. To avoid leaking, we store the allocated\nversion in a buffer with static lifetime, which is ugly. Worse, as we\nextend the header feature, we'll end up having to repeat this ugly\npattern.\n\nInstead, let's have our out-pointer pass ownership back to the caller,\nand duplicate the string when necessary. This does mean one extra\nallocation per commit when you use extra headers, but in the context of\nformat-patch which is showing diffs, I don't think that's even\nmeasurable.\n\nSigned-off-by: Jeff King \n---\nI don't think the extra allocation is a big deal, but if we do, there\nare some other options:\n\n - instead of an out-pointer we could take a strbuf, and the caller\n could reset and reuse a strbuf for each commit\n\n - the after_subject stuff could become a callback; we discussed this a\n long time ago (I had no recollection of the thread until finding it\n in the archive just now):\n\n https://lore.kernel.org/git/20170325211149.yyvocmdfw4zbjyoi@sigill.intra.peff.net/\n\n - this log_write_email_headers() function prints part of its output to\n stdout, and shoves part of it into the after_subject field to be\n shown by the pretty-printer. I wonder if it could just format the\n subject itself (though that would make \"rev-list --format=email\"\n even more awkward, I guess).\n\n builtin/log.c | 1 +\n log-tree.c | 11 ++++++-----\n log-tree.h | 2 +-\n pretty.h | 2 +-\n 4 files changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 071a7f3131..c0a8bb95e9 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1370,6 +1370,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,\n \t\t\t encoding, need_8bit_cte);\n \tfprintf(rev->diffopt.file, \"%s\\n\", sb.buf);\n \n+\tfree(pp.after_subject);\n \tstrbuf_release(&sb);\n \n \tshortlog_init(&log);\ndiff --git a/log-tree.c b/log-tree.c\nindex a50f79ec60..5092a75958 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -470,11 +470,11 @@ void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)\n }\n \n void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n-\t\t\t const char **extra_headers_p,\n+\t\t\t char **extra_headers_p,\n \t\t\t int *need_8bit_cte_p,\n \t\t\t int maybe_multipart)\n {\n-\tconst char *extra_headers = opt->extra_headers;\n+\tchar *extra_headers = xstrdup_or_null(opt->extra_headers);\n \tconst char *name = oid_to_hex(opt->zero_commit ?\n \t\t\t\t null_oid() : &commit->object.oid);\n \n@@ -496,12 +496,11 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\tgraph_show_oneline(opt->graph);\n \t}\n \tif (opt->mime_boundary && maybe_multipart) {\n-\t\tstatic struct strbuf subject_buffer = STRBUF_INIT;\n+\t\tstruct strbuf subject_buffer = STRBUF_INIT;\n \t\tstatic struct strbuf buffer = STRBUF_INIT;\n \t\tstruct strbuf filename = STRBUF_INIT;\n \t\t*need_8bit_cte_p = -1; /* NEVER */\n \n-\t\tstrbuf_reset(&subject_buffer);\n \t\tstrbuf_reset(&buffer);\n \n \t\tstrbuf_addf(&subject_buffer,\n@@ -519,7 +518,8 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t extra_headers ? extra_headers : \"\",\n \t\t\t mime_boundary_leader, opt->mime_boundary,\n \t\t\t mime_boundary_leader, opt->mime_boundary);\n-\t\textra_headers = subject_buffer.buf;\n+\t\tfree(extra_headers);\n+\t\textra_headers = strbuf_detach(&subject_buffer, NULL);\n \n \t\tif (opt->numbered_files)\n \t\t\tstrbuf_addf(&filename, \"%d\", opt->nr);\n@@ -854,6 +854,7 @@ void show_log(struct rev_info *opt)\n \n \tstrbuf_release(&msgbuf);\n \tfree(ctx.notes_message);\n+\tfree(ctx.after_subject);\n \n \tif (cmit_fmt_is_mail(ctx.fmt) && opt->idiff_oid1) {\n \t\tstruct diff_queue_struct dq;\ndiff --git a/log-tree.h b/log-tree.h\nindex 41c776fea5..94978e2c83 100644\n--- a/log-tree.h\n+++ b/log-tree.h\n@@ -29,7 +29,7 @@ void format_decorations(struct strbuf *sb, const struct commit *commit,\n \t\t\tint use_color, const struct decoration_options *opts);\n void show_decorations(struct rev_info *opt, struct commit *commit);\n void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n-\t\t\t const char **extra_headers_p,\n+\t\t\t char **extra_headers_p,\n \t\t\t int *need_8bit_cte_p,\n \t\t\t int maybe_multipart);\n void load_ref_decorations(struct decoration_filter *filter, int flags);\ndiff --git a/pretty.h b/pretty.h\nindex 021bc1d658..9cc9e5d42b 100644\n--- a/pretty.h\n+++ b/pretty.h\n@@ -35,7 +35,7 @@ struct pretty_print_context {\n \t */\n \tenum cmit_fmt fmt;\n \tint abbrev;\n-\tconst char *after_subject;\n+\tchar *after_subject;\n \tint preserve_subject;\n \tstruct date_mode date_mode;\n \tunsigned date_mode_explicit:1;\n-- \n2.44.0.643.g35f318e502\n\n"},{"id":"490986","messageId":"20240320003557.GF904136@coredump.intra.peff.net","threadId":"61071","inReplyTo":"20240320002555.GB903718@coredump.intra.peff.net","subject":"[PATCH 6/6] format-patch: simplify after-subject MIME header handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-20T00:35:57Z","receivedAt":"2024-03-20T00:35:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In log_write_email_headers(), we append our MIME headers to the set of\nextra headers by creating a new strbuf, adding the existing headers, and\nthen adding our new ones. We had to do it this way when our output\nbuffer might point to the constant opt->extra_headers variable.\n\nBut since the previous commit, we always make a local copy of that\nvariable. Let's turn that into a strbuf, which lets the MIME code simply\nappend to it. That simplifies the function and avoids a pointless extra\ncopy of the headers.\n\nSigned-off-by: Jeff King \n---\n log-tree.c | 14 ++++++--------\n 1 file changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 5092a75958..eb2e841046 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -474,12 +474,15 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t int *need_8bit_cte_p,\n \t\t\t int maybe_multipart)\n {\n-\tchar *extra_headers = xstrdup_or_null(opt->extra_headers);\n+\tstruct strbuf headers = STRBUF_INIT;\n \tconst char *name = oid_to_hex(opt->zero_commit ?\n \t\t\t\t null_oid() : &commit->object.oid);\n \n \t*need_8bit_cte_p = 0; /* unknown */\n \n+\tif (opt->extra_headers)\n+\t\tstrbuf_addstr(&headers, opt->extra_headers);\n+\n \tfprintf(opt->diffopt.file, \"From %s Mon Sep 17 00:00:00 2001\\n\", name);\n \tgraph_show_oneline(opt->graph);\n \tif (opt->message_id) {\n@@ -496,15 +499,13 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\tgraph_show_oneline(opt->graph);\n \t}\n \tif (opt->mime_boundary && maybe_multipart) {\n-\t\tstruct strbuf subject_buffer = STRBUF_INIT;\n \t\tstatic struct strbuf buffer = STRBUF_INIT;\n \t\tstruct strbuf filename = STRBUF_INIT;\n \t\t*need_8bit_cte_p = -1; /* NEVER */\n \n \t\tstrbuf_reset(&buffer);\n \n-\t\tstrbuf_addf(&subject_buffer,\n-\t\t\t \"%s\"\n+\t\tstrbuf_addf(&headers,\n \t\t\t \"MIME-Version: 1.0\\n\"\n \t\t\t \"Content-Type: multipart/mixed;\"\n \t\t\t \" boundary=\\\"%s%s\\\"\\n\"\n@@ -515,11 +516,8 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t \"Content-Type: text/plain; \"\n \t\t\t \"charset=UTF-8; format=fixed\\n\"\n \t\t\t \"Content-Transfer-Encoding: 8bit\\n\\n\",\n-\t\t\t extra_headers ? extra_headers : \"\",\n \t\t\t mime_boundary_leader, opt->mime_boundary,\n \t\t\t mime_boundary_leader, opt->mime_boundary);\n-\t\tfree(extra_headers);\n-\t\textra_headers = strbuf_detach(&subject_buffer, NULL);\n \n \t\tif (opt->numbered_files)\n \t\t\tstrbuf_addf(&filename, \"%d\", opt->nr);\n@@ -539,7 +537,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\topt->diffopt.stat_sep = buffer.buf;\n \t\tstrbuf_release(&filename);\n \t}\n-\t*extra_headers_p = extra_headers;\n+\t*extra_headers_p = headers.len ? strbuf_detach(&headers, NULL) : NULL;\n }\n \n static void show_sig_lines(struct rev_info *opt, int status, const char *bol)\n-- \n2.44.0.643.g35f318e502\n"},{"id":"490987","messageId":"20240320004314.GA907161@coredump.intra.peff.net","threadId":"61071","inReplyTo":"20240320002555.GB903718@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/3] revision: add a per-email field to rev-info","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-20T00:43:14Z","receivedAt":"2024-03-20T00:43:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 19, 2024 at 08:25:55PM -0400, Jeff King wrote:\n\n> Having now stared at this code for a bit, I do think there's another,\n> much simpler option for your series: keep the same ugly static-strbuf\n> allocation pattern in log_write_email_headers(), but extend it further.\n> I'll show that in a moment, too.\n\nSo something like this:\n\ndiff --git a/log-tree.c b/log-tree.c\nindex e5438b029d..ae0f4fc502 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -474,12 +474,21 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t int *need_8bit_cte_p,\n \t\t\t int maybe_multipart)\n {\n-\tconst char *extra_headers = opt->extra_headers;\n+\tstatic struct strbuf headers = STRBUF_INIT;\n \tconst char *name = oid_to_hex(opt->zero_commit ?\n \t\t\t\t null_oid() : &commit->object.oid);\n \n \t*need_8bit_cte_p = 0; /* unknown */\n \n+\tstrbuf_reset(&headers);\n+\tif (opt->extra_headers)\n+\t\tstrbuf_addstr(&headers, opt->extra_headers);\n+\t/*\n+\t * here's where you'd do your pe_headers; I wonder if you could even\n+\t * just run the header command directly here and not need to shove the\n+\t * string into rev_info?\n+\t */\n+\n \tfprintf(opt->diffopt.file, \"From %s Mon Sep 17 00:00:00 2001\\n\", name);\n \tgraph_show_oneline(opt->graph);\n \tif (opt->message_id) {\n@@ -496,16 +505,13 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\tgraph_show_oneline(opt->graph);\n \t}\n \tif (opt->mime_boundary && maybe_multipart) {\n-\t\tstatic struct strbuf subject_buffer = STRBUF_INIT;\n \t\tstatic struct strbuf buffer = STRBUF_INIT;\n \t\tstruct strbuf filename = STRBUF_INIT;\n \t\t*need_8bit_cte_p = -1; /* NEVER */\n \n-\t\tstrbuf_reset(&subject_buffer);\n \t\tstrbuf_reset(&buffer);\n \n-\t\tstrbuf_addf(&subject_buffer,\n-\t\t\t \"%s\"\n+\t\tstrbuf_addf(&headers,\n \t\t\t \"MIME-Version: 1.0\\n\"\n \t\t\t \"Content-Type: multipart/mixed;\"\n \t\t\t \" boundary=\\\"%s%s\\\"\\n\"\n@@ -516,10 +522,8 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t \"Content-Type: text/plain; \"\n \t\t\t \"charset=UTF-8; format=fixed\\n\"\n \t\t\t \"Content-Transfer-Encoding: 8bit\\n\\n\",\n-\t\t\t extra_headers ? extra_headers : \"\",\n \t\t\t mime_boundary_leader, opt->mime_boundary,\n \t\t\t mime_boundary_leader, opt->mime_boundary);\n-\t\textra_headers = subject_buffer.buf;\n \n \t\tif (opt->numbered_files)\n \t\t\tstrbuf_addf(&filename, \"%d\", opt->nr);\n@@ -539,7 +543,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\topt->diffopt.stat_sep = buffer.buf;\n \t\tstrbuf_release(&filename);\n \t}\n-\t*extra_headers_p = extra_headers;\n+\t*extra_headers_p = headers.len ? headers.buf : NULL;\n }\n \n static void show_sig_lines(struct rev_info *opt, int status, const char *bol)\n\nAnd then the callers can continue not caring about how or when to free\nthe returned pointer. I think in the long run the cleanups I showed are\na nicer place to end up, but I'd just worry that your feature work will\nbe held hostage by my desire to clean. ;)\n\nIf you did it this way (probably as a separate preparatory patch minus\nthe pe_headers comment), then either I could do my cleanups on top, or\nthey could even graduate independently (though obviously there will be a\nlittle bit of tricky merging at the end).\n\n-Peff\n"},{"id":"491225","messageId":"20240322095951.GA529578@coredump.intra.peff.net","threadId":"61071","inReplyTo":"20240320002555.GB903718@coredump.intra.peff.net","subject":"[PATCH 7/6] format-patch: fix leak of empty header string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-22T09:59:51Z","receivedAt":"2024-03-22T09:59:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 19, 2024 at 08:25:55PM -0400, Jeff King wrote:\n\n> [1/6]: shortlog: stop setting pp.print_email_subject\n> [2/6]: pretty: split oneline and email subject printing\n> [3/6]: pretty: drop print_email_subject flag\n> [4/6]: log: do not set up extra_headers for non-email formats\n> [5/6]: format-patch: return an allocated string from log_write_email_headers()\n> [6/6]: format-patch: simplify after-subject MIME header handling\n\nThese patches introduce a small leak into format-patch. I didn't notice\nbefore because the \"leaks\" CI jobs were broken due to sanitizer problems\nin the base image (which now seem fixed?).\n\nHere's a fix that can go on top of jk/pretty-subject-cleanup. That topic\nis not in 'next' yet, so I could also re-roll. The issue was subtle\nenough that a separate commit is not such a bad thing, but I'm happy to\nsquash it in if we'd prefer.\n\n-- >8 --\nSubject: [PATCH] format-patch: fix leak of empty header string\n\nThe log_write_email_headers() function recently learned to return the\n\"extra_headers_p\" variable to the caller as an allocated string. We\nstart by copying rev_info.extra_headers into a strbuf, and then detach\nthe strbuf at the end of the function. If there are no extra headers, we\nleave the strbuf empty. Likewise, if there are no headers to return, we\npass back NULL.\n\nThis misses a corner case which can cause a leak. The \"do we have any\nheaders to copy\" check is done by looking for a NULL opt->extra_headers.\nBut the \"do we have a non-empty string to return\" check is done by\nchecking the length of the strbuf. That means if opt->extra_headers is\nthe empty string, we'll \"copy\" it into the strbuf, triggering an\nallocation, but then leak the buffer when we return NULL from the\nfunction.\n\nWe can solve this in one of two ways:\n\n 1. Rather than checking headers->len at the end, we could check\n headers->alloc to see if we allocated anything. That retains the\n original behavior before the recent change, where an empty\n extra_headers string is \"passed through\" to the caller. In practice\n this doesn't matter, though (the code which eventually looks at the\n result treats NULL or the empty string the same).\n\n 2. Only bother copying a non-empty string into the strbuf. This has\n the added bonus of avoiding a pointless allocation.\n\n Arguably strbuf_addstr() could do this optimization itself, though\n it may be slightly dangerous to do so (some existing callers may\n not get a fresh allocation when they expect to). In theory callers\n are all supposed to use strbuf_detach() in such a case, but there's\n no guarantee that this is the case.\n\nThis patch uses option 2. Without it, building with SANITIZE=leak shows\nmany errors in t4021 and elsewhere.\n\nSigned-off-by: Jeff King \n---\n log-tree.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex eb2e841046..59eeaef1f7 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -480,7 +480,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \n \t*need_8bit_cte_p = 0; /* unknown */\n \n-\tif (opt->extra_headers)\n+\tif (opt->extra_headers && *opt->extra_headers)\n \t\tstrbuf_addstr(&headers, opt->extra_headers);\n \n \tfprintf(opt->diffopt.file, \"From %s Mon Sep 17 00:00:00 2001\\n\", name);\n-- \n2.44.0.682.g01e1dab148\n\n"},{"id":"491226","messageId":"8a4d6e5e-5640-4230-8651-8e0846b383fd@app.fastmail.com","threadId":"61071","inReplyTo":"20240322095951.GA529578@coredump.intra.peff.net","subject":"Re: [PATCH 7/6] format-patch: fix leak of empty header string","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-22T10:03:21Z","receivedAt":"2024-03-22T10:03:44Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Fri, Mar 22, 2024, at 10:59, Jeff King wrote:\n> On Tue, Mar 19, 2024 at 08:25:55PM -0400, Jeff King wrote:\n>\n>> [1/6]: shortlog: stop setting pp.print_email_subject\n>> [2/6]: pretty: split oneline and email subject printing\n>> [3/6]: pretty: drop print_email_subject flag\n>> [4/6]: log: do not set up extra_headers for non-email formats\n>> [5/6]: format-patch: return an allocated string from log_write_email_headers()\n>> [6/6]: format-patch: simplify after-subject MIME header handling\n>\n> These patches introduce a small leak into format-patch. I didn't notice\n> before because the \"leaks\" CI jobs were broken due to sanitizer problems\n> in the base image (which now seem fixed?).\n>\n> Here's a fix that can go on top of jk/pretty-subject-cleanup. That topic\n> is not in 'next' yet, so I could also re-roll. The issue was subtle\n> enough that a separate commit is not such a bad thing, but I'm happy to\n> squash it in if we'd prefer.\n>\n> -- >8 --\n> Subject: [PATCH] format-patch: fix leak of empty header string\n> [snip]\n\nHi Peff, and thanks a lot for making this series.\n\nI’ll have a look at it this evening.\n\nThanks\n\n-- \nKristoffer Haugsbakk\n"},{"id":"491252","messageId":"xmqqr0g21a8n.fsf@gitster.g","threadId":"61071","inReplyTo":"20240322095951.GA529578@coredump.intra.peff.net","subject":"Re: [PATCH 7/6] format-patch: fix leak of empty header string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-22T16:50:48Z","receivedAt":"2024-03-22T16:50:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Mar 19, 2024 at 08:25:55PM -0400, Jeff King wrote:\n>\n>> [1/6]: shortlog: stop setting pp.print_email_subject\n>> [2/6]: pretty: split oneline and email subject printing\n>> [3/6]: pretty: drop print_email_subject flag\n>> [4/6]: log: do not set up extra_headers for non-email formats\n>> [5/6]: format-patch: return an allocated string from log_write_email_headers()\n>> [6/6]: format-patch: simplify after-subject MIME header handling\n>\n> These patches introduce a small leak into format-patch. I didn't notice\n> before because the \"leaks\" CI jobs were broken due to sanitizer problems\n> in the base image (which now seem fixed?).\n>\n> Here's a fix that can go on top of jk/pretty-subject-cleanup. That topic\n> is not in 'next' yet, so I could also re-roll. The issue was subtle\n> enough that a separate commit is not such a bad thing, but I'm happy to\n> squash it in if we'd prefer.\n\nIndeed it is subtle and I like the corner case described separately\nlike this one does. Very much appreciated.\n\nThanks.\n\n> -- >8 --\n> Subject: [PATCH] format-patch: fix leak of empty header string\n>\n> The log_write_email_headers() function recently learned to return the\n> \"extra_headers_p\" variable to the caller as an allocated string. We\n> start by copying rev_info.extra_headers into a strbuf, and then detach\n> the strbuf at the end of the function. If there are no extra headers, we\n> leave the strbuf empty. Likewise, if there are no headers to return, we\n> pass back NULL.\n>\n> This misses a corner case which can cause a leak. The \"do we have any\n> headers to copy\" check is done by looking for a NULL opt->extra_headers.\n> But the \"do we have a non-empty string to return\" check is done by\n> checking the length of the strbuf. That means if opt->extra_headers is\n> the empty string, we'll \"copy\" it into the strbuf, triggering an\n> allocation, but then leak the buffer when we return NULL from the\n> function.\n>\n> We can solve this in one of two ways:\n>\n> 1. Rather than checking headers->len at the end, we could check\n> headers->alloc to see if we allocated anything. That retains the\n> original behavior before the recent change, where an empty\n> extra_headers string is \"passed through\" to the caller. In practice\n> this doesn't matter, though (the code which eventually looks at the\n> result treats NULL or the empty string the same).\n>\n> 2. Only bother copying a non-empty string into the strbuf. This has\n> the added bonus of avoiding a pointless allocation.\n>\n> Arguably strbuf_addstr() could do this optimization itself, though\n> it may be slightly dangerous to do so (some existing callers may\n> not get a fresh allocation when they expect to). In theory callers\n> are all supposed to use strbuf_detach() in such a case, but there's\n> no guarantee that this is the case.\n>\n> This patch uses option 2. Without it, building with SANITIZE=leak shows\n> many errors in t4021 and elsewhere.\n>\n> Signed-off-by: Jeff King \n> ---\n> log-tree.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/log-tree.c b/log-tree.c\n> index eb2e841046..59eeaef1f7 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -480,7 +480,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n> \n> \t*need_8bit_cte_p = 0; /* unknown */\n> \n> -\tif (opt->extra_headers)\n> +\tif (opt->extra_headers && *opt->extra_headers)\n> \t\tstrbuf_addstr(&headers, opt->extra_headers);\n> \n> \tfprintf(opt->diffopt.file, \"From %s Mon Sep 17 00:00:00 2001\\n\", name);\n"},{"id":"491262","messageId":"69904aaf-7830-4b2a-8fd9-7f6fe55b45fe@app.fastmail.com","threadId":"61071","inReplyTo":"20240320002851.GB904136@coredump.intra.peff.net","subject":"Re: [PATCH 2/6] pretty: split oneline and email subject printing","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-22T22:00:32Z","receivedAt":"2024-03-22T22:00:53Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Wed, Mar 20, 2024, at 01:28, Jeff King wrote:\n> The pp_title_line() function is used for two formats: the oneline format\n> and the subject line of the email format. But most of the logic in the\n> function does not make any sense for oneline; it is about special\n> formatting of email headers.\n>\n> Lumping the two formats together made sense long ago in 4234a76167\n> (Extend --pretty=oneline to cover the first paragraph, 2007-06-11), when\n> there was a lot of manual logic to paste lines together. But later,\n> 88c44735ab (pretty: factor out format_subject(), 2008-12-27) pulled that\n> logic into its own function.\n>\n> We can implement the oneline format by just calling that one function.\n> This makes the intention of the code much more clear, as we know we only\n> need to worry about those extra email options when dealing with actual\n> email.\n>\n> While the intent here is cleanup, it is possible to trigger these cases\n> in practice by running format-patch with an explicit --oneline option.\n> But if you did, the results are basically nonsense. For example, with\n> the preserve_subject flag:\n>\n> $ printf \"%s\\n\" one two three | git commit --allow-empty -F -\n> $ git format-patch -1 --stdout -k | grep ^Subject\n> Subject: =?UTF-8?q?one=0Atwo=0Athree?=\n> $ git format-patch -1 --stdout -k --oneline --no-signature\n> 2af7fbe one\n> two\n> three\n>\n> Or with extra headers:\n>\n> $ git format-patch -1 --stdout --cc=me --oneline --no-signature\n> 2af7fbe one two three\n> Cc: me\n>\n> So I'd actually consider this to be an improvement, though you are\n> probably crazy to use other formats with format-patch in the first place\n> (arguably it should forbid non-email formats entirely, but that's a\n> bigger change).\n\nMakes sense. This make the code more focused.\n\n> As a bonus, it eliminates some pointless extra allocations for the\n> oneline output. The email code, since it has to deal with wrapping,\n> formats into an extra auxiliary buffer. The speedup is tiny, though like\n> \"rev-list --no-abbrev --format=oneline\" seems to improve by a consistent\n> 1-2% for me.\n\nNice. That could add up when formatting a moderate amount of patches.\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"491263","messageId":"ad20990e-1328-458a-a142-7d2d93bda170@app.fastmail.com","threadId":"61071","inReplyTo":"20240320003139.GD904136@coredump.intra.peff.net","subject":"Re: [PATCH 4/6] log: do not set up extra_headers for non-email formats","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-22T22:04:31Z","receivedAt":"2024-03-22T22:04:57Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Wed, Mar 20, 2024, at 01:31, Jeff King wrote:\n> The commit pretty-printer code has an \"after_subject\" parameter which it\n> uses to insert extra headers into the email format. In show_log() we set\n> this by calling log_write_email_headers() if we are using an email\n> format, but otherwise default the variable to the rev_info.extra_headers\n> variable.\n>\n> Since the pretty-printer code will ignore after_subject unless we are\n> using an email format, this default is pointless. We can just set\n> after_subject directly, eliminating an extra variable.\n>\n> Signed-off-by: Jeff King \n\nGood. I did feel like the code was kind of daisy-chaining assignments\nfor no obvious reason.\n\n> ---\n> This one is enabled by the previous commits. And after this now both\n> callers of log_write_email_headers() directly pass in \"after_subject\",\n> which makes the next steps easy.\n\nYep, these changes are being done in a nice progression.\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"491265","messageId":"c94e0ba2-87b9-4272-afce-67f5faf0275f@app.fastmail.com","threadId":"61071","inReplyTo":"20240320003533.GE904136@coredump.intra.peff.net","subject":"Re: [PATCH 5/6] format-patch: return an allocated string from log_write_email_headers()","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-22T22:06:53Z","receivedAt":"2024-03-22T22:07:15Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Wed, Mar 20, 2024, at 01:35, Jeff King wrote:\n> When pretty-printing a commit in the email format, we have to fill in\n> the \"after subject\" field of the pretty_print_context with any extra\n> headers the user provided (e.g., from \"--to\" or \"--cc\" options) plus any\n> special MIME headers.\n>\n> We return an out-pointer that sometimes points to a newly heap-allocated\n> string and sometimes not. To avoid leaking, we store the allocated\n> version in a buffer with static lifetime, which is ugly. Worse, as we\n> extend the header feature, we'll end up having to repeat this ugly\n> pattern.\n>\n> Instead, let's have our out-pointer pass ownership back to the caller,\n> and duplicate the string when necessary. This does mean one extra\n> allocation per commit when you use extra headers, but in the context of\n> format-patch which is showing diffs, I don't think that's even\n> measurable.\n>\n> Signed-off-by: Jeff King \n\nGood presentation of motivation here.\n\n> ---\n> I don't think the extra allocation is a big deal, but if we do, there\n> are some other options:\n>\n> - instead of an out-pointer we could take a strbuf, and the caller\n> could reset and reuse a strbuf for each commit\n>\n> - the after_subject stuff could become a callback; we discussed this a\n> long time ago (I had no recollection of the thread until finding it\n> in the archive just now):\n>\n>\n> https://lore.kernel.org/git/20170325211149.yyvocmdfw4zbjyoi@sigill.intra.peff.net/\n>\n> - this log_write_email_headers() function prints part of its output to\n> stdout, and shoves part of it into the after_subject field to be\n> shown by the pretty-printer. I wonder if it could just format the\n> subject itself (though that would make \"rev-list --format=email\"\n> even more awkward, I guess).\n\nI don’t quite understand all of these alternatives but the first one\nmakes sense. Leave the responsibility to the caller. That could work.\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"491267","messageId":"e9a1da5a-733b-4122-917a-5f5356ea1f7a@app.fastmail.com","threadId":"61071","inReplyTo":"20240320003557.GF904136@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] format-patch: simplify after-subject MIME header handling","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-22T22:08:49Z","receivedAt":"2024-03-22T22:09:11Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Wed, Mar 20, 2024, at 01:35, Jeff King wrote:\n> In log_write_email_headers(), we append our MIME headers to the set of\n> extra headers by creating a new strbuf, adding the existing headers, and\n> then adding our new ones. We had to do it this way when our output\n> buffer might point to the constant opt->extra_headers variable.\n>\n> But since the previous commit, we always make a local copy of that\n> variable. Let's turn that into a strbuf, which lets the MIME code simply\n> append to it. That simplifies the function and avoids a pointless extra\n> copy of the headers.\n>\n> Signed-off-by: Jeff King \n\nI like how all the previous work makes this change straightforward.\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"491275","messageId":"fe89de4a-44aa-4d2a-8bb5-742a0bb4a7e0@app.fastmail.com","threadId":"61071","inReplyTo":"20240322095951.GA529578@coredump.intra.peff.net","subject":"Re: [PATCH 7/6] format-patch: fix leak of empty header string","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-22T22:16:59Z","receivedAt":"2024-03-22T22:17:32Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Fri, Mar 22, 2024, at 10:59, Jeff King wrote:\n> On Tue, Mar 19, 2024 at 08:25:55PM -0400, Jeff King wrote:\n>\n>> [1/6]: shortlog: stop setting pp.print_email_subject\n>> [2/6]: pretty: split oneline and email subject printing\n>> [3/6]: pretty: drop print_email_subject flag\n>> [4/6]: log: do not set up extra_headers for non-email formats\n>> [5/6]: format-patch: return an allocated string from log_write_email_headers()\n>> [6/6]: format-patch: simplify after-subject MIME header handling\n>\n> These patches introduce a small leak into format-patch. I didn't notice\n> before because the \"leaks\" CI jobs were broken due to sanitizer problems\n> in the base image (which now seem fixed?).\n>\n> Here's a fix that can go on top of jk/pretty-subject-cleanup. That topic\n> is not in 'next' yet, so I could also re-roll. The issue was subtle\n> enough that a separate commit is not such a bad thing, but I'm happy to\n> squash it in if we'd prefer.\n>\n> -- >8 --\n> Subject: [PATCH] format-patch: fix leak of empty header string\n>\n> The log_write_email_headers() function recently learned to return the\n> \"extra_headers_p\" variable to the caller as an allocated string. We\n> start by copying rev_info.extra_headers into a strbuf, and then detach\n> the strbuf at the end of the function. If there are no extra headers, we\n> leave the strbuf empty. Likewise, if there are no headers to return, we\n> pass back NULL.\n>\n> This misses a corner case which can cause a leak. The \"do we have any\n> headers to copy\" check is done by looking for a NULL opt->extra_headers.\n> But the \"do we have a non-empty string to return\" check is done by\n> checking the length of the strbuf. That means if opt->extra_headers is\n> the empty string, we'll \"copy\" it into the strbuf, triggering an\n> allocation, but then leak the buffer when we return NULL from the\n> function.\n>\n> We can solve this in one of two ways:\n>\n> 1. Rather than checking headers->len at the end, we could check\n> headers->alloc to see if we allocated anything. That retains the\n> original behavior before the recent change, where an empty\n> extra_headers string is \"passed through\" to the caller. In practice\n> this doesn't matter, though (the code which eventually looks at the\n> result treats NULL or the empty string the same).\n>\n> 2. Only bother copying a non-empty string into the strbuf. This has\n> the added bonus of avoiding a pointless allocation.\n>\n> Arguably strbuf_addstr() could do this optimization itself, though\n> it may be slightly dangerous to do so (some existing callers may\n> not get a fresh allocation when they expect to). In theory callers\n> are all supposed to use strbuf_detach() in such a case, but there's\n> no guarantee that this is the case.\n>\n> This patch uses option 2. Without it, building with SANITIZE=leak shows\n> many errors in t4021 and elsewhere.\n>\n> Signed-off-by: Jeff King \n> ---\n> log-tree.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/log-tree.c b/log-tree.c\n> index eb2e841046..59eeaef1f7 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -480,7 +480,7 @@ void log_write_email_headers(struct rev_info *opt,\n> struct commit *commit,\n>\n> \t*need_8bit_cte_p = 0; /* unknown */\n>\n> -\tif (opt->extra_headers)\n> +\tif (opt->extra_headers && *opt->extra_headers)\n> \t\tstrbuf_addstr(&headers, opt->extra_headers);\n>\n> \tfprintf(opt->diffopt.file, \"From %s Mon Sep 17 00:00:00 2001\\n\", name);\n> --\n> 2.44.0.682.g01e1dab148\n\nI was wondering if the new empty-string check now makes the condition\nlook non-obvious. I mean given that\n\n• You explain how headers-to-copy-check and have-non-empty-string are\n not the same\n• You explain how strbuf_addstr() could do this itself (which makes\n sense) but how it could be risky\n\nThe condition looks bare without a comment. But the empty-string check\nof course makes sense without this context. And it could also be read as\nan optimization (and not a leak fix).\n\nAnd maybe most people just `git log -S'*opt->extra_headers'` if they\nhave questions in their head. So no information is really missing.\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"491279","messageId":"a459091e-b570-4d5b-9b12-3e4ed7f70615@app.fastmail.com","threadId":"61071","inReplyTo":"20240320004314.GA907161@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/3] revision: add a per-email field to rev-info","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-22T22:31:08Z","receivedAt":"2024-03-22T22:31:31Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Wed, Mar 20, 2024, at 01:43, Jeff King wrote:\n> On Tue, Mar 19, 2024 at 08:25:55PM -0400, Jeff King wrote:\n>\n>> Having now stared at this code for a bit, I do think there's another,\n>> much simpler option for your series: keep the same ugly static-strbuf\n>> allocation pattern in log_write_email_headers(), but extend it further.\n>> I'll show that in a moment, too.\n>\n> So something like this:\n>\n> diff --git a/log-tree.c b/log-tree.c\n> index e5438b029d..ae0f4fc502 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -474,12 +474,21 @@ void log_write_email_headers(struct rev_info\n> *opt, struct commit *commit,\n> \t\t\t int *need_8bit_cte_p,\n> \t\t\t int maybe_multipart)\n> {\n> -\tconst char *extra_headers = opt->extra_headers;\n> +\tstatic struct strbuf headers = STRBUF_INIT;\n> \tconst char *name = oid_to_hex(opt->zero_commit ?\n> \t\t\t\t null_oid() : &commit->object.oid);\n>\n> \t*need_8bit_cte_p = 0; /* unknown */\n>\n> +\tstrbuf_reset(&headers);\n> +\tif (opt->extra_headers)\n> +\t\tstrbuf_addstr(&headers, opt->extra_headers);\n> +\t/*\n> +\t * here's where you'd do your pe_headers; I wonder if you could even\n> +\t * just run the header command directly here and not need to shove the\n> +\t * string into rev_info?\n> +\t */\n> +\n\nHmm. I’ll look into that. This seems like a nicer place to do it\ncompared to `log.c`.\n\n> \tfprintf(opt->diffopt.file, \"From %s Mon Sep 17 00:00:00 2001\\n\",\n> name);\n> \tgraph_show_oneline(opt->graph);\n> \tif (opt->message_id) {\n> @@ -496,16 +505,13 @@ void log_write_email_headers(struct rev_info\n> *opt, struct commit *commit,\n> \t\tgraph_show_oneline(opt->graph);\n> \t}\n> \tif (opt->mime_boundary && maybe_multipart) {\n> -\t\tstatic struct strbuf subject_buffer = STRBUF_INIT;\n> \t\tstatic struct strbuf buffer = STRBUF_INIT;\n> \t\tstruct strbuf filename = STRBUF_INIT;\n> \t\t*need_8bit_cte_p = -1; /* NEVER */\n>\n> -\t\tstrbuf_reset(&subject_buffer);\n> \t\tstrbuf_reset(&buffer);\n>\n> -\t\tstrbuf_addf(&subject_buffer,\n> -\t\t\t \"%s\"\n> +\t\tstrbuf_addf(&headers,\n> \t\t\t \"MIME-Version: 1.0\\n\"\n> \t\t\t \"Content-Type: multipart/mixed;\"\n> \t\t\t \" boundary=\\\"%s%s\\\"\\n\"\n> @@ -516,10 +522,8 @@ void log_write_email_headers(struct rev_info *opt,\n> struct commit *commit,\n> \t\t\t \"Content-Type: text/plain; \"\n> \t\t\t \"charset=UTF-8; format=fixed\\n\"\n> \t\t\t \"Content-Transfer-Encoding: 8bit\\n\\n\",\n> -\t\t\t extra_headers ? extra_headers : \"\",\n> \t\t\t mime_boundary_leader, opt->mime_boundary,\n> \t\t\t mime_boundary_leader, opt->mime_boundary);\n> -\t\textra_headers = subject_buffer.buf;\n>\n> \t\tif (opt->numbered_files)\n> \t\t\tstrbuf_addf(&filename, \"%d\", opt->nr);\n> @@ -539,7 +543,7 @@ void log_write_email_headers(struct rev_info *opt,\n> struct commit *commit,\n> \t\topt->diffopt.stat_sep = buffer.buf;\n> \t\tstrbuf_release(&filename);\n> \t}\n> -\t*extra_headers_p = extra_headers;\n> +\t*extra_headers_p = headers.len ? headers.buf : NULL;\n> }\n>\n> static void show_sig_lines(struct rev_info *opt, int status, const char *bol)\n>\n> And then the callers can continue not caring about how or when to free\n> the returned pointer. I think in the long run the cleanups I showed are\n> a nicer place to end up, but I'd just worry that your feature work will\n> be held hostage by my desire to clean. ;)\n\nHah! Definitely don’t worry about that, this has been very helpful.\n\n> If you did it this way (probably as a separate preparatory patch minus\n> the pe_headers comment), then either I could do my cleanups on top, or\n> they could even graduate independently (though obviously there will be a\n> little bit of tricky merging at the end).\n>\n> -Peff\n\nI think your series should take precedence. I’ll put my series on the\nbackburner for a while. There’s no rush with that one. These changes of\nyours will make extending the header logic easier overall.\n\nThen when yours is merged I’ll have an even easier time.\n\nThanks again\n\nKristoffer\n"}]}