Re: [PATCH] pretty: add extra headers and MIME boundary directly
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(..., _sep); > > opt->diffopt.stat_sep = stat_sep.buf; > > > > diff_flush(>diffopt); > > opt->diffopt.stat_sep = NULL; > > strbuf_release(_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
Re: [PATCH] pretty: add extra headers and MIME boundary directly
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(..., _sep); > opt->diffopt.stat_sep = stat_sep.buf; > > diff_flush(>diffopt); > opt->diffopt.stat_sep = NULL; > strbuf_release(_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(, "%d", opt->nr); > + else > + fmt_output_commit(, 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(); > +} > + > 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(, "%d", opt->nr); > - else > - fmt_output_commit(, 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(); > + 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
Re: [PATCH] pretty: add extra headers and MIME boundary directly
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(..., _sep); opt->diffopt.stat_sep = stat_sep.buf; diff_flush(>diffopt); opt->diffopt.stat_sep = NULL; strbuf_release(_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",
Re: [PATCH] pretty: add extra headers and MIME boundary directly
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
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