[PATCH 0/3] format-patch: --interiff/--range-diff tweaks

[PATCH 0/3] format-patch: --interiff/--range-diff tweaks

[Eric Sunshine]

This series addresses a few comments[2,3,4,5] which cropped up during
review of the series which added --interdiff and --range-diff options to
git-format-patch[1]. That series made it into 'next' before I could address
the comments, so these patches (based upon 'master') make minor tweaks
"incrementally" (over 2 years late).

[1]: https://lore.kernel.org/git/20180722095717.17912-1-sunshine@sunshineco.com/
[2]: https://lore.kernel.org/git/CACsJy8C8RK6HkfoEYJGZg=sgtJS0WksHD3=7Souw3jYebRo=Sg@mail.gmail.com/
[3]: https://lore.kernel.org/git/xmqqh8kp4otz.fsf@gitster-ct.c.googlers.com/
[4]: https://lore.kernel.org/git/CAPig+cSuYUYSPTuKx08wcmQM-G12_-W2T4BS07fA=6grM1b8Gw@mail.gmail.com/
[5]: https://lore.kernel.org/git/xmqqva93t4u7.fsf@gitster-ct.c.googlers.com/

Eric Sunshine (3):
  diff: move show_interdiff() from its own file to diff-lib
  diff-lib: tighten show_interdiff()'s interface
  format-patch: use 'origin' as start of current-series-range when known

 Makefile      |  1 -
 builtin/log.c | 22 +++++++++++++---------
 diff-lib.c    | 25 +++++++++++++++++++++++++
 diff.h        |  7 +++++++
 interdiff.c   | 28 ----------------------------
 interdiff.h   |  8 --------
 log-tree.c    |  4 ++--
 7 files changed, 47 insertions(+), 48 deletions(-)
 delete mode 100644 interdiff.c
 delete mode 100644 interdiff.h

-- 
2.28.0.618.gf4bc123cb7

[PATCH 1/3] diff: move show_interdiff() from its own file to diff-lib

[Eric Sunshine]

show_interdiff() is a relatively small function and not likely to grow
larger or more complicated. Rather than dedicating an entire source file
to it, relocate it to diff-lib.c which houses other "take two things and
compare them" functions meant to be re-used but not so low-level as to
reside in the core diff implementation.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

Notes:
    Suggested by Duy[1] and seconded by Junio[2] during review.
    
    [1]: https://lore.kernel.org/git/CACsJy8C8RK6HkfoEYJGZg=sgtJS0WksHD3=7Souw3jYebRo=Sg@mail.gmail.com/
    [2]: https://lore.kernel.org/git/xmqqh8kp4otz.fsf@gitster-ct.c.googlers.com/

 Makefile      |  1 -
 builtin/log.c |  1 -
 diff-lib.c    | 24 ++++++++++++++++++++++++
 diff.h        |  2 ++
 interdiff.c   | 28 ----------------------------
 interdiff.h   |  8 --------
 log-tree.c    |  1 -
 7 files changed, 26 insertions(+), 39 deletions(-)
 delete mode 100644 interdiff.c
 delete mode 100644 interdiff.h

diff --git a/Makefile b/Makefile
index 86e5411f39..cf47141939 100644
--- a/Makefile
+++ b/Makefile
@@ -883,7 +883,6 @@ LIB_OBJS += hashmap.o
 LIB_OBJS += help.o
 LIB_OBJS += hex.o
 LIB_OBJS += ident.o
-LIB_OBJS += interdiff.o
 LIB_OBJS += json-writer.o
 LIB_OBJS += kwset.o
 LIB_OBJS += levenshtein.o
diff --git a/builtin/log.c b/builtin/log.c
index b58f8da09e..ae9380da1b 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -33,7 +33,6 @@
 #include "commit-slab.h"
 #include "repository.h"
 #include "commit-reach.h"
-#include "interdiff.h"
 #include "range-diff.h"
 
 #define MAIL_DEFAULT_WRAP 72
diff --git a/diff-lib.c b/diff-lib.c
index 50521e2093..9bab907412 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -571,3 +571,27 @@ int index_differs_from(struct repository *r,
 	object_array_clear(&rev.pending);
 	return (rev.diffopt.flags.has_changes != 0);
 }
