[PATCH 0/7] format-patch --reroll-count

[PATCH 0/7] format-patch --reroll-count

[Junio C Hamano]

The --reroll-count=$N option, when given a positive integer:

 - Adds " v$N" to the subject prefix specified.  As the default
   subject prefix string is "PATCH", --reroll-count=2 makes it
   "PATCH v2".

 - Prefixes "v$N-" to the names used for output files.  The cover
   letter, whose name is usually 0000-cover-letter.patch, becomes
   v2-0000-cover-letter.patch when given --reroll-count=2.

This allows users to use the same --output-directory for multiple
iterations of the same series, without letting the output for a
newer round overwrite output files from the earlier rounds.  The
user can incorporate materials from earlier rounds to update the
newly minted iteration, and use "send-email v2-*.patch" to send out
the patches belonging to the second iteration easily.

The early patches of this series are all preparatory clean-ups. I
think reopen_stdout() also can be cleaned up, but I'll leave it to
a future reroll.

Junio C Hamano (7):
  builtin/log.c: drop unused "numbered" parameter from
    make_cover_letter()
  builtin/log.c: drop redundant "numbered_files" parameter from
    make_cover_letter()
  builtin/log.c: stop using global patch_suffix
  get_patch_filename(): simplify function signature
  get_patch_filename(): drop "just-numbers" hack
  get_patch_filename(): split into two functions
  format-patch: add --reroll-count=$N option

 builtin/log.c | 33 ++++++++++++++++++++++++---------
 log-tree.c    | 52 +++++++++++++++++++++++++++++++---------------------
 log-tree.h    |  4 ++--
 revision.h    |  1 +
 4 files changed, 58 insertions(+), 32 deletions(-)

-- 
1.8.0.6.gd28b5d4.dirty

