Re: [PATCH v2 1/2] format-patch: add ability to use alt cover format

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

Mirko Faina <mroik@delayed.space> writes:

> Often when sending patch series there's a need to clarify to the
> reviewer what's the purpose of said series, since it might be difficult
> to understand it from reading the commits messages one by one.
>
> "git format-patch" provides the useful "--cover-letter" flag to declare
> if we want it to generate a template for us to use. By default it will
> generate a "git shortlog" of the changes, which developers find less
> useful than they'd like, mainly because the shortlog groups commits by
> author, and gives no obvious chronological order.
>
> Give the ability to format-patch to specify an alternative format spec
> through the "--cover-letter-format" option. This option either takes
> "shortlog", which is the current format, or a format spec prefixed with
> "log:".
>
> Example:
>     git format-patch --cover-letter \
>         --cover-letter-format="log:%s (%an)" HEAD~3
>
>     [1/3] this is a commit summary (Mirko Faina)
>     [2/3] this is another commit summary (Mirko Faina)
>     ...
>
> Signed-off-by: Mirko Faina <mroik@delayed.space>
> ---
>  builtin/log.c | 65 ++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index c1cd3999a7..5e99660d7c 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1324,13 +1324,32 @@ static void get_notes_args(struct strvec *arg, struct rev_info *rev)
>  	}
>  }
>  
> +static void generate_commit_list_cover(FILE *cover_file,const char *format,

"cover_file,const" -> "cover_file, const" (missing SP).

> +				       struct commit **list, int n)
> +{
> +	struct strbuf commit_line = STRBUF_INIT;
> +	struct pretty_print_context ctx = {0};
> +
> +	strbuf_init(&commit_line, 0);

So a single commit_line is given to repo_format_commit_message()
repeatedly to accumulate those lines in it, which makes sense.

We prepare pretty_print_context once above and feed it repeatedly to
repo_format_commit_message()---is that intended?  Not a rhetorical
question; I do not know the answer.  I guess some existing callers
(like builtin/archive.c) do reuse the structure when making multiple
calls to the function, so this would be kosher, perhaps?

> +	for (int i = n - 1; i >= 0; i--) {
> +		strbuf_addf(&commit_line, "[%0*d/%d] ", decimal_width(n), n - i, n);
> +		repo_format_commit_message(the_repository, list[i], format, &commit_line, &ctx);

Let's line-wrap this overly long line (my lithmus test to complain
about "overly long lines" is after losing three columns to the left
for "> +" in e-mail quote like the above the right edge of the line
does not fit on my 92-column terminal, which will never happen if
you stick to the official "fit 80-column after quoted for a few
times in e-mail" guideline).

		repo_format_commit_message(the_repository, list[i], format,
					   &commit_line, &ctx);

perhaps.

> +		fprintf(cover_file, "%s\n", commit_line.buf);

I somehow would have expected that as we internally prepare "format"
string given to this function , we ensure it ends with "\n" so we do
not have to do a fprintf() here.

> +		strbuf_reset(&commit_line);
> +	}
> +	fprintf(cover_file, "\n");

OK, so we have a blank line after a block of one line per commit.
Sensible.

> +	strbuf_release(&commit_line);
> +}
> +
>  static void make_cover_letter(struct rev_info *rev, int use_separate_file,
>  			      struct commit *origin,
>  			      int nr, struct commit **list,
>  			      const char *description_file,
>  			      const char *branch_name,
>  			      int quiet,
> -			      const struct format_config *cfg)
> +			      const struct format_config *cfg,
> +			      const char *format)
>  {
>  	const char *from;
>  	struct shortlog log;
> @@ -1342,6 +1361,8 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
>  	struct commit *head = list[0];
>  	char *to_free = NULL;
>  
> +	assert(format);
> +

Curious.  We do not assert() for any other pointer arguments.  What
makes this so special?

>  	if (!cmit_fmt_is_mail(rev->commit_format))
>  		die(_("cover letter needs email format"));
>  
> @@ -1377,18 +1398,22 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
>  	free(pp.after_subject);
>  	strbuf_release(&sb);
>  
> -	shortlog_init(&log);
> -	log.wrap_lines = 1;
> -	log.wrap = MAIL_DEFAULT_WRAP;
> -	log.in1 = 2;
> -	log.in2 = 4;
> -	log.file = rev->diffopt.file;
> -	log.groups = SHORTLOG_GROUP_AUTHOR;
> -	shortlog_finish_setup(&log);
> -	for (i = 0; i < nr; i++)
> -		shortlog_add_commit(&log, list[i]);

It would have been much nicer to first create a short helper
function generate_shortlog_cover(), move the above plus a call to
shortlog_output(&log) to that helper function, without doing
anything else and make it a preliminary patch [1/n].  Then introduce
the corresponding generate_commit_list_cover() function in patch
[2/n] (which is what this step is doing), which would have resulted
in the body of the make_cover_letter() around here a short-and-sweet

	if (... we are doing shortlog style ...)
		generate_shortlog_cover(...);
	else
		generate_commit_list_cover(...);

Just like readers of _this_ function are helped by not having to
know the details of what happens inside commit-list style cover
generation, they can concentratre on the flow without having to care
about details on shortlog side that way.

> @@ -1906,6 +1931,7 @@ int cmd_format_patch(int argc,
>  	int just_numbers = 0;
>  	int ignore_if_in_upstream = 0;
>  	int cover_letter = -1;
> +	char *cover_letter_fmt = NULL;
>  	int boundary_count = 0;
>  	int no_binary_diff = 0;
>  	int zero_commit = 0;
> @@ -1952,6 +1978,8 @@ int cmd_format_patch(int argc,
>  			    N_("print patches to standard out")),
>  		OPT_BOOL(0, "cover-letter", &cover_letter,
>  			    N_("generate a cover letter")),
> +		OPT_STRING(0, "cover-letter-format", &cover_letter_fmt, N_("format-spec"),
> +			    N_("format spec used for the commit list in the cover letter")),
>  		OPT_BOOL(0, "numbered-files", &just_numbers,
>  			    N_("use simple number sequence for output file names")),
>  		OPT_STRING(0, "suffix", &fmt_patch_suffix, N_("sfx"),
> @@ -2289,13 +2317,14 @@ int cmd_format_patch(int argc,
>  		/* nothing to do */
>  		goto done;
>  	total = list.nr;
> +

What is this churn about?

>  	if (cover_letter == -1) {
>  		if (cfg.config_cover_letter == COVER_AUTO)
> -			cover_letter = (total > 1);
> +			cover_letter = total > 1;

What is this churn about?

>  		else if ((idiff_prev.nr || rdiff_prev) && (total > 1))
> -			cover_letter = (cfg.config_cover_letter != COVER_OFF);
> +			cover_letter = cfg.config_cover_letter != COVER_OFF;

What is this churn about?

>  		else
> -			cover_letter = (cfg.config_cover_letter == COVER_ON);
> +			cover_letter = cfg.config_cover_letter == COVER_ON;

What is this churn about?

Please do not distract reviewers by mixing immaterial "style fixes"
on existing code to a patch whose primary purpose is to introduce
new code.  A separate "preliminary fix and/or clean-up" patch before
the main series begins is often a welcome addition, though.


> @@ -2375,12 +2404,16 @@ int cmd_format_patch(int argc,
>  	}
>  	rev.numbered_files = just_numbers;
>  	rev.patch_suffix = fmt_patch_suffix;
> +
> +	if (cover_letter && !cover_letter_fmt)
> +		cover_letter_fmt = "shortlog";
> +

I do not quite see the point of doing this.  There is no law that
"cover_letter_fmt != NULL" is a crime when cover_letter is false.

cover_letter_fmt can be initialized to a fixed string "shortlog",
and nobody cares what random value cover_letter_fmt has when
cover_letter is false.

>  	if (cover_letter) {
>  		if (cfg.thread)
>  			gen_message_id(&rev, "cover");
>  		make_cover_letter(&rev, !!output_directory,
>  				  origin, list.nr, list.items,
> -				  description_file, branch_name, quiet, &cfg);
> +				  description_file, branch_name, quiet, &cfg, cover_letter_fmt);

This line has become overly long.  Please wrap it.

>  		print_bases(&bases, rev.diffopt.file);
>  		print_signature(signature, rev.diffopt.file);
>  		total++;

In any case, the implementation in this iteration looks much nicer
than the previous one.

New set of tests should come together with implementation of a new
feature in the same patch.

Thanks.