{"thread":{"id":"45253","subject":"format-patch subject-prefix gets truncated when using the --numbered flag","startedAt":"2017-02-28T16:29:20Z","lastAt":"2017-03-02T01:10:34Z","messageCount":13,"participants":["Adrian Dudau","Junio C Hamano","Jeff King","René Scharfe"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"312851","messageId":"1488297556.2955.11.camel@enea.com","threadId":"45253","inReplyTo":null,"subject":"format-patch subject-prefix gets truncated when using the --numbered flag","fromName":"Adrian Dudau","fromEmail":"adrian.dudau@enea.com","sentAt":"2017-02-28T15:59:16Z","receivedAt":"2017-02-28T16:29:20Z","isPatch":false,"sender":{"key":"adrian.dudau@enea.com","avatar":null},"body":"Hello,\n\nI noticed that the --subject-prefix string gets truncated sometimes,\nbut only when using the --numbered flat. Here's an example:\n\naddu@sestofb11:/data/fb/addu/git$ export longm=\"very very very very\nvery very very very very very very very very very long prefix\"\n\n\naddu@sestofb11:/data/fb/addu/git$ git format-patch -1 --subject-\nprefix=\"$longm][PATCH\"\n\nAs expected, in the generated patch file we have:\nSubject: [very very very very very very very very very very very very\nvery very long prefix][PATCH] \n First batch after 2.12\n\nBut now, if I pass the --numbered flag too:\naddu@sestofb11:/data/fb/addu/git$ git format-patch -1 --numbered --\nsubject-prefix=\"$longm][PATCH\"\n\nIn the generated patch file we get this: \nSubject: [very very very very very very very very very very verFirst\nbatch \n after 2.12\n\nThis is happening on the latest master branch, so I dug through the\ncode and tracked the issue to this piece of code in log-tree.c:\n\n        if (opt->total > 0) {\n                static char buffer[64];\n                snprintf(buffer, sizeof(buffer),\n                         \"Subject: [%s%s%0*d/%d] \",\n                         opt->subject_prefix,\n                         *opt->subject_prefix ? \" \" : \"\",\n                         digits_in_number(opt->total),\n                         opt->nr, opt->total);\n                subject = buffer;\n        } else if (opt->total == 0 && opt->subject_prefix && *opt-\n>subject_prefix) {\n                static char buffer[256];\n                snprintf(buffer, sizeof(buffer),\n                         \"Subject: [%s] \",\n                         opt->subject_prefix);\n                subject = buffer;\n        } else {\n                subject = \"Subject: \";\n        }\n\nApparently the size of the \"buffer\" var is different in the two\nsituations. Anybody knows if this is by design or just an old\noversight?\nI can send a patch to fix it but I'm not very familiar with the git\ncode and I'm afraid some hidden consequence I don't see right now.\n\nCheers,\n--Adrian"},{"id":"312869","messageId":"xmqqinnuky9e.fsf@gitster.mtv.corp.google.com","threadId":"45253","inReplyTo":"1488297556.2955.11.camel@enea.com","subject":"Re: format-patch subject-prefix gets truncated when using the --numbered flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-28T17:42:37Z","receivedAt":"2017-02-28T18:50:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adrian Dudau <Adrian.Dudau@enea.com> writes:\n\n> I noticed that the --subject-prefix string gets truncated sometimes,\n> but only when using the --numbered flat. Here's an example:\n>\n> addu@sestofb11:/data/fb/addu/git$ export longm=\"very very very very\n> very very very very very very very very very very long prefix\"\n\nThis looks like \"dr, my arm hurts when I twist it this way---don't\ndo that then\" ;-).  Truncation is designed and intended to preserve\nspace for the real title of commit.\n\nHaving said that...\n\n> This is happening on the latest master branch, so I dug through the\n> code and tracked the issue to this piece of code in log-tree.c:\n>\n>         if (opt->total > 0) {\n>                 static char buffer[64];\n>                 snprintf(buffer, sizeof(buffer),\n>                          \"Subject: [%s%s%0*d/%d] \",\n>                          opt->subject_prefix,\n>                          *opt->subject_prefix ? \" \" : \"\",\n>                          digits_in_number(opt->total),\n>                          opt->nr, opt->total);\n>                 subject = buffer;\n>         } else if (opt->total == 0 && opt->subject_prefix && *opt-\n>>subject_prefix) {\n>                 static char buffer[256];\n>                 snprintf(buffer, sizeof(buffer),\n>                          \"Subject: [%s] \",\n>                          opt->subject_prefix);\n>                 subject = buffer;\n>         } else {\n>                 subject = \"Subject: \";\n>         }\n>\n> Apparently the size of the \"buffer\" var is different in the two\n> situations. Anybody knows if this is by design or just an old\n> oversight?\n\nI think this is just an old oversight.  The latter should have been\neven shorter than the former one (or the former should be longer\nthan the latter) to account for the width of the counter e.g. 01/27.\n"},{"id":"312915","messageId":"20170228181719.t67abnnnsdcx2xfd@sigill.intra.peff.net","threadId":"45253","inReplyTo":"1488297556.2955.11.camel@enea.com","subject":"Re: format-patch subject-prefix gets truncated when using the --numbered flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T18:17:19Z","receivedAt":"2017-02-28T22:19:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 28, 2017 at 03:59:16PM +0000, Adrian Dudau wrote:\n\n> This is happening on the latest master branch, so I dug through the\n> code and tracked the issue to this piece of code in log-tree.c:\n> \n>         if (opt->total > 0) {\n>                 static char buffer[64];\n>                 snprintf(buffer, sizeof(buffer),\n>                          \"Subject: [%s%s%0*d/%d] \",\n>                          opt->subject_prefix,\n>                          *opt->subject_prefix ? \" \" : \"\",\n>                          digits_in_number(opt->total),\n>                          opt->nr, opt->total);\n>                 subject = buffer;\n>         } else if (opt->total == 0 && opt->subject_prefix && *opt-\n> >subject_prefix) {\n>                 static char buffer[256];\n>                 snprintf(buffer, sizeof(buffer),\n>                          \"Subject: [%s] \",\n>                          opt->subject_prefix);\n>                 subject = buffer;\n>         } else {\n>                 subject = \"Subject: \";\n>         }\n> \n> Apparently the size of the \"buffer\" var is different in the two\n> situations. Anybody knows if this is by design or just an old\n> oversight?\n\nI think it's just an old oversight. There are some other fixed-size\nbuffers later, too, which could similarly truncate.\n\nI think these should all be \"static strbuf\", and entering the function\nthey should get strbuf_reset(), followed by a strbuf_addf(). The static\nstrbufs will be the owners of the allocated heap memory, and it will get\nreused from call to call.\n\nThat stops the immediate problem. As a function interface, it's pretty\nugly. It would probably make more sense for the caller to pass in a\nstrbuf rather than have us pass out pointers to static storage. For the\ncall in make_cover_letter(), that would be fine. For show_log(), it's\nless clear. That's called for every commit in \"git log\", which might be\na little sensitive to allocations.\n\nThe only persistent storage it has is via the rev_info. Perhaps it could\nhold some scratch buffers for the traversal (though I don't think we\ncurrently do any resource-freeing when done with a rev_info, so it\neffectively becomes a leak).\n\n-Peff\n"},{"id":"312919","messageId":"xmqqzih6jejg.fsf@gitster.mtv.corp.google.com","threadId":"45253","inReplyTo":"xmqqinnuky9e.fsf@gitster.mtv.corp.google.com","subject":"Re: format-patch subject-prefix gets truncated when using the --numbered flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-28T19:33:55Z","receivedAt":"2017-02-28T22:35:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Adrian Dudau <Adrian.Dudau@enea.com> writes:\n>\n>> I noticed that the --subject-prefix string gets truncated sometimes,\n>> but only when using the --numbered flat. Here's an example:\n>>\n>> addu@sestofb11:/data/fb/addu/git$ export longm=\"very very very very\n>> very very very very very very very very very very long prefix\"\n>\n> This looks like \"dr, my arm hurts when I twist it this way---don't\n> do that then\" ;-).  Truncation is designed and intended to preserve\n> space for the real title of commit.\n> \n> Having said that...\n> ...\n> I think this is just an old oversight.  The latter should have been\n> even shorter than the former one (or the former should be longer\n> than the latter) to account for the width of the counter e.g. 01/27.\n\nAnd having said all that, if we really want to allow overlong\nsubject prefix that would end up hiding the real title of the patch,\na modern way to do so would be to use xstrfmt() like the attached\ntoy-patch does.  Note that this is merely a small first step---you'd\nnotice that \"subject\" is kept around as a \"static\" and only freed\nupon entry to this function for the second time, to preserve the\nownership model of the original code.  In a real \"fix\" (if this\nneeds to be \"fixed\", that is), I think the ownership model of the\nstorage used for *subject_p and *extra_headers_p needs to be updated\nso that it will become caller's responsibility to free them\n(similarly, the ownership model of opt->diffopt.stat_sep that is\nassigned the address of the static buffer[] in the same function\nneeds to be revisited).\n\nThat \"buffer\" thing I think would need to be a bit more careful even\nin the current code, which _does_ use snprintf() correctly to avoid\noverflowing the buffer[], by the way.  If you have an overlong\nopt->mime_boundary, the resulting \"e-mail\" looking output can become\nstructurely broken.  The truncation may happen way before the full\nline for Content-Transfer-Encoding: is written, for example.\n\nSo this function seems to have a lot more graver problems that need\nto be looked at.\n\n log-tree.c | 24 +++++++++---------------\n 1 file changed, 9 insertions(+), 15 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 8c2415747a..24c98f5a80 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -337,29 +337,23 @@ 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 *subject = NULL;\n+\tstatic const char *subject = NULL;\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+\tfree((void *)subject);\n \t*need_8bit_cte_p = 0; /* unknown */\n \tif (opt->total > 0) {\n-\t\tstatic char buffer[64];\n-\t\tsnprintf(buffer, sizeof(buffer),\n-\t\t\t \"Subject: [%s%s%0*d/%d] \",\n-\t\t\t opt->subject_prefix,\n-\t\t\t *opt->subject_prefix ? \" \" : \"\",\n-\t\t\t digits_in_number(opt->total),\n-\t\t\t opt->nr, opt->total);\n-\t\tsubject = buffer;\n+\t\tsubject = xstrfmt(\"Subject: [%s%s%0*d/%d] \",\n+\t\t\t\t  opt->subject_prefix,\n+\t\t\t\t  *opt->subject_prefix ? \" \" : \"\",\n+\t\t\t\t  digits_in_number(opt->total),\n+\t\t\t\t  opt->nr, opt->total);\n \t} else if (opt->total == 0 && opt->subject_prefix && *opt->subject_prefix) {\n-\t\tstatic char buffer[256];\n-\t\tsnprintf(buffer, sizeof(buffer),\n-\t\t\t \"Subject: [%s] \",\n-\t\t\t opt->subject_prefix);\n-\t\tsubject = buffer;\n+\t\tsubject = xstrfmt(\"Subject: [%s] \", opt->subject_prefix);\n \t} else {\n-\t\tsubject = \"Subject: \";\n+\t\tsubject = xstrdup(\"Subject: \");\n \t}\n \n \tfprintf(opt->diffopt.file, \"From %s Mon Sep 17 00:00:00 2001\\n\", name);\n"},{"id":"312931","messageId":"20170228224424.p2po5giijeyvkhib@sigill.intra.peff.net","threadId":"45253","inReplyTo":"xmqqzih6jejg.fsf@gitster.mtv.corp.google.com","subject":"Re: format-patch subject-prefix gets truncated when using the --numbered flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T22:44:24Z","receivedAt":"2017-03-01T00:32:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 28, 2017 at 11:33:55AM -0800, Junio C Hamano wrote:\n\n> And having said all that, if we really want to allow overlong\n> subject prefix that would end up hiding the real title of the patch,\n> a modern way to do so would be to use xstrfmt() like the attached\n> toy-patch does.\n\nIf you are going to keep the ownership inside this function via statics,\nyou should use a strbuf. That lets you avoid reallocating on each entry\n(and we call this once per commit in a traversal, though see below).\n\n> Note that this is merely a small first step---you'd\n> notice that \"subject\" is kept around as a \"static\" and only freed\n> upon entry to this function for the second time, to preserve the\n> ownership model of the original code.  In a real \"fix\" (if this\n> needs to be \"fixed\", that is), I think the ownership model of the\n> storage used for *subject_p and *extra_headers_p needs to be updated\n> so that it will become caller's responsibility to free them\n> (similarly, the ownership model of opt->diffopt.stat_sep that is\n> assigned the address of the static buffer[] in the same function\n> needs to be revisited).\n\nI agree the ownership model is ugly, and would be nicer if the caller\npassed in a strbuf to write into (or a pointer out-parameter, I guess,\nbut I think strbufs make the ownership semantics much more obvious).\n\nI would just worry about allocation overhead, but that is probably an\noverblown concern for format-patch. It tends to be called on a much\nsmaller set of commits, and it is generally used with \"-p\", which\ncreates a lot of overhead already.\n\n-Peff\n"},{"id":"312941","messageId":"58e05599-5dc4-9881-d8c0-89ad1f2e3838@web.de","threadId":"45253","inReplyTo":"20170228181719.t67abnnnsdcx2xfd@sigill.intra.peff.net","subject":"[PATCH 2/2] pretty: use fmt_output_email_subject()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-01T11:37:07Z","receivedAt":"2017-03-01T11:44:15Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add the email-style subject prefix (e.g. \"Subject: [PATCH] \") directly\nwhen it's needed instead of letting log_write_email_headers() prepare\nit in a static buffer in advance.  This simplifies storage ownership and\ncode flow.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\nThis slows down the last three tests in p4000 by ca. 3% for some reason,\nso we may want to only do the first part for now, which is performance\nneutral on my machine.\n\n builtin/log.c      | 5 +++--\n builtin/shortlog.c | 2 +-\n commit.h           | 6 ++++--\n log-tree.c         | 9 +++------\n log-tree.h         | 1 -\n pretty.c           | 7 ++++---\n 6 files changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 55d20cc2d8..281af8c1ec 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -989,8 +989,7 @@ 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.subject, &pp.after_subject,\n-\t\t\t\t&need_8bit_cte);\n+\tlog_write_email_headers(rev, head, &pp.after_subject, &need_8bit_cte);\n \n \tfor (i = 0; !need_8bit_cte && i < nr; i++) {\n \t\tconst char *buf = get_commit_buffer(list[i], NULL);\n@@ -1005,6 +1004,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \tmsg = body;\n \tpp.fmt = CMIT_FMT_EMAIL;\n \tpp.date_mode.type = DATE_RFC2822;\n+\tpp.rev = rev;\n+\tpp.print_email_subject = 1;\n \tpp_user_info(&pp, NULL, &sb, committer, encoding);\n \tpp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);\n \tpp_remainder(&pp, &msg, &sb, 0);\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex c9585d475d..f78bb4818d 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -148,7 +148,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n \n \tctx.fmt = CMIT_FMT_USERFORMAT;\n \tctx.abbrev = log->abbrev;\n-\tctx.subject = \"\";\n+\tctx.print_email_subject = 1;\n \tctx.after_subject = \"\";\n \tctx.date_mode.type = DATE_NORMAL;\n \tctx.output_encoding = get_log_output_encoding();\ndiff --git a/commit.h b/commit.h\nindex 9c12abb911..459daef94a 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -142,21 +142,24 @@ static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)\n \treturn (fmt == CMIT_FMT_EMAIL || fmt == CMIT_FMT_MBOXRD);\n }\n \n+struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */\n+\n struct pretty_print_context {\n \t/*\n \t * Callers should tweak these to change the behavior of pp_* functions.\n \t */\n \tenum cmit_fmt fmt;\n \tint abbrev;\n-\tconst char *subject;\n \tconst char *after_subject;\n \tint preserve_subject;\n \tstruct date_mode date_mode;\n \tunsigned date_mode_explicit:1;\n+\tunsigned print_email_subject:1;\n \tint expand_tabs_in_log;\n \tint need_8bit_cte;\n \tchar *notes_message;\n \tstruct reflog_walk_info *reflog_info;\n+\tstruct rev_info *rev;\n \tconst char *output_encoding;\n \tstruct string_list *mailmap;\n \tint color;\n@@ -175,7 +178,6 @@ struct userformat_want {\n };\n \n extern int has_non_ascii(const char *text);\n-struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */\n extern const char *logmsg_reencode(const struct commit *commit,\n \t\t\t\t   char **commit_encoding,\n \t\t\t\t   const char *output_encoding);\ndiff --git a/log-tree.c b/log-tree.c\nindex 44febb75ab..4618dd04ca 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -349,11 +349,9 @@ 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 **subject_p,\n \t\t\t     const char **extra_headers_p,\n \t\t\t     int *need_8bit_cte_p)\n {\n-\tstatic struct strbuf subject = STRBUF_INIT;\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@@ -415,9 +413,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-\tstrbuf_reset(&subject);\n-\tfmt_output_email_subject(&subject, opt);\n-\t*subject_p = subject.buf;\n \t*extra_headers_p = extra_headers;\n }\n \n@@ -602,8 +597,10 @@ 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, &ctx.subject, &extra_headers,\n+\t\tlog_write_email_headers(opt, commit, &extra_headers,\n \t\t\t\t\t&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 \t\tfputs(diff_get_color_opt(&opt->diffopt, DIFF_COMMIT), opt->diffopt.file);\n \t\tif (opt->commit_format != CMIT_FMT_ONELINE)\ndiff --git a/log-tree.h b/log-tree.h\nindex dd75dd7770..48f11fb740 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 **subject_p,\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);\ndiff --git a/pretty.c b/pretty.c\nindex 5e683830d9..d0f86f5d85 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1607,8 +1607,9 @@ void pp_title_line(struct pretty_print_context *pp,\n \t\t\t\tpp->preserve_subject ? \"\\n\" : \" \");\n \n \tstrbuf_grow(sb, title.len + 1024);\n-\tif (pp->subject) {\n-\t\tstrbuf_addstr(sb, pp->subject);\n+\tif (pp->print_email_subject) {\n+\t\tif (pp->rev)\n+\t\t\tfmt_output_email_subject(sb, pp->rev);\n \t\tif (needs_rfc2047_encoding(title.buf, title.len, RFC2047_SUBJECT))\n \t\t\tadd_rfc2047(sb, title.buf, title.len,\n \t\t\t\t\t\tencoding, RFC2047_SUBJECT);\n@@ -1818,7 +1819,7 @@ void pretty_print_commit(struct pretty_print_context *pp,\n \t}\n \n \tpp_header(pp, encoding, commit, &msg, sb);\n-\tif (pp->fmt != CMIT_FMT_ONELINE && !pp->subject) {\n+\tif (pp->fmt != CMIT_FMT_ONELINE && !pp->print_email_subject) {\n \t\tstrbuf_addch(sb, '\\n');\n \t}\n \n-- \n2.12.0\n\n"},{"id":"312942","messageId":"e3de1725-99c3-d5a2-2cab-a05aa8546efb@web.de","threadId":"45253","inReplyTo":"20170228181719.t67abnnnsdcx2xfd@sigill.intra.peff.net","subject":"[PATCH 1/2] log-tree: factor out fmt_output_email_subject()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-01T11:36:38Z","receivedAt":"2017-03-01T11:44:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use a strbuf to store the subject prefix string and move its\nconstruction into its own function.  This gets rid of two arbitrary\nlength limits and allows the string to be added by callers directly.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n log-tree.c | 40 ++++++++++++++++++++--------------------\n log-tree.h |  1 +\n 2 files changed, 21 insertions(+), 20 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 8c2415747a..44febb75ab 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -332,35 +332,33 @@ void fmt_output_commit(struct strbuf *filename,\n \tstrbuf_release(&subject);\n }\n \n+void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)\n+{\n+\tif (opt->total > 0) {\n+\t\tstrbuf_addf(sb, \"Subject: [%s%s%0*d/%d] \",\n+\t\t\t    opt->subject_prefix,\n+\t\t\t    *opt->subject_prefix ? \" \" : \"\",\n+\t\t\t    digits_in_number(opt->total),\n+\t\t\t    opt->nr, opt->total);\n+\t} else if (opt->total == 0 && opt->subject_prefix && *opt->subject_prefix) {\n+\t\tstrbuf_addf(sb, \"Subject: [%s] \",\n+\t\t\t    opt->subject_prefix);\n+\t} else {\n+\t\tstrbuf_addstr(sb, \"Subject: \");\n+\t}\n+}\n+\n void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t     const char **subject_p,\n \t\t\t     const char **extra_headers_p,\n \t\t\t     int *need_8bit_cte_p)\n {\n-\tconst char *subject = NULL;\n+\tstatic struct strbuf subject = STRBUF_INIT;\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 \t*need_8bit_cte_p = 0; /* unknown */\n-\tif (opt->total > 0) {\n-\t\tstatic char buffer[64];\n-\t\tsnprintf(buffer, sizeof(buffer),\n-\t\t\t \"Subject: [%s%s%0*d/%d] \",\n-\t\t\t opt->subject_prefix,\n-\t\t\t *opt->subject_prefix ? \" \" : \"\",\n-\t\t\t digits_in_number(opt->total),\n-\t\t\t opt->nr, opt->total);\n-\t\tsubject = buffer;\n-\t} else if (opt->total == 0 && opt->subject_prefix && *opt->subject_prefix) {\n-\t\tstatic char buffer[256];\n-\t\tsnprintf(buffer, sizeof(buffer),\n-\t\t\t \"Subject: [%s] \",\n-\t\t\t opt->subject_prefix);\n-\t\tsubject = buffer;\n-\t} else {\n-\t\tsubject = \"Subject: \";\n-\t}\n \n \tfprintf(opt->diffopt.file, \"From %s Mon Sep 17 00:00:00 2001\\n\", name);\n \tgraph_show_oneline(opt->graph);\n@@ -417,7 +415,9 @@ 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*subject_p = subject;\n+\tstrbuf_reset(&subject);\n+\tfmt_output_email_subject(&subject, opt);\n+\t*subject_p = subject.buf;\n \t*extra_headers_p = extra_headers;\n }\n \ndiff --git a/log-tree.h b/log-tree.h\nindex c8116e60cd..dd75dd7770 100644\n--- a/log-tree.h\n+++ b/log-tree.h\n@@ -30,5 +30,6 @@ void load_ref_decorations(int flags);\n #define FORMAT_PATCH_NAME_MAX 64\n void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);\n void fmt_output_subject(struct strbuf *, const char *subject, struct rev_info *);\n+void fmt_output_email_subject(struct strbuf *, struct rev_info *);\n \n #endif\n-- \n2.12.0\n\n"},{"id":"312958","messageId":"xmqqfuiwhnud.fsf@gitster.mtv.corp.google.com","threadId":"45253","inReplyTo":"58e05599-5dc4-9881-d8c0-89ad1f2e3838@web.de","subject":"Re: [PATCH 2/2] pretty: use fmt_output_email_subject()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-01T18:08:10Z","receivedAt":"2017-03-01T18:23:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Add the email-style subject prefix (e.g. \"Subject: [PATCH] \") directly\n> when it's needed instead of letting log_write_email_headers() prepare\n> it in a static buffer in advance.  This simplifies storage ownership and\n> code flow.\n\nIt certainly does.  At first I wondered if there are places other\nthan log-write-email-headers that sets its own string to pp.subject,\nbut this patch removes the field from the structure to guarantee\nthat such a place, if existed, is properly dealt with.  Otherwise,\nthe resulting code would not compile.\n\n> @@ -1005,6 +1004,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n>  \tmsg = body;\n>  \tpp.fmt = CMIT_FMT_EMAIL;\n>  \tpp.date_mode.type = DATE_RFC2822;\n> +\tpp.rev = rev;\n> +\tpp.print_email_subject = 1;\n\nThese two are always set together?  We'll shortly see that it is not\nthe case below.\n\n>  \tpp_user_info(&pp, NULL, &sb, committer, encoding);\n>  \tpp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);\n>  \tpp_remainder(&pp, &msg, &sb, 0);\n> diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n> index c9585d475d..f78bb4818d 100644\n> --- a/builtin/shortlog.c\n> +++ b/builtin/shortlog.c\n> @@ -148,7 +148,7 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit)\n>  \n>  \tctx.fmt = CMIT_FMT_USERFORMAT;\n>  \tctx.abbrev = log->abbrev;\n> -\tctx.subject = \"\";\n> +\tctx.print_email_subject = 1;\n\nHere we see .rev is left to NULL here, with print_email_subject set\nto true.  And in the entire patch this is the only such place.\n\n> diff --git a/pretty.c b/pretty.c\n> index 5e683830d9..d0f86f5d85 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1607,8 +1607,9 @@ void pp_title_line(struct pretty_print_context *pp,\n>  \t\t\t\tpp->preserve_subject ? \"\\n\" : \" \");\n>  \n>  \tstrbuf_grow(sb, title.len + 1024);\n> -\tif (pp->subject) {\n> -\t\tstrbuf_addstr(sb, pp->subject);\n> +\tif (pp->print_email_subject) {\n> +\t\tif (pp->rev)\n> +\t\t\tfmt_output_email_subject(sb, pp->rev);\n\nA hidden assumption this code makes is that anybody who does not\nwant .rev (aka \"doing it as part of format-patch that may want\nnr/total etc\") does not want _any_ \"Subject: \".  It obviously holds\ntrue in today's code (the one in shortlog-add-commit is the only one\nand it sets an empty string to .subject).\n\nDoes the loss of flexibility to the future callers matter, though?\nI cannot tell offhand.\n\nThanks.  Let's see what others think.\n"},{"id":"312966","messageId":"20170301192230.fslurnghqvktuyou@sigill.intra.peff.net","threadId":"45253","inReplyTo":"58e05599-5dc4-9881-d8c0-89ad1f2e3838@web.de","subject":"Re: [PATCH 2/2] pretty: use fmt_output_email_subject()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-01T19:22:30Z","receivedAt":"2017-03-01T19:24:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 01, 2017 at 12:37:07PM +0100, René Scharfe wrote:\n\n> Add the email-style subject prefix (e.g. \"Subject: [PATCH] \") directly\n> when it's needed instead of letting log_write_email_headers() prepare\n> it in a static buffer in advance.  This simplifies storage ownership and\n> code flow.\n\nThis looks much cleaner to me.\n\nI suspect we can do the same thing with the mime headers below there.\nThey end up in extra_headers, which is shown via after_subject. But we\ncould mostly replace after_subject with a call to fmt_output_email_mime()\nor similar.\n\nThe only other use of extra_headers seems to be the static to/cc fields\none can add via format-patch. But those _should_ be treated differently,\nas they can be allocated once in the rev_info, not per-commit. Which I\nthink shows off another bug. If you have a large to/cc list, that all\ngets lumped into the same 1024-byte buffer, and may cause truncation.\n\nI think the diffopt.stat_sep thing could get similar handling, too. It\nappears to be set only in this one spot, and gets looked at in exactly\none. That could be replaced with an on-the-fly function call.\n\n> This slows down the last three tests in p4000 by ca. 3% for some reason,\n> so we may want to only do the first part for now, which is performance\n> neutral on my machine.\n\nIt sounds like the bitfield was the cause, so that should be an easy\nfix. The other question is whether it makes \"--format=email\" any slower.\nIt shouldn't, as your new approach doesn't do any extra per-commit\nallocations (and in fact, it avoids some useless buffer-copying).\n\nI couldn't measure any difference.\n\n-Peff\n"},{"id":"312968","messageId":"20170301193210.tgvnjgc2hfaeebqf@sigill.intra.peff.net","threadId":"45253","inReplyTo":"xmqqfuiwhnud.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] pretty: use fmt_output_email_subject()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-01T19:32:10Z","receivedAt":"2017-03-01T19:39:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 01, 2017 at 10:08:10AM -0800, Junio C Hamano wrote:\n\n> >  \tstrbuf_grow(sb, title.len + 1024);\n> > -\tif (pp->subject) {\n> > -\t\tstrbuf_addstr(sb, pp->subject);\n> > +\tif (pp->print_email_subject) {\n> > +\t\tif (pp->rev)\n> > +\t\t\tfmt_output_email_subject(sb, pp->rev);\n> \n> A hidden assumption this code makes is that anybody who does not\n> want .rev (aka \"doing it as part of format-patch that may want\n> nr/total etc\") does not want _any_ \"Subject: \".  It obviously holds\n> true in today's code (the one in shortlog-add-commit is the only one\n> and it sets an empty string to .subject).\n> \n> Does the loss of flexibility to the future callers matter, though?\n> I cannot tell offhand.\n> \n> Thanks.  Let's see what others think.\n\nI would think that future callers would just need to provide a dummy\npp->rev. I guess that logic could be pushed down into\nfmt_output_email_subject(), so that it skips looking at\nopt->subject_prefix, etc, when \"opt\" is NULL, and just hits the\n\"Subject:\" case arm.\n\nI don't think it's a big deal, but it would be easy to fix now, like:\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 4618dd04c..c73df6857 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -334,13 +334,13 @@ void fmt_output_commit(struct strbuf *filename,\n \n void fmt_output_email_subject(struct strbuf *sb, struct rev_info *opt)\n {\n-\tif (opt->total > 0) {\n+\tif (opt && opt->total > 0) {\n \t\tstrbuf_addf(sb, \"Subject: [%s%s%0*d/%d] \",\n \t\t\t    opt->subject_prefix,\n \t\t\t    *opt->subject_prefix ? \" \" : \"\",\n \t\t\t    digits_in_number(opt->total),\n \t\t\t    opt->nr, opt->total);\n-\t} else if (opt->total == 0 && opt->subject_prefix && *opt->subject_prefix) {\n+\t} else if (opt && opt->total == 0 && opt->subject_prefix && *opt->subject_prefix) {\n \t\tstrbuf_addf(sb, \"Subject: [%s] \",\n \t\t\t    opt->subject_prefix);\n \t} else {\ndiff --git a/pretty.c b/pretty.c\nindex d0f86f5d8..6b321c68c 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1608,8 +1608,7 @@ void pp_title_line(struct pretty_print_context *pp,\n \n \tstrbuf_grow(sb, title.len + 1024);\n \tif (pp->print_email_subject) {\n-\t\tif (pp->rev)\n-\t\t\tfmt_output_email_subject(sb, pp->rev);\n+\t\tfmt_output_email_subject(sb, pp->rev);\n \t\tif (needs_rfc2047_encoding(title.buf, title.len, RFC2047_SUBJECT))\n \t\t\tadd_rfc2047(sb, title.buf, title.len,\n \t\t\t\t\t\tencoding, RFC2047_SUBJECT);\n\n-Peff\n"},{"id":"312969","messageId":"CAPc5daUyu_=5GeGqBAG_KvkZFE_0oD6Wa_HhoRQwZ2ANUwOF+Q@mail.gmail.com","threadId":"45253","inReplyTo":"20170301193210.tgvnjgc2hfaeebqf@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] pretty: use fmt_output_email_subject()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-01T19:38:57Z","receivedAt":"2017-03-01T19:41:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Wed, Mar 1, 2017 at 11:32 AM, Jeff King <peff@peff.net> wrote:\n> I would think that future callers would just need to provide a dummy\n> pp->rev. I guess that logic could be pushed down into\n> fmt_output_email_subject(), so that it skips looking at\n> opt->subject_prefix, etc, when \"opt\" is NULL, and just hits the\n> \"Subject:\" case arm.\n\nThe \"flexibility\" I was wondering about is that the current .subject can\npoint at any caller-supplied string, not \"Subject:\".\n"},{"id":"313010","messageId":"373c7cca-3fd8-bf25-f45b-16249f160ee5@web.de","threadId":"45253","inReplyTo":"58e05599-5dc4-9881-d8c0-89ad1f2e3838@web.de","subject":"Re: [PATCH 2/2] pretty: use fmt_output_email_subject()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-03-01T15:41:36Z","receivedAt":"2017-03-02T00:08:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.03.2017 um 12:37 schrieb René Scharfe:\n> Add the email-style subject prefix (e.g. \"Subject: [PATCH] \") directly\n> when it's needed instead of letting log_write_email_headers() prepare\n> it in a static buffer in advance.  This simplifies storage ownership and\n> code flow.\n>\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n> This slows down the last three tests in p4000 by ca. 3% for some reason,\n> so we may want to only do the first part for now, which is performance\n> neutral on my machine.\n\nUpdate below.\n\n> diff --git a/commit.h b/commit.h\n> index 9c12abb911..459daef94a 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -142,21 +142,24 @@ static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)\n>  \treturn (fmt == CMIT_FMT_EMAIL || fmt == CMIT_FMT_MBOXRD);\n>  }\n>\n> +struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */\n> +\n>  struct pretty_print_context {\n>  \t/*\n>  \t * Callers should tweak these to change the behavior of pp_* functions.\n>  \t */\n>  \tenum cmit_fmt fmt;\n>  \tint abbrev;\n> -\tconst char *subject;\n>  \tconst char *after_subject;\n>  \tint preserve_subject;\n>  \tstruct date_mode date_mode;\n>  \tunsigned date_mode_explicit:1;\n> +\tunsigned print_email_subject:1;\n\nTurning this into an int restores performance according to p4000. \nDidn't know that bitfields can be *that* expensive.\n\nRené\n"},{"id":"313023","messageId":"20170301194340.5phhr7lqdqiji277@sigill.intra.peff.net","threadId":"45253","inReplyTo":"CAPc5daUyu_=5GeGqBAG_KvkZFE_0oD6Wa_HhoRQwZ2ANUwOF+Q@mail.gmail.com","subject":"Re: [PATCH 2/2] pretty: use fmt_output_email_subject()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-01T19:43:40Z","receivedAt":"2017-03-02T01:10:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 01, 2017 at 11:38:57AM -0800, Junio C Hamano wrote:\n\n> On Wed, Mar 1, 2017 at 11:32 AM, Jeff King <peff@peff.net> wrote:\n> > I would think that future callers would just need to provide a dummy\n> > pp->rev. I guess that logic could be pushed down into\n> > fmt_output_email_subject(), so that it skips looking at\n> > opt->subject_prefix, etc, when \"opt\" is NULL, and just hits the\n> > \"Subject:\" case arm.\n> \n> The \"flexibility\" I was wondering about is that the current .subject can\n> point at any caller-supplied string, not \"Subject:\".\n\nAh, I see. I don't think that is a huge loss, as nobody was using it.\nAnd \"Subject:\" is already hard-coded in the nr/total counting bits,\nwhich are what you'd want to reuse. I think it is fine to punt to the\nfuture. If somebody really wants it later, the right fix is for them to\nprovide a string that fmt_output_email_subject() would use in place of\n\"Subject:\" when it adds to the strbuf.\n\n-Peff\n"}]}