git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] builtin/diff: learn --merge-base
@ 2020-09-05 19:08 Denton Liu
  2020-09-05 19:08 ` [PATCH 1/4] t4068: remove unnecessary >tmp Denton Liu
                   ` (4 more replies)
  0 siblings, 5 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-05 19:08 UTC (permalink / raw)
  To: Git Mailing List

The range-notation in `git diff` has been cited as a mistake since diff
compares two endpoints, not whole ranges.[0]  In fact, the ranges seem
to take on the opposite meanings when compared to range notation in
`git log`.

In an effort to reduce the use of range notation as much as possible,
introduce the `--merge-base` flag, slightly modified from a suggestion
by Jonathan Nieder.[1] This flag allows us to replace the first commit
given on the command-line with its merge base between the first and
second commits. This allows us to gently deprecate the `...` form
entirely, although that is left as an exercise to the reader ;)

One additional bonus is that this flag allows the "after" side to be not
just constrained to a commit (like with `...` notation). It can now be
the working tree or the index as well.

[0]: https://lore.kernel.org/git/xmqqy2v26hu0.fsf@gitster-ct.c.googlers.com/
[1]: https://lore.kernel.org/git/20191223215928.GB38316@google.com/

Denton Liu (4):
  t4068: remove unnecessary >tmp
  git-diff.txt: backtick quote command text
  builtin/diff: parse --no-index using parse_options()
  builtin/diff: learn --merge-base

 Documentation/git-diff.txt | 40 ++++++++++++-----
 builtin/diff.c             | 92 +++++++++++++++++++++++++++++++++-----
 diff-no-index.c            | 15 ++-----
 t/t4068-diff-symmetric.sh  | 89 +++++++++++++++++++++++++++++++++---
 4 files changed, 197 insertions(+), 39 deletions(-)

-- 
2.28.0.rc0.135.gc7877b767d


^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH 1/4] t4068: remove unnecessary >tmp
  2020-09-05 19:08 [PATCH 0/4] builtin/diff: learn --merge-base Denton Liu
@ 2020-09-05 19:08 ` Denton Liu
  2020-09-05 19:08 ` [PATCH 2/4] git-diff.txt: backtick quote command text Denton Liu
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-05 19:08 UTC (permalink / raw)
  To: Git Mailing List

The many `git diff` invocations have a `>tmp` redirection even though
the file is not being used afterwards. Remove these unnecessary
redirections.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4068-diff-symmetric.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh
index 31d17a5af0..60c506c2b2 100755
--- a/t/t4068-diff-symmetric.sh
+++ b/t/t4068-diff-symmetric.sh
@@ -64,27 +64,27 @@ test_expect_success 'diff with two merge bases' '
 '
 
 test_expect_success 'diff with no merge bases' '
-	test_must_fail git diff br2...br3 >tmp 2>err &&
+	test_must_fail git diff br2...br3 2>err &&
 	test_i18ngrep "fatal: br2...br3: no merge base" err
 '
 
 test_expect_success 'diff with too many symmetric differences' '
-	test_must_fail git diff br1...master br2...br3 >tmp 2>err &&
+	test_must_fail git diff br1...master br2...br3 2>err &&
 	test_i18ngrep "usage" err
 '
 
 test_expect_success 'diff with symmetric difference and extraneous arg' '
-	test_must_fail git diff master br1...master >tmp 2>err &&
+	test_must_fail git diff master br1...master 2>err &&
 	test_i18ngrep "usage" err
 '
 
 test_expect_success 'diff with two ranges' '
-	test_must_fail git diff master br1..master br2..br3 >tmp 2>err &&
+	test_must_fail git diff master br1..master br2..br3 2>err &&
 	test_i18ngrep "usage" err
 '
 
 test_expect_success 'diff with ranges and extra arg' '
-	test_must_fail git diff master br1..master commit-D >tmp 2>err &&
+	test_must_fail git diff master br1..master commit-D 2>err &&
 	test_i18ngrep "usage" err
 '
 
-- 
2.28.0.rc0.135.gc7877b767d


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH 2/4] git-diff.txt: backtick quote command text
  2020-09-05 19:08 [PATCH 0/4] builtin/diff: learn --merge-base Denton Liu
  2020-09-05 19:08 ` [PATCH 1/4] t4068: remove unnecessary >tmp Denton Liu
@ 2020-09-05 19:08 ` Denton Liu
  2020-09-05 19:08 ` [PATCH 3/4] builtin/diff: parse --no-index using parse_options() Denton Liu
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-05 19:08 UTC (permalink / raw)
  To: Git Mailing List

The modern way to quote commands in the documentation is to use
backticks instead of double-quotes as this renders the text with the
code style. Convert double-quoted command text to backtick-quoted
commands. While we're at it, quote one instance of `^@`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-diff.txt | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 727f24d16e..8f7b4ed3ca 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -68,13 +68,13 @@ files on disk.
 	This form is to view the results of a merge commit.  The first
 	listed <commit> must be the merge itself; the remaining two or
 	more commits should be its parents.  A convenient way to produce
-	the desired set of revisions is to use the {caret}@ suffix.
+	the desired set of revisions is to use the `^@` suffix.
 	For instance, if `master` names a merge commit, `git diff master
 	master^@` gives the same combined diff as `git show master`.
 
 'git diff' [<options>] <commit>..<commit> [--] [<path>...]::
 
-	This is synonymous to the earlier form (without the "..") for
+	This is synonymous to the earlier form (without the `..`) for
 	viewing the changes between two arbitrary <commit>.  If <commit> on
 	one side is omitted, it will have the same effect as
 	using HEAD instead.
@@ -83,20 +83,20 @@ files on disk.
 
 	This form is to view the changes on the branch containing
 	and up to the second <commit>, starting at a common ancestor
-	of both <commit>.  "git diff A\...B" is equivalent to
-	"git diff $(git merge-base A B) B".  You can omit any one
+	of both <commit>.  `git diff A...B` is equivalent to
+	`git diff $(git merge-base A B) B`.  You can omit any one
 	of <commit>, which has the same effect as using HEAD instead.
 
 Just in case you are doing something exotic, it should be
 noted that all of the <commit> in the above description, except
-in the last two forms that use ".." notations, can be any
+in the last two forms that use `..` notations, can be any
 <tree>.
 
 For a more complete list of ways to spell <commit>, see
 "SPECIFYING REVISIONS" section in linkgit:gitrevisions[7].
 However, "diff" is about comparing two _endpoints_, not ranges,
-and the range notations ("<commit>..<commit>" and
-"<commit>\...<commit>") do not mean a range as defined in the
+and the range notations (`<commit>..<commit>` and
+`<commit>...<commit>`) do not mean a range as defined in the
 "SPECIFYING RANGES" section in linkgit:gitrevisions[7].
 
 'git diff' [<options>] <blob> <blob>::
@@ -144,9 +144,9 @@ $ git diff HEAD       <3>
 +
 <1> Changes in the working tree not yet staged for the next commit.
 <2> Changes between the index and your last commit; what you
-    would be committing if you run "git commit" without "-a" option.
+    would be committing if you run `git commit` without `-a` option.
 <3> Changes in the working tree since your last commit; what you
-    would be committing if you run "git commit -a"
+    would be committing if you run `git commit -a`
 
 Comparing with arbitrary commits::
 +
-- 
2.28.0.rc0.135.gc7877b767d


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH 3/4] builtin/diff: parse --no-index using parse_options()
  2020-09-05 19:08 [PATCH 0/4] builtin/diff: learn --merge-base Denton Liu
  2020-09-05 19:08 ` [PATCH 1/4] t4068: remove unnecessary >tmp Denton Liu
  2020-09-05 19:08 ` [PATCH 2/4] git-diff.txt: backtick quote command text Denton Liu
@ 2020-09-05 19:08 ` Denton Liu
  2020-09-05 19:08 ` [PATCH 4/4] builtin/diff: learn --merge-base Denton Liu
  2020-09-10  7:32 ` [PATCH v2 0/4] " Denton Liu
  4 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-05 19:08 UTC (permalink / raw)
  To: Git Mailing List

Instead of parsing the `--no-index` option with a plain strcmp, use
parse_options() to parse options. This allows us to easily add more
options in a future commit.

As a result of this change, `--no-index` is removed from `argv` so, as a
result, we no longer need to handle it in diff_no_index().

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/diff.c  | 33 ++++++++++++++++++++++-----------
 diff-no-index.c | 15 +++------------
 2 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index cb98811c21..0e086ed7c4 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -373,6 +373,13 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	int nongit = 0, no_index = 0;
 	int result = 0;
 	struct symdiff sdiff;
+	struct option options[] = {
+		OPT_SET_INT_F(0, "no-index", &no_index,
+			   N_("compare the given paths on the filesystem"),
+			   DIFF_NO_INDEX_EXPLICIT,
+			   PARSE_OPT_NONEG),
+		OPT_END(),
+	};
 
 	/*
 	 * We could get N tree-ish in the rev.pending_objects list.
@@ -406,21 +413,25 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	 * Other cases are errors.
 	 */
 
-	/* Were we asked to do --no-index explicitly? */
-	for (i = 1; i < argc; i++) {
-		if (!strcmp(argv[i], "--")) {
-			i++;
-			break;
-		}
-		if (!strcmp(argv[i], "--no-index"))
-			no_index = DIFF_NO_INDEX_EXPLICIT;
-		if (argv[i][0] != '-')
-			break;
-	}
+	argc = parse_options(argc, argv, prefix, options, NULL,
+			     PARSE_OPT_KEEP_DASHDASH |
+			     PARSE_OPT_KEEP_ARGV0 |
+			     PARSE_OPT_KEEP_UNKNOWN |
+			     PARSE_OPT_NO_INTERNAL_HELP);
 
 	prefix = setup_git_directory_gently(&nongit);
 
 	if (!no_index) {
+		int i;
+		for (i = 1; i < argc; i++) {
+			if (!strcmp(argv[i], "--")) {
+				i++;
+				break;
+			}
+			if (argv[i][0] != '-')
+				break;
+		}
+
 		/*
 		 * Treat git diff with at least one path outside of the
 		 * repo the same as if the command would have been executed
diff --git a/diff-no-index.c b/diff-no-index.c
index 7814eabfe0..da82e2d090 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -243,28 +243,19 @@ int diff_no_index(struct rev_info *revs,
 		  int implicit_no_index,
 		  int argc, const char **argv)
 {
-	int i, no_index;
+	int i;
 	const char *paths[2];
 	struct strbuf replacement = STRBUF_INIT;
 	const char *prefix = revs->prefix;
-	struct option no_index_options[] = {
-		OPT_BOOL_F(0, "no-index", &no_index, "",
-			   PARSE_OPT_NONEG | PARSE_OPT_HIDDEN),
-		OPT_END(),
-	};
-	struct option *options;
 
-	options = parse_options_concat(no_index_options,
-				       revs->diffopt.parseopts);
-	argc = parse_options(argc, argv, revs->prefix, options,
+	argc = parse_options(argc, argv, revs->prefix, revs->diffopt.parseopts,
 			     diff_no_index_usage, 0);
 	if (argc != 2) {
 		if (implicit_no_index)
 			warning(_("Not a git repository. Use --no-index to "
 				  "compare two paths outside a working tree"));
-		usage_with_options(diff_no_index_usage, options);
+		usage_with_options(diff_no_index_usage, revs->diffopt.parseopts);
 	}
-	FREE_AND_NULL(options);
 	for (i = 0; i < 2; i++) {
 		const char *p = argv[argc - 2 + i];
 		if (!strcmp(p, "-"))
-- 
2.28.0.rc0.135.gc7877b767d


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH 4/4] builtin/diff: learn --merge-base
  2020-09-05 19:08 [PATCH 0/4] builtin/diff: learn --merge-base Denton Liu
                   ` (2 preceding siblings ...)
  2020-09-05 19:08 ` [PATCH 3/4] builtin/diff: parse --no-index using parse_options() Denton Liu
@ 2020-09-05 19:08 ` Denton Liu
  2020-09-06 21:58   ` Junio C Hamano
  2020-09-10  7:32 ` [PATCH v2 0/4] " Denton Liu
  4 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-05 19:08 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Jonathan Nieder

In order to get the diff between a commit and its merge base, the
currently preferred method is to use `git diff A...B`. However, the
range-notation with diff has, time and time again, been noted as a point
of confusion and thus, it should be avoided. Although we have a
substitute for the double-dot notation, we don't have any replacement
for the triple-dot notation.

Introduce the `--merge-base` flag as a replacement for triple-dot
notation. Thus, we would be able to write the above as
`git diff --merge-base A B`, allowing us to gently deprecate
range-notation completely.

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-diff.txt | 24 ++++++++++--
 builtin/diff.c             | 59 ++++++++++++++++++++++++++++
 t/t4068-diff-symmetric.sh  | 79 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 159 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 8f7b4ed3ca..0031729794 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 'git diff' [<options>] [<commit>] [--] [<path>...]
 'git diff' [<options>] --cached [<commit>] [--] [<path>...]
 'git diff' [<options>] <commit> [<commit>...] <commit> [--] [<path>...]
-'git diff' [<options>] <commit>...<commit> [--] [<path>...]
+'git diff' [<options>] --merge-base [--cached] [<commit> [<commit>]] [--] [<path>...]
 'git diff' [<options>] <blob> <blob>
 'git diff' [<options>] --no-index [--] <path> <path>
 
@@ -63,6 +63,24 @@ files on disk.
 	This is to view the changes between two arbitrary
 	<commit>.
 
+'git diff' [<options>] --merge-base [--cached] [<commit> [<commit>]] [--] [<path>...]::
+
+	In this form, the "before" side will be the merge base of the
+	two given commits.  If either commit is omitted, it will default
+	to HEAD.
++
+In the case where two commits are given, a diff is displayed between the
+merge base and the second commit.  `git diff --merge-base A B` is
+equivalent to `git diff $(git merge-base A B) B`.
++
+In the case where one commit is given, a diff is displayed between the
+merge base and the working tree or the index if `--cached` is given.
+`git diff --merge-base A` is equivalent to `git diff $(git merge-base A
+HEAD)`.
++
+In the case where no commits are given, this form behaves identically to
+as if no `--merge-base` were supplied.
+
 'git diff' [<options>] <commit> <commit>... <commit> [--] [<path>...]::
 
 	This form is to view the results of a merge commit.  The first
@@ -89,8 +107,8 @@ files on disk.
 
 Just in case you are doing something exotic, it should be
 noted that all of the <commit> in the above description, except
-in the last two forms that use `..` notations, can be any
-<tree>.
+in the `--merge-base` case and the last two forms that use `..`
+notations, can be any <tree>.
 
 For a more complete list of ways to spell <commit>, see
 "SPECIFYING REVISIONS" section in linkgit:gitrevisions[7].
diff --git a/builtin/diff.c b/builtin/diff.c
index 0e086ed7c4..0af5a6c8c9 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -19,6 +19,7 @@
 #include "builtin.h"
 #include "submodule.h"
 #include "oid-array.h"
+#include "commit-reach.h"
 
 #define DIFF_NO_INDEX_EXPLICIT 1
 #define DIFF_NO_INDEX_IMPLICIT 2
@@ -371,6 +372,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	int blobs = 0, paths = 0;
 	struct object_array_entry *blob[2];
 	int nongit = 0, no_index = 0;
+	int merge_base = 0;
 	int result = 0;
 	struct symdiff sdiff;
 	struct option options[] = {
@@ -378,6 +380,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 			   N_("compare the given paths on the filesystem"),
 			   DIFF_NO_INDEX_EXPLICIT,
 			   PARSE_OPT_NONEG),
+		OPT_BOOL(0, "merge-base", &merge_base,
+			 N_("use the merge base between the two commits as the diff base")),
 		OPT_END(),
 	};
 
@@ -457,6 +461,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	rev.diffopt.flags.allow_external = 1;
 	rev.diffopt.flags.allow_textconv = 1;
 
+	if (no_index && merge_base)
+		die(_("--no-index and --merge-base are mutually exclusive"));
+
 	/* If this is a no-index diff, just run it and exit there. */
 	if (no_index)
 		exit(diff_no_index(&rev, no_index == DIFF_NO_INDEX_IMPLICIT,
@@ -513,6 +520,58 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	}
 
 	symdiff_prepare(&rev, &sdiff);
+
+	if (merge_base && rev.pending.nr) {
+		int i;
+		struct commit *mb_child[2] = {0};
+		struct commit_list *merge_bases;
+		int old_nr;
+
+		for (i = 0; i < rev.pending.nr; i++) {
+			struct object *obj = rev.pending.objects[i].item;
+			if (obj->flags)
+				die(_("--merge-base does not work with ranges"));
+			if (obj->type != OBJ_COMMIT)
+				die(_("--merge-base only works with commits"));
+		}
+
+		/*
+		 * This check must go after the for loop above because A...B
+		 * ranges produce three pending commits, resulting in a
+		 * misleading error message.
+		 */
+		if (rev.pending.nr > ARRAY_SIZE(mb_child))
+			die(_("--merge-base does not work with more than two commits"));
+
+		for (i = 0; i < rev.pending.nr; i++)
+			mb_child[i] = lookup_commit_reference(the_repository, &rev.pending.objects[i].item->oid);
+		if (rev.pending.nr < ARRAY_SIZE(mb_child)) {
+			struct object_id oid;
+
+			if (rev.pending.nr != 1)
+				BUG("unexpected rev.pending.nr: %d", rev.pending.nr);
+
+			if (get_oid("HEAD", &oid))
+				die(_("unable to get HEAD"));
+
+			mb_child[1] = lookup_commit_reference(the_repository, &oid);
+		}
+
+		merge_bases = repo_get_merge_bases(the_repository, mb_child[0], mb_child[1]);
+		if (!merge_bases)
+			die(_("no merge base found"));
+		if (merge_bases->next)
+			die(_("multiple merge bases found"));
+
+		old_nr = rev.pending.nr;
+		rev.pending.nr = 1;
+		object_array_pop(&rev.pending);
+		add_object_array(&merge_bases->item->object, oid_to_hex(&merge_bases->item->object.oid), &rev.pending);
+		rev.pending.nr = old_nr;
+
+		free_commit_list(merge_bases);
+	}
+
 	for (i = 0; i < rev.pending.nr; i++) {
 		struct object_array_entry *entry = &rev.pending.objects[i];
 		struct object *obj = entry->item;
diff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh
index 60c506c2b2..0e43ed7660 100755
--- a/t/t4068-diff-symmetric.sh
+++ b/t/t4068-diff-symmetric.sh
@@ -88,4 +88,83 @@ test_expect_success 'diff with ranges and extra arg' '
 	test_i18ngrep "usage" err
 '
 
+test_expect_success 'diff --merge-base with two commits' '
+	git diff commit-C master >expect &&
+	git diff --merge-base br2 master >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'diff --merge-base with no commits' '
+	git diff --merge-base >actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success 'diff --merge-base with one commit' '
+	git checkout master &&
+	git diff commit-C >expect &&
+	git diff --merge-base br2 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'diff --merge-base with one commit and unstaged changes' '
+	git checkout master &&
+	test_when_finished git reset --hard &&
+	echo unstaged >>c &&
+	git diff commit-C >expect &&
+	git diff --merge-base br2 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'diff --merge-base with one commit and staged and unstaged changes' '
+	git checkout master &&
+	test_when_finished git reset --hard &&
+	echo staged >>c &&
+	git add c &&
+	echo unstaged >>c &&
+	git diff commit-C >expect &&
+	git diff --merge-base br2 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'diff --merge-base --cached with one commit and staged and unstaged changes' '
+	git checkout master &&
+	test_when_finished git reset --hard &&
+	echo staged >>c &&
+	git add c &&
+	echo unstaged >>c &&
+	git diff --cached commit-C >expect &&
+	git diff --cached --merge-base br2 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'diff --merge-base with --no-index' '
+	test_must_fail git diff --merge-base --no-index expect actual 2>err &&
+	test_i18ngrep "fatal: --no-index and --merge-base are mutually exclusive" err
+'
+
+test_expect_success 'diff --merge-base with range' '
+	test_must_fail git diff --merge-base br2..br3 2>err &&
+	test_i18ngrep "fatal: --merge-base does not work with ranges" err
+'
+
+test_expect_success 'diff --merge-base non-commit' '
+	test_must_fail git diff --merge-base master^{tree} 2>err &&
+	test_i18ngrep "fatal: --merge-base only works with commits" err
+'
+
+test_expect_success 'diff --merge-base with three commits' '
+	test_must_fail git diff --merge-base br1 br2 master 2>err &&
+	test_i18ngrep "fatal: --merge-base does not work with more than two commits" err
+'
+
+test_expect_success 'diff --merge-base with no merge bases' '
+	test_must_fail git diff --merge-base br2 br3 2>err &&
+	test_i18ngrep "fatal: no merge base found" err
+'
+
+test_expect_success 'diff --merge-base with multiple merge bases' '
+	test_must_fail git diff --merge-base master br1 2>err &&
+	test_i18ngrep "fatal: multiple merge bases found" err
+'
+
 test_done
-- 
2.28.0.rc0.135.gc7877b767d


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* Re: [PATCH 4/4] builtin/diff: learn --merge-base
  2020-09-05 19:08 ` [PATCH 4/4] builtin/diff: learn --merge-base Denton Liu
@ 2020-09-06 21:58   ` Junio C Hamano
  0 siblings, 0 replies; 58+ messages in thread
From: Junio C Hamano @ 2020-09-06 21:58 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List, Jonathan Nieder

Denton Liu <liu.denton@gmail.com> writes:

> +In the case where one commit is given, a diff is displayed between the
> +merge base and the working tree or the index if `--cached` is given.

s/merge base/& between the commit and the HEAD/

As "merge base" in the context of talking about "one commit" does
not make sense, such a clarification would be necessary.

Jonathan suggested "--fork-point A B"; this replaces it with
"--merge-base A B".  Neither is quite satisfying, but other
people might be able to come up with a better names.

In any case, "git diff $(git merge-base MERGE_HEAD HEAD)" while
merging to see how the changes in the working tree look like is
something I've always felt cumbersome to spell, and this is a good
addition.

> +		OPT_BOOL(0, "merge-base", &merge_base,
> +			 N_("use the merge base between the two commits as the diff base"))

Define "diff base".  Eh, that was rhetoric---do not use such an
undefined word.  "compare with the merge base between two commits"
may work better.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v2 0/4] builtin/diff: learn --merge-base
  2020-09-05 19:08 [PATCH 0/4] builtin/diff: learn --merge-base Denton Liu
                   ` (3 preceding siblings ...)
  2020-09-05 19:08 ` [PATCH 4/4] builtin/diff: learn --merge-base Denton Liu
@ 2020-09-10  7:32 ` Denton Liu
  2020-09-10  7:32   ` [PATCH v2 1/4] t4068: remove unnecessary >tmp Denton Liu
                     ` (4 more replies)
  4 siblings, 5 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-10  7:32 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

The range-notation in `git diff` has been cited as a mistake since diff
compares two endpoints, not whole ranges.[0]  In fact, the ranges seem
to take on the opposite meanings when compared to range notation in
`git log`.

In an effort to reduce the use of range notation as much as possible,
introduce the `--merge-base` flag, slightly modified from a suggestion
by Jonathan Nieder.[1] This flag allows us to replace the first commit
given on the command-line with its merge base between the first and
second commits. This allows us to gently deprecate the `...` form
entirely, although that is left as an exercise to the reader ;)

One additional bonus is that this flag allows the "after" side to be not
just constrained to a commit (like with `...` notation). It can now be
the working tree or the index as well.

The `--merge-base` name isn't very satisfying. If anyone has any better
suggestions, please let me know.

Changes since v1:

* Implement Junio's documentation suggestions

* Update git diff's usage to include this option

[0]: https://lore.kernel.org/git/xmqqy2v26hu0.fsf@gitster-ct.c.googlers.com/
[1]: https://lore.kernel.org/git/20191223215928.GB38316@google.com/

Denton Liu (4):
  t4068: remove unnecessary >tmp
  git-diff.txt: backtick quote command text
  builtin/diff: parse --no-index using parse_options()
  builtin/diff: learn --merge-base

 Documentation/git-diff.txt | 40 +++++++++++-----
 builtin/diff.c             | 94 +++++++++++++++++++++++++++++++++-----
 diff-no-index.c            | 15 ++----
 t/t4068-diff-symmetric.sh  | 89 ++++++++++++++++++++++++++++++++++--
 4 files changed, 198 insertions(+), 40 deletions(-)

Range-diff against v1:
1:  231ba3f661 ! 1:  4f219cf0d1 builtin/diff: learn --merge-base
    @@ Commit message
     
         Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
     
    +
    + ## Notes ##
    +    The `--merge-base` name isn't very satisfying. If anyone has any
    +    suggestions for alternative names, please let me know.
    +
      ## Documentation/git-diff.txt ##
     @@ Documentation/git-diff.txt: SYNOPSIS
      'git diff' [<options>] [<commit>] [--] [<path>...]
    @@ Documentation/git-diff.txt: files on disk.
     +equivalent to `git diff $(git merge-base A B) B`.
     ++
     +In the case where one commit is given, a diff is displayed between the
    -+merge base and the working tree or the index if `--cached` is given.
    -+`git diff --merge-base A` is equivalent to `git diff $(git merge-base A
    -+HEAD)`.
    ++merge base of the commit and the HEAD and the working tree or the index
    ++if `--cached` is given. `git diff --merge-base A` is equivalent to `git
    ++diff $(git merge-base A HEAD)`.
     ++
     +In the case where no commits are given, this form behaves identically to
     +as if no `--merge-base` were supplied.
    @@ builtin/diff.c
      
      #define DIFF_NO_INDEX_EXPLICIT 1
      #define DIFF_NO_INDEX_IMPLICIT 2
    +@@ builtin/diff.c: static const char builtin_diff_usage[] =
    + "git diff [<options>] [<commit>] [--] [<path>...]\n"
    + "   or: git diff [<options>] --cached [<commit>] [--] [<path>...]\n"
    + "   or: git diff [<options>] <commit> [<commit>...] <commit> [--] [<path>...]\n"
    +-"   or: git diff [<options>] <commit>...<commit>] [--] [<path>...]\n"
    ++"   or: git diff [<options>] --merge-base [<commit> [<commit>]] [--] [<path>...]\n"
    + "   or: git diff [<options>] <blob> <blob>]\n"
    + "   or: git diff [<options>] --no-index [--] <path> <path>]\n"
    + COMMON_DIFF_OPTIONS_HELP;
     @@ builtin/diff.c: int cmd_diff(int argc, const char **argv, const char *prefix)
      	int blobs = 0, paths = 0;
      	struct object_array_entry *blob[2];
    @@ builtin/diff.c: int cmd_diff(int argc, const char **argv, const char *prefix)
      			   DIFF_NO_INDEX_EXPLICIT,
      			   PARSE_OPT_NONEG),
     +		OPT_BOOL(0, "merge-base", &merge_base,
    -+			 N_("use the merge base between the two commits as the diff base")),
    ++			 N_("compare with the merge base between two commits")),
      		OPT_END(),
      	};
      
-- 
2.28.0.618.gf4bc123cb7


^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v2 1/4] t4068: remove unnecessary >tmp
  2020-09-10  7:32 ` [PATCH v2 0/4] " Denton Liu
@ 2020-09-10  7:32   ` Denton Liu
  2020-09-10  7:32   ` [PATCH v2 2/4] git-diff.txt: backtick quote command text Denton Liu
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-10  7:32 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

The many `git diff` invocations have a `>tmp` redirection even though
the file is not being used afterwards. Remove these unnecessary
redirections.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4068-diff-symmetric.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh
index 31d17a5af0..60c506c2b2 100755
--- a/t/t4068-diff-symmetric.sh
+++ b/t/t4068-diff-symmetric.sh
@@ -64,27 +64,27 @@ test_expect_success 'diff with two merge bases' '
 '
 
 test_expect_success 'diff with no merge bases' '
-	test_must_fail git diff br2...br3 >tmp 2>err &&
+	test_must_fail git diff br2...br3 2>err &&
 	test_i18ngrep "fatal: br2...br3: no merge base" err
 '
 
 test_expect_success 'diff with too many symmetric differences' '
-	test_must_fail git diff br1...master br2...br3 >tmp 2>err &&
+	test_must_fail git diff br1...master br2...br3 2>err &&
 	test_i18ngrep "usage" err
 '
 
 test_expect_success 'diff with symmetric difference and extraneous arg' '
-	test_must_fail git diff master br1...master >tmp 2>err &&
+	test_must_fail git diff master br1...master 2>err &&
 	test_i18ngrep "usage" err
 '
 
 test_expect_success 'diff with two ranges' '
-	test_must_fail git diff master br1..master br2..br3 >tmp 2>err &&
+	test_must_fail git diff master br1..master br2..br3 2>err &&
 	test_i18ngrep "usage" err
 '
 
 test_expect_success 'diff with ranges and extra arg' '
-	test_must_fail git diff master br1..master commit-D >tmp 2>err &&
+	test_must_fail git diff master br1..master commit-D 2>err &&
 	test_i18ngrep "usage" err
 '
 
-- 
2.28.0.618.gf4bc123cb7


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v2 2/4] git-diff.txt: backtick quote command text
  2020-09-10  7:32 ` [PATCH v2 0/4] " Denton Liu
  2020-09-10  7:32   ` [PATCH v2 1/4] t4068: remove unnecessary >tmp Denton Liu
@ 2020-09-10  7:32   ` Denton Liu
  2020-09-10  7:32   ` [PATCH v2 3/4] builtin/diff: parse --no-index using parse_options() Denton Liu
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-10  7:32 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

The modern way to quote commands in the documentation is to use
backticks instead of double-quotes as this renders the text with the
code style. Convert double-quoted command text to backtick-quoted
commands. While we're at it, quote one instance of `^@`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-diff.txt | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 727f24d16e..8f7b4ed3ca 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -68,13 +68,13 @@ files on disk.
 	This form is to view the results of a merge commit.  The first
 	listed <commit> must be the merge itself; the remaining two or
 	more commits should be its parents.  A convenient way to produce
-	the desired set of revisions is to use the {caret}@ suffix.
+	the desired set of revisions is to use the `^@` suffix.
 	For instance, if `master` names a merge commit, `git diff master
 	master^@` gives the same combined diff as `git show master`.
 
 'git diff' [<options>] <commit>..<commit> [--] [<path>...]::
 
-	This is synonymous to the earlier form (without the "..") for
+	This is synonymous to the earlier form (without the `..`) for
 	viewing the changes between two arbitrary <commit>.  If <commit> on
 	one side is omitted, it will have the same effect as
 	using HEAD instead.
@@ -83,20 +83,20 @@ files on disk.
 
 	This form is to view the changes on the branch containing
 	and up to the second <commit>, starting at a common ancestor
-	of both <commit>.  "git diff A\...B" is equivalent to
-	"git diff $(git merge-base A B) B".  You can omit any one
+	of both <commit>.  `git diff A...B` is equivalent to
+	`git diff $(git merge-base A B) B`.  You can omit any one
 	of <commit>, which has the same effect as using HEAD instead.
 
 Just in case you are doing something exotic, it should be
 noted that all of the <commit> in the above description, except
-in the last two forms that use ".." notations, can be any
+in the last two forms that use `..` notations, can be any
 <tree>.
 
 For a more complete list of ways to spell <commit>, see
 "SPECIFYING REVISIONS" section in linkgit:gitrevisions[7].
 However, "diff" is about comparing two _endpoints_, not ranges,
-and the range notations ("<commit>..<commit>" and
-"<commit>\...<commit>") do not mean a range as defined in the
+and the range notations (`<commit>..<commit>` and
+`<commit>...<commit>`) do not mean a range as defined in the
 "SPECIFYING RANGES" section in linkgit:gitrevisions[7].
 
 'git diff' [<options>] <blob> <blob>::
@@ -144,9 +144,9 @@ $ git diff HEAD       <3>
 +
 <1> Changes in the working tree not yet staged for the next commit.
 <2> Changes between the index and your last commit; what you
-    would be committing if you run "git commit" without "-a" option.
+    would be committing if you run `git commit` without `-a` option.
 <3> Changes in the working tree since your last commit; what you
-    would be committing if you run "git commit -a"
+    would be committing if you run `git commit -a`
 
 Comparing with arbitrary commits::
 +
-- 
2.28.0.618.gf4bc123cb7


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v2 3/4] builtin/diff: parse --no-index using parse_options()
  2020-09-10  7:32 ` [PATCH v2 0/4] " Denton Liu
  2020-09-10  7:32   ` [PATCH v2 1/4] t4068: remove unnecessary >tmp Denton Liu
  2020-09-10  7:32   ` [PATCH v2 2/4] git-diff.txt: backtick quote command text Denton Liu
@ 2020-09-10  7:32   ` Denton Liu
  2020-09-10 18:35     ` Junio C Hamano
  2020-09-10  7:32   ` [PATCH v2 4/4] builtin/diff: learn --merge-base Denton Liu
  2020-09-17  7:44   ` [PATCH v3 00/10] " Denton Liu
  4 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-10  7:32 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

Instead of parsing the `--no-index` option with a plain strcmp, use
parse_options() to parse options. This allows us to easily add more
options in a future commit.

As a result of this change, `--no-index` is removed from `argv` so, as a
result, we no longer need to handle it in diff_no_index().

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/diff.c  | 33 ++++++++++++++++++++++-----------
 diff-no-index.c | 15 +++------------
 2 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index cb98811c21..0e086ed7c4 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -373,6 +373,13 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	int nongit = 0, no_index = 0;
 	int result = 0;
 	struct symdiff sdiff;
+	struct option options[] = {
+		OPT_SET_INT_F(0, "no-index", &no_index,
+			   N_("compare the given paths on the filesystem"),
+			   DIFF_NO_INDEX_EXPLICIT,
+			   PARSE_OPT_NONEG),
+		OPT_END(),
+	};
 
 	/*
 	 * We could get N tree-ish in the rev.pending_objects list.
@@ -406,21 +413,25 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	 * Other cases are errors.
 	 */
 
-	/* Were we asked to do --no-index explicitly? */
-	for (i = 1; i < argc; i++) {
-		if (!strcmp(argv[i], "--")) {
-			i++;
-			break;
-		}
-		if (!strcmp(argv[i], "--no-index"))
-			no_index = DIFF_NO_INDEX_EXPLICIT;
-		if (argv[i][0] != '-')
-			break;
-	}
+	argc = parse_options(argc, argv, prefix, options, NULL,
+			     PARSE_OPT_KEEP_DASHDASH |
+			     PARSE_OPT_KEEP_ARGV0 |
+			     PARSE_OPT_KEEP_UNKNOWN |
+			     PARSE_OPT_NO_INTERNAL_HELP);
 
 	prefix = setup_git_directory_gently(&nongit);
 
 	if (!no_index) {
+		int i;
+		for (i = 1; i < argc; i++) {
+			if (!strcmp(argv[i], "--")) {
+				i++;
+				break;
+			}
+			if (argv[i][0] != '-')
+				break;
+		}
+
 		/*
 		 * Treat git diff with at least one path outside of the
 		 * repo the same as if the command would have been executed
diff --git a/diff-no-index.c b/diff-no-index.c
index 7814eabfe0..da82e2d090 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -243,28 +243,19 @@ int diff_no_index(struct rev_info *revs,
 		  int implicit_no_index,
 		  int argc, const char **argv)
 {
-	int i, no_index;
+	int i;
 	const char *paths[2];
 	struct strbuf replacement = STRBUF_INIT;
 	const char *prefix = revs->prefix;
-	struct option no_index_options[] = {
-		OPT_BOOL_F(0, "no-index", &no_index, "",
-			   PARSE_OPT_NONEG | PARSE_OPT_HIDDEN),
-		OPT_END(),
-	};
-	struct option *options;
 
-	options = parse_options_concat(no_index_options,
-				       revs->diffopt.parseopts);
-	argc = parse_options(argc, argv, revs->prefix, options,
+	argc = parse_options(argc, argv, revs->prefix, revs->diffopt.parseopts,
 			     diff_no_index_usage, 0);
 	if (argc != 2) {
 		if (implicit_no_index)
 			warning(_("Not a git repository. Use --no-index to "
 				  "compare two paths outside a working tree"));
-		usage_with_options(diff_no_index_usage, options);
+		usage_with_options(diff_no_index_usage, revs->diffopt.parseopts);
 	}
-	FREE_AND_NULL(options);
 	for (i = 0; i < 2; i++) {
 		const char *p = argv[argc - 2 + i];
 		if (!strcmp(p, "-"))
-- 
2.28.0.618.gf4bc123cb7


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v2 4/4] builtin/diff: learn --merge-base
  2020-09-10  7:32 ` [PATCH v2 0/4] " Denton Liu
                     ` (2 preceding siblings ...)
  2020-09-10  7:32   ` [PATCH v2 3/4] builtin/diff: parse --no-index using parse_options() Denton Liu
