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

[Jonathan Nieder <jrnieder@gmail.com>]

Nguyễn Thái Ngọc Duy wrote:

>  I have a ritual of doing "git dic" (short for diff --cached) before
>  committing and does not want to break it, even on new repos.
> 
>  Looks like a good thing and no harm to the rest of the world.

This explanation belongs in the commit message, methinks.

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

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

fail?

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