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

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

[René Scharfe]

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(-)

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

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

[Jeff King]

On Sat, Mar 25, 2017 at 01:16:42PM +0100, René Scharfe wrote:

> 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:

> @@ -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?

> 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

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

[René Scharfe]

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.

>> 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é

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

[Jeff King]

On Sat, Mar 25, 2017 at 05:56:55PM +0100, René Scharfe wrote:

> 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.

> > > +	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.

> > 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.

---
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;

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

[René Scharfe]

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().

> 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

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

[Jeff King]

On Sun, Mar 26, 2017 at 03:41:14PM +0200, René Scharfe wrote:

> 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.

> 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