@ 2020-09-10  7:32   ` Denton Liu
  2020-09-17  7:44   ` [PATCH v3 00/10] " Denton Liu
  4 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-10  7:32 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Jonathan Nieder

In order to get the diff between a commit and its merge base, the
currently preferred method is to use `git diff A...B`. However, the
range-notation with diff has, time and time again, been noted as a point
of confusion and thus, it should be avoided. Although we have a
substitute for the double-dot notation, we don't have any replacement
for the triple-dot notation.

Introduce the `--merge-base` flag as a replacement for triple-dot
notation. Thus, we would be able to write the above as
`git diff --merge-base A B`, allowing us to gently deprecate
range-notation completely.

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---

Notes:
    The `--merge-base` name isn't very satisfying. If anyone has any
    suggestions for alternative names, please let me know.

 Documentation/git-diff.txt | 24 ++++++++++--
 builtin/diff.c             | 61 ++++++++++++++++++++++++++++-
 t/t4068-diff-symmetric.sh  | 79 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 160 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 8f7b4ed3ca..2cab6eabe1 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 'git diff' [<options>] [<commit>] [--] [<path>...]
 'git diff' [<options>] --cached [<commit>] [--] [<path>...]
 'git diff' [<options>] <commit> [<commit>...] <commit> [--] [<path>...]
-'git diff' [<options>] <commit>...<commit> [--] [<path>...]
+'git diff' [<options>] --merge-base [--cached] [<commit> [<commit>]] [--] [<path>...]
 'git diff' [<options>] <blob> <blob>
 'git diff' [<options>] --no-index [--] <path> <path>
 
@@ -63,6 +63,24 @@ files on disk.
 	This is to view the changes between two arbitrary
 	<commit>.
 
+'git diff' [<options>] --merge-base [--cached] [<commit> [<commit>]] [--] [<path>...]::
+
+	In this form, the "before" side will be the merge base of the
+	two given commits.  If either commit is omitted, it will default
+	to HEAD.
++
+In the case where two commits are given, a diff is displayed between the
+merge base and the second commit.  `git diff --merge-base A B` is
+equivalent to `git diff $(git merge-base A B) B`.
++
+In the case where one commit is given, a diff is displayed between the
+merge base of the commit and the HEAD and the working tree or the index
+if `--cached` is given. `git diff --merge-base A` is equivalent to `git
+diff $(git merge-base A HEAD)`.
++
+In the case where no commits are given, this form behaves identically to
+as if no `--merge-base` were supplied.
+
 'git diff' [<options>] <commit> <commit>... <commit> [--] [<path>...]::
 
 	This form is to view the results of a merge commit.  The first
@@ -89,8 +107,8 @@ files on disk.
 
 Just in case you are doing something exotic, it should be
 noted that all of the <commit> in the above description, except
-in the last two forms that use `..` notations, can be any
-<tree>.
+in the `--merge-base` case and the last two forms that use `..`
+notations, can be any <tree>.
 
 For a more complete list of ways to spell <commit>, see
 "SPECIFYING REVISIONS" section in linkgit:gitrevisions[7].
diff --git a/builtin/diff.c b/builtin/diff.c
index 0e086ed7c4..448d2dd69a 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -19,6 +19,7 @@
 #include "builtin.h"
 #include "submodule.h"
 #include "oid-array.h"
+#include "commit-reach.h"
 
 #define DIFF_NO_INDEX_EXPLICIT 1
 #define DIFF_NO_INDEX_IMPLICIT 2
@@ -27,7 +28,7 @@ static const char builtin_diff_usage[] =
 "git diff [<options>] [<commit>] [--] [<path>...]\n"
 "   or: git diff [<options>] --cached [<commit>] [--] [<path>...]\n"
 "   or: git diff [<options>] <commit> [<commit>...] <commit> [--] [<path>...]\n"
-"   or: git diff [<options>] <commit>...<commit>] [--] [<path>...]\n"
+"   or: git diff [<options>] --merge-base [<commit> [<commit>]] [--] [<path>...]\n"
 "   or: git diff [<options>] <blob> <blob>]\n"
 "   or: git diff [<options>] --no-index [--] <path> <path>]\n"
 COMMON_DIFF_OPTIONS_HELP;
@@ -371,6 +372,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	int blobs = 0, paths = 0;
 	struct object_array_entry *blob[2];
 	int nongit = 0, no_index = 0;
+	int merge_base = 0;
 	int result = 0;
 	struct symdiff sdiff;
 	struct option options[] = {
@@ -378,6 +380,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 			   N_("compare the given paths on the filesystem"),
 			   DIFF_NO_INDEX_EXPLICIT,
 			   PARSE_OPT_NONEG),
+		OPT_BOOL(0, "merge-base", &merge_base,
+			 N_("compare with the merge base between two commits")),
 		OPT_END(),
 	};
 
@@ -457,6 +461,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	rev.diffopt.flags.allow_external = 1;
 	rev.diffopt.flags.allow_textconv = 1;
 
+	if (no_index && merge_base)
+		die(_("--no-index and --merge-base are mutually exclusive"));
+
 	/* If this is a no-index diff, just run it and exit there. */
 	if (no_index)
 		exit(diff_no_index(&rev, no_index == DIFF_NO_INDEX_IMPLICIT,
@@ -513,6 +520,58 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	}
 
 	symdiff_prepare(&rev, &sdiff);
+
+	if (merge_base && rev.pending.nr) {
+		int i;
+		struct commit *mb_child[2] = {0};
+		struct commit_list *merge_bases;
+		int old_nr;
+
+		for (i = 0; i < rev.pending.nr; i++) {
+			struct object *obj = rev.pending.objects[i].item;
+			if (obj->flags)
+				die(_("--merge-base does not work with ranges"));
+			if (obj->type != OBJ_COMMIT)
+				die(_("--merge-base only works with commits"));
+		}
+
+		/*
+		 * This check must go after the for loop above because A...B
+		 * ranges produce three pending commits, resulting in a
+		 * misleading error message.
+		 */
+		if (rev.pending.nr > ARRAY_SIZE(mb_child))
+			die(_("--merge-base does not work with more than two commits"));
+
+		for (i = 0; i < rev.pending.nr; i++)
+			mb_child[i] = lookup_commit_reference(the_repository, &rev.pending.objects[i].item->oid);
+		if (rev.pending.nr < ARRAY_SIZE(mb_child)) {
+			struct object_id oid;
+
+			if (rev.pending.nr != 1)
+				BUG("unexpected rev.pending.nr: %d", rev.pending.nr);
+
+			if (get_oid("HEAD", &oid))
+				die(_("unable to get HEAD"));
+
+			mb_child[1] = lookup_commit_reference(the_repository, &oid);
+		}
+
+		merge_bases = repo_get_merge_bases(the_repository, mb_child[0], mb_child[1]);
+		if (!merge_bases)
+			die(_("no merge base found"));
+		if (merge_bases->next)
+			die(_("multiple merge bases found"));
+
+		old_nr = rev.pending.nr;
+		rev.pending.nr = 1;
+		object_array_pop(&rev.pending);
+		add_object_array(&merge_bases->item->object, oid_to_hex(&merge_bases->item->object.oid), &rev.pending);
+		rev.pending.nr = old_nr;
+
+		free_commit_list(merge_bases);
+	}
+
 	for (i = 0; i < rev.pending.nr; i++) {
 		struct object_array_entry *entry = &rev.pending.objects[i];
 		struct object *obj = entry->item;
diff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh
index 60c506c2b2..0e43ed7660 100755
--- a/t/t4068-diff-symmetric.sh
+++ b/t/t4068-diff-symmetric.sh
@@ -88,4 +88,83 @@ test_expect_success 'diff with ranges and extra arg' '
 	test_i18ngrep "usage" err
 '
 
+test_expect_success 'diff --merge-base with two commits' '
+	git diff commit-C master >expect &&
+	git diff --merge-base br2 master >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'diff --merge-base with no commits' '
+	git diff --merge-base >actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success 'diff --merge-base with one commit' '
+	git checkout master &&
+	git diff commit-C >expect &&
+	git diff --merge-base br2 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'diff --merge-base with one commit and unstaged changes' '
+	git checkout master &&
+	test_when_finished git reset --hard &&
+	echo unstaged >>c &&
+	git diff commit-C >expect &&
+	git diff --merge-base br2 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'diff --merge-base with one commit and staged and unstaged changes' '
+	git checkout master &&
+	test_when_finished git reset --hard &&
+	echo staged >>c &&
+	git add c &&
+	echo unstaged >>c &&
+	git diff commit-C >expect &&
+	git diff --merge-base br2 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'diff --merge-base --cached with one commit and staged and unstaged changes' '
+	git checkout master &&
+	test_when_finished git reset --hard &&
+	echo staged >>c &&
+	git add c &&
+	echo unstaged >>c &&
+	git diff --cached commit-C >expect &&
+	git diff --cached --merge-base br2 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'diff --merge-base with --no-index' '
+	test_must_fail git diff --merge-base --no-index expect actual 2>err &&
+	test_i18ngrep "fatal: --no-index and --merge-base are mutually exclusive" err
+'
+
+test_expect_success 'diff --merge-base with range' '
+	test_must_fail git diff --merge-base br2..br3 2>err &&
+	test_i18ngrep "fatal: --merge-base does not work with ranges" err
+'
+
+test_expect_success 'diff --merge-base non-commit' '
+	test_must_fail git diff --merge-base master^{tree} 2>err &&
+	test_i18ngrep "fatal: --merge-base only works with commits" err
+'
+
+test_expect_success 'diff --merge-base with three commits' '
+	test_must_fail git diff --merge-base br1 br2 master 2>err &&
+	test_i18ngrep "fatal: --merge-base does not work with more than two commits" err
+'
+
+test_expect_success 'diff --merge-base with no merge bases' '
+	test_must_fail git diff --merge-base br2 br3 2>err &&
+	test_i18ngrep "fatal: no merge base found" err
+'
+
+test_expect_success 'diff --merge-base with multiple merge bases' '
+	test_must_fail git diff --merge-base master br1 2>err &&
+	test_i18ngrep "fatal: multiple merge bases found" err
+'
+
 test_done
-- 
2.28.0.618.gf4bc123cb7


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* Re: [PATCH v2 3/4] builtin/diff: parse --no-index using parse_options()
  2020-09-10  7:32   ` [PATCH v2 3/4] builtin/diff: parse --no-index using parse_options() Denton Liu
@ 2020-09-10 18:35     ` Junio C Hamano
  2020-09-13  8:31       ` Denton Liu
  0 siblings, 1 reply; 58+ messages in thread
From: Junio C Hamano @ 2020-09-10 18:35 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

Denton Liu <liu.denton@gmail.com> writes:

> Instead of parsing the `--no-index` option with a plain strcmp, use
> parse_options() to parse options. This allows us to easily add more
> options in a future commit.
>
> As a result of this change, `--no-index` is removed from `argv` so, as a
> result, we no longer need to handle it in diff_no_index().
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  builtin/diff.c  | 33 ++++++++++++++++++++++-----------
>  diff-no-index.c | 15 +++------------
>  2 files changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/builtin/diff.c b/builtin/diff.c
> index cb98811c21..0e086ed7c4 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -373,6 +373,13 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>  	int nongit = 0, no_index = 0;
>  	int result = 0;
>  	struct symdiff sdiff;
> +	struct option options[] = {
> +		OPT_SET_INT_F(0, "no-index", &no_index,
> +			   N_("compare the given paths on the filesystem"),
> +			   DIFF_NO_INDEX_EXPLICIT,
> +			   PARSE_OPT_NONEG),
> +		OPT_END(),
> +	};

What's the reasoning behind teaching the "--merge-base" option only
to "diff" and not allowing "diff-index" and "diff-tree" to also
benefit from the new support?  It should be taught to be handled by
diff.c::diff_opt_parse() instead, like all other "diff" options.  I
simply do not see what makes "--merge-base" so special compared to
other options like "-R", "--color-words", etc.

And with that, this step will become totally unnecessary, because
the options[] array will still have only the "no-index" option and
nothing else.

UNLESS we are planning to use this array to parse all diff options
here, whether it is for the no-index mode or the normal invocation
of diff, but that is unlikely to happen---we'd still share many
options between the plumbing "diff-*" family and Porcelain "diff"
commands.

Thanks.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v2 3/4] builtin/diff: parse --no-index using parse_options()
  2020-09-10 18:35     ` Junio C Hamano
@ 2020-09-13  8:31       ` Denton Liu
  2020-09-13 21:45         ` Junio C Hamano
  0 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-13  8:31 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List

Hi Junio,

On Thu, Sep 10, 2020 at 11:35:42AM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > Instead of parsing the `--no-index` option with a plain strcmp, use
> > parse_options() to parse options. This allows us to easily add more
> > options in a future commit.
> >
> > As a result of this change, `--no-index` is removed from `argv` so, as a
> > result, we no longer need to handle it in diff_no_index().
> >
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> >  builtin/diff.c  | 33 ++++++++++++++++++++++-----------
> >  diff-no-index.c | 15 +++------------
> >  2 files changed, 25 insertions(+), 23 deletions(-)
> >
> > diff --git a/builtin/diff.c b/builtin/diff.c
> > index cb98811c21..0e086ed7c4 100644
> > --- a/builtin/diff.c
> > +++ b/builtin/diff.c
> > @@ -373,6 +373,13 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
> >  	int nongit = 0, no_index = 0;
> >  	int result = 0;
> >  	struct symdiff sdiff;
> > +	struct option options[] = {
> > +		OPT_SET_INT_F(0, "no-index", &no_index,
> > +			   N_("compare the given paths on the filesystem"),
> > +			   DIFF_NO_INDEX_EXPLICIT,
> > +			   PARSE_OPT_NONEG),
> > +		OPT_END(),
> > +	};
> 
> What's the reasoning behind teaching the "--merge-base" option only
> to "diff" and not allowing "diff-index" and "diff-tree" to also
> benefit from the new support?

This is a good idea. Initially, I was hesitant because the manpages for
diff-index and diff-tree both list the argument as <treeish> but I think
it makes sense that these commands should accept --merge-base too.

> It should be taught to be handled by
> diff.c::diff_opt_parse() instead, like all other "diff" options.  I
> simply do not see what makes "--merge-base" so special compared to
> other options like "-R", "--color-words", etc.

Unfortunately, despite the above, I don't think we'll be able to handle
this in diff_opt_parse(). --merge-base won't be common to all diff
modes. It'll only work with diff-index and diff-tree, so it'll have to
be handled in those modes specifically.

Thanks,
Denton

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v2 3/4] builtin/diff: parse --no-index using parse_options()
  2020-09-13  8:31       ` Denton Liu
@ 2020-09-13 21:45         ` Junio C Hamano
  0 siblings, 0 replies; 58+ messages in thread
From: Junio C Hamano @ 2020-09-13 21:45 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

Denton Liu <liu.denton@gmail.com> writes:

>> It should be taught to be handled by
>> diff.c::diff_opt_parse() instead, like all other "diff" options.  I
>> simply do not see what makes "--merge-base" so special compared to
>> other options like "-R", "--color-words", etc.
>
> Unfortunately, despite the above, I don't think we'll be able to handle
> this in diff_opt_parse(). --merge-base won't be common to all diff
> modes. It'll only work with diff-index and diff-tree, so it'll have to
> be handled in those modes specifically.

Yes, I think we can follow precedences, like the "--cached" option,
"builtin/diff-index.c::cmd_diff_index()" knows about it and it is
also handled by "builtin/diff.c::builtin_diff_index()".

Thanks.



^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v3 00/10] builtin/diff: learn --merge-base
  2020-09-10  7:32 ` [PATCH v2 0/4] " Denton Liu
                     ` (3 preceding siblings ...)
  2020-09-10  7:32   ` [PATCH v2 4/4] builtin/diff: learn --merge-base Denton Liu
@ 2020-09-17  7:44   ` Denton Liu
  2020-09-17  7:44     ` [PATCH v3 01/10] t4068: remove unnecessary >tmp Denton Liu
                       ` (10 more replies)
  4 siblings, 11 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-17  7:44 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

