{"thread":{"id":"6941","subject":"[RFC/PATCH] Fix git-diff --cached to not error out if HEAD points to a nonexistant branch","startedAt":"2007-02-24T17:20:37Z","lastAt":"2007-02-25T10:28:49Z","messageCount":8,"participants":["Peter Baumann","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"35400","messageId":"20070224172037.GA31963@xp.machine.xx","threadId":"6941","inReplyTo":null,"subject":"[RFC/PATCH] Fix git-diff --cached to not error out if HEAD points to a nonexistant branch","fromName":"Peter Baumann","fromEmail":"waste.manager@gmx.de","sentAt":"2007-02-24T17:20:37Z","receivedAt":"2007-02-24T17:20:37Z","isPatch":true,"sender":{"key":"waste.manager@gmx.de","avatar":null},"body":"The documentation mentions \"git-diff --cached\" to see what is staged for\nthe next commit. But this failes if you haven't done any commits yet.\nSo lets fix it.\n\nSigned-off-by: Peter Baumann <siprbaum@stud.informatik.uni-erlangen.de>\n---\n\nI was bitten by this during explaining a total git newbie the index and\nthe several ways to diff index <-> wd, index <-> commit, commit <-> commit.\nI posted this example\n\n mkdir testrepo && cd testrepo\n git init\n echo foo > test.txt\n git add test.txt\n\n git diff --cached\n usage: git-diff <options> <rev>{0,2} -- <path>*\n\nand was totaly shocked to see the above error message.\nI am not sure if this is the right fix and/or if git-diff-index\nshould also be fixed. I decided against it and let the core cmd git-diff-index\nstay as it is now.\n\n builtin-diff.c         |   22 +++++++++++++++++++---\n t/t4017-diff-cached.sh |   21 +++++++++++++++++++++\n 2 files changed, 40 insertions(+), 3 deletions(-)\n create mode 100755 t/t4017-diff-cached.sh\n\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex c387ebb..aace507 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -203,11 +203,27 @@ void add_head(struct rev_info *revs)\n {\n \tunsigned char sha1[20];\n \tstruct object *obj;\n-\tif (get_sha1(\"HEAD\", sha1))\n+\tint ret = get_sha1(\"HEAD\", sha1);\n+\tif (ret > 0)\n \t\treturn;\n-\tobj = parse_object(sha1);\n-\tif (!obj)\n+\n+\tif (ret == 0)\n+\t\tobj = parse_object(sha1);\n+\telse {\n+\t\t/* HEAD exists but the branch it points to does not;\n+\t\t * we haven't done any commit yet => create an empty tree\n+\t\t * to make git diff --cached work\n+\t\t */\n+\t\tobj = xcalloc(1, sizeof(struct object));\n+\t        obj->parsed = 1;\n+\t\tobj->type = OBJ_TREE;\n+\n+\t\tpretend_sha1_file(NULL, 0, tree_type, obj->sha1);\n+\t}\n+\n+\tif (!obj) {\n \t\treturn;\n+\t}\n \tadd_pending_object(revs, obj, \"HEAD\");\n }\n \ndiff --git a/t/t4017-diff-cached.sh b/t/t4017-diff-cached.sh\nnew file mode 100755\nindex 0000000..39fc32f\n--- /dev/null\n+++ b/t/t4017-diff-cached.sh\n@@ -0,0 +1,21 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2007 Peter Baumann\n+#\n+\n+test_description='Test diff --cached without inital commit.\n+\n+'\n+. ./test-lib.sh\n+\n+test_expect_success \\\n+    'setup' \\\n+    'echo frotz >rezrov &&\n+     git-update-index --add rezrov'\n+\n+test_expect_success \\\n+    'git-diff --cached' \\\n+    'git-diff --cached'\n+\n+test_done\n+\n-- \n1.5.0.1.213.g509b-dirty\n"},{"id":"35403","messageId":"7vvehrw9mz.fsf@assigned-by-dhcp.cox.net","threadId":"6941","inReplyTo":"20070224172037.GA31963@xp.machine.xx","subject":"Re: [RFC/PATCH] Fix git-diff --cached to not error out if HEAD points to a nonexistant branch","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-24T21:03:48Z","receivedAt":"2007-02-24T21:03:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Baumann <waste.manager@gmx.de> writes:\n\n> The documentation mentions \"git-diff --cached\" to see what is staged for\n> the next commit. But this failes if you haven't done any commits yet.\n> So lets fix it.\n> ...\n> ...  I am\n> not sure if this is the right fix and/or if git-diff-index\n> should also be fixed. I decided against it and let the core\n> cmd git-diff-index stay as it is now.\n\nI think you decision here is a correct one.  The plumbing level\ncommand git-diff-index should error out if you do not give a\ntree to compare against.\n\nMy preference for 'git-diff --cached' issue is to fix the\nexplanation.  Clearly document that --cached is to review the\ndifference between any commit (we could even be more precise to\nsay any tree, but I think we should say commit here, as the\ndescription is at the end-user level) and what is staged for the\ncommit that will be created with your next 'git-commit'.  For\nconvenience it defaults to 'HEAD', the latest commit on your\ncurrent branch, because that is what people would do most often.\n\nUntil you have a commit at HEAD, there really is nothing to diff\nagainst.  I think \"foo is a new entry, no comparison available.\"\nis one of the very few things that CVS got right.\n\nThe bug in the current code is that we do not check if that HEAD\nis sensible when we add it as the default commit to compare\nwith.  The error message coming out of the low-level diff-index\ncode might be sensible if that 'HEAD' were what the user\nactually gave us, but clearly not the right error message in\nthis case.\n\n---\n\n builtin-diff.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-diff.c b/builtin-diff.c\nindex c387ebb..67f4932 100644\n--- a/builtin-diff.c\n+++ b/builtin-diff.c\n@@ -261,6 +261,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\t\tbreak;\n \t\t\telse if (!strcmp(arg, \"--cached\")) {\n \t\t\t\tadd_head(&rev);\n+\t\t\t\tif (!rev.pending.nr)\n+\t\t\t\t\tdie(\"No HEAD commit to compare with (yet)\");\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n"},{"id":"35404","messageId":"20070224221622.GA3897@xp.machine.xx","threadId":"6941","inReplyTo":"7vvehrw9mz.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC/PATCH] Fix git-diff --cached to not error out if HEAD points to a nonexistant branch","fromName":"Peter Baumann","fromEmail":"waste.manager@gmx.de","sentAt":"2007-02-24T22:16:22Z","receivedAt":"2007-02-24T22:16:22Z","isPatch":true,"sender":{"key":"waste.manager@gmx.de","avatar":null},"body":"On Sat, Feb 24, 2007 at 01:03:48PM -0800, Junio C Hamano wrote:\n> Peter Baumann <waste.manager@gmx.de> writes:\n> \n> > The documentation mentions \"git-diff --cached\" to see what is staged for\n> > the next commit. But this failes if you haven't done any commits yet.\n> > So lets fix it.\n> > ...\n> > ...  I am\n> > not sure if this is the right fix and/or if git-diff-index\n> > should also be fixed. I decided against it and let the core\n> > cmd git-diff-index stay as it is now.\n> \n> I think you decision here is a correct one.  The plumbing level\n> command git-diff-index should error out if you do not give a\n> tree to compare against.\n> \n> My preference for 'git-diff --cached' issue is to fix the\n> explanation.  Clearly document that --cached is to review the\n> difference between any commit (we could even be more precise to\n> say any tree, but I think we should say commit here, as the\n> description is at the end-user level) and what is staged for the\n> commit that will be created with your next 'git-commit'.  For\n> convenience it defaults to 'HEAD', the latest commit on your\n> current branch, because that is what people would do most often.\n> \n\nI tend to agree, but I'd like to also have somethin in the spirit of\n\"log.showroot = true\" which handles the diff of the first commit like\ndiffing against an empty tree. Why should diff --cached differ from\nthis? At least it is easier to explain, just mention that diff --cached\nshows everything which would become the next commit.\n\n> Until you have a commit at HEAD, there really is nothing to diff\n> against.  I think \"foo is a new entry, no comparison available.\"\n> is one of the very few things that CVS got right.\n> \n\nHm. But a diff against nothing should show you what you have added :-)\n\n> The bug in the current code is that we do not check if that HEAD\n> is sensible when we add it as the default commit to compare\n> with.  The error message coming out of the low-level diff-index\n> code might be sensible if that 'HEAD' were what the user\n> actually gave us, but clearly not the right error message in\n> this case.\n> \n\nHere I agree with you. But I guess you know what I would prefere.\n\n-Peter\n\n> ---\n> \n>  builtin-diff.c |    2 ++\n>  1 files changed, 2 insertions(+), 0 deletions(-)\n> \n> diff --git a/builtin-diff.c b/builtin-diff.c\n> index c387ebb..67f4932 100644\n> --- a/builtin-diff.c\n> +++ b/builtin-diff.c\n> @@ -261,6 +261,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n>  \t\t\t\tbreak;\n>  \t\t\telse if (!strcmp(arg, \"--cached\")) {\n>  \t\t\t\tadd_head(&rev);\n> +\t\t\t\tif (!rev.pending.nr)\n> +\t\t\t\t\tdie(\"No HEAD commit to compare with (yet)\");\n>  \t\t\t\tbreak;\n>  \t\t\t}\n>  \t\t}\n>\n"},{"id":"35425","messageId":"7vvehqtzns.fsf@assigned-by-dhcp.cox.net","threadId":"6941","inReplyTo":"20070224221622.GA3897@xp.machine.xx","subject":"Re: [RFC/PATCH] Fix git-diff --cached to not error out if HEAD points to a nonexistant branch","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-25T08:22:15Z","receivedAt":"2007-02-25T08:22:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Baumann <waste.manager@gmx.de> writes:\n\n> I tend to agree, but I'd like to also have somethin in the spirit of\n> \"log.showroot = true\" which handles the diff of the first commit like\n> diffing against an empty tree. Why should diff --cached differ from\n> this? At least it is easier to explain, just mention that diff --cached\n> shows everything which would become the next commit.\n\nI think it is _actively wrong_ to explain that \"diff --cached\nshows everything which would become the next commit\".  It\ninstills an incorrect mental model to new users.  What would\nbecome the next commit is \"git tar-tree $(git-write-tree)\".  A\ncommit records the tree state, not difference from _the_\nprevious _single_ commit.\n\nHaving said that, showing an \"add everything\" patch when the\nuser says \"git diff --cached\" or even \"git diff --cached HEAD\"\non a yet-to-be-born branch might actually make sense, although I\nam a bit afraid that the added inconsistency makes the command\nmore confusing and harder to explain at the end.\n\nThe output would become indistinguishable from the case where\nyour previous commit indeed was with an empty tree.  In essense,\nthis is about making the state before the first commit less\nspecial.  That may or may not be a good thing, and I agree that\nthe preference on this may be related to what log.showroot\ncontrols.\n"},{"id":"35426","messageId":"20070225091302.GB3897@xp.machine.xx","threadId":"6941","inReplyTo":"7vvehqtzns.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC/PATCH] Fix git-diff --cached to not error out if HEAD points to a nonexistant branch","fromName":"Peter Baumann","fromEmail":"waste.manager@gmx.de","sentAt":"2007-02-25T09:13:02Z","receivedAt":"2007-02-25T09:13:02Z","isPatch":true,"sender":{"key":"waste.manager@gmx.de","avatar":null},"body":"On Sun, Feb 25, 2007 at 12:22:15AM -0800, Junio C Hamano wrote:\n> Peter Baumann <waste.manager@gmx.de> writes:\n> \n> > I tend to agree, but I'd like to also have somethin in the spirit of\n> > \"log.showroot = true\" which handles the diff of the first commit like\n> > diffing against an empty tree. Why should diff --cached differ from\n> > this? At least it is easier to explain, just mention that diff --cached\n> > shows everything which would become the next commit.\n> \n> I think it is _actively wrong_ to explain that \"diff --cached\n> shows everything which would become the next commit\".  It\n> instills an incorrect mental model to new users.  What would\n> become the next commit is \"git tar-tree $(git-write-tree)\".  A\n> commit records the tree state, not difference from _the_\n> previous _single_ commit.\n> \n\nOk. You are obviously right here. Not the difference but the actual tree\nstate would become the next commit. What I meant to say that everything\nwhich is shown in git-diff --cached would be applied to the previous\ncommit to get to the new tree state. And quoting from the manpage of\n\n git-diff [--options] --cached [<commit>] [--] [<path>...]\n          This form is to view the changes you staged for the next commit\n          relative to the named <commit>. Typically you would want comparison\n          with the latest commit, so if you do not give <commit>, it defaults\n          to HEAD.\n\nI interpreted this to mean that it would show me the diff from an empty\ncommit (= empty tree state) to the index if I am in a new git repo without\na previous commit.\n\n> Having said that, showing an \"add everything\" patch when the\n> user says \"git diff --cached\" or even \"git diff --cached HEAD\"\n> on a yet-to-be-born branch might actually make sense, although I\n> am a bit afraid that the added inconsistency makes the command\n> more confusing and harder to explain at the end.\n> \n> The output would become indistinguishable from the case where\n> your previous commit indeed was with an empty tree.  In essense,\n> this is about making the state before the first commit less\n> special.  That may or may not be a good thing, and I agree that\n> the preference on this may be related to what log.showroot\n> controls.\n> \n\nYes, I find the special case before the first commit really annoying for\nthe _porcelain_commands_. You have to explain to someone that a command\ndoes this and that, but in the same sentence you have to say\n\"Oh, did I mention that it doesn't work until you have made a first commit?\"\n\nPeople new to git are often confused about this, because they read\nthe manpage and then tried it in an empty repository (or after adding\nsome files to it but just before the very first commit) to see how it\nworks. At least thats the way I was getting familiar with git. And it is in\n_no_way_ obvious that git diff --cached only operates on\ncommits <-> index. Ok, I reallized very quickly that I have to do a commit\nfirst, but if I hadn't glanced over the mailinglist since the very beginning\nof git, it would have taken me _much_ longer to reallize this, especially with\nthe .. uhm .. \"meaningful\" error messages. Thank god (or better all you\npeople; you are doing a great job) that this has become much better in 1.5.0.\n\nThe plumbing commands are a diffrent story. They shouldn't do this, but\nif you stay on the level of the porcelain commands, as most people and\nespecially newbies do, a little more onsistency would be nice.\n\n-Peter\n"},{"id":"35427","messageId":"7vlkimtvsn.fsf@assigned-by-dhcp.cox.net","threadId":"6941","inReplyTo":"20070225091302.GB3897@xp.machine.xx","subject":"Re: [RFC/PATCH] Fix git-diff --cached to not error out if HEAD points to a nonexistant branch","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-25T09:45:44Z","receivedAt":"2007-02-25T09:45:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Baumann <waste.manager@gmx.de> writes:\n\n> ..., but in the same sentence you have to say\n> \"Oh, did I mention that it doesn't work until you have made a first commit?\"\n\nWell, you do not even have to mention that if you are explaining\nthings correctly.  If something does its thing relative to the\nlatest commit, then it obviously would not work until you have\nthat \"latest commit\".  There is _no_ initial \"empty\" commit in\ngit.\n\nI think getting people used to that concept early on would\nactually avoid such confusion.  In that sense,...\n\n> The plumbing commands are a diffrent story.\n\n... I do not think the plumbing should be a different story.\nOtherwise you would confuse people when they start learning the\nplumbing.\n"},{"id":"35429","messageId":"20070225102431.GC3897@xp.machine.xx","threadId":"6941","inReplyTo":"7vlkimtvsn.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC/PATCH] Fix git-diff --cached to not error out if HEAD points to a nonexistant branch","fromName":"Peter Baumann","fromEmail":"waste.manager@gmx.de","sentAt":"2007-02-25T10:24:31Z","receivedAt":"2007-02-25T10:24:31Z","isPatch":true,"sender":{"key":"waste.manager@gmx.de","avatar":null},"body":"On Sun, Feb 25, 2007 at 01:45:44AM -0800, Junio C Hamano wrote:\n> Peter Baumann <waste.manager@gmx.de> writes:\n> \n> > ..., but in the same sentence you have to say\n> > \"Oh, did I mention that it doesn't work until you have made a first commit?\"\n> \n> Well, you do not even have to mention that if you are explaining\n> things correctly.  If something does its thing relative to the\n> latest commit, then it obviously would not work until you have\n> that \"latest commit\".  There is _no_ initial \"empty\" commit in\n> git.\n> \n\nYes. I know that there is no empty commit. But at least it's me thinking\nthat I start with \"nothing\" from the beginning.\n\nBut thats how people learn git! I _know_ of three people who just want\nto play with it by creating a new repo with git-init-db (yes, that was\nv1.4.4.4), doing git-add etc and start wondering why git diff didn't\nshow what the expected. After mentioning the index they realised that\nthey have to specify --cached to diff to get what they want (they have\nused svn). Starting from a freshly initalized repo they played some more\nand actually got _bitten_ by the confussing error message. At least put\nyour patch with the sane error message into git, so they would have\ngotten a clou what went wrong if I couldn't convince you otherwise.\n\nBut I _really_ think that making it the same behaviour as in\n\"log.showroot = true\" to show a diff against an empty tree.\n\n> I think getting people used to that concept early on would\n> actually avoid such confusion.  In that sense,...\n> \n\nBut people will start using git in newly created repos! You don't want\nto clone the kernel to just see if that new SCM system fits your needs.\nYou start by creating some test repos to fool around and later throw\nthem away. They did it that way and so was I.\n\nNot to mention that I think the majority of the users don't even have a\n_need_ to go down to the plumbing commands! At least I am totally happy\nwith what the porcelain offers me and I could get all my work done with\nit (commit, diff, rebase, merge, log ...)\n\nSide note: Not that it matters, but they gave up on git because they found\nit to confusing (remember, it was v1.4.4.4). All those \"strange\" concepts\nlike the index, diff not working as expected and those strange errors ...\nand they actually had some work to do and didn't have the time to fully\nmaster git, so they staid with svn.\n\nI'll try to convince them again that git is much\nsuperiour to svn, after git-core 1.5.0 is available in debian because I\n_know_ the won't bother to compile it themself. For them, a SCM is just\na tool to get the work done. But with the new git-gui I have convincing\nargument to try git again and it gives them time to acomodate to the\n\"new\" behaviour of their SCM.\nAnd obviously the learning curve is now much flatter than in v1.4.x.\n\n-Peter\n\n> > The plumbing commands are a diffrent story.\n> \n> ... I do not think the plumbing should be a different story.\n> Otherwise you would confuse people when they start learning the\n> plumbing.\n> \n\nsee above\n"},{"id":"35430","messageId":"20070225102849.GD3897@xp.machine.xx","threadId":"6941","inReplyTo":"20070225102431.GC3897@xp.machine.xx","subject":"Re: [RFC/PATCH] Fix git-diff --cached to not error out if HEAD points to a nonexistant branch","fromName":"Peter Baumann","fromEmail":"waste.manager@gmx.de","sentAt":"2007-02-25T10:28:49Z","receivedAt":"2007-02-25T10:28:49Z","isPatch":true,"sender":{"key":"waste.manager@gmx.de","avatar":null},"body":"On Sun, Feb 25, 2007 at 11:24:31AM +0100, Peter Baumann wrote:\n> On Sun, Feb 25, 2007 at 01:45:44AM -0800, Junio C Hamano wrote:\n> > Peter Baumann <waste.manager@gmx.de> writes:\n> > \n> > > ..., but in the same sentence you have to say\n> > > \"Oh, did I mention that it doesn't work until you have made a first commit?\"\n> > \n> > Well, you do not even have to mention that if you are explaining\n> > things correctly.  If something does its thing relative to the\n> > latest commit, then it obviously would not work until you have\n> > that \"latest commit\".  There is _no_ initial \"empty\" commit in\n> > git.\n> > \n> \n> Yes. I know that there is no empty commit. But at least it's me thinking\n> that I start with \"nothing\" from the beginning.\n> \n> But thats how people learn git! I _know_ of three people who just want\n> to play with it by creating a new repo with git-init-db (yes, that was\n> v1.4.4.4), doing git-add etc and start wondering why git diff didn't\n> show what the expected. After mentioning the index they realised that\n> they have to specify --cached to diff to get what they want (they have\n> used svn). Starting from a freshly initalized repo they played some more\n> and actually got _bitten_ by the confussing error message. At least put\n> your patch with the sane error message into git, so they would have\n> gotten a clou what went wrong if I couldn't convince you otherwise.\n> \n> But I _really_ think that making it the same behaviour as in\n> \"log.showroot = true\" to show a diff against an empty tree.\n                                                            ^\n                                                            |\nshould be the default  --------------------------------------\n\n> \n> > I think getting people used to that concept early on would\n> > actually avoid such confusion.  In that sense,...\n> > \n> \n> But people will start using git in newly created repos! You don't want\n> to clone the kernel to just see if that new SCM system fits your needs.\n> You start by creating some test repos to fool around and later throw\n> them away. They did it that way and so was I.\n> \n> Not to mention that I think the majority of the users don't even have a\n> _need_ to go down to the plumbing commands! At least I am totally happy\n> with what the porcelain offers me and I could get all my work done with\n> it (commit, diff, rebase, merge, log ...)\n> \n> Side note: Not that it matters, but they gave up on git because they found\n> it to confusing (remember, it was v1.4.4.4). All those \"strange\" concepts\n> like the index, diff not working as expected and those strange errors ...\n> and they actually had some work to do and didn't have the time to fully\n> master git, so they staid with svn.\n> \n> I'll try to convince them again that git is much\n> superiour to svn, after git-core 1.5.0 is available in debian because I\n> _know_ the won't bother to compile it themself. For them, a SCM is just\n> a tool to get the work done. But with the new git-gui I have convincing\n> argument to try git again and it gives them time to acomodate to the\n> \"new\" behaviour of their SCM.\n> And obviously the learning curve is now much flatter than in v1.4.x.\n> \n> -Peter\n> \n> > > The plumbing commands are a diffrent story.\n> > \n> > ... I do not think the plumbing should be a different story.\n> > Otherwise you would confuse people when they start learning the\n> > plumbing.\n> > \n> \n> see above\n"}]}