[PATCH 0/5] format-patch: Add --{to,cc}-cmd support

[PATCH 0/5] format-patch: Add --{to,cc}-cmd support

[Zev Weiss]

Hello,

On more than one occasion I've found myself wishing that git's
format-patch command supported the --to-cmd/--cc-cmd flags that
send-email does; this patch series is my attempt at implementing it.

This is my first foray into the git codebase, so odds are good I've
bungled something or missed the existence of some useful API, but it
passes the existing test suite (and the tests added for the new
feature), and I'm giving it its inaugural dogfooding on this very
series with `contrib/contacts/git-contacts`.

The first patch adds a test I found myself wanting when I realized I
had accidentally broken something that wasn't covered by the existing
tests.  The next two do a bit of refactoring and rearrangement, the
fourth adds a bit of library infrastructure to pretty.c, and the final
patch implements the feature itself.

There are a few points where I wasn't sure if the approach I chose was
really the best:

 - The 'name_and_address_only' pretty_print_context flag added in the
   fourth patch is kind of clunky, but I didn't see any other obvious
   great way to reuse the bits of pp_user_info() that I wanted.

 - Fork/exec-ing another internal format-patch command to generate the
   temporary patch file to run the commands on -- my attempts to
   recurse back into show_log() to do it more directly didn't go well.

 - As currently written any commands specified are only run on the
   patches themselves, not on the cover letter.  The slight
   inconsistency with git-send-email is a bit unfortunate, but I
   figure it's probably not often all that useful in that context
   anyway (and the implementation looked like it would be just
   non-trivial enough that laziness may have played a role as well).

Suggestions on better alternatives there (or elsewhere) are of course
welcome.


Thanks,
Zev Weiss

Zev Weiss (5):
  t4014: Add test checking cover-letter To header
  log: Refactor duplicated code to headerize recipient lists
  log: Push to/cc handling down into show_log()
  pretty: Add name_and_address_only parameter
  format-patch: Add support for --{to,cc}-cmd flags

 builtin/log.c           |  31 +++---
 log-tree.c              | 208 +++++++++++++++++++++++++++++++++++++++-
 log-tree.h              |   1 +
 pretty.c                |  15 ++-
 pretty.h                |   1 +
 revision.h              |   4 +
 t/t4014-format-patch.sh |  26 +++++
 7 files changed, 262 insertions(+), 24 deletions(-)

-- 
2.39.1.236.ga8a28b9eace8

[PATCH 1/5] t4014: Add test checking cover-letter To header

[Zev Weiss]

I at first inadvertently broke this in the process of adding
--{to,cc}-cmd support to format-patch and didn't immediately notice
because there wasn't a test for it, so let's add one now.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 t/t4014-format-patch.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 012f155e10ae..ba5fd0efe2ae 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -2368,4 +2368,11 @@ test_expect_success 'interdiff: solo-patch' '
 	test_cmp expect actual
 '
 
+test_expect_success 'cover-letter gets To: header' '
+	rm -fr patches &&
+	git config --unset-all format.to &&
+	git format-patch -o patches --cover-letter --to "R E Cipient <rcipient@example.com>" main..side >list &&
+	grep "^To: R E Cipient <rcipient@example.com>\$" patches/0000-cover-letter.patch
+'
+
 test_done
-- 
2.39.1.236.ga8a28b9eace8

[PATCH 2/5] log: Refactor duplicated code to headerize recipient lists

[Zev Weiss]

The To and Cc headers are handled identically (the only difference is
the header name tag), so we might as well reuse the same code for both
instead of duplicating it.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 builtin/log.c | 23 ++---------------------
 log-tree.c    | 15 +++++++++++++++
 log-tree.h    |  2 ++
 3 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 057e299c245c..ad9d7ebc6d73 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -2028,27 +2028,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		strbuf_addch(&buf, '\n');
 	}
 
-	if (extra_to.nr)
-		strbuf_addstr(&buf, "To: ");
-	for (i = 0; i < extra_to.nr; i++) {
-		if (i)
-			strbuf_addstr(&buf, "    ");
-		strbuf_addstr(&buf, extra_to.items[i].string);
-		if (i + 1 < extra_to.nr)
-			strbuf_addch(&buf, ',');
-		strbuf_addch(&buf, '\n');
-	}
-
-	if (extra_cc.nr)
-		strbuf_addstr(&buf, "Cc: ");
-	for (i = 0; i < extra_cc.nr; i++) {
-		if (i)
-			strbuf_addstr(&buf, "    ");
-		strbuf_addstr(&buf, extra_cc.items[i].string);
-		if (i + 1 < extra_cc.nr)
-			strbuf_addch(&buf, ',');
-		strbuf_addch(&buf, '\n');
-	}
+	recipients_to_header_buf("To", &buf, &extra_to);
+	recipients_to_header_buf("Cc", &buf, &extra_cc);
 
 	rev.extra_headers = to_free = strbuf_detach(&buf, NULL);
 
