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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 29, 2010, 10:19 UTC
Message-ID: <20101029101959.GA27662@burratino>
In-Reply-To: <1288346087-20263-1-git-send-email-pclouds@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.

Show 12 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. "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).