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

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Oct 29, 2010, 11:00 UTC
Message-ID: <AANLkTikDhABbnT-tV-5WnUk1HaTzOfEywM1C0gJZ=bhk@mail.gmail.com>
In-Reply-To: <20101029101959.GA27662@burratino>

2010/10/29 Jonathan Nieder <jrnieder@gmail.com>:

Show 19 quoted lines

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

Show 14 quoted lines

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

Show 9 quoted lines

>
> (1a) and (1b) are the only useful interpretations.  So for simplicity,
> would it make sense to drop the "if ()" for --root and make
>
>> +test_expect_success 'diff --cached' '
>> +     test_must_fail git diff --cached
>> +'
>
> fail?

All for simpler, yes.

Show 8 quoted lines

>
>> +                                     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