diff --git a/log-tree.c b/log-tree.c
index 1dd5fcbf7be4..0e8863fe545a 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -426,6 +426,21 @@ void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)
 	}
 }
 
+void recipients_to_header_buf(const char *hdr, struct strbuf *buf,
+			      const struct string_list *recipients)
+{
+	for (int i = 0; i < recipients->nr; i++) {
+		if (!i)
+			strbuf_addf(buf, "%s: ", hdr);
+		else
+			strbuf_addstr(buf, "    ");
+		strbuf_addstr(buf, recipients->items[i].string);
+		if (i + 1 < recipients->nr)
+			strbuf_addch(buf, ',');
+		strbuf_addch(buf, '\n');
+	}
+}
+
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			     const char **extra_headers_p,
 			     int *need_8bit_cte_p,
diff --git a/log-tree.h b/log-tree.h
index e7e4641cf83c..227edc564121 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -25,6 +25,8 @@ void format_decorations_extended(struct strbuf *sb, const struct commit *commit,
 #define format_decorations(strbuf, commit, color) \
 			     format_decorations_extended((strbuf), (commit), (color), " (", ", ", ")")
 void show_decorations(struct rev_info *opt, struct commit *commit);
+void recipients_to_header_buf(const char *hdr, struct strbuf *buf,
+			      const struct string_list *recipients);
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			     const char **extra_headers_p,
 			     int *need_8bit_cte_p,
-- 
2.39.1.236.ga8a28b9eace8

[PATCH 3/5] log: Push to/cc handling down into show_log()

[Zev Weiss]

This rearrangement is a measure to facilitate adding --to-cmd/--cc-cmd
support to format-patch.  The command will need to be run separately
for each commit, so the lists of individual recipients specified via
--to and --cc (and potentially --add-header) need to be available at
the per-commit level instead of just the combined headers in a single
string as has been the case until now.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 builtin/log.c |  6 +++---
 log-tree.c    | 22 +++++++++++++++++++---
 log-tree.h    |  3 +--
 revision.h    |  2 ++
 4 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index ad9d7ebc6d73..c0c7b8544d73 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1326,6 +1326,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
 	pp.rev = rev;
 	pp.print_email_subject = 1;
 	pp_user_info(&pp, NULL, &sb, committer, encoding);
+	format_recipients(rev, &sb);
 	prepare_cover_text(&pp, branch_name, &sb, encoding, need_8bit_cte);
 	fprintf(rev->diffopt.file, "%s\n", sb.buf);
 
@@ -2028,9 +2029,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		strbuf_addch(&buf, '\n');
 	}
 
-	recipients_to_header_buf("To", &buf, &extra_to);
-	recipients_to_header_buf("Cc", &buf, &extra_cc);
-
+	rev.to_recipients = &extra_to;
+	rev.cc_recipients = &extra_cc;
 	rev.extra_headers = to_free = strbuf_detach(&buf, NULL);
 
 	if (from) {
diff --git a/log-tree.c b/log-tree.c
index 0e8863fe545a..7aa2841dd803 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -426,8 +426,8 @@ void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)
 	}
 }
 
