{"thread":{"id":"5217","subject":"[PATCH] Better error message when we are unable to lock the index file","startedAt":"2006-08-12T07:37:56Z","lastAt":"2006-08-12T19:19:30Z","messageCount":5,"participants":["Fredrik Kuivinen","Junio C Hamano","Shawn Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"25155","messageId":"20060812073756.5442.5498.stgit@c165","threadId":"5217","inReplyTo":null,"subject":"[PATCH] Better error message when we are unable to lock the index file","fromName":"Fredrik Kuivinen","fromEmail":"freku045@student.liu.se","sentAt":"2006-08-12T07:37:56Z","receivedAt":"2006-08-12T07:37:56Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"\n\n\nSigned-off-by: Fredrik Kuivinen <freku045@student.liu.se>\n\n---\n\n builtin-update-index.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-update-index.c b/builtin-update-index.c\nindex 24dca47..f8f5e10 100644\n--- a/builtin-update-index.c\n+++ b/builtin-update-index.c\n@@ -493,7 +493,7 @@ int cmd_update_index(int argc, const cha\n \n \tnewfd = hold_lock_file_for_update(lock_file, get_index_file());\n \tif (newfd < 0)\n-\t\tdie(\"unable to create new cachefile\");\n+\t\tdie(\"unable to lock index file: %s\", strerror(errno));\n \n \tentries = read_cache();\n \tif (entries < 0)\n"},{"id":"25156","messageId":"7v8xluqt24.fsf@assigned-by-dhcp.cox.net","threadId":"5217","inReplyTo":"20060812073756.5442.5498.stgit@c165","subject":"Re: [PATCH] Better error message when we are unable to lock the index file","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-12T08:03:47Z","receivedAt":"2006-08-12T08:03:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fredrik Kuivinen <freku045@student.liu.se> writes:\n\n> diff --git a/builtin-update-index.c b/builtin-update-index.c\n> index 24dca47..f8f5e10 100644\n> --- a/builtin-update-index.c\n> +++ b/builtin-update-index.c\n> @@ -493,7 +493,7 @@ int cmd_update_index(int argc, const cha\n>  \n>  \tnewfd = hold_lock_file_for_update(lock_file, get_index_file());\n>  \tif (newfd < 0)\n> -\t\tdie(\"unable to create new cachefile\");\n> +\t\tdie(\"unable to lock index file: %s\", strerror(errno));\n\nLooking at output from:\n\n\t$ git grep -A 3 hold_lock_file_for_update\n\nI wonder if it might be more consistent to do something like\nthis instead.  It removes more lines than it adds ;-).\n\nMost of the callers except the one in refs.c use the function to\nupdate the index file.  Among the index writers, everybody\nexcept write-tree dies if they cannot open it for writing.\n\n---\n\n builtin-add.c          |    4 +---\n builtin-apply.c        |    7 ++-----\n builtin-mv.c           |    5 +----\n builtin-read-tree.c    |    4 +---\n builtin-rm.c           |    4 +---\n builtin-update-index.c |    4 +---\n builtin-write-tree.c   |    2 +-\n cache.h                |    2 +-\n lockfile.c             |   10 +++++++++-\n refs.c                 |    2 +-\n 10 files changed, 19 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin-add.c b/builtin-add.c\nindex 096b611..0cb9c81 100644\n--- a/builtin-add.c\n+++ b/builtin-add.c\n@@ -93,9 +93,7 @@ int cmd_add(int argc, const char **argv,\n \n \tgit_config(git_default_config);\n \n-\tnewfd = hold_lock_file_for_update(&lock_file, get_index_file());\n-\tif (newfd < 0)\n-\t\tdie(\"unable to create new index file\");\n+\tnewfd = hold_lock_file_for_update(&lock_file, get_index_file(), 1);\n \n \tif (read_cache() < 0)\n \t\tdie(\"index file corrupt\");\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex be2c715..9cf477c 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -2234,12 +2234,9 @@ static int apply_patch(int fd, const cha\n \t\tapply = 0;\n \n \twrite_index = check_index && apply;\n-\tif (write_index && newfd < 0) {\n+\tif (write_index && newfd < 0)\n \t\tnewfd = hold_lock_file_for_update(&lock_file,\n-\t\t\t\t\t\t  get_index_file());\n-\t\tif (newfd < 0)\n-\t\t\tdie(\"unable to create new index file\");\n-\t}\n+\t\t\t\t\t\t  get_index_file(), 1);\n \tif (check_index) {\n \t\tif (read_cache() < 0)\n \t\t\tdie(\"unable to read index file\");\ndiff --git a/builtin-mv.c b/builtin-mv.c\nindex ce8187c..a731f8d 100644\n--- a/builtin-mv.c\n+++ b/builtin-mv.c\n@@ -72,10 +72,7 @@ int cmd_mv(int argc, const char **argv, \n \n \tgit_config(git_default_config);\n \n-\tnewfd = hold_lock_file_for_update(&lock_file, get_index_file());\n-\tif (newfd < 0)\n-\t\tdie(\"unable to create new index file\");\n-\n+\tnewfd = hold_lock_file_for_update(&lock_file, get_index_file(), 1);\n \tif (read_cache() < 0)\n \t\tdie(\"index file corrupt\");\n \ndiff --git a/builtin-read-tree.c b/builtin-read-tree.c\nindex b30160a..71a7026 100644\n--- a/builtin-read-tree.c\n+++ b/builtin-read-tree.c\n@@ -884,9 +884,7 @@ int cmd_read_tree(int argc, const char *\n \n \tgit_config(git_default_config);\n \n-\tnewfd = hold_lock_file_for_update(&lock_file, get_index_file());\n-\tif (newfd < 0)\n-\t\tdie(\"unable to create new index file\");\n+\tnewfd = hold_lock_file_for_update(&lock_file, get_index_file(), 1);\n \n \tgit_config(git_default_config);\n \ndiff --git a/builtin-rm.c b/builtin-rm.c\nindex 8af3d7e..593d867 100644\n--- a/builtin-rm.c\n+++ b/builtin-rm.c\n@@ -52,9 +52,7 @@ int cmd_rm(int argc, const char **argv, \n \n \tgit_config(git_default_config);\n \n-\tnewfd = hold_lock_file_for_update(&lock_file, get_index_file());\n-\tif (newfd < 0)\n-\t\tdie(\"unable to create new index file\");\n+\tnewfd = hold_lock_file_for_update(&lock_file, get_index_file(), 1);\n \n \tif (read_cache() < 0)\n \t\tdie(\"index file corrupt\");\ndiff --git a/builtin-update-index.c b/builtin-update-index.c\nindex 24dca47..d2556f3 100644\n--- a/builtin-update-index.c\n+++ b/builtin-update-index.c\n@@ -491,9 +491,7 @@ int cmd_update_index(int argc, const cha\n \t/* We can't free this memory, it becomes part of a linked list parsed atexit() */\n \tlock_file = xcalloc(1, sizeof(struct lock_file));\n \n-\tnewfd = hold_lock_file_for_update(lock_file, get_index_file());\n-\tif (newfd < 0)\n-\t\tdie(\"unable to create new cachefile\");\n+\tnewfd = hold_lock_file_for_update(lock_file, get_index_file(), 1);\n \n \tentries = read_cache();\n \tif (entries < 0)\ndiff --git a/builtin-write-tree.c b/builtin-write-tree.c\nindex 6b62d7d..ca06149 100644\n--- a/builtin-write-tree.c\n+++ b/builtin-write-tree.c\n@@ -18,7 +18,7 @@ int write_tree(unsigned char *sha1, int \n \t/* We can't free this memory, it becomes part of a linked list parsed atexit() */\n \tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n \n-\tnewfd = hold_lock_file_for_update(lock_file, get_index_file());\n+\tnewfd = hold_lock_file_for_update(lock_file, get_index_file(), 0);\n \n \tentries = read_cache();\n \tif (entries < 0)\ndiff --git a/cache.h b/cache.h\nindex b8c21e0..b2ab208 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -175,7 +175,7 @@ struct lock_file {\n \tstruct lock_file *next;\n \tchar filename[PATH_MAX];\n };\n-extern int hold_lock_file_for_update(struct lock_file *, const char *path);\n+extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n extern int commit_lock_file(struct lock_file *);\n extern void rollback_lock_file(struct lock_file *);\n \ndiff --git a/lockfile.c b/lockfile.c\nindex 2346e0e..a5ea49b 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -22,7 +22,7 @@ static void remove_lock_file_on_signal(i\n \traise(signo);\n }\n \n-int hold_lock_file_for_update(struct lock_file *lk, const char *path)\n+static int lock_file(struct lock_file *lk, const char *path)\n {\n \tint fd;\n \tsprintf(lk->filename, \"%s.lock\", path);\n@@ -41,6 +41,14 @@ int hold_lock_file_for_update(struct loc\n \treturn fd;\n }\n \n+int hold_lock_file_for_update(struct lock_file *lk, const char *path, int die_on_error)\n+{\n+\tint fd = lock_file(lk, path);\n+\tif (fd < 0 && die_on_error)\n+\t\tdie(\"unable to create new index file\");\n+\treturn fd;\n+}\n+\n int commit_lock_file(struct lock_file *lk)\n {\n \tchar result_file[PATH_MAX];\ndiff --git a/refs.c b/refs.c\nindex 28a9394..564f8a7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -319,7 +319,7 @@ static struct ref_lock *lock_ref_sha1_ba\n \n \tif (safe_create_leading_directories(lock->ref_file))\n \t\tdie(\"unable to create directory for %s\", lock->ref_file);\n-\tlock->lock_fd = hold_lock_file_for_update(lock->lk, lock->ref_file);\n+\tlock->lock_fd = hold_lock_file_for_update(lock->lk, lock->ref_file, 0);\n \tif (lock->lock_fd < 0) {\n \t\terror(\"Couldn't open lock file %s: %s\",\n \t\t      lock->lk->filename, strerror(errno));\n"},{"id":"25157","messageId":"7v4pwiqss7.fsf@assigned-by-dhcp.cox.net","threadId":"5217","inReplyTo":"7v8xluqt24.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Better error message when we are unable to lock the index file","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-12T08:09:44Z","receivedAt":"2006-08-12T08:09:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> Looking at output from:\n>\n> \t$ git grep -A 3 hold_lock_file_for_update\n>\n> I wonder if it might be more consistent to do something like\n> this instead.  It removes more lines than it adds ;-).\n>\n> Most of the callers except the one in refs.c use the function to\n> update the index file.  Among the index writers, everybody\n> except write-tree dies if they cannot open it for writing.\n>\n> diff --git a/refs.c b/refs.c\n> index 28a9394..564f8a7 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -319,7 +319,7 @@ static struct ref_lock *lock_ref_sha1_ba\n>  \n>  \tif (safe_create_leading_directories(lock->ref_file))\n>  \t\tdie(\"unable to create directory for %s\", lock->ref_file);\n> -\tlock->lock_fd = hold_lock_file_for_update(lock->lk, lock->ref_file);\n> +\tlock->lock_fd = hold_lock_file_for_update(lock->lk, lock->ref_file, 0);\n>  \tif (lock->lock_fd < 0) {\n>  \t\terror(\"Couldn't open lock file %s: %s\",\n>  \t\t      lock->lk->filename, strerror(errno));\n\nLooking at this part further, it seems that this one could\nsimply die when it fails -- after all it dies when leading\ndirectories cannot be created, so dying upon failure of\nhold_lock_file_for_update() would be consistent ;-).\n\nWhich makes write-tree the only odd-man-out.\n"},{"id":"25163","messageId":"20060812171626.GA18401@spearce.org","threadId":"5217","inReplyTo":"7v4pwiqss7.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Better error message when we are unable to lock the index file","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-08-12T17:16:26Z","receivedAt":"2006-08-12T17:16:26Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <junkio@cox.net> wrote:\n> Junio C Hamano <junkio@cox.net> writes:\n> \n> > Looking at output from:\n> >\n> > \t$ git grep -A 3 hold_lock_file_for_update\n> >\n> > I wonder if it might be more consistent to do something like\n> > this instead.  It removes more lines than it adds ;-).\n> >\n> > Most of the callers except the one in refs.c use the function to\n> > update the index file.  Among the index writers, everybody\n> > except write-tree dies if they cannot open it for writing.\n> >\n> > diff --git a/refs.c b/refs.c\n> > index 28a9394..564f8a7 100644\n> > --- a/refs.c\n> > +++ b/refs.c\n> > @@ -319,7 +319,7 @@ static struct ref_lock *lock_ref_sha1_ba\n> >  \n> >  \tif (safe_create_leading_directories(lock->ref_file))\n> >  \t\tdie(\"unable to create directory for %s\", lock->ref_file);\n> > -\tlock->lock_fd = hold_lock_file_for_update(lock->lk, lock->ref_file);\n> > +\tlock->lock_fd = hold_lock_file_for_update(lock->lk, lock->ref_file, 0);\n> >  \tif (lock->lock_fd < 0) {\n> >  \t\terror(\"Couldn't open lock file %s: %s\",\n> >  \t\t      lock->lk->filename, strerror(errno));\n> \n> Looking at this part further, it seems that this one could\n> simply die when it fails -- after all it dies when leading\n> directories cannot be created, so dying upon failure of\n> hold_lock_file_for_update() would be consistent ;-).\n\nAgreed.\n\n-- \nShawn.\n"},{"id":"25164","messageId":"20060812191930.GA5205@c165.ib.student.liu.se","threadId":"5217","inReplyTo":"7v8xluqt24.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Better error message when we are unable to lock the index file","fromName":"Fredrik Kuivinen","fromEmail":"freku045@student.liu.se","sentAt":"2006-08-12T19:19:30Z","receivedAt":"2006-08-12T19:19:30Z","isPatch":true,"sender":{"key":"frekui@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13770967?v=4"},"body":"On Sat, Aug 12, 2006 at 01:03:47AM -0700, Junio C Hamano wrote:\n> Fredrik Kuivinen <freku045@student.liu.se> writes:\n> \n> > diff --git a/builtin-update-index.c b/builtin-update-index.c\n> > index 24dca47..f8f5e10 100644\n> > --- a/builtin-update-index.c\n> > +++ b/builtin-update-index.c\n> > @@ -493,7 +493,7 @@ int cmd_update_index(int argc, const cha\n> >  \n> >  \tnewfd = hold_lock_file_for_update(lock_file, get_index_file());\n> >  \tif (newfd < 0)\n> > -\t\tdie(\"unable to create new cachefile\");\n> > +\t\tdie(\"unable to lock index file: %s\", strerror(errno));\n> \n> Looking at output from:\n> \n> \t$ git grep -A 3 hold_lock_file_for_update\n> \n> I wonder if it might be more consistent to do something like\n> this instead.  It removes more lines than it adds ;-).\n\nLooks good, one small comment below.\n\n> diff --git a/lockfile.c b/lockfile.c\n> index 2346e0e..a5ea49b 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -22,7 +22,7 @@ static void remove_lock_file_on_signal(i\n>  \traise(signo);\n>  }\n>  \n> -int hold_lock_file_for_update(struct lock_file *lk, const char *path)\n> +static int lock_file(struct lock_file *lk, const char *path)\n>  {\n>  \tint fd;\n>  \tsprintf(lk->filename, \"%s.lock\", path);\n> @@ -41,6 +41,14 @@ int hold_lock_file_for_update(struct loc\n>  \treturn fd;\n>  }\n>  \n> +int hold_lock_file_for_update(struct lock_file *lk, const char *path, int die_on_error)\n> +{\n> +\tint fd = lock_file(lk, path);\n> +\tif (fd < 0 && die_on_error)\n> +\t\tdie(\"unable to create new index file\");\n\nCould we have (something like)\n\n    die(\"unable to lock index file: %s\", strerror(errno));\n\nhere instead?\n\n> +\treturn fd;\n> +}\n> +\n>  int commit_lock_file(struct lock_file *lk)\n>  {\n> \tchar result_file[PATH_MAX];\n\n- Fredrik\n"}]}