threads / patch / 48936

patch, 14 partsformat-patch: add --interdiff and --range-diff options

Subject: [PATCH 00/14] format-patch: add --interdiff and --range-diff options

## tl;dr

38 messages between Jul 22, 2018 and Sep 7, 2018. Diffs are folded; open one to read it.

replies: 37people: 4as markdown or json

Eric Sunshine· Jul 22, 2018, 09:57 UTC · lore

When re-submitting a patch series, it is often helpful (for reviewers) to include an interdiff or range-diff against the previous version. Doing so requires manually running git-diff or git-range-diff and copy/pasting the result into the cover letter of the new version.

This series automates the process by introducing git-format-patch options --interdiff and --range-diff which insert such a diff into the cover-letter or into the commentary section of the lone patch of a 1-patch series. In the latter case, the interdiff or range-diff is indented to avoid confusing git-am and human readers.

Patches 1-6 add --interdiff and can apply directly on 'master'. Patches 7-14 add --range-diff and apply atop js/range-diff v4[1].

An earlier RFC[2] implemented only --range-diff, and only for the cover-letter.

Changes since the RFC:
* add --interdiff option for cover-letter and lone patch
* based on js/range-diff v4[1]
* --range-diff works with lone patch (no longer limited to cover
  letter)
* --range-diff colors output when used with --stdout, just as patches
  themselves are already colored
* --range-diff takes advantage of libified range-diff mechanism in v4
  rather than invoking git-range-diff command

No interdiff or range-diff is included in this cover-letter since the implementation changed dramatically.

[1]: https://public-inbox.org/git/pull.1.v4.git.gitgitgadget@gmail.com/ [2]: https://public-inbox.org/git/20180530080325.37520-1-sunshine@sunshineco.com/

Eric Sunshine (14):
  format-patch: allow additional generated content in
    make_cover_letter()
  format-patch: add --interdiff option to embed diff in cover letter
  format-patch: teach --interdiff to respect -v/--reroll-count
  interdiff: teach show_interdiff() to indent interdiff
  log-tree: show_log: make commentary block delimiting reusable
  format-patch: allow --interdiff to apply to a lone-patch
  range-diff: respect diff_option.file rather than assuming 'stdout'
  range-diff: publish default creation factor
  range-diff: relieve callers of low-level configuration burden
  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-factor tweak for --range-diff
  format-patch: allow --range-diff to apply to a lone-patch
 Documentation/git-format-patch.txt |  29 ++++++
 Makefile                           |   1 +
 builtin/log.c                      | 139 ++++++++++++++++++++++++-----
 builtin/range-diff.c               |  25 ++----
 interdiff.c                        |  28 ++++++
 interdiff.h                        |   8 ++
 log-tree.c                         |  52 +++++++++--
 range-diff.c                       |  26 +++++-
 range-diff.h                       |   5 +-
 revision.h                         |  11 +++
 t/t3206-range-diff.sh              |  12 +++
 t/t4014-format-patch.sh            |  34 +++++++
 12 files changed, 319 insertions(+), 51 deletions(-)
 create mode 100644 interdiff.c
 create mode 100644 interdiff.h
-- 
2.18.0.345.g5c9ce644c3
Eric Sunshine· Jul 22, 2018, 09:57 UTC · re: Eric Sunshine · lore

[PATCH 01/14] format-patch: allow additional generated content in make_cover_letter()

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(-)
Show changes to builtin/log.c +23 −20
diff --git a/builtin/log.c b/builtin/log.c
index 805f89d7e1..873aabcf40 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -997,6 +997,26 @@ static char *find_branch_name(struct rev_info *rev)
 	return branch;
 }
 
+static void show_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(get_commit_tree_oid(origin),
+		      get_commit_tree_oid(head),
+		      "", &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,
@@ -1010,7 +1030,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];
@@ -1060,25 +1079,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(get_commit_tree_oid(origin),
-		      get_commit_tree_oid(head),
-		      "", &opts);
-	diffcore_std(&opts);
-	diff_flush(&opts);
-
-	fprintf(rev->diffopt.file, "\n");
+	/* We can only do diffstat with a unique reference point */
+	if (origin)
+		show_diffstat(rev, origin, head);
 }
 
 static const char *clean_message_id(const char *msg_id)
-- 
2.18.0.345.g5c9ce644c3
Eric Sunshine· Jul 22, 2018, 09:57 UTC · re: Eric Sunshine · lore

[PATCH 02/14] format-patch: add --interdiff option to embed diff in cover letter

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, however, doing so involves manually copy/pasting the diff into the cover letter.

Add an --interdiff option to automate this process. The argument to --interdiff specifies the tip of the previous attempt against which to generate the interdiff. For example:

    git format-patch --cover-letter --interdiff=v1 -3 v2

The previous attempt and the patch series being formatted must share a common base.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/git-format-patch.txt |  9 +++++++++
 Makefile                           |  1 +
 builtin/log.c                      | 24 ++++++++++++++++++++++--
 interdiff.c                        | 17 +++++++++++++++++
 interdiff.h                        |  8 ++++++++
 revision.h                         |  4 ++++
 t/t4014-format-patch.sh            | 17 +++++++++++++++++
 7 files changed, 78 insertions(+), 2 deletions(-)
 create mode 100644 interdiff.c
 create mode 100644 interdiff.h
Show changes to 7 files +78 −2

Documentation/git-format-patch.txt, Makefile, builtin/log.c, interdiff.c, interdiff.h, revision.h, t/t4014-format-patch.sh

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index b41e1329a7..a1b1bafee7 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>]]
+		   [--interdiff=<previous>]
 		   [--progress]
 		   [<common diff options>]
 		   [ <since> | <revision range> ]
@@ -228,6 +229,14 @@ 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.
 
+--interdiff=<previous>::
+	As a reviewer aid, insert an interdiff 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 --interdiff=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/Makefile b/Makefile
index 41b93689ad..2af389c0d9 100644
--- a/Makefile
+++ b/Makefile
@@ -872,6 +872,7 @@ LIB_OBJS += linear-assignment.o
 LIB_OBJS += help.o
 LIB_OBJS += hex.o
 LIB_OBJS += ident.o
+LIB_OBJS += interdiff.o
 LIB_OBJS += kwset.o
 LIB_OBJS += levenshtein.o
 LIB_OBJS += line-log.o
diff --git a/builtin/log.c b/builtin/log.c
index 873aabcf40..1020b78477 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -30,6 +30,7 @@
 #include "gpg-interface.h"
 #include "progress.h"
 #include "commit-slab.h"
+#include "interdiff.h"
 
 #define MAIL_DEFAULT_WRAP 72
 
@@ -1082,6 +1083,11 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	/* We can only do diffstat with a unique reference point */
 	if (origin)
 		show_diffstat(rev, origin, head);
+
+	if (rev->idiff_oid1) {
+		fprintf_ln(rev->diffopt.file, "%s", _("Interdiff:"));
+		show_interdiff(rev);
+	}
 }
 
 static const char *clean_message_id(const char *msg_id)
@@ -1448,6 +1454,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;
+	struct oid_array idiff_prev = OID_ARRAY_INIT;
 
 	const struct option builtin_format_patch_options[] = {
 		{ OPTION_CALLBACK, 'n', "numbered", &numbered, NULL,
@@ -1521,6 +1528,9 @@ 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_CALLBACK(0, "interdiff", &idiff_prev, N_("rev"),
+			     N_("show changes against <rev> in cover letter"),
+			     parse_opt_object_name),
 		OPT_END()
 	};
 
@@ -1706,7 +1716,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		if (rev.pending.nr == 2) {
 			struct object_array_entry *o = rev.pending.objects;
 			if (oidcmp(&o[0].item->oid, &o[1].item->oid) == 0)
-				return 0;
+				goto done;
 		}
 		get_patch_ids(&rev, &ids);
 	}
@@ -1730,7 +1740,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	}
 	if (nr == 0)
 		/* nothing to do */