The range-notation in `git diff` has been cited as a mistake since diff
compares two endpoints, not whole ranges.[0]  In fact, the ranges seem
to take on the opposite meanings when compared to range notation in
`git log`.

In an effort to reduce the use of range notation as much as possible,
introduce the `--merge-base` flag, slightly modified from a suggestion
by Jonathan Nieder.[1] This flag allows us to replace the first commit
given on the command-line with its merge base between the first and
second commits. This allows us to gently deprecate the `...` form
entirely, although that is left as an exercise to the reader ;)

One additional bonus is that this flag allows the "after" side to be not
just constrained to a commit (like with `...` notation). It can now be
the working tree or the index as well.

The `--merge-base` name isn't very satisfying. If anyone has any better
suggestions, please let me know.

Changes since v1:

* Implement Junio's documentation suggestions

* Update git diff's usage to include this option

Changes since v2:

* Teach diff-index and diff-tree the --merge-base option as well

[0]: https://lore.kernel.org/git/xmqqy2v26hu0.fsf@gitster-ct.c.googlers.com/
[1]: https://lore.kernel.org/git/20191223215928.GB38316@google.com/

Denton Liu (10):
  t4068: remove unnecessary >tmp
  git-diff-index.txt: make --cached description a proper sentence
  git-diff.txt: backtick quote command text
  contrib/completion: extract common diff/difftool options
  diff-lib: accept option flags in run_diff_index()
  diff-lib: define diff_get_merge_base()
  t4068: add --merge-base tests
  builtin/diff-index: learn --merge-base
  builtin/diff-tree: learn --merge-base
  contrib/completion: complete `git diff --merge-base`

 Documentation/git-diff-index.txt       |   9 +-
 Documentation/git-diff-tree.txt        |   7 +-
 Documentation/git-diff.txt             |  37 +++--
 builtin/diff-index.c                   |  10 +-
 builtin/diff-tree.c                    |  18 +++
 builtin/diff.c                         |  50 ++++---
 contrib/completion/git-completion.bash |  15 +-
 diff-lib.c                             |  65 ++++++++-
 diff.h                                 |   6 +-
 t/t4068-diff-symmetric-merge-base.sh   | 193 +++++++++++++++++++++++++
 t/t4068-diff-symmetric.sh              |  91 ------------
 11 files changed, 358 insertions(+), 143 deletions(-)
 create mode 100755 t/t4068-diff-symmetric-merge-base.sh
 delete mode 100755 t/t4068-diff-symmetric.sh

Range-diff against v2:
 1:  80e9066a59 =  1:  80e9066a59 t4068: remove unnecessary >tmp
 -:  ---------- >  2:  21b20281e6 git-diff-index.txt: make --cached description a proper sentence
 2:  8e72bd8fea =  3:  ca9568c2ea git-diff.txt: backtick quote command text
 3:  ea6717e7b3 <  -:  ---------- builtin/diff: parse --no-index using parse_options()
 4:  4f219cf0d1 <  -:  ---------- builtin/diff: learn --merge-base
 -:  ---------- >  4:  1ac8459541 contrib/completion: extract common diff/difftool options
 -:  ---------- >  5:  496908ac10 diff-lib: accept option flags in run_diff_index()
 -:  ---------- >  6:  6aac57ca02 diff-lib: define diff_get_merge_base()
 -:  ---------- >  7:  c9225a0440 t4068: add --merge-base tests
 -:  ---------- >  8:  1e4f805e57 builtin/diff-index: learn --merge-base
 -:  ---------- >  9:  c0d27b125e builtin/diff-tree: learn --merge-base
 -:  ---------- > 10:  42a8c9b665 contrib/completion: complete `git diff --merge-base`
-- 
2.28.0.618.gf4bc123cb7


^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v3 01/10] t4068: remove unnecessary >tmp
  2020-09-17  7:44   ` [PATCH v3 00/10] " Denton Liu
@ 2020-09-17  7:44     ` Denton Liu
  2020-09-17  7:44     ` [PATCH v3 02/10] git-diff-index.txt: make --cached description a proper sentence Denton Liu
                       ` (9 subsequent siblings)
  10 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-17  7:44 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

The many `git diff` invocations have a `>tmp` redirection even though
the file is not being used afterwards. Remove these unnecessary
redirections.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4068-diff-symmetric.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh
index 31d17a5af0..60c506c2b2 100755
--- a/t/t4068-diff-symmetric.sh
+++ b/t/t4068-diff-symmetric.sh
@@ -64,27 +64,27 @@ test_expect_success 'diff with two merge bases' '
 '
 
 test_expect_success 'diff with no merge bases' '
-	test_must_fail git diff br2...br3 >tmp 2>err &&
+	test_must_fail git diff br2...br3 2>err &&
 	test_i18ngrep "fatal: br2...br3: no merge base" err
 '
 
 test_expect_success 'diff with too many symmetric differences' '
-	test_must_fail git diff br1...master br2...br3 >tmp 2>err &&
+	test_must_fail git diff br1...master br2...br3 2>err &&
 	test_i18ngrep "usage" err
 '
 
 test_expect_success 'diff with symmetric difference and extraneous arg' '
-	test_must_fail git diff master br1...master >tmp 2>err &&
+	test_must_fail git diff master br1...master 2>err &&
 	test_i18ngrep "usage" err
 '
 
 test_expect_success 'diff with two ranges' '
-	test_must_fail git diff master br1..master br2..br3 >tmp 2>err &&
+	test_must_fail git diff master br1..master br2..br3 2>err &&
 	test_i18ngrep "usage" err
 '
 
 test_expect_success 'diff with ranges and extra arg' '
-	test_must_fail git diff master br1..master commit-D >tmp 2>err &&
+	test_must_fail git diff master br1..master commit-D 2>err &&
 	test_i18ngrep "usage" err
 '
 
-- 
2.28.0.618.gf4bc123cb7


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v3 02/10] git-diff-index.txt: make --cached description a proper sentence
  2020-09-17  7:44   ` [PATCH v3 00/10] " Denton Liu
  2020-09-17  7:44     ` [PATCH v3 01/10] t4068: remove unnecessary >tmp Denton Liu
@ 2020-09-17  7:44     ` Denton Liu
  2020-09-17  7:44     ` [PATCH v3 03/10] git-diff.txt: backtick quote command text Denton Liu
                       ` (8 subsequent siblings)
  10 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-17  7:44 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-diff-index.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-diff-index.txt b/Documentation/git-diff-index.txt
index f4bd8155c0..25fe165f00 100644
--- a/Documentation/git-diff-index.txt
+++ b/Documentation/git-diff-index.txt
@@ -27,7 +27,7 @@ include::diff-options.txt[]
 	The id of a tree object to diff against.
 
 --cached::
-	do not consider the on-disk file at all
+	Do not consider the on-disk file at all.
 
 -m::
 	By default, files recorded in the index but not checked
-- 
2.28.0.618.gf4bc123cb7


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v3 03/10] git-diff.txt: backtick quote command text
  2020-09-17  7:44   ` [PATCH v3 00/10] " Denton Liu
  2020-09-17  7:44     ` [PATCH v3 01/10] t4068: remove unnecessary >tmp Denton Liu
  2020-09-17  7:44     ` [PATCH v3 02/10] git-diff-index.txt: make --cached description a proper sentence Denton Liu
@ 2020-09-17  7:44     ` Denton Liu
  2020-09-17  7:44     ` [PATCH v3 04/10] contrib/completion: extract common diff/difftool options Denton Liu
                       ` (7 subsequent siblings)
  10 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-17  7:44 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

The modern way to quote commands in the documentation is to use
backticks instead of double-quotes as this renders the text with the
code style. Convert double-quoted command text to backtick-quoted
commands. While we're at it, quote one instance of `^@`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-diff.txt | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 727f24d16e..8f7b4ed3ca 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -68,13 +68,13 @@ files on disk.
 	This form is to view the results of a merge commit.  The first
 	listed <commit> must be the merge itself; the remaining two or
 	more commits should be its parents.  A convenient way to produce
-	the desired set of revisions is to use the {caret}@ suffix.
+	the desired set of revisions is to use the `^@` suffix.
 	For instance, if `master` names a merge commit, `git diff master
 	master^@` gives the same combined diff as `git show master`.
 
 'git diff' [<options>] <commit>..<commit> [--] [<path>...]::
 
-	This is synonymous to the earlier form (without the "..") for
+	This is synonymous to the earlier form (without the `..`) for
 	viewing the changes between two arbitrary <commit>.  If <commit> on
 	one side is omitted, it will have the same effect as
 	using HEAD instead.
@@ -83,20 +83,20 @@ files on disk.
 
 	This form is to view the changes on the branch containing
 	and up to the second <commit>, starting at a common ancestor
-	of both <commit>.  "git diff A\...B" is equivalent to
-	"git diff $(git merge-base A B) B".  You can omit any one
+	of both <commit>.  `git diff A...B` is equivalent to
+	`git diff $(git merge-base A B) B`.  You can omit any one
 	of <commit>, which has the same effect as using HEAD instead.
 
 Just in case you are doing something exotic, it should be
 noted that all of the <commit> in the above description, except
-in the last two forms that use ".." notations, can be any
+in the last two forms that use `..` notations, can be any
 <tree>.
 
 For a more complete list of ways to spell <commit>, see
 "SPECIFYING REVISIONS" section in linkgit:gitrevisions[7].
 However, "diff" is about comparing two _endpoints_, not ranges,
-and the range notations ("<commit>..<commit>" and
-"<commit>\...<commit>") do not mean a range as defined in the
+and the range notations (`<commit>..<commit>` and
+`<commit>...<commit>`) do not mean a range as defined in the
 "SPECIFYING RANGES" section in linkgit:gitrevisions[7].
 
 'git diff' [<options>] <blob> <blob>::
@@ -144,9 +144,9 @@ $ git diff HEAD       <3>
 +
 <1> Changes in the working tree not yet staged for the next commit.
 <2> Changes between the index and your last commit; what you
-    would be committing if you run "git commit" without "-a" option.
+    would be committing if you run `git commit` without `-a` option.
 <3> Changes in the working tree since your last commit; what you
-    would be committing if you run "git commit -a"
+    would be committing if you run `git commit -a`
 
 Comparing with arbitrary commits::
 +
-- 
2.28.0.618.gf4bc123cb7


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v3 04/10] contrib/completion: extract common diff/difftool options
  2020-09-17  7:44   ` [PATCH v3 00/10] " Denton Liu
                       ` (2 preceding siblings ...)
  2020-09-17  7:44     ` [PATCH v3 03/10] git-diff.txt: backtick quote command text Denton Liu
@ 2020-09-17  7:44     ` Denton Liu
  2020-09-17  7:44     ` [PATCH v3 05/10] diff-lib: accept option flags in run_diff_index() Denton Liu
                       ` (6 subsequent siblings)
  10 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-17  7:44 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

difftool parses its own options and then passes the remaining options
onto diff. As a result, they share common command-line options. Instead
of duplicating the list, use a shared $__git_diff_difftool_options list.

The completion for diff is missing --relative and the completion for
difftool is missing --no-index. Add both of these to the common list.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/completion/git-completion.bash | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9147fba3d5..f68c8e0646 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1691,6 +1691,10 @@ __git_diff_common_options="--stat --numstat --shortstat --summary
 			--textconv --no-textconv
 "
 
+__git_diff_difftool_options="--cached --staged --pickaxe-all --pickaxe-regex
+			--base --ours --theirs --no-index --relative
+			$__git_diff_common_options"
+
 _git_diff ()
 {
 	__git_has_doubledash && return
@@ -1713,10 +1717,7 @@ _git_diff ()
 		return
 		;;
 	--*)
-		__gitcomp "--cached --staged --pickaxe-all --pickaxe-regex
-			--base --ours --theirs --no-index
-			$__git_diff_common_options
-			"
+		__gitcomp "$__git_diff_difftool_options"
 		return
 		;;
 	esac
@@ -1738,11 +1739,7 @@ _git_difftool ()
 		return
 		;;
 	--*)
-		__gitcomp_builtin difftool "$__git_diff_common_options
-					--base --cached --ours --theirs
-					--pickaxe-all --pickaxe-regex
-					--relative --staged
-					"
+		__gitcomp_builtin difftool "$__git_diff_difftool_options"
 		return
 		;;
 	esac
-- 
2.28.0.618.gf4bc123cb7


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v3 05/10] diff-lib: accept option flags in run_diff_index()
  2020-09-17  7:44   ` [PATCH v3 00/10] " Denton Liu
                       ` (3 preceding siblings ...)
  2020-09-17  7:44     ` [PATCH v3 04/10] contrib/completion: extract common diff/difftool options Denton Liu
@ 2020-09-17  7:44     ` Denton Liu
  2020-09-17 17:00       ` Junio C Hamano
  2020-09-17  7:44     ` [PATCH v3 06/10] diff-lib: define diff_get_merge_base() Denton Liu
                       ` (5 subsequent siblings)
  10 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-17  7:44 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

In a future commit, we will teach run_diff_index() to accept more
options via flag bits. For now, change `cached` into a flag in the
`option` bitfield. The behaviour should remain exactly the same.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/diff-index.c | 8 ++++----
 builtin/diff.c       | 8 ++++----
 diff-lib.c           | 6 +++---
 diff.h               | 3 ++-
 4 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 93ec642423..c3878f7ad6 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -15,7 +15,7 @@ COMMON_DIFF_OPTIONS_HELP;
 int cmd_diff_index(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info rev;
-	int cached = 0;
+	unsigned int option = 0;
 	int i;
 	int result;
 
@@ -32,7 +32,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 		const char *arg = argv[i];
 
 		if (!strcmp(arg, "--cached"))
-			cached = 1;
+			option |= DIFF_INDEX_CACHED;
 		else
 			usage(diff_cache_usage);
 	}
@@ -46,7 +46,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	if (rev.pending.nr != 1 ||
 	    rev.max_count != -1 || rev.min_age != -1 || rev.max_age != -1)
 		usage(diff_cache_usage);