+
+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/diff.h b/diff.h
index e0c0af6286..308937c94b 100644
--- a/diff.h
+++ b/diff.h
@@ -600,6 +600,8 @@ int index_differs_from(struct repository *r, const char *def,
 		       const struct diff_flags *flags,
 		       int ita_invisible_in_index);
 
+void show_interdiff(struct rev_info *, int indent);
+
 /*
  * Fill the contents of the filespec "df", respecting any textconv defined by
  * its userdiff driver.  The "driver" parameter must come from a
diff --git a/interdiff.c b/interdiff.c
deleted file mode 100644
index c81d680a6c..0000000000
--- a/interdiff.c
+++ /dev/null
@@ -1,28 +0,0 @@
-#include "cache.h"
-#include "commit.h"
-#include "revision.h"
-#include "interdiff.h"
-
-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
deleted file mode 100644
index 01c730a5c9..0000000000
--- a/interdiff.h
+++ /dev/null
@@ -1,8 +0,0 @@
-#ifndef INTERDIFF_H
-#define INTERDIFF_H
-
-struct rev_info;
-
-void show_interdiff(struct rev_info *, int indent);
-
-#endif
diff --git a/log-tree.c b/log-tree.c
index 55a68d0c61..39bb362d5e 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -15,7 +15,6 @@
 #include "sequencer.h"
 #include "line-log.h"
 #include "help.h"
-#include "interdiff.h"
 #include "range-diff.h"
 
 static struct decoration name_decoration = { "object names" };
-- 
2.28.0.618.gf4bc123cb7

[PATCH 2/3] diff-lib: tighten show_interdiff()'s interface

[Eric Sunshine]

To compute and show an interdiff, show_interdiff() needs only the two
OID's to compare and a diffopts, yet it expects callers to supply an
entire rev_info. The demand for rev_info is not only overkill, but also
places unnecessary burden on potential future callers which might not
otherwise have a rev_info at hand. Address this by tightening its
signature to require only the items it needs instead of a full rev_info.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

Notes:
    Suggested by Sunshine[1] during review.
    
    [1]: https://lore.kernel.org/git/CAPig+cSuYUYSPTuKx08wcmQM-G12_-W2T4BS07fA=6grM1b8Gw@mail.gmail.com/

 builtin/log.c | 3 ++-
 diff-lib.c    | 7 ++++---
 diff.h        | 7 ++++++-
 log-tree.c    | 3 ++-
 4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index ae9380da1b..37177b3e7f 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1206,7 +1206,8 @@ 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, 0);
+		show_interdiff(rev->idiff_oid1, rev->idiff_oid2, 0,
+			       &rev->diffopt);
 	}
 
 	if (rev->rdiff1) {
diff --git a/diff-lib.c b/diff-lib.c
index 9bab907412..a17becc509 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -577,19 +577,20 @@ static struct strbuf *idiff_prefix_cb(struct diff_options *opt, void *data)
 	return data;
 }
 
-void show_interdiff(struct rev_info *rev, int indent)
+void show_interdiff(const struct object_id *oid1, const struct object_id *oid2,
+		    int indent, struct diff_options *diffopt)
 {
 	struct diff_options opts;
 	struct strbuf prefix = STRBUF_INIT;
 
-	memcpy(&opts, &rev->diffopt, sizeof(opts));
+	memcpy(&opts, 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);
+	diff_tree_oid(oid1, oid2, "", &opts);
 	diffcore_std(&opts);
 	diff_flush(&opts);
 
diff --git a/diff.h b/diff.h
index 308937c94b..49242d2733 100644
--- a/diff.h
+++ b/diff.h
@@ -600,7 +600,12 @@ int index_differs_from(struct repository *r, const char *def,
 		       const struct diff_flags *flags,
 		       int ita_invisible_in_index);
 
-void show_interdiff(struct rev_info *, int indent);
+/*
+ * Emit an interdiff of two object ID's to 'diff_options.file' optionally
+ * indented by 'indent' spaces.
+ */
+void show_interdiff(const struct object_id *, const struct object_id *,
+		    int indent, struct diff_options *);
 
 /*
  * Fill the contents of the filespec "df", respecting any textconv defined by
diff --git a/log-tree.c b/log-tree.c
index 39bb362d5e..ad1e7e31f8 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -799,7 +799,8 @@ void show_log(struct rev_info *opt)
 
 		next_commentary_block(opt, NULL);
 		fprintf_ln(opt->diffopt.file, "%s", opt->idiff_title);
-		show_interdiff(opt, 2);
+		show_interdiff(opt->idiff_oid1, opt->idiff_oid2, 2,
+			       &opt->diffopt);
 
 		memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
 	}
-- 
2.28.0.618.gf4bc123cb7

[PATCH 3/3] format-patch: use 'origin' as start of current-series-range when known

[Eric Sunshine]

When formatting a patch series over `origin..HEAD`, one would expect
that range to be used as the current-series-range when computing a
range-diff between the previous and current versions of a patch series.
However, infer_range_diff_ranges() ignores `origin..HEAD` when
--range-diff=<prev> specifies a single revision rather than a range, and
instead unexpectedly computes the current-series-range based upon
<prev>. Address this anomaly by unconditionally using `origin..HEAD` as
the current-series-range regardless of <prev> as long as `origin` is
known, and only fall back to basing current-series-range on <prev> when
`origin` is not known.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

Notes:
    Suggested by Junio[1] during review.
    
    I had some difficulty composing the commit message and am not
    convinced that it does a good job of explaining the change or
    justifying it. If anyone can suggest improvements, I'd appreciate
    the help.
    
    I'm also not sure if this change deserves a test. If it does, it
    isn't clear to me exactly how to craft one. So, again, assistance
    would be appreciated.
    
    [1]: https://lore.kernel.org/git/xmqqva93t4u7.fsf@gitster-ct.c.googlers.com/

 builtin/log.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 37177b3e7f..f79b2b8775 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1595,16 +1595,20 @@ static void infer_range_diff_ranges(struct strbuf *r1,
 				    struct commit *head)
 {
 	const char *head_oid = oid_to_hex(&head->object.oid);
+	int prev_is_range = !!strstr(prev, "..");
 
-	if (!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(_("failed to infer range-diff origin of current series"));
+	else {
+		warning(_("using '%s' as range-diff origin of current series"), 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);
 	}
 }
 
-- 
2.28.0.618.gf4bc123cb7

Re: [PATCH 0/3] format-patch: --interiff/--range-diff tweaks

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> This series addresses a few comments[2,3,4,5] which cropped up during
> review of the series which added --interdiff and --range-diff options to
> git-format-patch[1]. That series made it into 'next' before I could address
> the comments, so these patches (based upon 'master') make minor tweaks
> "incrementally" (over 2 years late).

The last step subtly changes the behaviour, if I am reading its
description correctly.  Does it deserve a documentation update, or
are we just making the code behave "better" but still within the
boundary of how it is documented to work, hence no need to update
the doc (but deserves an advertisement in the release notes)?

Thanks.

Re: [PATCH 0/3] format-patch: --interiff/--range-diff tweaks

[Eric Sunshine]

On Tue, Sep 8, 2020 at 6:11 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > This series addresses a few comments[2,3,4,5] which cropped up during
> > review of the series which added --interdiff and --range-diff options to
> > git-format-patch[1]. That series made it into 'next' before I could address
> > the comments, so these patches (based upon 'master') make minor tweaks
> > "incrementally" (over 2 years late).
>
> The last step subtly changes the behaviour, if I am reading its
> description correctly.  Does it deserve a documentation update, or
> are we just making the code behave "better" but still within the
> boundary of how it is documented to work, hence no need to update
> the doc (but deserves an advertisement in the release notes)?

I honestly don't have an answer because I have trouble reasoning about
these cases (perhaps due to unclear mental model). That's part of the
reason why this patch series took so long. The first two patches were
ready in September 2018, but I kept putting off the third one because
I was having trouble understanding your suggested changes. It only
started to click the other day when I sat down and really studied your
proposal for a long time. Attempting to write a meaningful commit
message, rather than a hand-wavy one, forced me to think about it even
more critically, which helped (perhaps even more than the code itself)
to better understand it.

I wouldn't be opposed to dropping the last patch, however, with
whatever understanding I gained, I do agree that the way you suggested
coding it does make more sense and is more intuitive. But that doesn't
mean that it is sufficiently concrete in my brain to say whether a
documentation update is warranted and, if so, exactly how to
articulate such an update.