-		return 0;
+		goto done;
 	total = nr;
 	if (cover_letter == -1) {
 		if (config_cover_letter == COVER_AUTO)
@@ -1743,6 +1753,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (numbered)
 		rev.total = total + start_number - 1;
 
+	if (idiff_prev.nr) {
+		if (!cover_letter)
+			die(_("--interdiff requires --cover-letter"));
+		rev.idiff_oid1 = &idiff_prev.oid[idiff_prev.nr - 1];
+		rev.idiff_oid2 = get_commit_tree_oid(list[0]);
+	}
+
 	if (!signature) {
 		; /* --no-signature inhibits all signatures */
 	} else if (signature && signature != git_version_string) {
@@ -1860,6 +1877,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	string_list_clear(&extra_hdr, 0);
 	if (ignore_if_in_upstream)
 		free_patch_ids(&ids);
+
+done:
+	oid_array_clear(&idiff_prev);
 	return 0;
 }
 
diff --git a/interdiff.c b/interdiff.c
new file mode 100644
index 0000000000..d0fac10c7c
--- /dev/null
+++ b/interdiff.c
@@ -0,0 +1,17 @@
+#include "cache.h"
+#include "commit.h"
+#include "revision.h"
+#include "interdiff.h"
+
+void show_interdiff(struct rev_info *rev)
+{
+	struct diff_options opts;
+
+	memcpy(&opts, &rev->diffopt, sizeof(opts));
+	opts.output_format = DIFF_FORMAT_PATCH;
+	diff_setup_done(&opts);
+
+	diff_tree_oid(rev->idiff_oid1, rev->idiff_oid2, "", &opts);
+	diffcore_std(&opts);
+	diff_flush(&opts);
+}
diff --git a/interdiff.h b/interdiff.h
new file mode 100644
index 0000000000..793c0144fe
--- /dev/null
+++ b/interdiff.h
@@ -0,0 +1,8 @@
+#ifndef INTERDIFF_H
+#define INTERDIFF_H
+
+struct rev_info;
+
+void show_interdiff(struct rev_info *);
+
+#endif
diff --git a/revision.h b/revision.h
index bf2239f876..61931fbac5 100644
--- a/revision.h
+++ b/revision.h
@@ -212,6 +212,10 @@ struct rev_info {
 	/* notes-specific options: which refs to show */
 	struct display_notes_opt notes_opt;
 
+	/* interdiff */
+	const struct object_id *idiff_oid1;
+	const struct object_id *idiff_oid2;
+
 	/* commit counts */
 	int count_left;
 	int count_right;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 53880da7bb..57b46322aa 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1717,4 +1717,21 @@ test_expect_success 'format-patch --pretty=mboxrd' '
 	test_cmp expect actual
 '
 
+test_expect_success 'interdiff: setup' '
+	git checkout -b boop master &&
+	test_commit fnorp blorp &&
+	test_commit fleep blorp
+'
+
+test_expect_success 'interdiff: cover-letter' '
+	sed "y/q/ /" >expect <<-\EOF &&
+	+fleep
+	--q
+	EOF
+	git format-patch --cover-letter --interdiff=boop~2 -1 boop &&
+	test_i18ngrep "^Interdiff:$" 0000-cover-letter.patch &&
+	sed "1,/^@@ /d; /^-- $/q" <0000-cover-letter.patch >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.18.0.345.g5c9ce644c3
Eric Sunshine· Jul 22, 2018, 10:31 UTC · re: Eric Sunshine · lore

Re: [PATCH 02/14] format-patch: add --interdiff option to embed diff in cover letter

On Sun, Jul 22, 2018 at 5:57 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 17 quoted lines
> Add an --interdiff option to automate this process. The argument to
> --interdiff specifies the tip of the previous attempt against which to
> generate the interdiff.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
> diff --git a/interdiff.c b/interdiff.c
> @@ -0,0 +1,17 @@
> +void show_interdiff(struct rev_info *rev)
> +{
> +       struct diff_options opts;
> +
> +       memcpy(&opts, &rev->diffopt, sizeof(opts));
> +       opts.output_format = DIFF_FORMAT_PATCH;
> +       diff_setup_done(&opts);
> +
> +       diff_tree_oid(rev->idiff_oid1, rev->idiff_oid2, "", &opts);

Passing a 'rev_info' to this function is overkill. It actually only needs the two OID's and a 'diff_options'. I'll make the change if I need to reroll for some other reason, otherwise I'll do it as a follow-up patch.

Duy Nguyen· Jul 23, 2018, 16:02 UTC · re: Eric Sunshine · lore

Re: [PATCH 02/14] format-patch: add --interdiff option to embed diff in cover letter

On Sun, Jul 22, 2018 at 11:57 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 23 quoted lines
> diff --git a/interdiff.c b/interdiff.c
> new file mode 100644
> index 0000000000..d0fac10c7c
> --- /dev/null
> +++ b/interdiff.c
> @@ -0,0 +1,17 @@
> +#include "cache.h"
> +#include "commit.h"
> +#include "revision.h"
> +#include "interdiff.h"
> +
> +void show_interdiff(struct rev_info *rev)
> +{
> +       struct diff_options opts;
> +
> +       memcpy(&opts, &rev->diffopt, sizeof(opts));
> +       opts.output_format = DIFF_FORMAT_PATCH;
> +       diff_setup_done(&opts);
> +
> +       diff_tree_oid(rev->idiff_oid1, rev->idiff_oid2, "", &opts);
> +       diffcore_std(&opts);
> +       diff_flush(&opts);
> +}

Is it worth adding a new file just for a single function? I haven't read the rest of the series, but the cover letter's diffstat suggests this is it. Is interdiff intended to become a lot more complicated in the future? If not maybe just add this function in diff-lib.c

-- 
Duy
Eric Sunshine· Jul 23, 2018, 19:18 UTC · re: Duy Nguyen · lore

Re: [PATCH 02/14] format-patch: add --interdiff option to embed diff in cover letter

On Mon, Jul 23, 2018 at 12:03 PM Duy Nguyen <pclouds@gmail.com> wrote:
Show 19 quoted lines
> On Sun, Jul 22, 2018 at 11:57 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > @@ -0,0 +1,17 @@
> > +void show_interdiff(struct rev_info *rev)
> > +{
> > +       struct diff_options opts;
> > +
> > +       memcpy(&opts, &rev->diffopt, sizeof(opts));
> > +       opts.output_format = DIFF_FORMAT_PATCH;
> > +       diff_setup_done(&opts);
> > +
> > +       diff_tree_oid(rev->idiff_oid1, rev->idiff_oid2, "", &opts);
> > +       diffcore_std(&opts);
> > +       diff_flush(&opts);
> > +}
>
> Is it worth adding a new file just for a single function? I haven't
> read the rest of the series, but the cover letter's diffstat suggests
> this is it. Is interdiff intended to become a lot more complicated in
> the future? If not maybe just add this function in diff-lib.c

Good question. The functionality originally lived in builtin/log.c but moved to log-tree.c when I added the ability to embed an interdiff in a single patch. However, it didn't "feel" right in log-tree.c, so I moved it to its own file to mirror how the range-diff engine resides in its own file.

And, the function actually did several more things as originally implemented. For instance, it took care of not clobbering global diff-queue state, and consulting 'reroll_count' and printing the "Interdiff:" header, but those bits eventually moved to live at more "correct" locations, leaving this relatively minimal function behind. It does get a bit more complex in a later patch, but not significantly so.

I wasn't aware of diff-lib.c, but it does seem like show_interdiff() could be at home there.

Junio C Hamano· Jul 23, 2018, 21:36 UTC · re: Eric Sunshine · lore

Re: [PATCH 02/14] format-patch: add --interdiff option to embed diff in cover letter

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 26 quoted lines
> On Mon, Jul 23, 2018 at 12:03 PM Duy Nguyen <pclouds@gmail.com> wrote:
>> On Sun, Jul 22, 2018 at 11:57 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>> > @@ -0,0 +1,17 @@
>> > +void show_interdiff(struct rev_info *rev)
>> > +{
>> > +       struct diff_options opts;
>> > +
>> > +       memcpy(&opts, &rev->diffopt, sizeof(opts));
>> > +       opts.output_format = DIFF_FORMAT_PATCH;
>> > +       diff_setup_done(&opts);
>> > +
>> > +       diff_tree_oid(rev->idiff_oid1, rev->idiff_oid2, "", &opts);
>> > +       diffcore_std(&opts);
>> > +       diff_flush(&opts);
>> > +}
>>
>> Is it worth adding a new file just for a single function? I haven't
>> read the rest of the series, but the cover letter's diffstat suggests
>> this is it. Is interdiff intended to become a lot more complicated in
>> the future? If not maybe just add this function in diff-lib.c
>
> Good question. The functionality originally lived in builtin/log.c but
> moved to log-tree.c when I added the ability to embed an interdiff in
> a single patch. However, it didn't "feel" right in log-tree.c, so I
> moved it to its own file to mirror how the range-diff engine resides
> in its own file.
Show 10 quoted lines
> And, the function actually did several more things as originally
> implemented. For instance, it took care of not clobbering global
> diff-queue state, and consulting 'reroll_count' and printing the
> "Interdiff:" header, but those bits eventually moved to live at more
> "correct" locations, leaving this relatively minimal function behind.
> It does get a bit more complex in a later patch, but not significantly
> so.
>
> I wasn't aware of diff-lib.c, but it does seem like show_interdiff()
> could be at home there.

Yeah, diff-lib.c is meant to be a home for the implementation of low level "diff-{files,tree,index}" plumbing that can be called from other routines; if you are adding "take two things, compare them" that can be reused from places, then it is a good match.

Eric Sunshine· Jul 22, 2018, 09:57 UTC · re: Eric Sunshine · lore

[PATCH 03/14] format-patch: teach --interdiff to respect -v/--reroll-count

The --interdiff option introduces the embedded interdiff generically as "Interdiff:", however, we can do better when --reroll-count is specified by emitting "Interdiff against v{n}:" instead.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/log.c           | 17 ++++++++++++++++-
 revision.h              |  1 +
 t/t4014-format-patch.sh |  5 +++++
 3 files changed, 22 insertions(+), 1 deletion(-)
Show changes to 3 files +22 −1

builtin/log.c, revision.h, t/t4014-format-patch.sh

diff --git a/builtin/log.c b/builtin/log.c
index 1020b78477..99ddfe8bb0 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1085,7 +1085,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 		show_diffstat(rev, origin, head);
 
 	if (rev->idiff_oid1) {
-		fprintf_ln(rev->diffopt.file, "%s", _("Interdiff:"));
+		fprintf_ln(rev->diffopt.file, "%s", rev->idiff_title);
 		show_interdiff(rev);
 	}
 }
@@ -1427,6 +1427,16 @@ static void print_bases(struct base_tree_info *bases, FILE *file)
 	oidclr(&bases->base_commit);
 }
 
+static const char *diff_title(struct strbuf *sb, int reroll_count,
+		       const char *generic, const char *rerolled)
+{
+	if (reroll_count <= 0)
+		strbuf_addstr(sb, generic);
+	else /* RFC may be v0, so allow -v1 to diff against v0 */
+		strbuf_addf(sb, rerolled, reroll_count - 1);
+	return sb->buf;
+}
+
 int cmd_format_patch(int argc, const char **argv, const char *prefix)
 {
 	struct commit *commit;
@@ -1455,6 +1465,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int show_progress = 0;
 	struct progress *progress = NULL;
 	struct oid_array idiff_prev = OID_ARRAY_INIT;
+	struct strbuf idiff_title = STRBUF_INIT;
 
 	const struct option builtin_format_patch_options[] = {
 		{ OPTION_CALLBACK, 'n', "numbered", &numbered, NULL,
@@ -1758,6 +1769,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			die(_("--interdiff requires --cover-letter"));
 		rev.idiff_oid1 = &idiff_prev.oid[idiff_prev.nr - 1];
 		rev.idiff_oid2 = get_commit_tree_oid(list[0]);
+		rev.idiff_title = diff_title(&idiff_title, reroll_count,
+					     _("Interdiff:"),
+					     _("Interdiff against v%d:"));
 	}
 
 	if (!signature) {
@@ -1880,6 +1894,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 
 done:
 	oid_array_clear(&idiff_prev);
+	strbuf_release(&idiff_title);
 	return 0;
 }
 
diff --git a/revision.h b/revision.h
index 61931fbac5..ffeadc261a 100644
--- a/revision.h
+++ b/revision.h
@@ -215,6 +215,7 @@ struct rev_info {
 	/* interdiff */
 	const struct object_id *idiff_oid1;
 	const struct object_id *idiff_oid2;
+	const char *idiff_title;
 
 	/* commit counts */
 	int count_left;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 57b46322aa..5950890d30 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1734,4 +1734,9 @@ test_expect_success 'interdiff: cover-letter' '
 	test_cmp expect actual
 '
 
+test_expect_success 'interdiff: reroll-count' '
+	git format-patch --cover-letter --interdiff=boop~2 -v2 -1 boop &&
+	test_i18ngrep "^Interdiff ..* v1:$" v2-0000-cover-letter.patch
+'
+
 test_done
-- 
2.18.0.345.g5c9ce644c3
Duy Nguyen· Jul 23, 2018, 16:12 UTC · re: Eric Sunshine · lore

Re: [PATCH 03/14] format-patch: teach --interdiff to respect -v/--reroll-count

On Sun, Jul 22, 2018 at 11:57 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> The --interdiff option introduces the embedded interdiff generically as
> "Interdiff:", however, we can do better when --reroll-count is specified

Oh boy. --reroll-count was added in 2012 and here I am typing --subject-prefix='PATCH vX' everyday, thinking that somebody should really do something about it. I've learned --reroll-count today!

Show 9 quoted lines
> diff --git a/revision.h b/revision.h
> index 61931fbac5..ffeadc261a 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -215,6 +215,7 @@ struct rev_info {
>         /* interdiff */
>         const struct object_id *idiff_oid1;
>         const struct object_id *idiff_oid2;
> +       const char *idiff_title;

I feel we're abusing struct rev_info a bit for this since this interdiff thing is very builtin/log.c's business and not at all related to rev walk. Is it possible (and easy) to just pass idfff_title from cmd_format_patch to make_cover_letter()? If it's a lot of code, then I guess we can just leave it here.

-- 
Duy
Eric Sunshine· Jul 23, 2018, 19:32 UTC · re: Duy Nguyen · lore

Re: [PATCH 03/14] format-patch: teach --interdiff to respect -v/--reroll-count

On Mon, Jul 23, 2018 at 12:12 PM Duy Nguyen <pclouds@gmail.com> wrote:
Show 12 quoted lines
> On Sun, Jul 22, 2018 at 11:57 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > @@ -215,6 +215,7 @@ struct rev_info {
> >         /* interdiff */
> >         const struct object_id *idiff_oid1;
> >         const struct object_id *idiff_oid2;
> > +       const char *idiff_title;
>
> I feel we're abusing struct rev_info a bit for this since this
> interdiff thing is very builtin/log.c's business and not at all
> related to rev walk. Is it possible (and easy) to just pass
> idfff_title from cmd_format_patch to make_cover_letter()? If it's a
> lot of code, then I guess we can just leave it here.

As originally implemented, this information was passed directly to make_cover_letter(), however, as you discovered in your review of patch 6/14[1], which makes it possible to embed an interdiff in the commentary section of a lone patch, 'struct rev_info' is the only way to pass this information down that deeply in the patch-generation process. So, yes, this pretty much needs to use 'struct rev_info', even if that need doesn't exist at this early step in the series.

[1]: https://public-inbox.org/git/CACsJy8Aw6R8-3kDfhCqunXziajCg9O_1WrEYc4rfKa+-=m1D5g@mail.gmail.com/
Eric Sunshine· Jul 22, 2018, 09:57 UTC · re: Eric Sunshine · lore

[PATCH 04/14] interdiff: teach show_interdiff() to indent interdiff

A future change will allow "git format-patch --interdiff=<prev> -1" to insert an interdiff into the commentary section of the lone patch of a 1-patch series. However, to prevent the inserted interdiff from confusing git-am, as well as human readers, it needs to be indented. Therefore, teach show_interdiff() how to indent.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/log.c |  2 +-
 interdiff.c   | 13 ++++++++++++-
 interdiff.h   |  2 +-
 3 files changed, 14 insertions(+), 3 deletions(-)
Show changes to 3 files +14 −3

builtin/log.c, interdiff.c, interdiff.h

diff --git a/builtin/log.c b/builtin/log.c
index 99ddfe8bb0..8078a43d14 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1086,7 +1086,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 
 	if (rev->idiff_oid1) {
 		fprintf_ln(rev->diffopt.file, "%s", rev->idiff_title);
-		show_interdiff(rev);
+		show_interdiff(rev, 0);
 	}
 }
 
diff --git a/interdiff.c b/interdiff.c
index d0fac10c7c..c81d680a6c 100644
--- a/interdiff.c
+++ b/interdiff.c
@@ -3,15 +3,26 @@
 #include "revision.h"
 #include "interdiff.h"
 
-void show_interdiff(struct rev_info *rev)
+static struct strbuf *idiff_prefix_cb(struct diff_options *opt, void *data)
+{
+	return data;
+}
+
+void show_interdiff(struct rev_info *rev, int indent)
 {
 	struct diff_options opts;
+	struct strbuf prefix = STRBUF_INIT;
 
 	memcpy(&opts, &rev->diffopt, sizeof(opts));
 	opts.output_format = DIFF_FORMAT_PATCH;
+	opts.output_prefix = idiff_prefix_cb;
+	strbuf_addchars(&prefix, ' ', indent);
+	opts.output_prefix_data = &prefix;
 	diff_setup_done(&opts);
 
 	diff_tree_oid(rev->idiff_oid1, rev->idiff_oid2, "", &opts);
 	diffcore_std(&opts);
 	diff_flush(&opts);
+
+	strbuf_release(&prefix);
 }
diff --git a/interdiff.h b/interdiff.h
index 793c0144fe..01c730a5c9 100644
--- a/interdiff.h
+++ b/interdiff.h
@@ -3,6 +3,6 @@
 
 struct rev_info;
 
-void show_interdiff(struct rev_info *);
+void show_interdiff(struct rev_info *, int indent);
 
 #endif
-- 
2.18.0.345.g5c9ce644c3
Eric Sunshine· Jul 22, 2018, 09:57 UTC · re: Eric Sunshine · lore

[PATCH 05/14] log-tree: show_log: make commentary block delimiting reusable

In patches generated by git-format-patch, the area below the "---" line following the commit message and before the actual 'diff' can be used for commentary which the patch author wants to convey to readers of the patch itself but not include in the commit message proper.

By default, the commentary area is empty, however, the --notes option causes it to be populated with notes associated with the commit. In the future, other options may be added which also insert content into the commentary section.

To accommodate this, factor out the logic which delimits commentary blocks from the commit message so that it can be re-used for upcoming optional inserted content.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 log-tree.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)
Show changes to log-tree.c +16 −7
diff --git a/log-tree.c b/log-tree.c
index 4a3907fea0..9d38f1cf79 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -541,6 +541,16 @@ static int show_mergetag(struct rev_info *opt, struct commit *commit)
 	return for_each_mergetag(show_one_mergetag, commit, opt);
 }
 
+static void next_commentary_block(struct rev_info *opt, struct strbuf *sb)
+{
+	const char *x = opt->shown_dashes ? "\n" : "---\n";
+	if (sb)
+		strbuf_addstr(sb, x);
+	else
+		fputs(x, opt->diffopt.file);
+	opt->shown_dashes = 1;
+}
+
 void show_log(struct rev_info *opt)
 {
 	struct strbuf msgbuf = STRBUF_INIT;
@@ -698,10 +708,8 @@ void show_log(struct rev_info *opt)
 
 	if ((ctx.fmt != CMIT_FMT_USERFORMAT) &&
 	    ctx.notes_message && *ctx.notes_message) {
-		if (cmit_fmt_is_mail(ctx.fmt)) {
-			strbuf_addstr(&msgbuf, "---\n");
-			opt->shown_dashes = 1;
-		}
+		if (cmit_fmt_is_mail(ctx.fmt))
+			next_commentary_block(opt, &msgbuf);
 		strbuf_addstr(&msgbuf, ctx.notes_message);
 	}
 
@@ -765,9 +773,10 @@ int log_tree_diff_flush(struct rev_info *opt)
 
 			/*
 			 * We may have shown three-dashes line early
-			 * between notes and the log message, in which
-			 * case we only want a blank line after the
-			 * notes without (an extra) three-dashes line.
+			 * between generated commentary (notes, etc.)
+			 * and the log message, in which case we only
+			 * want a blank line after the commentary
+			 * without (an extra) three-dashes line.
 			 * Otherwise, we show the three-dashes line if
 			 * we are showing the patch with diffstat, but
 			 * in that case, there is no extra blank line
-- 
2.18.0.345.g5c9ce644c3
Eric Sunshine· Jul 22, 2018, 09:57 UTC · re: Eric Sunshine · lore

[PATCH 06/14] format-patch: allow --interdiff to apply to a lone-patch

When submitting a revised version of a patch or series, it can be helpful (to reviewers) to include a summary of changes since the previous attempt in the form of an interdiff, typically in the cover letter. However, it is occasionally useful, despite making for a noisy read, to insert an interdiff into the commentary section of the lone patch of a 1-patch series.

Therefore, extend "git format-patch --interdiff=<prev>" to insert an interdiff into the commentary section of a lone patch rather than requiring a cover letter. The interdiff is indented to avoid confusing git-am and human readers into considering it part of the patch proper.

Implementation note: Generating an interdiff for insertion into the commentary section of a patch which itself is currently being generated requires invoking the diffing machinery recursively. However, the machinery does not (presently) support this since it uses global state. Consequently, we need to take care to stash away the state of the in-progress operation while generating the interdiff, and restore it after.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/git-format-patch.txt |  3 ++-
 builtin/log.c                      |  8 +++++---
 log-tree.c                         | 14 ++++++++++++++
 t/t4014-format-patch.sh            | 12 ++++++++++++
 4 files changed, 33 insertions(+), 4 deletions(-)
Show changes to 4 files +33 −4

Documentation/git-format-patch.txt, builtin/log.c, log-tree.c, t/t4014-format-patch.sh

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index a1b1bafee7..f8a061794d 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -230,7 +230,8 @@ feeding the result to `git send-email`.
 	fill in a description in the file before sending it out.
 
 --interdiff=<previous>::
-	As a reviewer aid, insert an interdiff into the cover letter showing
+	As a reviewer aid, insert an interdiff into the cover letter,
+	or as commentary of the lone patch of a 1-patch series, 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
diff --git a/builtin/log.c b/builtin/log.c
index 8078a43d14..e990027c28 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1540,7 +1540,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "progress", &show_progress,
 			 N_("show progress while generating patches")),
 		OPT_CALLBACK(0, "interdiff", &idiff_prev, N_("rev"),
-			     N_("show changes against <rev> in cover letter"),
+			     N_("show changes against <rev> in cover letter or single patch"),
 			     parse_opt_object_name),
 		OPT_END()
 	};
@@ -1765,8 +1765,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		rev.total = total + start_number - 1;
 
 	if (idiff_prev.nr) {
-		if (!cover_letter)
-			die(_("--interdiff requires --cover-letter"));
+		if (!cover_letter && total != 1)
+			die(_("--interdiff requires --cover-letter or single patch"));
 		rev.idiff_oid1 = &idiff_prev.oid[idiff_prev.nr - 1];
 		rev.idiff_oid2 = get_commit_tree_oid(list[0]);
 		rev.idiff_title = diff_title(&idiff_title, reroll_count,
@@ -1811,6 +1811,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		print_signature(rev.diffopt.file);
 		total++;
 		start_number--;
+		/* interdiff in cover-letter; omit from patches */
+		rev.idiff_oid1 = NULL;
 	}
 	rev.add_signoff = do_signoff;
 
diff --git a/log-tree.c b/log-tree.c
index 9d38f1cf79..56513fa83d 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -14,6 +14,7 @@
 #include "sequencer.h"
 #include "line-log.h"
 #include "help.h"
+#include "interdiff.h"
 
 static struct decoration name_decoration = { "object names" };
 static int decoration_loaded;
@@ -736,6 +737,19 @@ void show_log(struct rev_info *opt)
 
 	strbuf_release(&msgbuf);
 	free(ctx.notes_message);
+
+	if (cmit_fmt_is_mail(ctx.fmt) && opt->idiff_oid1) {
+		struct diff_queue_struct dq;
+
+		memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));
+		DIFF_QUEUE_CLEAR(&diff_queued_diff);
+
+		next_commentary_block(opt, NULL);
+		fprintf_ln(opt->diffopt.file, "%s", opt->idiff_title);
+		show_interdiff(opt, 2);
+
+		memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
+	}
 }
 
 int log_tree_diff_flush(struct rev_info *opt)
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 5950890d30..909c743c13 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1730,6 +1730,7 @@ test_expect_success 'interdiff: cover-letter' '
 	EOF
 	git format-patch --cover-letter --interdiff=boop~2 -1 boop &&
 	test_i18ngrep "^Interdiff:$" 0000-cover-letter.patch &&
