[RFC PATCH 0/5] format-patch: automate cover letter range-diff

[RFC PATCH 0/5] format-patch: automate cover letter range-diff

[Eric Sunshine]

Dscho recently implemented a 'tbdiff' replacement as a Git builtin named
git-branch-diff[1] which computes differences between two versions of a
patch series. Such a diff can be a useful aid for reviewers when
inserted into a cover letter. However, doing so requires manual
generation (invoking git-branch-diff) and copy/pasting the result into
the cover letter.

This series fully automates the process by adding a --range-diff option
to git-format-patch. It is RFC for a couple reasons:

* The final name for the 'tbdiff' replacement has not yet been nailed
  down. The name git-branch-diff is moribund[2]; Dscho favors merging
  the functionality into git-branch as a new --diff option[3]; others
  prefer a standalone command named git-range-diff or
  git-series-diff[4] or similar.

* I have some ideas for future enhancements and want to be careful not
  to lock in a UI which doesn't mesh well with them (though I think the
  current UI turned out reasonably well). First, I foresee a
  complementary --interdiff option for inserting an interdiff-style diff
  for cases when that style is easier to read or simply more
  appropriate. Second, although the current patch series only supports
  --range-diff for the cover letter, some people insert interdiff- or
  tbdiff-style diffs (indented) into the commentary section of
  standalone patches. Although this often makes for a noisy mess, it is
  periodically useful.

This series is built atop js/branch-diff in 'pu'.

[1]: https://public-inbox.org/git/cover.1525448066.git.johannes.schindelin@gmx.de/
[2]: https://public-inbox.org/git/nycvar.QRO.7.76.6.1805061401260.77@tvgsbejvaqbjf.bet/
[3]: https://public-inbox.org/git/nycvar.QRO.7.76.6.1805062155120.77@tvgsbejvaqbjf.bet/
[4]: https://public-inbox.org/git/xmqqin7gzbkb.fsf@gitster-ct.c.googlers.com/

Eric Sunshine (5):
  format-patch: allow additional generated content in
    make_cover_letter()
  format-patch: add --range-diff option to embed diff in cover letter
  format-patch: extend --range-diff to accept revision range
  format-patch: teach --range-diff to respect -v/--reroll-count
  format-patch: add --creation-weight tweak for --range-diff

 Documentation/git-format-patch.txt |  18 +++++
 builtin/log.c                      | 119 ++++++++++++++++++++++++-----
 t/t7910-branch-diff.sh             |  16 ++++
 3 files changed, 132 insertions(+), 21 deletions(-)

-- 
2.17.1.1235.ge295dfb56e

[RFC PATCH 1/5] format-patch: allow additional generated content in make_cover_letter()

[Eric Sunshine]

make_cover_letter() returns early when it lacks sufficient state to emit
a diffstat, which makes it difficult to extend the function to reliably
emit additional generated content. Work around this shortcoming by
factoring diffstat-printing logic out to its own function and calling it
as needed without otherwise inhibiting normal control flow.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/log.c | 43 +++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 71f68a3e4f..e01a256c11 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -992,6 +992,26 @@ static char *find_branch_name(struct rev_info *rev)
 	return branch;
 }
 
+static void emit_diffstat(struct rev_info *rev,
+			  struct commit *origin, struct commit *head)
+{
+	struct diff_options opts;
+
+	memcpy(&opts, &rev->diffopt, sizeof(opts));
+	opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
+	opts.stat_width = MAIL_DEFAULT_WRAP;
+
+	diff_setup_done(&opts);
+
+	diff_tree_oid(&origin->tree->object.oid,
+		      &head->tree->object.oid,
+		      "", &opts);
+	diffcore_std(&opts);
+	diff_flush(&opts);
+
+	fprintf(rev->diffopt.file, "\n");
+}
+
 static void make_cover_letter(struct rev_info *rev, int use_stdout,
 			      struct commit *origin,
 			      int nr, struct commit **list,
@@ -1005,7 +1025,6 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	struct strbuf sb = STRBUF_INIT;
 	int i;
 	const char *encoding = "UTF-8";
-	struct diff_options opts;
 	int need_8bit_cte = 0;
 	struct pretty_print_context pp = {0};
 	struct commit *head = list[0];
@@ -1055,25 +1074,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 
 	shortlog_output(&log);
 
-	/*
-	 * We can only do diffstat with a unique reference point
-	 */
-	if (!origin)
-		return;
-
-	memcpy(&opts, &rev->diffopt, sizeof(opts));
-	opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
-	opts.stat_width = MAIL_DEFAULT_WRAP;
-
-	diff_setup_done(&opts);
-
-	diff_tree_oid(&origin->tree->object.oid,
-		      &head->tree->object.oid,
-		      "", &opts);
-	diffcore_std(&opts);
-	diff_flush(&opts);
-
-	fprintf(rev->diffopt.file, "\n");
+	/* We can only do diffstat with a unique reference point */
+	if (origin)
+		emit_diffstat(rev, origin, head);
 }
 
 static const char *clean_message_id(const char *msg_id)
-- 
2.17.1.1235.ge295dfb56e

[RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter

[Eric Sunshine]

When submitting a revised version of a patch series, it can be helpful
(to reviewers) to include a summary of changes since the previous
attempt in the form of an interdiff or range-diff, however, doing so
involves manually copy/pasting the diff into the cover letter.

Add a --range-diff option to automate this process for range-diffs. The
argument to --range-diff specifies the tip of the previous attempt
against which to generate the range-diff. For example:

    git format-patch --cover-letter --range-diff=v1 -3 v2

(At this early stage, the previous attempt and the patch series being
formatted must share a common base, however, a subsequent enhancement
will make it possible to specify an explicit revision range for the
previous attempt.)

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/git-format-patch.txt | 10 ++++++
 builtin/log.c                      | 55 ++++++++++++++++++++++++++++--
 t/t7910-branch-diff.sh             | 15 ++++++++
 3 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 6cbe462a77..f4c70e6b64 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -23,6 +23,7 @@ SYNOPSIS
 		   [(--reroll-count|-v) <n>]
 		   [--to=<email>] [--cc=<email>]
 		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
+		   [--range-diff=<previous>]
 		   [--progress]
 		   [<common diff options>]
 		   [ <since> | <revision range> ]
@@ -228,6 +229,15 @@ feeding the result to `git send-email`.
 	containing the branch description, shortlog and the overall diffstat.  You can
 	fill in a description in the file before sending it out.
 
+--range-diff=<previous>::
+	As a reviewer aid, insert a range-diff (see linkgit:git-branch-diff[1])
+	into the cover letter showing the differences between the previous
+	version of the patch series and the series currently being formatted.
+	`previous` is a single revision naming the tip of the previous
+	series which shares a common base with the series being formatted (for
+	example `git format-patch --cover-letter --range-diff=feature/v1 -3
+	feature/v2`).
+
 --notes[=<ref>]::
 	Append the notes (see linkgit:git-notes[1]) for the commit
 	after the three-dash line.
diff --git a/builtin/log.c b/builtin/log.c
index e01a256c11..460c31a293 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -28,6 +28,7 @@
 #include "mailmap.h"
 #include "gpg-interface.h"
 #include "progress.h"
+#include "run-command.h"
 
 #define MAIL_DEFAULT_WRAP 72
 
@@ -992,6 +993,25 @@ static char *find_branch_name(struct rev_info *rev)
 	return branch;
 }
 
