[PATCH] Make 'diff C^!' show the same diff as 'show C'

[PATCH] Make 'diff C^!' show the same diff as 'show C'

[Thomas Rast]

Ideally, we'd like 'git diff C^!' to show the same diff that 'git show C'
does (with log.showroot enabled).  This gives easy access to a readable
diff for the commit, irrespective of how many parents it has and without
any trickery to remove the commit message from the git-show output.

cmd_diff relied on telling the various diff invocations apart from
only the number of revisions parsed by setup_revision() (with a twist
for A...B).  In the case of C^! this failed on two counts:

* If C has no parents, setup_revision() turns C^! into simply C.  This
  meant that 'git diff C^!' compared the current worktree to C, which
  is certainly not what the user asked for.

* Otherwise setup_revision puts C itself last, i.e., the rev.pending
  are ^C^1 ... ^C^N C.  So the first revision is uninteresting and in
  the case of exactly two parents, the symmetric difference revspec
  (diff A...B) case fired, and compared C only to C^1 (instead of
  showing a combined diff).

Detect the presence of A...B or C^! style arguments before running
setup_revisions(), so that we know in which case we are in.  We can
then dispatch to the right case without peeking at UNINTERESTING
flags.

There's still some complication in builtin_diff_combined() because
0fe7c1d (built-in diff: assorted updates., 2006-04-29) advertises that
'git diff T0 T1 ... Tn' does a combined diff of arbitrary trees where T0
is the merge result, so we have to handle both this case and C^!.

Note that UNINTERESTING is not a good criterion at all, as it is
tacked onto *trees*; if any of the involved revisions share the same
trees, the flags will overwrite each other.

Thanks to Abhijit "crab" Menon-Sen for noticing that 'diff C^!' didn't
work as expected on root commits, and Björn "doener" Steinbrink for
helpful discussions.
---

Error checking is still iffy, but I'm not sure that can be fixed
without throwing out the whole "argument parsing through
setup_revisions" code and handrolling it.


 Documentation/git-diff.txt  |   10 +++++++-
 builtin-diff.c              |   54 +++++++++++++++++++++++++++++++++++-------
 t/t4013-diff-various.sh     |    3 ++
 t/t4013/diff.diff_initial^! |   28 ++++++++++++++++++++++
 t/t4013/diff.diff_master^!  |   29 +++++++++++++++++++++++
 t/t4013/diff.diff_side^!    |   32 +++++++++++++++++++++++++
 6 files changed, 146 insertions(+), 10 deletions(-)
 create mode 100644 t/t4013/diff.diff_initial^!
 create mode 100644 t/t4013/diff.diff_master^!
 create mode 100644 t/t4013/diff.diff_side^!

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 0ac7112..5d0f2a6 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -62,9 +62,17 @@ forced by --no-index.
 	"git diff $(git-merge-base A B) B".  You can omit any one
 	of <commit>, which has the same effect as using HEAD instead.
 
+'git diff' [--options] <commit>^{caret}! [--] [<path>...]::
+
+	This shows the changes that <commit> made relative to its
+	parents.  For an ordinary commit it is the same as `git diff
+	<commit>{caret} <commit>`.  For a root commit it shows a
+	creation patch and for a merge commit it shows a combined
+	diff.
+
 Just in case if you are doing something exotic, it should be
 noted that all of the <commit> in the above description, except
-for the last two forms that use ".." notations, can be any
+for the two forms that use ".." notations, can be any
 <tree-ish>.
 
 For a more complete list of ways to spell <commit>, see
diff --git a/builtin-diff.c b/builtin-diff.c
index ffcdd05..285bf29 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -163,10 +163,17 @@ static int builtin_diff_tree(struct rev_info *revs,
 	return 0;
 }
 
