Re: [PATCH] diff: support --root --cached combination

Jonathan Nieder [off-list ref] writes:

Using --root as the backward-compatibility option seems like
an abuse of language, anyway.  "git diff --cached" has two
meanings:

 1. show changes to be committed

    1b. show what git show --format=" " would say after a commit

 2. show differences between the index and the commit named by the
    (implicit) HEAD argument

With interpretation (1b), --root should be respected, and the output
should be empty (!), not an error, when "[log] showroot" is false.

With interpretation (2), --root should not be respected, and an
attempt to diff --cached in an unborn branch should be an error.

(1a) and (1b) are the only useful interpretations.  So for simplicity,
would it make sense to drop the "if ()" for --root and make

quoted

+test_expect_success 'diff --cached' '
+	test_must_fail git diff --cached
+'

fail?

I don't see 1a up above, but I agree.

Let's explain the patch this way (yes, I am writing a proposed commit log
message Duy should have written):

    "git diff --cached" (without revision) used to mean "git diff --cached
    HEAD" (i.e. the user was too lazy to type HEAD).  This "correctly"
    failed when there was no commit yet.  But was that correctness useful?

    This patch changes the definition of what particular command means.
    It is a request to show what _would_ be committed without further "git
    add".  The internal implementation is still the same "git diff
    --cached HEAD" when HEAD exists, but when there is no commit yet, it
    compares the index with an empty tree object to achieve the desired
    result.

Unlike "diff-index --cached HEAD" that must fail when HEAD does not name a
valid rev, we do not have to be so strict in "git diff" Porcelain,
especially when the end user does not even explicitly say HEAD.

To put it in another way, we should strive to define the behaviour of the
plumbing precisely in terms of the mechanism and machinery (e.g. if you
ask for an operation between a tree and the index, and if you incorrectly
specified the tree, you _should_ get an error, instead of a result that
somebody randomly chose, saying "we thought it would be more useful for
you this way").  But we should try to define the behaviour of the
Porcelain commands in terms of the use case and the workflow we try to
encourage and support.

"git diff --cached" (without revision) used to mean "git diff --cached
HEAD" (i.e. the user was too lazy to type HEAD). This "correctly"
failed when there was no commit yet. But was that correctness useful?

This patch changes the definition of what particular command means.
It is a request to show what _would_ be committed without further "git
add". The internal implementation is still the same "git diff
--cached HEAD" when HEAD exists, but when there is no commit yet, it
compares the index with an empty tree object to achieve the desired
result.

(Written by Junio)

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 "git diff --cached HEAD" does fail, but I don't really care.

 builtin/diff.c                      |    7 ++++-
 t/t4013-diff-various.sh             |   11 ++++++++++
 t/t4013/diff.diff_--cached          |   38 +++++++++++++++++++++++++++++++++++
 t/t4013/diff.diff_--cached_--_file0 |   15 +++++++++++++
 4 files changed, 69 insertions(+), 2 deletions(-)
 create mode 100644 t/t4013/diff.diff_--cached
 create mode 100644 t/t4013/diff.diff_--cached_--_file0

diff --git a/builtin/diff.c b/builtin/diff.c
index a43d326..d8db957 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c

@@ -330,8 +330,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 			else if (!strcmp(arg, "--cached") ||
 				 !strcmp(arg, "--staged")) {
 				add_head_to_pending(&rev);
-				if (!rev.pending.nr)
-					die("No HEAD commit to compare with (yet)");
+				if (!rev.pending.nr) {
+					struct tree *tree;
+					tree = lookup_tree((const unsigned char*)EMPTY_TREE_SHA1_BIN);
+					add_pending_object(&rev, &tree->object, "HEAD");
+				}
 				break;
 			}
 		}

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 9a66520..b8f81d0 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh

@@ -290,4 +290,15 @@ test_expect_success 'log -S requires an argument' '
 	test_must_fail git log -S
 '
 
+test_expect_success 'diff --cached on unborn branch' '
+	echo ref: refs/heads/unborn >.git/HEAD &&
+	git diff --cached >result &&
+	test_cmp "$TEST_DIRECTORY/t4013/diff.diff_--cached" result
+'
+
+test_expect_success 'diff --cached -- file on unborn branch' '
+	git diff --cached -- file0 >result &&
+	test_cmp "$TEST_DIRECTORY/t4013/diff.diff_--cached_--_file0" result
+'
+
 test_done

diff --git a/t/t4013/diff.diff_--cached b/t/t4013/diff.diff_--cached
new file mode 100644
index 0000000..ff16e83
--- /dev/null
+++ b/t/t4013/diff.diff_--cached

@@ -0,0 +1,38 @@
+diff --git a/dir/sub b/dir/sub
+new file mode 100644
+index 0000000..992913c
+--- /dev/null
++++ b/dir/sub
+@@ -0,0 +1,8 @@
++A
++B
++C
++D
++E
++F
++1
++2
+diff --git a/file0 b/file0
+new file mode 100644
+index 0000000..10a8a9f
+--- /dev/null
++++ b/file0
+@@ -0,0 +1,9 @@
++1
++2
++3
++4
++5
++6
++A
++B
++C
+diff --git a/file1 b/file1
+new file mode 100644
+index 0000000..b1e6722
+--- /dev/null
++++ b/file1
+@@ -0,0 +1,3 @@
++A
++B
++C

diff --git a/t/t4013/diff.diff_--cached_--_file0 b/t/t4013/diff.diff_--cached_--_file0
new file mode 100644
index 0000000..b9bb858
--- /dev/null
+++ b/t/t4013/diff.diff_--cached_--_file0

@@ -0,0 +1,15 @@
+diff --git a/file0 b/file0
+new file mode 100644
+index 0000000..10a8a9f
+--- /dev/null
++++ b/file0
+@@ -0,0 +1,9 @@
++1
++2
++3
++4
++5
++6
++A
++B
++C

-- 
1.7.0.2.445.gcbdb3