+static void infer_diff_ranges(struct argv_array *args,
+			      const char *prev,
+			      struct commit *head)
+{
+	argv_array_pushf(args, "%s...%s", prev,
+			 oid_to_hex(&head->object.oid));
+}
+
+static int get_range_diff(struct strbuf *sb,
+			  const struct argv_array *ranges)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "branch-diff", "--no-color", NULL);
+	argv_array_pushv(&cp.args, ranges->argv);
+	return capture_command(&cp, sb, 0);
+}
+
 static void emit_diffstat(struct rev_info *rev,
 			  struct commit *origin, struct commit *head)
 {
@@ -1016,7 +1036,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 			      struct commit *origin,
 			      int nr, struct commit **list,
 			      const char *branch_name,
-			      int quiet)
+			      int quiet,
+			      const char *range_diff)
 {
 	const char *committer;
 	const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
@@ -1028,15 +1049,25 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	int need_8bit_cte = 0;
 	struct pretty_print_context pp = {0};
 	struct commit *head = list[0];
+	struct strbuf diff = STRBUF_INIT;
 
 	if (!cmit_fmt_is_mail(rev->commit_format))
 		die(_("Cover letter needs email format"));
 
 	committer = git_committer_info(0);
 
+	/* might die from bad user input so try before creating cover letter */
+	if (range_diff) {
+		struct argv_array ranges = ARGV_ARRAY_INIT;
+		infer_diff_ranges(&ranges, range_diff, head);
+		if (get_range_diff(&diff, &ranges))
+			die(_("failed to generate range-diff"));
+		argv_array_clear(&ranges);
+	}
+
 	if (!use_stdout &&
 	    open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
-		return;
+		goto done;
 
 	log_write_email_headers(rev, head, &pp.after_subject, &need_8bit_cte);
 
@@ -1077,6 +1108,17 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	/* We can only do diffstat with a unique reference point */
 	if (origin)
 		emit_diffstat(rev, origin, head);
+
+	if (diff.len) {
+		FILE *fp = rev->diffopt.file;
+		fputs(_("Changes since previous version:"), fp);
+		fputs("\n\n", fp);
+		fputs(diff.buf, fp);
+		fputc('\n', fp);
+	}
+
+done:
+	strbuf_release(&diff);
 }
 
 static const char *clean_message_id(const char *msg_id)
@@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	struct base_tree_info bases;
 	int show_progress = 0;
 	struct progress *progress = NULL;
+	const char *range_diff = NULL;
 
 	const struct option builtin_format_patch_options[] = {
 		{ OPTION_CALLBACK, 'n', "numbered", &numbered, NULL,
@@ -1511,6 +1554,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		OPT__QUIET(&quiet, N_("don't print the patch filenames")),
 		OPT_BOOL(0, "progress", &show_progress,
 			 N_("show progress while generating patches")),
+		OPT_STRING(0, "range-diff", &range_diff, N_("rev-range"),
+			   N_("show changes against <rev-range> in cover letter")),
 		OPT_END()
 	};
 
@@ -1733,6 +1778,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (numbered)
 		rev.total = total + start_number - 1;
 
+	if (range_diff && !cover_letter)
+		die(_("--range-diff requires --cover-letter"));
+
 	if (!signature) {
 		; /* --no-signature inhibits all signatures */
 	} else if (signature && signature != git_version_string) {
@@ -1764,7 +1812,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		if (thread)
 			gen_message_id(&rev, "cover");
 		make_cover_letter(&rev, use_stdout,
-				  origin, nr, list, branch_name, quiet);
+				  origin, nr, list, branch_name, quiet,
+				  range_diff);
 		print_bases(&bases, rev.diffopt.file);
 		print_signature(rev.diffopt.file);
 		total++;
diff --git a/t/t7910-branch-diff.sh b/t/t7910-branch-diff.sh
index a7fece8804..edbd69b6f8 100755
--- a/t/t7910-branch-diff.sh
+++ b/t/t7910-branch-diff.sh
@@ -141,4 +141,19 @@ test_expect_success 'changed message' '
 	test_cmp expected actual
 '
 
+format_patch () {
+	title=$1 &&
+	range=$2 &&
+	test_expect_success "format-patch --range-diff ($title)" '
+		git format-patch --stdout --cover-letter --range-diff=$range \
+			master..unmodified >actual &&
+		grep "= 1: .* s/5/A" actual &&
+		grep "= 2: .* s/4/A" actual &&
+		grep "= 3: .* s/11/B" actual &&
+		grep "= 4: .* s/12/B" actual
+	'
+}
+
+format_patch 'B...C' topic
+
 test_done
-- 
2.17.1.1235.ge295dfb56e

[RFC PATCH 3/5] format-patch: extend --range-diff to accept revision range

[Eric Sunshine]

When submitting a revised a patch series, the --range-diff option embeds
a range-diff in the cover letter showing changes since the previous
version of the patch series. The argument to --range-diff is a simple
revision naming the tip of the previous series, which works fine if the
previous and current versions of the patch series share a common base.

However, it fails if the revision ranges of the old and new versions of
the series are disjoint. To address this shortcoming, extend
--range-diff to also accept an explicit revision range for the previous
series. For example:

    git format-patch --cover-letter --range-diff=v1~3..v1 -3 v2

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/git-format-patch.txt |  8 +++++---
 builtin/log.c                      | 16 +++++++++++++---
 t/t7910-branch-diff.sh             |  1 +
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index f4c70e6b64..25026ae26e 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -233,10 +233,12 @@ feeding the result to `git send-email`.
 	As a reviewer aid, insert a range-diff (see linkgit:git-branch-diff[1])
 	into the cover letter showing the differences between the previous
 	version of the patch series and the series currently being formatted.
-	`previous` is a single revision naming the tip of the previous
-	series which shares a common base with the series being formatted (for
+	`previous` can be a single revision naming the tip of the previous
+	series if it shares a common base with the series being formatted (for
 	example `git format-patch --cover-letter --range-diff=feature/v1 -3
-	feature/v2`).
+	feature/v2`), or a revision range if the two versions of the series are
+	disjoint (for example `git format-patch --cover-letter
+	--range-diff=feature/v1~3..feature/v1 -3 feature/v2`).
 
 --notes[=<ref>]::
 	Append the notes (see linkgit:git-notes[1]) for the commit
diff --git a/builtin/log.c b/builtin/log.c
index 460c31a293..e38cf06050 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -995,10 +995,20 @@ static char *find_branch_name(struct rev_info *rev)
 
 static void infer_diff_ranges(struct argv_array *args,
 			      const char *prev,
+			      struct commit *origin,
 			      struct commit *head)
 {
-	argv_array_pushf(args, "%s...%s", prev,
-			 oid_to_hex(&head->object.oid));
+	if (strstr(prev, "..")) {
+		if (!origin)
+			die(_("failed to infer range-diff ranges"));
+		argv_array_push(args, prev);
+		argv_array_pushf(args, "%s..%s",
+				 oid_to_hex(&origin->object.oid),
+				 oid_to_hex(&head->object.oid));
+	} else {
+		argv_array_pushf(args, "%s...%s", prev,
+				 oid_to_hex(&head->object.oid));
+	}
 }
 
 static int get_range_diff(struct strbuf *sb,
@@ -1059,7 +1069,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	/* might die from bad user input so try before creating cover letter */
 	if (range_diff) {
 		struct argv_array ranges = ARGV_ARRAY_INIT;
-		infer_diff_ranges(&ranges, range_diff, head);
+		infer_diff_ranges(&ranges, range_diff, origin, head);
 		if (get_range_diff(&diff, &ranges))
 			die(_("failed to generate range-diff"));
 		argv_array_clear(&ranges);
diff --git a/t/t7910-branch-diff.sh b/t/t7910-branch-diff.sh
index edbd69b6f8..c0e8668ed9 100755
--- a/t/t7910-branch-diff.sh
+++ b/t/t7910-branch-diff.sh
@@ -155,5 +155,6 @@ format_patch () {
 }
 
 format_patch 'B...C' topic
+format_patch 'A..B A..C' master..topic
 
 test_done
-- 
2.17.1.1235.ge295dfb56e

[RFC PATCH 4/5] format-patch: teach --range-diff to respect -v/--reroll-count

[Eric Sunshine]

The --range-diff option introduces the embedded range-diff generically
as "Changes since previous version:", however, we can do better when
--reroll-count is specified by emitting "Changes since v{n}:" instead.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/log.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index e38cf06050..3089d3a50a 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1121,7 +1121,11 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 
 	if (diff.len) {
 		FILE *fp = rev->diffopt.file;
-		fputs(_("Changes since previous version:"), fp);
+		if (rev->reroll_count <= 0)
+			fputs(_("Changes since previous version:"), fp);
+		else /* RFC may be v0, so allow -v1 to diff against v0 */
+			fprintf(fp, _("Changes since v%d:"),
+				rev->reroll_count - 1);
 		fputs("\n\n", fp);
 		fputs(diff.buf, fp);
 		fputc('\n', fp);
-- 
2.17.1.1235.ge295dfb56e

[RFC PATCH 5/5] format-patch: add --creation-weight tweak for --range-diff

[Eric Sunshine]

When generating a range-diff, matching up commits between two version of
a patch series involves heuristics, thus may give unexpected results.
git-branch-diff allows tweaking the heuristic via --creation-weight.
Follow suit by accepting --creation-weight in combination with
--range-diff when generating a range-diff for a cover-letter.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/git-format-patch.txt |  8 +++++++-
 builtin/log.c                      | 19 +++++++++++++++----
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 25026ae26e..7ed9ec9dae 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -23,7 +23,7 @@ SYNOPSIS
 		   [(--reroll-count|-v) <n>]
 		   [--to=<email>] [--cc=<email>]
 		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
-		   [--range-diff=<previous>]
+		   [--range-diff=<previous> [--creation-weight=<factor>]]
 		   [--progress]
 		   [<common diff options>]
 		   [ <since> | <revision range> ]
@@ -240,6 +240,12 @@ feeding the result to `git send-email`.
 	disjoint (for example `git format-patch --cover-letter
 	--range-diff=feature/v1~3..feature/v1 -3 feature/v2`).
 
+--creation-weight=<factor>::
+	Used with `--range-diff`, tweak the heuristic which matches up commits
+	between the previous and current series of patches by adjusting the
+	creation/deletion cost fudge factor. See linkgit:git-branch-diff[1])
+	for details.
+
 --notes[=<ref>]::
 	Append the notes (see linkgit:git-notes[1]) for the commit
 	after the three-dash line.
diff --git a/builtin/log.c b/builtin/log.c
index 3089d3a50a..2c49011b51 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1012,12 +1012,16 @@ static void infer_diff_ranges(struct argv_array *args,
 }
 
 static int get_range_diff(struct strbuf *sb,
-			  const struct argv_array *ranges)
+			  const struct argv_array *ranges,
+			  const char *creation_weight)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 
 	cp.git_cmd = 1;
 	argv_array_pushl(&cp.args, "branch-diff", "--no-color", NULL);
+	if (creation_weight)
+		argv_array_pushf(&cp.args,
+				 "--creation-weight=%s", creation_weight);
 	argv_array_pushv(&cp.args, ranges->argv);
 	return capture_command(&cp, sb, 0);
 }
@@ -1047,7 +1051,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 			      int nr, struct commit **list,
 			      const char *branch_name,
 			      int quiet,
-			      const char *range_diff)
+			      const char *range_diff,
+			      const char *creation_weight)
 {
 	const char *committer;
 	const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
@@ -1070,7 +1075,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	if (range_diff) {
 		struct argv_array ranges = ARGV_ARRAY_INIT;
 		infer_diff_ranges(&ranges, range_diff, origin, head);
-		if (get_range_diff(&diff, &ranges))
+		if (get_range_diff(&diff, &ranges, creation_weight))
 			die(_("failed to generate range-diff"));
 		argv_array_clear(&ranges);
 	}
@@ -1495,6 +1500,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int show_progress = 0;
 	struct progress *progress = NULL;
 	const char *range_diff = NULL;
+	const char *creation_weight = NULL;
 
 	const struct option builtin_format_patch_options[] = {
 		{ OPTION_CALLBACK, 'n', "numbered", &numbered, NULL,
@@ -1570,6 +1576,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			 N_("show progress while generating patches")),
 		OPT_STRING(0, "range-diff", &range_diff, N_("rev-range"),
 			   N_("show changes against <rev-range> in cover letter")),
+		OPT_STRING(0, "creation-weight", &creation_weight, N_("factor"),
+			   N_("fudge factor by which creation is weighted")),
 		OPT_END()
 	};
 
@@ -1664,6 +1672,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		die (_("--subject-prefix/--rfc and -k are mutually exclusive."));
 	rev.preserve_subject = keep_subject;
 
+	if (creation_weight && !range_diff)
+		die(_("--creation-weight requires --range-diff"));
+
 	argc = setup_revisions(argc, argv, &rev, &s_r_opt);
 	if (argc > 1)
 		die (_("unrecognized argument: %s"), argv[1]);
@@ -1827,7 +1838,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			gen_message_id(&rev, "cover");
 		make_cover_letter(&rev, use_stdout,
 				  origin, nr, list, branch_name, quiet,
-				  range_diff);
+				  range_diff, creation_weight);
 		print_bases(&bases, rev.diffopt.file);
 		print_signature(rev.diffopt.file);
 		total++;
-- 
2.17.1.1235.ge295dfb56e

Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff

[Ævar Arnfjörð Bjarmason]

On Wed, May 30 2018, Eric Sunshine wrote:

> * The final name for the 'tbdiff' replacement has not yet been nailed
>   down. The name git-branch-diff is moribund[2]; Dscho favors merging
>   the functionality into git-branch as a new --diff option[3]; others
>   prefer a standalone command named git-range-diff or
>   git-series-diff[4] or similar.

FWIW Dscho in an IRC conversation on May 25th seems to have settled on
calling it "git range-diff <args>". He just hasn't gotten around to
submitting a new version.

Re: [RFC PATCH 3/5] format-patch: extend --range-diff to accept revision range

[Stefan Beller]

On Wed, May 30, 2018 at 1:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> When submitting a revised a patch series, the --range-diff option embeds
> a range-diff in the cover letter showing changes since the previous
> version of the patch series. The argument to --range-diff is a simple
> revision naming the tip of the previous series, which works fine if the
> previous and current versions of the patch series share a common base.
>
> However, it fails if the revision ranges of the old and new versions of
> the series are disjoint. To address this shortcoming, extend
> --range-diff to also accept an explicit revision range for the previous
> series. For example:
>
>     git format-patch --cover-letter --range-diff=v1~3..v1 -3 v2
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  Documentation/git-format-patch.txt |  8 +++++---
>  builtin/log.c                      | 16 +++++++++++++---
>  t/t7910-branch-diff.sh             |  1 +
>  3 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index f4c70e6b64..25026ae26e 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -233,10 +233,12 @@ feeding the result to `git send-email`.
>         As a reviewer aid, insert a range-diff (see linkgit:git-branch-diff[1])
>         into the cover letter showing the differences between the previous
>         version of the patch series and the series currently being formatted.
> -       `previous` is a single revision naming the tip of the previous
> -       series which shares a common base with the series being formatted (for
> +       `previous` can be a single revision naming the tip of the previous
> +       series if it shares a common base with the series being formatted (for
>         example `git format-patch --cover-letter --range-diff=feature/v1 -3
> -       feature/v2`).
> +       feature/v2`), or a revision range if the two versions of the series are
> +       disjoint (for example `git format-patch --cover-letter
> +       --range-diff=feature/v1~3..feature/v1 -3 feature/v2`).
>
>  --notes[=<ref>]::
>         Append the notes (see linkgit:git-notes[1]) for the commit
> diff --git a/builtin/log.c b/builtin/log.c
> index 460c31a293..e38cf06050 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -995,10 +995,20 @@ static char *find_branch_name(struct rev_info *rev)
>
>  static void infer_diff_ranges(struct argv_array *args,
>                               const char *prev,
> +                             struct commit *origin,
>                               struct commit *head)
>  {
> -       argv_array_pushf(args, "%s...%s", prev,
> -                        oid_to_hex(&head->object.oid));
> +       if (strstr(prev, "..")) {
> +               if (!origin)
> +                       die(_("failed to infer range-diff ranges"));


>  static int get_range_diff(struct strbuf *sb,
> @@ -1059,7 +1069,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>         /* might die from bad user input so try before creating cover letter */
>         if (range_diff) {
>                 struct argv_array ranges = ARGV_ARRAY_INIT;
> -               infer_diff_ranges(&ranges, range_diff, head);
> +               infer_diff_ranges(&ranges, range_diff, origin, head);

This is way before the check for 'if (origin) emit_diffstat' as done in
patch 1, as we want to do this early. We need to check the non-NULLness
in infer_diff_ranges as it is allowed to be NULL when the range-diff doesn't
contain "..", which means we assume the same branch point.

Would it make sense to give a better error message in:

> +               if (!origin)
> +                       die(_("failed to infer range-diff ranges"));

above? (The failure sounds like it could be anything, but the
reason for it is actually well understood: The origin was
computed and as the boundary commit of the given range,
or NULL if there is no boundary commit or multiple commits.

If we find not exactly one boundary, we cannot compute the
range to give to the range-diff tool.

Stepping back a bit:

  Consider the following DAG:

        O -
           \
    A - B - C - D

a series from B..D (or '-2' when D is HEAD) would fail
to generate both diffstat as well as the range diff as the
merge of O produces a second boundary, (but wouldn't
the creation of the patch C fail eventually anyway?)

Another DAG:

    A - B - C - D
         \    /
           E

when asking for B..D the diffstat and range diff would
compute ok, but the patch C would fail to generate again?

Stepping back to the error message, I have no good
suggestion on what to say there. Maybe we'd want to
refactor the code in cmd_format_patch, that computes the
origin:

        if (prepare_revision_walk(&rev))
                die(_("revision walk setup failed"));
        rev.boundary = 1;
        while ((commit = get_revision(&rev)) != NULL) {
                if (commit->object.flags & BOUNDARY) {
                        boundary_count++;
                        origin = (boundary_count == 1) ? commit : NULL;
                        continue;
                }

                if (ignore_if_in_upstream && has_commit_patch_id(commit, &ids))
                        continue;

                nr++;
                REALLOC_ARRAY(list, nr);
                list[nr - 1] = commit;
        }

We could prefix that with

    need_origin = (range_diff && strstr(prev, "..")  || emit_diff_stat;

and then if need_origin is about to be unset again we could issue
a warning and die early after the loop warning about bad DAG form?

I guess that can wait for a follow up patch.

Thanks,
Stefan

Re: [RFC PATCH 4/5] format-patch: teach --range-diff to respect -v/--reroll-count

[Stefan Beller]

On Wed, May 30, 2018 at 1:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> The --range-diff option introduces the embedded range-diff generically
> as "Changes since previous version:", however, we can do better when
> --reroll-count is specified by emitting "Changes since v{n}:" instead.

A very similar option that I used before finding reroll count is
--subject-prefix
I still use that for RFC/WIP tags, but I sometimes used it with "PATCHv2"
as an argument, too.

Would we want to extend the niceties of this patch to that workflow?

Unrelated to this patch: how does this series cope with range diffs
that are not in commit-ish but patches on the file system?

Re: [RFC PATCH 3/5] format-patch: extend --range-diff to accept revision range

[Eric Sunshine]

On Wed, May 30, 2018 at 2:58 PM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, May 30, 2018 at 1:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> When submitting a revised a patch series, the --range-diff option embeds
>> a range-diff in the cover letter showing changes since the previous
>> version of the patch series. The argument to --range-diff is a simple
>> revision naming the tip of the previous series, which works fine if the
>> previous and current versions of the patch series share a common base.
>>
>> However, it fails if the revision ranges of the old and new versions of
>> the series are disjoint. To address this shortcoming, extend
>> --range-diff to also accept an explicit revision range for the previous
>> series. For example:
>>
>>     git format-patch --cover-letter --range-diff=v1~3..v1 -3 v2
>>
>> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>> ---
>>  static void infer_diff_ranges(struct argv_array *args,
>>                               const char *prev,
>> +                             struct commit *origin,
>>                               struct commit *head)
>>  {
>> -       argv_array_pushf(args, "%s...%s", prev,
>> -                        oid_to_hex(&head->object.oid));
>> +       if (strstr(prev, "..")) {
>> +               if (!origin)
>> +                       die(_("failed to infer range-diff ranges"));
>
>>  static int get_range_diff(struct strbuf *sb,
>> @@ -1059,7 +1069,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>>         /* might die from bad user input so try before creating cover letter */
>>         if (range_diff) {
>>                 struct argv_array ranges = ARGV_ARRAY_INIT;
>> -               infer_diff_ranges(&ranges, range_diff, head);
>> +               infer_diff_ranges(&ranges, range_diff, origin, head);
>
> This is way before the check for 'if (origin) emit_diffstat' as done in
> patch 1, as we want to do this early. We need to check the non-NULLness
> in infer_diff_ranges [...]

Correct.

> Would it make sense to give a better error message in:
>
>> +               if (!origin)
>> +                       die(_("failed to infer range-diff ranges"));
>
> above? (The failure sounds like it could be anything, but the
> reason for it is actually well understood: The origin was
> computed and as the boundary commit of the given range,
> or NULL if there is no boundary commit or multiple commits.

I'm not entirely happy with the generic error message either but
didn't come up with anything better which was both succinct and
actually helpful. I was hoping that a reviewer might suggest something
better.

> If we find not exactly one boundary, we cannot compute the
> range to give to the range-diff tool.

I considered allowing the argument to --range-diff to be much more
complex: to provide a way for the user to supply all information
needed for the invocation of git-range-diff (basically, to manually
supply arguments to git-range-diff) for cases when inference wasn't
possible. I even went so far as to consider allowing the
'creation-weight' to be specified as part of the --range-diff
argument. However, that sort of complexity is both difficult to
explain (document) and tends to be painful for users to specify (type
correctly).

Therefore, in the end, I opted for simplicity: namely, handle the
common cases with straight-forward minimal-learning-curve standard
options. Both --range-diff=v3 and --range-diff=v3~4..v3 are easy to
document and understand, as is the distinct --creation-weight= option.
This seems a good balance between extreme flexibility and relative
simplicity for handling common needs. (And, --range-diff is just a
convenience, after all; more complex scenarios can still be handled
manually by invoking git-range-diff followed by copy/paste.)

> Stepping back to the error message, I have no good
> suggestion on what to say there. Maybe we'd want to
> refactor the code in cmd_format_patch, that computes the
> origin:
>
>         if (prepare_revision_walk(&rev))
>                 die(_("revision walk setup failed"));
>         rev.boundary = 1;
>         while ((commit = get_revision(&rev)) != NULL) {
>                 if (commit->object.flags & BOUNDARY) {
>                         boundary_count++;
>                         origin = (boundary_count == 1) ? commit : NULL;
>                         continue;
>                 }
>
> We could prefix that with
>
>     need_origin = (range_diff && strstr(prev, "..")  || emit_diff_stat;
>
> and then if need_origin is about to be unset again we could issue
> a warning and die early after the loop warning about bad DAG form?
>
> I guess that can wait for a follow up patch.

I agree. The feature can be improved and made more fancy incrementally
as we gain better understanding of areas which need improvement. As a
first step, I wanted to keep it minimal but usable for the most common
cases.

Thanks for the review.

Re: [RFC PATCH 4/5] format-patch: teach --range-diff to respect -v/--reroll-count

[Eric Sunshine]

On Wed, May 30, 2018 at 3:03 PM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, May 30, 2018 at 1:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> The --range-diff option introduces the embedded range-diff generically
>> as "Changes since previous version:", however, we can do better when
>> --reroll-count is specified by emitting "Changes since v{n}:" instead.
>
> A very similar option that I used before finding reroll count is
> --subject-prefix
> I still use that for RFC/WIP tags, but I sometimes used it with "PATCHv2"
> as an argument, too.
>
> Would we want to extend the niceties of this patch to that workflow?

I would not include such functionality directly in this patch, as the
two ideas are only superficially related ("computing the previous
version number by *some* mechanism") but not related in actual
implementation.

Computing the previous version number by consulting --reroll-count, as
done by this patch, is deterministic and was just low-hanging fruit.
What you suggest probably involves heuristics and parsing, thus ought
to be done in its own patch (or patches). It's also the sort of
incremental improvement that can be done later (rather than in this
initial implementation) if someone deems it desirable.

BTW: You can use "git format-patch --rfc" for RFC patches (in fact, I
did so for this series).

> Unrelated to this patch: how does this series cope with range diffs
> that are not in commit-ish but patches on the file system?

I'm not following. Can you provide a concrete example to get me up to speed?

Re: [RFC PATCH 4/5] format-patch: teach --range-diff to respect -v/--reroll-count

[Stefan Beller]

On Wed, May 30, 2018 at 1:44 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

>> Unrelated to this patch: how does this series cope with range diffs
>> that are not in commit-ish but patches on the file system?
>
> I'm not following. Can you provide a concrete example to get me up to speed?

I was just wondering if things like

    git format-patch --range-diff=v3-00*.patch --reroll-count=4 -3

would be supported, but that doesn't seem to be the case, now that I read
the whole series. I don't think it is crucial to support right now.

This whole series is reviewed by me and I think it is good for inclusion
once we have a reroll of the range-diff series and aligned the command
name to call.

Thanks,
Stefan

Re: [RFC PATCH 4/5] format-patch: teach --range-diff to respect -v/--reroll-count

[Eric Sunshine]

On Wed, May 30, 2018 at 5:03 PM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, May 30, 2018 at 1:44 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> Unrelated to this patch: how does this series cope with range diffs
>>> that are not in commit-ish but patches on the file system?
>>
>> I'm not following. Can you provide a concrete example to get me up to speed?
>
> I was just wondering if things like
>
>     git format-patch --range-diff=v3-00*.patch --reroll-count=4 -3
>
> would be supported, but that doesn't seem to be the case, now that I read
> the whole series. I don't think it is crucial to support right now.

Okay, that's what I thought you meant (upon thinking harder about it
after hitting Send). git-range-diff doesn't support that mode of
operation (nor does 'tbdiff', as far as I can tell), so as a thin
wrapper around git-range-diff, "git format-patch --range-diff" doesn't
support it either.

Perhaps that sort of functionality could implement later by someone,
if desired. In the meantime, the following (manual procedure) would
approximate it:

    git checkout --detach <base>
    git am v3-00*.patch
    git format-patch --range-diff=...

> This whole series is reviewed by me and I think it is good for inclusion
> once we have a reroll of the range-diff series and aligned the command
> name to call.

Thanks.

Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff

[Duy Nguyen]

On Wed, May 30, 2018 at 10:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> Dscho recently implemented a 'tbdiff' replacement as a Git builtin named
> git-branch-diff[1] which computes differences between two versions of a
> patch series. Such a diff can be a useful aid for reviewers when
> inserted into a cover letter. However, doing so requires manual
> generation (invoking git-branch-diff) and copy/pasting the result into
> the cover letter.

Another option which I wanted to go is delegate part of cover letter
generation to a hook (or just a config key that contains a shell
command). This way it's easier to customize cover letters. We could
still have a good fallback that does shortlog, diffstat and tbdiff.
-- 
Duy

Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff

[Eric Sunshine]

On Wed, Jun 6, 2018 at 3:16 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Wed, May 30, 2018 at 10:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> Dscho recently implemented a 'tbdiff' replacement as a Git builtin named
>> git-branch-diff[1] which computes differences between two versions of a
>> patch series. Such a diff can be a useful aid for reviewers when
>> inserted into a cover letter. However, doing so requires manual
>> generation (invoking git-branch-diff) and copy/pasting the result into
>> the cover letter.
>
> Another option which I wanted to go is delegate part of cover letter
> generation to a hook (or just a config key that contains a shell
> command). This way it's easier to customize cover letters. We could
> still have a good fallback that does shortlog, diffstat and tbdiff.

It is common on this mailing list to turn down requests for new hooks
when the requested functionality could just as easily be implemented
via a wrapper script. So, my knee-jerk reaction is that a hook to
customize the cover letter may be overkill when the same functionality
could likely be implemented relatively easily by a shell script which
invokes git-format-patch and customizes the cover letter
after-the-fact. Same argument regarding a config key holding a shell
command. But, perhaps there are cases which don't occur to me which
could be helped by a config variable or such.

Of course, by the same reasoning, the --range-diff functionality
implemented by this patch series, which is just a convenience, could
be handled by a wrapper script, thus is not strictly needed. On the
other hand, given that interdiffs and range-diffs are so regularly
used in re-rolls on this list (and perhaps other mailing list-based
projects) may be argument enough in favor of having such an option
built into git-format-patch.

Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff

[Duy Nguyen]

On Thu, Jun 7, 2018 at 10:34 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Jun 6, 2018 at 3:16 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Wed, May 30, 2018 at 10:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> Dscho recently implemented a 'tbdiff' replacement as a Git builtin named
>>> git-branch-diff[1] which computes differences between two versions of a
>>> patch series. Such a diff can be a useful aid for reviewers when
>>> inserted into a cover letter. However, doing so requires manual
>>> generation (invoking git-branch-diff) and copy/pasting the result into
>>> the cover letter.
>>
>> Another option which I wanted to go is delegate part of cover letter
>> generation to a hook (or just a config key that contains a shell
>> command). This way it's easier to customize cover letters. We could
>> still have a good fallback that does shortlog, diffstat and tbdiff.
>
> It is common on this mailing list to turn down requests for new hooks
> when the requested functionality could just as easily be implemented
> via a wrapper script. So, my knee-jerk reaction is that a hook to
> customize the cover letter may be overkill when the same functionality
> could likely be implemented relatively easily by a shell script which
> invokes git-format-patch and customizes the cover letter
> after-the-fact. Same argument regarding a config key holding a shell
> command. But, perhaps there are cases which don't occur to me which
> could be helped by a config variable or such.

I think format-patch --cover-letter nowadays does more stuff that's
not so easy to simply rewrite it in a shell script. My original
problem with format-patch is it hard codes shortlog settings and you
can't list patches with patch number (e.g. "[1/2] foo bar"). The
simplest way is let format-patch does it stuff as usual and
"outsource" some cover letter's body generation to a script.

But it's ok. I could try to code the patch numbering thing in
format-patch and maybe submit a patch or two for that later.

> Of course, by the same reasoning, the --range-diff functionality
> implemented by this patch series, which is just a convenience, could
> be handled by a wrapper script, thus is not strictly needed. On the
> other hand, given that interdiffs and range-diffs are so regularly
> used in re-rolls on this list (and perhaps other mailing list-based
> projects) may be argument enough in favor of having such an option
> built into git-format-patch.
-- 
Duy

Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff

[Johannes Schindelin]

Hi Eric,

On Wed, 30 May 2018, Eric Sunshine wrote:

> Dscho recently implemented a 'tbdiff' replacement as a Git builtin named
> git-branch-diff[1] which computes differences between two versions of a
> patch series. Such a diff can be a useful aid for reviewers when
> inserted into a cover letter. However, doing so requires manual
> generation (invoking git-branch-diff) and copy/pasting the result into
> the cover letter.
> 
> This series fully automates the process by adding a --range-diff option
> to git-format-patch.

Nice!

> It is RFC for a couple reasons:
> 
> * The final name for the 'tbdiff' replacement has not yet been nailed
>   down. The name git-branch-diff is moribund[2]; Dscho favors merging
>   the functionality into git-branch as a new --diff option[3]; others
>   prefer a standalone command named git-range-diff or
>   git-series-diff[4] or similar.

I think this has been settled now: range-diff it is.

> * I have some ideas for future enhancements and want to be careful not
>   to lock in a UI which doesn't mesh well with them (though I think the
>   current UI turned out reasonably well). First, I foresee a
>   complementary --interdiff option for inserting an interdiff-style diff
>   for cases when that style is easier to read or simply more
>   appropriate. Second, although the current patch series only supports
>   --range-diff for the cover letter, some people insert interdiff- or
>   tbdiff-style diffs (indented) into the commentary section of
>   standalone patches. Although this often makes for a noisy mess, it is
>   periodically useful.

Sure.

> This series is built atop js/branch-diff in 'pu'.
> 
> [1]: https://public-inbox.org/git/cover.1525448066.git.johannes.schindelin@gmx.de/
> [2]: https://public-inbox.org/git/nycvar.QRO.7.76.6.1805061401260.77@tvgsbejvaqbjf.bet/
> [3]: https://public-inbox.org/git/nycvar.QRO.7.76.6.1805062155120.77@tvgsbejvaqbjf.bet/
> [4]: https://public-inbox.org/git/xmqqin7gzbkb.fsf@gitster-ct.c.googlers.com/
> 
> Eric Sunshine (5):
>   format-patch: allow additional generated content in
>     make_cover_letter()
>   format-patch: add --range-diff option to embed diff in cover letter
>   format-patch: extend --range-diff to accept revision range
>   format-patch: teach --range-diff to respect -v/--reroll-count
>   format-patch: add --creation-weight tweak for --range-diff
> 
>  Documentation/git-format-patch.txt |  18 +++++
>  builtin/log.c                      | 119 ++++++++++++++++++++++++-----
>  t/t7910-branch-diff.sh             |  16 ++++
>  3 files changed, 132 insertions(+), 21 deletions(-)
> 
> -- 
> 2.17.1.1235.ge295dfb56e

Thanks!
Dscho

Re: [RFC PATCH 1/5] format-patch: allow additional generated content in make_cover_letter()

[Johannes Schindelin]

Hi Eric,

On Wed, 30 May 2018, Eric Sunshine wrote:

> make_cover_letter() returns early when it lacks sufficient state to emit
> a diffstat, which makes it difficult to extend the function to reliably
> emit additional generated content. Work around this shortcoming by
> factoring diffstat-printing logic out to its own function and calling it
> as needed without otherwise inhibiting normal control flow.
> 
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>

Makes sense.

Ciao,
Dscho

> ---
>  builtin/log.c | 43 +++++++++++++++++++++++--------------------
>  1 file changed, 23 insertions(+), 20 deletions(-)
> 
> diff --git a/builtin/log.c b/builtin/log.c
> index 71f68a3e4f..e01a256c11 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -992,6 +992,26 @@ static char *find_branch_name(struct rev_info *rev)
>  	return branch;
>  }
>  
> +static void emit_diffstat(struct rev_info *rev,
> +			  struct commit *origin, struct commit *head)
> +{
> +	struct diff_options opts;
> +
> +	memcpy(&opts, &rev->diffopt, sizeof(opts));
> +	opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> +	opts.stat_width = MAIL_DEFAULT_WRAP;
> +
> +	diff_setup_done(&opts);
> +
> +	diff_tree_oid(&origin->tree->object.oid,
> +		      &head->tree->object.oid,
> +		      "", &opts);
> +	diffcore_std(&opts);
> +	diff_flush(&opts);
> +
> +	fprintf(rev->diffopt.file, "\n");
> +}
> +
>  static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  			      struct commit *origin,
>  			      int nr, struct commit **list,
> @@ -1005,7 +1025,6 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  	struct strbuf sb = STRBUF_INIT;
>  	int i;
>  	const char *encoding = "UTF-8";
> -	struct diff_options opts;
>  	int need_8bit_cte = 0;
>  	struct pretty_print_context pp = {0};
>  	struct commit *head = list[0];
> @@ -1055,25 +1074,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  
>  	shortlog_output(&log);
>  
> -	/*
> -	 * We can only do diffstat with a unique reference point
> -	 */
> -	if (!origin)
> -		return;
> -
> -	memcpy(&opts, &rev->diffopt, sizeof(opts));
> -	opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> -	opts.stat_width = MAIL_DEFAULT_WRAP;
> -
> -	diff_setup_done(&opts);
> -
> -	diff_tree_oid(&origin->tree->object.oid,
> -		      &head->tree->object.oid,
> -		      "", &opts);
> -	diffcore_std(&opts);
> -	diff_flush(&opts);
> -
> -	fprintf(rev->diffopt.file, "\n");
> +	/* We can only do diffstat with a unique reference point */
> +	if (origin)
> +		emit_diffstat(rev, origin, head);
>  }
>  
>  static const char *clean_message_id(const char *msg_id)
> -- 
> 2.17.1.1235.ge295dfb56e
> 
> 

Re: [RFC PATCH 1/5] format-patch: allow additional generated content in make_cover_letter()

[Eric Sunshine]

On Tue, Jul 17, 2018 at 6:15 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Wed, 30 May 2018, Eric Sunshine wrote:
> > make_cover_letter() returns early when it lacks sufficient state to emit
> > a diffstat, which makes it difficult to extend the function to reliably
> > emit additional generated content. Work around this shortcoming by
> > factoring diffstat-printing logic out to its own function and calling it
> > as needed without otherwise inhibiting normal control flow.
> >
> > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>
> Makes sense.

Thanks, but it's probably not worth spending time reviewing this RFC
series. I already have a new series in the works (in fact, mostly
finished) in which the implementation is drastically changed from this
one. Aside from adding an --interdiff option to git-format-patch (in
addition to a --range-diff option) and allowing interdiff and
range-diff to be added as commentary to a single-patch, the new series
also takes advantage of the newly-libified range-diff engine rather
than running git-range-diff as a command. So, most or all of the code
has changed.

(Though, perhaps it wouldn't hurt to review the documentation changes
in this RFC series to see if I botched how I described the option.)

Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter

[Johannes Schindelin]

Hi Eric,

On Wed, 30 May 2018, Eric Sunshine wrote:

> diff --git a/builtin/log.c b/builtin/log.c
> index e01a256c11..460c31a293 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -28,6 +28,7 @@
>  #include "mailmap.h"
>  #include "gpg-interface.h"
>  #include "progress.h"
> +#include "run-command.h"
>  
>  #define MAIL_DEFAULT_WRAP 72
>  
> @@ -992,6 +993,25 @@ static char *find_branch_name(struct rev_info *rev)
>  	return branch;
>  }
>  
> +static void infer_diff_ranges(struct argv_array *args,
> +			      const char *prev,
> +			      struct commit *head)
> +{
> +	argv_array_pushf(args, "%s...%s", prev,
> +			 oid_to_hex(&head->object.oid));
> +}
> +
> +static int get_range_diff(struct strbuf *sb,

If you rename `sb` to `out`, it makes it more obvious to the casual reader
that this strbuf will receive the output.

> +			  const struct argv_array *ranges)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +
> +	cp.git_cmd = 1;
> +	argv_array_pushl(&cp.args, "branch-diff", "--no-color", NULL);

Obviously, this needs to be "range-diff" now.

> +	argv_array_pushv(&cp.args, ranges->argv);

As there must be exactly two ranges, it would make more sense to pass them
explicitly. And then you can use one single call to `argv_array_pushl()`
instead.

> +	return capture_command(&cp, sb, 0);
> +}
> +
>  static void emit_diffstat(struct rev_info *rev,
>  			  struct commit *origin, struct commit *head)
>  {
> @@ -1016,7 +1036,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  			      struct commit *origin,
>  			      int nr, struct commit **list,
>  			      const char *branch_name,
> -			      int quiet)
> +			      int quiet,
> +			      const char *range_diff)
>  {
>  	const char *committer;
>  	const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
> @@ -1028,15 +1049,25 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  	int need_8bit_cte = 0;
>  	struct pretty_print_context pp = {0};
>  	struct commit *head = list[0];
> +	struct strbuf diff = STRBUF_INIT;

I guess you want to call it `diff` instead of `range_diff` because a
future change will reuse this for the interdiff instead? And then also to
avoid a naming conflict with the parameter.

Dunno. I would still call it `range_diff` until the day comes (if ever)
when `--interdiff` is implemented. And I would call the `range_diff`
parameter `range_diff_opt` instead, or some such.

Or maybe use `extra_footer` instead of `diff`.

>  	if (!cmit_fmt_is_mail(rev->commit_format))
>  		die(_("Cover letter needs email format"));
>  
>  	committer = git_committer_info(0);
>  
> +	/* might die from bad user input so try before creating cover letter */
> +	if (range_diff) {
> +		struct argv_array ranges = ARGV_ARRAY_INIT;
> +		infer_diff_ranges(&ranges, range_diff, head);
> +		if (get_range_diff(&diff, &ranges))
> +			die(_("failed to generate range-diff"));

BTW I like to have an extra space in front of all the range-diff lines, to
make it easier to discern them from the rest.

> +		argv_array_clear(&ranges);
> +	}
> +
>  	if (!use_stdout &&
>  	    open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
> -		return;
> +		goto done;

Hmm. If you think you will add more `goto done`s in the future, I guess
this is okay. Otherwise, it would probably make sense to go ahead and
simply `strbuf_release(&diff)` before `return`ing.

>  	log_write_email_headers(rev, head, &pp.after_subject, &need_8bit_cte);
>  
> @@ -1077,6 +1108,17 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  	/* We can only do diffstat with a unique reference point */
>  	if (origin)
>  		emit_diffstat(rev, origin, head);
> +
> +	if (diff.len) {
> +		FILE *fp = rev->diffopt.file;
> +		fputs(_("Changes since previous version:"), fp);
> +		fputs("\n\n", fp);
> +		fputs(diff.buf, fp);
> +		fputc('\n', fp);
> +	}
> +
> +done:
> +	strbuf_release(&diff);
>  }
>  
>  static const char *clean_message_id(const char *msg_id)
> @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	struct base_tree_info bases;
>  	int show_progress = 0;
>  	struct progress *progress = NULL;
> +	const char *range_diff = NULL;

Maybe `range_diff_opt`? It's not exactly the range diff that is contained
in this variable.

>  	const struct option builtin_format_patch_options[] = {
>  		{ OPTION_CALLBACK, 'n', "numbered", &numbered, NULL,
> @@ -1511,6 +1554,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		OPT__QUIET(&quiet, N_("don't print the patch filenames")),
>  		OPT_BOOL(0, "progress", &show_progress,
>  			 N_("show progress while generating patches")),
> +		OPT_STRING(0, "range-diff", &range_diff, N_("rev-range"),
> +			   N_("show changes against <rev-range> in cover letter")),
>  		OPT_END()
>  	};
>  
> @@ -1733,6 +1778,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	if (numbered)
>  		rev.total = total + start_number - 1;
>  
> +	if (range_diff && !cover_letter)
> +		die(_("--range-diff requires --cover-letter"));

I guess this will be changed in a future patch, allowing a single patch to
ship with a range diff, too?

> +
>  	if (!signature) {
>  		; /* --no-signature inhibits all signatures */
>  	} else if (signature && signature != git_version_string) {
> @@ -1764,7 +1812,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		if (thread)
>  			gen_message_id(&rev, "cover");
>  		make_cover_letter(&rev, use_stdout,
> -				  origin, nr, list, branch_name, quiet);
> +				  origin, nr, list, branch_name, quiet,
> +				  range_diff);
>  		print_bases(&bases, rev.diffopt.file);
>  		print_signature(rev.diffopt.file);
>  		total++;
> diff --git a/t/t7910-branch-diff.sh b/t/t7910-branch-diff.sh
> index a7fece8804..edbd69b6f8 100755
> --- a/t/t7910-branch-diff.sh
> +++ b/t/t7910-branch-diff.sh
> @@ -141,4 +141,19 @@ test_expect_success 'changed message' '
>  	test_cmp expected actual
>  '
>  
> +format_patch () {
> +	title=$1 &&
> +	range=$2 &&
> +	test_expect_success "format-patch --range-diff ($title)" '
> +		git format-patch --stdout --cover-letter --range-diff=$range \
> +			master..unmodified >actual &&
> +		grep "= 1: .* s/5/A" actual &&
> +		grep "= 2: .* s/4/A" actual &&
> +		grep "= 3: .* s/11/B" actual &&
> +		grep "= 4: .* s/12/B" actual

I guess this might make sense if `format_patch` was not a function, but it
is specifically marked as a function... so... shouldn't these `grep`s also
be using function parameters?

> +	'
> +}
> +
> +format_patch 'B...C' topic
> +
>  test_done
> -- 
> 2.17.1.1235.ge295dfb56e

Thanks,
Dscho

Re: [RFC PATCH 3/5] format-patch: extend --range-diff to accept revision range

[Johannes Schindelin]

Hi Eric,

On Wed, 30 May 2018, Eric Sunshine wrote:

> diff --git a/builtin/log.c b/builtin/log.c
> index 460c31a293..e38cf06050 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -995,10 +995,20 @@ static char *find_branch_name(struct rev_info *rev)
>  
>  static void infer_diff_ranges(struct argv_array *args,
>  			      const char *prev,
> +			      struct commit *origin,
>  			      struct commit *head)
>  {
> -	argv_array_pushf(args, "%s...%s", prev,
> -			 oid_to_hex(&head->object.oid));
> +	if (strstr(prev, "..")) {
> +		if (!origin)
> +			die(_("failed to infer range-diff ranges"));
> +		argv_array_push(args, prev);
> +		argv_array_pushf(args, "%s..%s",
> +				 oid_to_hex(&origin->object.oid),
> +				 oid_to_hex(&head->object.oid));
> +	} else {
> +		argv_array_pushf(args, "%s...%s", prev,
> +				 oid_to_hex(&head->object.oid));
> +	}

This would be easier to read if the order was inverted:

	if (!strstr(...))
		...
	else if (!origin)
		die(...)
	else {
		...
	}

Otherwise, it makes sense to me.

Thanks,
Dscho

>  }
>  
>  static int get_range_diff(struct strbuf *sb,
> @@ -1059,7 +1069,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  	/* might die from bad user input so try before creating cover letter */
>  	if (range_diff) {
>  		struct argv_array ranges = ARGV_ARRAY_INIT;
> -		infer_diff_ranges(&ranges, range_diff, head);
> +		infer_diff_ranges(&ranges, range_diff, origin, head);
>  		if (get_range_diff(&diff, &ranges))
>  			die(_("failed to generate range-diff"));
>  		argv_array_clear(&ranges);
> diff --git a/t/t7910-branch-diff.sh b/t/t7910-branch-diff.sh
> index edbd69b6f8..c0e8668ed9 100755
> --- a/t/t7910-branch-diff.sh
> +++ b/t/t7910-branch-diff.sh
> @@ -155,5 +155,6 @@ format_patch () {
>  }
>  
>  format_patch 'B...C' topic
> +format_patch 'A..B A..C' master..topic
>  
>  test_done
> -- 
> 2.17.1.1235.ge295dfb56e
> 
> 

Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter

[Eric Sunshine]

Thanks for the review comments. In the new version of the series
(almost complete), almost the entire implementation has changed,
including most of the stuff on which you commented. Anyhow, see my
responses to your comments below...

On Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Wed, 30 May 2018, Eric Sunshine wrote:
> > +static int get_range_diff(struct strbuf *sb,
>
> If you rename `sb` to `out`, it makes it more obvious to the casual reader
> that this strbuf will receive the output.

This is gone in the re-roll.

> > +     cp.git_cmd = 1;
> > +     argv_array_pushl(&cp.args, "branch-diff", "--no-color", NULL);
>
> Obviously, this needs to be "range-diff" now.

The re-roll takes advantage of the libified range-diff rather than
invoking it as a command.

> > +     argv_array_pushv(&cp.args, ranges->argv);
>
> As there must be exactly two ranges, it would make more sense to pass them
> explicitly. And then you can use one single call to `argv_array_pushl()`
> instead.

Gone in the re-roll.

> > +     struct strbuf diff = STRBUF_INIT;
>
> I guess you want to call it `diff` instead of `range_diff` because a
> future change will reuse this for the interdiff instead? And then also to
> avoid a naming conflict with the parameter.
>
> Dunno. I would still call it `range_diff` until the day comes (if ever)
> when `--interdiff` is implemented. And I would call the `range_diff`
> parameter `range_diff_opt` instead, or some such.

Sharing the variable between interdiff and range-diff was the idea
initially, however, I later decided that the --range-diff and
--interdiff options didn't need to be mutually-exclusive, so, in the
re-roll, all variables now have distinct names (no commonality between
them).

> > +     if (range_diff) {
> > +             struct argv_array ranges = ARGV_ARRAY_INIT;
> > +             infer_diff_ranges(&ranges, range_diff, head);
> > +             if (get_range_diff(&diff, &ranges))
> > +                     die(_("failed to generate range-diff"));
>
> BTW I like to have an extra space in front of all the range-diff lines, to
> make it easier to discern them from the rest.

I'm not sure what you mean. Perhaps I'm misreading your comment.

> >       if (!use_stdout &&
> >           open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
> > -             return;
> > +             goto done;
>
> Hmm. If you think you will add more `goto done`s in the future, I guess
> this is okay. Otherwise, it would probably make sense to go ahead and
> simply `strbuf_release(&diff)` before `return`ing.

In the re-roll, there are several more things which need to be cleaned
up, so the 'goto' makes life easier.

> > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> > +     const char *range_diff = NULL;
>
> Maybe `range_diff_opt`? It's not exactly the range diff that is contained
> in this variable.

I could, though I was trying to keep it shorter rather than longer.
This is still the same in the re-roll, but I can rename it if you
insist.

> > +     if (range_diff && !cover_letter)
> > +             die(_("--range-diff requires --cover-letter"));
>
> I guess this will be changed in a future patch, allowing a single patch to
> ship with a range diff, too?

Yes, that's already the case in the re-roll.

> > +format_patch () {
> > +     title=$1 &&
> > +     range=$2 &&
> > +     test_expect_success "format-patch --range-diff ($title)" '
> > +             git format-patch --stdout --cover-letter --range-diff=$range \
> > +                     master..unmodified >actual &&
> > +             grep "= 1: .* s/5/A" actual &&
> > +             grep "= 2: .* s/4/A" actual &&
> > +             grep "= 3: .* s/11/B" actual &&
> > +             grep "= 4: .* s/12/B" actual
>
> I guess this might make sense if `format_patch` was not a function, but it
> is specifically marked as a function... so... shouldn't these `grep`s also
> be using function parameters?

A later patch adds a second test which specifies the same ranges but
in a different way, so the result will be the same, hence the
hard-coded grep'ing. The function avoids repetition across the two
tests. I suppose I could do this a bit differently, though, to avoid
pretending it's a general-purpose function.

Re: [RFC PATCH 3/5] format-patch: extend --range-diff to accept revision range

[Eric Sunshine]

On Tue, Jul 17, 2018 at 6:45 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Wed, 30 May 2018, Eric Sunshine wrote:
> > +     if (strstr(prev, "..")) {
> > +             if (!origin)
> > +                     die(_("failed to infer range-diff ranges"));
> > +             argv_array_push(args, prev);
> > +             argv_array_pushf(args, "%s..%s",
> > +                              oid_to_hex(&origin->object.oid),
> > +                              oid_to_hex(&head->object.oid));
> > +     } else {
> > +             argv_array_pushf(args, "%s...%s", prev,
> > +                              oid_to_hex(&head->object.oid));
> > +     }
>
> This would be easier to read if the order was inverted:
>
>         if (!strstr(...))
>                 ...
>         else if (!origin)
>                 die(...)
>         else {
>                 ...
>         }
>
> Otherwise, it makes sense to me.

Thanks, I re-wrote it that way in the re-roll already.

Re: [RFC PATCH 5/5] format-patch: add --creation-weight tweak for --range-diff

[Johannes Schindelin]

Hi Eric,

On Wed, 30 May 2018, Eric Sunshine wrote:

> When generating a range-diff, matching up commits between two version of
> a patch series involves heuristics, thus may give unexpected results.
> git-branch-diff allows tweaking the heuristic via --creation-weight.
> Follow suit by accepting --creation-weight in combination with
> --range-diff when generating a range-diff for a cover-letter.

Since I opted to change this to `--creation-factor`, taking an integer
between 0 and 100 (essentially), this patch will need heavy adjustment ;-=)

Thanks,
Dscho

> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  Documentation/git-format-patch.txt |  8 +++++++-
>  builtin/log.c                      | 19 +++++++++++++++----
>  2 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index 25026ae26e..7ed9ec9dae 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -23,7 +23,7 @@ SYNOPSIS
>  		   [(--reroll-count|-v) <n>]
>  		   [--to=<email>] [--cc=<email>]
>  		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
> -		   [--range-diff=<previous>]
> +		   [--range-diff=<previous> [--creation-weight=<factor>]]
>  		   [--progress]
>  		   [<common diff options>]
>  		   [ <since> | <revision range> ]
> @@ -240,6 +240,12 @@ feeding the result to `git send-email`.
>  	disjoint (for example `git format-patch --cover-letter
>  	--range-diff=feature/v1~3..feature/v1 -3 feature/v2`).
>  
> +--creation-weight=<factor>::
> +	Used with `--range-diff`, tweak the heuristic which matches up commits
> +	between the previous and current series of patches by adjusting the
> +	creation/deletion cost fudge factor. See linkgit:git-branch-diff[1])
> +	for details.
> +
>  --notes[=<ref>]::
>  	Append the notes (see linkgit:git-notes[1]) for the commit
>  	after the three-dash line.
> diff --git a/builtin/log.c b/builtin/log.c
> index 3089d3a50a..2c49011b51 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1012,12 +1012,16 @@ static void infer_diff_ranges(struct argv_array *args,
>  }
>  
>  static int get_range_diff(struct strbuf *sb,
> -			  const struct argv_array *ranges)
> +			  const struct argv_array *ranges,
> +			  const char *creation_weight)
>  {
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  
>  	cp.git_cmd = 1;
>  	argv_array_pushl(&cp.args, "branch-diff", "--no-color", NULL);
> +	if (creation_weight)
> +		argv_array_pushf(&cp.args,
> +				 "--creation-weight=%s", creation_weight);
>  	argv_array_pushv(&cp.args, ranges->argv);
>  	return capture_command(&cp, sb, 0);
>  }
> @@ -1047,7 +1051,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  			      int nr, struct commit **list,
>  			      const char *branch_name,
>  			      int quiet,
> -			      const char *range_diff)
> +			      const char *range_diff,
> +			      const char *creation_weight)
>  {
>  	const char *committer;
>  	const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
> @@ -1070,7 +1075,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  	if (range_diff) {
>  		struct argv_array ranges = ARGV_ARRAY_INIT;
>  		infer_diff_ranges(&ranges, range_diff, origin, head);
> -		if (get_range_diff(&diff, &ranges))
> +		if (get_range_diff(&diff, &ranges, creation_weight))
>  			die(_("failed to generate range-diff"));
>  		argv_array_clear(&ranges);
>  	}
> @@ -1495,6 +1500,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	int show_progress = 0;
>  	struct progress *progress = NULL;
>  	const char *range_diff = NULL;
> +	const char *creation_weight = NULL;
>  
>  	const struct option builtin_format_patch_options[] = {
>  		{ OPTION_CALLBACK, 'n', "numbered", &numbered, NULL,
> @@ -1570,6 +1576,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  			 N_("show progress while generating patches")),
>  		OPT_STRING(0, "range-diff", &range_diff, N_("rev-range"),
>  			   N_("show changes against <rev-range> in cover letter")),
> +		OPT_STRING(0, "creation-weight", &creation_weight, N_("factor"),
> +			   N_("fudge factor by which creation is weighted")),
>  		OPT_END()
>  	};
>  
> @@ -1664,6 +1672,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		die (_("--subject-prefix/--rfc and -k are mutually exclusive."));
>  	rev.preserve_subject = keep_subject;
>  
> +	if (creation_weight && !range_diff)
> +		die(_("--creation-weight requires --range-diff"));
> +
>  	argc = setup_revisions(argc, argv, &rev, &s_r_opt);
>  	if (argc > 1)
>  		die (_("unrecognized argument: %s"), argv[1]);
> @@ -1827,7 +1838,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  			gen_message_id(&rev, "cover");
>  		make_cover_letter(&rev, use_stdout,
>  				  origin, nr, list, branch_name, quiet,
> -				  range_diff);
> +				  range_diff, creation_weight);
>  		print_bases(&bases, rev.diffopt.file);
>  		print_signature(rev.diffopt.file);
>  		total++;
> -- 
> 2.17.1.1235.ge295dfb56e
> 
> 

Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter

[Johannes Schindelin]

Hi Eric,

On Tue, 17 Jul 2018, Eric Sunshine wrote:

> On Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > On Wed, 30 May 2018, Eric Sunshine wrote:
> 
> > > +     if (range_diff) {
> > > +             struct argv_array ranges = ARGV_ARRAY_INIT;
> > > +             infer_diff_ranges(&ranges, range_diff, head);
> > > +             if (get_range_diff(&diff, &ranges))
> > > +                     die(_("failed to generate range-diff"));
> >
> > BTW I like to have an extra space in front of all the range-diff lines, to
> > make it easier to discern them from the rest.
> 
> I'm not sure what you mean. Perhaps I'm misreading your comment.

Sorry, I was really unclear.

In the cover letters sent out by GitGitGadget (or earlier, my
mail-patch-series.sh command), I took pains to indent the entire
range-diff (or interdiff) with a single space. That is, the footer
"Range-diff vs v<n>:" is not indented at all, but all subsequent lines of
the range-diff have a leading space.

The original reason was to stop confusing `git apply` when sending an
interdiff as part of a single patch without a cover letter (in which case
mail-patch-series.sh inserted the interdiff below the `---` marker, and
the interdiff would have looked like the start of the real diff
otherwise).

In the meantime, I got used to this indentation so much that I do not want
to miss it, it is a relatively easy and intuitive visual marker.

This, however, will be harder to achieve now that you are using the
libified range-diff.

> > > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> > > +     const char *range_diff = NULL;
> >
> > Maybe `range_diff_opt`? It's not exactly the range diff that is contained
> > in this variable.
> 
> I could, though I was trying to keep it shorter rather than longer.
> This is still the same in the re-roll, but I can rename it if you
> insist.

I think it will confuse me in the future if I read `range_diff` and even
the data type suggests that it could hold the output of a `git range-diff
<options>` run.

So I would like to insist.

> > > +format_patch () {
> > > +     title=$1 &&
> > > +     range=$2 &&
> > > +     test_expect_success "format-patch --range-diff ($title)" '
> > > +             git format-patch --stdout --cover-letter --range-diff=$range \
> > > +                     master..unmodified >actual &&
> > > +             grep "= 1: .* s/5/A" actual &&
> > > +             grep "= 2: .* s/4/A" actual &&
> > > +             grep "= 3: .* s/11/B" actual &&
> > > +             grep "= 4: .* s/12/B" actual
> >
> > I guess this might make sense if `format_patch` was not a function, but it
> > is specifically marked as a function... so... shouldn't these `grep`s also
> > be using function parameters?
> 
> A later patch adds a second test which specifies the same ranges but
> in a different way, so the result will be the same, hence the
> hard-coded grep'ing. The function avoids repetition across the two
> tests. I suppose I could do this a bit differently, though, to avoid
> pretending it's a general-purpose function.

If you can think of a way that would make this easier to read for, say,
myself if I ever find myself debugging a regression caught by this test, I
would appreciate that.

Ciao,
Dscho

Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff

[Andrei Rybak]

On 2018-05-30 10:03, Eric Sunshine wrote:
> Dscho recently implemented a 'tbdiff' replacement as a Git builtin named
> git-branch-diff[1] which computes differences between two versions of a
> patch series. Such a diff can be a useful aid for reviewers when
> inserted into a cover letter. However, doing so requires manual
> generation (invoking git-branch-diff) and copy/pasting the result into
> the cover letter.
> 
> This series fully automates the process by adding a --range-diff option
> to git-format-patch. 

[...]

> 
> Eric Sunshine (5):
>   format-patch: allow additional generated content in
>     make_cover_letter()
>   format-patch: add --range-diff option to embed diff in cover letter
>   format-patch: extend --range-diff to accept revision range
>   format-patch: teach --range-diff to respect -v/--reroll-count
>   format-patch: add --creation-weight tweak for --range-diff
> 
>  Documentation/git-format-patch.txt |  18 +++++
>  builtin/log.c                      | 119 ++++++++++++++++++++++++-----
>  t/t7910-branch-diff.sh             |  16 ++++
>  3 files changed, 132 insertions(+), 21 deletions(-)

Would it make sense to mention new option in the cover letter section of
Documentation/SubmittingPatches?

--
Best regards, Andrei Rybak

Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff

[Johannes Schindelin]

Hi Andrei,

On Thu, 26 Jul 2018, Andrei Rybak wrote:

> On 2018-05-30 10:03, Eric Sunshine wrote:
> > Dscho recently implemented a 'tbdiff' replacement as a Git builtin named
> > git-branch-diff[1] which computes differences between two versions of a
> > patch series. Such a diff can be a useful aid for reviewers when
> > inserted into a cover letter. However, doing so requires manual
> > generation (invoking git-branch-diff) and copy/pasting the result into
> > the cover letter.
> > 
> > This series fully automates the process by adding a --range-diff option
> > to git-format-patch. 
> 
> [...]
> 
> > 
> > Eric Sunshine (5):
> >   format-patch: allow additional generated content in
> >     make_cover_letter()
> >   format-patch: add --range-diff option to embed diff in cover letter
> >   format-patch: extend --range-diff to accept revision range
> >   format-patch: teach --range-diff to respect -v/--reroll-count
> >   format-patch: add --creation-weight tweak for --range-diff
> > 
> >  Documentation/git-format-patch.txt |  18 +++++
> >  builtin/log.c                      | 119 ++++++++++++++++++++++++-----
> >  t/t7910-branch-diff.sh             |  16 ++++
> >  3 files changed, 132 insertions(+), 21 deletions(-)
> 
> Would it make sense to mention new option in the cover letter section of
> Documentation/SubmittingPatches?

I would be hesitant to add it there already. Let's prove first that these
options are really as useful as we hope they are.

It might turn out that in many cases, the range-diff is just stupidly
unreadable, for example. Personally, I already miss the color coding when
looking at range-diffs sent via mails.

Ciao,
Johannes

Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter

[Eric Sunshine]

On Thu, Jul 26, 2018 at 6:56 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Tue, 17 Jul 2018, Eric Sunshine wrote:
> > On Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > > BTW I like to have an extra space in front of all the range-diff lines, to
> > > make it easier to discern them from the rest.
> >
> > I'm not sure what you mean. Perhaps I'm misreading your comment.
>
> Sorry, I was really unclear.
>
> In the cover letters sent out by GitGitGadget (or earlier, my
> mail-patch-series.sh command), I took pains to indent the entire
> range-diff (or interdiff) with a single space. That is, the footer
> "Range-diff vs v<n>:" is not indented at all, but all subsequent lines of
> the range-diff have a leading space.
>
> The original reason was to stop confusing `git apply` when sending an
> interdiff as part of a single patch without a cover letter (in which case
> mail-patch-series.sh inserted the interdiff below the `---` marker, and
> the interdiff would have looked like the start of the real diff
> otherwise).

The new version[1] likewise indents the interdiff to avoid confusing
git-am / git-apply.

[1]: https://public-inbox.org/git/20180722095717.17912-1-sunshine@sunshineco.com/

> In the meantime, I got used to this indentation so much that I do not want
> to miss it, it is a relatively easy and intuitive visual marker.
>
> This, however, will be harder to achieve now that you are using the
> libified range-diff.

I toyed with indenting the range-diff in both the cover letter and
below the "---" line in a patch. With the libified range-diff, doing
so involves modifying the range-diff implementation (rather than
having the consumer of the range-diff manage the indentation locally),
so it adds a bit of complexity to show_range_diff(), though perhaps
not too much.

However, I opted against it for a few reasons. First, "header" lines
apart, all lines of the range-diff are already indented, and the
existing indentation was sufficient (for me, at least) as a visual
marker. Second, range-diffs tend to be _wide_, especially the header
lines, and I was loath to make it wider by indenting more. Third, due
to the existing indentation of the diff proper, a range-diff won't
confuse git-am / git-apply, nor will the unindented header lines, so
extra indentation seemed superfluous.

> > > > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> > > > +     const char *range_diff = NULL;
> > >
> > > Maybe `range_diff_opt`? It's not exactly the range diff that is contained
> > > in this variable.
> >
> > I could, though I was trying to keep it shorter rather than longer.
> > This is still the same in the re-roll, but I can rename it if you
> > insist.
>
> I think it will confuse me in the future if I read `range_diff` and even
> the data type suggests that it could hold the output of a `git range-diff
> <options>` run.
>
> So I would like to insist.

In the new version[1], this variable is named 'rdiff_prev' (the
"previous" version against which the range-diff is to be generated).

> > > > +format_patch () {
> > > > +     title=$1 &&
> > > > +     range=$2 &&
> > > > +     test_expect_success "format-patch --range-diff ($title)" '
> > > > +             git format-patch --stdout --cover-letter --range-diff=$range \
> > > > +                     master..unmodified >actual &&
> > > > +             grep "= 1: .* s/5/A" actual &&
> > > > +             grep "= 2: .* s/4/A" actual &&
> > > > +             grep "= 3: .* s/11/B" actual &&
> > > > +             grep "= 4: .* s/12/B" actual
> > >
> > > I guess this might make sense if `format_patch` was not a function, but it
> > > is specifically marked as a function... so... shouldn't these `grep`s also
> > > be using function parameters?
> >
> > A later patch adds a second test which specifies the same ranges but
> > in a different way, so the result will be the same, hence the
> > hard-coded grep'ing. The function avoids repetition across the two
> > tests. I suppose I could do this a bit differently, though, to avoid
> > pretending it's a general-purpose function.
>
> If you can think of a way that would make this easier to read for, say,
> myself if I ever find myself debugging a regression caught by this test, I
> would appreciate that.

In the new version, the function is gone; it looks like this:

--- >8 ---
for prev in topic master..topic
do
    test_expect_success "format-patch --range-diff=$prev" '
        git format-patch --stdout --cover-letter --range-diff=$prev \
            master..unmodified >actual &&
        grep "= 1: .* s/5/A" actual &&
        grep "= 2: .* s/4/A" actual &&
        grep "= 3: .* s/11/B" actual &&
        grep "= 4: .* s/12/B" actual
    '
done
--- >8 ---

Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter

[Johannes Schindelin]

Hi Eric,

On Thu, 26 Jul 2018, Eric Sunshine wrote:

> On Thu, Jul 26, 2018 at 6:56 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > On Tue, 17 Jul 2018, Eric Sunshine wrote:
> > > On Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin
> > > <Johannes.Schindelin@gmx.de> wrote:
> > > > BTW I like to have an extra space in front of all the range-diff
> > > > lines, to make it easier to discern them from the rest.
> > >
> > > I'm not sure what you mean. Perhaps I'm misreading your comment.
> >
> > Sorry, I was really unclear.
> >
> > In the cover letters sent out by GitGitGadget (or earlier, my
> > mail-patch-series.sh command), I took pains to indent the entire
> > range-diff (or interdiff) with a single space. That is, the footer
> > "Range-diff vs v<n>:" is not indented at all, but all subsequent lines
> > of the range-diff have a leading space.
> >
> > The original reason was to stop confusing `git apply` when sending an
> > interdiff as part of a single patch without a cover letter (in which
> > case mail-patch-series.sh inserted the interdiff below the `---`
> > marker, and the interdiff would have looked like the start of the real
> > diff otherwise).
> 
> The new version[1] likewise indents the interdiff to avoid confusing
> git-am / git-apply.
> 
> [1]: https://public-inbox.org/git/20180722095717.17912-1-sunshine@sunshineco.com/

Great!

> > In the meantime, I got used to this indentation so much that I do not
> > want to miss it, it is a relatively easy and intuitive visual marker.
> >
> > This, however, will be harder to achieve now that you are using the
> > libified range-diff.
> 
> I toyed with indenting the range-diff in both the cover letter and
> below the "---" line in a patch. With the libified range-diff, doing
> so involves modifying the range-diff implementation (rather than
> having the consumer of the range-diff manage the indentation locally),
> so it adds a bit of complexity to show_range_diff(), though perhaps
> not too much.
> 
> However, I opted against it for a few reasons. First, "header" lines
> apart, all lines of the range-diff are already indented, and the
> existing indentation was sufficient (for me, at least) as a visual
> marker. Second, range-diffs tend to be _wide_, especially the header
> lines, and I was loath to make it wider by indenting more. Third, due
> to the existing indentation of the diff proper, a range-diff won't
> confuse git-am / git-apply, nor will the unindented header lines, so
> extra indentation seemed superfluous.

Totally understandable. For some reason, I never thought about that fact
(a range-diff is *not* a diff) when changing mail-patch-series.ts to use
range-diffs instead of interdiffs.

> > > > > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> > > > > +     const char *range_diff = NULL;
> > > >
> > > > Maybe `range_diff_opt`? It's not exactly the range diff that is
> > > > contained in this variable.
> > >
> > > I could, though I was trying to keep it shorter rather than longer.
> > > This is still the same in the re-roll, but I can rename it if you
> > > insist.
> >
> > I think it will confuse me in the future if I read `range_diff` and
> > even the data type suggests that it could hold the output of a `git
> > range-diff <options>` run.
> >
> > So I would like to insist.
> 
> In the new version[1], this variable is named 'rdiff_prev' (the
> "previous" version against which the range-diff is to be generated).

Thank you.

> > > > > +format_patch () {
> > > > > +     title=$1 &&
> > > > > +     range=$2 &&
> > > > > +     test_expect_success "format-patch --range-diff ($title)" '
> > > > > +             git format-patch --stdout --cover-letter --range-diff=$range \
> > > > > +                     master..unmodified >actual &&
> > > > > +             grep "= 1: .* s/5/A" actual &&
> > > > > +             grep "= 2: .* s/4/A" actual &&
> > > > > +             grep "= 3: .* s/11/B" actual &&
> > > > > +             grep "= 4: .* s/12/B" actual
> > > >
> > > > I guess this might make sense if `format_patch` was not a
> > > > function, but it is specifically marked as a function... so...
> > > > shouldn't these `grep`s also be using function parameters?
> > >
> > > A later patch adds a second test which specifies the same ranges but
> > > in a different way, so the result will be the same, hence the
> > > hard-coded grep'ing. The function avoids repetition across the two
> > > tests. I suppose I could do this a bit differently, though, to avoid
> > > pretending it's a general-purpose function.
> >
> > If you can think of a way that would make this easier to read for,
> > say, myself if I ever find myself debugging a regression caught by
> > this test, I would appreciate that.
> 
> In the new version, the function is gone; it looks like this:
> 
> --- >8 ---
> for prev in topic master..topic
> do
>     test_expect_success "format-patch --range-diff=$prev" '
>         git format-patch --stdout --cover-letter --range-diff=$prev \
>             master..unmodified >actual &&
>         grep "= 1: .* s/5/A" actual &&
>         grep "= 2: .* s/4/A" actual &&
>         grep "= 3: .* s/11/B" actual &&
>         grep "= 4: .* s/12/B" actual
>     '
> done
> --- >8 ---

Looks good.

Thank you so much!
Dscho