patch, 4 partsAdd a --cover-letter option to format-patch

> If --cover-letter is provided, generate a cover letter message before
> the patches, numbered 0.
> 
> Original patch thanks to Johannes Schindelin
> 
> Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
> ---
> +static int reopen_stdout(const char *oneline, int nr, int total)
>  {
>  	char filename[PATH_MAX];
> -	char *sol;
>  	int len = 0;
>  	int suffix_len = strlen(fmt_patch_suffix) + 1;
>  
>  	if (output_directory) {
> -		if (strlen(output_directory) >=
> +		len = snprintf(filename, sizeof(filename), "%s",
> +				output_directory);
> +		if (len >=
>  		    sizeof(filename) - FORMAT_PATCH_NAME_MAX - suffix_len)
>  			return error("name of output directory is too long");
> -		strlcpy(filename, output_directory, sizeof(filename) - suffix_len);
> -		len = strlen(filename);
>  		if (filename[len - 1] != '/')
>  			filename[len++] = '/';
>  	}
>  

> +	else {
> +		len += sprintf(filename + len, "%04d-", nr);
> +		len += snprintf(filename + len, sizeof(filename) - len - 1
> +				- suffix_len, "%s", oneline);
>  		strcpy(filename + len, fmt_patch_suffix);
>  	}

> +static void make_cover_letter(struct rev_info *rev,
> +		int use_stdout, int numbered, int numbered_files,
> +			      struct commit *origin, struct commit *head)
> +{
> +	const char *committer;
> +	const char *origin_sha1, *head_sha1;
> +	const char *argv[7];
> +	const char *subject_start = NULL;
> +	const char *body = "*** SUBJECT HERE ***\n\n\n*** BLURB HERE ***\n";

> On Mittwoch 06 Februar 2008, Daniel Barkalow wrote:
> > If --cover-letter is provided, generate a cover letter message before
> > the patches, numbered 0.
> > 
> > Original patch thanks to Johannes Schindelin
> > 
> > Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
> > ---
> > +static int reopen_stdout(const char *oneline, int nr, int total)
> >  {
> >  	char filename[PATH_MAX];
> > -	char *sol;
> >  	int len = 0;
> >  	int suffix_len = strlen(fmt_patch_suffix) + 1;
> >  
> >  	if (output_directory) {
> > -		if (strlen(output_directory) >=
> > +		len = snprintf(filename, sizeof(filename), "%s",
> > +				output_directory);
> > +		if (len >=
> >  		    sizeof(filename) - FORMAT_PATCH_NAME_MAX - suffix_len)
> >  			return error("name of output directory is too long");
> > -		strlcpy(filename, output_directory, sizeof(filename) - suffix_len);
> > -		len = strlen(filename);
> >  		if (filename[len - 1] != '/')
> >  			filename[len++] = '/';
> >  	}
> >  
> *snip*
> > +	if (!filename)
> > +		len += sprintf(filename + len, "%d", nr);
> maybe this should be !oneline instead?

> > +	else {
> > +		len += sprintf(filename + len, "%04d-", nr);
> > +		len += snprintf(filename + len, sizeof(filename) - len - 1
> > +				- suffix_len, "%s", oneline);
> >  		strcpy(filename + len, fmt_patch_suffix);
> >  	}
> 
> > +static void make_cover_letter(struct rev_info *rev,
> > +		int use_stdout, int numbered, int numbered_files,
> > +			      struct commit *origin, struct commit *head)
> > +{
> > +	const char *committer;
> > +	const char *origin_sha1, *head_sha1;
> > +	const char *argv[7];
> > +	const char *subject_start = NULL;
> > +	const char *body = "*** SUBJECT HERE ***\n\n\n*** BLURB HERE ***\n";
> I don't know git policy but maybe use
> const char body[] = "*** SUBJECT HERE ***\n\n\n*** BLURB HERE ***\n";
> since you don't change the pointer.
> (or remove the variable)

> > +	const char *msg;
> > +	const char *extra_headers = rev->extra_headers;
> > +	struct strbuf sb;
> > +	const char *encoding = "utf-8";
> same here
> > +
> > +	if (rev->commit_format != CMIT_FMT_EMAIL)
> > +		die("Cover letter needs email format");
> 
> It might be useful to split the reopen_stdout/get_oneline_for_filename
> into a separate patch.

> Greetings Peter
> Who tried to create this --cover-letter function
> but gave up silently when his patch mails never reached the ML :-(
> 
> I don't know if my patches are of any use (do not apply cleanly anymore, 
> reading cover letter message from a file does not honor encoding in any way)
> But i can send them to you if you want...

> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index 651efe6..7ec01a0 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -17,6 +17,7 @@ SYNOPSIS
>                     [--in-reply-to=Message-Id] [--suffix=.<sfx>]
>                     [--ignore-if-in-upstream]
>                     [--subject-prefix=Subject-Prefix]
> +                   [--cover-letter]
>  		   [ <since> | <revision range> ]
>  
>  DESCRIPTION
> @@ -139,6 +140,11 @@ include::diff-options.txt[]
>  	Instead of using `.patch` as the suffix for generated
>  	filenames, use specified suffix.  A common alternative is
>  	`--suffix=.txt`.
> +
> +--cover-letter::
> +	Generate a cover letter template.  You still have to fill in
> +	a description, but the shortlog and the diffstat will be
> +	generated for you.
>  +
>  Note that you would need to include the leading dot `.` if you
>  want a filename like `0001-description-of-my-change.patch`, and

> diff --git a/builtin-log.c b/builtin-log.c
> index 1f74d66..c6e3f91 100644
> --- a/builtin-log.c
> +++ b/builtin-log.c
> @@ -453,74 +454,77 @@ static int git_format_config(...
> ...
>  static FILE *realstdout = NULL;
>  static const char *output_directory = NULL;
>  
> +static int reopen_stdout(const char *oneline, int nr, int total)
>  {
>  	char filename[PATH_MAX];
>  	int len = 0;
>  	int suffix_len = strlen(fmt_patch_suffix) + 1;
>  
>  	if (output_directory) {
> +		len = snprintf(filename, sizeof(filename), "%s",
> +				output_directory);
> +		if (len >=
>  		    sizeof(filename) - FORMAT_PATCH_NAME_MAX - suffix_len)
>  			return error("name of output directory is too long");
>  		if (filename[len - 1] != '/')
>  			filename[len++] = '/';
>  	}
>  
> +	if (!filename)
> +		len += sprintf(filename + len, "%d", nr);

> +	else {
> +		len += sprintf(filename + len, "%04d-", nr);
> +		len += snprintf(filename + len, sizeof(filename) - len - 1
> +				- suffix_len, "%s", oneline);
>  		strcpy(filename + len, fmt_patch_suffix);
>  	}
>  
> @@ -591,6 +595,74 @@ static void gen_message_id(struct rev_info *info, char *base)
> +	/*
> +	 * We can only do diffstat with a unique reference point, and
> +	 * log is a bit tricky, so just skip it.
> +	 */
> +	if (!origin)
> +		return;

> +	argv[0] = "shortlog";
> +	argv[1] = head_sha1;
> +	argv[2] = "--not";
> +	argv[3] = origin_sha1;
> +	argv[4] = NULL;
> +	fflush(stdout);
> +	run_command_v_opt(argv, RUN_GIT_CMD);
> +
> +	argv[0] = "diff";
> +	argv[1] = "--stat";
> +	argv[2] = "--summary";
> +	argv[3] = head_sha1;
> +	argv[4] = "--not";
> +	argv[5] = origin_sha1;
> +	argv[6] = NULL;
> +	fflush(stdout);
> +	run_command_v_opt(argv, RUN_GIT_CMD);

> @@ -776,6 +852,20 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		 * get_revision() to do the usual traversal.
>  		 */
>  	}
> +	if (cover_letter) {
> +		/* remember the range */
> +		int i;
> +		for (i = 0; i < rev.pending.nr; i++) {
> +			struct object *o = rev.pending.objects[i].item;
> +			if (o->flags & UNINTERESTING)
> +				origin = (struct commit *)o;
> +			else
> +				head = (struct commit *)o;
> +		}

> @@ -802,9 +892,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		numbered = 1;
>  	if (numbered)
>  		rev.total = total + start_number - 1;
> -	rev.add_signoff = add_signoff;
>  	if (in_reply_to)
>  		rev.ref_message_id = clean_message_id(in_reply_to);
> +	if (cover_letter) {
> +		if (thread) {
> +			gen_message_id(&rev, "cover");
> +		}
> +		make_cover_letter(&rev, use_stdout, numbered, numbered_files,
> +				  origin, head);
> +		total++;
> +		start_number--;
> +	}
> +	rev.add_signoff = add_signoff;

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> > index 651efe6..7ec01a0 100644
> > --- a/Documentation/git-format-patch.txt
> > +++ b/Documentation/git-format-patch.txt
> > @@ -17,6 +17,7 @@ SYNOPSIS
> >                     [--in-reply-to=Message-Id] [--suffix=.<sfx>]
> >                     [--ignore-if-in-upstream]
> >                     [--subject-prefix=Subject-Prefix]
> > +                   [--cover-letter]
> >  		   [ <since> | <revision range> ]
> >  
> >  DESCRIPTION
> > @@ -139,6 +140,11 @@ include::diff-options.txt[]
> >  	Instead of using `.patch` as the suffix for generated
> >  	filenames, use specified suffix.  A common alternative is
> >  	`--suffix=.txt`.
> > +
> > +--cover-letter::
> > +	Generate a cover letter template.  You still have to fill in
> > +	a description, but the shortlog and the diffstat will be
> > +	generated for you.
> >  +
> >  Note that you would need to include the leading dot `.` if you
> >  want a filename like `0001-description-of-my-change.patch`, and
> 
> Huh?  Leading dot?  The insertion is one paragraph too low or high.

> > diff --git a/builtin-log.c b/builtin-log.c
> > index 1f74d66..c6e3f91 100644
> > --- a/builtin-log.c
> > +++ b/builtin-log.c
> > @@ -453,74 +454,77 @@ static int git_format_config(...
> > ...
> >  static FILE *realstdout = NULL;
> >  static const char *output_directory = NULL;
> >  
> > +static int reopen_stdout(const char *oneline, int nr, int total)
> >  {
> >  	char filename[PATH_MAX];
> >  	int len = 0;
> >  	int suffix_len = strlen(fmt_patch_suffix) + 1;
> >  
> >  	if (output_directory) {
> > +		len = snprintf(filename, sizeof(filename), "%s",
> > +				output_directory);
> > +		if (len >=
> >  		    sizeof(filename) - FORMAT_PATCH_NAME_MAX - suffix_len)
> >  			return error("name of output directory is too long");
> >  		if (filename[len - 1] != '/')
> >  			filename[len++] = '/';
> >  	}
> >  
> > +	if (!filename)
> > +		len += sprintf(filename + len, "%d", nr);
> 
> How can this trigger?  Do you mean "oneline"?

> I can understand that you do not want to pass numbered_files
> variable separately to the function, but then there should be a
> comment at the beginning of this function that says "oneline is
> NULL under numbered_files mode, the caller is responsible for
> ensuring that".

> > +	else {
> > +		len += sprintf(filename + len, "%04d-", nr);
> > +		len += snprintf(filename + len, sizeof(filename) - len - 1
> > +				- suffix_len, "%s", oneline);
> >  		strcpy(filename + len, fmt_patch_suffix);
> >  	}
> >  
> > @@ -591,6 +595,74 @@ static void gen_message_id(struct rev_info *info, char *base)
> > +	/*
> > +	 * We can only do diffstat with a unique reference point, and
> > +	 * log is a bit tricky, so just skip it.
> > +	 */
> > +	if (!origin)
> > +		return;
> 
> Maybe we would want to leave a note in the output to tell your
> users that we punted here.

> > +	argv[0] = "shortlog";
> > +	argv[1] = head_sha1;
> > +	argv[2] = "--not";
> > +	argv[3] = origin_sha1;
> > +	argv[4] = NULL;
> > +	fflush(stdout);
> > +	run_command_v_opt(argv, RUN_GIT_CMD);
> > +
> > +	argv[0] = "diff";
> > +	argv[1] = "--stat";
> > +	argv[2] = "--summary";
> > +	argv[3] = head_sha1;
> > +	argv[4] = "--not";
> > +	argv[5] = origin_sha1;
> > +	argv[6] = NULL;
> > +	fflush(stdout);
> > +	run_command_v_opt(argv, RUN_GIT_CMD);
> 
> Makes me wonder if we already have enough infrastructure to do
> this without spawning two new processes.  Not complaining, but
> just wondering.

> > @@ -776,6 +852,20 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >  		 * get_revision() to do the usual traversal.
> >  		 */
> >  	}
> > +	if (cover_letter) {
> > +		/* remember the range */
> > +		int i;
> > +		for (i = 0; i < rev.pending.nr; i++) {
> > +			struct object *o = rev.pending.objects[i].item;
> > +			if (o->flags & UNINTERESTING)
> > +				origin = (struct commit *)o;
> > +			else
> > +				head = (struct commit *)o;
> > +		}
> 
> What if you have more than one origin or head?  Perhaps the
> punting logic should be modified to make sure we only have one
> negative and one positive and nothing else?

> > +		/* We can't generate a cover letter without any patches */
> > +		if (!head)
> > +			return 0;
> > +	}
> 
> That's true, but with or without cover letter we won't have
> anything to format if we do not have any positive commit.

> > @@ -802,9 +892,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >  		numbered = 1;
> >  	if (numbered)
> >  		rev.total = total + start_number - 1;
> > -	rev.add_signoff = add_signoff;
> >  	if (in_reply_to)
> >  		rev.ref_message_id = clean_message_id(in_reply_to);
> > +	if (cover_letter) {
> > +		if (thread) {
> > +			gen_message_id(&rev, "cover");
> > +		}
> > +		make_cover_letter(&rev, use_stdout, numbered, numbered_files,
> > +				  origin, head);
> > +		total++;
> > +		start_number--;
> > +	}
> > +	rev.add_signoff = add_signoff;
> 
> Nice attention to the detail, moving add_signoff after the cover
> letter.