Re: [PATCH 2/2] pretty: use fmt_output_email_subject()

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 1, 2017, 18:08 UTC
Message-ID: <xmqqfuiwhnud.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <58e05599-5dc4-9881-d8c0-89ad1f2e3838@web.de>

René Scharfe <l.s.r@web.de> 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.

Show 6 quoted lines

> @@ -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.

Show 13 quoted lines

>  	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.

Show 13 quoted lines

> 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.