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

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

[Peter Baumann]

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.

Signed-off-by: Peter Baumann <siprbaum@stud.informatik.uni-erlangen.de>
---

I was bitten by this during explaining a total git newbie the index and
the several ways to diff index <-> wd, index <-> commit, commit <-> commit.
I posted this example

 mkdir testrepo && cd testrepo
 git init
 echo foo > test.txt
 git add test.txt

 git diff --cached
 usage: git-diff <options> <rev>{0,2} -- <path>*

and was totaly shocked to see the above error message.
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.

 builtin-diff.c         |   22 +++++++++++++++++++---
 t/t4017-diff-cached.sh |   21 +++++++++++++++++++++
 2 files changed, 40 insertions(+), 3 deletions(-)
 create mode 100755 t/t4017-diff-cached.sh

diff --git a/builtin-diff.c b/builtin-diff.c
index c387ebb..aace507 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -203,11 +203,27 @@ void add_head(struct rev_info *revs)
 {
 	unsigned char sha1[20];
 	struct object *obj;
-	if (get_sha1("HEAD", sha1))
+	int ret = get_sha1("HEAD", sha1);
+	if (ret > 0)
 		return;
-	obj = parse_object(sha1);
-	if (!obj)
+
+	if (ret == 0)
+		obj = parse_object(sha1);
+	else {
+		/* HEAD exists but the branch it points to does not;
+		 * we haven't done any commit yet => create an empty tree
+		 * to make git diff --cached work
+		 */
+		obj = xcalloc(1, sizeof(struct object));
+	        obj->parsed = 1;
+		obj->type = OBJ_TREE;
+
+		pretend_sha1_file(NULL, 0, tree_type, obj->sha1);
+	}
+
+	if (!obj) {
 		return;
+	}
 	add_pending_object(revs, obj, "HEAD");
 }
 
diff --git a/t/t4017-diff-cached.sh b/t/t4017-diff-cached.sh
new file mode 100755
index 0000000..39fc32f
--- /dev/null
+++ b/t/t4017-diff-cached.sh
@@ -0,0 +1,21 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Peter Baumann
+#
+
+test_description='Test diff --cached without inital commit.
+
+'
+. ./test-lib.sh
+
+test_expect_success \
+    'setup' \
+    'echo frotz >rezrov &&
+     git-update-index --add rezrov'
+
+test_expect_success \
+    'git-diff --cached' \
+    'git-diff --cached'
+
+test_done
+
-- 
1.5.0.1.213.g509b-dirty

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

[Junio C Hamano]

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.

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.

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.

---

 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;
 			}
 		}

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

[Peter Baumann]

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;
>  			}
>  		}
>

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

[Junio C Hamano]

Peter Baumann <waste.manager@gmx.de> writes:

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

I think it is _actively wrong_ to explain that "diff --cached
shows everything which would become the next commit".  It
instills an incorrect mental model to new users.  What would
become the next commit is "git tar-tree $(git-write-tree)".  A
commit records the tree state, not difference from _the_
previous _single_ commit.

Having said that, showing an "add everything" patch when the
user says "git diff --cached" or even "git diff --cached HEAD"
on a yet-to-be-born branch might actually make sense, although I
am a bit afraid that the added inconsistency makes the command
more confusing and harder to explain at the end.

The output would become indistinguishable from the case where
your previous commit indeed was with an empty tree.  In essense,
this is about making the state before the first commit less
special.  That may or may not be a good thing, and I agree that
the preference on this may be related to what log.showroot
controls.

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

[Peter Baumann]

On Sun, Feb 25, 2007 at 12:22:15AM -0800, Junio C Hamano wrote:
> Peter Baumann <waste.manager@gmx.de> writes:
> 
> > 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.
> 
> I think it is _actively wrong_ to explain that "diff --cached
> shows everything which would become the next commit".  It
> instills an incorrect mental model to new users.  What would
> become the next commit is "git tar-tree $(git-write-tree)".  A
> commit records the tree state, not difference from _the_
> previous _single_ commit.
> 

