Re: [PATCH] pretty: add extra headers and MIME boundary directly

From: René Scharfe <hidden>
Date: 2017-03-26 13:45:52
Subsystem: the rest · Maintainer: Linus Torvalds

Am 25.03.2017 um 22:11 schrieb Jeff King:

The most correct way is that the caller of log_write_email_headers() and
diff_flush() should have a function-local strbuf which holds the data,
gets passed to diff_flush() as some kind opaque context, and then is
freed afterwards. We don't have such a context, but if we were to abuse
diff_options.stat_sep _temporarily_, that would still be a lot cleaner.
I.e., something like this:

  struct strbuf stat_sep = STRBUF_INIT;

  /* may write into stat_sep, depending on options */
  log_write_email_headers(..., &stat_sep);
  opt->diffopt.stat_sep = stat_sep.buf;

  diff_flush(&opt->diffopt);
  opt->diffopt.stat_sep = NULL;
  strbuf_release(&stat_sep);

But it's a bit tricky because those two hunks happen in separate
functions, which means passing the strbuf around.

You could have a destructor callback, called at the end of diff_flush().

quoted hunk ↗ jump to hunk

Anyway. Here's my attempt at the callback version of stat_sep.

---

diff --git a/diff.c b/diff.c
index a628ac3a9..d061f9e18 100644
--- a/diff.c
+++ b/diff.c

@@ -4819,10 +4819,9 @@ void diff_flush(struct diff_options *options)
 			fprintf(options->file, "%s%c",
 				diff_line_prefix(options),
 				options->line_termination);
-			if (options->stat_sep) {
-				/* attach patch instead of inline */
-				fputs(options->stat_sep, options->file);
-			}
+			if (options->stat_sep)
+				options->stat_sep(options->file,
+						  options->stat_sep_data);
 		}
 
 		for (i = 0; i < q->nr; i++) {

diff --git a/diff.h b/diff.h
index e9ccb38c2..4785f3b23 100644
--- a/diff.h
+++ b/diff.h

@@ -154,9 +154,11 @@ struct diff_options {
 	unsigned ws_error_highlight;
 	const char *prefix;
 	int prefix_length;
-	const char *stat_sep;
 	long xdl_opts;
 
+	void (*stat_sep)(FILE *, void *);
+	void *stat_sep_data;
+
 	int stat_width;
 	int stat_name_width;
 	int stat_graph_width;

diff --git a/log-tree.c b/log-tree.c
index 7049a1778..5cf825c41 100644
--- a/log-tree.c
+++ b/log-tree.c

@@ -348,6 +348,31 @@ void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)
 	}
 }
 
+static void show_mime_attachment(FILE *out, void *data)
+{
+	struct rev_info *opt = data;
+	struct strbuf filename = STRBUF_INIT;
+
+	if (opt->numbered_files)
+		strbuf_addf(&filename, "%d", opt->nr);
+	else
+		fmt_output_commit(&filename, opt->commit_for_mime, opt);
+
+	fprintf(out,
+		"\n--%s%s\n"
+		"Content-Type: text/x-patch;"
+		" name=\"%s\"\n"
+		"Content-Transfer-Encoding: 8bit\n"
+		"Content-Disposition: %s;"
+		" filename=\"%s\"\n\n",
+		mime_boundary_leader, opt->mime_boundary,
+		filename.buf,
+		opt->no_inline ? "attachment" : "inline",
+		filename.buf);
+
+	strbuf_release(&filename);
+}
+
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			     int *need_8bit_cte_p)
 {

@@ -372,27 +397,10 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 		graph_show_oneline(opt->graph);
 	}
 	if (opt->mime_boundary) {
-		static char buffer[1024];
-		struct strbuf filename =  STRBUF_INIT;
 		*need_8bit_cte_p = -1; /* NEVER */
-
-		if (opt->numbered_files)
-			strbuf_addf(&filename, "%d", opt->nr);
-		else
-			fmt_output_commit(&filename, commit, opt);
-		snprintf(buffer, sizeof(buffer) - 1,
-			 "\n--%s%s\n"
-			 "Content-Type: text/x-patch;"
-			 " name=\"%s\"\n"
-			 "Content-Transfer-Encoding: 8bit\n"
-			 "Content-Disposition: %s;"
-			 " filename=\"%s\"\n\n",
-			 mime_boundary_leader, opt->mime_boundary,
-			 filename.buf,
-			 opt->no_inline ? "attachment" : "inline",
-			 filename.buf);
-		opt->diffopt.stat_sep = buffer;
-		strbuf_release(&filename);
+		opt->diffopt.stat_sep = show_mime_attachment;
+		opt->diffopt.stat_sep_data = opt;
+		opt->commit_for_mime = commit;
 	}
 }

