diff mbox series

[v3,08/10] builtin/diff-index: learn --merge-base

Message ID 1e4f805e570b8caad26c43b4c8293413e9128d5f.1600328335.git.liu.denton@gmail.com
State Superseded
Headers show
Series builtin/diff: learn --merge-base | expand

Commit Message

Denton Liu Sept. 17, 2020, 7:44 a.m. UTC
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(-)

Comments

Junio C Hamano Sept. 17, 2020, 5:28 p.m. UTC | #1
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.
Jeff King Sept. 17, 2020, 6:13 p.m. UTC | #2
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
Junio C Hamano Sept. 18, 2020, 5:11 a.m. UTC | #3
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.
Jeff King Sept. 18, 2020, 6:12 p.m. UTC | #4
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
diff mbox series

Patch

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