Re: [RFC/PATCH] Fix git-diff --cached to not error out if HEAD points to a nonexistant branch

[Peter Baumann <waste.manager@gmx.de>]

On Sat, Feb 24, 2007 at 01:03:48PM -0800, Junio C Hamano wrote:
> Peter Baumann <waste.manager@gmx.de> writes:
> 
> > The documentation mentions "git-diff --cached" to see what is staged for
> > the next commit. But this failes if you haven't done any commits yet.
> > So lets fix it.
> > ...
> > ...  I am
> > not sure if this is the right fix and/or if git-diff-index
> > should also be fixed. I decided against it and let the core
> > cmd git-diff-index stay as it is now.
> 
> I think you decision here is a correct one.  The plumbing level
> command git-diff-index should error out if you do not give a
> tree to compare against.
> 
> My preference for 'git-diff --cached' issue is to fix the
> explanation.  Clearly document that --cached is to review the
> difference between any commit (we could even be more precise to
> say any tree, but I think we should say commit here, as the
> description is at the end-user level) and what is staged for the
> commit that will be created with your next 'git-commit'.  For
> convenience it defaults to 'HEAD', the latest commit on your
> current branch, because that is what people would do most often.
> 

I tend to agree, but I'd like to also have somethin in the spirit of
"log.showroot = true" which handles the diff of the first commit like
diffing against an empty tree. Why should diff --cached differ from
this? At least it is easier to explain, just mention that diff --cached
shows everything which would become the next commit.

> Until you have a commit at HEAD, there really is nothing to diff
> against.  I think "foo is a new entry, no comparison available."
> is one of the very few things that CVS got right.
> 

Hm. But a diff against nothing should show you what you have added :-)

> The bug in the current code is that we do not check if that HEAD
> is sensible when we add it as the default commit to compare
> with.  The error message coming out of the low-level diff-index
> code might be sensible if that 'HEAD' were what the user
> actually gave us, but clearly not the right error message in
> this case.
> 

Here I agree with you. But I guess you know what I would prefere.

-Peter

> ---
> 
>  builtin-diff.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/builtin-diff.c b/builtin-diff.c
> index c387ebb..67f4932 100644
> --- a/builtin-diff.c
> +++ b/builtin-diff.c
> @@ -261,6 +261,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>  				break;
>  			else if (!strcmp(arg, "--cached")) {
>  				add_head(&rev);
> +				if (!rev.pending.nr)
> +					die("No HEAD commit to compare with (yet)");
>  				break;
>  			}
>  		}
>