-	if (!cached) {
+	if (!(option & DIFF_INDEX_CACHED)) {
 		setup_work_tree();
 		if (read_cache_preload(&rev.diffopt.pathspec) < 0) {
 			perror("read_cache_preload");
@@ -56,7 +56,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 		perror("read_cache");
 		return -1;
 	}
-	result = run_diff_index(&rev, cached);
+	result = run_diff_index(&rev, option);
 	UNLEAK(rev);
 	return diff_result_code(&rev.diffopt, result);
 }
diff --git a/builtin/diff.c b/builtin/diff.c
index cb98811c21..e45e19e37e 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -134,11 +134,11 @@ static int builtin_diff_blobs(struct rev_info *revs,
 static int builtin_diff_index(struct rev_info *revs,
 			      int argc, const char **argv)
 {
-	int cached = 0;
+	unsigned int option = 0;
 	while (1 < argc) {
 		const char *arg = argv[1];
 		if (!strcmp(arg, "--cached") || !strcmp(arg, "--staged"))
-			cached = 1;
+			option |= DIFF_INDEX_CACHED;
 		else
 			usage(builtin_diff_usage);
 		argv++; argc--;
@@ -151,7 +151,7 @@ static int builtin_diff_index(struct rev_info *revs,
 	    revs->max_count != -1 || revs->min_age != -1 ||
 	    revs->max_age != -1)
 		usage(builtin_diff_usage);
-	if (!cached) {
+	if (!(option & DIFF_INDEX_CACHED)) {
 		setup_work_tree();
 		if (read_cache_preload(&revs->diffopt.pathspec) < 0) {
 			perror("read_cache_preload");
@@ -161,7 +161,7 @@ static int builtin_diff_index(struct rev_info *revs,
 		perror("read_cache");
 		return -1;
 	}
-	return run_diff_index(revs, cached);
+	return run_diff_index(revs, option);
 }
 
 static int builtin_diff_tree(struct rev_info *revs,
diff --git a/diff-lib.c b/diff-lib.c
index 50521e2093..0a0e69113c 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -518,7 +518,7 @@ static int diff_cache(struct rev_info *revs,
 	return unpack_trees(1, &t, &opts);
 }
 
-int run_diff_index(struct rev_info *revs, int cached)
+int run_diff_index(struct rev_info *revs, unsigned int option)
 {
 	struct object_array_entry *ent;
 
@@ -527,10 +527,10 @@ int run_diff_index(struct rev_info *revs, int cached)
 
 	trace_performance_enter();
 	ent = revs->pending.objects;
-	if (diff_cache(revs, &ent->item->oid, ent->name, cached))
+	if (diff_cache(revs, &ent->item->oid, ent->name, !!(option & DIFF_INDEX_CACHED)))
 		exit(128);
 
-	diff_set_mnemonic_prefix(&revs->diffopt, "c/", cached ? "i/" : "w/");
+	diff_set_mnemonic_prefix(&revs->diffopt, "c/", (option & DIFF_INDEX_CACHED) ? "i/" : "w/");
 	diffcore_fix_diff_index();
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
diff --git a/diff.h b/diff.h
index e0c0af6286..0cc874f2d5 100644
--- a/diff.h
+++ b/diff.h
@@ -585,7 +585,8 @@ const char *diff_aligned_abbrev(const struct object_id *sha1, int);
 /* report racily-clean paths as modified */
 #define DIFF_RACY_IS_MODIFIED 02
 int run_diff_files(struct rev_info *revs, unsigned int option);
-int run_diff_index(struct rev_info *revs, int cached);
+#define DIFF_INDEX_CACHED 01
+int run_diff_index(struct rev_info *revs, unsigned int option);
 
 int do_diff_cache(const struct object_id *, struct diff_options *);
 int diff_flush_patch_id(struct diff_options *, struct object_id *, int, int);
-- 
2.28.0.618.gf4bc123cb7


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v3 06/10] diff-lib: define diff_get_merge_base()
  2020-09-17  7:44   ` [PATCH v3 00/10] " Denton Liu
                       ` (4 preceding siblings ...)
  2020-09-17  7:44     ` [PATCH v3 05/10] diff-lib: accept option flags in run_diff_index() Denton Liu
@ 2020-09-17  7:44     ` Denton Liu
  2020-09-17 17:16       ` Junio C Hamano
  2020-09-17  7:44     ` [PATCH v3 07/10] t4068: add --merge-base tests Denton Liu
                       ` (4 subsequent siblings)
  10 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-17  7:44 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

In a future commit, we will be using this function to implement
--merge-base functionality in various diff commands.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 diff-lib.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 diff.h     |  2 ++
 2 files changed, 50 insertions(+)

diff --git a/diff-lib.c b/diff-lib.c
index 0a0e69113c..e01c3f0612 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -13,6 +13,7 @@
 #include "submodule.h"
 #include "dir.h"
 #include "fsmonitor.h"
+#include "commit-reach.h"
 
 /*
  * diff-files
@@ -518,6 +519,53 @@ static int diff_cache(struct rev_info *revs,
 	return unpack_trees(1, &t, &opts);
 }
 
+void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
+{
+	int i;
+	struct commit *mb_child[2] = {0};
+	struct commit_list *merge_bases;
+
+	for (i = 0; i < revs->pending.nr; i++) {
+		struct object *obj = revs->pending.objects[i].item;
+		if (obj->flags)
+			die(_("--merge-base does not work with ranges"));
+		if (obj->type != OBJ_COMMIT)
+			die(_("--merge-base only works with commits"));
+	}
+
+	/*
+	 * This check must go after the for loop above because A...B
+	 * ranges produce three pending commits, resulting in a
+	 * misleading error message.
+	 */
+	if (revs->pending.nr > ARRAY_SIZE(mb_child))
+		die(_("--merge-base does not work with more than two commits"));
+
+	for (i = 0; i < revs->pending.nr; i++)
+		mb_child[i] = lookup_commit_reference(the_repository, &revs->pending.objects[i].item->oid);
+	if (revs->pending.nr < ARRAY_SIZE(mb_child)) {
+		struct object_id oid;
+
+		if (revs->pending.nr != 1)
+			BUG("unexpected revs->pending.nr: %d", revs->pending.nr);
+
+		if (get_oid("HEAD", &oid))
+			die(_("unable to get HEAD"));
+
+		mb_child[1] = lookup_commit_reference(the_repository, &oid);
+	}
+
+	merge_bases = repo_get_merge_bases(the_repository, mb_child[0], mb_child[1]);
+	if (!merge_bases)
+		die(_("no merge base found"));
+	if (merge_bases->next)
+		die(_("multiple merge bases found"));
+
+	oidcpy(mb, &merge_bases->item->object.oid);
+
+	free_commit_list(merge_bases);
+}
+
 int run_diff_index(struct rev_info *revs, unsigned int option)
 {
 	struct object_array_entry *ent;
diff --git a/diff.h b/diff.h
index 0cc874f2d5..ae2bb7001a 100644
--- a/diff.h
+++ b/diff.h
@@ -580,6 +580,8 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);
  */
 const char *diff_aligned_abbrev(const struct object_id *sha1, int);
 
+void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);
+
 /* do not report anything on removed paths */
 #define DIFF_SILENT_ON_REMOVED 01
 /* report racily-clean paths as modified */
-- 
2.28.0.618.gf4bc123cb7


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v3 07/10] t4068: add --merge-base tests
  2020-09-17  7:44   ` [PATCH v3 00/10] " Denton Liu
                       ` (5 preceding siblings ...)
  2020-09-17  7:44     ` [PATCH v3 06/10] diff-lib: define diff_get_merge_base() Denton Liu
@ 2020-09-17  7:44     ` Denton Liu
  2020-09-17  7:44     ` [PATCH v3 08/10] builtin/diff-index: learn --merge-base Denton Liu
                       ` (3 subsequent siblings)
  10 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-17  7:44 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

In the future, we will be adding more --merge-base tests to this test
script. To prepare for that, rename the script accordingly and update
its description. Also, add two basic --merge-base tests that don't
require any functionality to be implemented yet.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 ...ymmetric.sh => t4068-diff-symmetric-merge-base.sh} | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
 rename t/{t4068-diff-symmetric.sh => t4068-diff-symmetric-merge-base.sh} (86%)

diff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric-merge-base.sh
similarity index 86%
rename from t/t4068-diff-symmetric.sh
rename to t/t4068-diff-symmetric-merge-base.sh
index 60c506c2b2..bd4cf254d9 100755
--- a/t/t4068-diff-symmetric.sh
+++ b/t/t4068-diff-symmetric-merge-base.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='behavior of diff with symmetric-diff setups'
+test_description='behavior of diff with symmetric-diff setups and --merge-base'
 
 . ./test-lib.sh
 
@@ -88,4 +88,13 @@ test_expect_success 'diff with ranges and extra arg' '
 	test_i18ngrep "usage" err
 '
 
+test_expect_success 'diff --merge-base with no commits' '
+	test_must_fail git diff --merge-base
+'
+
+test_expect_success 'diff --merge-base with three commits' '
+	test_must_fail git diff --merge-base br1 br2 master 2>err &&
+	test_i18ngrep "usage" err
+'
+
 test_done
-- 
2.28.0.618.gf4bc123cb7


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v3 08/10] builtin/diff-index: learn --merge-base
  2020-09-17  7:44   ` [PATCH v3 00/10] " Denton Liu
                       ` (6 preceding siblings ...)
  2020-09-17  7:44     ` [PATCH v3 07/10] t4068: add --merge-base tests Denton Liu
@ 2020-09-17  7:44     ` Denton Liu
  2020-09-17 17:28       ` Junio C Hamano
  2020-09-17  7:44     ` [PATCH v3 09/10] builtin/diff-tree: " Denton Liu
                       ` (2 subsequent siblings)
  10 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-17  7:44 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

There is currently no easy way to take the diff between the working tree
or index and the merge base between an arbitrary commit and HEAD. Even
diff's `...` notation doesn't allow this because it only works between
commits. However, the ability to do this would be desirable to a user
who would like to see all the changes they've made on a branch plus
uncommitted changes without taking into account changes made in the
upstream branch.

Teach diff-index and diff (with one commit) the --merge-base option
which allows a user to use the merge base of a commit and HEAD as the
"before" side.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-diff-index.txt     |  7 +++-
 Documentation/git-diff.txt           | 12 ++++--
 builtin/diff-index.c                 |  2 +
 builtin/diff.c                       |  2 +
 diff-lib.c                           | 13 +++++-
 diff.h                               |  1 +
 t/t4068-diff-symmetric-merge-base.sh | 59 ++++++++++++++++++++++++++++
 7 files changed, 90 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-diff-index.txt b/Documentation/git-diff-index.txt
index 25fe165f00..27acb31cbf 100644
--- a/Documentation/git-diff-index.txt
+++ b/Documentation/git-diff-index.txt
@@ -9,7 +9,7 @@ git-diff-index - Compare a tree to the working tree or index
 SYNOPSIS
 --------
 [verse]
-'git diff-index' [-m] [--cached] [<common diff options>] <tree-ish> [<path>...]
+'git diff-index' [-m] [--cached] [--merge-base] [<common diff options>] <tree-ish> [<path>...]
 
 DESCRIPTION
 -----------
@@ -29,6 +29,11 @@ include::diff-options.txt[]
 --cached::
 	Do not consider the on-disk file at all.
 
+--merge-base::
+	Instead of comparing <tree-ish> directly, use the merge base
+	between <tree-ish> and HEAD instead.  <tree-ish> must be a
+	commit.
+
 -m::
 	By default, files recorded in the index but not checked
 	out are reported as deleted.  This flag makes
diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 8f7b4ed3ca..762ee6d074 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git diff' [<options>] [<commit>] [--] [<path>...]
-'git diff' [<options>] --cached [<commit>] [--] [<path>...]
+'git diff' [<options>] --cached [--merge-base] [<commit>] [--] [<path>...]
 'git diff' [<options>] <commit> [<commit>...] <commit> [--] [<path>...]
 'git diff' [<options>] <commit>...<commit> [--] [<path>...]
 'git diff' [<options>] <blob> <blob>
@@ -40,7 +40,7 @@ files on disk.
 	or when running the command outside a working tree
 	controlled by Git. This form implies `--exit-code`.
 
-'git diff' [<options>] --cached [<commit>] [--] [<path>...]::
+'git diff' [<options>] --cached [--merge-base] [<commit>] [--] [<path>...]::
 
 	This form is to view the changes you staged for the next
 	commit relative to the named <commit>.  Typically you
@@ -49,6 +49,10 @@ files on disk.
 	If HEAD does not exist (e.g. unborn branches) and
 	<commit> is not given, it shows all staged changes.
 	--staged is a synonym of --cached.
++
+If --merge-base is given, instead of using <commit>, use the merge base
+of <commit> and HEAD.  `git diff --merge-base A` is equivalent to
+`git diff $(git merge-base A HEAD)`.
 
 'git diff' [<options>] <commit> [--] [<path>...]::
 
@@ -89,8 +93,8 @@ files on disk.
 
 Just in case you are doing something exotic, it should be
 noted that all of the <commit> in the above description, except
-in the last two forms that use `..` notations, can be any
-<tree>.
+in the `--merge-base` case and in the last two forms that use `..`
+notations, can be any <tree>.
 
 For a more complete list of ways to spell <commit>, see
 "SPECIFYING REVISIONS" section in linkgit:gitrevisions[7].
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index c3878f7ad6..7f5281c461 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -33,6 +33,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 
 		if (!strcmp(arg, "--cached"))
 			option |= DIFF_INDEX_CACHED;
+		else if (!strcmp(arg, "--merge-base"))
+			option |= DIFF_INDEX_MERGE_BASE;
 		else
 			usage(diff_cache_usage);
 	}
diff --git a/builtin/diff.c b/builtin/diff.c
index e45e19e37e..1baea18ae0 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -139,6 +139,8 @@ static int builtin_diff_index(struct rev_info *revs,
 		const char *arg = argv[1];
 		if (!strcmp(arg, "--cached") || !strcmp(arg, "--staged"))
 			option |= DIFF_INDEX_CACHED;
+		else if (!strcmp(arg, "--merge-base"))
+			option |= DIFF_INDEX_MERGE_BASE;
 		else
 			usage(builtin_diff_usage);
 		argv++; argc--;
diff --git a/diff-lib.c b/diff-lib.c
index e01c3f0612..68bf86f289 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -569,13 +569,24 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
 int run_diff_index(struct rev_info *revs, unsigned int option)
 {
 	struct object_array_entry *ent;
+	struct object_id oid;
+	const char *name;
 
 	if (revs->pending.nr != 1)
 		BUG("run_diff_index must be passed exactly one tree");
 
 	trace_performance_enter();
 	ent = revs->pending.objects;
-	if (diff_cache(revs, &ent->item->oid, ent->name, !!(option & DIFF_INDEX_CACHED)))
+
+	if (option & DIFF_INDEX_MERGE_BASE) {
+		diff_get_merge_base(revs, &oid);
+		name = xstrdup(oid_to_hex(&oid));
+	} else {
+		oidcpy(&oid, &ent->item->oid);
+		name = ent->name;
+	}
+
+	if (diff_cache(revs, &oid, name, !!(option & DIFF_INDEX_CACHED)))
 		exit(128);
 
 	diff_set_mnemonic_prefix(&revs->diffopt, "c/", (option & DIFF_INDEX_CACHED) ? "i/" : "w/");
diff --git a/diff.h b/diff.h
index ae2bb7001a..0485786b68 100644
--- a/diff.h
+++ b/diff.h
@@ -588,6 +588,7 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);
 #define DIFF_RACY_IS_MODIFIED 02
 int run_diff_files(struct rev_info *revs, unsigned int option);
 #define DIFF_INDEX_CACHED 01
+#define DIFF_INDEX_MERGE_BASE 02
 int run_diff_index(struct rev_info *revs, unsigned int option);
 
 int do_diff_cache(const struct object_id *, struct diff_options *);
diff --git a/t/t4068-diff-symmetric-merge-base.sh b/t/t4068-diff-symmetric-merge-base.sh
index bd4cf254d9..49432379cb 100755
--- a/t/t4068-diff-symmetric-merge-base.sh
+++ b/t/t4068-diff-symmetric-merge-base.sh
@@ -97,4 +97,63 @@ test_expect_success 'diff --merge-base with three commits' '
 	test_i18ngrep "usage" err
 '
 
+for cmd in diff-index diff
+do
+	test_expect_success "$cmd --merge-base with one commit" '
+		git checkout master &&
+		git $cmd commit-C >expect &&
+		git $cmd --merge-base br2 >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "$cmd --merge-base with one commit and unstaged changes" '
+		git checkout master &&
+		test_when_finished git reset --hard &&
+		echo unstaged >>c &&
+		git $cmd commit-C >expect &&
+		git $cmd --merge-base br2 >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "$cmd --merge-base with one commit and staged and unstaged changes" '
+		git checkout master &&
+		test_when_finished git reset --hard &&
+		echo staged >>c &&
+		git add c &&
+		echo unstaged >>c &&
+		git $cmd commit-C >expect &&
+		git $cmd --merge-base br2 >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "$cmd --merge-base --cached with one commit and staged and unstaged changes" '
+		git checkout master &&
+		test_when_finished git reset --hard &&
+		echo staged >>c &&
+		git add c &&
+		echo unstaged >>c &&
+		git $cmd --cached commit-C >expect &&
+		git $cmd --cached --merge-base br2 >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "$cmd --merge-base with non-commit" '
+		git checkout master &&
+		test_must_fail git $cmd --merge-base master^{tree} 2>err &&
+		test_i18ngrep "fatal: --merge-base only works with commits" err
+	'
+
+	test_expect_success "$cmd --merge-base with no merge bases and one commit" '
+		git checkout master &&
+		test_must_fail git $cmd --merge-base br3 2>err &&
+		test_i18ngrep "fatal: no merge base found" err
+	'
+
+	test_expect_success "$cmd --merge-base with multiple merge bases and one commit" '
+		git checkout master &&
+		test_must_fail git $cmd --merge-base br1 2>err &&
+		test_i18ngrep "fatal: multiple merge bases found" err
+	'
+done
+
 test_done
-- 
2.28.0.618.gf4bc123cb7


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
  2020-09-17  7:44   ` [PATCH v3 00/10] " Denton Liu
                       ` (7 preceding siblings ...)
  2020-09-17  7:44     ` [PATCH v3 08/10] builtin/diff-index: learn --merge-base Denton Liu
@ 2020-09-17  7:44     ` Denton Liu
  2020-09-17 18:23       ` Junio C Hamano
  2020-09-17  7:44     ` [PATCH v3 10/10] contrib/completion: complete `git diff --merge-base` Denton Liu
  2020-09-20 11:22     ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
  10 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-17  7:44 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

In order to get the diff between a commit and its merge base, the
currently preferred method is to use `git diff A...B`. However, the
range-notation with diff has, time and time again, been noted as a point
of confusion and thus, it should be avoided. Although we have a
substitute for the double-dot notation, we don't have any replacement
for the triple-dot notation.

Introduce the --merge-base flag as a replacement for triple-dot
notation. Thus, we would be able to write the above as
`git diff --merge-base A B`, allowing us to gently deprecate
range-notation completely.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-diff-tree.txt      |  7 ++++-
 Documentation/git-diff.txt           |  9 ++++---
 builtin/diff-tree.c                  | 18 +++++++++++++
 builtin/diff.c                       | 40 +++++++++++++++++++---------
 t/t4068-diff-symmetric-merge-base.sh | 34 +++++++++++++++++++++++
 5 files changed, 91 insertions(+), 17 deletions(-)

diff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt
index 5c8a2a5e97..2fc24c542f 100644
--- a/Documentation/git-diff-tree.txt
+++ b/Documentation/git-diff-tree.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git diff-tree' [--stdin] [-m] [-s] [-v] [--no-commit-id] [--pretty]
-	      [-t] [-r] [-c | --cc] [--combined-all-paths] [--root]
+	      [-t] [-r] [-c | --cc] [--combined-all-paths] [--root] [--merge-base]
 	      [<common diff options>] <tree-ish> [<tree-ish>] [<path>...]
 
 DESCRIPTION
@@ -43,6 +43,11 @@ include::diff-options.txt[]
 	When `--root` is specified the initial commit will be shown as a big
 	creation event. This is equivalent to a diff against the NULL tree.
 
+--merge-base::
+	Instead of comparing the <tree-ish>s directly, use the merge
+	base between the two <tree-ish>s as the "before" side.  There
+	must be two <tree-ish>s given and they must both be commits.
+
 --stdin::
 	When `--stdin` is specified, the command does not take
 	<tree-ish> arguments from the command line.  Instead, it
diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 762ee6d074..d3b526e00a 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -11,8 +11,7 @@ SYNOPSIS
 [verse]
 'git diff' [<options>] [<commit>] [--] [<path>...]
 'git diff' [<options>] --cached [--merge-base] [<commit>] [--] [<path>...]
-'git diff' [<options>] <commit> [<commit>...] <commit> [--] [<path>...]
-'git diff' [<options>] <commit>...<commit> [--] [<path>...]
+'git diff' [<options>] [--merge-base] <commit> [<commit>...] <commit> [--] [<path>...]
 'git diff' [<options>] <blob> <blob>
 'git diff' [<options>] --no-index [--] <path> <path>
 
@@ -62,10 +61,14 @@ of <commit> and HEAD.  `git diff --merge-base A` is equivalent to
 	branch name to compare with the tip of a different
 	branch.
 
-'git diff' [<options>] <commit> <commit> [--] [<path>...]::
+'git diff' [<options>] [--merge-base] <commit> <commit> [--] [<path>...]::
 
 	This is to view the changes between two arbitrary
 	<commit>.
++
+If --merge-base is given, use the merge base of the two commits for the
+"before" side.  `git diff --merge-base A B` is equivalent to
+`git diff $(git merge-base A B) B`.
 
 'git diff' [<options>] <commit> <commit>... <commit> [--] [<path>...]::
 
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 802363d0a2..823d6678e5 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -111,6 +111,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	struct setup_revision_opt s_r_opt;
 	struct userformat_want w;
 	int read_stdin = 0;
+	int merge_base = 0;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage(diff_tree_usage);
@@ -143,9 +144,26 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 			read_stdin = 1;
 			continue;
 		}
+		if (!strcmp(arg, "--merge-base")) {
+			merge_base = 1;
+			continue;
+		}
 		usage(diff_tree_usage);
 	}
 
+	if (read_stdin && merge_base)
+		die(_("--stdin and --merge-base are mutually exclusive"));
+
+	if (merge_base) {
+		struct object_id oid;
+
+		if (opt->pending.nr != 2)
+			die(_("--merge-base only works with two commits"));
+
+		diff_get_merge_base(opt, &oid);
+		opt->pending.objects[0].item = lookup_object(the_repository, &oid);
+	}
+
 	/*
 	 * NOTE!  We expect "a..b" to expand to "^a b" but it is
 	 * perfectly valid for revision range parser to yield "b ^a",
diff --git a/builtin/diff.c b/builtin/diff.c
index 1baea18ae0..ad78bc89b3 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -26,8 +26,7 @@
 static const char builtin_diff_usage[] =
 "git diff [<options>] [<commit>] [--] [<path>...]\n"
 "   or: git diff [<options>] --cached [<commit>] [--] [<path>...]\n"
-"   or: git diff [<options>] <commit> [<commit>...] <commit> [--] [<path>...]\n"
-"   or: git diff [<options>] <commit>...<commit>] [--] [<path>...]\n"
+"   or: git diff [<options>] <commit> [--merge-base] [<commit>...] <commit> [--] [<path>...]\n"
 "   or: git diff [<options>] <blob> <blob>]\n"
 "   or: git diff [<options>] --no-index [--] <path> <path>]\n"
 COMMON_DIFF_OPTIONS_HELP;
@@ -172,19 +171,34 @@ static int builtin_diff_tree(struct rev_info *revs,
 			     struct object_array_entry *ent1)
 {
 	const struct object_id *(oid[2]);
-	int swap = 0;
+	struct object_id mb_oid;
+	int merge_base = 0;
 
-	if (argc > 1)
-		usage(builtin_diff_usage);
+	while (1 < argc) {
+		const char *arg = argv[1];
+		if (!strcmp(arg, "--merge-base"))
+			merge_base = 1;
+		else
+			usage(builtin_diff_usage);
+		argv++; argc--;
+	}
 
-	/*
-	 * We saw two trees, ent0 and ent1.  If ent1 is uninteresting,
-	 * swap them.
-	 */
-	if (ent1->item->flags & UNINTERESTING)
-		swap = 1;
-	oid[swap] = &ent0->item->oid;
-	oid[1 - swap] = &ent1->item->oid;
+	if (merge_base) {
+		diff_get_merge_base(revs, &mb_oid);
+		oid[0] = &mb_oid;
+		oid[1] = &revs->pending.objects[1].item->oid;
+	} else {
+		int swap = 0;
+
+		/*
+		 * We saw two trees, ent0 and ent1.  If ent1 is uninteresting,
+		 * swap them.
+		 */
+		if (ent1->item->flags & UNINTERESTING)
+			swap = 1;
+		oid[swap] = &ent0->item->oid;
+		oid[1 - swap] = &ent1->item->oid;
+	}
 	diff_tree_oid(oid[0], oid[1], "", &revs->diffopt);
 	log_tree_diff_flush(revs);
 	return 0;
diff --git a/t/t4068-diff-symmetric-merge-base.sh b/t/t4068-diff-symmetric-merge-base.sh
index 49432379cb..03487cc945 100755
--- a/t/t4068-diff-symmetric-merge-base.sh
+++ b/t/t4068-diff-symmetric-merge-base.sh
@@ -156,4 +156,38 @@ do
 	'
 done
 
+for cmd in diff-tree diff
+do
+	test_expect_success "$cmd --merge-base with two commits" '
+		git $cmd commit-C master >expect &&
+		git $cmd --merge-base br2 master >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "$cmd --merge-base commit and non-commit" '
+		test_must_fail git $cmd --merge-base br2 master^{tree} 2>err &&
+		test_i18ngrep "fatal: --merge-base only works with commits" err
+	'
+
+	test_expect_success "$cmd --merge-base with no merge bases and two commits" '
+		test_must_fail git $cmd --merge-base br2 br3 2>err &&
+		test_i18ngrep "fatal: no merge base found" err
+	'
+
+	test_expect_success "$cmd --merge-base with multiple merge bases and two commits" '
+		test_must_fail git $cmd --merge-base master br1 2>err &&
+		test_i18ngrep "fatal: multiple merge bases found" err
+	'
+done
+
+test_expect_success 'diff-tree --merge-base with one commit' '
+	test_must_fail git diff-tree --merge-base master 2>err &&
+	test_i18ngrep "fatal: --merge-base only works with two commits" err
+'
+
+test_expect_success 'diff --merge-base with range' '
+	test_must_fail git diff --merge-base br2..br3 2>err &&
+	test_i18ngrep "fatal: --merge-base does not work with ranges" err
+'
+
 test_done
-- 
2.28.0.618.gf4bc123cb7


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v3 10/10] contrib/completion: complete `git diff --merge-base`
  2020-09-17  7:44   ` [PATCH v3 00/10] " Denton Liu
                       ` (8 preceding siblings ...)
  2020-09-17  7:44     ` [PATCH v3 09/10] builtin/diff-tree: " Denton Liu
@ 2020-09-17  7:44     ` Denton Liu
  2020-09-20 11:22     ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
  10 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-17  7:44 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f68c8e0646..679d1ec8a8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1692,7 +1692,7 @@ __git_diff_common_options="--stat --numstat --shortstat --summary
 "
 
 __git_diff_difftool_options="--cached --staged --pickaxe-all --pickaxe-regex
-			--base --ours --theirs --no-index --relative
+			--base --ours --theirs --no-index --relative --merge-base
 			$__git_diff_common_options"
 
 _git_diff ()
-- 
2.28.0.618.gf4bc123cb7


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 05/10] diff-lib: accept option flags in run_diff_index()
  2020-09-17  7:44     ` [PATCH v3 05/10] diff-lib: accept option flags in run_diff_index() Denton Liu
@ 2020-09-17 17:00       ` Junio C Hamano
  0 siblings, 0 replies; 58+ messages in thread
From: Junio C Hamano @ 2020-09-17 17:00 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

Denton Liu <liu.denton@gmail.com> writes:

> diff --git a/diff-lib.c b/diff-lib.c
> index 50521e2093..0a0e69113c 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -518,7 +518,7 @@ static int diff_cache(struct rev_info *revs,
>  	return unpack_trees(1, &t, &opts);
>  }
>  
> -int run_diff_index(struct rev_info *revs, int cached)
> +int run_diff_index(struct rev_info *revs, unsigned int option)
>  {
>  	struct object_array_entry *ent;

If we introduce

	int cached = !!(option & DIFF_INDEX_CACHED);

we do not have to touch the remainder of the function, and it makes
it easier to read the place(s) where 'cached' is used.  At that
point, the fact that we were instructed by a bit in the option flag
word is much much less important than we were instructed to compare
the tree-ish with the index and not the working tree files.

The same technique is used with DIFF_RACY_IS_MODIFIED flag in
run_diff_files().

> diff --git a/diff.h b/diff.h
> index e0c0af6286..0cc874f2d5 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -585,7 +585,8 @@ const char *diff_aligned_abbrev(const struct object_id *sha1, int);
>  /* report racily-clean paths as modified */
>  #define DIFF_RACY_IS_MODIFIED 02
>  int run_diff_files(struct rev_info *revs, unsigned int option);
> -int run_diff_index(struct rev_info *revs, int cached);
> +#define DIFF_INDEX_CACHED 01
> +int run_diff_index(struct rev_info *revs, unsigned int option);

It is unclear from the above if the option word for run_diff_files()
and run_diff_index() share the same bit assignment.  After reading
the series through to the end, I know they are independent set of
bits and never shared, but I wonder if we can make it more obvious
here.

Perhaps an extra blank line before this new #define is sufficient to
make it clear?

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 06/10] diff-lib: define diff_get_merge_base()
  2020-09-17  7:44     ` [PATCH v3 06/10] diff-lib: define diff_get_merge_base() Denton Liu
@ 2020-09-17 17:16       ` Junio C Hamano
  2020-09-18 10:34         ` Denton Liu
  0 siblings, 1 reply; 58+ messages in thread
From: Junio C Hamano @ 2020-09-17 17:16 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

Denton Liu <liu.denton@gmail.com> writes:

> +void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
> +{
> +	int i;
> +	struct commit *mb_child[2] = {0};
> +	struct commit_list *merge_bases;
> +
> +	for (i = 0; i < revs->pending.nr; i++) {
> +		struct object *obj = revs->pending.objects[i].item;
> +		if (obj->flags)
> +			die(_("--merge-base does not work with ranges"));
> +		if (obj->type != OBJ_COMMIT)
> +			die(_("--merge-base only works with commits"));
> +	}

This is the first use of die() in this file, that is designed to
keep a set of reusable library functions so that the caller(s) can
do their own die().  They may want to become

	return error(_(...));

The same comment applies to all die()s the patch adds.

> +	/*
> +	 * This check must go after the for loop above because A...B
> +	 * ranges produce three pending commits, resulting in a
> +	 * misleading error message.
> +	 */

Should "git diff --merge-base A...B" be forbidden, or does it just
ask the same thing twice and is not a die-worthy offence?

> +	if (revs->pending.nr > ARRAY_SIZE(mb_child))
> +		die(_("--merge-base does not work with more than two commits"));

Compare with hardcoded '2' in the condition.  The requirement for
mb_child[] is that it can hold at least 2 (changes in the future may
find it convenient if its size gets increased to 3 to hold NULL sentinel
to signal end-of-list, for example).   Comparison with ARRAY_SIZE() may
be appropriate in different situations but not here where the code knows
it wants to reject more than two no matter how big mb_child[] is.

> +	for (i = 0; i < revs->pending.nr; i++)
> +		mb_child[i] = lookup_commit_reference(the_repository, &revs->pending.objects[i].item->oid);
> +	if (revs->pending.nr < ARRAY_SIZE(mb_child)) {
> +		struct object_id oid;
> +
> +		if (revs->pending.nr != 1)
> +			BUG("unexpected revs->pending.nr: %d", revs->pending.nr);

This is an obviously impossible condition as we will not take more
than 2.

> +		if (get_oid("HEAD", &oid))
> +			die(_("unable to get HEAD"));
> +		mb_child[1] = lookup_commit_reference(the_repository, &oid);
> +	}
> +
> +	merge_bases = repo_get_merge_bases(the_repository, mb_child[0], mb_child[1]);
> +	if (!merge_bases)
> +		die(_("no merge base found"));
> +	if (merge_bases->next)
> +		die(_("multiple merge bases found"));
> +
> +	oidcpy(mb, &merge_bases->item->object.oid);
> +
> +	free_commit_list(merge_bases);
> +}


OK.

>  int run_diff_index(struct rev_info *revs, unsigned int option)
>  {
>  	struct object_array_entry *ent;
> diff --git a/diff.h b/diff.h
> index 0cc874f2d5..ae2bb7001a 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -580,6 +580,8 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);
>   */
>  const char *diff_aligned_abbrev(const struct object_id *sha1, int);
>  
> +void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);

Without an actual caller, we cannot judge if this interface is
designed right, but we'll see soon in the next steps ;-)

Looking good so far.

Thanks.

> +
>  /* do not report anything on removed paths */
>  #define DIFF_SILENT_ON_REMOVED 01
>  /* report racily-clean paths as modified */

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 08/10] builtin/diff-index: learn --merge-base
  2020-09-17  7:44     ` [PATCH v3 08/10] builtin/diff-index: learn --merge-base Denton Liu
@ 2020-09-17 17:28       ` Junio C Hamano
  2020-09-17 18:13         ` Jeff King
  0 siblings, 1 reply; 58+ messages in thread
From: Junio C Hamano @ 2020-09-17 17:28 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

Denton Liu <liu.denton@gmail.com> writes:

> diff --git a/builtin/diff-index.c b/builtin/diff-index.c
> index c3878f7ad6..7f5281c461 100644
> --- a/builtin/diff-index.c
> +++ b/builtin/diff-index.c
> @@ -33,6 +33,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
>  
>  		if (!strcmp(arg, "--cached"))
>  			option |= DIFF_INDEX_CACHED;
> +		else if (!strcmp(arg, "--merge-base"))
> +			option |= DIFF_INDEX_MERGE_BASE;
>  		else
>  			usage(diff_cache_usage);
>  	}
> diff --git a/builtin/diff.c b/builtin/diff.c
> index e45e19e37e..1baea18ae0 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -139,6 +139,8 @@ static int builtin_diff_index(struct rev_info *revs,
>  		const char *arg = argv[1];
>  		if (!strcmp(arg, "--cached") || !strcmp(arg, "--staged"))
>  			option |= DIFF_INDEX_CACHED;
> +		else if (!strcmp(arg, "--merge-base"))
> +			option |= DIFF_INDEX_MERGE_BASE;
>  		else
>  			usage(builtin_diff_usage);
>  		argv++; argc--;

OK.

> diff --git a/diff-lib.c b/diff-lib.c
> index e01c3f0612..68bf86f289 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -569,13 +569,24 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
>  int run_diff_index(struct rev_info *revs, unsigned int option)
>  {
>  	struct object_array_entry *ent;
> +	struct object_id oid;
> +	const char *name;

Let's do the same

	int compare_with_merge_base = !!(option & DIFF_INDEX_MERGE_BASE);

here to keep the result easier to follow.

>  	if (revs->pending.nr != 1)
>  		BUG("run_diff_index must be passed exactly one tree");
>  
>  	trace_performance_enter();
>  	ent = revs->pending.objects;
> -	if (diff_cache(revs, &ent->item->oid, ent->name, !!(option & DIFF_INDEX_CACHED)))
> +
> +	if (option & DIFF_INDEX_MERGE_BASE) {
> +		diff_get_merge_base(revs, &oid);
> +		name = xstrdup(oid_to_hex(&oid));

Leak?

> +	} else {
> +		oidcpy(&oid, &ent->item->oid);
> +		name = ent->name;
> +	}
> +
> +	if (diff_cache(revs, &oid, name, !!(option & DIFF_INDEX_CACHED)))
>  		exit(128);
>  
>  	diff_set_mnemonic_prefix(&revs->diffopt, "c/", (option & DIFF_INDEX_CACHED) ? "i/" : "w/");

> +for cmd in diff-index diff
> +do
> +	test_expect_success "$cmd --merge-base with one commit" '
> +		git checkout master &&
> +		git $cmd commit-C >expect &&
> +		git $cmd --merge-base br2 >actual &&
> +		test_cmp expect actual
> +	'

OK, the same command, when comparing with commit-C and with
"--merge-base br2" that should compute the same commit-C, should
give the same answer.  Good testing strategy.

Thanks.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 08/10] builtin/diff-index: learn --merge-base
  2020-09-17 17:28       ` Junio C Hamano
@ 2020-09-17 18:13         ` Jeff King
  2020-09-18  5:11           ` Junio C Hamano
  0 siblings, 1 reply; 58+ messages in thread
From: Jeff King @ 2020-09-17 18:13 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Denton Liu, Git Mailing List

On Thu, Sep 17, 2020 at 10:28:53AM -0700, Junio C Hamano wrote:

> > -	if (diff_cache(revs, &ent->item->oid, ent->name, !!(option & DIFF_INDEX_CACHED)))
> > +
> > +	if (option & DIFF_INDEX_MERGE_BASE) {
> > +		diff_get_merge_base(revs, &oid);
> > +		name = xstrdup(oid_to_hex(&oid));
> 
> Leak?

Using oid_to_hex_r() avoids an extra copy, and the leak goes away too:

  char merge_base_hex[GIT_MAX_HEXSZ + 1];
  ...
  name = oid_to_hex_r(merge_base_hex, &oid);

-Peff

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
  2020-09-17  7:44     ` [PATCH v3 09/10] builtin/diff-tree: " Denton Liu
@ 2020-09-17 18:23       ` Junio C Hamano
  2020-09-18 10:48         ` Denton Liu
  0 siblings, 1 reply; 58+ messages in thread
From: Junio C Hamano @ 2020-09-17 18:23 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

Denton Liu <liu.denton@gmail.com> writes:

> +	if (read_stdin && merge_base)
> +		die(_("--stdin and --merge-base are mutually exclusive"));
> +
> +	if (merge_base) {
> +		struct object_id oid;
> +
> +		if (opt->pending.nr != 2)
> +			die(_("--merge-base only works with two commits"));
> +
> +		diff_get_merge_base(opt, &oid);
> +		opt->pending.objects[0].item = lookup_object(the_repository, &oid);
> +	}
> +

This looks quite straight-forward.

> -	/*
> -	 * We saw two trees, ent0 and ent1.  If ent1 is uninteresting,
> -	 * swap them.
> -	 */
> -	if (ent1->item->flags & UNINTERESTING)
> -		swap = 1;
> -	oid[swap] = &ent0->item->oid;
> -	oid[1 - swap] = &ent1->item->oid;
> +	if (merge_base) {
> +		diff_get_merge_base(revs, &mb_oid);
> +		oid[0] = &mb_oid;
> +		oid[1] = &revs->pending.objects[1].item->oid;
> +	} else {
> +		int swap = 0;
> +
> +		/*
> +		 * We saw two trees, ent0 and ent1.  If ent1 is uninteresting,
> +		 * swap them.
> +		 */
> +		if (ent1->item->flags & UNINTERESTING)
> +			swap = 1;
> +		oid[swap] = &ent0->item->oid;
> +		oid[1 - swap] = &ent1->item->oid;
> +	}

It is not entirely clear why the original has to become an [else]
clause here, unlike the change we saw earlier in cmd_diff_tree().
It feels quite inconsistent.

Thanks.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 08/10] builtin/diff-index: learn --merge-base
  2020-09-17 18:13         ` Jeff King
@ 2020-09-18  5:11           ` Junio C Hamano
  2020-09-18 18:12             ` Jeff King
  0 siblings, 1 reply; 58+ messages in thread
From: Junio C Hamano @ 2020-09-18  5:11 UTC (permalink / raw)
  To: Jeff King; +Cc: Denton Liu, Git Mailing List

Jeff King <peff@peff.net> writes:

> On Thu, Sep 17, 2020 at 10:28:53AM -0700, Junio C Hamano wrote:
>
>> > -	if (diff_cache(revs, &ent->item->oid, ent->name, !!(option & DIFF_INDEX_CACHED)))
>> > +
>> > +	if (option & DIFF_INDEX_MERGE_BASE) {
>> > +		diff_get_merge_base(revs, &oid);
>> > +		name = xstrdup(oid_to_hex(&oid));
>> 
>> Leak?
>
> Using oid_to_hex_r() avoids an extra copy, and the leak goes away too:
>
>   char merge_base_hex[GIT_MAX_HEXSZ + 1];
>   ...
>   name = oid_to_hex_r(merge_base_hex, &oid);
>
> -Peff

Yes, I was debating myself if I should mention it or trust/assume
that the contributor can easily figure it out.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 06/10] diff-lib: define diff_get_merge_base()
  2020-09-17 17:16       ` Junio C Hamano
@ 2020-09-18 10:34         ` Denton Liu
  2020-09-19  0:33           ` Junio C Hamano
  0 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-18 10:34 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List

Hi Junio,

On Thu, Sep 17, 2020 at 10:16:51AM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > +void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
> > +{
> > +	int i;
> > +	struct commit *mb_child[2] = {0};
> > +	struct commit_list *merge_bases;
> > +
> > +	for (i = 0; i < revs->pending.nr; i++) {
> > +		struct object *obj = revs->pending.objects[i].item;
> > +		if (obj->flags)
> > +			die(_("--merge-base does not work with ranges"));
> > +		if (obj->type != OBJ_COMMIT)
> > +			die(_("--merge-base only works with commits"));
> > +	}
> 
> This is the first use of die() in this file, that is designed to
> keep a set of reusable library functions so that the caller(s) can
> do their own die().  They may want to become

Although this is the first instance of die(), run_diff_index() has an
exit(128), which is basically a die() in disguise.

> 	return error(_(...));
> 
> The same comment applies to all die()s the patch adds.

I applied this change but then each callsite of diff_get_merge_base()
became something like

	if (diff_get_merge_base(revs, &oid))
		exit(128);

so I do agree with the spirit of the change but in reality, it just
creates more busywork for the callers.

> > +	/*
> > +	 * This check must go after the for loop above because A...B
> > +	 * ranges produce three pending commits, resulting in a
> > +	 * misleading error message.
> > +	 */
> 
> Should "git diff --merge-base A...B" be forbidden, or does it just
> ask the same thing twice and is not a die-worthy offence?

I think that it should be die-worthy because it's a logic error for a
user to do this. I can't think of any situation where it wouldn't be
more desirable error early to correct a user's thinking. Plus, we're
trying to move away from the `...` notation anyway ;)

> > +	for (i = 0; i < revs->pending.nr; i++)
> > +		mb_child[i] = lookup_commit_reference(the_repository, &revs->pending.objects[i].item->oid);
> > +	if (revs->pending.nr < ARRAY_SIZE(mb_child)) {
> > +		struct object_id oid;
> > +
> > +		if (revs->pending.nr != 1)
> > +			BUG("unexpected revs->pending.nr: %d", revs->pending.nr);
> 
> This is an obviously impossible condition as we will not take more
> than 2.

We also want to ensure that revs->pending.nr isn't 0 here. That being
said, I can explicitly check earlier in the function that the number of
pending is 1 or 2 so that it's more clearly written.

Thanks,
Denton

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
  2020-09-17 18:23       ` Junio C Hamano
@ 2020-09-18 10:48         ` Denton Liu
  2020-09-18 16:52           ` Junio C Hamano
  0 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-18 10:48 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List

On Thu, Sep 17, 2020 at 11:23:54AM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > +	if (read_stdin && merge_base)
> > +		die(_("--stdin and --merge-base are mutually exclusive"));
> > +
> > +	if (merge_base) {
> > +		struct object_id oid;
> > +
> > +		if (opt->pending.nr != 2)
> > +			die(_("--merge-base only works with two commits"));
> > +
> > +		diff_get_merge_base(opt, &oid);
> > +		opt->pending.objects[0].item = lookup_object(the_repository, &oid);
> > +	}
> > +
> 
> This looks quite straight-forward.
> 
> > -	/*
> > -	 * We saw two trees, ent0 and ent1.  If ent1 is uninteresting,
> > -	 * swap them.
> > -	 */
> > -	if (ent1->item->flags & UNINTERESTING)
> > -		swap = 1;
> > -	oid[swap] = &ent0->item->oid;
> > -	oid[1 - swap] = &ent1->item->oid;
> > +	if (merge_base) {
> > +		diff_get_merge_base(revs, &mb_oid);
> > +		oid[0] = &mb_oid;
> > +		oid[1] = &revs->pending.objects[1].item->oid;
> > +	} else {
> > +		int swap = 0;
> > +
> > +		/*
> > +		 * We saw two trees, ent0 and ent1.  If ent1 is uninteresting,
> > +		 * swap them.
> > +		 */
> > +		if (ent1->item->flags & UNINTERESTING)
> > +			swap = 1;
> > +		oid[swap] = &ent0->item->oid;
> > +		oid[1 - swap] = &ent1->item->oid;
> > +	}
> 
> It is not entirely clear why the original has to become an [else]
> clause here, unlike the change we saw earlier in cmd_diff_tree().
> It feels quite inconsistent.

Since we're only interested in the oids, I thought that it would be
possible to save a lookup_object() and just use the oids directly. If
it's clearer, this can be written as something like this but the lookup
feels unnecessary:

	/*
	 * We saw two trees, ent0 and ent1.  If ent1 is uninteresting,
	 * swap them.
	 */
	if (ent1->item->flags & UNINTERESTING)
		swap = 1;

	if (merge_base) {
		struct object_id mb_oid;
		if (swap)
			BUG("swap is unexpectedly set");
		if (diff_get_merge_base(revs, &mb_oid))
			exit(128);
		ent0->item = lookup_object(the_repository, &mb_oid);
	}


	oid[swap] = &ent0->item->oid;
	oid[1 - swap] = &ent1->item->oid;

Thanks,
Denton

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
  2020-09-18 10:48         ` Denton Liu
@ 2020-09-18 16:52           ` Junio C Hamano
  2020-09-20 11:01             ` Denton Liu
  0 siblings, 1 reply; 58+ messages in thread
From: Junio C Hamano @ 2020-09-18 16:52 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

Denton Liu <liu.denton@gmail.com> writes:

> Since we're only interested in the oids, I thought that it would be
> possible to save a lookup_object() and just use the oids directly. If
> it's clearer, this can be written as something like this but the lookup
> feels unnecessary:

When running the tree diff, we'd need the object anyway, and the
result of the look-up made here is cached, right?

That is why I expected it would just be an insertion before the
existing code, like the other side.

But the existing "if we got either ^A B or B ^A, treat it as A..B"
logic is just like "if we got '--merge-base A B', treat it as
something else" we are adding, and they (and any future such special
syntax) should not interact with each other.  So in that sense, the
code structure you have in the originally posted patch (not the code
snippet in your message I am responding to) that does

    ...
    if (using merge-base feature) {
	do the merge base thing to populate oid[]
    } else if (user used A..B) {
	ensure "^A B" and "B ^A" both have A in oid[0] and B in oid[1]
    }
    ...
    call diff-tree between oid[0] and oid[1]

makes a lot more sense than anything else we discussed so far.

I wonder if turning the builtin/diff-tree.c to match that structure
make the result easier to understand (and I'll be perfectly happy if
the answer to this question turns out to be "no, the result of the
posted patch is the easiest to follow").

Thanks.


^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 08/10] builtin/diff-index: learn --merge-base
  2020-09-18  5:11           ` Junio C Hamano
@ 2020-09-18 18:12             ` Jeff King
  0 siblings, 0 replies; 58+ messages in thread
From: Jeff King @ 2020-09-18 18:12 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Denton Liu, Git Mailing List

On Thu, Sep 17, 2020 at 10:11:56PM -0700, Junio C Hamano wrote:

> > Using oid_to_hex_r() avoids an extra copy, and the leak goes away too:
> >
> >   char merge_base_hex[GIT_MAX_HEXSZ + 1];
> >   ...
> >   name = oid_to_hex_r(merge_base_hex, &oid);
> >
> > -Peff
> 
> Yes, I was debating myself if I should mention it or trust/assume
> that the contributor can easily figure it out.

I was tempted to call "xstrdup(oid_to_hex())" an anti-pattern, but
looking at most of the other calls, they really do want a new string
that lasts longer than a stack variable.

And even the stack ones are a bit ugly in that you have to know to size
things correctly.

So I lost my enthusiasm to crusade against it. ;)

-Peff

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 06/10] diff-lib: define diff_get_merge_base()
  2020-09-18 10:34         ` Denton Liu
@ 2020-09-19  0:33           ` Junio C Hamano
  0 siblings, 0 replies; 58+ messages in thread
