{"thread":{"id":"4914","subject":"print errors from git-update-ref","startedAt":"2006-07-18T13:13:48Z","lastAt":"2006-07-29T03:44:51Z","messageCount":7,"participants":["Alex Riesen","Junio C Hamano","Shawn Pearce","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"23921","messageId":"81b0412b0607180613t603551b8t865b407c40ab8aef@mail.gmail.com","threadId":"4914","inReplyTo":null,"subject":"print errors from git-update-ref","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2006-07-18T13:13:48Z","receivedAt":"2006-07-18T13:13:48Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"...otherwise it not clear what happened when update-ref fails.\n\nE.g., git checkout -b a/b/c HEAD would print nothing if refs/heads/a\nexists and is a directory (it does return 1, so scripts checking for\nreturn code should be ok).\n\nI'm attaching two patches, because I'm not quite sure where it should\nbe done: git-checkout is the least intrusive, but only builtin-update-ref.c\nhas enough info to help user to resolve the problem (errno is ENOTDIR,\nwhich is selfexplanatory). And I happen to use git-update-ref directly\nsometimes.\n\n\nFrom 5398f0ee6bab039701912fdaf784792f4cf76afe Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen \nDate: Tue, 18 Jul 2006 14:52:15 +0200\nSubject: [PATCH] update-ref: print errors\n\notherwise it not clear what happened\n\nSigned-off-by: Alex Riesen \n---\n builtin-update-ref.c | 8 ++++++--\n 1 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-update-ref.c b/builtin-update-ref.c\nindex 83094ab..ad4a44d 100644\n--- a/builtin-update-ref.c\n+++ b/builtin-update-ref.c\n@@ -50,10 +50,14 @@ int cmd_update_ref(int argc, const char \n \t\tdie(\"%s: not a valid old SHA1\", oldval);\n \n \tlock = lock_any_ref_for_update(refname, oldval ? oldsha1 : NULL, 0);\n-\tif (!lock)\n+\tif (!lock) {\n+\t\terror(\"%s: %s\", refname, strerror(errno));\n \t\treturn 1;\n-\tif (write_ref_sha1(lock, sha1, msg) < 0)\n+ }\n+\tif (write_ref_sha1(lock, sha1, msg) < 0) {\n+\t\terror(\"%s: %s\", refname, strerror(errno));\n \t\treturn 1;\n+\t}\n \n \t/* write_ref_sha1 always unlocks the ref, no need to do it explicitly */\n \treturn 0;\n-- \n1.4.2.rc1.g22734\n\n\n\nFrom 7ea3177aec909e333bacccd00693f223997e2613 Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen \nDate: Tue, 18 Jul 2006 15:10:54 +0200\nSubject: [PATCH] git-checkout.sh: print errors, otherwise it is not clear what happened\n\nSigned-off-by: Alex Riesen \n---\n git-checkout.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-checkout.sh b/git-checkout.sh\nindex 5613bfc..7b335e5 100755\n--- a/git-checkout.sh\n+++ b/git-checkout.sh\n@@ -198,7 +198,7 @@ if [ \"$?\" -eq 0 ]; then\n \t\t\tmkdir -p $(dirname \"$GIT_DIR/logs/refs/heads/$newbranch\")\n \t\t\ttouch \"$GIT_DIR/logs/refs/heads/$newbranch\"\n \t\tfi\n-\t\tgit-update-ref -m \"checkout: Created from $new_name\" \"refs/heads/$newbranch\" $new || exit\n+\t\tgit-update-ref -m \"checkout: Created from $new_name\" \"refs/heads/$newbranch\" $new || die \"failed to create branch $newbranch\"\n \t\tbranch=\"$newbranch\"\n \tfi\n \t[ \"$branch\" ] &&\n-- \n1.4.2.rc1.g22734\n\n"},{"id":"24036","messageId":"7vk663h6zi.fsf@assigned-by-dhcp.cox.net","threadId":"4914","inReplyTo":"81b0412b0607180613t603551b8t865b407c40ab8aef@mail.gmail.com","subject":"Re: print errors from git-update-ref","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-07-24T06:06:25Z","receivedAt":"2006-07-24T06:06:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex Riesen\" writes:\n\n> ...otherwise it not clear what happened when update-ref fails.\n>\n> E.g., git checkout -b a/b/c HEAD would print nothing if refs/heads/a\n> exists and is a directory (it does return 1, so scripts checking for\n> return code should be ok).\n>\n> I'm attaching two patches, because I'm not quite sure where it should\n> be done: git-checkout is the least intrusive, but only builtin-update-ref.c\n> has enough info to help user to resolve the problem (errno is ENOTDIR,\n> which is selfexplanatory). And I happen to use git-update-ref directly\n> sometimes.\n\nMy gut feeling is that complaining from update-ref is fine, but\nI am still tired after a long week and not thinking straight, so\nI will not be applying this tonight.\n\ngit-applypatch, git-am, and git-branch would be helped by\nupdate-ref complaining.\n\nPorcelains?\n\nBTW, I wonder what happens when .git/logs/refs/a is a directory\n(by mistake or malice), .git/refs/a does not exist, and the user\ndoes \"git checkout -b a/b/c HEAD\". Or when .git/logs/refs/a/b/c\ndoes exist but is not writable. My preference is just warn but\ndo not interrupt the primary operation, since ref-log is just an\noptional part of the system, but that would probably lead to\nconfusion, so we might be better off erroring the caller out in\nsuch a case. Opinions?\n\ngit-resolve does not check exit value from update-ref, which is\n*BAD*, but we should be deprecating it anyway.\n\ngit-reset has the same problem of not checking the exit status\nfrom update ref. Worse yet, it calls update-ref with wrong\nparameter ($@ in its parameter should be $*).\n\nPatches to fix these two and half problems should be trivial but\nI won't be doing that myself tonight.\n"},{"id":"24181","messageId":"20060727012856.GB32492@spearce.org","threadId":"4914","inReplyTo":"7vk663h6zi.fsf@assigned-by-dhcp.cox.net","subject":"Re: print errors from git-update-ref","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-07-27T01:28:56Z","receivedAt":"2006-07-27T01:28:56Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Alex Riesen\" writes:\n> \n> > ...otherwise it not clear what happened when update-ref fails.\n> >\n> > E.g., git checkout -b a/b/c HEAD would print nothing if refs/heads/a\n> > exists and is a directory (it does return 1, so scripts checking for\n> > return code should be ok).\n> \n> My gut feeling is that complaining from update-ref is fine, but\n> I am still tired after a long week and not thinking straight, so\n> I will not be applying this tonight.\n\nSo I looked into this issue tonight. For starters I can't seem to\nreproduce the situtation reported by Alex, and since he didn't\nsupply new test cases its difficult to actually fix it.\n\nI did however find problems with git-update-ref a/b/c when a is\nactually an existing ref. This didn't report any error, so here's\na fix. It may resolve Alex's problem - or maybe not.\n\n-->8--\nDisplay an error from update-ref if target ref name is invalid.\n\nAlex Riesen (raa.lkml@gmail.com) recently observed that git branch\nwould fail with no error message due to unexpected situations with\nregards to refs. For example, if .git/refs/heads/gu is a file but\n`git branch -b refs/heads/gu/fixa HEAD` was invoked by the user\nit would fail silently due to refs/heads/gu being a file and not\na directory.\n\nThis change adds a test for trying to create a ref within a directory\nthat is actually currently a file, and adds error printing within\nthe ref locking routine should the resolve operation fail.\n\nThe error printing code probably belongs at this level of the library\nas other failures within the ref locking, writing and logging code\nare also currently at this level of the code.\n\nSigned-off-by: Shawn O. Pearce \n---\n refs.c | 33 +++++++++++++++++++++++++++++++++\n t/t1400-update-ref.sh | 12 ++++++++++++\n 2 files changed, 45 insertions(+), 0 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 56db394..e99e9e4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -290,10 +290,33 @@ static struct ref_lock *verify_lock(stru\n \treturn lock;\n }\n \n+static char* not_a_directory (const char *orig_path)\n+{\n+\tchar *p = strdup(orig_path);\n+\tstruct stat st;\n+\n+\tdo {\n+\t\tchar * s = strrchr(p, '/');\n+\t\tif (s) {\n+\t\t\t*s = 0;\n+\t\t\tif (lstat(p, &st) == 0 && S_ISDIR(st.st_mode)) {\n+\t\t\t\t*s = '/';\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t} else {\n+\t\t\tstrcpy(p, orig_path);\n+\t\t\tbreak;\n+\t\t}\n+\t} while (errno == ENOTDIR);\n+\n+\treturn p;\n+}\n+\n static struct ref_lock *lock_ref_sha1_basic(const char *path,\n \tint plen,\n \tconst unsigned char *old_sha1, int mustexist)\n {\n+\tconst char *orig_path = path;\n \tstruct ref_lock *lock;\n \tstruct stat st;\n \n@@ -303,7 +326,17 @@ static struct ref_lock *lock_ref_sha1_ba\n \tplen = strlen(path) - plen;\n \tpath = resolve_ref(path, lock->old_sha1, mustexist);\n \tif (!path) {\n+\t\tint last_errno = errno;\n+\t\tif (errno == ENOTDIR) {\n+\t\t\tchar* p = not_a_directory(orig_path);\n+\t\t\terror(\"unable to resolve reference %s: %s\",\n+\t\t\t\tp, strerror(errno));\n+\t\t\tfree(p);\n+\t\t} else\n+\t\t\terror(\"unable to resolve reference %s: %s\",\n+\t\t\t\torig_path, strerror(errno));\n \t\tunlock_ref(lock);\n+\t\terrno = last_errno;\n \t\treturn NULL;\n \t}\n \tlock->lk = xcalloc(1, sizeof(struct lock_file));\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 04fab26..e73827c 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -14,6 +14,8 @@ D=44444444444444444444444444444444444444\n E=5555555555555555555555555555555555555555\n F=6666666666666666666666666666666666666666\n m=refs/heads/master\n+n_dir=refs/heads/gu\n+n=$n_dir/fixes\n \n test_expect_success \\\n \t\"create $m\" \\\n@@ -26,6 +28,16 @@ test_expect_success \\\n rm -f .git/$m\n \n test_expect_success \\\n+\t\"fail to create $n\" \\\n+\t'touch .git/$n_dir\n+\t git-update-ref $n $A >out 2>err\n+\t test $? = 1 &&\n+\t test \"\" = \"$(cat out)\" &&\n+\t grep \"error: unable to resolve reference\" err &&\n+\t grep $n_dir err'\n+rm -f .git/$n_dir out err\n+\n+test_expect_success \\\n \t\"create $m (by HEAD)\" \\\n \t'git-update-ref HEAD $A &&\n \t test $A = $(cat .git/$m)'\n-- \n1.4.2.rc1.g802da\n"},{"id":"24192","messageId":"Pine.LNX.4.63.0607271302150.29667@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"4914","inReplyTo":"20060727012856.GB32492@spearce.org","subject":"Re: print errors from git-update-ref","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-07-27T11:04:25Z","receivedAt":"2006-07-27T11:04:25Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 26 Jul 2006, Shawn Pearce wrote:\n\n> This change adds a test for trying to create a ref within a directory\n> that is actually currently a file, and adds error printing within\n> the ref locking routine should the resolve operation fail.\n\nWhy not just print an error message when the resolve operation fails, \ninstead of special casing this obscure corner case? It is way shorter, \ntoo. The test should stay, though.\n\nCiao,\nDscho\n"},{"id":"24283","messageId":"20060728062720.GC30783@spearce.org","threadId":"4914","inReplyTo":"Pine.LNX.4.63.0607271302150.29667@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: print errors from git-update-ref","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-07-28T06:27:20Z","receivedAt":"2006-07-28T06:27:20Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Schindelin wrote:\n> Hi,\n> \n> On Wed, 26 Jul 2006, Shawn Pearce wrote:\n> \n> > This change adds a test for trying to create a ref within a directory\n> > that is actually currently a file, and adds error printing within\n> > the ref locking routine should the resolve operation fail.\n> \n> Why not just print an error message when the resolve operation fails, \n> instead of special casing this obscure corner case? It is way shorter, \n> too. The test should stay, though.\n\nDid you read the patch? If resolve_ref returns NULL then this\nchange prints an error (from errno) no matter what. If errno is\nENOTDIR then it tries to figure out what part of the ref path wasn't\na directory (but was attempted to be used as such) and prints an\nENOTDIR error about that path instead of the one actually given\nto the ref lock function\n\nSo I think I'm doing what you are suggesting...\n\n-- \nShawn.\n"},{"id":"24288","messageId":"7vejw6mbqj.fsf@assigned-by-dhcp.cox.net","threadId":"4914","inReplyTo":"20060728062720.GC30783@spearce.org","subject":"Re: print errors from git-update-ref","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-07-28T07:26:12Z","receivedAt":"2006-07-28T07:26:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Pearce writes:\n\n> Johannes Schindelin wrote:\n>> Hi,\n>> \n>> On Wed, 26 Jul 2006, Shawn Pearce wrote:\n>> \n>> > This change adds a test for trying to create a ref within a directory\n>> > that is actually currently a file, and adds error printing within\n>> > the ref locking routine should the resolve operation fail.\n>> \n>> Why not just print an error message when the resolve operation fails, \n>> instead of special casing this obscure corner case? It is way shorter, \n>> too. The test should stay, though.\n>\n> Did you read the patch? If resolve_ref returns NULL then this\n> change prints an error (from errno) no matter what. If errno is\n> ENOTDIR then it tries to figure out what part of the ref path wasn't\n> a directory (but was attempted to be used as such) and prints an\n> ENOTDIR error about that path instead of the one actually given\n> to the ref lock function\n>\n> So I think I'm doing what you are suggesting...\n\nNot quite.\n\n+\t\tint last_errno = errno;\n+\t\tif (errno == ENOTDIR) {\n+\t\t\tchar* p = not_a_directory(orig_path);\n+\t\t\terror(\"unable to resolve reference %s: %s\",\n+\t\t\t\tp, strerror(errno));\n+\t\t\tfree(p);\n+\t\t} else\n+\t\t\terror(\"unable to resolve reference %s: %s\",\n+\t\t\t\torig_path, strerror(errno));\n \t\tunlock_ref(lock);\n+\t\terrno = last_errno;\n\nI know you are trying to be nice by pinpointing which component\nof the directory hierarchy is offending, but at the same time\nthe nicety is hiding the orig_path given to the program from the\nuser. Maybe showing orig_path _and_ p would be nicer.\n\nBut I suspect there is even more serious problem here.\n\n - lock_ref_sha1_basic() gets \"path\" from the user; you stash it\n away in \"orig_path\".\n\n - resolve_ref() tries to resolve both symbolic links and\n symrefs. If it fails, it returns NULL.\n\n - When it returns NULL, you use orig_path (say, \"a/b/c/d\") to\n see which path component is not a directory (say, \"a/b/c\" is\n a file).\n\nBut the last step does not take into account what resolve_ref()\ndoes, doesn't it? What if orig_path is \"HEAD\", which is a\nsymref, which contained \"ref: refs/heads/myhack/one\" and\n\"refs/heads/myhack\" is a file? Ideally you may want to say\nsomething like:\n\n '''while resolving %s, which points at %s,\n we found out %s is not a directory''' %\n (\"HEAD\", \"refs/heads/myhack/one\", \"refs/heads/myhack\")\n\nSo I tend to agree with Johannes's \"why bother?\" reaction.\n"},{"id":"24325","messageId":"20060729034451.GB28128@spearce.org","threadId":"4914","inReplyTo":"7vejw6mbqj.fsf@assigned-by-dhcp.cox.net","subject":"Re: print errors from git-update-ref","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-07-29T03:44:51Z","receivedAt":"2006-07-29T03:44:51Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> Shawn Pearce writes:\n> \n> > Johannes Schindelin wrote:\n> >> Hi,\n> >> \n> >> On Wed, 26 Jul 2006, Shawn Pearce wrote:\n> >> \n> >> > This change adds a test for trying to create a ref within a directory\n> >> > that is actually currently a file, and adds error printing within\n> >> > the ref locking routine should the resolve operation fail.\n> >> \n> >> Why not just print an error message when the resolve operation fails, \n> >> instead of special casing this obscure corner case? It is way shorter, \n> >> too. The test should stay, though.\n> >\n> > Did you read the patch? If resolve_ref returns NULL then this\n> > change prints an error (from errno) no matter what. If errno is\n> > ENOTDIR then it tries to figure out what part of the ref path wasn't\n> > a directory (but was attempted to be used as such) and prints an\n> > ENOTDIR error about that path instead of the one actually given\n> > to the ref lock function\n> >\n> > So I think I'm doing what you are suggesting...\n>\n[snip]\n> But the last step does not take into account what resolve_ref()\n> does, doesn't it? What if orig_path is \"HEAD\", which is a\n> symref, which contained \"ref: refs/heads/myhack/one\" and\n> \"refs/heads/myhack\" is a file? Ideally you may want to say\n> something like:\n> \n> '''while resolving %s, which points at %s,\n> we found out %s is not a directory''' %\n> (\"HEAD\", \"refs/heads/myhack/one\", \"refs/heads/myhack\")\n> \n> So I tend to agree with Johannes's \"why bother?\" reaction.\n\nOK, that's a bug. It would be horribly misleading. So I'm taking\nthe shortcut here and just telling the user ENOTDIR and orig_path\nrather than resolving it and finding that bad directory component.\n\n-->8--\nDisplay an error from update-ref if target ref name is invalid.\n\nAlex Riesen (raa.lkml@gmail.com) recently observed that git branch\nwould fail with no error message due to unexpected situations with\nregards to refs. For example, if .git/refs/heads/gu is a file but\n`git branch -b refs/heads/gu/fixa HEAD` was invoked by the user\nit would fail silently due to refs/heads/gu being a file and not\na directory.\n\nThis change adds a test for trying to create a ref within a directory\nthat is actually currently a file, and adds error printing within\nthe ref locking routine should the resolve operation fail.\n\nThe error printing code probably belongs at this level of the library\nas other failures within the ref locking, writing and logging code\nare also currently at this level of the code.\n\nSigned-off-by: Shawn O. Pearce \n---\n refs.c | 5 +++++\n t/t1400-update-ref.sh | 12 ++++++++++++\n 2 files changed, 17 insertions(+), 0 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 56db394..02850b6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -294,6 +294,7 @@ static struct ref_lock *lock_ref_sha1_ba\n \tint plen,\n \tconst unsigned char *old_sha1, int mustexist)\n {\n+\tconst char *orig_path = path;\n \tstruct ref_lock *lock;\n \tstruct stat st;\n \n@@ -303,7 +304,11 @@ static struct ref_lock *lock_ref_sha1_ba\n \tplen = strlen(path) - plen;\n \tpath = resolve_ref(path, lock->old_sha1, mustexist);\n \tif (!path) {\n+\t\tint last_errno = errno;\n+\t\terror(\"unable to resolve reference %s: %s\",\n+\t\t\torig_path, strerror(errno));\n \t\tunlock_ref(lock);\n+\t\terrno = last_errno;\n \t\treturn NULL;\n \t}\n \tlock->lk = xcalloc(1, sizeof(struct lock_file));\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 04fab26..ddc80bb 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -14,6 +14,8 @@ D=44444444444444444444444444444444444444\n E=5555555555555555555555555555555555555555\n F=6666666666666666666666666666666666666666\n m=refs/heads/master\n+n_dir=refs/heads/gu\n+n=$n_dir/fixes\n \n test_expect_success \\\n \t\"create $m\" \\\n@@ -26,6 +28,16 @@ test_expect_success \\\n rm -f .git/$m\n \n test_expect_success \\\n+\t\"fail to create $n\" \\\n+\t'touch .git/$n_dir\n+\t git-update-ref $n $A >out 2>err\n+\t test $? = 1 &&\n+\t test \"\" = \"$(cat out)\" &&\n+\t grep \"error: unable to resolve reference\" err &&\n+\t grep $n err'\n+rm -f .git/$n_dir out err\n+\n+test_expect_success \\\n \t\"create $m (by HEAD)\" \\\n \t'git-update-ref HEAD $A &&\n \t test $A = $(cat .git/$m)'\n-- \n1.4.2.rc1.g802da\n"}]}