+enum diff_mode {
+	DIFF_MODE_NORMAL,
+	DIFF_MODE_SYMMETRIC,
+	DIFF_MODE_SHOW
+};
+
 static int builtin_diff_combined(struct rev_info *revs,
 				 int argc, const char **argv,
 				 struct object_array_entry *ent,
-				 int ents)
+				 int ents,
+				 enum diff_mode mode)
 {
 	const unsigned char (*parent)[20];
 	int i;
@@ -177,8 +184,18 @@ static int builtin_diff_combined(struct rev_info *revs,
 	if (!revs->dense_combined_merges && !revs->combine_merges)
 		revs->dense_combined_merges = revs->combine_merges = 1;
 	parent = xmalloc(ents * sizeof(*parent));
-	for (i = 0; i < ents; i++)
-		hashcpy((unsigned char *)(parent + i), ent[i].item->sha1);
+
+	if (mode == DIFF_MODE_SHOW) {
+		/* diff C^!, we exploit knowledge that C is last */
+		for (i = 1; i < ents; i++)
+			hashcpy((unsigned char *)(parent + i),
+				ent[i-1].item->sha1);
+		hashcpy((unsigned char *)(parent),
+			ent[ents-1].item->sha1);
+	} else {
+		for (i = 0; i < ents; i++)
+			hashcpy((unsigned char *)(parent + i), ent[i].item->sha1);
+	}
 	diff_tree_combined(parent[0], parent + 1, ents - 1,
 			   revs->dense_combined_merges, revs);
 	return 0;
@@ -254,6 +271,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	struct blobinfo blob[2];
 	int nongit;
 	int result = 0;
+	enum diff_mode mode = DIFF_MODE_NORMAL;
 
 	/*
 	 * We could get N tree-ish in the rev.pending_objects list.
@@ -292,6 +310,17 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	/* Otherwise, we are doing the usual "git" diff */
 	rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
 
+	for (i = 1; i < argc; i++) {
+		if (prefixcmp(argv[i], "--")) {
+			if (strstr(argv[i], "..."))
+				mode = DIFF_MODE_SYMMETRIC;
+			else if (strstr(argv[i], "^!"))
+				mode = DIFF_MODE_SHOW;
+		} else if (!strcmp(argv[i], "--")) {
+			break;
+		}
+	}
+
 	/* Default to let external and textconv be used */
 	DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);
 	DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
@@ -403,11 +432,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	}
 	else if (blobs)
 		usage(builtin_diff_usage);
-	else if (ents == 1)
-		result = builtin_diff_index(&rev, argc, argv);
-	else if (ents == 2)
-		result = builtin_diff_tree(&rev, argc, argv, ent);
-	else if ((ents == 3) && (ent[0].item->flags & UNINTERESTING)) {
+	else if (mode == DIFF_MODE_SYMMETRIC) {
 		/* diff A...B where there is one sane merge base between
 		 * A and B.  We have ent[0] == merge-base, ent[1] == A,
 		 * and ent[2] == B.  Show diff between the base and B.
@@ -415,9 +440,20 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 		ent[1] = ent[2];
 		result = builtin_diff_tree(&rev, argc, argv, ent);
 	}
+	else if (ents == 1 && mode == DIFF_MODE_SHOW) {
+		/* diff R^! where R is a root commit: creation patch */
+		diff_tree_sha1((const unsigned char *) EMPTY_TREE_SHA1_BIN,
+			       ent[0].item->sha1, "", &rev.diffopt);
+		log_tree_diff_flush(&rev);
+		result = 0;
+	}
+	else if (ents == 1)
+		result = builtin_diff_index(&rev, argc, argv);
+	else if (ents == 2)
+		result = builtin_diff_tree(&rev, argc, argv, ent);
 	else
 		result = builtin_diff_combined(&rev, argc, argv,
-					     ent, ents);
+					       ent, ents, mode);
 	result = diff_result_code(&rev.diffopt, result);
 	if (1 < rev.diffopt.skip_stat_unmatch)
 		refresh_index_quietly();
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 8b33321..2ce7204 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -273,6 +273,9 @@ diff --no-index --name-status -- dir2 dir
 diff --no-index dir dir3
 diff master master^ side
 diff --dirstat master~1 master~2
+diff initial^!
+diff side^!
+diff master^!
 EOF
 
 test_done
