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