From: Junio C Hamano @ 2020-09-19  0:33 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

Denton Liu <liu.denton@gmail.com> writes:

> became something like
>
> 	if (diff_get_merge_base(revs, &oid))
> 		exit(128);
>
> so I do agree with the spirit of the change but in reality, it just
> creates more busywork for the callers.

OK, then lets keep them as die()s.

> I think that it should be die-worthy because it's a logic error for a
> user to do this. I can't think of any situation where it wouldn't be
> more desirable error early to correct a user's thinking. Plus, we're
> trying to move away from the `...` notation anyway ;)

I do not think so.  We are *NOT* trying to move away from A...B;
what is mistake is A..B and that is what we want to move away from.
Luckily, there is no need to introduce a new option there, because
the user can just stop typing .. and instead type SP.

The primary value of the new option you are adding is that it allows
us to compare the index and the working tree with the merge base.
The current A...B notation can only be used to compare two trees.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
  2020-09-18 16:52           ` Junio C Hamano
@ 2020-09-20 11:01             ` Denton Liu
  2020-09-21 16:05               ` Junio C Hamano
  0 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:01 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List

Hi Junio,

On Fri, Sep 18, 2020 at 09:52:39AM -0700, Junio C Hamano wrote:
> I wonder if turning the builtin/diff-tree.c to match that structure
> make the result easier to understand (and I'll be perfectly happy if
> the answer to this question turns out to be "no, the result of the
> posted patch is the easiest to follow").

git diff-tree does not even recognise ranges so as a result, the else
case does not even need to exist there, unlike in git diff.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v4 00/10] builtin/diff: learn --merge-base
  2020-09-17  7:44   ` [PATCH v3 00/10] " Denton Liu
                       ` (9 preceding siblings ...)
  2020-09-17  7:44     ` [PATCH v3 10/10] contrib/completion: complete `git diff --merge-base` Denton Liu