Ok. You are obviously right here. Not the difference but the actual tree
state would become the next commit. What I meant to say that everything
which is shown in git-diff --cached would be applied to the previous
commit to get to the new tree state. And quoting from the manpage of

 git-diff [--options] --cached [<commit>] [--] [<path>...]
          This form is to view the changes you staged for the next commit
          relative to the named <commit>. Typically you would want comparison
          with the latest commit, so if you do not give <commit>, it defaults
          to HEAD.

I interpreted this to mean that it would show me the diff from an empty
commit (= empty tree state) to the index if I am in a new git repo without
a previous commit.

> Having said that, showing an "add everything" patch when the
> user says "git diff --cached" or even "git diff --cached HEAD"
> on a yet-to-be-born branch might actually make sense, although I
> am a bit afraid that the added inconsistency makes the command
> more confusing and harder to explain at the end.
> 
> The output would become indistinguishable from the case where
> your previous commit indeed was with an empty tree.  In essense,
> this is about making the state before the first commit less
> special.  That may or may not be a good thing, and I agree that
> the preference on this may be related to what log.showroot
> controls.
> 

Yes, I find the special case before the first commit really annoying for
the _porcelain_commands_. You have to explain to someone that a command
does this and that, but in the same sentence you have to say
"Oh, did I mention that it doesn't work until you have made a first commit?"

People new to git are often confused about this, because they read
the manpage and then tried it in an empty repository (or after adding
some files to it but just before the very first commit) to see how it
works. At least thats the way I was getting familiar with git. And it is in
_no_way_ obvious that git diff --cached only operates on
commits <-> index. Ok, I reallized very quickly that I have to do a commit
first, but if I hadn't glanced over the mailinglist since the very beginning
of git, it would have taken me _much_ longer to reallize this, especially with
the .. uhm .. "meaningful" error messages. Thank god (or better all you
people; you are doing a great job) that this has become much better in 1.5.0.

The plumbing commands are a diffrent story. They shouldn't do this, but
if you stay on the level of the porcelain commands, as most people and
especially newbies do, a little more onsistency would be nice.

-Peter

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

[Junio C Hamano]

Peter Baumann <waste.manager@gmx.de> writes:

> ..., but in the same sentence you have to say
> "Oh, did I mention that it doesn't work until you have made a first commit?"

Well, you do not even have to mention that if you are explaining
things correctly.  If something does its thing relative to the
latest commit, then it obviously would not work until you have
that "latest commit".  There is _no_ initial "empty" commit in
git.

I think getting people used to that concept early on would
actually avoid such confusion.  In that sense,...

> The plumbing commands are a diffrent story.

... I do not think the plumbing should be a different story.
Otherwise you would confuse people when they start learning the
plumbing.

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

[Peter Baumann]

On Sun, Feb 25, 2007 at 01:45:44AM -0800, Junio C Hamano wrote:
> Peter Baumann <waste.manager@gmx.de> writes:
> 
> > ..., but in the same sentence you have to say
> > "Oh, did I mention that it doesn't work until you have made a first commit?"
> 
> Well, you do not even have to mention that if you are explaining
> things correctly.  If something does its thing relative to the
> latest commit, then it obviously would not work until you have
> that "latest commit".  There is _no_ initial "empty" commit in
> git.
> 

Yes. I know that there is no empty commit. But at least it's me thinking
that I start with "nothing" from the beginning.

But thats how people learn git! I _know_ of three people who just want
to play with it by creating a new repo with git-init-db (yes, that was
v1.4.4.4), doing git-add etc and start wondering why git diff didn't
show what the expected. After mentioning the index they realised that
they have to specify --cached to diff to get what they want (they have
used svn). Starting from a freshly initalized repo they played some more
and actually got _bitten_ by the confussing error message. At least put
your patch with the sane error message into git, so they would have
gotten a clou what went wrong if I couldn't convince you otherwise.

But I _really_ think that making it the same behaviour as in
"log.showroot = true" to show a diff against an empty tree.

> I think getting people used to that concept early on would
> actually avoid such confusion.  In that sense,...
> 

But people will start using git in newly created repos! You don't want
to clone the kernel to just see if that new SCM system fits your needs.
You start by creating some test repos to fool around and later throw
them away. They did it that way and so was I.