[PATCH 1/7] builtin/log.c: drop unused "numbered" parameter from make_cover_letter()

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/log.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 09cf43e..28d9063 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -768,7 +768,7 @@ static void add_branch_description(struct strbuf *buf, const char *branch_name)
 }
 
 static void make_cover_letter(struct rev_info *rev, int use_stdout,
-			      int numbered, int numbered_files,
+			      int numbered_files,
 			      struct commit *origin,
 			      int nr, struct commit **list, struct commit *head,
 			      const char *branch_name,
@@ -1343,7 +1343,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (cover_letter) {
 		if (thread)
 			gen_message_id(&rev, "cover");
-		make_cover_letter(&rev, use_stdout, numbered, numbered_files,
+		make_cover_letter(&rev, use_stdout, numbered_files,
 				  origin, nr, list, head, branch_name, quiet);
 		total++;
 		start_number--;
-- 
1.8.0.6.gd28b5d4.dirty

[PATCH 2/7] builtin/log.c: drop redundant "numbered_files" parameter from make_cover_letter()

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/log.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 28d9063..f1d086e 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -768,7 +768,6 @@ static void add_branch_description(struct strbuf *buf, const char *branch_name)
 }
 
 static void make_cover_letter(struct rev_info *rev, int use_stdout,
-			      int numbered_files,
 			      struct commit *origin,
 			      int nr, struct commit **list, struct commit *head,
 			      const char *branch_name,
@@ -791,7 +790,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	committer = git_committer_info(0);
 
 	if (!use_stdout &&
-	    reopen_stdout(NULL, numbered_files ? NULL : "cover-letter", rev, quiet))
+	    reopen_stdout(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
 		return;
 
 	log_write_email_headers(rev, head, &pp.subject, &pp.after_subject,
@@ -1045,7 +1044,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int nr = 0, total, i;
 	int use_stdout = 0;
 	int start_number = -1;
-	int numbered_files = 0;		/* _just_ numbers */
+	int just_numbers = 0;
 	int ignore_if_in_upstream = 0;
 	int cover_letter = 0;
 	int boundary_count = 0;
@@ -1070,7 +1069,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    N_("print patches to standard out")),
 		OPT_BOOLEAN(0, "cover-letter", &cover_letter,
 			    N_("generate a cover letter")),
-		OPT_BOOLEAN(0, "numbered-files", &numbered_files,
+		OPT_BOOLEAN(0, "numbered-files", &just_numbers,
 			    N_("use simple number sequence for output file names")),
 		OPT_STRING(0, "suffix", &fmt_patch_suffix, N_("sfx"),
 			    N_("use <sfx> instead of '.patch'")),
@@ -1338,12 +1337,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		const char *msgid = clean_message_id(in_reply_to);
 		string_list_append(rev.ref_message_ids, msgid);
 	}
-	rev.numbered_files = numbered_files;
+	rev.numbered_files = just_numbers;
 	rev.patch_suffix = fmt_patch_suffix;
 	if (cover_letter) {
 		if (thread)
 			gen_message_id(&rev, "cover");
-		make_cover_letter(&rev, use_stdout, numbered_files,
+		make_cover_letter(&rev, use_stdout,
 				  origin, nr, list, head, branch_name, quiet);
 		total++;
 		start_number--;
@@ -1390,7 +1389,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		}
 
 		if (!use_stdout &&
-		    reopen_stdout(numbered_files ? NULL : commit, NULL, &rev, quiet))
+		    reopen_stdout(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))
 			die(_("Failed to create output files"));
 		shown = log_tree_commit(&rev, commit);
 		free(commit->buffer);
-- 
1.8.0.6.gd28b5d4.dirty

[PATCH 3/7] builtin/log.c: stop using global patch_suffix

[Junio C Hamano]

The suffix for the output filename is found in rev->patch_suffix; do
not keep using the global that is only used to parse the command
line and configuration.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/log.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index f1d086e..7cf157e 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -673,7 +673,7 @@ static int reopen_stdout(struct commit *commit, const char *subject,
 			 struct rev_info *rev, int quiet)
 {
 	struct strbuf filename = STRBUF_INIT;
-	int suffix_len = strlen(fmt_patch_suffix) + 1;
+	int suffix_len = strlen(rev->patch_suffix) + 1;
 
 	if (output_directory) {
 		strbuf_addstr(&filename, output_directory);
@@ -684,7 +684,7 @@ static int reopen_stdout(struct commit *commit, const char *subject,
 			strbuf_addch(&filename, '/');
 	}
 
-	get_patch_filename(commit, subject, rev->nr, fmt_patch_suffix, &filename);
+	get_patch_filename(commit, subject, rev->nr, rev->patch_suffix, &filename);
 
 	if (!quiet)
 		fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
-- 
1.8.0.6.gd28b5d4.dirty

[PATCH 4/7] get_patch_filename(): simplify function signature

[Junio C Hamano]

Most functions that emit to a strbuf take the strbuf as their first
parameter; make this function follow suit.

The serial number of the patch being emitted (nr) and suffix used
for patch filename (suffix) are both recorded in rev_info; drop
these separate parameters and pass the rev_info directly.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/log.c |  2 +-
 log-tree.c    | 12 ++++++++----
 log-tree.h    |  5 +++--
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 7cf157e..d9946ec 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -684,7 +684,7 @@ static int reopen_stdout(struct commit *commit, const char *subject,
 			strbuf_addch(&filename, '/');
 	}
 
-	get_patch_filename(commit, subject, rev->nr, rev->patch_suffix, &filename);
+	get_patch_filename(&filename, commit, subject, rev);
 
 	if (!quiet)
 		fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
diff --git a/log-tree.c b/log-tree.c
index c894930..ceed6b6 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -299,9 +299,12 @@ static unsigned int digits_in_number(unsigned int number)
 	return result;
 }
 
-void get_patch_filename(struct commit *commit, const char *subject, int nr,
-			const char *suffix, struct strbuf *buf)
+void get_patch_filename(struct strbuf *buf,
+			struct commit *commit, const char *subject,
+			struct rev_info *info)
 {
+	const char *suffix = info->patch_suffix;
+	int nr = info->nr;
 	int suffix_len = strlen(suffix) + 1;
 	int start_len = buf->len;
 
@@ -387,8 +390,9 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			 mime_boundary_leader, opt->mime_boundary);
 		extra_headers = subject_buffer;
 
-		get_patch_filename(opt->numbered_files ? NULL : commit, NULL,
-				   opt->nr, opt->patch_suffix, &filename);
+		get_patch_filename(&filename,
+				   opt->numbered_files ? NULL : commit, NULL,
+				   opt);
 		snprintf(buffer, sizeof(buffer) - 1,
 			 "\n--%s%s\n"
 			 "Content-Type: text/x-patch;"
diff --git a/log-tree.h b/log-tree.h
index f5ac238..c6a944a 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -21,7 +21,8 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 void load_ref_decorations(int flags);
 
 #define FORMAT_PATCH_NAME_MAX 64
-void get_patch_filename(struct commit *commit, const char *subject, int nr,
-			const char *suffix, struct strbuf *buf);
+void get_patch_filename(struct strbuf *buf,
+			struct commit *commit, const char *subject,
+			struct rev_info *);
 
 #endif
-- 
1.8.0.6.gd28b5d4.dirty

[PATCH 5/7] get_patch_filename(): drop "just-numbers" hack

[Junio C Hamano]

The function chooses from three operating modes (format using the
subject, the commit, or just number) based on NULL-ness of two of
its parameters, which is an ugly hack for sharing only a bit of
code.

Separate out the "just numbers" part out to the callers.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/log.c |  5 ++++-
 log-tree.c    | 29 ++++++++++++++---------------
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index d9946ec..3c6f20a 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -684,7 +684,10 @@ static int reopen_stdout(struct commit *commit, const char *subject,
 			strbuf_addch(&filename, '/');
 	}
 
-	get_patch_filename(&filename, commit, subject, rev);
+	if (rev->numbered_files)
+		strbuf_addf(&filename, "%d", rev->nr);
+	else
+		get_patch_filename(&filename, commit, subject, rev);
 
 	if (!quiet)
 		fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
diff --git a/log-tree.c b/log-tree.c
index ceed6b6..d9f86ce 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -307,21 +307,18 @@ void get_patch_filename(struct strbuf *buf,
 	int nr = info->nr;
 	int suffix_len = strlen(suffix) + 1;
 	int start_len = buf->len;
+	int max_len = start_len + FORMAT_PATCH_NAME_MAX - suffix_len;
 
-	strbuf_addf(buf, commit || subject ? "%04d-" : "%d", nr);
-	if (commit || subject) {
-		int max_len = start_len + FORMAT_PATCH_NAME_MAX - suffix_len;
+	strbuf_addf(buf, "%04d-", nr);
+	if (subject)
+		strbuf_addstr(buf, subject);
+	else if (commit) {
 		struct pretty_print_context ctx = {0};
-
-		if (subject)
-			strbuf_addstr(buf, subject);
-		else if (commit)
-			format_commit_message(commit, "%f", buf, &ctx);
-
-		if (max_len < buf->len)
-			strbuf_setlen(buf, max_len);
-		strbuf_addstr(buf, suffix);
+		format_commit_message(commit, "%f", buf, &ctx);
 	}
+	if (max_len < buf->len)
+		strbuf_setlen(buf, max_len);
+	strbuf_addstr(buf, suffix);
 }
 
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
@@ -390,9 +387,11 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			 mime_boundary_leader, opt->mime_boundary);
 		extra_headers = subject_buffer;
 
-		get_patch_filename(&filename,
-				   opt->numbered_files ? NULL : commit, NULL,
-				   opt);
+		if (opt->numbered_files)
+			strbuf_addf(&filename, "%d", opt->nr);
+		else
+			get_patch_filename(&filename, commit, NULL,
+					   opt);
 		snprintf(buffer, sizeof(buffer) - 1,
 			 "\n--%s%s\n"
 			 "Content-Type: text/x-patch;"
-- 
1.8.0.6.gd28b5d4.dirty

[PATCH 6/7] get_patch_filename(): split into two functions

[Junio C Hamano]

The function switched between two operating modes depending on the
NULL-ness of its two parameters, as a hacky way to share small part
of implementation, sacrificing cleanliness of the API.

Implement "fmt_output_subject()" function that takes a subject
string and gives the name for the output file, and on top of it,
implement "fmt_output_commit()" function that takes a commit and
gives the name for the output file.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/log.c |  4 +++-
 log-tree.c    | 41 +++++++++++++++++++++++------------------
 log-tree.h    |  5 ++---
 3 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 3c6f20a..8cfb4da 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -686,8 +686,10 @@ static int reopen_stdout(struct commit *commit, const char *subject,
 
 	if (rev->numbered_files)
 		strbuf_addf(&filename, "%d", rev->nr);
+	else if (commit)
+		fmt_output_commit(&filename, commit, rev);
 	else
-		get_patch_filename(&filename, commit, subject, rev);
+		fmt_output_subject(&filename, subject, rev);
 
 	if (!quiet)
 		fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
diff --git a/log-tree.c b/log-tree.c
index d9f86ce..670beae 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -299,26 +299,32 @@ static unsigned int digits_in_number(unsigned int number)
 	return result;
 }
 
-void get_patch_filename(struct strbuf *buf,
-			struct commit *commit, const char *subject,
+void fmt_output_subject(struct strbuf *filename,
+			const char *subject,
 			struct rev_info *info)
 {
 	const char *suffix = info->patch_suffix;
 	int nr = info->nr;
-	int suffix_len = strlen(suffix) + 1;
-	int start_len = buf->len;
-	int max_len = start_len + FORMAT_PATCH_NAME_MAX - suffix_len;
-
-	strbuf_addf(buf, "%04d-", nr);
-	if (subject)
-		strbuf_addstr(buf, subject);
-	else if (commit) {
-		struct pretty_print_context ctx = {0};
-		format_commit_message(commit, "%f", buf, &ctx);
-	}
-	if (max_len < buf->len)
-		strbuf_setlen(buf, max_len);
-	strbuf_addstr(buf, suffix);
+	int start_len = filename->len;
+	int max_len = start_len + FORMAT_PATCH_NAME_MAX - (strlen(suffix) + 1);
+
+	strbuf_addf(filename, "%04d-%s", nr, subject);
+
+	if (max_len < filename->len)
+		strbuf_setlen(filename, max_len);
+	strbuf_addstr(filename, suffix);
+}
+
+void fmt_output_commit(struct strbuf *filename,
+		       struct commit *commit,
+		       struct rev_info *info)
+{
+	struct pretty_print_context ctx = {0};
+	struct strbuf subject = STRBUF_INIT;
+
+	format_commit_message(commit, "%f", &subject, &ctx);
+	fmt_output_subject(filename, subject.buf, info);
+	strbuf_release(&subject);
 }
 
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
@@ -390,8 +396,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 		if (opt->numbered_files)
 			strbuf_addf(&filename, "%d", opt->nr);
 		else
-			get_patch_filename(&filename, commit, NULL,
-					   opt);
+			fmt_output_commit(&filename, commit, opt);
 		snprintf(buffer, sizeof(buffer) - 1,
 			 "\n--%s%s\n"
 			 "Content-Type: text/x-patch;"
diff --git a/log-tree.h b/log-tree.h
index c6a944a..9140f48 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -21,8 +21,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 void load_ref_decorations(int flags);
 
 #define FORMAT_PATCH_NAME_MAX 64
-void get_patch_filename(struct strbuf *buf,
-			struct commit *commit, const char *subject,
-			struct rev_info *);
+void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);
+void fmt_output_subject(struct strbuf *, const char *subject, struct rev_info *);
 
 #endif
-- 
1.8.0.6.gd28b5d4.dirty

[PATCH 7/7] format-patch: add --reroll-count=$N option

[Junio C Hamano]

The --reroll-count=$N option, when given a positive integer:

 - Adds " v$N" to the subject prefix specified.  As the default
   subject prefix string is "PATCH", --reroll-count=2 makes it
   "PATCH v2".

 - Prefixes "v$N-" to the names used for output files.  The cover
   letter, whose name is usually 0000-cover-letter.patch, becomes
   v2-0000-cover-letter.patch when given --reroll-count=2.

This allows users to use the same --output-directory for multiple
iterations of the same series, without letting the output for a
newer round overwrite output files from the earlier rounds.  The
user can incorporate materials from earlier rounds to update the
newly minted iteration, and use "send-email v2-*.patch" to send out
the patches belonging to the second iteration easily.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/log.c | 11 +++++++++++
 log-tree.c    |  2 ++
 revision.h    |  1 +
 3 files changed, 14 insertions(+)

diff --git a/builtin/log.c b/builtin/log.c
index 8cfb4da..e101498 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1061,6 +1061,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	struct strbuf buf = STRBUF_INIT;
 	int use_patch_format = 0;
 	int quiet = 0;
+	int reroll_count = -1;
 	char *branch_name = NULL;
 	const struct option builtin_format_patch_options[] = {
 		{ OPTION_CALLBACK, 'n', "numbered", &numbered, NULL,
@@ -1080,6 +1081,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    N_("use <sfx> instead of '.patch'")),
 		OPT_INTEGER(0, "start-number", &start_number,
 			    N_("start numbering patches at <n> instead of 1")),
+		OPT_INTEGER(0, "reroll-count", &reroll_count,
+			    N_("mark the series as Nth re-roll")),
 		{ OPTION_CALLBACK, 0, "subject-prefix", &rev, N_("prefix"),
 			    N_("Use [<prefix>] instead of [PATCH]"),
 			    PARSE_OPT_NONEG, subject_prefix_callback },
@@ -1152,6 +1155,14 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
 			     PARSE_OPT_KEEP_DASHDASH);
 
+	if (0 < reroll_count) {
+		struct strbuf sprefix = STRBUF_INIT;
+		strbuf_addf(&sprefix, "%s v%d",
+			    rev.subject_prefix, reroll_count);
+		rev.reroll_count = reroll_count;
+		rev.subject_prefix = strbuf_detach(&sprefix, NULL);
+	}
+
 	if (do_signoff) {
 		const char *committer;
 		const char *endpos;
diff --git a/log-tree.c b/log-tree.c
index 670beae..5dc126b 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -308,6 +308,8 @@ void fmt_output_subject(struct strbuf *filename,
 	int start_len = filename->len;
 	int max_len = start_len + FORMAT_PATCH_NAME_MAX - (strlen(suffix) + 1);
 
+	if (0 < info->reroll_count)
+		strbuf_addf(filename, "v%d-", info->reroll_count);
 	strbuf_addf(filename, "%04d-%s", nr, subject);
 
 	if (max_len < filename->len)
diff --git a/revision.h b/revision.h
index a95bd0b..e4a912a 100644
--- a/revision.h
+++ b/revision.h
@@ -134,6 +134,7 @@ struct rev_info {
 	const char	*mime_boundary;
 	const char	*patch_suffix;
 	int		numbered_files;
+	int		reroll_count;
 	char		*message_id;
 	struct string_list *ref_message_ids;
 	const char	*add_signoff;
-- 
1.8.0.6.gd28b5d4.dirty

Re: [PATCH 0/7] format-patch --reroll-count

[Duy Nguyen]

On Sat, Dec 22, 2012 at 3:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> The --reroll-count=$N option, when given a positive integer:
>
>  - Adds " v$N" to the subject prefix specified.  As the default
>    subject prefix string is "PATCH", --reroll-count=2 makes it
>    "PATCH v2".
>
>  - Prefixes "v$N-" to the names used for output files.  The cover
>    letter, whose name is usually 0000-cover-letter.patch, becomes
>    v2-0000-cover-letter.patch when given --reroll-count=2.

On the same subject of format-patch improvements, perhaps we should also:

   - Keep subject line in branch.%s.description too. No point in
     keeping writing the subject on every reroll.

   - Perhaps an option to save the current tip in reflog with reroll
     count so one can easily diff between two rerolls.
-- 
Duy