Re: format-patch subject-prefix gets truncated when using the --numbered flag
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 28, 2017, 19:33 UTC
- Message-ID
- <xmqqzih6jejg.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <xmqqinnuky9e.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
> Adrian Dudau <Adrian.Dudau@enea.com> writes: > >> I noticed that the --subject-prefix string gets truncated sometimes, >> but only when using the --numbered flat. Here's an example: >> >> addu@sestofb11:/data/fb/addu/git$ export longm="very very very very >> very very very very very very very very very very long prefix" > > This looks like "dr, my arm hurts when I twist it this way---don't > do that then" ;-). Truncation is designed and intended to preserve > space for the real title of commit. > > Having said that... > ... > I think this is just an old oversight. The latter should have been > even shorter than the former one (or the former should be longer > than the latter) to account for the width of the counter e.g. 01/27.
And having said all that, if we really want to allow overlong subject prefix that would end up hiding the real title of the patch, a modern way to do so would be to use xstrfmt() like the attached toy-patch does. Note that this is merely a small first step---you'd notice that "subject" is kept around as a "static" and only freed upon entry to this function for the second time, to preserve the ownership model of the original code. In a real "fix" (if this needs to be "fixed", that is), I think the ownership model of the storage used for *subject_p and *extra_headers_p needs to be updated so that it will become caller's responsibility to free them (similarly, the ownership model of opt->diffopt.stat_sep that is assigned the address of the static buffer[] in the same function needs to be revisited).
That "buffer" thing I think would need to be a bit more careful even in the current code, which _does_ use snprintf() correctly to avoid overflowing the buffer[], by the way. If you have an overlong opt->mime_boundary, the resulting "e-mail" looking output can become structurely broken. The truncation may happen way before the full line for Content-Transfer-Encoding: is written, for example.
So this function seems to have a lot more graver problems that need to be looked at.
log-tree.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/log-tree.c b/log-tree.c index 8c2415747a..24c98f5a80 100644 --- a/log-tree.c +++ b/log-tree.c @@ -337,29 +337,23 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit, const char **extra_headers_p, int *need_8bit_cte_p) { - const char *subject = NULL; + static const char *subject = NULL; const char *extra_headers = opt->extra_headers; const char *name = oid_to_hex(opt->zero_commit ? &null_oid : &commit->object.oid); + free((void *)subject); *need_8bit_cte_p = 0; /* unknown */ if (opt->total > 0) { - static char buffer[64]; - snprintf(buffer, sizeof(buffer), - "Subject: [%s%s%0*d/%d] ", - opt->subject_prefix, - *opt->subject_prefix ? " " : "", - digits_in_number(opt->total), - opt->nr, opt->total); - subject = buffer; + subject = xstrfmt("Subject: [%s%s%0*d/%d] ", + opt->subject_prefix, + *opt->subject_prefix ? " " : "", + digits_in_number(opt->total), + opt->nr, opt->total); } else if (opt->total == 0 && opt->subject_prefix && *opt->subject_prefix) { - static char buffer[256]; - snprintf(buffer, sizeof(buffer), - "Subject: [%s] ", - opt->subject_prefix); - subject = buffer; + subject = xstrfmt("Subject: [%s] ", opt->subject_prefix); } else { - subject = "Subject: "; + subject = xstrdup("Subject: "); } fprintf(opt->diffopt.file, "From %s Mon Sep 17 00:00:00 2001\n", name);