@ 2020-09-20 11:22     ` Denton Liu
  2020-09-20 11:22       ` [PATCH v4 01/10] t4068: remove unnecessary >tmp Denton Liu
                         ` (9 more replies)
  10 siblings, 10 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Jeff King

The range-notation in `git diff` has been cited as a mistake since diff
compares two endpoints, not whole ranges.[0]  In fact, the ranges seem
to take on the opposite meanings when compared to range notation in
`git log`.

In an effort to reduce the use of range notation as much as possible,
introduce the `--merge-base` flag, slightly modified from a suggestion
by Jonathan Nieder.[1] This flag allows us to replace the first commit
given on the command-line with its merge base between the first and
second commits.

One additional bonus is that this flag allows the "after" side to be not
just constrained to a commit (like with `...` notation). It can now be
the working tree or the index as well.

The `--merge-base` name isn't very satisfying. If anyone has any better
suggestions, please let me know.

Changes since v1:

* Implement Junio's documentation suggestions

* Update git diff's usage to include this option

Changes since v2:

* Teach diff-index and diff-tree the --merge-base option as well

Changes since v3:

* Don't use bitfields directly; extract them to an intermediate variable

* Use explicit literals in diff_get_merge_base() for clarity

* Fix xstrdup() leak

* I misunderstood and thought we wanted to deprecate `...` notation;
  this is not the case so remove all references to "gentle deprecation"
  and don't remove usage text that references it

[0]: https://lore.kernel.org/git/xmqqy2v26hu0.fsf@gitster-ct.c.googlers.com/
[1]: https://lore.kernel.org/git/20191223215928.GB38316@google.com/

Denton Liu (10):
  t4068: remove unnecessary >tmp
  git-diff-index.txt: make --cached description a proper sentence
  git-diff.txt: backtick quote command text
  contrib/completion: extract common diff/difftool options
  diff-lib: accept option flags in run_diff_index()
  diff-lib: define diff_get_merge_base()
  t4068: add --merge-base tests
  builtin/diff-index: learn --merge-base
  builtin/diff-tree: learn --merge-base
  contrib/completion: complete `git diff --merge-base`

 Documentation/git-diff-index.txt       |   9 +-
 Documentation/git-diff-tree.txt        |   7 +-
 Documentation/git-diff.txt             |  36 +++--
 builtin/diff-index.c                   |  10 +-
 builtin/diff-tree.c                    |  18 +++
 builtin/diff.c                         |  49 +++++--
 contrib/completion/git-completion.bash |  15 +-
 diff-lib.c                             |  63 +++++++-
 diff.h                                 |   7 +-
 t/t4068-diff-symmetric-merge-base.sh   | 193 +++++++++++++++++++++++++
 t/t4068-diff-symmetric.sh              |  91 ------------
 11 files changed, 358 insertions(+), 140 deletions(-)
 create mode 100755 t/t4068-diff-symmetric-merge-base.sh
 delete mode 100755 t/t4068-diff-symmetric.sh

Range-diff against v3:
 1:  80e9066a59 =  1:  80e9066a59 t4068: remove unnecessary >tmp
 2:  21b20281e6 =  2:  21b20281e6 git-diff-index.txt: make --cached description a proper sentence
 3:  ca9568c2ea =  3:  ca9568c2ea git-diff.txt: backtick quote command text
 4:  1ac8459541 =  4:  1ac8459541 contrib/completion: extract common diff/difftool options
 5:  496908ac10 !  5:  99d8b51585 diff-lib: accept option flags in run_diff_index()
    @@ diff-lib.c: static int diff_cache(struct rev_info *revs,
     +int run_diff_index(struct rev_info *revs, unsigned int option)
      {
      	struct object_array_entry *ent;
    ++	int cached = !!(option & DIFF_INDEX_CACHED);
      
    -@@ diff-lib.c: int run_diff_index(struct rev_info *revs, int cached)
    - 
    - 	trace_performance_enter();
    - 	ent = revs->pending.objects;
    --	if (diff_cache(revs, &ent->item->oid, ent->name, cached))
    -+	if (diff_cache(revs, &ent->item->oid, ent->name, !!(option & DIFF_INDEX_CACHED)))
    - 		exit(128);
    - 
    --	diff_set_mnemonic_prefix(&revs->diffopt, "c/", cached ? "i/" : "w/");
    -+	diff_set_mnemonic_prefix(&revs->diffopt, "c/", (option & DIFF_INDEX_CACHED) ? "i/" : "w/");
    - 	diffcore_fix_diff_index();
    - 	diffcore_std(&revs->diffopt);
    - 	diff_flush(&revs->diffopt);
    + 	if (revs->pending.nr != 1)
    + 		BUG("run_diff_index must be passed exactly one tree");
     
      ## diff.h ##
     @@ diff.h: const char *diff_aligned_abbrev(const struct object_id *sha1, int);
    @@ diff.h: const char *diff_aligned_abbrev(const struct object_id *sha1, int);
      #define DIFF_RACY_IS_MODIFIED 02
      int run_diff_files(struct rev_info *revs, unsigned int option);
     -int run_diff_index(struct rev_info *revs, int cached);
    ++
     +#define DIFF_INDEX_CACHED 01
     +int run_diff_index(struct rev_info *revs, unsigned int option);
      
 6:  6aac57ca02 !  6:  3287e649f1 diff-lib: define diff_get_merge_base()
    @@ diff-lib.c: static int diff_cache(struct rev_info *revs,
     +	 * ranges produce three pending commits, resulting in a
     +	 * misleading error message.
     +	 */
    -+	if (revs->pending.nr > ARRAY_SIZE(mb_child))
    -+		die(_("--merge-base does not work with more than two commits"));
    ++	if (revs->pending.nr < 1 || revs->pending.nr > 2)
    ++		BUG("unexpected revs->pending.nr: %d", revs->pending.nr);
     +
     +	for (i = 0; i < revs->pending.nr; i++)
     +		mb_child[i] = lookup_commit_reference(the_repository, &revs->pending.objects[i].item->oid);
    -+	if (revs->pending.nr < ARRAY_SIZE(mb_child)) {
    ++	if (revs->pending.nr == 1) {
     +		struct object_id oid;
     +
    -+		if (revs->pending.nr != 1)
    -+			BUG("unexpected revs->pending.nr: %d", revs->pending.nr);
    -+
     +		if (get_oid("HEAD", &oid))
     +			die(_("unable to get HEAD"));
     +
 7:  c9225a0440 =  7:  27930d4476 t4068: add --merge-base tests
 8:  1e4f805e57 !  8:  f54baa4ecd builtin/diff-index: learn --merge-base
    @@ builtin/diff.c: static int builtin_diff_index(struct rev_info *revs,
      		argv++; argc--;
     
      ## diff-lib.c ##
    -@@ diff-lib.c: void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
    - int run_diff_index(struct rev_info *revs, unsigned int option)
    +@@ diff-lib.c: int run_diff_index(struct rev_info *revs, unsigned int option)
      {
      	struct object_array_entry *ent;
    + 	int cached = !!(option & DIFF_INDEX_CACHED);
    ++	int merge_base = !!(option & DIFF_INDEX_MERGE_BASE);
     +	struct object_id oid;
     +	const char *name;
    ++	char merge_base_hex[GIT_MAX_HEXSZ + 1];
      
      	if (revs->pending.nr != 1)
      		BUG("run_diff_index must be passed exactly one tree");
      
      	trace_performance_enter();
      	ent = revs->pending.objects;
    --	if (diff_cache(revs, &ent->item->oid, ent->name, !!(option & DIFF_INDEX_CACHED)))
    +-	if (diff_cache(revs, &ent->item->oid, ent->name, cached))
     +
    -+	if (option & DIFF_INDEX_MERGE_BASE) {
    ++	if (merge_base) {
     +		diff_get_merge_base(revs, &oid);
    -+		name = xstrdup(oid_to_hex(&oid));
    ++		name = oid_to_hex_r(merge_base_hex, &oid);
     +	} else {
     +		oidcpy(&oid, &ent->item->oid);
     +		name = ent->name;
     +	}
     +
    -+	if (diff_cache(revs, &oid, name, !!(option & DIFF_INDEX_CACHED)))
    ++	if (diff_cache(revs, &oid, name, cached))
      		exit(128);
      
    - 	diff_set_mnemonic_prefix(&revs->diffopt, "c/", (option & DIFF_INDEX_CACHED) ? "i/" : "w/");
    + 	diff_set_mnemonic_prefix(&revs->diffopt, "c/", cached ? "i/" : "w/");
     
      ## diff.h ##
     @@ diff.h: void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);
    - #define DIFF_RACY_IS_MODIFIED 02
      int run_diff_files(struct rev_info *revs, unsigned int option);
    + 
      #define DIFF_INDEX_CACHED 01
     +#define DIFF_INDEX_MERGE_BASE 02
      int run_diff_index(struct rev_info *revs, unsigned int option);
 9:  c0d27b125e !  9:  4880d21119 builtin/diff-tree: learn --merge-base
    @@ Metadata
      ## Commit message ##
         builtin/diff-tree: learn --merge-base
     
    -    In order to get the diff between a commit and its merge base, the
    -    currently preferred method is to use `git diff A...B`. However, the
    -    range-notation with diff has, time and time again, been noted as a point
    -    of confusion and thus, it should be avoided. Although we have a
    -    substitute for the double-dot notation, we don't have any replacement
    -    for the triple-dot notation.
    +    The previous commit introduced ---merge-base a way to take the diff
    +    between the working tree or index and the merge base between an arbitrary
    +    commit and HEAD. It makes sense to extend this option to support the
    +    case where two commits are given too and behave in a manner identical to
    +    `git diff A...B`.
     
    -    Introduce the --merge-base flag as a replacement for triple-dot
    +    Introduce the --merge-base flag as an alternative to triple-dot
         notation. Thus, we would be able to write the above as
    -    `git diff --merge-base A B`, allowing us to gently deprecate
    -    range-notation completely.
    +    `git diff --merge-base A B`.
     
      ## Documentation/git-diff-tree.txt ##
     @@ Documentation/git-diff-tree.txt: SYNOPSIS
    @@ Documentation/git-diff.txt: SYNOPSIS
      'git diff' [<options>] [<commit>] [--] [<path>...]
      'git diff' [<options>] --cached [--merge-base] [<commit>] [--] [<path>...]
     -'git diff' [<options>] <commit> [<commit>...] <commit> [--] [<path>...]
    --'git diff' [<options>] <commit>...<commit> [--] [<path>...]
     +'git diff' [<options>] [--merge-base] <commit> [<commit>...] <commit> [--] [<path>...]
    + 'git diff' [<options>] <commit>...<commit> [--] [<path>...]
      'git diff' [<options>] <blob> <blob>
      'git diff' [<options>] --no-index [--] <path> <path>
    - 
     @@ Documentation/git-diff.txt: of <commit> and HEAD.  `git diff --merge-base A` is equivalent to
      	branch name to compare with the tip of a different
      	branch.
    @@ builtin/diff.c
      "git diff [<options>] [<commit>] [--] [<path>...]\n"
      "   or: git diff [<options>] --cached [<commit>] [--] [<path>...]\n"
     -"   or: git diff [<options>] <commit> [<commit>...] <commit> [--] [<path>...]\n"
    --"   or: git diff [<options>] <commit>...<commit>] [--] [<path>...]\n"
     +"   or: git diff [<options>] <commit> [--merge-base] [<commit>...] <commit> [--] [<path>...]\n"
    + "   or: git diff [<options>] <commit>...<commit>] [--] [<path>...]\n"
      "   or: git diff [<options>] <blob> <blob>]\n"
      "   or: git diff [<options>] --no-index [--] <path> <path>]\n"
    - COMMON_DIFF_OPTIONS_HELP;
     @@ builtin/diff.c: static int builtin_diff_tree(struct rev_info *revs,
      			     struct object_array_entry *ent1)
      {
10:  42a8c9b665 = 10:  32e3e52b5f contrib/completion: complete `git diff --merge-base`
-- 
2.28.0.760.g8d73e04208


^ permalink raw reply	[flat|nested] 58+ messages in thread

* [PATCH v4 01/10] t4068: remove unnecessary >tmp
  2020-09-20 11:22     ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
@ 2020-09-20 11:22       ` Denton Liu
  2020-09-20 11:22       ` [PATCH v4 02/10] git-diff-index.txt: make --cached description a proper sentence Denton Liu
                         ` (8 subsequent siblings)
  9 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Jeff King

The many `git diff` invocations have a `>tmp` redirection even though
the file is not being used afterwards. Remove these unnecessary
redirections.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4068-diff-symmetric.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh
index 31d17a5af0..60c506c2b2 100755
--- a/t/t4068-diff-symmetric.sh
+++ b/t/t4068-diff-symmetric.sh
@@ -64,27 +64,27 @@ test_expect_success 'diff with two merge bases' '
 '
 
 test_expect_success 'diff with no merge bases' '
-	test_must_fail git diff br2...br3 >tmp 2>err &&
+	test_must_fail git diff br2...br3 2>err &&
 	test_i18ngrep "fatal: br2...br3: no merge base" err
 '
 
 test_expect_success 'diff with too many symmetric differences' '
-	test_must_fail git diff br1...master br2...br3 >tmp 2>err &&
+	test_must_fail git diff br1...master br2...br3 2>err &&
 	test_i18ngrep "usage" err
 '
 
 test_expect_success 'diff with symmetric difference and extraneous arg' '
-	test_must_fail git diff master br1...master >tmp 2>err &&
+	test_must_fail git diff master br1...master 2>err &&
 	test_i18ngrep "usage" err
 '
 
 test_expect_success 'diff with two ranges' '
-	test_must_fail git diff master br1..master br2..br3 >tmp 2>err &&
+	test_must_fail git diff master br1..master br2..br3 2>err &&
 	test_i18ngrep "usage" err
 '
 
 test_expect_success 'diff with ranges and extra arg' '
-	test_must_fail git diff master br1..master commit-D >tmp 2>err &&
+	test_must_fail git diff master br1..master commit-D 2>err &&
 	test_i18ngrep "usage" err
 '
 
-- 
2.28.0.760.g8d73e04208


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v4 02/10] git-diff-index.txt: make --cached description a proper sentence
  2020-09-20 11:22     ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
  2020-09-20 11:22       ` [PATCH v4 01/10] t4068: remove unnecessary >tmp Denton Liu
@ 2020-09-20 11:22       ` Denton Liu
  2020-09-20 11:22       ` [PATCH v4 03/10] git-diff.txt: backtick quote command text Denton Liu
                         ` (7 subsequent siblings)
  9 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Jeff King

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-diff-index.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-diff-index.txt b/Documentation/git-diff-index.txt
index f4bd8155c0..25fe165f00 100644
--- a/Documentation/git-diff-index.txt
+++ b/Documentation/git-diff-index.txt
@@ -27,7 +27,7 @@ include::diff-options.txt[]
 	The id of a tree object to diff against.
 
 --cached::
-	do not consider the on-disk file at all
+	Do not consider the on-disk file at all.
 
 -m::
 	By default, files recorded in the index but not checked
-- 
2.28.0.760.g8d73e04208


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v4 03/10] git-diff.txt: backtick quote command text
  2020-09-20 11:22     ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
  2020-09-20 11:22       ` [PATCH v4 01/10] t4068: remove unnecessary >tmp Denton Liu
  2020-09-20 11:22       ` [PATCH v4 02/10] git-diff-index.txt: make --cached description a proper sentence Denton Liu
@ 2020-09-20 11:22       ` Denton Liu
  2020-09-20 11:22       ` [PATCH v4 04/10] contrib/completion: extract common diff/difftool options Denton Liu
                         ` (6 subsequent siblings)
  9 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Jeff King

The modern way to quote commands in the documentation is to use
backticks instead of double-quotes as this renders the text with the
code style. Convert double-quoted command text to backtick-quoted
commands. While we're at it, quote one instance of `^@`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-diff.txt | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 727f24d16e..8f7b4ed3ca 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -68,13 +68,13 @@ files on disk.
 	This form is to view the results of a merge commit.  The first
 	listed <commit> must be the merge itself; the remaining two or
 	more commits should be its parents.  A convenient way to produce
-	the desired set of revisions is to use the {caret}@ suffix.
+	the desired set of revisions is to use the `^@` suffix.
 	For instance, if `master` names a merge commit, `git diff master
 	master^@` gives the same combined diff as `git show master`.
 
 'git diff' [<options>] <commit>..<commit> [--] [<path>...]::
 
-	This is synonymous to the earlier form (without the "..") for
+	This is synonymous to the earlier form (without the `..`) for
 	viewing the changes between two arbitrary <commit>.  If <commit> on
 	one side is omitted, it will have the same effect as
 	using HEAD instead.
@@ -83,20 +83,20 @@ files on disk.
 
 	This form is to view the changes on the branch containing
 	and up to the second <commit>, starting at a common ancestor
-	of both <commit>.  "git diff A\...B" is equivalent to
-	"git diff $(git merge-base A B) B".  You can omit any one
+	of both <commit>.  `git diff A...B` is equivalent to
+	`git diff $(git merge-base A B) B`.  You can omit any one
 	of <commit>, which has the same effect as using HEAD instead.
 
 Just in case you are doing something exotic, it should be
 noted that all of the <commit> in the above description, except
-in the last two forms that use ".." notations, can be any
+in the last two forms that use `..` notations, can be any
 <tree>.
 
 For a more complete list of ways to spell <commit>, see
 "SPECIFYING REVISIONS" section in linkgit:gitrevisions[7].
 However, "diff" is about comparing two _endpoints_, not ranges,
-and the range notations ("<commit>..<commit>" and
-"<commit>\...<commit>") do not mean a range as defined in the
+and the range notations (`<commit>..<commit>` and
+`<commit>...<commit>`) do not mean a range as defined in the
 "SPECIFYING RANGES" section in linkgit:gitrevisions[7].
 
 'git diff' [<options>] <blob> <blob>::
@@ -144,9 +144,9 @@ $ git diff HEAD       <3>
 +
 <1> Changes in the working tree not yet staged for the next commit.
 <2> Changes between the index and your last commit; what you
-    would be committing if you run "git commit" without "-a" option.
+    would be committing if you run `git commit` without `-a` option.
 <3> Changes in the working tree since your last commit; what you
-    would be committing if you run "git commit -a"
+    would be committing if you run `git commit -a`
 
 Comparing with arbitrary commits::
 +
-- 
2.28.0.760.g8d73e04208


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v4 04/10] contrib/completion: extract common diff/difftool options
  2020-09-20 11:22     ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
                         ` (2 preceding siblings ...)
  2020-09-20 11:22       ` [PATCH v4 03/10] git-diff.txt: backtick quote command text Denton Liu
@ 2020-09-20 11:22       ` Denton Liu
  2020-09-20 11:22       ` [PATCH v4 05/10] diff-lib: accept option flags in run_diff_index() Denton Liu
                         ` (5 subsequent siblings)
  9 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Jeff King

difftool parses its own options and then passes the remaining options
onto diff. As a result, they share common command-line options. Instead
of duplicating the list, use a shared $__git_diff_difftool_options list.

The completion for diff is missing --relative and the completion for
difftool is missing --no-index. Add both of these to the common list.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/completion/git-completion.bash | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9147fba3d5..f68c8e0646 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1691,6 +1691,10 @@ __git_diff_common_options="--stat --numstat --shortstat --summary
 			--textconv --no-textconv
 "
 
+__git_diff_difftool_options="--cached --staged --pickaxe-all --pickaxe-regex
+			--base --ours --theirs --no-index --relative
+			$__git_diff_common_options"
+
 _git_diff ()
 {
 	__git_has_doubledash && return
@@ -1713,10 +1717,7 @@ _git_diff ()
 		return
 		;;
 	--*)
-		__gitcomp "--cached --staged --pickaxe-all --pickaxe-regex
-			--base --ours --theirs --no-index
-			$__git_diff_common_options
-			"
+		__gitcomp "$__git_diff_difftool_options"
 		return
 		;;
 	esac
@@ -1738,11 +1739,7 @@ _git_difftool ()
 		return
 		;;
 	--*)
-		__gitcomp_builtin difftool "$__git_diff_common_options
-					--base --cached --ours --theirs
-					--pickaxe-all --pickaxe-regex
-					--relative --staged
-					"
+		__gitcomp_builtin difftool "$__git_diff_difftool_options"
 		return
 		;;
 	esac
-- 
2.28.0.760.g8d73e04208


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v4 05/10] diff-lib: accept option flags in run_diff_index()
  2020-09-20 11:22     ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
                         ` (3 preceding siblings ...)
  2020-09-20 11:22       ` [PATCH v4 04/10] contrib/completion: extract common diff/difftool options Denton Liu
@ 2020-09-20 11:22       ` Denton Liu
  2020-09-20 11:22       ` [PATCH v4 06/10] diff-lib: define diff_get_merge_base() Denton Liu
                         ` (4 subsequent siblings)
  9 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Jeff King

In a future commit, we will teach run_diff_index() to accept more
options via flag bits. For now, change `cached` into a flag in the
`option` bitfield. The behaviour should remain exactly the same.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/diff-index.c | 8 ++++----
 builtin/diff.c       | 8 ++++----
 diff-lib.c           | 3 ++-
 diff.h               | 4 +++-
 4 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 93ec642423..c3878f7ad6 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -15,7 +15,7 @@ COMMON_DIFF_OPTIONS_HELP;
 int cmd_diff_index(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info rev;
-	int cached = 0;
+	unsigned int option = 0;
 	int i;
 	int result;
 
@@ -32,7 +32,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 		const char *arg = argv[i];
 
 		if (!strcmp(arg, "--cached"))
-			cached = 1;
+			option |= DIFF_INDEX_CACHED;
 		else
 			usage(diff_cache_usage);
 	}
@@ -46,7 +46,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	if (rev.pending.nr != 1 ||
 	    rev.max_count != -1 || rev.min_age != -1 || rev.max_age != -1)
 		usage(diff_cache_usage);
-	if (!cached) {
+	if (!(option & DIFF_INDEX_CACHED)) {
 		setup_work_tree();
 		if (read_cache_preload(&rev.diffopt.pathspec) < 0) {
 			perror("read_cache_preload");
@@ -56,7 +56,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 		perror("read_cache");
 		return -1;
 	}
-	result = run_diff_index(&rev, cached);
+	result = run_diff_index(&rev, option);
 	UNLEAK(rev);
 	return diff_result_code(&rev.diffopt, result);
 }
diff --git a/builtin/diff.c b/builtin/diff.c
index cb98811c21..e45e19e37e 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -134,11 +134,11 @@ static int builtin_diff_blobs(struct rev_info *revs,
 static int builtin_diff_index(struct rev_info *revs,
 			      int argc, const char **argv)
 {
-	int cached = 0;
+	unsigned int option = 0;
 	while (1 < argc) {
 		const char *arg = argv[1];
 		if (!strcmp(arg, "--cached") || !strcmp(arg, "--staged"))
-			cached = 1;
+			option |= DIFF_INDEX_CACHED;
 		else
 			usage(builtin_diff_usage);
 		argv++; argc--;
@@ -151,7 +151,7 @@ static int builtin_diff_index(struct rev_info *revs,
 	    revs->max_count != -1 || revs->min_age != -1 ||
 	    revs->max_age != -1)
 		usage(builtin_diff_usage);
-	if (!cached) {
+	if (!(option & DIFF_INDEX_CACHED)) {
 		setup_work_tree();
 		if (read_cache_preload(&revs->diffopt.pathspec) < 0) {
 			perror("read_cache_preload");
@@ -161,7 +161,7 @@ static int builtin_diff_index(struct rev_info *revs,
 		perror("read_cache");
 		return -1;
 	}
-	return run_diff_index(revs, cached);
+	return run_diff_index(revs, option);
 }
 
 static int builtin_diff_tree(struct rev_info *revs,
diff --git a/diff-lib.c b/diff-lib.c
index 50521e2093..d5b2c8af56 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -518,9 +518,10 @@ static int diff_cache(struct rev_info *revs,
 	return unpack_trees(1, &t, &opts);
 }
 
-int run_diff_index(struct rev_info *revs, int cached)
+int run_diff_index(struct rev_info *revs, unsigned int option)
 {
 	struct object_array_entry *ent;
+	int cached = !!(option & DIFF_INDEX_CACHED);
 
 	if (revs->pending.nr != 1)
 		BUG("run_diff_index must be passed exactly one tree");
diff --git a/diff.h b/diff.h
index e0c0af6286..aea0d5b96b 100644
--- a/diff.h
+++ b/diff.h
@@ -585,7 +585,9 @@ const char *diff_aligned_abbrev(const struct object_id *sha1, int);
 /* report racily-clean paths as modified */
 #define DIFF_RACY_IS_MODIFIED 02
 int run_diff_files(struct rev_info *revs, unsigned int option);
-int run_diff_index(struct rev_info *revs, int cached);
+
+#define DIFF_INDEX_CACHED 01
+int run_diff_index(struct rev_info *revs, unsigned int option);
 
 int do_diff_cache(const struct object_id *, struct diff_options *);
 int diff_flush_patch_id(struct diff_options *, struct object_id *, int, int);
-- 
2.28.0.760.g8d73e04208


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v4 06/10] diff-lib: define diff_get_merge_base()
  2020-09-20 11:22     ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
                         ` (4 preceding siblings ...)
  2020-09-20 11:22       ` [PATCH v4 05/10] diff-lib: accept option flags in run_diff_index() Denton Liu
@ 2020-09-20 11:22       ` Denton Liu
  2020-09-20 11:22       ` [PATCH v4 07/10] t4068: add --merge-base tests Denton Liu
                         ` (3 subsequent siblings)
  9 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Jeff King

In a future commit, we will be using this function to implement
--merge-base functionality in various diff commands.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 diff-lib.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 diff.h     |  2 ++
 2 files changed, 47 insertions(+)

diff --git a/diff-lib.c b/diff-lib.c
index d5b2c8af56..fa64e64bbe 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -13,6 +13,7 @@
 #include "submodule.h"
 #include "dir.h"
 #include "fsmonitor.h"
+#include "commit-reach.h"
 
 /*
  * diff-files
@@ -518,6 +519,50 @@ static int diff_cache(struct rev_info *revs,
 	return unpack_trees(1, &t, &opts);
 }
 
+void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)
+{
+	int i;
+	struct commit *mb_child[2] = {0};
+	struct commit_list *merge_bases;
+
+	for (i = 0; i < revs->pending.nr; i++) {
+		struct object *obj = revs->pending.objects[i].item;
+		if (obj->flags)
+			die(_("--merge-base does not work with ranges"));
+		if (obj->type != OBJ_COMMIT)
+			die(_("--merge-base only works with commits"));
+	}
+
+	/*
+	 * This check must go after the for loop above because A...B
+	 * ranges produce three pending commits, resulting in a
+	 * misleading error message.
+	 */
+	if (revs->pending.nr < 1 || revs->pending.nr > 2)
+		BUG("unexpected revs->pending.nr: %d", revs->pending.nr);
+
+	for (i = 0; i < revs->pending.nr; i++)
+		mb_child[i] = lookup_commit_reference(the_repository, &revs->pending.objects[i].item->oid);
+	if (revs->pending.nr == 1) {
+		struct object_id oid;
+
+		if (get_oid("HEAD", &oid))
+			die(_("unable to get HEAD"));
+
+		mb_child[1] = lookup_commit_reference(the_repository, &oid);
+	}
+
+	merge_bases = repo_get_merge_bases(the_repository, mb_child[0], mb_child[1]);
+	if (!merge_bases)
+		die(_("no merge base found"));
+	if (merge_bases->next)
+		die(_("multiple merge bases found"));
+
+	oidcpy(mb, &merge_bases->item->object.oid);
+
+	free_commit_list(merge_bases);
+}
+
 int run_diff_index(struct rev_info *revs, unsigned int option)
 {
 	struct object_array_entry *ent;
diff --git a/diff.h b/diff.h
index aea0d5b96b..fedfeab7a2 100644
--- a/diff.h
+++ b/diff.h
@@ -580,6 +580,8 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);
  */
 const char *diff_aligned_abbrev(const struct object_id *sha1, int);
 
+void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);
+
 /* do not report anything on removed paths */
 #define DIFF_SILENT_ON_REMOVED 01
 /* report racily-clean paths as modified */
-- 
2.28.0.760.g8d73e04208


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v4 07/10] t4068: add --merge-base tests
  2020-09-20 11:22     ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
                         ` (5 preceding siblings ...)
  2020-09-20 11:22       ` [PATCH v4 06/10] diff-lib: define diff_get_merge_base() Denton Liu