-void recipients_to_header_buf(const char *hdr, struct strbuf *buf,
-			      const struct string_list *recipients)
+static void recipients_to_header_buf(const char *hdr, struct strbuf *buf,
+				     const struct string_list *recipients)
 {
 	for (int i = 0; i < recipients->nr; i++) {
 		if (!i)
@@ -441,6 +441,14 @@ void recipients_to_header_buf(const char *hdr, struct strbuf *buf,
 	}
 }
 
+void format_recipients(struct rev_info *rev, struct strbuf *sb)
+{
+	if (rev->to_recipients)
+		recipients_to_header_buf("To", sb, rev->to_recipients);
+	if (rev->cc_recipients)
+		recipients_to_header_buf("Cc", sb, rev->cc_recipients);
+}
+
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			     const char **extra_headers_p,
 			     int *need_8bit_cte_p,
@@ -647,10 +655,12 @@ static void next_commentary_block(struct rev_info *opt, struct strbuf *sb)
 void show_log(struct rev_info *opt)
 {
 	struct strbuf msgbuf = STRBUF_INIT;
+	struct strbuf hdrbuf = STRBUF_INIT;
 	struct log_info *log = opt->loginfo;
 	struct commit *commit = log->commit, *parent = log->parent;
 	int abbrev_commit = opt->abbrev_commit ? opt->abbrev : the_hash_algo->hexsz;
 	const char *extra_headers = opt->extra_headers;
+	char *to_free;
 	struct pretty_print_context ctx = {0};
 
 	opt->loginfo = NULL;
@@ -770,6 +780,11 @@ void show_log(struct rev_info *opt)
 		ctx.notes_message = strbuf_detach(&notebuf, NULL);
 	}
 
+	format_recipients(opt, &hdrbuf);
+
+	if (extra_headers)
+		strbuf_addstr(&hdrbuf, extra_headers);
+
 	/*
 	 * And then the pretty-printed message itself
 	 */
@@ -779,7 +794,7 @@ void show_log(struct rev_info *opt)
 	ctx.date_mode = opt->date_mode;
 	ctx.date_mode_explicit = opt->date_mode_explicit;
 	ctx.abbrev = opt->diffopt.abbrev;
-	ctx.after_subject = extra_headers;
+	ctx.after_subject = to_free = strbuf_detach(&hdrbuf, NULL);
 	ctx.preserve_subject = opt->preserve_subject;
 	ctx.encode_email_headers = opt->encode_email_headers;
 	ctx.reflog_info = opt->reflog_info;
@@ -828,6 +843,7 @@ void show_log(struct rev_info *opt)
 
 	strbuf_release(&msgbuf);
 	free(ctx.notes_message);
+	free(to_free);
 
 	if (cmit_fmt_is_mail(ctx.fmt) && opt->idiff_oid1) {
 		struct diff_queue_struct dq;
diff --git a/log-tree.h b/log-tree.h
index 227edc564121..ace50dad6c83 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -25,8 +25,7 @@ void format_decorations_extended(struct strbuf *sb, const struct commit *commit,
 #define format_decorations(strbuf, commit, color) \
 			     format_decorations_extended((strbuf), (commit), (color), " (", ", ", ")")
 void show_decorations(struct rev_info *opt, struct commit *commit);
-void recipients_to_header_buf(const char *hdr, struct strbuf *buf,
-			      const struct string_list *recipients);
+void format_recipients(struct rev_info *rev, struct strbuf *sb);
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			     const char **extra_headers_p,
 			     int *need_8bit_cte_p,
diff --git a/revision.h b/revision.h
index 30febad09a1e..330d351b2e4c 100644
--- a/revision.h
+++ b/revision.h
@@ -283,6 +283,8 @@ struct rev_info {
 	struct ident_split from_ident;
 	struct string_list *ref_message_ids;
 	int		add_signoff;
+	struct string_list *to_recipients;
+	struct string_list *cc_recipients;
 	const char	*extra_headers;
 	const char	*log_reencode;
 	const char	*subject_prefix;
-- 
2.39.1.236.ga8a28b9eace8

[PATCH 4/5] pretty: Add name_and_address_only parameter

[Zev Weiss]

This is meant to be used with pp_user_info() when using it to format
email recipients generated by --to-cmd/--cc-cmd.  When set it omits
the leading 'From: ', trailing linefeed, and the date suffix, and
additionally will return the input string unmodified if
split_ident_line() can't parse it (e.g. for a bare email address).

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 pretty.c | 15 ++++++++++++---
 pretty.h |  1 +
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/pretty.c b/pretty.c
index 1e1e21878c83..e6798fadc107 100644
--- a/pretty.c
+++ b/pretty.c
@@ -509,8 +509,11 @@ void pp_user_info(struct pretty_print_context *pp,
 		return;
 
 	line_end = strchrnul(line, '\n');
-	if (split_ident_line(&ident, line, line_end - line))
+	if (split_ident_line(&ident, line, line_end - line)) {
+		if (pp->name_and_address_only)
+			strbuf_addstr(sb, line);
 		return;
+	}
 
 	mailbuf = ident.mail_begin;
 	maillen = ident.mail_end - ident.mail_begin;
@@ -538,7 +541,8 @@ void pp_user_info(struct pretty_print_context *pp,
 			namelen = pp->from_ident->name_end - namebuf;
 		}
 
-		strbuf_addstr(sb, "From: ");
+		if (!pp->name_and_address_only)
+			strbuf_addstr(sb, "From: ");
 		if (pp->encode_email_headers &&
 		    needs_rfc2047_encoding(namebuf, namelen)) {
 			add_rfc2047(sb, namebuf, namelen,
@@ -558,7 +562,9 @@ void pp_user_info(struct pretty_print_context *pp,
 		if (max_length <
 		    last_line_length(sb) + strlen(" <") + maillen + strlen(">"))
 			strbuf_addch(sb, '\n');
-		strbuf_addf(sb, " <%.*s>\n", (int)maillen, mailbuf);
+		strbuf_addf(sb, " <%.*s>", (int)maillen, mailbuf);
+		if (!pp->name_and_address_only)
+			strbuf_addch(sb, '\n');
 	} else {
 		struct strbuf id = STRBUF_INIT;
 		enum grep_header_field field = GREP_HEADER_FIELD_MAX;
@@ -582,6 +588,9 @@ void pp_user_info(struct pretty_print_context *pp,
 		strbuf_release(&id);
 	}
 
+	if (pp->name_and_address_only)
+		return;
+
 	switch (pp->fmt) {
 	case CMIT_FMT_MEDIUM:
 		strbuf_addf(sb, "Date:   %s\n",
diff --git a/pretty.h b/pretty.h
index f34e24c53a49..306666e99294 100644
--- a/pretty.h
+++ b/pretty.h
@@ -39,6 +39,7 @@ struct pretty_print_context {
 	int preserve_subject;
 	struct date_mode date_mode;
 	unsigned date_mode_explicit:1;
+	unsigned name_and_address_only:1;
 	int print_email_subject;
 	int expand_tabs_in_log;
 	int need_8bit_cte;
-- 
2.39.1.236.ga8a28b9eace8

[PATCH 5/5] format-patch: Add support for --{to,cc}-cmd flags

[Zev Weiss]

Having these flags available for format-patch instead of only on
send-email makes it much easier to use an automated command to do the
bulk of the recipient-selection work but still manually adjust it if
desired.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 builtin/log.c           |  10 +++
 log-tree.c              | 179 +++++++++++++++++++++++++++++++++++++++-
 revision.h              |   2 +
 t/t4014-format-patch.sh |  19 +++++
 4 files changed, 208 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index c0c7b8544d73..da3edb2a8299 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1877,6 +1877,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	struct strbuf rdiff2 = STRBUF_INIT;
 	struct strbuf rdiff_title = STRBUF_INIT;
 	int creation_factor = -1;
+	char *to_cmd_arg = NULL;
+	char *cc_cmd_arg = NULL;
 
 	const struct option builtin_format_patch_options[] = {
 		OPT_CALLBACK_F('n', "numbered", &numbered, NULL,
@@ -1929,6 +1931,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    N_("add email header"), header_callback),
 		OPT_CALLBACK(0, "to", NULL, N_("email"), N_("add To: header"), to_callback),
 		OPT_CALLBACK(0, "cc", NULL, N_("email"), N_("add Cc: header"), cc_callback),
+		OPT_STRING(0, "to-cmd", &to_cmd_arg, N_("command"),
+		           N_("command to generate To: addresses for a patch")),
+		OPT_STRING(0, "cc-cmd", &cc_cmd_arg, N_("command"),
+		           N_("command to generate Cc: addresses for a patch")),
 		OPT_CALLBACK_F(0, "from", &from, N_("ident"),
 			    N_("set From address to <ident> (or committer ident if absent)"),
 			    PARSE_OPT_OPTARG, from_callback),
@@ -2031,6 +2037,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 
 	rev.to_recipients = &extra_to;
 	rev.cc_recipients = &extra_cc;
+
+	rev.to_cmd = to_cmd_arg;
+	rev.cc_cmd = cc_cmd_arg;
+
 	rev.extra_headers = to_free = strbuf_detach(&buf, NULL);
 
 	if (from) {
diff --git a/log-tree.c b/log-tree.c
index 7aa2841dd803..6fdc3a3ffb7f 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -20,6 +20,7 @@
 #include "help.h"
 #include "range-diff.h"
 #include "strmap.h"
+#include "run-command.h"
 
 static struct decoration name_decoration = { "object names" };
 static int decoration_loaded;
@@ -652,11 +653,175 @@ static void next_commentary_block(struct rev_info *opt, struct strbuf *sb)
 	opt->shown_dashes = 1;
 }
 
+/*
+ * Aims to mirror git-send-email.perl's function of the same name, returning a
+ * malloc()ed string that the caller must free().
+ */
+static char *sanitize_address(const struct rev_info *opt, const char *entry)
+{
+	int escaped = 0;
+	const char *inp;
+	char *tmp, *outp;
+	struct strbuf addr = STRBUF_INIT;
+	struct pretty_print_context ctx = {0};
+
+	/* Skip over any leading whitespace */
+	while (isspace(*entry))
+		entry++;
+
+	outp = tmp = xmalloc(strlen(entry) + 1);
+
+	/* Remove non-escaped quotes */
+	for (inp = entry; *inp; inp++) {
+		if (escaped) {
+			escaped = 0;
+		} else {
+			switch (*inp) {
+			case '\\':
+				escaped = 1;
+				/* fallthrough */
+			case '"':
+				continue;
+			}
+		}
+		*outp++ = *inp;
+	}
+	*outp = '\0';
+
+	ctx.fmt = opt->commit_format;
+	ctx.mailmap = opt->mailmap;
+	ctx.encode_email_headers = opt->encode_email_headers;
+	ctx.output_encoding = get_log_output_encoding();
+	ctx.name_and_address_only = 1;
+
+	pp_user_info(&ctx, NULL, &addr, tmp, get_log_output_encoding());
+
+	free(tmp);
+
+	return strbuf_detach(&addr, NULL);
+}
+
+/*
+ * Given 'text' as the output of a --to-cmd or --cc-cmd command, add each
+ * entry to 'list'.
+ */
+static void ingest_recipients_to_list(const char *text, const struct rev_info *opt,
+				      struct string_list *list)
+{
+	struct string_list_item *item;
+	struct string_list lines = STRING_LIST_INIT_DUP;
+
+	string_list_split(&lines, text, '\n', -1);
+
+	for_each_string_list_item(item, &lines) {
+		char *addr = sanitize_address(opt, item->string);
+		if (*addr)
+			string_list_append_nodup(list, addr);
+		else
+			free(addr);
+	}
+
+	string_list_clear(&lines, 0);
+}
+
+/*
+ * Generate a temporary patch file for the given commit, returning its path as
+ * a malloc()ed string the caller must free (and should unlink when finished
+ * with it).
+ */
+static char *generate_temp_patch(struct commit *commit)
+{
+	char path[PATH_MAX];
+	char *diff_output_arg;
+	struct strbuf diff_output_arg_buf = STRBUF_INIT;
+	struct child_process diffproc = CHILD_PROCESS_INIT;
+
+	xsnprintf(path, sizeof(path), ".git-temp-diff.XXXXXX");
+	close(xmkstemp(path));
+
+	strbuf_addf(&diff_output_arg_buf, "--output=%s", path);
+	diff_output_arg = strbuf_detach(&diff_output_arg_buf, NULL);
+
+	diffproc.git_cmd = 1;
+	strvec_push(&diffproc.args, "format-patch");
+	strvec_push(&diffproc.args, "-1");
+	strvec_push(&diffproc.args, diff_output_arg);
+	strvec_push(&diffproc.args, oid_to_hex(&commit->object.oid));
+
+	if (run_command(&diffproc))
+		die(_("Error generating temporary diff"));
+
+	free(diff_output_arg);
+
+	return xstrdup(path);
+}
+
+/*
+ * Execute a --to-cmd or --cc-cmd command on a temporary patch file, adding
+ * each recipient produced to 'list'.
+ */
+static void run_recipients_command(const char *cmd, const char *tmpdiff,
+				   const struct rev_info *opt,
+				   struct string_list *list)
+{
+	char *full_cmd;
+	char *cmd_output;
+	struct strbuf cmd_output_buf = STRBUF_INIT;
+	struct strbuf cmd_buf = STRBUF_INIT;
+	struct child_process cmdproc = CHILD_PROCESS_INIT;
+
+	strbuf_addf(&cmd_buf, "%s %s", cmd, tmpdiff);
+	full_cmd = strbuf_detach(&cmd_buf, NULL);
+
+	cmdproc.use_shell = 1;
+	strvec_push(&cmdproc.args, full_cmd);
+	if (capture_command(&cmdproc, &cmd_output_buf, 1024))
+		die(_("Error generating recipients list: command failed"));
+
+	cmd_output = strbuf_detach(&cmd_output_buf, NULL);
+	ingest_recipients_to_list(cmd_output, opt, list);
+
+	free(cmd_output);
+	free(full_cmd);
+}
+
+/*
+ * Generate a To or Cc header into 'buf', where 'header' is "To" or "Cc",
+ * 'fixed' is the list of fixed recipients (e.g. those specified by --to or
+ * --cc, optional), 'cmd' is the (optional) --to-cmd or --cc-cmd argument to
+ * generate recipients, and 'tmpdiff' is the path of a temp file containing a
+ * patch file to run 'cmd' on (mandatory if 'cmd' is non-NULL).
+ */
+static void headerize_recipients(const char *header, const char *tmpdiff,
+				 const struct rev_info *opt,
+				 const struct string_list *fixed,
+				 const char *cmd, struct strbuf *buf)
+{
+	struct string_list_item *item;
+	struct string_list recipients = STRING_LIST_INIT_DUP;
+
+	if (fixed) {
+		for_each_string_list_item(item, fixed)
+			string_list_append(&recipients, item->string);
+	}
+
+	if (cmd)
+		run_recipients_command(cmd, tmpdiff, opt, &recipients);
+
+	string_list_sort(&recipients);
+	string_list_remove_duplicates(&recipients, 0);
+
+	recipients_to_header_buf(header, buf, &recipients);
+
+	string_list_clear(&recipients, 0);
+}
+
 void show_log(struct rev_info *opt)
 {
 	struct strbuf msgbuf = STRBUF_INIT;
 	struct strbuf hdrbuf = STRBUF_INIT;
 	struct log_info *log = opt->loginfo;
+	char *tmpdiff = NULL;
 	struct commit *commit = log->commit, *parent = log->parent;
 	int abbrev_commit = opt->abbrev_commit ? opt->abbrev : the_hash_algo->hexsz;
 	const char *extra_headers = opt->extra_headers;
@@ -715,6 +880,18 @@ void show_log(struct rev_info *opt)
 	 */
 	graph_show_commit(opt->graph);
 
+	/* Generate dynamic To/Cc lists as needed */
+	if (opt->to_cmd || opt->cc_cmd)
+		tmpdiff = generate_temp_patch(commit);
+
+	headerize_recipients("To", tmpdiff, opt, opt->to_recipients, opt->to_cmd, &hdrbuf);
+	headerize_recipients("Cc", tmpdiff, opt, opt->cc_recipients, opt->cc_cmd, &hdrbuf);
+
+	if (tmpdiff) {
+		unlink_or_warn(tmpdiff);
+		free(tmpdiff);
+	}
+
 	/*
 	 * Print header line of header..
 	 */
@@ -780,8 +957,6 @@ void show_log(struct rev_info *opt)
 		ctx.notes_message = strbuf_detach(&notebuf, NULL);
 	}
 
-	format_recipients(opt, &hdrbuf);
-
 	if (extra_headers)
 		strbuf_addstr(&hdrbuf, extra_headers);
 
diff --git a/revision.h b/revision.h
index 330d351b2e4c..9611309ae496 100644
--- a/revision.h
+++ b/revision.h
@@ -285,6 +285,8 @@ struct rev_info {
 	int		add_signoff;
 	struct string_list *to_recipients;
 	struct string_list *cc_recipients;
+	const char	*to_cmd;
+	const char	*cc_cmd;
 	const char	*extra_headers;
 	const char	*log_reencode;
 	const char	*subject_prefix;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index ba5fd0efe2ae..2387bda4f272 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -228,6 +228,25 @@ test_expect_failure 'configuration To: header (rfc2047)' '
 	grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\$" hdrs9
 '
 
+test_expect_success 'dynamic To: header (ascii)' '
+	git config --unset-all format.to &&
+	git format-patch --to-cmd="echo \"R E Cipient <rcipient@example.com>\" #" --stdout main..side >patch10 &&
+	sed -e "/^\$/q" patch10 >hdrs10 &&
+	grep "^To: R E Cipient <rcipient@example.com>\$" hdrs10
+'
+
+test_expect_success 'dynamic To: header (rfc822)' '
+	git format-patch --to-cmd="echo \"R. E. Cipient <rcipient@example.com>\" #" --stdout main..side >patch10 &&
+	sed -e "/^\$/q" patch10 >hdrs10 &&
+	grep "^To: \"R. E. Cipient\" <rcipient@example.com>\$" hdrs10
+'
+
+test_expect_success 'dynamic To: header (rfc2047)' '
+	git format-patch --to-cmd="echo \"R Ä Cipient <rcipient@example.com>\" #" --stdout main..side >patch10 &&
+	sed -e "/^\$/q" patch10 >hdrs10 &&
+	grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\$" hdrs10
+'
+
 # check_patch <patch>: Verify that <patch> looks like a half-sane
 # patch email to avoid a false positive with !grep
 check_patch () {
-- 
2.39.1.236.ga8a28b9eace8

Re: [PATCH 3/5] log: Push to/cc handling down into show_log()

[Junio C Hamano]

Zev Weiss <zev@bewilderbeest.net> writes:

> Subject: Re: [PATCH 3/5] log: Push to/cc handling down into show_log()

s/Push/push/
cf. Documentation/SubmittingPatches[[summary-section]]

This is common to all these patches.

> @@ -1326,6 +1326,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
>  	pp.rev = rev;
>  	pp.print_email_subject = 1;
>  	pp_user_info(&pp, NULL, &sb, committer, encoding);
> +	format_recipients(rev, &sb);

This is where the two new members in the rev structure is used.

> @@ -2028,9 +2029,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		strbuf_addch(&buf, '\n');
>  	}
>  
> -	recipients_to_header_buf("To", &buf, &extra_to);
> -	recipients_to_header_buf("Cc", &buf, &extra_cc);
> -
> +	rev.to_recipients = &extra_to;
> +	rev.cc_recipients = &extra_cc;
>  	rev.extra_headers = to_free = strbuf_detach(&buf, NULL);

And these two members point at borrowed memory.  extra_to and
extra_cc is freed after everything is done, near the end of the
cmd_format_patch() function.  We don't leak any extra memory by this
change, which is good.

Re: [PATCH 2/5] log: Refactor duplicated code to headerize recipient lists

[Junio C Hamano]

Zev Weiss <zev@bewilderbeest.net> writes:

> Subject: Re: [PATCH 2/5] log: Refactor duplicated code to headerize recipient lists

Style: "log: Refactor" -> "log: refactor"

cf. Documentation/SubmittingPatches[[summary-section]]

> The To and Cc headers are handled identically (the only difference is
> the header name tag), so we might as well reuse the same code for both
> instead of duplicating it.

Makes tons of sense.  But seeing this one ...

> +	recipients_to_header_buf("To", &buf, &extra_to);
> +	recipients_to_header_buf("Cc", &buf, &extra_cc);

... the parameters to the function probably should be ...

> +void recipients_to_header_buf(const char *hdr, struct strbuf *buf,
> +			      const struct string_list *recipients);

... in this order, instead:

    format_recipients(&buf, "To", &extra_to);

That is, "To" and &extra_to are much closely related to each other
than they are to &buf in the sense that they are both input to the
helper function to work on, while &buf is an output buffer, and we
tend to place closer things together, next to each other.

Other than that, removal of repetition is very good.

Thanks.

Re: [PATCH 4/5] pretty: Add name_and_address_only parameter

[Junio C Hamano]

Zev Weiss <zev@bewilderbeest.net> writes:

> This is meant to be used with pp_user_info() when using it to format
> email recipients generated by --to-cmd/--cc-cmd.  When set it omits
> the leading 'From: ', trailing linefeed, and the date suffix, and
> additionally will return the input string unmodified if
> split_ident_line() can't parse it (e.g. for a bare email address).

It is somewhat disturbing to see that this only takes effect when
cmit_fmt_is_mail(pp->fmt) and completely ignored otherwise.  Seeing
pp->fmt and pp->name_and_address_only sitting next to each other, it
looks like a layering error.

I wonder if you instead want a new value for pp->fmt that
cmit_fmt_is_mail() considers an e-mail format but is different from
what we used for usual e-mail format?

> diff --git a/pretty.c b/pretty.c
> index 1e1e21878c83..e6798fadc107 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -509,8 +509,11 @@ void pp_user_info(struct pretty_print_context *pp,
>  		return;
>  
>  	line_end = strchrnul(line, '\n');
> -	if (split_ident_line(&ident, line, line_end - line))
> +	if (split_ident_line(&ident, line, line_end - line)) {
> +		if (pp->name_and_address_only)
> +			strbuf_addstr(sb, line);
>  		return;
> +	}

This error handling is also disturbing.  What makes it correct to
parrot the original input that does not parse correctly as an ident
line to the output, only when name_and_address_only bit is on?  It
does not make any sense to do so when cmit_fmt_is_mail(pp->fmt) is
false, especially.

Re: [PATCH 0/5] format-patch: Add --{to,cc}-cmd support

[Calvin Wan]

Hi Zev! Thanks for the patch series. I agree that these flags are a very
good idea to add. I appreciate the well written cover letter -- it does
well describing the need for this series and calling out design
questions. I'll leave the rest of my comments in the particular patches,
looking forward to the reroll!

>  - As currently written any commands specified are only run on the
>    patches themselves, not on the cover letter.  The slight
>    inconsistency with git-send-email is a bit unfortunate, but I
>    figure it's probably not often all that useful in that context
>    anyway (and the implementation looked like it would be just
>    non-trivial enough that laziness may have played a role as well).

It is also inconsistent with git-format-patch --to and --cc. I strongly
believe that your flags should also work with the cover letter, since
users should expect equivalency across similar flags. git-send-email has
config options, sendemail.tocmd and sendemail.cccmd, that I also believe
should be implemented.

Re: [PATCH 1/5] t4014: Add test checking cover-letter To header

[Calvin Wan]

> +test_expect_success 'cover-letter gets To: header' '
> +	rm -fr patches &&
> +	git config --unset-all format.to &&
> +	git format-patch -o patches --cover-letter --to "R E Cipient <rcipient@example.com>" main..side >list &&
> +	grep "^To: R E Cipient <rcipient@example.com>\$" patches/0000-cover-letter.patch
> +'
> +
>  test_done

'>list' is unnecessary. The test can also be moved closer to other
cover-letter or header tests.

Re: [PATCH 3/5] log: Push to/cc handling down into show_log()

[Calvin Wan]

>  void log_write_email_headers(struct rev_info *opt, struct commit *commit,
>  			     const char **extra_headers_p,
>  			     int *need_8bit_cte_p,
> @@ -647,10 +655,12 @@ static void next_commentary_block(struct rev_info *opt, struct strbuf *sb)
>  void show_log(struct rev_info *opt)
>  {
>  	struct strbuf msgbuf = STRBUF_INIT;
> +	struct strbuf hdrbuf = STRBUF_INIT;
>  	struct log_info *log = opt->loginfo;
>  	struct commit *commit = log->commit, *parent = log->parent;
>  	int abbrev_commit = opt->abbrev_commit ? opt->abbrev : the_hash_algo->hexsz;
>  	const char *extra_headers = opt->extra_headers;
> +	char *to_free;
>  	struct pretty_print_context ctx = {0};
>  
>  	opt->loginfo = NULL;
> @@ -770,6 +780,11 @@ void show_log(struct rev_info *opt)
>  		ctx.notes_message = strbuf_detach(&notebuf, NULL);
>  	}
>  
> +	format_recipients(opt, &hdrbuf);
> +
> +	if (extra_headers)
> +		strbuf_addstr(&hdrbuf, extra_headers);
> +
>  	/*
>  	 * And then the pretty-printed message itself
>  	 */
> @@ -779,7 +794,7 @@ void show_log(struct rev_info *opt)
>  	ctx.date_mode = opt->date_mode;
>  	ctx.date_mode_explicit = opt->date_mode_explicit;
>  	ctx.abbrev = opt->diffopt.abbrev;
> -	ctx.after_subject = extra_headers;
> +	ctx.after_subject = to_free = strbuf_detach(&hdrbuf, NULL);

Can you explain in the commit message why hdrbuf, which contains the
output from format_recipients and extra_headers, is still the same
functionality as before?

> -void recipients_to_header_buf(const char *hdr, struct strbuf *buf,
> -			      const struct string_list *recipients);
> +void format_recipients(struct rev_info *rev, struct strbuf *sb);

What do you think about renaming this function to strbuf_add_recipients
and swapping the parameters? 

Re: [PATCH 4/5] pretty: Add name_and_address_only parameter

[Calvin Wan]

Zev Weiss <zev@bewilderbeest.net> writes:
> This is meant to be used with pp_user_info() when using it to format
> email recipients generated by --to-cmd/--cc-cmd.  When set it omits
> the leading 'From: ', trailing linefeed, and the date suffix, and
> additionally will return the input string unmodified if
> split_ident_line() can't parse it (e.g. for a bare email address).

I think the ideal solution, instead of choosing this hacky approach, is
to refactor out the common functionality you need from pp_user_info()
and to call that instead in your next patch.

Re: [PATCH 5/5] format-patch: Add support for --{to,cc}-cmd flags

[Calvin Wan]

Zev Weiss <zev@bewilderbeest.net> writes:
> Having these flags available for format-patch instead of only on
> send-email makes it much easier to use an automated command to do the
> bulk of the recipient-selection work but still manually adjust it if
> desired.

Commit message could use some more explanation to make blame diving
easier later on. In particular, why is creating temp files necessary? 

Missing update to Documentation/git-format-patch.txt

This patch also needs many more tests
 - tests for --cc-cmd
 - tests for commands that run on the patches
 - similar tests in t9001-send-email.sh

Will wait for reroll to dive deeper into the implementation here, but
overall a well-intentioned v1!