diff --git a/t/t4013/diff.diff_initial^! b/t/t4013/diff.diff_initial^!
new file mode 100644
index 0000000..22f6bb7
--- /dev/null
+++ b/t/t4013/diff.diff_initial^!
@@ -0,0 +1,28 @@
+$ git diff initial^!
+diff --git a/dir/sub b/dir/sub
+new file mode 100644
+index 0000000..35d242b
+--- /dev/null
++++ b/dir/sub
+@@ -0,0 +1,2 @@
++A
++B
+diff --git a/file0 b/file0
+new file mode 100644
+index 0000000..01e79c3
+--- /dev/null
++++ b/file0
+@@ -0,0 +1,3 @@
++1
++2
++3
+diff --git a/file2 b/file2
+new file mode 100644
+index 0000000..01e79c3
+--- /dev/null
++++ b/file2
+@@ -0,0 +1,3 @@
++1
++2
++3
+$
diff --git a/t/t4013/diff.diff_master^! b/t/t4013/diff.diff_master^!
new file mode 100644
index 0000000..ca2eaa1
--- /dev/null
+++ b/t/t4013/diff.diff_master^!
@@ -0,0 +1,29 @@
+$ git diff master^!
+diff --cc dir/sub
+index cead32e,7289e35..992913c
+--- a/dir/sub
++++ b/dir/sub
+@@@ -1,6 -1,4 +1,8 @@@
+  A
+  B
+ +C
+ +D
+ +E
+ +F
++ 1
++ 2
+diff --cc file0
+index b414108,f4615da..10a8a9f
+--- a/file0
++++ b/file0
+@@@ -1,6 -1,6 +1,9 @@@
+  1
+  2
+  3
+ +4
+ +5
+ +6
++ A
++ B
++ C
+$
diff --git a/t/t4013/diff.diff_side^! b/t/t4013/diff.diff_side^!
new file mode 100644
index 0000000..6d4378a
--- /dev/null
+++ b/t/t4013/diff.diff_side^!
@@ -0,0 +1,32 @@
+$ git diff side^!
+diff --git a/dir/sub b/dir/sub
+index 35d242b..7289e35 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -1,2 +1,4 @@
+ A
+ B
++1
++2
+diff --git a/file0 b/file0
+index 01e79c3..f4615da 100644
+--- a/file0
++++ b/file0
+@@ -1,3 +1,6 @@
+ 1
+ 2
+ 3
++A
++B
++C
+diff --git a/file3 b/file3
+new file mode 100644
+index 0000000..7289e35
+--- /dev/null
++++ b/file3
+@@ -0,0 +1,4 @@
++A
++B
++1
++2
+$
-- 
1.6.4.363.g2183a

Re: [PATCH] Make 'diff C^!' show the same diff as 'show C'

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> Ideally, we'd like 'git diff C^!' to show the same diff that 'git show C'
> does (with log.showroot enabled).  This gives easy access to a readable
> diff for the commit, irrespective of how many parents it has and without
> any trickery to remove the commit message from the git-show output.

Not interested yet, as the "git show" discussion is not convincing at all.

Is the message annoying enough to warrant this change?

If that is indeed the case and if it is a common thing to ask, isn't it
more productive to teach "show" a way to do so in a simpler way than
doing, say,

	$ git show --pretty=format: HEAD

Re: [PATCH] Make 'diff C^!' show the same diff as 'show C'

[Jeff King]

On Thu, Aug 20, 2009 at 03:25:16PM -0700, Junio C Hamano wrote:

> Thomas Rast <trast@student.ethz.ch> writes:
> 
> > Ideally, we'd like 'git diff C^!' to show the same diff that 'git show C'
> > does (with log.showroot enabled).  This gives easy access to a readable
> > diff for the commit, irrespective of how many parents it has and without
> > any trickery to remove the commit message from the git-show output.
> 
> Not interested yet, as the "git show" discussion is not convincing at all.
> 
> Is the message annoying enough to warrant this change?

I thought the same thing when I saw his message, but reading further,
the current output is nonsensical. So if not this patch, we should
probably at least complain about bogus input and die (though if it is
easy to make it work, why not...).

-Peff

Re: [PATCH] Make 'diff C^!' show the same diff as 'show C'

[Thomas Rast]

Junio C Hamano wrote:
> Thomas Rast <trast@student.ethz.ch> writes:
> 
> > Ideally, we'd like 'git diff C^!' to show the same diff that 'git show C'
> > does (with log.showroot enabled).  This gives easy access to a readable
> > diff for the commit, irrespective of how many parents it has and without
> > any trickery to remove the commit message from the git-show output.
> 
> Not interested yet, as the "git show" discussion is not convincing at all.

Well, it is currently half-supported, diff doesn't complain about it,
yet it does something wildly different from what people expect in the
zero- and two-parent cases.

And yes, people expect this to work; searching the #git logs (I won't
link here as it takes a fair while even for one request) shows that
people such as Dscho, Thiago, Jakub and Björn have recommended this
syntax in the context of diff.

> Is the message annoying enough to warrant this change?
> 
> If that is indeed the case and if it is a common thing to ask, isn't it
> more productive to teach "show" a way to do so in a simpler way than
> doing, say,
> 
> 	$ git show --pretty=format: HEAD