+	test_i18ngrep ! "^Interdiff:$" 0001-fleep.patch &&
 	sed "1,/^@@ /d; /^-- $/q" <0000-cover-letter.patch >actual &&
 	test_cmp expect actual
 '
@@ -1739,4 +1740,15 @@ test_expect_success 'interdiff: reroll-count' '
 	test_i18ngrep "^Interdiff ..* v1:$" v2-0000-cover-letter.patch
 '
 
+test_expect_success 'interdiff: solo-patch' '
+	cat >expect <<-\EOF &&
+	  +fleep
+
+	EOF
+	git format-patch --interdiff=boop~2 -1 boop &&
+	test_i18ngrep "^Interdiff:$" 0001-fleep.patch &&
+	sed "1,/^  @@ /d; /^$/q" <0001-fleep.patch >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.18.0.345.g5c9ce644c3
Duy Nguyen· Jul 23, 2018, 16:22 UTC · re: Eric Sunshine · lore

Re: [PATCH 06/14] format-patch: allow --interdiff to apply to a lone-patch

On Sun, Jul 22, 2018 at 11:58 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 18 quoted lines
> diff --git a/log-tree.c b/log-tree.c
> index 9d38f1cf79..56513fa83d 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -14,6 +14,7 @@
>  #include "sequencer.h"
>  #include "line-log.h"
>  #include "help.h"
> +#include "interdiff.h"
>
>  static struct decoration name_decoration = { "object names" };
>  static int decoration_loaded;
> @@ -736,6 +737,19 @@ void show_log(struct rev_info *opt)
>
>         strbuf_release(&msgbuf);
>         free(ctx.notes_message);
> +
> +       if (cmit_fmt_is_mail(ctx.fmt) && opt->idiff_oid1) {

OK putting idiff stuff in rev_info is probably the right choice. But we all three fields prefixed with idiff_, maybe you could just add a new struct "idiff_options" to contain them and add a pointer to that struct in rev_info. Then "opt->idiff" is enough to know if idiff is requested instead of relying on idiff_oid1 (seems too random).

Show 12 quoted lines
> +               struct diff_queue_struct dq;
> +
> +               memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));
> +               DIFF_QUEUE_CLEAR(&diff_queued_diff);
> +
> +               next_commentary_block(opt, NULL);
> +               fprintf_ln(opt->diffopt.file, "%s", opt->idiff_title);
> +               show_interdiff(opt, 2);
> +
> +               memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
> +       }
>  }
-- 
Duy
Eric Sunshine· Jul 23, 2018, 19:46 UTC · re: Duy Nguyen · lore

