René Scharfe <[email protected]> writes:
> Add the email-style subject prefix (e.g. "Subject: [PATCH] ") directly
> when it's needed instead of letting log_write_email_headers() prepare
> it in a static buffer in advance. This simplifies storage ownership and
> code flow.
It certainly does. At first I wondered if there are places other
than log-write-email-headers that sets its own string to pp.subject,
but this patch removes the field from the structure to guarantee
that such a place, if existed, is properly dealt with. Otherwise,
the resulting code would not compile.
> @@ -1005,6 +1004,8 @@ static void make_cover_letter(struct rev_info *rev, int
> use_stdout,
> msg = body;
> pp.fmt = CMIT_FMT_EMAIL;
> pp.date_mode.type = DATE_RFC2822;
> + pp.rev = rev;
> + pp.print_email_subject = 1;
These two are always set together? We'll shortly see that it is not
the case below.
> pp_user_info(&pp, NULL, &sb, committer, encoding);
> pp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);
> pp_remainder(&pp, &msg, &sb, 0);
> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index c9585d475d..f78bb4818d 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -148,7 +148,7 @@ void shortlog_add_commit(struct shortlog *log, struct
> commit *commit)
>
> ctx.fmt = CMIT_FMT_USERFORMAT;
> ctx.abbrev = log->abbrev;
> - ctx.subject = "";
> + ctx.print_email_subject = 1;
Here we see .rev is left to NULL here, with print_email_subject set
to true. And in the entire patch this is the only such place.
> diff --git a/pretty.c b/pretty.c
> index 5e683830d9..d0f86f5d85 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1607,8 +1607,9 @@ void pp_title_line(struct pretty_print_context *pp,
> pp->preserve_subject ? "\n" : " ");
>
> strbuf_grow(sb, title.len + 1024);
> - if (pp->subject) {
> - strbuf_addstr(sb, pp->subject);
> + if (pp->print_email_subject) {
> + if (pp->rev)
> + fmt_output_email_subject(sb, pp->rev);
A hidden assumption this code makes is that anybody who does not
want .rev (aka "doing it as part of format-patch that may want
nr/total etc") does not want _any_ "Subject: ". It obviously holds
true in today's code (the one in shortlog-add-commit is the only one
and it sets an empty string to .subject).
Does the loss of flexibility to the future callers matter, though?
I cannot tell offhand.
Thanks. Let's see what others think.