@ 2020-09-20 11:22       ` Denton Liu
  2020-09-20 11:22       ` [PATCH v4 08/10] builtin/diff-index: learn --merge-base Denton Liu
                         ` (2 subsequent siblings)
  9 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Jeff King

In the future, we will be adding more --merge-base tests to this test
script. To prepare for that, rename the script accordingly and update
its description. Also, add two basic --merge-base tests that don't
require any functionality to be implemented yet.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 ...ymmetric.sh => t4068-diff-symmetric-merge-base.sh} | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
 rename t/{t4068-diff-symmetric.sh => t4068-diff-symmetric-merge-base.sh} (86%)

diff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric-merge-base.sh
similarity index 86%
rename from t/t4068-diff-symmetric.sh
rename to t/t4068-diff-symmetric-merge-base.sh
index 60c506c2b2..bd4cf254d9 100755
--- a/t/t4068-diff-symmetric.sh
+++ b/t/t4068-diff-symmetric-merge-base.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='behavior of diff with symmetric-diff setups'
+test_description='behavior of diff with symmetric-diff setups and --merge-base'
 
 . ./test-lib.sh
 
@@ -88,4 +88,13 @@ test_expect_success 'diff with ranges and extra arg' '
 	test_i18ngrep "usage" err
 '
 
+test_expect_success 'diff --merge-base with no commits' '
+	test_must_fail git diff --merge-base
+'
+
+test_expect_success 'diff --merge-base with three commits' '
+	test_must_fail git diff --merge-base br1 br2 master 2>err &&
+	test_i18ngrep "usage" err
+'
+
 test_done
-- 
2.28.0.760.g8d73e04208


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v4 08/10] builtin/diff-index: learn --merge-base
  2020-09-20 11:22     ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
                         ` (6 preceding siblings ...)
  2020-09-20 11:22       ` [PATCH v4 07/10] t4068: add --merge-base tests Denton Liu
@ 2020-09-20 11:22       ` Denton Liu
  2020-09-29 19:53         ` Martin Ågren
  2020-09-20 11:22       ` [PATCH v4 09/10] builtin/diff-tree: " Denton Liu
  2020-09-20 11:22       ` [PATCH v4 10/10] contrib/completion: complete `git diff --merge-base` Denton Liu
  9 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Jeff King

There is currently no easy way to take the diff between the working tree
or index and the merge base between an arbitrary commit and HEAD. Even
diff's `...` notation doesn't allow this because it only works between
commits. However, the ability to do this would be desirable to a user
who would like to see all the changes they've made on a branch plus
uncommitted changes without taking into account changes made in the
upstream branch.

Teach diff-index and diff (with one commit) the --merge-base option
which allows a user to use the merge base of a commit and HEAD as the
"before" side.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-diff-index.txt     |  7 +++-
 Documentation/git-diff.txt           | 12 ++++--
 builtin/diff-index.c                 |  2 +
 builtin/diff.c                       |  2 +
 diff-lib.c                           | 15 ++++++-
 diff.h                               |  1 +
 t/t4068-diff-symmetric-merge-base.sh | 59 ++++++++++++++++++++++++++++
 7 files changed, 92 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-diff-index.txt b/Documentation/git-diff-index.txt
index 25fe165f00..27acb31cbf 100644
--- a/Documentation/git-diff-index.txt
+++ b/Documentation/git-diff-index.txt
@@ -9,7 +9,7 @@ git-diff-index - Compare a tree to the working tree or index
 SYNOPSIS
 --------
 [verse]
-'git diff-index' [-m] [--cached] [<common diff options>] <tree-ish> [<path>...]
+'git diff-index' [-m] [--cached] [--merge-base] [<common diff options>] <tree-ish> [<path>...]
 
 DESCRIPTION
 -----------
@@ -29,6 +29,11 @@ include::diff-options.txt[]
 --cached::
 	Do not consider the on-disk file at all.
 
+--merge-base::
+	Instead of comparing <tree-ish> directly, use the merge base
+	between <tree-ish> and HEAD instead.  <tree-ish> must be a
+	commit.
+
 -m::
 	By default, files recorded in the index but not checked
 	out are reported as deleted.  This flag makes
diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 8f7b4ed3ca..762ee6d074 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git diff' [<options>] [<commit>] [--] [<path>...]
-'git diff' [<options>] --cached [<commit>] [--] [<path>...]
+'git diff' [<options>] --cached [--merge-base] [<commit>] [--] [<path>...]
 'git diff' [<options>] <commit> [<commit>...] <commit> [--] [<path>...]
 'git diff' [<options>] <commit>...<commit> [--] [<path>...]
 'git diff' [<options>] <blob> <blob>
@@ -40,7 +40,7 @@ files on disk.
 	or when running the command outside a working tree
 	controlled by Git. This form implies `--exit-code`.
 
-'git diff' [<options>] --cached [<commit>] [--] [<path>...]::
+'git diff' [<options>] --cached [--merge-base] [<commit>] [--] [<path>...]::
 
 	This form is to view the changes you staged for the next
 	commit relative to the named <commit>.  Typically you
@@ -49,6 +49,10 @@ files on disk.
 	If HEAD does not exist (e.g. unborn branches) and
 	<commit> is not given, it shows all staged changes.
 	--staged is a synonym of --cached.
++
+If --merge-base is given, instead of using <commit>, use the merge base
+of <commit> and HEAD.  `git diff --merge-base A` is equivalent to
+`git diff $(git merge-base A HEAD)`.
 
 'git diff' [<options>] <commit> [--] [<path>...]::
 
@@ -89,8 +93,8 @@ files on disk.
 
 Just in case you are doing something exotic, it should be
 noted that all of the <commit> in the above description, except
-in the last two forms that use `..` notations, can be any
-<tree>.
+in the `--merge-base` case and in the last two forms that use `..`
+notations, can be any <tree>.
 
 For a more complete list of ways to spell <commit>, see
 "SPECIFYING REVISIONS" section in linkgit:gitrevisions[7].
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index c3878f7ad6..7f5281c461 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -33,6 +33,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 
 		if (!strcmp(arg, "--cached"))
 			option |= DIFF_INDEX_CACHED;
+		else if (!strcmp(arg, "--merge-base"))
+			option |= DIFF_INDEX_MERGE_BASE;
 		else
 			usage(diff_cache_usage);
 	}
diff --git a/builtin/diff.c b/builtin/diff.c
index e45e19e37e..1baea18ae0 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -139,6 +139,8 @@ static int builtin_diff_index(struct rev_info *revs,
 		const char *arg = argv[1];
 		if (!strcmp(arg, "--cached") || !strcmp(arg, "--staged"))
 			option |= DIFF_INDEX_CACHED;
+		else if (!strcmp(arg, "--merge-base"))
+			option |= DIFF_INDEX_MERGE_BASE;
 		else
 			usage(builtin_diff_usage);
 		argv++; argc--;
diff --git a/diff-lib.c b/diff-lib.c
index fa64e64bbe..79defdc6b8 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -567,13 +567,26 @@ int run_diff_index(struct rev_info *revs, unsigned int option)
 {
 	struct object_array_entry *ent;
 	int cached = !!(option & DIFF_INDEX_CACHED);
+	int merge_base = !!(option & DIFF_INDEX_MERGE_BASE);
+	struct object_id oid;
+	const char *name;
+	char merge_base_hex[GIT_MAX_HEXSZ + 1];
 
 	if (revs->pending.nr != 1)
 		BUG("run_diff_index must be passed exactly one tree");
 
 	trace_performance_enter();
 	ent = revs->pending.objects;
-	if (diff_cache(revs, &ent->item->oid, ent->name, cached))
+
+	if (merge_base) {
+		diff_get_merge_base(revs, &oid);
+		name = oid_to_hex_r(merge_base_hex, &oid);
+	} else {
+		oidcpy(&oid, &ent->item->oid);
+		name = ent->name;
+	}
+
+	if (diff_cache(revs, &oid, name, cached))
 		exit(128);
 
 	diff_set_mnemonic_prefix(&revs->diffopt, "c/", cached ? "i/" : "w/");
diff --git a/diff.h b/diff.h
index fedfeab7a2..6c2efa16fd 100644
--- a/diff.h
+++ b/diff.h
@@ -589,6 +589,7 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb);
 int run_diff_files(struct rev_info *revs, unsigned int option);
 
 #define DIFF_INDEX_CACHED 01
+#define DIFF_INDEX_MERGE_BASE 02
 int run_diff_index(struct rev_info *revs, unsigned int option);
 
 int do_diff_cache(const struct object_id *, struct diff_options *);
diff --git a/t/t4068-diff-symmetric-merge-base.sh b/t/t4068-diff-symmetric-merge-base.sh
index bd4cf254d9..49432379cb 100755
--- a/t/t4068-diff-symmetric-merge-base.sh
+++ b/t/t4068-diff-symmetric-merge-base.sh
@@ -97,4 +97,63 @@ test_expect_success 'diff --merge-base with three commits' '
 	test_i18ngrep "usage" err
 '
 
+for cmd in diff-index diff
+do
+	test_expect_success "$cmd --merge-base with one commit" '
+		git checkout master &&
+		git $cmd commit-C >expect &&
+		git $cmd --merge-base br2 >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "$cmd --merge-base with one commit and unstaged changes" '
+		git checkout master &&
+		test_when_finished git reset --hard &&
+		echo unstaged >>c &&
+		git $cmd commit-C >expect &&
+		git $cmd --merge-base br2 >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "$cmd --merge-base with one commit and staged and unstaged changes" '
+		git checkout master &&
+		test_when_finished git reset --hard &&
+		echo staged >>c &&
+		git add c &&
+		echo unstaged >>c &&
+		git $cmd commit-C >expect &&
+		git $cmd --merge-base br2 >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "$cmd --merge-base --cached with one commit and staged and unstaged changes" '
+		git checkout master &&
+		test_when_finished git reset --hard &&
+		echo staged >>c &&
+		git add c &&
+		echo unstaged >>c &&
+		git $cmd --cached commit-C >expect &&
+		git $cmd --cached --merge-base br2 >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "$cmd --merge-base with non-commit" '
+		git checkout master &&
+		test_must_fail git $cmd --merge-base master^{tree} 2>err &&
+		test_i18ngrep "fatal: --merge-base only works with commits" err
+	'
+
+	test_expect_success "$cmd --merge-base with no merge bases and one commit" '
+		git checkout master &&
+		test_must_fail git $cmd --merge-base br3 2>err &&
+		test_i18ngrep "fatal: no merge base found" err
+	'
+
+	test_expect_success "$cmd --merge-base with multiple merge bases and one commit" '
+		git checkout master &&
+		test_must_fail git $cmd --merge-base br1 2>err &&
+		test_i18ngrep "fatal: multiple merge bases found" err
+	'
+done
+
 test_done
-- 
2.28.0.760.g8d73e04208


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v4 09/10] builtin/diff-tree: learn --merge-base
  2020-09-20 11:22     ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
                         ` (7 preceding siblings ...)
  2020-09-20 11:22       ` [PATCH v4 08/10] builtin/diff-index: learn --merge-base Denton Liu
@ 2020-09-20 11:22       ` Denton Liu
  2020-09-20 11:22       ` [PATCH v4 10/10] contrib/completion: complete `git diff --merge-base` Denton Liu
  9 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Jeff King

The previous commit introduced ---merge-base a way to take the diff
between the working tree or index and the merge base between an arbitrary
commit and HEAD. It makes sense to extend this option to support the
case where two commits are given too and behave in a manner identical to
`git diff A...B`.

Introduce the --merge-base flag as an alternative to triple-dot
notation. Thus, we would be able to write the above as
`git diff --merge-base A B`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-diff-tree.txt      |  7 ++++-
 Documentation/git-diff.txt           |  8 ++++--
 builtin/diff-tree.c                  | 18 +++++++++++++
 builtin/diff.c                       | 39 +++++++++++++++++++---------
 t/t4068-diff-symmetric-merge-base.sh | 34 ++++++++++++++++++++++++
 5 files changed, 91 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt
index 5c8a2a5e97..2fc24c542f 100644
--- a/Documentation/git-diff-tree.txt
+++ b/Documentation/git-diff-tree.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git diff-tree' [--stdin] [-m] [-s] [-v] [--no-commit-id] [--pretty]
-	      [-t] [-r] [-c | --cc] [--combined-all-paths] [--root]
+	      [-t] [-r] [-c | --cc] [--combined-all-paths] [--root] [--merge-base]
 	      [<common diff options>] <tree-ish> [<tree-ish>] [<path>...]
 
 DESCRIPTION
@@ -43,6 +43,11 @@ include::diff-options.txt[]
 	When `--root` is specified the initial commit will be shown as a big
 	creation event. This is equivalent to a diff against the NULL tree.
 
+--merge-base::
+	Instead of comparing the <tree-ish>s directly, use the merge
+	base between the two <tree-ish>s as the "before" side.  There
+	must be two <tree-ish>s given and they must both be commits.
+
 --stdin::
 	When `--stdin` is specified, the command does not take
 	<tree-ish> arguments from the command line.  Instead, it
diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 762ee6d074..7f4c8a8ce7 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git diff' [<options>] [<commit>] [--] [<path>...]
 'git diff' [<options>] --cached [--merge-base] [<commit>] [--] [<path>...]
-'git diff' [<options>] <commit> [<commit>...] <commit> [--] [<path>...]
+'git diff' [<options>] [--merge-base] <commit> [<commit>...] <commit> [--] [<path>...]
 'git diff' [<options>] <commit>...<commit> [--] [<path>...]
 'git diff' [<options>] <blob> <blob>
 'git diff' [<options>] --no-index [--] <path> <path>
@@ -62,10 +62,14 @@ of <commit> and HEAD.  `git diff --merge-base A` is equivalent to
 	branch name to compare with the tip of a different
 	branch.
 
-'git diff' [<options>] <commit> <commit> [--] [<path>...]::
+'git diff' [<options>] [--merge-base] <commit> <commit> [--] [<path>...]::
 
 	This is to view the changes between two arbitrary
 	<commit>.
++
+If --merge-base is given, use the merge base of the two commits for the
+"before" side.  `git diff --merge-base A B` is equivalent to
+`git diff $(git merge-base A B) B`.
 
 'git diff' [<options>] <commit> <commit>... <commit> [--] [<path>...]::
 
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 802363d0a2..823d6678e5 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -111,6 +111,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	struct setup_revision_opt s_r_opt;
 	struct userformat_want w;
 	int read_stdin = 0;
+	int merge_base = 0;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage(diff_tree_usage);
@@ -143,9 +144,26 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 			read_stdin = 1;
 			continue;
 		}
+		if (!strcmp(arg, "--merge-base")) {
+			merge_base = 1;
+			continue;
+		}
 		usage(diff_tree_usage);
 	}
 
+	if (read_stdin && merge_base)
+		die(_("--stdin and --merge-base are mutually exclusive"));
+
+	if (merge_base) {
+		struct object_id oid;
+
+		if (opt->pending.nr != 2)
+			die(_("--merge-base only works with two commits"));
+
+		diff_get_merge_base(opt, &oid);
+		opt->pending.objects[0].item = lookup_object(the_repository, &oid);
+	}
+
 	/*
 	 * NOTE!  We expect "a..b" to expand to "^a b" but it is
 	 * perfectly valid for revision range parser to yield "b ^a",
diff --git a/builtin/diff.c b/builtin/diff.c
index 1baea18ae0..b50fc68c2a 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -26,7 +26,7 @@
 static const char builtin_diff_usage[] =
 "git diff [<options>] [<commit>] [--] [<path>...]\n"
 "   or: git diff [<options>] --cached [<commit>] [--] [<path>...]\n"
-"   or: git diff [<options>] <commit> [<commit>...] <commit> [--] [<path>...]\n"
+"   or: git diff [<options>] <commit> [--merge-base] [<commit>...] <commit> [--] [<path>...]\n"
 "   or: git diff [<options>] <commit>...<commit>] [--] [<path>...]\n"
 "   or: git diff [<options>] <blob> <blob>]\n"
 "   or: git diff [<options>] --no-index [--] <path> <path>]\n"
@@ -172,19 +172,34 @@ static int builtin_diff_tree(struct rev_info *revs,
 			     struct object_array_entry *ent1)
 {
 	const struct object_id *(oid[2]);
-	int swap = 0;
+	struct object_id mb_oid;
+	int merge_base = 0;
 
-	if (argc > 1)
-		usage(builtin_diff_usage);
+	while (1 < argc) {
+		const char *arg = argv[1];
+		if (!strcmp(arg, "--merge-base"))
+			merge_base = 1;
+		else
+			usage(builtin_diff_usage);
+		argv++; argc--;
+	}
 
-	/*
-	 * We saw two trees, ent0 and ent1.  If ent1 is uninteresting,
-	 * swap them.
-	 */
-	if (ent1->item->flags & UNINTERESTING)
-		swap = 1;
-	oid[swap] = &ent0->item->oid;
-	oid[1 - swap] = &ent1->item->oid;
+	if (merge_base) {
+		diff_get_merge_base(revs, &mb_oid);
+		oid[0] = &mb_oid;
+		oid[1] = &revs->pending.objects[1].item->oid;
+	} else {
+		int swap = 0;
+
+		/*
+		 * We saw two trees, ent0 and ent1.  If ent1 is uninteresting,
+		 * swap them.
+		 */
+		if (ent1->item->flags & UNINTERESTING)
+			swap = 1;
+		oid[swap] = &ent0->item->oid;
+		oid[1 - swap] = &ent1->item->oid;
+	}
 	diff_tree_oid(oid[0], oid[1], "", &revs->diffopt);
 	log_tree_diff_flush(revs);
 	return 0;
diff --git a/t/t4068-diff-symmetric-merge-base.sh b/t/t4068-diff-symmetric-merge-base.sh
index 49432379cb..03487cc945 100755
--- a/t/t4068-diff-symmetric-merge-base.sh
+++ b/t/t4068-diff-symmetric-merge-base.sh
@@ -156,4 +156,38 @@ do
 	'
 done
 
+for cmd in diff-tree diff
+do
+	test_expect_success "$cmd --merge-base with two commits" '
+		git $cmd commit-C master >expect &&
+		git $cmd --merge-base br2 master >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "$cmd --merge-base commit and non-commit" '
+		test_must_fail git $cmd --merge-base br2 master^{tree} 2>err &&
+		test_i18ngrep "fatal: --merge-base only works with commits" err
+	'
+
+	test_expect_success "$cmd --merge-base with no merge bases and two commits" '
+		test_must_fail git $cmd --merge-base br2 br3 2>err &&
+		test_i18ngrep "fatal: no merge base found" err
+	'
+
+	test_expect_success "$cmd --merge-base with multiple merge bases and two commits" '
+		test_must_fail git $cmd --merge-base master br1 2>err &&
+		test_i18ngrep "fatal: multiple merge bases found" err
+	'
+done
+
+test_expect_success 'diff-tree --merge-base with one commit' '
+	test_must_fail git diff-tree --merge-base master 2>err &&
+	test_i18ngrep "fatal: --merge-base only works with two commits" err
+'
+
+test_expect_success 'diff --merge-base with range' '
+	test_must_fail git diff --merge-base br2..br3 2>err &&
+	test_i18ngrep "fatal: --merge-base does not work with ranges" err
+'
+
 test_done
-- 
2.28.0.760.g8d73e04208


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* [PATCH v4 10/10] contrib/completion: complete `git diff --merge-base`
  2020-09-20 11:22     ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
                         ` (8 preceding siblings ...)
  2020-09-20 11:22       ` [PATCH v4 09/10] builtin/diff-tree: " Denton Liu
@ 2020-09-20 11:22       ` Denton Liu
  9 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-20 11:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Jeff King

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f68c8e0646..679d1ec8a8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1692,7 +1692,7 @@ __git_diff_common_options="--stat --numstat --shortstat --summary
 "
 
 __git_diff_difftool_options="--cached --staged --pickaxe-all --pickaxe-regex
-			--base --ours --theirs --no-index --relative
+			--base --ours --theirs --no-index --relative --merge-base
 			$__git_diff_common_options"
 
 _git_diff ()
-- 
2.28.0.760.g8d73e04208


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
  2020-09-20 11:01             ` Denton Liu
@ 2020-09-21 16:05               ` Junio C Hamano
  2020-09-21 17:27                 ` Denton Liu
  0 siblings, 1 reply; 58+ messages in thread
From: Junio C Hamano @ 2020-09-21 16:05 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

Denton Liu <liu.denton@gmail.com> writes:

> Hi Junio,
>
> On Fri, Sep 18, 2020 at 09:52:39AM -0700, Junio C Hamano wrote:
>> I wonder if turning the builtin/diff-tree.c to match that structure
>> make the result easier to understand (and I'll be perfectly happy if
>> the answer to this question turns out to be "no, the result of the
>> posted patch is the easiest to follow").
>
> git diff-tree does not even recognise ranges so as a result, the else
> case does not even need to exist there, unlike in git diff.

(caution: before morning caffeine so what I say may be totally off)

Do you mean "git diff-tree HEAD^..HEAD" would fail, or something
else?

Thanks.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
  2020-09-21 16:05               ` Junio C Hamano
@ 2020-09-21 17:27                 ` Denton Liu
  2020-09-21 21:09                   ` Junio C Hamano
  0 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-21 17:27 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List

Hi Junio,

On Mon, Sep 21, 2020 at 09:05:26AM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > Hi Junio,
> >
> > On Fri, Sep 18, 2020 at 09:52:39AM -0700, Junio C Hamano wrote:
> >> I wonder if turning the builtin/diff-tree.c to match that structure
> >> make the result easier to understand (and I'll be perfectly happy if
> >> the answer to this question turns out to be "no, the result of the
> >> posted patch is the easiest to follow").
> >
> > git diff-tree does not even recognise ranges so as a result, the else
> > case does not even need to exist there, unlike in git diff.
> 
> (caution: before morning caffeine so what I say may be totally off)
> 
> Do you mean "git diff-tree HEAD^..HEAD" would fail, or something
> else?

Yes, that is what I meant but I can see that what I wrote is totally
wrong. I was reading git-diff-tree.txt and I assumed that ranges were
not supported at all.

Anyway, now that I've realised my mistake, I've rewritten the diff-tree
part so that the structure matches what was written in diff and it
should be easier to follow.

-- >8 --

From: Denton Liu <liu.denton@gmail.com>
Date: Mon, 14 Sep 2020 11:36:52 -0700
Subject: [PATCH] builtin/diff-tree: learn --merge-base

The previous commit introduced ---merge-base a way to take the diff
between the working tree or index and the merge base between an arbitrary
commit and HEAD. It makes sense to extend this option to support the
case where two commits are given too and behave in a manner identical to
`git diff A...B`.

Introduce the --merge-base flag as an alternative to triple-dot
notation. Thus, we would be able to write the above as
`git diff --merge-base A B`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-diff-tree.txt      |  7 ++++-
 Documentation/git-diff.txt           |  8 ++++--
 builtin/diff-tree.c                  | 17 +++++++++++-
 builtin/diff.c                       | 39 +++++++++++++++++++---------
 t/t4068-diff-symmetric-merge-base.sh | 34 ++++++++++++++++++++++++
 5 files changed, 89 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt
index 5c8a2a5e97..2fc24c542f 100644
--- a/Documentation/git-diff-tree.txt
+++ b/Documentation/git-diff-tree.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git diff-tree' [--stdin] [-m] [-s] [-v] [--no-commit-id] [--pretty]
-	      [-t] [-r] [-c | --cc] [--combined-all-paths] [--root]
+	      [-t] [-r] [-c | --cc] [--combined-all-paths] [--root] [--merge-base]
 	      [<common diff options>] <tree-ish> [<tree-ish>] [<path>...]
 
 DESCRIPTION
@@ -43,6 +43,11 @@ include::diff-options.txt[]
 	When `--root` is specified the initial commit will be shown as a big
 	creation event. This is equivalent to a diff against the NULL tree.
 
+--merge-base::
+	Instead of comparing the <tree-ish>s directly, use the merge
+	base between the two <tree-ish>s as the "before" side.  There
+	must be two <tree-ish>s given and they must both be commits.
+
 --stdin::
 	When `--stdin` is specified, the command does not take
 	<tree-ish> arguments from the command line.  Instead, it
diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 762ee6d074..7f4c8a8ce7 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git diff' [<options>] [<commit>] [--] [<path>...]
 'git diff' [<options>] --cached [--merge-base] [<commit>] [--] [<path>...]
-'git diff' [<options>] <commit> [<commit>...] <commit> [--] [<path>...]
+'git diff' [<options>] [--merge-base] <commit> [<commit>...] <commit> [--] [<path>...]
 'git diff' [<options>] <commit>...<commit> [--] [<path>...]
 'git diff' [<options>] <blob> <blob>
 'git diff' [<options>] --no-index [--] <path> <path>
@@ -62,10 +62,14 @@ of <commit> and HEAD.  `git diff --merge-base A` is equivalent to
 	branch name to compare with the tip of a different
 	branch.
 
-'git diff' [<options>] <commit> <commit> [--] [<path>...]::
+'git diff' [<options>] [--merge-base] <commit> <commit> [--] [<path>...]::
 
 	This is to view the changes between two arbitrary
 	<commit>.
++
+If --merge-base is given, use the merge base of the two commits for the
+"before" side.  `git diff --merge-base A B` is equivalent to
+`git diff $(git merge-base A B) B`.
 
 'git diff' [<options>] <commit> <commit>... <commit> [--] [<path>...]::
 
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 802363d0a2..9fc95e959f 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -111,6 +111,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	struct setup_revision_opt s_r_opt;
 	struct userformat_want w;
 	int read_stdin = 0;
+	int merge_base = 0;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage(diff_tree_usage);
@@ -143,9 +144,18 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 			read_stdin = 1;
 			continue;
 		}