Re: [PATCH 06/14] format-patch: allow --interdiff to apply to a lone-patch

On Mon, Jul 23, 2018 at 12:22 PM Duy Nguyen <pclouds@gmail.com> wrote:
Show 8 quoted lines
> On Sun, Jul 22, 2018 at 11:58 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > +       if (cmit_fmt_is_mail(ctx.fmt) && opt->idiff_oid1) {
>
> OK putting idiff stuff in rev_info is probably the right choice. But
> we all three fields prefixed with idiff_, maybe you could just add a
> new struct "idiff_options" to contain them and add a pointer to that
> struct in rev_info. Then "opt->idiff" is enough to know if idiff is
> requested instead of relying on idiff_oid1 (seems too random).
I have mixed feelings about this suggestion for the following reasons:
* 'struct rev_info' already contains a number of specialized fields
which apply in only certain use cases but not others, and those fields
often are grouped textually to show relationship rather than being
bundled in a struct.
* These new fields are very specific to this particular operation and
are unlikely to ever have wider use, so adding the extra level of
indirection/abstraction (whatever you'd call it) feels overkill and,
while nice theoretically, adds complexity without much practical
value.
* Bundling these fields in a struct might incorrectly imply to readers
that these items, collectively, can be used in some general-purpose
fashion, which isn't at all the case; they are very specific to this
operation and that struct would never be used elsewhere or for any
other purpose.

The upside of bundling them in a struct, as you mention, is that "opt->idiff" would be slightly more obvious than "opt->idiff_oid1" as a "should we print an interdiff?" conditional. On the other hand, this case is so specific and narrow that I'm not sure it warrants such generality.

Eric Sunshine· Jul 22, 2018, 09:57 UTC · re: Eric Sunshine · lore

[PATCH 08/14] range-diff: publish default creation factor

The range-diff back-end allows its heuristic to be tweaked via the "creation factor". git-range-diff, the only client of the back-end, defaults the factor to 60% (hard-coded in builtin/range-diff.c), but allows the user to override it with the --creation-factor option.

Publish the default range factor to allow new callers of the range-diff back-end to default to the same value without duplicating the hard-coded constant, and to avoid worrying about various callers becoming out-of-sync if the default ever needs to change.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/range-diff.c | 2 +-
 range-diff.h         | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)
Show changes to 2 files +3 −1

builtin/range-diff.c, range-diff.h

diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index 77ac3bff7b..bbe6b05ae9 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -18,7 +18,7 @@ static struct strbuf *output_prefix_cb(struct diff_options *opt, void *data)
 
 int cmd_range_diff(int argc, const char **argv, const char *prefix)
 {
-	int creation_factor = 60;
+	int creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
 	struct diff_options diffopt = { NULL };
 	int simple_color = -1;
 	struct option options[] = {
diff --git a/range-diff.h b/range-diff.h
index aea9d43f34..73d6e232fe 100644
--- a/range-diff.h
+++ b/range-diff.h
@@ -3,6 +3,8 @@
 
 #include "diff.h"
 
+#define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60
+
 int show_range_diff(const char *range1, const char *range2,
 		    int creation_factor, struct diff_options *diffopt);
 
-- 
2.18.0.345.g5c9ce644c3
Eric Sunshine· Jul 22, 2018, 09:57 UTC · re: Eric Sunshine · lore

[PATCH 09/14] range-diff: relieve callers of low-level configuration burden

There are a number of very low-level configuration details which need to be managed precisely to generate a proper range-diff. In particular, 'diff_options' output format, header suppression, indentation, and dual-color mode must all be set appropriately to ensure proper behavior.

Handle these details locally in the libified range-diff back-end rather than forcing each caller to have specialized knowledge of these implementation details, and to avoid duplication as new callers are added.

While at it, localize these tweaks to be active only while generating the range-diff, so they don't clobber the caller-provided 'diff_options', which might be used beyond range-diff generation.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/range-diff.c | 23 ++++-------------------
 range-diff.c         | 24 ++++++++++++++++++++++--
 range-diff.h         |  3 ++-
 3 files changed, 28 insertions(+), 22 deletions(-)
Show changes to 3 files +28 −22

builtin/range-diff.c, range-diff.c, range-diff.h

diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index bbe6b05ae9..91463a1df6 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -11,11 +11,6 @@ N_("git range-diff [<options>] <base> <old-tip> <new-tip>"),
 NULL
 };
 
-static struct strbuf *output_prefix_cb(struct diff_options *opt, void *data)
-{
-	return data;
-}
-
 int cmd_range_diff(int argc, const char **argv, const char *prefix)
 {
 	int creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
@@ -29,17 +24,11 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 	int i, j, res = 0;
-	struct strbuf four_spaces = STRBUF_INIT;
 	struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
 
 	git_config(git_diff_ui_config, NULL);
 
 	diff_setup(&diffopt);
-	diffopt.output_format = DIFF_FORMAT_PATCH;
-	diffopt.flags.suppress_diff_headers = 1;
-	diffopt.output_prefix = output_prefix_cb;
-	strbuf_addstr(&four_spaces, "    ");
-	diffopt.output_prefix_data = &four_spaces;
 
 	argc = parse_options(argc, argv, NULL, options,
 			     builtin_range_diff_usage, PARSE_OPT_KEEP_UNKNOWN);
@@ -55,12 +44,9 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 	argc = j;
 	diff_setup_done(&diffopt);
 
-	if (simple_color < 1) {
-		if (!simple_color)
-			/* force color when --dual-color was used */
-			diffopt.use_color = 1;
-		diffopt.flags.dual_color_diffed_diffs = 1;
-	}
+	/* force color when --dual-color was used */
+	if (!simple_color)
+		diffopt.use_color = 1;
 
 	if (argc == 2) {
 		if (!strstr(argv[0], ".."))
@@ -96,11 +82,10 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 	}
 
 	res = show_range_diff(range1.buf, range2.buf, creation_factor,
-			      &diffopt);
+			      simple_color < 1, &diffopt);
 
 	strbuf_release(&range1);
 	strbuf_release(&range2);
-	strbuf_release(&four_spaces);
 
 	return res;
 }
diff --git a/range-diff.c b/range-diff.c
index 76e053c2b2..091fc344cd 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -414,8 +414,14 @@ static void output(struct string_list *a, struct string_list *b,
 	strbuf_release(&dashes);
 }
 
+static struct strbuf *output_prefix_cb(struct diff_options *opt, void *data)
+{
+	return data;
+}
+
 int show_range_diff(const char *range1, const char *range2,
-		    int creation_factor, struct diff_options *diffopt)
+		    int creation_factor, int dual_color,
+		    struct diff_options *diffopt)
 {
 	int res = 0;
 
@@ -428,9 +434,23 @@ int show_range_diff(const char *range1, const char *range2,
 		res = error(_("could not parse log for '%s'"), range2);
 
 	if (!res) {
+		struct diff_options opts;
+		struct strbuf indent = STRBUF_INIT;
+
+		memcpy(&opts, diffopt, sizeof(opts));
+		opts.output_format = DIFF_FORMAT_PATCH;
+		opts.flags.suppress_diff_headers = 1;
+		opts.flags.dual_color_diffed_diffs = dual_color;
+		opts.output_prefix = output_prefix_cb;
+		strbuf_addstr(&indent, "    ");
+		opts.output_prefix_data = &indent;
+		diff_setup_done(&opts);
+
 		find_exact_matches(&branch1, &branch2);
 		get_correspondences(&branch1, &branch2, creation_factor);
-		output(&branch1, &branch2, diffopt);
+		output(&branch1, &branch2, &opts);
+
+		strbuf_release(&indent);
 	}
 
 	string_list_clear(&branch1, 1);
diff --git a/range-diff.h b/range-diff.h
index 73d6e232fe..bf265594f3 100644
--- a/range-diff.h
+++ b/range-diff.h
@@ -6,6 +6,7 @@
 #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60
 
 int show_range_diff(const char *range1, const char *range2,
-		    int creation_factor, struct diff_options *diffopt);
+		    int creation_factor, int dual_color,
+		    struct diff_options *diffopt);
 
 #endif
-- 
2.18.0.345.g5c9ce644c3
Eric Sunshine· Jul 22, 2018, 09:57 UTC · re: Eric Sunshine · lore

[PATCH 10/14] format-patch: add --range-diff option to embed diff in cover letter

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 a range-diff, however, doing so involves manually copy/pasting the diff into the cover letter.

Add a --range-diff option to automate this process. 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 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                      | 35 ++++++++++++++++++++++++++++++
 revision.h                         |  5 +++++
 t/t3206-range-diff.sh              | 12 ++++++++++
 4 files changed, 62 insertions(+)
Show changes to 4 files +62 −0

Documentation/git-format-patch.txt, builtin/log.c, revision.h, t/t3206-range-diff.sh

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index f8a061794d..e7f404be3d 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -24,6 +24,7 @@ SYNOPSIS
 		   [--to=<email>] [--cc=<email>]
 		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
 		   [--interdiff=<previous>]
+		   [--range-diff=<previous>]
 		   [--progress]
 		   [<common diff options>]
 		   [ <since> | <revision range> ]
@@ -238,6 +239,15 @@ feeding the result to `git send-email`.
 	the series being formatted (for example `git format-patch
 	--cover-letter --interdiff=feature/v1 -3 feature/v2`).
 
+--range-diff=<previous>::
+	As a reviewer aid, insert a range-diff (see linkgit:git-range-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 e990027c28..d6e57e8b04 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -31,6 +31,7 @@
 #include "progress.h"
 #include "commit-slab.h"
 #include "interdiff.h"
+#include "range-diff.h"
 
 #define MAIL_DEFAULT_WRAP 72
 
@@ -1088,6 +1089,12 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 		fprintf_ln(rev->diffopt.file, "%s", rev->idiff_title);
 		show_interdiff(rev, 0);
 	}
+
+	if (rev->rdiff1) {
+		fprintf_ln(rev->diffopt.file, "%s", _("Range-diff:"));
+		show_range_diff(rev->rdiff1, rev->rdiff2,
+				rev->creation_factor, 1, &rev->diffopt);
+	}
 }
 
 static const char *clean_message_id(const char *msg_id)
