threads / patch / 45510

patchpretty: add extra headers and MIME boundary directly

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

## tl;dr

6 messages between Mar 25, 2017 and Mar 27, 2017. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

René Scharfe· Mar 25, 2017, 12:16 UTC · lore

Use the after_subject member of struct pretty_print_context to pass the extra_headers unchanged, and construct and add the MIME boundary headers directly in pretty.c::pp_title_line() instead of writing both to a static buffer in log-tree.c::log_write_email_headers() first. That's easier, quicker and gets rid of said static buffer.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/log.c |  3 ++-
 log-tree.c    | 26 ++------------------------
 log-tree.h    |  1 -
 pretty.c      | 15 +++++++++++++++
 4 files changed, 19 insertions(+), 26 deletions(-)
Show changes to 4 files +19 −26

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

diff --git a/builtin/log.c b/builtin/log.c
index 281af8c1ec..be564039c1 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -989,7 +989,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	    open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
 		return;
 
-	log_write_email_headers(rev, head, &pp.after_subject, &need_8bit_cte);
+	log_write_email_headers(rev, head, &need_8bit_cte);
+	pp.after_subject = rev->extra_headers;
 
 	for (i = 0; !need_8bit_cte && i < nr; i++) {
 		const char *buf = get_commit_buffer(list[i], NULL);
diff --git a/log-tree.c b/log-tree.c
index 4618dd04ca..7049a17781 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -349,10 +349,8 @@ void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)
 }
 
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
-			     const char **extra_headers_p,
 			     int *need_8bit_cte_p)
 {
-	const char *extra_headers = opt->extra_headers;
 	const char *name = oid_to_hex(opt->zero_commit ?
 				      &null_oid : &commit->object.oid);
 
@@ -374,26 +372,9 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 		graph_show_oneline(opt->graph);
 	}
 	if (opt->mime_boundary) {
-		static char subject_buffer[1024];
 		static char buffer[1024];
 		struct strbuf filename =  STRBUF_INIT;
 		*need_8bit_cte_p = -1; /* NEVER */
-		snprintf(subject_buffer, sizeof(subject_buffer) - 1,
-			 "%s"
-			 "MIME-Version: 1.0\n"
-			 "Content-Type: multipart/mixed;"
-			 " boundary=\"%s%s\"\n"
-			 "\n"
-			 "This is a multi-part message in MIME "
-			 "format.\n"
-			 "--%s%s\n"
-			 "Content-Type: text/plain; "
-			 "charset=UTF-8; format=fixed\n"
-			 "Content-Transfer-Encoding: 8bit\n\n",
-			 extra_headers ? extra_headers : "",
-			 mime_boundary_leader, opt->mime_boundary,
-			 mime_boundary_leader, opt->mime_boundary);
-		extra_headers = subject_buffer;
 
 		if (opt->numbered_files)
 			strbuf_addf(&filename, "%d", opt->nr);
@@ -413,7 +394,6 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 		opt->diffopt.stat_sep = buffer;
 		strbuf_release(&filename);
 	}
-	*extra_headers_p = extra_headers;
 }
 
 static void show_sig_lines(struct rev_info *opt, int status, const char *bol)
@@ -537,7 +517,6 @@ void show_log(struct rev_info *opt)
 	struct log_info *log = opt->loginfo;
 	struct commit *commit = log->commit, *parent = log->parent;
 	int abbrev_commit = opt->abbrev_commit ? opt->abbrev : 40;
-	const char *extra_headers = opt->extra_headers;
 	struct pretty_print_context ctx = {0};
 
 	opt->loginfo = NULL;
