* [PATCH] Better error message when we are unable to lock the index file @ 2006-08-12 7:37 Fredrik Kuivinen 2006-08-12 8:03 ` Junio C Hamano 0 siblings, 1 reply; 5+ messages in thread From: Fredrik Kuivinen @ 2006-08-12 7:37 UTC (permalink / raw) To: git; +Cc: junkio Signed-off-by: Fredrik Kuivinen <freku045@student.liu.se> --- builtin-update-index.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/builtin-update-index.c b/builtin-update-index.c index 24dca47..f8f5e10 100644 --- a/builtin-update-index.c +++ b/builtin-update-index.c @@ -493,7 +493,7 @@ int cmd_update_index(int argc, const cha newfd = hold_lock_file_for_update(lock_file, get_index_file()); if (newfd < 0) - die("unable to create new cachefile"); + die("unable to lock index file: %s", strerror(errno)); entries = read_cache(); if (entries < 0) ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Better error message when we are unable to lock the index file 2006-08-12 7:37 [PATCH] Better error message when we are unable to lock the index file Fredrik Kuivinen @ 2006-08-12 8:03 ` Junio C Hamano 2006-08-12 8:09 ` Junio C Hamano 2006-08-12 19:19 ` Fredrik Kuivinen 0 siblings, 2 replies; 5+ messages in thread From: Junio C Hamano @ 2006-08-12 8:03 UTC (permalink / raw) To: Fredrik Kuivinen; +Cc: git Fredrik Kuivinen <freku045@student.liu.se> writes: > diff --git a/builtin-update-index.c b/builtin-update-index.c > index 24dca47..f8f5e10 100644 > --- a/builtin-update-index.c > +++ b/builtin-update-index.c > @@ -493,7 +493,7 @@ int cmd_update_index(int argc, const cha > > newfd = hold_lock_file_for_update(lock_file, get_index_file()); > if (newfd < 0) > - die("unable to create new cachefile"); > + die("unable to lock index file: %s", strerror(errno)); Looking at output from: $ git grep -A 3 hold_lock_file_for_update I wonder if it might be more consistent to do something like this instead. It removes more lines than it adds ;-). Most of the callers except the one in refs.c use the function to update the index file. Among the index writers, everybody except write-tree dies if they cannot open it for writing. --- builtin-add.c | 4 +--- builtin-apply.c | 7 ++----- builtin-mv.c | 5 +---- builtin-read-tree.c | 4 +--- builtin-rm.c | 4 +--- builtin-update-index.c | 4 +--- builtin-write-tree.c | 2 +- cache.h | 2 +- lockfile.c | 10 +++++++++- refs.c | 2 +- 10 files changed, 19 insertions(+), 25 deletions(-) diff --git a/builtin-add.c b/builtin-add.c index 096b611..0cb9c81 100644 --- a/builtin-add.c +++ b/builtin-add.c @@ -93,9 +93,7 @@ int cmd_add(int argc, const char **argv, git_config(git_default_config); - newfd = hold_lock_file_for_update(&lock_file, get_index_file()); - if (newfd < 0) - die("unable to create new index file"); + newfd = hold_lock_file_for_update(&lock_file, get_index_file(), 1); if (read_cache() < 0) die("index file corrupt"); diff --git a/builtin-apply.c b/builtin-apply.c index be2c715..9cf477c 100644 --- a/builtin-apply.c +++ b/builtin-apply.c @@ -2234,12 +2234,9 @@ static int apply_patch(int fd, const cha apply = 0; write_index = check_index && apply; - if (write_index && newfd < 0) { + if (write_index && newfd < 0) newfd = hold_lock_file_for_update(&lock_file, - get_index_file()); - if (newfd < 0) - die("unable to create new index file"); - } + get_index_file(), 1); if (check_index) { if (read_cache() < 0) die("unable to read index file"); diff --git a/builtin-mv.c b/builtin-mv.c index ce8187c..a731f8d 100644 --- a/builtin-mv.c +++ b/builtin-mv.c @@ -72,10 +72,7 @@ int cmd_mv(int argc, const char **argv, git_config(git_default_config); - newfd = hold_lock_file_for_update(&lock_file, get_index_file()); - if (newfd < 0) - die("unable to create new index file"); - + newfd = hold_lock_file_for_update(&lock_file, get_index_file(), 1); if (read_cache() < 0) die("index file corrupt"); diff --git a/builtin-read-tree.c b/builtin-read-tree.c index b30160a..71a7026 100644 --- a/builtin-read-tree.c +++ b/builtin-read-tree.c @@ -884,9 +884,7 @@ int cmd_read_tree(int argc, const char * git_config(git_default_config); - newfd = hold_lock_file_for_update(&lock_file, get_index_file()); - if (newfd < 0) - die("unable to create new index file"); + newfd = hold_lock_file_for_update(&lock_file, get_index_file(), 1); git_config(git_default_config); diff --git a/builtin-rm.c b/builtin-rm.c index 8af3d7e..593d867 100644 --- a/builtin-rm.c +++ b/builtin-rm.c @@ -52,9 +52,7 @@ int cmd_rm(int argc, const char **argv, git_config(git_default_config); - newfd = hold_lock_file_for_update(&lock_file, get_index_file()); - if (newfd < 0) - die("unable to create new index file"); + newfd = hold_lock_file_for_update(&lock_file, get_index_file(), 1); if (read_cache() < 0) die("index file corrupt"); diff --git a/builtin-update-index.c b/builtin-update-index.c index 24dca47..d2556f3 100644 --- a/builtin-update-index.c +++ b/builtin-update-index.c @@ -491,9 +491,7 @@ int cmd_update_index(int argc, const cha /* We can't free this memory, it becomes part of a linked list parsed atexit() */ lock_file = xcalloc(1, sizeof(struct lock_file)); - newfd = hold_lock_file_for_update(lock_file, get_index_file()); - if (newfd < 0) - die("unable to create new cachefile"); + newfd = hold_lock_file_for_update(lock_file, get_index_file(), 1); entries = read_cache(); if (entries < 0) diff --git a/builtin-write-tree.c b/builtin-write-tree.c index 6b62d7d..ca06149 100644 --- a/builtin-write-tree.c +++ b/builtin-write-tree.c @@ -18,7 +18,7 @@ int write_tree(unsigned char *sha1, int /* We can't free this memory, it becomes part of a linked list parsed atexit() */ struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file)); - newfd = hold_lock_file_for_update(lock_file, get_index_file()); + newfd = hold_lock_file_for_update(lock_file, get_index_file(), 0); entries = read_cache(); if (entries < 0) diff --git a/cache.h b/cache.h index b8c21e0..b2ab208 100644 --- a/cache.h +++ b/cache.h @@ -175,7 +175,7 @@ struct lock_file { struct lock_file *next; char filename[PATH_MAX]; }; -extern int hold_lock_file_for_update(struct lock_file *, const char *path); +extern int hold_lock_file_for_update(struct lock_file *, const char *path, int); extern int commit_lock_file(struct lock_file *); extern void rollback_lock_file(struct lock_file *); diff --git a/lockfile.c b/lockfile.c index 2346e0e..a5ea49b 100644 --- a/lockfile.c +++ b/lockfile.c @@ -22,7 +22,7 @@ static void remove_lock_file_on_signal(i raise(signo); } -int hold_lock_file_for_update(struct lock_file *lk, const char *path) +static int lock_file(struct lock_file *lk, const char *path) { int fd; sprintf(lk->filename, "%s.lock", path); @@ -41,6 +41,14 @@ int hold_lock_file_for_update(struct loc return fd; } +int hold_lock_file_for_update(struct lock_file *lk, const char *path, int die_on_error) +{ + int fd = lock_file(lk, path); + if (fd < 0 && die_on_error) + die("unable to create new index file"); + return fd; +} + int commit_lock_file(struct lock_file *lk) { char result_file[PATH_MAX]; diff --git a/refs.c b/refs.c index 28a9394..564f8a7 100644 --- a/refs.c +++ b/refs.c @@ -319,7 +319,7 @@ static struct ref_lock *lock_ref_sha1_ba if (safe_create_leading_directories(lock->ref_file)) die("unable to create directory for %s", lock->ref_file); - lock->lock_fd = hold_lock_file_for_update(lock->lk, lock->ref_file); + lock->lock_fd = hold_lock_file_for_update(lock->lk, lock->ref_file, 0); if (lock->lock_fd < 0) { error("Couldn't open lock file %s: %s", lock->lk->filename, strerror(errno)); ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Better error message when we are unable to lock the index file 2006-08-12 8:03 ` Junio C Hamano @ 2006-08-12 8:09 ` Junio C Hamano 2006-08-12 17:16 ` Shawn Pearce 2006-08-12 19:19 ` Fredrik Kuivinen 1 sibling, 1 reply; 5+ messages in thread From: Junio C Hamano @ 2006-08-12 8:09 UTC (permalink / raw) To: Shawn Pearce; +Cc: git, Fredrik Kuivinen Junio C Hamano <junkio@cox.net> writes: > Looking at output from: > > $ git grep -A 3 hold_lock_file_for_update > > I wonder if it might be more consistent to do something like > this instead. It removes more lines than it adds ;-). > > Most of the callers except the one in refs.c use the function to > update the index file. Among the index writers, everybody > except write-tree dies if they cannot open it for writing. > > diff --git a/refs.c b/refs.c > index 28a9394..564f8a7 100644 > --- a/refs.c > +++ b/refs.c > @@ -319,7 +319,7 @@ static struct ref_lock *lock_ref_sha1_ba > > if (safe_create_leading_directories(lock->ref_file)) > die("unable to create directory for %s", lock->ref_file); > - lock->lock_fd = hold_lock_file_for_update(lock->lk, lock->ref_file); > + lock->lock_fd = hold_lock_file_for_update(lock->lk, lock->ref_file, 0); > if (lock->lock_fd < 0) { > error("Couldn't open lock file %s: %s", > lock->lk->filename, strerror(errno)); Looking at this part further, it seems that this one could simply die when it fails -- after all it dies when leading directories cannot be created, so dying upon failure of hold_lock_file_for_update() would be consistent ;-). Which makes write-tree the only odd-man-out. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Better error message when we are unable to lock the index file 2006-08-12 8:09 ` Junio C Hamano @ 2006-08-12 17:16 ` Shawn Pearce 0 siblings, 0 replies; 5+ messages in thread From: Shawn Pearce @ 2006-08-12 17:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Fredrik Kuivinen Junio C Hamano <junkio@cox.net> wrote: > Junio C Hamano <junkio@cox.net> writes: > > > Looking at output from: > > > > $ git grep -A 3 hold_lock_file_for_update > > > > I wonder if it might be more consistent to do something like > > this instead. It removes more lines than it adds ;-). > > > > Most of the callers except the one in refs.c use the function to > > update the index file. Among the index writers, everybody > > except write-tree dies if they cannot open it for writing. > > > > diff --git a/refs.c b/refs.c > > index 28a9394..564f8a7 100644 > > --- a/refs.c > > +++ b/refs.c > > @@ -319,7 +319,7 @@ static struct ref_lock *lock_ref_sha1_ba > > > > if (safe_create_leading_directories(lock->ref_file)) > > die("unable to create directory for %s", lock->ref_file); > > - lock->lock_fd = hold_lock_file_for_update(lock->lk, lock->ref_file); > > + lock->lock_fd = hold_lock_file_for_update(lock->lk, lock->ref_file, 0); > > if (lock->lock_fd < 0) { > > error("Couldn't open lock file %s: %s", > > lock->lk->filename, strerror(errno)); > > Looking at this part further, it seems that this one could > simply die when it fails -- after all it dies when leading > directories cannot be created, so dying upon failure of > hold_lock_file_for_update() would be consistent ;-). Agreed. -- Shawn. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Better error message when we are unable to lock the index file 2006-08-12 8:03 ` Junio C Hamano 2006-08-12 8:09 ` Junio C Hamano @ 2006-08-12 19:19 ` Fredrik Kuivinen 1 sibling, 0 replies; 5+ messages in thread From: Fredrik Kuivinen @ 2006-08-12 19:19 UTC (permalink / raw) To: Junio C Hamano; +Cc: Fredrik Kuivinen, git On Sat, Aug 12, 2006 at 01:03:47AM -0700, Junio C Hamano wrote: > Fredrik Kuivinen <freku045@student.liu.se> writes: > > > diff --git a/builtin-update-index.c b/builtin-update-index.c > > index 24dca47..f8f5e10 100644 > > --- a/builtin-update-index.c > > +++ b/builtin-update-index.c > > @@ -493,7 +493,7 @@ int cmd_update_index(int argc, const cha > > > > newfd = hold_lock_file_for_update(lock_file, get_index_file()); > > if (newfd < 0) > > - die("unable to create new cachefile"); > > + die("unable to lock index file: %s", strerror(errno)); > > Looking at output from: > > $ git grep -A 3 hold_lock_file_for_update > > I wonder if it might be more consistent to do something like > this instead. It removes more lines than it adds ;-). Looks good, one small comment below. > diff --git a/lockfile.c b/lockfile.c > index 2346e0e..a5ea49b 100644 > --- a/lockfile.c > +++ b/lockfile.c > @@ -22,7 +22,7 @@ static void remove_lock_file_on_signal(i > raise(signo); > } > > -int hold_lock_file_for_update(struct lock_file *lk, const char *path) > +static int lock_file(struct lock_file *lk, const char *path) > { > int fd; > sprintf(lk->filename, "%s.lock", path); > @@ -41,6 +41,14 @@ int hold_lock_file_for_update(struct loc > return fd; > } > > +int hold_lock_file_for_update(struct lock_file *lk, const char *path, int die_on_error) > +{ > + int fd = lock_file(lk, path); > + if (fd < 0 && die_on_error) > + die("unable to create new index file"); Could we have (something like) die("unable to lock index file: %s", strerror(errno)); here instead? > + return fd; > +} > + > int commit_lock_file(struct lock_file *lk) > { > char result_file[PATH_MAX]; - Fredrik ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2006-08-12 19:19 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2006-08-12 7:37 [PATCH] Better error message when we are unable to lock the index file Fredrik Kuivinen 2006-08-12 8:03 ` Junio C Hamano 2006-08-12 8:09 ` Junio C Hamano 2006-08-12 17:16 ` Shawn Pearce 2006-08-12 19:19 ` Fredrik Kuivinen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).