{"thread":{"id":"45510","subject":"[PATCH] pretty: add extra headers and MIME boundary directly","startedAt":"2017-03-25T12:17:05Z","lastAt":"2017-03-27T03:12:25Z","messageCount":6,"participants":["René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"315388","messageId":"6f3d0b54-7a9f-8088-3675-bd2980f69735@web.de","threadId":"45510","inReplyTo":null,"subject":"[PATCH] pretty: add extra headers and MIME boundary directly","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-25T12:16:42Z","receivedAt":"2017-03-25T12:17:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use the after_subject member of struct pretty_print_context to pass the\nextra_headers unchanged, and construct and add the MIME boundary headers\ndirectly in pretty.c::pp_title_line() instead of writing both to a\nstatic buffer in log-tree.c::log_write_email_headers() first.  That's\neasier, quicker and gets rid of said static buffer.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n builtin/log.c |  3 ++-\n log-tree.c    | 26 ++------------------------\n log-tree.h    |  1 -\n pretty.c      | 15 +++++++++++++++\n 4 files changed, 19 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 281af8c1ec..be564039c1 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -989,7 +989,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \t    open_next_file(NULL, rev->numbered_files ? NULL : \"cover-letter\", rev, quiet))\n \t\treturn;\n \n-\tlog_write_email_headers(rev, head, &pp.after_subject, &need_8bit_cte);\n+\tlog_write_email_headers(rev, head, &need_8bit_cte);\n+\tpp.after_subject = rev->extra_headers;\n \n \tfor (i = 0; !need_8bit_cte && i < nr; i++) {\n \t\tconst char *buf = get_commit_buffer(list[i], NULL);\ndiff --git a/log-tree.c b/log-tree.c\nindex 4618dd04ca..7049a17781 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -349,10 +349,8 @@ void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)\n }\n \n void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n-\t\t\t     const char **extra_headers_p,\n \t\t\t     int *need_8bit_cte_p)\n {\n-\tconst char *extra_headers = opt->extra_headers;\n \tconst char *name = oid_to_hex(opt->zero_commit ?\n \t\t\t\t      &null_oid : &commit->object.oid);\n \n@@ -374,26 +372,9 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\tgraph_show_oneline(opt->graph);\n \t}\n \tif (opt->mime_boundary) {\n-\t\tstatic char subject_buffer[1024];\n \t\tstatic char buffer[1024];\n \t\tstruct strbuf filename =  STRBUF_INIT;\n \t\t*need_8bit_cte_p = -1; /* NEVER */\n-\t\tsnprintf(subject_buffer, sizeof(subject_buffer) - 1,\n-\t\t\t \"%s\"\n-\t\t\t \"MIME-Version: 1.0\\n\"\n-\t\t\t \"Content-Type: multipart/mixed;\"\n-\t\t\t \" boundary=\\\"%s%s\\\"\\n\"\n-\t\t\t \"\\n\"\n-\t\t\t \"This is a multi-part message in MIME \"\n-\t\t\t \"format.\\n\"\n-\t\t\t \"--%s%s\\n\"\n-\t\t\t \"Content-Type: text/plain; \"\n-\t\t\t \"charset=UTF-8; format=fixed\\n\"\n-\t\t\t \"Content-Transfer-Encoding: 8bit\\n\\n\",\n-\t\t\t extra_headers ? extra_headers : \"\",\n-\t\t\t mime_boundary_leader, opt->mime_boundary,\n-\t\t\t mime_boundary_leader, opt->mime_boundary);\n-\t\textra_headers = subject_buffer;\n \n \t\tif (opt->numbered_files)\n \t\t\tstrbuf_addf(&filename, \"%d\", opt->nr);\n@@ -413,7 +394,6 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\topt->diffopt.stat_sep = buffer;\n \t\tstrbuf_release(&filename);\n \t}\n-\t*extra_headers_p = extra_headers;\n }\n \n static void show_sig_lines(struct rev_info *opt, int status, const char *bol)\n@@ -537,7 +517,6 @@ void show_log(struct rev_info *opt)\n \tstruct log_info *log = opt->loginfo;\n \tstruct commit *commit = log->commit, *parent = log->parent;\n \tint abbrev_commit = opt->abbrev_commit ? opt->abbrev : 40;\n-\tconst char *extra_headers = opt->extra_headers;\n \tstruct pretty_print_context ctx = {0};\n \n \topt->loginfo = NULL;\n@@ -597,8 +576,7 @@ void show_log(struct rev_info *opt)\n \t */\n \n \tif (cmit_fmt_is_mail(opt->commit_format)) {\n-\t\tlog_write_email_headers(opt, commit, &extra_headers,\n-\t\t\t\t\t&ctx.need_8bit_cte);\n+\t\tlog_write_email_headers(opt, commit, &ctx.need_8bit_cte);\n \t\tctx.rev = opt;\n \t\tctx.print_email_subject = 1;\n \t} else if (opt->commit_format != CMIT_FMT_USERFORMAT) {\n@@ -672,7 +650,7 @@ void show_log(struct rev_info *opt)\n \tctx.date_mode = opt->date_mode;\n \tctx.date_mode_explicit = opt->date_mode_explicit;\n \tctx.abbrev = opt->diffopt.abbrev;\n-\tctx.after_subject = extra_headers;\n+\tctx.after_subject = opt->extra_headers;\n \tctx.preserve_subject = opt->preserve_subject;\n \tctx.reflog_info = opt->reflog_info;\n \tctx.fmt = opt->commit_format;\ndiff --git a/log-tree.h b/log-tree.h\nindex 48f11fb740..7f9c4f22b5 100644\n--- a/log-tree.h\n+++ b/log-tree.h\n@@ -22,7 +22,6 @@ void format_decorations_extended(struct strbuf *sb, const struct commit *commit,\n \t\t\t     format_decorations_extended((strbuf), (commit), (color), \" (\", \", \", \")\")\n void show_decorations(struct rev_info *opt, struct commit *commit);\n void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n-\t\t\t     const char **extra_headers_p,\n \t\t\t     int *need_8bit_cte_p);\n void load_ref_decorations(int flags);\n \ndiff --git a/pretty.c b/pretty.c\nindex d0f86f5d85..56e668781a 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1641,6 +1641,21 @@ void pp_title_line(struct pretty_print_context *pp,\n \tif (pp->after_subject) {\n \t\tstrbuf_addstr(sb, pp->after_subject);\n \t}\n+\tif (pp->print_email_subject && pp->rev && pp->rev->mime_boundary) {\n+\t\tstrbuf_addf(sb,\n+\t\t\t    \"MIME-Version: 1.0\\n\"\n+\t\t\t    \"Content-Type: multipart/mixed;\"\n+\t\t\t    \" boundary=\\\"%s%s\\\"\\n\"\n+\t\t\t    \"\\n\"\n+\t\t\t    \"This is a multi-part message in MIME \"\n+\t\t\t    \"format.\\n\"\n+\t\t\t    \"--%s%s\\n\"\n+\t\t\t    \"Content-Type: text/plain; \"\n+\t\t\t    \"charset=UTF-8; format=fixed\\n\"\n+\t\t\t    \"Content-Transfer-Encoding: 8bit\\n\\n\",\n+\t\t\t    mime_boundary_leader, pp->rev->mime_boundary,\n+\t\t\t    mime_boundary_leader, pp->rev->mime_boundary);\n+\t}\n \tif (cmit_fmt_is_mail(pp->fmt)) {\n \t\tstrbuf_addch(sb, '\\n');\n \t}\n-- \n2.12.2\n\n"},{"id":"315393","messageId":"20170325161705.wh4zueasik6iwktv@sigill.intra.peff.net","threadId":"45510","inReplyTo":"6f3d0b54-7a9f-8088-3675-bd2980f69735@web.de","subject":"Re: [PATCH] pretty: add extra headers and MIME boundary directly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-25T16:17:06Z","receivedAt":"2017-03-25T16:23:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 25, 2017 at 01:16:42PM +0100, René Scharfe wrote:\n\n> Use the after_subject member of struct pretty_print_context to pass the\n> extra_headers unchanged, and construct and add the MIME boundary headers\n> directly in pretty.c::pp_title_line() instead of writing both to a\n> static buffer in log-tree.c::log_write_email_headers() first.  That's\n> easier, quicker and gets rid of said static buffer.\n\nI'm definitely pleased with the direction. A few comments:\n\n> @@ -374,26 +372,9 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n>  \t\tgraph_show_oneline(opt->graph);\n>  \t}\n>  \tif (opt->mime_boundary) {\n> -\t\tstatic char subject_buffer[1024];\n>  \t\tstatic char buffer[1024];\n\nWe still have this other buffer, which ends up in stat_sep. It should\nprobably get the same treatment, though I think the module boundaries\nmake it a little more awkward. We look at it in diff_flush(), which\notherwise doesn't need to know much about the pretty-printing.\n\nPerhaps stat_sep should be a callback?\n\n> diff --git a/pretty.c b/pretty.c\n> index d0f86f5d85..56e668781a 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1641,6 +1641,21 @@ void pp_title_line(struct pretty_print_context *pp,\n>  \tif (pp->after_subject) {\n>  \t\tstrbuf_addstr(sb, pp->after_subject);\n>  \t}\n> +\tif (pp->print_email_subject && pp->rev && pp->rev->mime_boundary) {\n> +\t\tstrbuf_addf(sb,\n> +\t\t\t    \"MIME-Version: 1.0\\n\"\n\nIn the original, this would have been in \"after_subject\". Which means we\nwould print it even if print_email_subject is not true. Why do we need\nto check it in the new conditional?\n\nNot that I expect the behavior to be wrong either way; why would we have\na mime boundary without setting print_email_subject? But I would think\nthat \"do we have a mime boundary\" would be the right conditional to\ntrigger printing it.\n\n-Peff\n"},{"id":"315396","messageId":"c5591beb-8cb2-dc19-7820-c8b9c68aad15@web.de","threadId":"45510","inReplyTo":"20170325161705.wh4zueasik6iwktv@sigill.intra.peff.net","subject":"Re: [PATCH] pretty: add extra headers and MIME boundary directly","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-25T16:56:55Z","receivedAt":"2017-03-25T16:57:13Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 25.03.2017 um 17:17 schrieb Jeff King:\n> On Sat, Mar 25, 2017 at 01:16:42PM +0100, René Scharfe wrote:\n>> @@ -374,26 +372,9 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n>>  \t\tgraph_show_oneline(opt->graph);\n>>  \t}\n>>  \tif (opt->mime_boundary) {\n>> -\t\tstatic char subject_buffer[1024];\n>>  \t\tstatic char buffer[1024];\n>\n> We still have this other buffer, which ends up in stat_sep. It should\n> probably get the same treatment, though I think the module boundaries\n> make it a little more awkward. We look at it in diff_flush(), which\n> otherwise doesn't need to know much about the pretty-printing.\n>\n> Perhaps stat_sep should be a callback?\n\nYes, it would be nice to avoid it, but I haven't found a clean way, yet. \n  In diff.c, where it's used, we don't have commit and rev_info \navailable (which we'd have to pass to a callback, or consume right \nthere), and that's probably how it should be.  Perhaps preparing the \nfilename in advance and passing that as a string together with \nmime_boundary and no_inline might be the way to go.\n\n>> diff --git a/pretty.c b/pretty.c\n>> index d0f86f5d85..56e668781a 100644\n>> --- a/pretty.c\n>> +++ b/pretty.c\n>> @@ -1641,6 +1641,21 @@ void pp_title_line(struct pretty_print_context *pp,\n>>  \tif (pp->after_subject) {\n>>  \t\tstrbuf_addstr(sb, pp->after_subject);\n>>  \t}\n>> +\tif (pp->print_email_subject && pp->rev && pp->rev->mime_boundary) {\n>> +\t\tstrbuf_addf(sb,\n>> +\t\t\t    \"MIME-Version: 1.0\\n\"\n>\n> In the original, this would have been in \"after_subject\". Which means we\n> would print it even if print_email_subject is not true. Why do we need\n> to check it in the new conditional?\n\nNo, we only would have printed it if log_write_email_headers() was \ncalled to append it to the static buffer.  print_email_subject is only \nset when we call log_write_email_headers(), so checking it makes sure \nthat we get the same behavior as before.\n\n> Not that I expect the behavior to be wrong either way; why would we have\n> a mime boundary without setting print_email_subject? But I would think\n> that \"do we have a mime boundary\" would be the right conditional to\n> trigger printing it.\n\nFWIW, the test suite still passes with the print_email_subject check \nremoved.  And currently only cmd_format_patch() sets mime_boundary, so \nwe don't need the check indeed.\n\nRené\n"},{"id":"315408","messageId":"20170325211149.yyvocmdfw4zbjyoi@sigill.intra.peff.net","threadId":"45510","inReplyTo":"c5591beb-8cb2-dc19-7820-c8b9c68aad15@web.de","subject":"Re: [PATCH] pretty: add extra headers and MIME boundary directly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-25T21:11:49Z","receivedAt":"2017-03-25T21:18:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 25, 2017 at 05:56:55PM +0100, René Scharfe wrote:\n\n> Am 25.03.2017 um 17:17 schrieb Jeff King:\n> > On Sat, Mar 25, 2017 at 01:16:42PM +0100, René Scharfe wrote:\n> > > @@ -374,26 +372,9 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n> > >  \t\tgraph_show_oneline(opt->graph);\n> > >  \t}\n> > >  \tif (opt->mime_boundary) {\n> > > -\t\tstatic char subject_buffer[1024];\n> > >  \t\tstatic char buffer[1024];\n> > \n> > We still have this other buffer, which ends up in stat_sep. It should\n> > probably get the same treatment, though I think the module boundaries\n> > make it a little more awkward. We look at it in diff_flush(), which\n> > otherwise doesn't need to know much about the pretty-printing.\n> > \n> > Perhaps stat_sep should be a callback?\n> \n> Yes, it would be nice to avoid it, but I haven't found a clean way, yet.  In\n> diff.c, where it's used, we don't have commit and rev_info available (which\n> we'd have to pass to a callback, or consume right there), and that's\n> probably how it should be.  Perhaps preparing the filename in advance and\n> passing that as a string together with mime_boundary and no_inline might be\n> the way to go.\n\nI think the no-allocation way with a callback would be something like\nthe patch at the end of this email. Having to stuff the commit into\nrev_info is horrible, because rev_info shouldn't be carrying data\nspecific to individual commits. But it's really no different than what's\nthere now, which is stuffing a filled-out commit-specific buffer into\nthe same struct.\n\nDoing it with a single allocated buffer would involve less callback\nrigmarole, but it doesn't change the fact that we are stuffing\ncommit-specific data into the rev_info (via the diff_options member).\nAnd then you add on top the question of who is supposed to free it\n(which is punted on in the original by using a static; yech).\n\nThe most correct way is that the caller of log_write_email_headers() and\ndiff_flush() should have a function-local strbuf which holds the data,\ngets passed to diff_flush() as some kind opaque context, and then is\nfreed afterwards. We don't have such a context, but if we were to abuse\ndiff_options.stat_sep _temporarily_, that would still be a lot cleaner.\nI.e., something like this:\n\n  struct strbuf stat_sep = STRBUF_INIT;\n\n  /* may write into stat_sep, depending on options */\n  log_write_email_headers(..., &stat_sep);\n  opt->diffopt.stat_sep = stat_sep.buf;\n\n  diff_flush(&opt->diffopt);\n  opt->diffopt.stat_sep = NULL;\n  strbuf_release(&stat_sep);\n\nBut it's a bit tricky because those two hunks happen in separate\nfunctions, which means passing the strbuf around.\n\n> > > +\tif (pp->print_email_subject && pp->rev && pp->rev->mime_boundary) {\n> > > +\t\tstrbuf_addf(sb,\n> > > +\t\t\t    \"MIME-Version: 1.0\\n\"\n> > \n> > In the original, this would have been in \"after_subject\". Which means we\n> > would print it even if print_email_subject is not true. Why do we need\n> > to check it in the new conditional?\n> \n> No, we only would have printed it if log_write_email_headers() was called to\n> append it to the static buffer.  print_email_subject is only set when we\n> call log_write_email_headers(), so checking it makes sure that we get the\n> same behavior as before.\n\nOK. So your logic is \"you would always set print_email_subject alongside\nlog_write_email_headers()\", and mine is \"you would always call\nlog_write_email_headers() when opt->mime_boundary is set\". Both are true\ntoday, and I do not see a big chance of them becoming untrue.\n\nSo I'm OK with it either way.\n\n> > Not that I expect the behavior to be wrong either way; why would we have\n> > a mime boundary without setting print_email_subject? But I would think\n> > that \"do we have a mime boundary\" would be the right conditional to\n> > trigger printing it.\n> \n> FWIW, the test suite still passes with the print_email_subject check\n> removed.  And currently only cmd_format_patch() sets mime_boundary, so we\n> don't need the check indeed.\n\nYeah, while working on the other patch I looked at all the callers\n(there are only 2), and I think it's fine. I thought at first there\nmight be problems with:\n\n  git format-patch --attach --format=not-email:%s\n\nbut we skip this code when it's a non-email format (and anyway,\nformat-patch always sets print_email_subject whether the format is email\nor not).  That command _does_ generate nonsense, because\ncmd_format_patch() insists on showing the trailing mime boundary even if\nthe format is not email. I'm not sure there's a good reason to use a\nnon-email format with format-patch in the first place, let alone with\n--attach. Arguably it should bail as soon as it sees the incompatible\noptions.\n\nAnyway. Here's my attempt at the callback version of stat_sep.\n\n---\ndiff --git a/diff.c b/diff.c\nindex a628ac3a9..d061f9e18 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4819,10 +4819,9 @@ void diff_flush(struct diff_options *options)\n \t\t\tfprintf(options->file, \"%s%c\",\n \t\t\t\tdiff_line_prefix(options),\n \t\t\t\toptions->line_termination);\n-\t\t\tif (options->stat_sep) {\n-\t\t\t\t/* attach patch instead of inline */\n-\t\t\t\tfputs(options->stat_sep, options->file);\n-\t\t\t}\n+\t\t\tif (options->stat_sep)\n+\t\t\t\toptions->stat_sep(options->file,\n+\t\t\t\t\t\t  options->stat_sep_data);\n \t\t}\n \n \t\tfor (i = 0; i < q->nr; i++) {\ndiff --git a/diff.h b/diff.h\nindex e9ccb38c2..4785f3b23 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -154,9 +154,11 @@ struct diff_options {\n \tunsigned ws_error_highlight;\n \tconst char *prefix;\n \tint prefix_length;\n-\tconst char *stat_sep;\n \tlong xdl_opts;\n \n+\tvoid (*stat_sep)(FILE *, void *);\n+\tvoid *stat_sep_data;\n+\n \tint stat_width;\n \tint stat_name_width;\n \tint stat_graph_width;\ndiff --git a/log-tree.c b/log-tree.c\nindex 7049a1778..5cf825c41 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -348,6 +348,31 @@ void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)\n \t}\n }\n \n+static void show_mime_attachment(FILE *out, void *data)\n+{\n+\tstruct rev_info *opt = data;\n+\tstruct strbuf filename = STRBUF_INIT;\n+\n+\tif (opt->numbered_files)\n+\t\tstrbuf_addf(&filename, \"%d\", opt->nr);\n+\telse\n+\t\tfmt_output_commit(&filename, opt->commit_for_mime, opt);\n+\n+\tfprintf(out,\n+\t\t\"\\n--%s%s\\n\"\n+\t\t\"Content-Type: text/x-patch;\"\n+\t\t\" name=\\\"%s\\\"\\n\"\n+\t\t\"Content-Transfer-Encoding: 8bit\\n\"\n+\t\t\"Content-Disposition: %s;\"\n+\t\t\" filename=\\\"%s\\\"\\n\\n\",\n+\t\tmime_boundary_leader, opt->mime_boundary,\n+\t\tfilename.buf,\n+\t\topt->no_inline ? \"attachment\" : \"inline\",\n+\t\tfilename.buf);\n+\n+\tstrbuf_release(&filename);\n+}\n+\n void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t     int *need_8bit_cte_p)\n {\n@@ -372,27 +397,10 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\tgraph_show_oneline(opt->graph);\n \t}\n \tif (opt->mime_boundary) {\n-\t\tstatic char buffer[1024];\n-\t\tstruct strbuf filename =  STRBUF_INIT;\n \t\t*need_8bit_cte_p = -1; /* NEVER */\n-\n-\t\tif (opt->numbered_files)\n-\t\t\tstrbuf_addf(&filename, \"%d\", opt->nr);\n-\t\telse\n-\t\t\tfmt_output_commit(&filename, commit, opt);\n-\t\tsnprintf(buffer, sizeof(buffer) - 1,\n-\t\t\t \"\\n--%s%s\\n\"\n-\t\t\t \"Content-Type: text/x-patch;\"\n-\t\t\t \" name=\\\"%s\\\"\\n\"\n-\t\t\t \"Content-Transfer-Encoding: 8bit\\n\"\n-\t\t\t \"Content-Disposition: %s;\"\n-\t\t\t \" filename=\\\"%s\\\"\\n\\n\",\n-\t\t\t mime_boundary_leader, opt->mime_boundary,\n-\t\t\t filename.buf,\n-\t\t\t opt->no_inline ? \"attachment\" : \"inline\",\n-\t\t\t filename.buf);\n-\t\topt->diffopt.stat_sep = buffer;\n-\t\tstrbuf_release(&filename);\n+\t\topt->diffopt.stat_sep = show_mime_attachment;\n+\t\topt->diffopt.stat_sep_data = opt;\n+\t\topt->commit_for_mime = commit;\n \t}\n }\n \ndiff --git a/revision.h b/revision.h\nindex 14886ec92..46ca45d96 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -156,6 +156,7 @@ struct rev_info {\n \tstruct log_info *loginfo;\n \tint\t\tnr, total;\n \tconst char\t*mime_boundary;\n+\tstruct commit *commit_for_mime;\n \tconst char\t*patch_suffix;\n \tint\t\tnumbered_files;\n \tint\t\treroll_count;\n"},{"id":"315460","messageId":"9d31c27b-fecb-ac1d-2b3f-8227c93862c2@web.de","threadId":"45510","inReplyTo":"20170325211149.yyvocmdfw4zbjyoi@sigill.intra.peff.net","subject":"Re: [PATCH] pretty: add extra headers and MIME boundary directly","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-26T13:41:14Z","receivedAt":"2017-03-26T13:45:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 25.03.2017 um 22:11 schrieb Jeff King:\n> The most correct way is that the caller of log_write_email_headers() and\n> diff_flush() should have a function-local strbuf which holds the data,\n> gets passed to diff_flush() as some kind opaque context, and then is\n> freed afterwards. We don't have such a context, but if we were to abuse\n> diff_options.stat_sep _temporarily_, that would still be a lot cleaner.\n> I.e., something like this:\n> \n>   struct strbuf stat_sep = STRBUF_INIT;\n> \n>   /* may write into stat_sep, depending on options */\n>   log_write_email_headers(..., &stat_sep);\n>   opt->diffopt.stat_sep = stat_sep.buf;\n> \n>   diff_flush(&opt->diffopt);\n>   opt->diffopt.stat_sep = NULL;\n>   strbuf_release(&stat_sep);\n> \n> But it's a bit tricky because those two hunks happen in separate\n> functions, which means passing the strbuf around.\n\nYou could have a destructor callback, called at the end of diff_flush().\n\n> Anyway. Here's my attempt at the callback version of stat_sep.\n> \n> ---\n> diff --git a/diff.c b/diff.c\n> index a628ac3a9..d061f9e18 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4819,10 +4819,9 @@ void diff_flush(struct diff_options *options)\n>  \t\t\tfprintf(options->file, \"%s%c\",\n>  \t\t\t\tdiff_line_prefix(options),\n>  \t\t\t\toptions->line_termination);\n> -\t\t\tif (options->stat_sep) {\n> -\t\t\t\t/* attach patch instead of inline */\n> -\t\t\t\tfputs(options->stat_sep, options->file);\n> -\t\t\t}\n> +\t\t\tif (options->stat_sep)\n> +\t\t\t\toptions->stat_sep(options->file,\n> +\t\t\t\t\t\t  options->stat_sep_data);\n>  \t\t}\n>  \n>  \t\tfor (i = 0; i < q->nr; i++) {\n> diff --git a/diff.h b/diff.h\n> index e9ccb38c2..4785f3b23 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -154,9 +154,11 @@ struct diff_options {\n>  \tunsigned ws_error_highlight;\n>  \tconst char *prefix;\n>  \tint prefix_length;\n> -\tconst char *stat_sep;\n>  \tlong xdl_opts;\n>  \n> +\tvoid (*stat_sep)(FILE *, void *);\n> +\tvoid *stat_sep_data;\n> +\n>  \tint stat_width;\n>  \tint stat_name_width;\n>  \tint stat_graph_width;\n> diff --git a/log-tree.c b/log-tree.c\n> index 7049a1778..5cf825c41 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -348,6 +348,31 @@ void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)\n>  \t}\n>  }\n>  \n> +static void show_mime_attachment(FILE *out, void *data)\n> +{\n> +\tstruct rev_info *opt = data;\n> +\tstruct strbuf filename = STRBUF_INIT;\n> +\n> +\tif (opt->numbered_files)\n> +\t\tstrbuf_addf(&filename, \"%d\", opt->nr);\n> +\telse\n> +\t\tfmt_output_commit(&filename, opt->commit_for_mime, opt);\n> +\n> +\tfprintf(out,\n> +\t\t\"\\n--%s%s\\n\"\n> +\t\t\"Content-Type: text/x-patch;\"\n> +\t\t\" name=\\\"%s\\\"\\n\"\n> +\t\t\"Content-Transfer-Encoding: 8bit\\n\"\n> +\t\t\"Content-Disposition: %s;\"\n> +\t\t\" filename=\\\"%s\\\"\\n\\n\",\n> +\t\tmime_boundary_leader, opt->mime_boundary,\n> +\t\tfilename.buf,\n> +\t\topt->no_inline ? \"attachment\" : \"inline\",\n> +\t\tfilename.buf);\n> +\n> +\tstrbuf_release(&filename);\n> +}\n> +\n>  void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n>  \t\t\t     int *need_8bit_cte_p)\n>  {\n> @@ -372,27 +397,10 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n>  \t\tgraph_show_oneline(opt->graph);\n>  \t}\n>  \tif (opt->mime_boundary) {\n> -\t\tstatic char buffer[1024];\n> -\t\tstruct strbuf filename =  STRBUF_INIT;\n>  \t\t*need_8bit_cte_p = -1; /* NEVER */\n> -\n> -\t\tif (opt->numbered_files)\n> -\t\t\tstrbuf_addf(&filename, \"%d\", opt->nr);\n> -\t\telse\n> -\t\t\tfmt_output_commit(&filename, commit, opt);\n> -\t\tsnprintf(buffer, sizeof(buffer) - 1,\n> -\t\t\t \"\\n--%s%s\\n\"\n> -\t\t\t \"Content-Type: text/x-patch;\"\n> -\t\t\t \" name=\\\"%s\\\"\\n\"\n> -\t\t\t \"Content-Transfer-Encoding: 8bit\\n\"\n> -\t\t\t \"Content-Disposition: %s;\"\n> -\t\t\t \" filename=\\\"%s\\\"\\n\\n\",\n> -\t\t\t mime_boundary_leader, opt->mime_boundary,\n> -\t\t\t filename.buf,\n> -\t\t\t opt->no_inline ? \"attachment\" : \"inline\",\n> -\t\t\t filename.buf);\n> -\t\topt->diffopt.stat_sep = buffer;\n> -\t\tstrbuf_release(&filename);\n> +\t\topt->diffopt.stat_sep = show_mime_attachment;\n> +\t\topt->diffopt.stat_sep_data = opt;\n> +\t\topt->commit_for_mime = commit;\n>  \t}\n>  }\n>  \n> diff --git a/revision.h b/revision.h\n> index 14886ec92..46ca45d96 100644\n> --- a/revision.h\n> +++ b/revision.h\n> @@ -156,6 +156,7 @@ struct rev_info {\n>  \tstruct log_info *loginfo;\n>  \tint\t\tnr, total;\n>  \tconst char\t*mime_boundary;\n> +\tstruct commit *commit_for_mime;\n>  \tconst char\t*patch_suffix;\n>  \tint\t\tnumbered_files;\n>  \tint\t\treroll_count;\n\nHmm.  I'm a fan of callbacks, but using them can make the code a bit\nhard to follow.  And void context pointers add a type safety hazard.\nDo we need to be this generic?  How about switching stat_sep to strbuf?\nfmt_output_commit() requires an allocation anyway, so why not allocate\nstat_sep as well?\n\n---\n diff.c     | 7 ++++---\n diff.h     | 2 +-\n log-tree.c | 4 +---\n 3 files changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex a628ac3a95..a4afa8eba2 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -41,7 +41,7 @@ static int diff_mnemonic_prefix;\n static int diff_no_prefix;\n static int diff_stat_graph_width;\n static int diff_dirstat_permille_default = 30;\n-static struct diff_options default_diff_options;\n+static struct diff_options default_diff_options = { STRBUF_INIT };\n static long diff_algorithm;\n static unsigned ws_error_highlight_default = WSEH_NEW;\n \n@@ -4819,9 +4819,9 @@ void diff_flush(struct diff_options *options)\n \t\t\tfprintf(options->file, \"%s%c\",\n \t\t\t\tdiff_line_prefix(options),\n \t\t\t\toptions->line_termination);\n-\t\t\tif (options->stat_sep) {\n+\t\t\tif (options->stat_sep.len) {\n \t\t\t\t/* attach patch instead of inline */\n-\t\t\t\tfputs(options->stat_sep, options->file);\n+\t\t\t\tstrbuf_write(&options->stat_sep, options->file);\n \t\t\t}\n \t\t}\n \n@@ -4842,6 +4842,7 @@ void diff_flush(struct diff_options *options)\n \tDIFF_QUEUE_CLEAR(q);\n \tif (options->close_file)\n \t\tfclose(options->file);\n+\tstrbuf_release(&options->stat_sep);\n \n \t/*\n \t * Report the content-level differences with HAS_CHANGES;\ndiff --git a/diff.h b/diff.h\nindex e9ccb38c26..6a537df1ab 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -116,6 +116,7 @@ enum diff_submodule_format {\n };\n \n struct diff_options {\n+\tstruct strbuf stat_sep;\n \tconst char *orderfile;\n \tconst char *pickaxe;\n \tconst char *single_follow;\n@@ -154,7 +155,6 @@ struct diff_options {\n \tunsigned ws_error_highlight;\n \tconst char *prefix;\n \tint prefix_length;\n-\tconst char *stat_sep;\n \tlong xdl_opts;\n \n \tint stat_width;\ndiff --git a/log-tree.c b/log-tree.c\nindex 7049a17781..cd4f363d9b 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -372,7 +372,6 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\tgraph_show_oneline(opt->graph);\n \t}\n \tif (opt->mime_boundary) {\n-\t\tstatic char buffer[1024];\n \t\tstruct strbuf filename =  STRBUF_INIT;\n \t\t*need_8bit_cte_p = -1; /* NEVER */\n \n@@ -380,7 +379,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\tstrbuf_addf(&filename, \"%d\", opt->nr);\n \t\telse\n \t\t\tfmt_output_commit(&filename, commit, opt);\n-\t\tsnprintf(buffer, sizeof(buffer) - 1,\n+\t\tstrbuf_addf(&opt->diffopt.stat_sep,\n \t\t\t \"\\n--%s%s\\n\"\n \t\t\t \"Content-Type: text/x-patch;\"\n \t\t\t \" name=\\\"%s\\\"\\n\"\n@@ -391,7 +390,6 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t filename.buf,\n \t\t\t opt->no_inline ? \"attachment\" : \"inline\",\n \t\t\t filename.buf);\n-\t\topt->diffopt.stat_sep = buffer;\n \t\tstrbuf_release(&filename);\n \t}\n }\n-- \n2.12.2\n\n"},{"id":"315498","messageId":"20170327031216.pbxbmgnekt5zgxri@sigill.intra.peff.net","threadId":"45510","inReplyTo":"9d31c27b-fecb-ac1d-2b3f-8227c93862c2@web.de","subject":"Re: [PATCH] pretty: add extra headers and MIME boundary directly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-27T03:12:16Z","receivedAt":"2017-03-27T03:12:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 26, 2017 at 03:41:14PM +0200, René Scharfe wrote:\n\n> Am 25.03.2017 um 22:11 schrieb Jeff King:\n> > The most correct way is that the caller of log_write_email_headers() and\n> > diff_flush() should have a function-local strbuf which holds the data,\n> > gets passed to diff_flush() as some kind opaque context, and then is\n> > freed afterwards. We don't have such a context, but if we were to abuse\n> > diff_options.stat_sep _temporarily_, that would still be a lot cleaner.\n> > I.e., something like this:\n> > \n> >   struct strbuf stat_sep = STRBUF_INIT;\n> > \n> >   /* may write into stat_sep, depending on options */\n> >   log_write_email_headers(..., &stat_sep);\n> >   opt->diffopt.stat_sep = stat_sep.buf;\n> > \n> >   diff_flush(&opt->diffopt);\n> >   opt->diffopt.stat_sep = NULL;\n> >   strbuf_release(&stat_sep);\n> > \n> > But it's a bit tricky because those two hunks happen in separate\n> > functions, which means passing the strbuf around.\n> \n> You could have a destructor callback, called at the end of diff_flush().\n\nYeah, though now we have lifetime rules around stat_sep. How long does\nit stay good? Which functions decide it's now done? Are we sure they\nalweays get called? We're just tacking that onto the end of diff_flush()\nbecause the caller responsibilities are so unclear, but that's making an\nassumption that it always gets called.\n\nThis case might be simple enough that it's true, but it feels like a\nleaky module boundary.\n\n> Hmm.  I'm a fan of callbacks, but using them can make the code a bit\n> hard to follow.  And void context pointers add a type safety hazard.\n> Do we need to be this generic?  How about switching stat_sep to strbuf?\n> fmt_output_commit() requires an allocation anyway, so why not allocate\n> stat_sep as well?\n\nRight, I think that is the simplest thing, but it falls afoul of the\nlifetime rules above.\n\nIf the rule is \"the stat_sep gets printed once and then freed\", that's\npretty reasonable. But I wonder if there are cases where we might not\nprint stat_sep (or call diff_flush at all for that matter). I'm not sure\nif that's possible with --attach, though, which constrains us to\nformat-patch.\n\n>  \tif (opt->mime_boundary) {\n> -\t\tstatic char buffer[1024];\n>  \t\tstruct strbuf filename =  STRBUF_INIT;\n\nActually, I guess the absolute simplest thing would be swap this out for\na static strbuf, and leave the ownership with the function. That's ugly,\nbut it's how it works now, and lets us drop the fixed buffer.\n\nAnother option is to put it in rev_info, and make the rev_info owner\nfree it (which we know is restricted to cmd_format_patch(), as it is the\nonly one who uses mime_boundary).\n\n-Peff\n"}]}