diff --git a/revision.h b/revision.h
index 14886ec92..46ca45d96 100644
--- a/revision.h
+++ b/revision.h

@@ -156,6 +156,7 @@ struct rev_info {
 	struct log_info *loginfo;
 	int		nr, total;
 	const char	*mime_boundary;
+	struct commit *commit_for_mime;
 	const char	*patch_suffix;
 	int		numbered_files;
 	int		reroll_count;

Hmm.  I'm a fan of callbacks, but using them can make the code a bit
hard to follow.  And void context pointers add a type safety hazard.
Do we need to be this generic?  How about switching stat_sep to strbuf?
fmt_output_commit() requires an allocation anyway, so why not allocate
stat_sep as well?

---
 diff.c     | 7 ++++---
 diff.h     | 2 +-
 log-tree.c | 4 +---
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/diff.c b/diff.c
index a628ac3a95..a4afa8eba2 100644
--- a/diff.c
+++ b/diff.c

@@ -41,7 +41,7 @@ static int diff_mnemonic_prefix;
 static int diff_no_prefix;
 static int diff_stat_graph_width;
 static int diff_dirstat_permille_default = 30;
-static struct diff_options default_diff_options;
+static struct diff_options default_diff_options = { STRBUF_INIT };
 static long diff_algorithm;
 static unsigned ws_error_highlight_default = WSEH_NEW;

@@ -4819,9 +4819,9 @@ void diff_flush(struct diff_options *options)
 			fprintf(options->file, "%s%c",
 				diff_line_prefix(options),
 				options->line_termination);
-			if (options->stat_sep) {
+			if (options->stat_sep.len) {
 				/* attach patch instead of inline */
-				fputs(options->stat_sep, options->file);
+				strbuf_write(&options->stat_sep, options->file);
 			}
 		}

@@ -4842,6 +4842,7 @@ void diff_flush(struct diff_options *options)
 	DIFF_QUEUE_CLEAR(q);
 	if (options->close_file)
 		fclose(options->file);
+	strbuf_release(&options->stat_sep);
 
 	/*
 	 * Report the content-level differences with HAS_CHANGES;

diff --git a/diff.h b/diff.h
index e9ccb38c26..6a537df1ab 100644
--- a/diff.h
+++ b/diff.h

@@ -116,6 +116,7 @@ enum diff_submodule_format {
 };
 
 struct diff_options {
+	struct strbuf stat_sep;
 	const char *orderfile;
 	const char *pickaxe;
 	const char *single_follow;

@@ -154,7 +155,6 @@ struct diff_options {
 	unsigned ws_error_highlight;
 	const char *prefix;
 	int prefix_length;
-	const char *stat_sep;
 	long xdl_opts;
 
 	int stat_width;

diff --git a/log-tree.c b/log-tree.c
index 7049a17781..cd4f363d9b 100644
--- a/log-tree.c
+++ b/log-tree.c

@@ -372,7 +372,6 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 		graph_show_oneline(opt->graph);
 	}
 	if (opt->mime_boundary) {
-		static char buffer[1024];
 		struct strbuf filename =  STRBUF_INIT;
 		*need_8bit_cte_p = -1; /* NEVER */

@@ -380,7 +379,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			strbuf_addf(&filename, "%d", opt->nr);
 		else
 			fmt_output_commit(&filename, commit, opt);
-		snprintf(buffer, sizeof(buffer) - 1,
+		strbuf_addf(&opt->diffopt.stat_sep,
 			 "\n--%s%s\n"
 			 "Content-Type: text/x-patch;"
 			 " name=\"%s\"\n"

@@ -391,7 +390,6 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			 filename.buf,
 			 opt->no_inline ? "attachment" : "inline",
 			 filename.buf);
-		opt->diffopt.stat_sep = buffer;
 		strbuf_release(&filename);
 	}
 }

-- 
2.12.2

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help