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

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