threads / patch / 54207

patch, 3 partsformat-patch: --interiff/--range-diff tweaks

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

## tl;dr

6 messages between Sep 8, 2020 and Sep 9, 2020. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Eric Sunshine· Sep 8, 2020, 07:16 UTC · lore

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
Eric Sunshine· Sep 8, 2020, 07:16 UTC · re: Eric Sunshine · lore

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

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
Show changes to 7 files +26 −39

Makefile, builtin/log.c, diff-lib.c, diff.h, interdiff.c, interdiff.h, log-tree.c

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
Eric Sunshine· Sep 8, 2020, 07:16 UTC · re: Eric Sunshine · lore

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

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(-)
Show changes to 4 files +14 −6

builtin/log.c, diff-lib.c, diff.h, log-tree.c

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
Eric Sunshine· Sep 8, 2020, 07:16 UTC · re: Eric Sunshine · lore

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

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(-)
Show changes to builtin/log.c +11 −7
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
Junio C Hamano· Sep 8, 2020, 22:10 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 5 quoted lines
> 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.
Eric Sunshine· Sep 9, 2020, 06:02 UTC · re: Junio C Hamano · lore

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

On Tue, Sep 8, 2020 at 6:11 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> 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.

← back to recent threads