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

2010/10/29 Jonathan Nieder [off-list ref]:

quoted

--- a/builtin/diff.c
+++ b/builtin/diff.c

@@ -330,8 +330,13 @@ 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 object *obj;
+                                     if (!rev.show_root_diff)
+                                             die("No HEAD commit to compare with (yet)");

How does this condition get tripped?  The code allowing "[log]
showroot" to be set to false is only invoked by the log family of
commands.

Using --root as the backward-compatibility option seems like
an abuse of language, anyway.

Hmm.. I thought --root was supported by all diff command family. Now I
think of it, only "diff-tree --root" makes sense.

 "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.

If you commit to an unborn branch, it would become the first commit of
that branch. So by (1a), it should show what is to be commited, isn't
it?

(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?

All for simpler, yes.

quoted

+                                     obj = (struct object*)lookup_tree((unsigned char*)EMPTY_TREE_SHA1_BIN);

       struct tree *tree = lookup_tree((const unsigned char *) ...
       obj = &tree->object;

might be more clear (and robust against future layout changes).

OK

-- 
Duy