+		if (!strcmp(arg, "--merge-base")) {
+			merge_base = 1;
+			continue;
+		}
 		usage(diff_tree_usage);
 	}
 
+	if (read_stdin && merge_base)
+		die(_("--stdin and --merge-base are mutually exclusive"));
+	if (merge_base && opt->pending.nr != 2)
+		die(_("--merge-base only works with two commits"));
+
 	/*
 	 * NOTE!  We expect "a..b" to expand to "^a b" but it is
 	 * perfectly valid for revision range parser to yield "b ^a",
@@ -165,7 +175,12 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	case 2:
 		tree1 = opt->pending.objects[0].item;
 		tree2 = opt->pending.objects[1].item;
-		if (tree2->flags & UNINTERESTING) {
+		if (merge_base) {
+			struct object_id oid;
+
+			diff_get_merge_base(opt, &oid);
+			tree1 = lookup_object(the_repository, &oid);
+		} else if (tree2->flags & UNINTERESTING) {
 			SWAP(tree2, tree1);
 		}
 		diff_tree_oid(&tree1->oid, &tree2->oid, "", &opt->diffopt);
diff --git a/builtin/diff.c b/builtin/diff.c
index 1baea18ae0..b50fc68c2a 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -26,7 +26,7 @@
 static const char builtin_diff_usage[] =
 "git diff [<options>] [<commit>] [--] [<path>...]\n"
 "   or: git diff [<options>] --cached [<commit>] [--] [<path>...]\n"
-"   or: git diff [<options>] <commit> [<commit>...] <commit> [--] [<path>...]\n"
+"   or: git diff [<options>] <commit> [--merge-base] [<commit>...] <commit> [--] [<path>...]\n"
 "   or: git diff [<options>] <commit>...<commit>] [--] [<path>...]\n"
 "   or: git diff [<options>] <blob> <blob>]\n"
 "   or: git diff [<options>] --no-index [--] <path> <path>]\n"
@@ -172,19 +172,34 @@ static int builtin_diff_tree(struct rev_info *revs,
 			     struct object_array_entry *ent1)
 {
 	const struct object_id *(oid[2]);
-	int swap = 0;
+	struct object_id mb_oid;
+	int merge_base = 0;
 
-	if (argc > 1)
-		usage(builtin_diff_usage);
+	while (1 < argc) {
+		const char *arg = argv[1];
+		if (!strcmp(arg, "--merge-base"))
+			merge_base = 1;
+		else
+			usage(builtin_diff_usage);
+		argv++; argc--;
+	}
 
-	/*
-	 * We saw two trees, ent0 and ent1.  If ent1 is uninteresting,
-	 * swap them.
-	 */
-	if (ent1->item->flags & UNINTERESTING)
-		swap = 1;
-	oid[swap] = &ent0->item->oid;
-	oid[1 - swap] = &ent1->item->oid;
+	if (merge_base) {
+		diff_get_merge_base(revs, &mb_oid);
+		oid[0] = &mb_oid;
+		oid[1] = &revs->pending.objects[1].item->oid;
+	} else {
+		int swap = 0;
+
+		/*
+		 * We saw two trees, ent0 and ent1.  If ent1 is uninteresting,
+		 * swap them.
+		 */
+		if (ent1->item->flags & UNINTERESTING)
+			swap = 1;
+		oid[swap] = &ent0->item->oid;
+		oid[1 - swap] = &ent1->item->oid;
+	}
 	diff_tree_oid(oid[0], oid[1], "", &revs->diffopt);
 	log_tree_diff_flush(revs);
 	return 0;
diff --git a/t/t4068-diff-symmetric-merge-base.sh b/t/t4068-diff-symmetric-merge-base.sh
index 49432379cb..03487cc945 100755
--- a/t/t4068-diff-symmetric-merge-base.sh
+++ b/t/t4068-diff-symmetric-merge-base.sh
@@ -156,4 +156,38 @@ do
 	'
 done
 
+for cmd in diff-tree diff
+do
+	test_expect_success "$cmd --merge-base with two commits" '
+		git $cmd commit-C master >expect &&
+		git $cmd --merge-base br2 master >actual &&
+		test_cmp expect actual
+	'
+
+	test_expect_success "$cmd --merge-base commit and non-commit" '
+		test_must_fail git $cmd --merge-base br2 master^{tree} 2>err &&
+		test_i18ngrep "fatal: --merge-base only works with commits" err
+	'
+
+	test_expect_success "$cmd --merge-base with no merge bases and two commits" '
+		test_must_fail git $cmd --merge-base br2 br3 2>err &&
+		test_i18ngrep "fatal: no merge base found" err
+	'
+
+	test_expect_success "$cmd --merge-base with multiple merge bases and two commits" '
+		test_must_fail git $cmd --merge-base master br1 2>err &&
+		test_i18ngrep "fatal: multiple merge bases found" err
+	'
+done
+
+test_expect_success 'diff-tree --merge-base with one commit' '
+	test_must_fail git diff-tree --merge-base master 2>err &&
+	test_i18ngrep "fatal: --merge-base only works with two commits" err
+'
+
+test_expect_success 'diff --merge-base with range' '
+	test_must_fail git diff --merge-base br2..br3 2>err &&
+	test_i18ngrep "fatal: --merge-base does not work with ranges" err
+'
+
 test_done
-- 
2.28.0.760.g8d73e04208


^ permalink raw reply related	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
  2020-09-21 17:27                 ` Denton Liu
@ 2020-09-21 21:09                   ` Junio C Hamano
  2020-09-21 21:19                     ` Junio C Hamano
  2020-09-21 21:54                     ` Denton Liu
  0 siblings, 2 replies; 58+ messages in thread
From: Junio C Hamano @ 2020-09-21 21:09 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

Denton Liu <liu.denton@gmail.com> writes:

> @@ -165,7 +175,12 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
>  	case 2:
>  		tree1 = opt->pending.objects[0].item;
>  		tree2 = opt->pending.objects[1].item;
> -		if (tree2->flags & UNINTERESTING) {
> +		if (merge_base) {
> +			struct object_id oid;
> +
> +			diff_get_merge_base(opt, &oid);
> +			tree1 = lookup_object(the_repository, &oid);
> +		} else if (tree2->flags & UNINTERESTING) {
>  			SWAP(tree2, tree1);
>  		}
>  		diff_tree_oid(&tree1->oid, &tree2->oid, "", &opt->diffopt);

OK.  Handling this in that "case 2" does make sense.

However.

The above code as-is will allow something like

    git diff --merge-base A..B

and it will be taken the same as

    git diff --merge-base A B

But let's step back and think why we bother with SWAP() in the
normal case.  This is due to the possibility that A..B, which
currently is left in the pending.objects[] array as ^A B, might
someday be stored as B ^A.  If we leave that code to protect us from
the possibility, shouldn't we be protecting us from the same
"someday" for the new code, too?  

That is "git diff --merge-base A..B", when the control reaches this
part of the code, may have tree1=B tree2=^A

Which suggests that a consistently written code would look like so:

	tree1 = opt->pending.objects[0].item;
	tree2 = opt->pending.objects[1].item;

	if (tree2->flags & UNINTERESTING)
		/* 
                 * A..B currently becomes ^A B but it is perfectly
		 * ok for revision parser to leave us B ^A; detect
		 * and swap them in the original order.
		 */
		SWAP(tree2, tree1);

	if (merge_base) {
		struct object_id oid;

		diff_get_merge_base(opt, &oid);
		tree1 = lookup_object(the_repository, &oid);
	}
	diff_tree_oid(&tree1->oid, &tree2->oid, "", &opt->diffopt);
	log_tree_diff_flush(opt);

Another possibility is to error out when "--merge-base A..B" is
given, which might be simpler.  Then the code would look more like


	tree1 = ...
	tree2 = ...

	if (merge_base) {
		if ((tree1->flags | tree2->flags) & UNINTERESTING)
			die(_("use of --merge-base with A..B forbidden"));
		... get merge base and assign it to tree1 ...
	} else if (tree2->flags & UNINTERESTING) {
		SWAP();
	}

While we are at it, what happens when "--merge-base A...B" is given?

In the original code without "--merge-base", "git diff-tree A...B"
places the merge base between A and B in pending.objects[0] and B in
pending.objects[1], I think.  "git diff-tree --merge-base A...B"
would further compute the merge base between these two objects, but
luckily $(git merge-base $(merge-base A B) B) is the same as $(git
merge-base A B), so you won't get an incorrect answer from such a
request.  Is this something we want to diagnose as an error?  I am
inclined to say we should allow it (and if it hurts the user can
stop doing so) as there is no harm done.

Thanks.



^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
  2020-09-21 21:09                   ` Junio C Hamano
@ 2020-09-21 21:19                     ` Junio C Hamano
  2020-09-21 21:54                     ` Denton Liu
  1 sibling, 0 replies; 58+ messages in thread
From: Junio C Hamano @ 2020-09-21 21:19 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

Junio C Hamano <gitster@pobox.com> writes:

> Another possibility is to error out when "--merge-base A..B" is
> given, which might be simpler.  Then the code would look more like
> ...
>
> While we are at it, what happens when "--merge-base A...B" is given?
>
> ...  Is this something we want to diagnose as an error?  I am
> inclined to say we should allow it (and if it hurts the user can
> stop doing so) as there is no harm done.

My recommendation is to allow both "git --merge-base A..B" and "git
--merge-base A...B".  The discussion about A..B and SWAP() would
equally apply to builtin/diff part of the patch.  The posted patch
ignores the swap logic when --merge-base is given, but we should
apply the swap logic first and then make sure the merge_base logic
will have the oid[0] and oid[1] in the correct order.

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
  2020-09-21 21:09                   ` Junio C Hamano
  2020-09-21 21:19                     ` Junio C Hamano
@ 2020-09-21 21:54                     ` Denton Liu
  2020-09-21 22:18                       ` Junio C Hamano
  1 sibling, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-21 21:54 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List

Hi Junio,

On Mon, Sep 21, 2020 at 02:09:24PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > @@ -165,7 +175,12 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
> >  	case 2:
> >  		tree1 = opt->pending.objects[0].item;
> >  		tree2 = opt->pending.objects[1].item;
> > -		if (tree2->flags & UNINTERESTING) {
> > +		if (merge_base) {
> > +			struct object_id oid;
> > +
> > +			diff_get_merge_base(opt, &oid);
> > +			tree1 = lookup_object(the_repository, &oid);
> > +		} else if (tree2->flags & UNINTERESTING) {
> >  			SWAP(tree2, tree1);
> >  		}
> >  		diff_tree_oid(&tree1->oid, &tree2->oid, "", &opt->diffopt);
> 
> OK.  Handling this in that "case 2" does make sense.
> 
> However.
> 
> The above code as-is will allow something like
> 
>     git diff --merge-base A..B
> 
> and it will be taken the same as
> 
>     git diff --merge-base A B

This does not happen because at the top of diff_get_merge_base(), we
have

	for (i = 0; i < revs->pending.nr; i++) {
		struct object *obj = revs->pending.objects[i].item;
		if (obj->flags)
			die(_("--merge-base does not work with ranges"));
		if (obj->type != OBJ_COMMIT)
			die(_("--merge-base only works with commits"));
	}

which ensures that we don't accept any ranges at all. This is why I
considered the SWAP and merge_base cases to be mutually exclusive.

> Another possibility is to error out when "--merge-base A..B" is
> given, which might be simpler.  Then the code would look more like
> 
> 
> 	tree1 = ...
> 	tree2 = ...
> 
> 	if (merge_base) {
> 		if ((tree1->flags | tree2->flags) & UNINTERESTING)
> 			die(_("use of --merge-base with A..B forbidden"));
> 		... get merge base and assign it to tree1 ...
> 	} else if (tree2->flags & UNINTERESTING) {
> 		SWAP();
> 	}

This is the route I picked, although the logic for this is in
diff_get_merge_base().

> While we are at it, what happens when "--merge-base A...B" is given?
> 
> In the original code without "--merge-base", "git diff-tree A...B"
> places the merge base between A and B in pending.objects[0] and B in
> pending.objects[1], I think.  "git diff-tree --merge-base A...B"
> would further compute the merge base between these two objects, but
> luckily $(git merge-base $(merge-base A B) B) is the same as $(git
> merge-base A B), so you won't get an incorrect answer from such a
> request.  Is this something we want to diagnose as an error?  I am
> inclined to say we should allow it (and if it hurts the user can
> stop doing so) as there is no harm done.

I think that we should error out for all ranges because this option
semantically only really makes sense on two endpoints, not a range of
commits. Since the check is cheap to protect users from themselves, we
might as well actually do it.

Worst case, if someone has a legimitate use case for --merge-base and
ranges, we can allow it in the future, which would be easier than
removing this feature.

Thanks,
Denton

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
  2020-09-21 21:54                     ` Denton Liu
@ 2020-09-21 22:18                       ` Junio C Hamano
  2020-09-23  9:47                         ` Denton Liu
  0 siblings, 1 reply; 58+ messages in thread
From: Junio C Hamano @ 2020-09-21 22:18 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

Denton Liu <liu.denton@gmail.com> writes:

> This does not happen because at the top of diff_get_merge_base(), we
> have
>
> 	for (i = 0; i < revs->pending.nr; i++) {
> 		struct object *obj = revs->pending.objects[i].item;
> 		if (obj->flags)
> 			die(_("--merge-base does not work with ranges"));
> 		if (obj->type != OBJ_COMMIT)
> 			die(_("--merge-base only works with commits"));
> 	}
>
> which ensures that we don't accept any ranges at all.

I think we should lose that loop, or at least the first test.

If we are not removing the support for "A..B" notation and still
accept "diff A..B" happily, not accepting "diff --merge-base A..B"
would appear inconsistent to the users.  

The same applies to "A...B".

Thanks.


^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
  2020-09-21 22:18                       ` Junio C Hamano
@ 2020-09-23  9:47                         ` Denton Liu
  2020-09-25 21:02                           ` Junio C Hamano
  0 siblings, 1 reply; 58+ messages in thread
From: Denton Liu @ 2020-09-23  9:47 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List

Hi Junio,

On Mon, Sep 21, 2020 at 03:18:06PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > This does not happen because at the top of diff_get_merge_base(), we
> > have
> >
> > 	for (i = 0; i < revs->pending.nr; i++) {
> > 		struct object *obj = revs->pending.objects[i].item;
> > 		if (obj->flags)
> > 			die(_("--merge-base does not work with ranges"));
> > 		if (obj->type != OBJ_COMMIT)
> > 			die(_("--merge-base only works with commits"));
> > 	}
> >
> > which ensures that we don't accept any ranges at all.
> 
> I think we should lose that loop, or at least the first test.
> 
> If we are not removing the support for "A..B" notation and still
> accept "diff A..B" happily, not accepting "diff --merge-base A..B"
> would appear inconsistent to the users.  

I disagree, in the documentation, it clearly states that this option is
only available to the diff modes that accept endpoints, not
ranges:

	'git diff' [<options>] --cached [--merge-base] [<commit>] [--] [<path>...]::

and

	'git diff' [<options>] [--merge-base] <commit> <commit> [--] [<path>...]::

so it seems perfectly consistent to me. The documentation gives the
impression that the range notations are their own separate mode anyway.

And worst case scenario, if we receive user reports that they believe
the feature is inconsistent, it's 100x easier to change it to allow
ranges than attempting to remove support for ranges in the future.

Thanks,
Denton

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
  2020-09-23  9:47                         ` Denton Liu
@ 2020-09-25 21:02                           ` Junio C Hamano
  2020-09-26  1:52                             ` Denton Liu
  0 siblings, 1 reply; 58+ messages in thread
From: Junio C Hamano @ 2020-09-25 21:02 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

Denton Liu <liu.denton@gmail.com> writes:

> And worst case scenario, if we receive user reports that they believe
> the feature is inconsistent, it's 100x easier to change it to allow
> ranges than attempting to remove support for ranges in the future.

If we allow ranges from day one, we do not even have to worry about
it, no?

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v3 09/10] builtin/diff-tree: learn --merge-base
  2020-09-25 21:02                           ` Junio C Hamano
@ 2020-09-26  1:52                             ` Denton Liu
  0 siblings, 0 replies; 58+ messages in thread
From: Denton Liu @ 2020-09-26  1:52 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List

Hi Junio,

On Fri, Sep 25, 2020 at 02:02:11PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > And worst case scenario, if we receive user reports that they believe
> > the feature is inconsistent, it's 100x easier to change it to allow
> > ranges than attempting to remove support for ranges in the future.
> 
> If we allow ranges from day one, we do not even have to worry about
> it, no?

Yes, but I'm worried that being able to mix --merge-base with ranges
might cause more confusion for users since, in my opinion, it only
really makes sense for endpoints. That's why I restricted it in the
first place.

I think that since we're in disagreement, it makes more sense to take
the safer option where we can implement functionality later whereas if
we implement it and we want to remove it later, it'll be a much harder
time.

Thanks,
Denton

^ permalink raw reply	[flat|nested] 58+ messages in thread

* Re: [PATCH v4 08/10] builtin/diff-index: learn --merge-base
  2020-09-20 11:22       ` [PATCH v4 08/10] builtin/diff-index: learn --merge-base Denton Liu
@ 2020-09-29 19:53         ` Martin Ågren
  0 siblings, 0 replies; 58+ messages in thread
From: Martin Ågren @ 2020-09-29 19:53 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List, Junio C Hamano, Jeff King

Hi Denton,

On Sun, 20 Sep 2020 at 13:24, Denton Liu <liu.denton@gmail.com> wrote:
> --- a/Documentation/git-diff-index.txt
> +++ b/Documentation/git-diff-index.txt

> +--merge-base::
> +       Instead of comparing <tree-ish> directly, use the merge base
> +       between <tree-ish> and HEAD instead.  <tree-ish> must be a
> +       commit.

If you end up rerolling this patch series for other reasons, you might
want to consider using `backticks` around `<tree-ish>` and `HEAD` so
they get typeset as monospace.

>         If HEAD does not exist (e.g. unborn branches) and
>         <commit> is not given, it shows all staged changes.
>         --staged is a synonym of --cached.
> ++
> +If --merge-base is given, instead of using <commit>, use the merge base
> +of <commit> and HEAD.  `git diff --merge-base A` is equivalent to
> +`git diff $(git merge-base A HEAD)`.

Similarly here. (I realize there are existing offenders. I think it's
fine or even preferable to leave them as they are so that you don't get
lost in a huge while-at-it.) This also applies to the next patch which
touches git-diff-tree.txt.


Martin

^ permalink raw reply	[flat|nested] 58+ messages in thread

end of thread, other threads:[~2020-09-29 19:53 UTC | newest]

Thread overview: 58+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-05 19:08 [PATCH 0/4] builtin/diff: learn --merge-base Denton Liu
2020-09-05 19:08 ` [PATCH 1/4] t4068: remove unnecessary >tmp Denton Liu
2020-09-05 19:08 ` [PATCH 2/4] git-diff.txt: backtick quote command text Denton Liu
2020-09-05 19:08 ` [PATCH 3/4] builtin/diff: parse --no-index using parse_options() Denton Liu
2020-09-05 19:08 ` [PATCH 4/4] builtin/diff: learn --merge-base Denton Liu
2020-09-06 21:58   ` Junio C Hamano
2020-09-10  7:32 ` [PATCH v2 0/4] " Denton Liu
2020-09-10  7:32   ` [PATCH v2 1/4] t4068: remove unnecessary >tmp Denton Liu
2020-09-10  7:32   ` [PATCH v2 2/4] git-diff.txt: backtick quote command text Denton Liu
2020-09-10  7:32   ` [PATCH v2 3/4] builtin/diff: parse --no-index using parse_options() Denton Liu
2020-09-10 18:35     ` Junio C Hamano
2020-09-13  8:31       ` Denton Liu
2020-09-13 21:45         ` Junio C Hamano
2020-09-10  7:32   ` [PATCH v2 4/4] builtin/diff: learn --merge-base Denton Liu
2020-09-17  7:44   ` [PATCH v3 00/10] " Denton Liu
2020-09-17  7:44     ` [PATCH v3 01/10] t4068: remove unnecessary >tmp Denton Liu
2020-09-17  7:44     ` [PATCH v3 02/10] git-diff-index.txt: make --cached description a proper sentence Denton Liu
2020-09-17  7:44     ` [PATCH v3 03/10] git-diff.txt: backtick quote command text Denton Liu
2020-09-17  7:44     ` [PATCH v3 04/10] contrib/completion: extract common diff/difftool options Denton Liu
2020-09-17  7:44     ` [PATCH v3 05/10] diff-lib: accept option flags in run_diff_index() Denton Liu
2020-09-17 17:00       ` Junio C Hamano
2020-09-17  7:44     ` [PATCH v3 06/10] diff-lib: define diff_get_merge_base() Denton Liu
2020-09-17 17:16       ` Junio C Hamano
2020-09-18 10:34         ` Denton Liu
2020-09-19  0:33           ` Junio C Hamano
2020-09-17  7:44     ` [PATCH v3 07/10] t4068: add --merge-base tests Denton Liu
2020-09-17  7:44     ` [PATCH v3 08/10] builtin/diff-index: learn --merge-base Denton Liu
2020-09-17 17:28       ` Junio C Hamano
2020-09-17 18:13         ` Jeff King
2020-09-18  5:11           ` Junio C Hamano
2020-09-18 18:12             ` Jeff King
2020-09-17  7:44     ` [PATCH v3 09/10] builtin/diff-tree: " Denton Liu
2020-09-17 18:23       ` Junio C Hamano
2020-09-18 10:48         ` Denton Liu
2020-09-18 16:52           ` Junio C Hamano
2020-09-20 11:01             ` Denton Liu
2020-09-21 16:05               ` Junio C Hamano
2020-09-21 17:27                 ` Denton Liu
2020-09-21 21:09                   ` Junio C Hamano
2020-09-21 21:19                     ` Junio C Hamano
2020-09-21 21:54                     ` Denton Liu
2020-09-21 22:18                       ` Junio C Hamano
2020-09-23  9:47                         ` Denton Liu
2020-09-25 21:02                           ` Junio C Hamano
2020-09-26  1:52                             ` Denton Liu
2020-09-17  7:44     ` [PATCH v3 10/10] contrib/completion: complete `git diff --merge-base` Denton Liu
2020-09-20 11:22     ` [PATCH v4 00/10] builtin/diff: learn --merge-base Denton Liu
2020-09-20 11:22       ` [PATCH v4 01/10] t4068: remove unnecessary >tmp Denton Liu
2020-09-20 11:22       ` [PATCH v4 02/10] git-diff-index.txt: make --cached description a proper sentence Denton Liu
2020-09-20 11:22       ` [PATCH v4 03/10] git-diff.txt: backtick quote command text Denton Liu
2020-09-20 11:22       ` [PATCH v4 04/10] contrib/completion: extract common diff/difftool options Denton Liu
2020-09-20 11:22       ` [PATCH v4 05/10] diff-lib: accept option flags in run_diff_index() Denton Liu
2020-09-20 11:22       ` [PATCH v4 06/10] diff-lib: define diff_get_merge_base() Denton Liu
2020-09-20 11:22       ` [PATCH v4 07/10] t4068: add --merge-base tests Denton Liu
2020-09-20 11:22       ` [PATCH v4 08/10] builtin/diff-index: learn --merge-base Denton Liu
2020-09-29 19:53         ` Martin Ågren
2020-09-20 11:22       ` [PATCH v4 09/10] builtin/diff-tree: " Denton Liu
2020-09-20 11:22       ` [PATCH v4 10/10] contrib/completion: complete `git diff --merge-base` Denton Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).