Not to mention that I think the majority of the users don't even have a
_need_ to go down to the plumbing commands! At least I am totally happy
with what the porcelain offers me and I could get all my work done with
it (commit, diff, rebase, merge, log ...)

Side note: Not that it matters, but they gave up on git because they found
it to confusing (remember, it was v1.4.4.4). All those "strange" concepts
like the index, diff not working as expected and those strange errors ...
and they actually had some work to do and didn't have the time to fully
master git, so they staid with svn.

I'll try to convince them again that git is much
superiour to svn, after git-core 1.5.0 is available in debian because I
_know_ the won't bother to compile it themself. For them, a SCM is just
a tool to get the work done. But with the new git-gui I have convincing
argument to try git again and it gives them time to acomodate to the
"new" behaviour of their SCM.
And obviously the learning curve is now much flatter than in v1.4.x.

-Peter

> > The plumbing commands are a diffrent story.
> 
> ... I do not think the plumbing should be a different story.
> Otherwise you would confuse people when they start learning the
> plumbing.
> 

see above

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

[Peter Baumann]

On Sun, Feb 25, 2007 at 11:24:31AM +0100, Peter Baumann wrote:
> On Sun, Feb 25, 2007 at 01:45:44AM -0800, Junio C Hamano wrote:
> > Peter Baumann <waste.manager@gmx.de> writes:
> > 
> > > ..., but in the same sentence you have to say
> > > "Oh, did I mention that it doesn't work until you have made a first commit?"
> > 
> > Well, you do not even have to mention that if you are explaining
> > things correctly.  If something does its thing relative to the
> > latest commit, then it obviously would not work until you have
> > that "latest commit".  There is _no_ initial "empty" commit in
> > git.
> > 
> 
> Yes. I know that there is no empty commit. But at least it's me thinking
> that I start with "nothing" from the beginning.
> 
> But thats how people learn git! I _know_ of three people who just want
> to play with it by creating a new repo with git-init-db (yes, that was
> v1.4.4.4), doing git-add etc and start wondering why git diff didn't
> show what the expected. After mentioning the index they realised that
> they have to specify --cached to diff to get what they want (they have
> used svn). Starting from a freshly initalized repo they played some more
> and actually got _bitten_ by the confussing error message. At least put
> your patch with the sane error message into git, so they would have
> gotten a clou what went wrong if I couldn't convince you otherwise.
> 
> But I _really_ think that making it the same behaviour as in
> "log.showroot = true" to show a diff against an empty tree.
                                                            ^
                                                            |
should be the default  --------------------------------------

> 
> > I think getting people used to that concept early on would
> > actually avoid such confusion.  In that sense,...
> > 
> 
> But people will start using git in newly created repos! You don't want
> to clone the kernel to just see if that new SCM system fits your needs.
> You start by creating some test repos to fool around and later throw
> them away. They did it that way and so was I.
> 
> Not to mention that I think the majority of the users don't even have a
> _need_ to go down to the plumbing commands! At least I am totally happy
> with what the porcelain offers me and I could get all my work done with
> it (commit, diff, rebase, merge, log ...)
> 
> Side note: Not that it matters, but they gave up on git because they found
> it to confusing (remember, it was v1.4.4.4). All those "strange" concepts
> like the index, diff not working as expected and those strange errors ...
> and they actually had some work to do and didn't have the time to fully
> master git, so they staid with svn.
> 
> I'll try to convince them again that git is much
> superiour to svn, after git-core 1.5.0 is available in debian because I
> _know_ the won't bother to compile it themself. For them, a SCM is just
> a tool to get the work done. But with the new git-gui I have convincing
> argument to try git again and it gives them time to acomodate to the
> "new" behaviour of their SCM.
> And obviously the learning curve is now much flatter than in v1.4.x.
> 
> -Peter
> 
> > > The plumbing commands are a diffrent story.
> > 
> > ... I do not think the plumbing should be a different story.
> > Otherwise you would confuse people when they start learning the
> > plumbing.
> > 
> 
> see above