Re: [PATCH v5 3/3] format-patch: teach --cover-from-description option

[Junio C Hamano <gitster@pobox.com>]

Denton Liu <liu.denton@gmail.com> writes:

> diff --git a/builtin/log.c b/builtin/log.c
> index d212a8305d..af33fe9ffb 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1057,6 +1072,44 @@ static void show_diffstat(struct rev_info *rev,
>  	fprintf(rev->diffopt.file, "\n");
>  }
>  
> +static void pp_from_desc(struct pretty_print_context *pp,
> +			 const char *branch_name,
> +			 struct strbuf *sb,
> +			 const char *encoding,
> +			 int need_8bit_cte)
> +{
> +	const char *subject = "*** SUBJECT HERE ***";
> +	const char *body = "*** BLURB HERE ***";
> +	struct strbuf description_sb = STRBUF_INIT;
> +	struct strbuf subject_sb = STRBUF_INIT;
> +
> +	if (cover_from_description_mode == COVER_FROM_NONE)
> +		goto do_pp;
> +
> +	if (branch_name && *branch_name)
> +		read_branch_desc(&description_sb, branch_name);
> +	if (!description_sb.len)
> +		goto do_pp;
> +
> +	if (cover_from_description_mode == COVER_FROM_SUBJECT ||
> +			cover_from_description_mode == COVER_FROM_AUTO)
> +		body = format_subject(&subject_sb, description_sb.buf, " ");
> +
> +	if (cover_from_description_mode == COVER_FROM_MESSAGE ||
> +			(cover_from_description_mode == COVER_FROM_AUTO &&
> +			 subject_sb.len > COVER_FROM_AUTO_MAX_SUBJECT_LEN))
> +		body = description_sb.buf;
> +	else
> +		subject = subject_sb.buf;
> +
> +do_pp:
> +	pp_title_line(pp, &subject, sb, encoding, need_8bit_cte);
> +	pp_remainder(pp, &body, sb, 0);
> +
> +	strbuf_release(&description_sb);
> +	strbuf_release(&subject_sb);
> +}
> +

This implementation is very clear and easy to follow, and ...

> @@ -1064,8 +1117,6 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  			      int quiet)
>  {
>  	const char *committer;
>  	struct shortlog log;
>  	struct strbuf sb = STRBUF_INIT;
>  	int i;
> @@ -1095,15 +1146,12 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  	if (!branch_name)
>  		branch_name = find_branch_name(rev);
>  
>  	pp.fmt = CMIT_FMT_EMAIL;
>  	pp.date_mode.type = DATE_RFC2822;
>  	pp.rev = rev;
>  	pp.print_email_subject = 1;
>  	pp_user_info(&pp, NULL, &sb, committer, encoding);
> +	pp_from_desc(&pp, branch_name, &sb, encoding, need_8bit_cte);
>  	fprintf(rev->diffopt.file, "%s\n", sb.buf);
>  
>  	strbuf_release(&sb);

... made the caller much simpler.

One large nit is that pp_user_info() is about "pretty printing the
user info", pp_title_line() is about "pretty printing the title
line", but pp_from_desc() is not about "pretty printing the from
desc".  Naming matters.

How about calling it with more emphasis on what it does (i.e. the
helper is about preparing the subject and body of the cover letter
e-mail) and less emphasis on how it does or what it bases its
decision on?  prepare_cover_text() or soemthing, perhaps?

Other than that, this version is very much more preferrable than the
previous one.  Quite nicely done.

Thanks.