@@ -1437,6 +1444,17 @@ static const char *diff_title(struct strbuf *sb, int reroll_count,
 	return sb->buf;
 }
 
+static void infer_range_diff_ranges(struct strbuf *r1,
+				    struct strbuf *r2,
+				    const char *prev,
+				    struct commit *head)
+{
+	const char *head_oid = oid_to_hex(&head->object.oid);
+
+	strbuf_addf(r1, "%s..%s", head_oid, prev);
+	strbuf_addf(r2, "%s..%s", prev, head_oid);
+}
+
 int cmd_format_patch(int argc, const char **argv, const char *prefix)
 {
 	struct commit *commit;
@@ -1466,6 +1484,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	struct progress *progress = NULL;
 	struct oid_array idiff_prev = OID_ARRAY_INIT;
 	struct strbuf idiff_title = STRBUF_INIT;
+	const char *rdiff_prev = NULL;
+	struct strbuf rdiff1 = STRBUF_INIT;
+	struct strbuf rdiff2 = STRBUF_INIT;
 
 	const struct option builtin_format_patch_options[] = {
 		{ OPTION_CALLBACK, 'n', "numbered", &numbered, NULL,
@@ -1542,6 +1563,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK(0, "interdiff", &idiff_prev, N_("rev"),
 			     N_("show changes against <rev> in cover letter or single patch"),
 			     parse_opt_object_name),
+		OPT_STRING(0, "range-diff", &rdiff_prev, N_("refspec"),
+			   N_("show changes against <refspec> in cover letter")),
 		OPT_END()
 	};
 
@@ -1774,6 +1797,16 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 					     _("Interdiff against v%d:"));
 	}
 
+	if (rdiff_prev) {
+		if (!cover_letter)
+			die(_("--range-diff requires --cover-letter"));
+
+		infer_range_diff_ranges(&rdiff1, &rdiff2, rdiff_prev, list[0]);
+		rev.rdiff1 = rdiff1.buf;
+		rev.rdiff2 = rdiff2.buf;
+		rev.creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
+	}
+
 	if (!signature) {
 		; /* --no-signature inhibits all signatures */
 	} else if (signature && signature != git_version_string) {
@@ -1897,6 +1930,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 done:
 	oid_array_clear(&idiff_prev);
 	strbuf_release(&idiff_title);
+	strbuf_release(&rdiff1);
+	strbuf_release(&rdiff2);
 	return 0;
 }
 