@@ -597,8 +576,7 @@ void show_log(struct rev_info *opt)
 	 */
 
 	if (cmit_fmt_is_mail(opt->commit_format)) {
-		log_write_email_headers(opt, commit, &extra_headers,
-					&ctx.need_8bit_cte);
+		log_write_email_headers(opt, commit, &ctx.need_8bit_cte);
 		ctx.rev = opt;
 		ctx.print_email_subject = 1;
 	} else if (opt->commit_format != CMIT_FMT_USERFORMAT) {
@@ -672,7 +650,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 = opt->extra_headers;
 	ctx.preserve_subject = opt->preserve_subject;
 	ctx.reflog_info = opt->reflog_info;
 	ctx.fmt = opt->commit_format;
diff --git a/log-tree.h b/log-tree.h
index 48f11fb740..7f9c4f22b5 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -22,7 +22,6 @@ void format_decorations_extended(struct strbuf *sb, const struct commit *commit,
 			     format_decorations_extended((strbuf), (commit), (color), " (", ", ", ")")
 void show_decorations(struct rev_info *opt, struct commit *commit);
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
-			     const char **extra_headers_p,
 			     int *need_8bit_cte_p);
 void load_ref_decorations(int flags);
 
diff --git a/pretty.c b/pretty.c
index d0f86f5d85..56e668781a 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1641,6 +1641,21 @@ void pp_title_line(struct pretty_print_context *pp,
 	if (pp->after_subject) {
 		strbuf_addstr(sb, pp->after_subject);
 	}
+	if (pp->print_email_subject && pp->rev && pp->rev->mime_boundary) {
+		strbuf_addf(sb,
+			    "MIME-Version: 1.0\n"
+			    "Content-Type: multipart/mixed;"
+			    " boundary=\"%s%s\"\n"
+			    "\n"
+			    "This is a multi-part message in MIME "
+			    "format.\n"
+			    "--%s%s\n"
+			    "Content-Type: text/plain; "
+			    "charset=UTF-8; format=fixed\n"
+			    "Content-Transfer-Encoding: 8bit\n\n",
+			    mime_boundary_leader, pp->rev->mime_boundary,
+			    mime_boundary_leader, pp->rev->mime_boundary);
+	}
 	if (cmit_fmt_is_mail(pp->fmt)) {
 		strbuf_addch(sb, '\n');
 	}

-- 
2.12.2
Jeff King· Mar 25, 2017, 16:17 UTC · re: René Scharfe · lore

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

On Sat, Mar 25, 2017 at 01:16:42PM +0100, René Scharfe wrote:
Show 5 quoted lines
> Use the after_subject member of struct pretty_print_context to pass the
> extra_headers unchanged, and construct and add the MIME boundary headers
> directly in pretty.c::pp_title_line() instead of writing both to a
> static buffer in log-tree.c::log_write_email_headers() first.  That's
> easier, quicker and gets rid of said static buffer.
I'm definitely pleased with the direction. A few comments:
Show 6 quoted lines
> @@ -374,26 +372,9 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
>  		graph_show_oneline(opt->graph);
>  	}
>  	if (opt->mime_boundary) {
> -		static char subject_buffer[1024];
>  		static char buffer[1024];

We still have this other buffer, which ends up in stat_sep. It should probably get the same treatment, though I think the module boundaries make it a little more awkward. We look at it in diff_flush(), which otherwise doesn't need to know much about the pretty-printing.

Perhaps stat_sep should be a callback?
Show 11 quoted lines
> diff --git a/pretty.c b/pretty.c
> index d0f86f5d85..56e668781a 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1641,6 +1641,21 @@ void pp_title_line(struct pretty_print_context *pp,
>  	if (pp->after_subject) {
>  		strbuf_addstr(sb, pp->after_subject);
>  	}
> +	if (pp->print_email_subject && pp->rev && pp->rev->mime_boundary) {
> +		strbuf_addf(sb,
> +			    "MIME-Version: 1.0\n"

In the original, this would have been in "after_subject". Which means we would print it even if print_email_subject is not true. Why do we need to check it in the new conditional?

Not that I expect the behavior to be wrong either way; why would we have a mime boundary without setting print_email_subject? But I would think that "do we have a mime boundary" would be the right conditional to trigger printing it.

-Peff
René Scharfe· Mar 25, 2017, 16:56 UTC · re: Jeff King · lore

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

Am 25.03.2017 um 17:17 schrieb Jeff King:
Show 14 quoted lines
> On Sat, Mar 25, 2017 at 01:16:42PM +0100, René Scharfe wrote:
>> @@ -374,26 +372,9 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
>>  		graph_show_oneline(opt->graph);
>>  	}
>>  	if (opt->mime_boundary) {
>> -		static char subject_buffer[1024];
>>  		static char buffer[1024];
>
> We still have this other buffer, which ends up in stat_sep. It should
> probably get the same treatment, though I think the module boundaries
> make it a little more awkward. We look at it in diff_flush(), which
> otherwise doesn't need to know much about the pretty-printing.
>
> Perhaps stat_sep should be a callback?
Yes, it would be nice to avoid it, but I haven't found a clean way, yet. 
  In diff.c, where it's used, we don't have commit and rev_info 
available (which we'd have to pass to a callback, or consume right 
there), and that's probably how it should be.  Perhaps preparing the 
filename in advance and passing that as a string together with 
mime_boundary and no_inline might be the way to go.
Show 15 quoted lines
>> diff --git a/pretty.c b/pretty.c
>> index d0f86f5d85..56e668781a 100644
>> --- a/pretty.c
>> +++ b/pretty.c
>> @@ -1641,6 +1641,21 @@ void pp_title_line(struct pretty_print_context *pp,
>>  	if (pp->after_subject) {
>>  		strbuf_addstr(sb, pp->after_subject);
>>  	}
>> +	if (pp->print_email_subject && pp->rev && pp->rev->mime_boundary) {
>> +		strbuf_addf(sb,
>> +			    "MIME-Version: 1.0\n"
>
> In the original, this would have been in "after_subject". Which means we
> would print it even if print_email_subject is not true. Why do we need
> to check it in the new conditional?

No, we only would have printed it if log_write_email_headers() was called to append it to the static buffer. print_email_subject is only set when we call log_write_email_headers(), so checking it makes sure that we get the same behavior as before.

> Not that I expect the behavior to be wrong either way; why would we have
> a mime boundary without setting print_email_subject? But I would think
> that "do we have a mime boundary" would be the right conditional to
> trigger printing it.

FWIW, the test suite still passes with the print_email_subject check removed. And currently only cmd_format_patch() sets mime_boundary, so we don't need the check indeed.

René
Jeff King· Mar 25, 2017, 21:11 UTC · re: René Scharfe · lore

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

On Sat, Mar 25, 2017 at 05:56:55PM +0100, René Scharfe wrote:
Show 22 quoted lines
> Am 25.03.2017 um 17:17 schrieb Jeff King:
> > On Sat, Mar 25, 2017 at 01:16:42PM +0100, René Scharfe wrote:
> > > @@ -374,26 +372,9 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
> > >  		graph_show_oneline(opt->graph);
> > >  	}
> > >  	if (opt->mime_boundary) {
> > > -		static char subject_buffer[1024];
> > >  		static char buffer[1024];
> > 
> > We still have this other buffer, which ends up in stat_sep. It should
> > probably get the same treatment, though I think the module boundaries
> > make it a little more awkward. We look at it in diff_flush(), which
> > otherwise doesn't need to know much about the pretty-printing.
> > 
> > Perhaps stat_sep should be a callback?
> 
> Yes, it would be nice to avoid it, but I haven't found a clean way, yet.  In
> diff.c, where it's used, we don't have commit and rev_info available (which
> we'd have to pass to a callback, or consume right there), and that's
> probably how it should be.  Perhaps preparing the filename in advance and
> passing that as a string together with mime_boundary and no_inline might be
> the way to go.

I think the no-allocation way with a callback would be something like the patch at the end of this email. Having to stuff the commit into rev_info is horrible, because rev_info shouldn't be carrying data specific to individual commits. But it's really no different than what's there now, which is stuffing a filled-out commit-specific buffer into the same struct.

Doing it with a single allocated buffer would involve less callback rigmarole, but it doesn't change the fact that we are stuffing commit-specific data into the rev_info (via the diff_options member). And then you add on top the question of who is supposed to free it (which is punted on in the original by using a static; yech).

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.

Show 12 quoted lines
> > > +	if (pp->print_email_subject && pp->rev && pp->rev->mime_boundary) {
> > > +		strbuf_addf(sb,
> > > +			    "MIME-Version: 1.0\n"
> > 
> > In the original, this would have been in "after_subject". Which means we
> > would print it even if print_email_subject is not true. Why do we need
> > to check it in the new conditional?
> 
> No, we only would have printed it if log_write_email_headers() was called to
> append it to the static buffer.  print_email_subject is only set when we
> call log_write_email_headers(), so checking it makes sure that we get the
> same behavior as before.

OK. So your logic is "you would always set print_email_subject alongside log_write_email_headers()", and mine is "you would always call log_write_email_headers() when opt->mime_boundary is set". Both are true today, and I do not see a big chance of them becoming untrue.

So I'm OK with it either way.
Show 8 quoted lines
> > Not that I expect the behavior to be wrong either way; why would we have
> > a mime boundary without setting print_email_subject? But I would think
> > that "do we have a mime boundary" would be the right conditional to
> > trigger printing it.
> 
> FWIW, the test suite still passes with the print_email_subject check
> removed.  And currently only cmd_format_patch() sets mime_boundary, so we
> don't need the check indeed.

Yeah, while working on the other patch I looked at all the callers (there are only 2), and I think it's fine. I thought at first there might be problems with:

  git format-patch --attach --format=not-email:%s

but we skip this code when it's a non-email format (and anyway, format-patch always sets print_email_subject whether the format is email or not). That command _does_ generate nonsense, because cmd_format_patch() insists on showing the trailing mime boundary even if the format is not email. I'm not sure there's a good reason to use a non-email format with format-patch in the first place, let alone with --attach. Arguably it should bail as soon as it sees the incompatible options.

Anyway. Here's my attempt at the callback version of stat_sep.
---
Show changes to 4 files +35 −25

diff.c, diff.h, log-tree.c, revision.h

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;
René Scharfe· Mar 26, 2017, 13:41 UTC · re: Jeff King · lore

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

Am 25.03.2017 um 22:11 schrieb Jeff King:
Show 19 quoted lines
> 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().
Show 117 quoted lines
> 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(-)
Show changes to 3 files +6 −7

diff.c, diff.h, log-tree.c

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
Jeff King· Mar 27, 2017, 03:12 UTC · re: René Scharfe · lore

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

On Sun, Mar 26, 2017 at 03:41:14PM +0200, René Scharfe wrote:
Show 22 quoted lines
> 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().

Yeah, though now we have lifetime rules around stat_sep. How long does it stay good? Which functions decide it's now done? Are we sure they alweays get called? We're just tacking that onto the end of diff_flush() because the caller responsibilities are so unclear, but that's making an assumption that it always gets called.

This case might be simple enough that it's true, but it feels like a leaky module boundary.

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

Right, I think that is the simplest thing, but it falls afoul of the lifetime rules above.

If the rule is "the stat_sep gets printed once and then freed", that's pretty reasonable. But I wonder if there are cases where we might not print stat_sep (or call diff_flush at all for that matter). I'm not sure if that's possible with --attach, though, which constrains us to format-patch.

>  	if (opt->mime_boundary) {
> -		static char buffer[1024];
>  		struct strbuf filename =  STRBUF_INIT;

Actually, I guess the absolute simplest thing would be swap this out for a static strbuf, and leave the ownership with the function. That's ugly, but it's how it works now, and lets us drop the fixed buffer.

Another option is to put it in rev_info, and make the rev_info owner free it (which we know is restricted to cmd_format_patch(), as it is the only one who uses mime_boundary).

-Peff

← back to recent threads