threads / patch / 59125

patch, 5 partsformat-patch: Add --{to,cc}-cmd support

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

## tl;dr

14 messages between Jan 19, 2023 and Feb 1, 2023. Diffs are folded; open one to read it.

replies: 13people: 3as markdown or json

Zev Weiss· Jan 19, 2023, 22:38 UTC · lore
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
Zev Weiss· Jan 19, 2023, 22:38 UTC · re: Zev Weiss · lore

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

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(-)
Show changes to 4 files +25 −8

builtin/log.c, log-tree.c, log-tree.h, revision.h

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
Junio C Hamano· Jan 20, 2023, 00:33 UTC · re: Zev Weiss · lore

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

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.
Show 5 quoted lines
> @@ -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.
Show 10 quoted lines
> @@ -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.

Calvin Wan· Feb 1, 2023, 23:52 UTC · re: Zev Weiss · lore

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

Show 34 quoted lines
>  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?

Zev Weiss· Jan 19, 2023, 22:38 UTC · re: Zev Weiss · lore

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

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(-)
Show changes to 3 files +19 −21

builtin/log.c, log-tree.c, log-tree.h

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
Junio C Hamano· Jan 25, 2023, 02:11 UTC · re: Zev Weiss · lore

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

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.
Zev Weiss· Jan 19, 2023, 22:38 UTC · re: Zev Weiss · lore

[PATCH 4/5] pretty: Add name_and_address_only parameter

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(-)
Show changes to 2 files +13 −3

pretty.c, pretty.h

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
Junio C Hamano· Jan 25, 2023, 02:23 UTC · re: Zev Weiss · lore

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

Zev Weiss <zev@bewilderbeest.net> writes:
Show 5 quoted lines
> 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?

Show 14 quoted lines
> 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.

Calvin Wan· Feb 1, 2023, 23:52 UTC · re: Zev Weiss · lore

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

Zev Weiss <zev@bewilderbeest.net> writes:
Show 5 quoted lines
> 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.

Zev Weiss· Jan 19, 2023, 22:38 UTC · re: Zev Weiss · lore

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

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(+)
Show changes to t/t4014-format-patch.sh +7 −0
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
Calvin Wan· Feb 1, 2023, 23:52 UTC · re: Zev Weiss · lore

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

Show 8 quoted lines
> +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.

Zev Weiss· Jan 19, 2023, 22:38 UTC · re: Zev Weiss · lore

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

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(-)
Show changes to 4 files +208 −2

builtin/log.c, log-tree.c, revision.h, t/t4014-format-patch.sh

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
Calvin Wan· Feb 1, 2023, 23:52 UTC · re: Zev Weiss · lore

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

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!

Calvin Wan· Feb 1, 2023, 23:52 UTC · re: Zev Weiss · lore

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

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!

Show 6 quoted lines
>  - 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.

← back to recent threads