threads / rfc / 6941

RFC patchFix git-diff --cached to not error out if HEAD points to a nonexistant branch

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

## tl;dr

8 messages between Feb 24, 2007 and Feb 25, 2007. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Peter Baumann· Feb 24, 2007, 17:20 UTC · lore

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
Show changes to 2 files +40 −3

builtin-diff.c, 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
Junio C Hamano· Feb 24, 2007, 21:03 UTC · re: Peter Baumann · lore

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

Peter Baumann <waste.manager@gmx.de> writes:
Show 8 quoted lines
> 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(-)
Show changes to builtin-diff.c +2 −0
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;
 			}
 		}
Peter Baumann· Feb 24, 2007, 22:16 UTC · re: Junio C Hamano · lore

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

On Sat, Feb 24, 2007 at 01:03:48PM -0800, Junio C Hamano wrote:
Show 24 quoted lines
> 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 :-)
Show 7 quoted lines
> 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
Show 19 quoted lines
> ---
> 
>  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;
>  			}
>  		}
>
Junio C Hamano· Feb 25, 2007, 08:22 UTC · re: Peter Baumann · lore

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

Peter Baumann <waste.manager@gmx.de> writes:
Show 5 quoted lines
> 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.

Peter Baumann· Feb 25, 2007, 09:13 UTC · re: Junio C Hamano · lore

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

On Sun, Feb 25, 2007 at 12:22:15AM -0800, Junio C Hamano wrote:
Show 15 quoted lines
> 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.

Show 13 quoted lines
> 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
Junio C Hamano· Feb 25, 2007, 09:45 UTC · re: Peter Baumann · lore

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

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.

Peter Baumann· Feb 25, 2007, 10:24 UTC · re: Junio C Hamano · lore

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

On Sun, Feb 25, 2007 at 01:45:44AM -0800, Junio C Hamano wrote:
Show 11 quoted lines
> 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
Show 6 quoted lines
> > 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
Peter Baumann· Feb 25, 2007, 10:28 UTC · re: Peter Baumann · lore

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

On Sun, Feb 25, 2007 at 11:24:31AM +0100, Peter Baumann wrote:
Show 28 quoted lines
> 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  --------------------------------------
Show 39 quoted lines
> 
> > 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

← back to recent threads