That still doesn't get rid of the stray newline.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] Make 'diff C^!' show the same diff as 'show C'

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Aug 20, 2009 at 03:25:16PM -0700, Junio C Hamano wrote:
>
>> Thomas Rast <trast@student.ethz.ch> writes:
>> 
>> > Ideally, we'd like 'git diff C^!' to show the same diff that 'git show C'
>> > does (with log.showroot enabled).  This gives easy access to a readable
>> > diff for the commit, irrespective of how many parents it has and without
>> > any trickery to remove the commit message from the git-show output.
>> 
>> Not interested yet, as the "git show" discussion is not convincing at all.
>> 
>> Is the message annoying enough to warrant this change?
>
> I thought the same thing when I saw his message, but reading further,
> the current output is nonsensical. So if not this patch, we should
> probably at least complain about bogus input and die (though if it is
> easy to make it work, why not...).

Ok.

Re: [PATCH] Make 'diff C^!' show the same diff as 'show C'

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

>> If that is indeed the case and if it is a common thing to ask, isn't it
>> more productive to teach "show" a way to do so in a simpler way than
>> doing, say,
>> 
>> 	$ git show --pretty=format: HEAD
>
> That still doesn't get rid of the stray newline.

And the reason you don't want that newline, especially when you know there
always is one so if you really wanted to you could easily sed it out,
is...?

Re: [PATCH] Make 'diff C^!' show the same diff as 'show C'

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> * If C has no parents, setup_revision() turns C^! into simply C.  This
>   meant that 'git diff C^!' compared the current worktree to C, which
>   is certainly not what the user asked for.
>
> * Otherwise setup_revision puts C itself last, i.e., the rev.pending
>   are ^C^1 ... ^C^N C.  So the first revision is uninteresting and in
>   the case of exactly two parents, the symmetric difference revspec
>   (diff A...B) case fired, and compared C only to C^1 (instead of
>   showing a combined diff).

I actually have a vague recollection that this ugly syntax C^! (and I am
entitled to call it ugly as it was my invention) was advertised as a way
to get "diff C^..C", not the combined diff, iow, this could be deliberate
and people may depend on it.

In that light, I would say the first one (showing the root commit as root)
may be a good change, but the latter one is moderately iffy.

> @@ -292,6 +310,17 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>  	/* Otherwise, we are doing the usual "git" diff */
>  	rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
>  
> +	for (i = 1; i < argc; i++) {
> +		if (prefixcmp(argv[i], "--")) {
> +			if (strstr(argv[i], "..."))
> +				mode = DIFF_MODE_SYMMETRIC;
> +			else if (strstr(argv[i], "^!"))
> +				mode = DIFF_MODE_SHOW;
> +		} else if (!strcmp(argv[i], "--")) {
> +			break;
> +		}
> +	}
> +

This is too ugly beyond words.  

We already mark the left side commit "..." with SYMMETRIC_LEFT bit, so you
should be able to detect it from the setup_revisions() result.  If we were
to formerly add some special meaning (other than being a short-hand of
^C^n C) to the ugly C^! syntax, I would suggest marking the result of in a
similar way to allow you to detect it from the result.

But I do mean moderately strong negativeness when I say "if we _were_"
above.

As far as I recall, there were only two reasons C^! was invented for
(actually, one that was invented for, and another that was found to be
useful).

One was so that you can say "The traversal should stop around this commit,
but I want the commit itself to be included in the result".

	$ git log v1.6.3^! v1.6.3.1

This is much less useful than it used to be back when C^! was invented, as
we can ask for --boundary these days.

The other utility later found was "to view diff with its first parent".  I
think it is a useful mode of operation, and we should add a simpler way to
ask for it to "git show", as people often ask to view a merge not in the
combined way, but a simple diff relative to the merged-to commit, to get
an overview of the work done by the side branch.

So if anything, I'm actually for deprecating C^! syntax and removing it
before we hit 1.7.0.

Re: [PATCH] Make 'diff C^!' show the same diff as 'show C'

[Junio C Hamano]

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

> We already mark the left side commit "..." with SYMMETRIC_LEFT bit, so you
> should be able to detect it from the setup_revisions() result.  If we were
> to formerly add some special meaning (other than being a short-hand of

blush.  I meant "formally".

> ^C^n C) to the ugly C^! syntax, I would suggest marking the result of in a
> similar way to allow you to detect it from the result.
>
> But I do mean moderately strong negativeness when I say "if we _were_"
> above.