diff --git a/revision.h b/revision.h
index ffeadc261a..11159416dc 100644
--- a/revision.h
+++ b/revision.h
@@ -217,6 +217,11 @@ struct rev_info {
 	const struct object_id *idiff_oid2;
 	const char *idiff_title;
 
+	/* range-diff */
+	const char *rdiff1;
+	const char *rdiff2;
+	int creation_factor;
+
 	/* commit counts */
 	int count_left;
 	int count_right;
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 2237c7f4af..dd854b6ebc 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -142,4 +142,16 @@ test_expect_success 'changed message' '
 	test_cmp expected actual
 '
 
+for prev in 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
+
 test_done
-- 
2.18.0.345.g5c9ce644c3
Duy Nguyen· Jul 23, 2018, 16:28 UTC · re: Eric Sunshine · lore

Re: [PATCH 10/14] format-patch: add --range-diff option to embed diff in cover letter

On Sun, Jul 22, 2018 at 11:58 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 9 quoted lines
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index f8a061794d..e7f404be3d 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -24,6 +24,7 @@ SYNOPSIS
>                    [--to=<email>] [--cc=<email>]
>                    [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
>                    [--interdiff=<previous>]
> +                  [--range-diff=<previous>]

I wonder if people will use both --interdiff=<rev> and --range-diff=<rev> often enough to justify a shortcut "--all-kinds-of-diff=<rev>" so that we don't have to type <previous> twice. But I guess we don't have to worry about this right now.

-- 
Duy
Eric Sunshine· Jul 23, 2018, 19:58 UTC · re: Duy Nguyen · lore

Re: [PATCH 10/14] format-patch: add --range-diff option to embed diff in cover letter

On Mon, Jul 23, 2018 at 12:28 PM Duy Nguyen <pclouds@gmail.com> wrote:
Show 15 quoted lines
> On Sun, Jul 22, 2018 at 11:58 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> > index f8a061794d..e7f404be3d 100644
> > --- a/Documentation/git-format-patch.txt
> > +++ b/Documentation/git-format-patch.txt
> > @@ -24,6 +24,7 @@ SYNOPSIS
> >                    [--to=<email>] [--cc=<email>]
> >                    [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
> >                    [--interdiff=<previous>]
> > +                  [--range-diff=<previous>]
>
> I wonder if people will use both --interdiff=<rev> and
> --range-diff=<rev> often enough to justify a shortcut
> "--all-kinds-of-diff=<rev>" so that we don't have to type <previous>
> twice. But I guess we don't have to worry about this right now.

My original thought was that --interdiff and --range-diff would be mutually exclusive, however, I quickly realized that some people might like to use both options together since each format has its strengths and weaknesses. (I've used both types of diffs together when preparing rerolls of my own series and found that, together, they provided a better picture of the reroll than either would have alone.)

Based upon experience on this mailing list, I'd guess that most people would use only one or the other, though that doesn't speak for other projects. And, as you note, it's something that can be added later if warranted (plus, this series is already long and I'd like to avoid making it longer for something like this whose value is only speculative).

Junio C Hamano· Jul 25, 2018, 17:29 UTC · re: Eric Sunshine · lore

Re: [PATCH 10/14] format-patch: add --range-diff option to embed diff in cover letter

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 30 quoted lines
> On Mon, Jul 23, 2018 at 12:28 PM Duy Nguyen <pclouds@gmail.com> wrote:
>> On Sun, Jul 22, 2018 at 11:58 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>> > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
>> > index f8a061794d..e7f404be3d 100644
>> > --- a/Documentation/git-format-patch.txt
>> > +++ b/Documentation/git-format-patch.txt
>> > @@ -24,6 +24,7 @@ SYNOPSIS
>> >                    [--to=<email>] [--cc=<email>]
>> >                    [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
>> >                    [--interdiff=<previous>]
>> > +                  [--range-diff=<previous>]
>>
>> I wonder if people will use both --interdiff=<rev> and
>> --range-diff=<rev> often enough to justify a shortcut
>> "--all-kinds-of-diff=<rev>" so that we don't have to type <previous>
>> twice. But I guess we don't have to worry about this right now.
>
> My original thought was that --interdiff and --range-diff would be
> mutually exclusive, however, I quickly realized that some people might
> like to use both options together since each format has its strengths
> and weaknesses. (I've used both types of diffs together when preparing
> rerolls of my own series and found that, together, they provided a
> better picture of the reroll than either would have alone.)
>
> Based upon experience on this mailing list, I'd guess that most people
> would use only one or the other, though that doesn't speak for other
> projects. And, as you note, it's something that can be added later if
> warranted (plus, this series is already long and I'd like to avoid
> making it longer for something like this whose value is only
> speculative).
A few random thoughts.
 * I find it somewhat disturbing that use of dash is inconsistent
   between "--interdiff=<arg>" and "--range-diff=<arg>".
 * Perhaps "--interdiff=<previous> --range-diff" may be a possible
   way to say "use the same <previous> and show both"?  Do we want
   "--range-diff=<previous> --interdiff" to mean the same two
   meta-diff but shown in different order?
 * Do we expect that we may find a third-kind of "meta-diff" that
   sits next to interdiff and range-diff in the future?  I *think*
   two separate options "--interdiff=..." and "--range-diff=..." is
   still a good way forward, and we'd add "--frotzdiff=..." when
   such a third-kind of "meta-diff" turns out to be useful, without
   fearing proliferation of options, and that would be OK, but I am
   just thinking aloud to see if other people have better ideas.
Eric Sunshine· Jul 25, 2018, 19:30 UTC · re: Junio C Hamano · lore

Re: [PATCH 10/14] format-patch: add --range-diff option to embed diff in cover letter

On Wed, Jul 25, 2018 at 1:29 PM Junio C Hamano <gitster@pobox.com> wrote:
> A few random thoughts.
>
>  * I find it somewhat disturbing that use of dash is inconsistent
>    between "--interdiff=<arg>" and "--range-diff=<arg>".

I went with the common spellings we've seen on the mailing lists. "Interdiff", in particular, seems well established. "Range-diff" is new, so we don't have much data other than what we saw during the discussion when renaming git-branch-diff, and, of course, git-branch-diff itself is hyphenated. Another consideration: "inter" is a prefix, whereas "range" stands on its own.

I don't have a super strong opinion, as I'm used to both the hyphenated name (from discussion and the command name itself), and the unhyphenated name ("rangediff" was my local branch name). I'm open to opinions.

We probably wouldn't want to do so, but another possibility is to recognize both --range-diff and --rangediff.

>  * Perhaps "--interdiff=<previous> --range-diff" may be a possible
>    way to say "use the same <previous> and show both"?  Do we want
>    "--range-diff=<previous> --interdiff" to mean the same two
>    meta-diff but shown in different order?

That's not at all a bad shorthand. I like it better than the "--all-kinds-of-diff=<rev>" mentioned earlier. We don't need to make a decision for this series since such functionality can be added later.

The order of specification of the two options affecting the order of output is also an interesting idea. We may want to decide that before graduating this topic.

Show 7 quoted lines
>  * Do we expect that we may find a third-kind of "meta-diff" that
>    sits next to interdiff and range-diff in the future?  I *think*
>    two separate options "--interdiff=..." and "--range-diff=..." is
>    still a good way forward, and we'd add "--frotzdiff=..." when
>    such a third-kind of "meta-diff" turns out to be useful, without
>    fearing proliferation of options, and that would be OK, but I am
>    just thinking aloud to see if other people have better ideas.

I did consider other approaches, such as a more generic option. For example, --embed=range-diff:<prev>. I also considered supporting more complex use-cases. For instance, git-range-diff itself accepts the two ranges in several formats ("a..b c..d" or "x...z" or "i j k"), plus the creation-factor can be tweaked, so I weighed ways of incorporating all that detail into the single argument to --range-diff.

However, those are all difficult to use (onerous to type and remember) and difficult to document. Thus, I opted for simplicity of individual, straight-forward options (--interdiff, --range-diff, --creation-factor), which are easy to type, remember, and document.

We may want to revisit this later if git-format-patch grows additional similar options.

Junio C Hamano· Jul 25, 2018, 20:32 UTC · re: Eric Sunshine · lore

Re: [PATCH 10/14] format-patch: add --range-diff option to embed diff in cover letter

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 11 quoted lines
> I did consider other approaches, such as a more generic option. For
> example, --embed=range-diff:<prev>. I also considered supporting more
> complex use-cases. For instance, git-range-diff itself accepts the two
> ranges in several formats ("a..b c..d" or "x...z" or "i j k"), plus
> the creation-factor can be tweaked, so I weighed ways of incorporating
> all that detail into the single argument to --range-diff.
>
> However, those are all difficult to use (onerous to type and remember)
> and difficult to document. Thus, I opted for simplicity of individual,
> straight-forward options (--interdiff, --range-diff,
> --creation-factor), which are easy to type, remember, and document.

Yes, that matches my conclusion that it is OK to have these on separate options that can grow as many meta-diff formats we would support.

Duy Nguyen· Jul 25, 2018, 17:38 UTC · re: Eric Sunshine · lore

Re: [PATCH 10/14] format-patch: add --range-diff option to embed diff in cover letter

On Mon, Jul 23, 2018 at 9:59 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 24 quoted lines
>
> On Mon, Jul 23, 2018 at 12:28 PM Duy Nguyen <pclouds@gmail.com> wrote:
> > On Sun, Jul 22, 2018 at 11:58 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> > > index f8a061794d..e7f404be3d 100644
> > > --- a/Documentation/git-format-patch.txt
> > > +++ b/Documentation/git-format-patch.txt
> > > @@ -24,6 +24,7 @@ SYNOPSIS
> > >                    [--to=<email>] [--cc=<email>]
> > >                    [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
> > >                    [--interdiff=<previous>]
> > > +                  [--range-diff=<previous>]
> >
> > I wonder if people will use both --interdiff=<rev> and
> > --range-diff=<rev> often enough to justify a shortcut
> > "--all-kinds-of-diff=<rev>" so that we don't have to type <previous>
> > twice. But I guess we don't have to worry about this right now.
>
> My original thought was that --interdiff and --range-diff would be
> mutually exclusive, however, I quickly realized that some people might
> like to use both options together since each format has its strengths
> and weaknesses. (I've used both types of diffs together when preparing
> rerolls of my own series and found that, together, they provided a
> better picture of the reroll than either would have alone.)

I actually had another question that I answered myself: how do I know which one to choose? There's no preview option (and I'm lazy, I don't want to do separate diff commands myself). So my answer was "choose both, then delete the one that does not look good (and explain it in the cover too when I delete it)"

> And, as you note, it's something that can be added later if
> warranted (plus, this series is already long and I'd like to avoid
> making it longer for something like this whose value is only
> speculative).
Yes of course. We can revisit this later.
-- 
Duy
Eric Sunshine· Jul 22, 2018, 09:57 UTC · re: Eric Sunshine · lore

[PATCH 07/14] range-diff: respect diff_option.file rather than assuming 'stdout'

The actual diffs output by range-diff respect diff_option.file, which range-diff passes down the call-chain, thus are destination-agnostic. However, output_pair_header() is hard-coded to emit to 'stdout'. Fix this by making output_pair_header() respect diff_option.file, as well.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to range-diff.c +1 −1
diff --git a/range-diff.c b/range-diff.c
index 347b4a79f2..76e053c2b2 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -328,7 +328,7 @@ static void output_pair_header(struct diff_options *diffopt,
 	}
 	strbuf_addf(buf, "%s\n", color_reset);
 
-	fwrite(buf->buf, buf->len, 1, stdout);
+	fwrite(buf->buf, buf->len, 1, diffopt->file);
 }
 
 static struct userdiff_driver no_func_name = {
-- 
2.18.0.345.g5c9ce644c3
Stefan Beller· Jul 23, 2018, 22:59 UTC · re: Eric Sunshine · lore

Re: [PATCH 07/14] range-diff: respect diff_option.file rather than assuming 'stdout'

On Sun, Jul 22, 2018 at 2:58 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 7 quoted lines
>
> The actual diffs output by range-diff respect diff_option.file, which
> range-diff passes down the call-chain, thus are destination-agnostic.
> However, output_pair_header() is hard-coded to emit to 'stdout'. Fix
> this by making output_pair_header() respect diff_option.file, as well.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>

Depending how much the range diff series has progressed already, it might make sense to squash it there?

Thanks, Stefan

Show 20 quoted lines
> ---
>  range-diff.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/range-diff.c b/range-diff.c
> index 347b4a79f2..76e053c2b2 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -328,7 +328,7 @@ static void output_pair_header(struct diff_options *diffopt,
>         }
>         strbuf_addf(buf, "%s\n", color_reset);
>
> -       fwrite(buf->buf, buf->len, 1, stdout);
> +       fwrite(buf->buf, buf->len, 1, diffopt->file);
>  }
>
>  static struct userdiff_driver no_func_name = {
> --
> 2.18.0.345.g5c9ce644c3
>
Eric Sunshine· Jul 23, 2018, 23:20 UTC · re: Stefan Beller · lore

Re: [PATCH 07/14] range-diff: respect diff_option.file rather than assuming 'stdout'

On Mon, Jul 23, 2018 at 6:59 PM Stefan Beller <sbeller@google.com> wrote:
Show 10 quoted lines
> On Sun, Jul 22, 2018 at 2:58 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > The actual diffs output by range-diff respect diff_option.file, which
> > range-diff passes down the call-chain, thus are destination-agnostic.
> > However, output_pair_header() is hard-coded to emit to 'stdout'. Fix
> > this by making output_pair_header() respect diff_option.file, as well.
> >
> > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>
> Depending how much the range diff series has progressed
> already, it might make sense to squash it there?

It could be done that way, though it would be nice for Dscho's range-diff series to land without another re-roll, and it looks like I'll probably be re-rolling this series, anyhow, to move show_interdiff() to diff-lib.c and to fix its signature. Junio could manage it as a "squash", but that's probably more work for him than for me to retain it in this series. *And*, this change _is_ required for this series, whereas it doesn't really matter for Dscho's series, even though it's an obvious "fix".

Anyhow, whichever way Junio and Dscho would like to play it is fine with me.
Junio C Hamano· Jul 25, 2018, 18:25 UTC · re: Eric Sunshine · lore

Re: [PATCH 07/14] range-diff: respect diff_option.file rather than assuming 'stdout'

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 22 quoted lines
> On Mon, Jul 23, 2018 at 6:59 PM Stefan Beller <sbeller@google.com> wrote:
>> On Sun, Jul 22, 2018 at 2:58 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>> > The actual diffs output by range-diff respect diff_option.file, which
>> > range-diff passes down the call-chain, thus are destination-agnostic.
>> > However, output_pair_header() is hard-coded to emit to 'stdout'. Fix
>> > this by making output_pair_header() respect diff_option.file, as well.
>> >
>> > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
>>
>> Depending how much the range diff series has progressed
>> already, it might make sense to squash it there?
>
> It could be done that way, though it would be nice for Dscho's
> range-diff series to land without another re-roll, and it looks like
> I'll probably be re-rolling this series, anyhow, to move
> show_interdiff() to diff-lib.c and to fix its signature. Junio could
> manage it as a "squash", but that's probably more work for him than
> for me to retain it in this series. *And*, this change _is_ required
> for this series, whereas it doesn't really matter for Dscho's series,
> even though it's an obvious "fix".
>
> Anyhow, whichever way Junio and Dscho would like to play it is fine with me.

Having it at the beginning of your series (I consider 1-6/14 to be a separate series, on top of which another series whose first change is this 7/14 is builds upon) like you do here is probably fine.

Eric Sunshine· Jul 22, 2018, 09:57 UTC · re: Eric Sunshine · lore

[PATCH 12/14] format-patch: teach --range-diff to respect -v/--reroll-count

The --range-diff option announces the embedded range-diff generically as "Range-diff:", however, we can do better when --reroll-count is specified by emitting "Range-diff against v{n}:" instead.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/log.c | 7 ++++++-
 revision.h    | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)
Show changes to 2 files +7 −1

builtin/log.c, revision.h

diff --git a/builtin/log.c b/builtin/log.c
index 4f937aad15..fdb2180d7e 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1091,7 +1091,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	}
 
 	if (rev->rdiff1) {
-		fprintf_ln(rev->diffopt.file, "%s", _("Range-diff:"));
+		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
 		show_range_diff(rev->rdiff1, rev->rdiff2,
 				rev->creation_factor, 1, &rev->diffopt);
 	}
@@ -1496,6 +1496,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	const char *rdiff_prev = NULL;
 	struct strbuf rdiff1 = STRBUF_INIT;
 	struct strbuf rdiff2 = STRBUF_INIT;
+	struct strbuf rdiff_title = STRBUF_INIT;
 
 	const struct option builtin_format_patch_options[] = {
 		{ OPTION_CALLBACK, 'n', "numbered", &numbered, NULL,
@@ -1815,6 +1816,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		rev.rdiff1 = rdiff1.buf;
 		rev.rdiff2 = rdiff2.buf;
 		rev.creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
+		rev.rdiff_title = diff_title(&rdiff_title, reroll_count,
+					     _("Range-diff:"),
+					     _("Range-diff against v%d:"));
 	}
 
 	if (!signature) {
@@ -1942,6 +1946,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	strbuf_release(&idiff_title);
 	strbuf_release(&rdiff1);
 	strbuf_release(&rdiff2);
+	strbuf_release(&rdiff_title);
 	return 0;
 }
 
diff --git a/revision.h b/revision.h
index 11159416dc..cd0873b575 100644
--- a/revision.h
+++ b/revision.h
@@ -221,6 +221,7 @@ struct rev_info {
 	const char *rdiff1;
 	const char *rdiff2;
 	int creation_factor;
+	const char *rdiff_title;
 
 	/* commit counts */
 	int count_left;
-- 
2.18.0.345.g5c9ce644c3
Eric Sunshine· Jul 22, 2018, 09:57 UTC · re: Eric Sunshine · lore

[PATCH 14/14] format-patch: allow --range-diff to apply to a lone-patch

When submitting a revised version of a patch or series, it can be helpful (to reviewers) to include a summary of changes since the previous attempt in the form of a range-diff, typically in the cover letter. However, it is occasionally useful, despite making for a noisy read, to insert a range-diff into the commentary section of the lone patch of a 1-patch series.

Therefore, extend "git format-patch --range-diff=<refspec>" to insert a range-diff into the commentary section of a lone patch rather than requiring a cover letter.

Implementation note: Generating a range-diff for insertion into the commentary section of a patch which itself is currently being generated requires invoking the diffing machinery recursively. However, the machinery does not (presently) support this since it uses global state. Consequently, we need to take care to stash away the state of the in-progress operation while generating the range-diff, and restore it after.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/git-format-patch.txt |  3 ++-
 builtin/log.c                      |  9 +++++----
 log-tree.c                         | 15 +++++++++++++++
 3 files changed, 22 insertions(+), 5 deletions(-)
Show changes to 3 files +22 −5

Documentation/git-format-patch.txt, builtin/log.c, log-tree.c

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 9b2e172159..aba4c5febe 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -241,7 +241,8 @@ feeding the result to `git send-email`.
 
 --range-diff=<previous>::
 	As a reviewer aid, insert a range-diff (see linkgit:git-range-diff[1])
-	into the cover letter showing the differences between the previous
+	into the cover letter, or as commentary of the lone patch of a
+	1-patch series, showing the differences between the previous
 	version of the patch series and the series currently being formatted.
 	`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
diff --git a/builtin/log.c b/builtin/log.c
index 10c3801ceb..f0e99256a0 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1575,7 +1575,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			     N_("show changes against <rev> in cover letter or single patch"),
 			     parse_opt_object_name),
 		OPT_STRING(0, "range-diff", &rdiff_prev, N_("refspec"),
-			   N_("show changes against <refspec> in cover letter")),
+			   N_("show changes against <refspec> in cover letter or single patch")),
 		OPT_INTEGER(0, "creation-factor", &creation_factor,
 			    N_("percentage by which creation is weighted")),
 		OPT_END()
@@ -1816,8 +1816,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		die(_("--creation-factor requires --range-diff"));
 
 	if (rdiff_prev) {
-		if (!cover_letter)
-			die(_("--range-diff requires --cover-letter"));
+		if (!cover_letter && total != 1)
+			die(_("--range-diff requires --cover-letter or single patch"));
 
 		infer_range_diff_ranges(&rdiff1, &rdiff2, rdiff_prev,
 					origin, list[0]);
@@ -1866,8 +1866,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		print_signature(rev.diffopt.file);
 		total++;
 		start_number--;
-		/* interdiff in cover-letter; omit from patches */
+		/* interdiff/range-diff in cover-letter; omit from patches */
 		rev.idiff_oid1 = NULL;
+		rev.rdiff1 = NULL;
 	}
 	rev.add_signoff = do_signoff;
 
diff --git a/log-tree.c b/log-tree.c
index 56513fa83d..37afc585dc 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -15,6 +15,7 @@
 #include "line-log.h"
 #include "help.h"
 #include "interdiff.h"
+#include "range-diff.h"
 
 static struct decoration name_decoration = { "object names" };
 static int decoration_loaded;
@@ -750,6 +751,20 @@ void show_log(struct rev_info *opt)
 
 		memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
 	}
+
+	if (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) {
+		struct diff_queue_struct dq;
+
+		memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));
+		DIFF_QUEUE_CLEAR(&diff_queued_diff);
+
+		next_commentary_block(opt, NULL);
+		fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
+		show_range_diff(opt->rdiff1, opt->rdiff2,
+				opt->creation_factor, 1, &opt->diffopt);
+
+		memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
+	}
 }
 
 int log_tree_diff_flush(struct rev_info *opt)
-- 
2.18.0.345.g5c9ce644c3
Junio C Hamano· Jul 25, 2018, 21:07 UTC · re: Eric Sunshine · lore

Re: [PATCH 14/14] format-patch: allow --range-diff to apply to a lone-patch

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 21 quoted lines
> @@ -750,6 +751,20 @@ void show_log(struct rev_info *opt)
>  
>  		memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
>  	}
> +
> +	if (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) {
> +		struct diff_queue_struct dq;
> +
> +		memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));
> +		DIFF_QUEUE_CLEAR(&diff_queued_diff);
> +
> +		next_commentary_block(opt, NULL);
> +		fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
> +		show_range_diff(opt->rdiff1, opt->rdiff2,
> +				opt->creation_factor, 1, &opt->diffopt);
> +
> +		memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
> +	}
>  }
>  
>  int log_tree_diff_flush(struct rev_info *opt)
This essentially repeats what is already done for "interdiff".

Does the global diff_queued_diff gets cleaned up when show_interdiff() and show_range_diff() return, like diff_flush() does? Otherwise we'd be leaking the filepairs accumulated in the diff_queued_diff.

Eric Sunshine· Sep 7, 2018, 08:46 UTC · re: Junio C Hamano · lore

Re: [PATCH 14/14] format-patch: allow --range-diff to apply to a lone-patch

On Wed, Jul 25, 2018 at 5:07 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > +     if (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) {
> > +             struct diff_queue_struct dq;
> > +
> > +             memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));
> > +             DIFF_QUEUE_CLEAR(&diff_queued_diff);
> > +
> > +             next_commentary_block(opt, NULL);
> > +             fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
> > +             show_range_diff(opt->rdiff1, opt->rdiff2,
> > +                             opt->creation_factor, 1, &opt->diffopt);
> > +
> > +             memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
> > +     }
> >  }
>
> This essentially repeats what is already done for "interdiff".

Yes, the two blocks are very similar, although they access different members of 'rev_info' and call different functions to perform the actual diff. I explored ways of avoiding the repeated boilerplate (using macros or passing a function pointer to a driver function containing the boilerplate), but the end result was uglier and harder to understand due to the added abstraction. Introducing next_commentary_block()[1] reduced the repetition a bit.

> Does the global diff_queued_diff gets cleaned up when
> show_interdiff() and show_range_diff() return, like diff_flush()
> does?  Otherwise we'd be leaking the filepairs accumulated in the
> diff_queued_diff.

Both show_interdiff() and show_range_diff() call diff_flush(). So, the "temporary" diff_queued_diff set up here does get cleaned up by diff_flush(), as far as I understand (though this is my first foray into the diff-generation code, so I may be missing something). And, the diff_queued_diff which gets "interrupted" by this excursion into interdiff/range-diff gets cleaned up normally when the interrupted diff operation completes.

[1]: https://public-inbox.org/git/20180722095717.17912-6-sunshine@sunshineco.com/
Eric Sunshine· Jul 22, 2018, 09:57 UTC · re: Eric Sunshine · lore

[PATCH 13/14] format-patch: add --creation-factor tweak for --range-diff

When generating a range-diff, matching up commits between two version of a patch series involves heuristics, thus may give unexpected results. git-range-diff allows tweaking the heuristic via --creation-factor. Follow suit by accepting --creation-factor 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                      | 10 +++++++++-
 2 files changed, 16 insertions(+), 2 deletions(-)
Show changes to 2 files +16 −2

Documentation/git-format-patch.txt, builtin/log.c

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 425145f536..9b2e172159 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -24,7 +24,7 @@ SYNOPSIS
 		   [--to=<email>] [--cc=<email>]
 		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
 		   [--interdiff=<previous>]
-		   [--range-diff=<previous>]
+		   [--range-diff=<previous> [--creation-factor=<percent>]]
 		   [--progress]
 		   [<common diff options>]
 		   [ <since> | <revision range> ]
@@ -250,6 +250,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-factor=<percent>::
+	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-range-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 fdb2180d7e..10c3801ceb 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1497,6 +1497,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	struct strbuf rdiff1 = STRBUF_INIT;
 	struct strbuf rdiff2 = STRBUF_INIT;
 	struct strbuf rdiff_title = STRBUF_INIT;
+	int creation_factor = -1;
 
 	const struct option builtin_format_patch_options[] = {
 		{ OPTION_CALLBACK, 'n', "numbered", &numbered, NULL,
@@ -1575,6 +1576,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			     parse_opt_object_name),
 		OPT_STRING(0, "range-diff", &rdiff_prev, N_("refspec"),
 			   N_("show changes against <refspec> in cover letter")),
+		OPT_INTEGER(0, "creation-factor", &creation_factor,
+			    N_("percentage by which creation is weighted")),
 		OPT_END()
 	};
 
@@ -1807,6 +1810,11 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 					     _("Interdiff against v%d:"));
 	}
 
+	if (creation_factor < 0)
+		creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
+	else if (!rdiff_prev)
+		die(_("--creation-factor requires --range-diff"));
+
 	if (rdiff_prev) {
 		if (!cover_letter)
 			die(_("--range-diff requires --cover-letter"));
@@ -1815,7 +1823,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 					origin, list[0]);
 		rev.rdiff1 = rdiff1.buf;
 		rev.rdiff2 = rdiff2.buf;
-		rev.creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
+		rev.creation_factor = creation_factor;
 		rev.rdiff_title = diff_title(&rdiff_title, reroll_count,
 					     _("Range-diff:"),
 					     _("Range-diff against v%d:"));
-- 
2.18.0.345.g5c9ce644c3
Eric Sunshine· Jul 22, 2018, 09:57 UTC · re: Eric Sunshine · lore

[PATCH 11/14] format-patch: extend --range-diff to accept revision range

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/t3206-range-diff.sh              |  2 +-
 3 files changed, 19 insertions(+), 7 deletions(-)
Show changes to 3 files +19 −7

Documentation/git-format-patch.txt, builtin/log.c, t/t3206-range-diff.sh

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index e7f404be3d..425145f536 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -243,10 +243,12 @@ feeding the result to `git send-email`.
 	As a reviewer aid, insert a range-diff (see linkgit:git-range-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 d6e57e8b04..4f937aad15 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1447,12 +1447,21 @@ static const char *diff_title(struct strbuf *sb, int reroll_count,
 static void infer_range_diff_ranges(struct strbuf *r1,
 				    struct strbuf *r2,
 				    const char *prev,
+				    struct commit *origin,
 				    struct commit *head)
 {
 	const char *head_oid = oid_to_hex(&head->object.oid);
 
-	strbuf_addf(r1, "%s..%s", head_oid, prev);
-	strbuf_addf(r2, "%s..%s", prev, head_oid);
+	if (!strstr(prev, "..")) {
+		strbuf_addf(r1, "%s..%s", head_oid, prev);
+		strbuf_addf(r2, "%s..%s", prev, head_oid);
+	} else if (!origin) {
+		die(_("failed to infer range-diff ranges"));
+	} else {
+		strbuf_addstr(r1, prev);
+		strbuf_addf(r2, "%s..%s",
+			    oid_to_hex(&origin->object.oid), head_oid);
+	}
 }
 
 int cmd_format_patch(int argc, const char **argv, const char *prefix)
@@ -1801,7 +1810,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		if (!cover_letter)
 			die(_("--range-diff requires --cover-letter"));
 
-		infer_range_diff_ranges(&rdiff1, &rdiff2, rdiff_prev, list[0]);
+		infer_range_diff_ranges(&rdiff1, &rdiff2, rdiff_prev,
+					origin, list[0]);
 		rev.rdiff1 = rdiff1.buf;
 		rev.rdiff2 = rdiff2.buf;
 		rev.creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index dd854b6ebc..3d7a2d8a4d 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -142,7 +142,7 @@ test_expect_success 'changed message' '
 	test_cmp expected actual
 '
 
-for prev in topic
+for prev in topic master..topic
 do
 	test_expect_success "format-patch --range-diff=$prev" '
 		git format-patch --stdout --cover-letter --range-diff=$prev \
-- 
2.18.0.345.g5c9ce644c3
Junio C Hamano· Jul 25, 2018, 20:53 UTC · re: Eric Sunshine · lore

Re: [PATCH 11/14] format-patch: extend --range-diff to accept revision range

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 15 quoted lines
> 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>
> ---

Makes sense. Even though a single "common rev" would serve as a feature to discourage rebasing done "just to catch up" without a good reason, it is a good idea to give an escape hatch like this to support a case where rebasing is the right thing to do.

Show 10 quoted lines
>  static void infer_range_diff_ranges(struct strbuf *r1,
>  				    struct strbuf *r2,
>  				    const char *prev,
> +				    struct commit *origin,
>  				    struct commit *head)
>  {
>  	const char *head_oid = oid_to_hex(&head->object.oid);
>  
> -	strbuf_addf(r1, "%s..%s", head_oid, prev);
> -	strbuf_addf(r2, "%s..%s", prev, head_oid);

I thought "git range-diff" also took the three-dot notation as a short-hand but there is no point using that in this application, which wants the RHS and the LHS range in separate output variables.

Show 9 quoted lines
> +	if (!strstr(prev, "..")) {
> +		strbuf_addf(r1, "%s..%s", head_oid, prev);
> +		strbuf_addf(r2, "%s..%s", prev, head_oid);
> +	} else if (!origin) {
> +		die(_("failed to infer range-diff ranges"));
> +	} else {
> +		strbuf_addstr(r1, prev);
> +		strbuf_addf(r2, "%s..%s",
> +			    oid_to_hex(&origin->object.oid), head_oid);
Interesting.

I actually would feel better to see the second range for the normal case to be computed exactly the same way, i.e.

	int prev_is_range = strstr(prev, "..");
	if (prev_is_range)
		strbuf_addstr(r1, prev);
	else
		strbuf_addf(r1, "%s..%s", head_oid, prev);
	if (origin)
		strbuf_addf(r2, "%s..%s", oid_to_hex(&origin->object.oid, head_oid);
	else if (prev_is_range)
		die("cannot figure out the origin of new series");
	else {
		warning("falling back to use '%s' as the origin of new series",	prev);
		strbuf_addf(r2, "%s..%s", prev, head_oid);
	}

because origin..HEAD is always the set of the "new" series, no matter what "prev" the user chooses to compare that series against, when there is a single "origin".

Eric Sunshine· Sep 7, 2018, 09:15 UTC · re: Junio C Hamano · lore

Re: [PATCH 11/14] format-patch: extend --range-diff to accept revision range

On Wed, Jul 25, 2018 at 4:53 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > 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 [...]
> >
> > 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. [...]
>
> >  static void infer_range_diff_ranges(struct strbuf *r1,
> >  {
> > -     strbuf_addf(r1, "%s..%s", head_oid, prev);
> > -     strbuf_addf(r2, "%s..%s", prev, head_oid);
>
> I thought "git range-diff" also took the three-dot notation as a
> short-hand but there is no point using that in this application,
> which wants the RHS and the LHS range in separate output variables.
Correct.
Show 22 quoted lines
> I actually would feel better to see the second range for the normal
> case to be computed exactly the same way, i.e.
>
>         int prev_is_range = strstr(prev, "..");
>
>         if (prev_is_range)
>                 strbuf_addstr(r1, prev);
>         else
>                 strbuf_addf(r1, "%s..%s", head_oid, prev);
>
>         if (origin)
>                 strbuf_addf(r2, "%s..%s", oid_to_hex(&origin->object.oid, head_oid);
>         else if (prev_is_range)
>                 die("cannot figure out the origin of new series");
>         else {
>                 warning("falling back to use '%s' as the origin of new series", prev);
>                 strbuf_addf(r2, "%s..%s", prev, head_oid);
>         }
>
> because origin..HEAD is always the set of the "new" series, no
> matter what "prev" the user chooses to compare that series against,
> when there is a single "origin".

I plan to submit a short series as incremental changes atop es/format-patch-{interdiff,rangediff} to address the few review comments[1] (including my own[2]), so I can make this change, as well.

[1]: https://public-inbox.org/git/CAPig+cSuYUYSPTuKx08wcmQM-G12_-W2T4BS07fA=6grM1b8Gw@mail.gmail.com/T/#rfaf5a563e81b862bd8ed69232040056ab9a86dd8 [2]: https://public-inbox.org/git/CAPig+cSuYUYSPTuKx08wcmQM-G12_-W2T4BS07fA=6grM1b8Gw@mail.gmail.com/

Duy Nguyen· Jul 23, 2018, 16:32 UTC · re: Eric Sunshine · lore

Re: [PATCH 00/14] format-patch: add --interdiff and --range-diff options

On Sun, Jul 22, 2018 at 11:57 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 11 quoted lines
>
> When re-submitting a patch series, it is often helpful (for reviewers)
> to include an interdiff or range-diff against the previous version.
> Doing so requires manually running git-diff or git-range-diff and
> copy/pasting the result into the cover letter of the new version.
>
> This series automates the process by introducing git-format-patch
> options --interdiff and --range-diff which insert such a diff into the
> cover-letter or into the commentary section of the lone patch of a
> 1-patch series. In the latter case, the interdiff or range-diff is
> indented to avoid confusing git-am and human readers.

I gave up after 10/14. But what I've seen is nice (yes I have a couple comments here and there but you probably won't need to update anything).

-- 
Duy
Eric Sunshine· Jul 23, 2018, 20:03 UTC · re: Duy Nguyen · lore

Re: [PATCH 00/14] format-patch: add --interdiff and --range-diff options

On Mon, Jul 23, 2018 at 12:32 PM Duy Nguyen <pclouds@gmail.com> wrote:
Show 15 quoted lines
> On Sun, Jul 22, 2018 at 11:57 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > When re-submitting a patch series, it is often helpful (for reviewers)
> > to include an interdiff or range-diff against the previous version.
> > Doing so requires manually running git-diff or git-range-diff and
> > copy/pasting the result into the cover letter of the new version.
> >
> > This series automates the process by introducing git-format-patch
> > options --interdiff and --range-diff which insert such a diff into the
> > cover-letter or into the commentary section of the lone patch of a
> > 1-patch series. In the latter case, the interdiff or range-diff is
> > indented to avoid confusing git-am and human readers.
>
> I gave up after 10/14. But what I've seen is nice (yes I have a couple
> comments here and there but you probably won't need to update
> anything).
Thanks for the review comments.

← back to recent threads