From 751bacedaa507b7b6d10b2c1f48e019a01a8fa6e Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 1 Oct 2014 12:28:36 +0200 Subject: [PATCH] commit_lock_file_to(): refactor a helper out of commit_lock_file() commit_locked_index(), when writing to an alternate index file, duplicates (poorly) the code in commit_lock_file(). And anyway, it shouldn't have to know so much about the internal workings of lockfile objects. So extract a new function commit_lock_file_to() that does the work common to the two functions, and call it from both commit_lock_file() and commit_locked_index(). Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- Documentation/technical/api-lockfile.txt | 34 +++++++++++--------- cache.h | 1 + lockfile.c | 40 +++++++++++++++--------- read-cache.c | 13 ++------ 4 files changed, 50 insertions(+), 38 deletions(-) diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt index 9805da099d..aa7d822900 100644 --- a/Documentation/technical/api-lockfile.txt +++ b/Documentation/technical/api-lockfile.txt @@ -49,14 +49,14 @@ The caller: When finished writing, the caller can: * Close the file descriptor and rename the lockfile to its final - destination by calling `commit_lock_file`. + destination by calling `commit_lock_file` or `commit_lock_file_to`. * Close the file descriptor and remove the lockfile by calling `rollback_lock_file`. * Close the file descriptor without removing or renaming the lockfile by calling `close_lock_file`, and later call `commit_lock_file`, - `rollback_lock_file`, or `reopen_lock_file`. + `commit_lock_file_to`, `rollback_lock_file`, or `reopen_lock_file`. Even after the lockfile is committed or rolled back, the `lock_file` object must not be freed or altered by the caller. However, it may be @@ -64,20 +64,19 @@ reused; just pass it to another call of `hold_lock_file_for_update` or `hold_lock_file_for_append`. If the program exits before you have called one of `commit_lock_file`, -`rollback_lock_file`, or `close_lock_file`, an `atexit(3)` handler -will close and remove the lockfile, rolling back any uncommitted -changes. +`commit_lock_file_to`, `rollback_lock_file`, or `close_lock_file`, an +`atexit(3)` handler will close and remove the lockfile, rolling back +any uncommitted changes. If you need to close the file descriptor you obtained from a `hold_lock_file_*` function yourself, do so by calling `close_lock_file`. You should never call `close(2)` yourself! Otherwise the `struct lock_file` structure would still think that the -file descriptor needs to be closed, and a later call to -`commit_lock_file` or `rollback_lock_file` or program exit would +file descriptor needs to be closed, and a commit or rollback would result in duplicate calls to `close(2)`. Worse yet, if you `close(2)` and then later open another file descriptor for a completely different -purpose, then a call to `commit_lock_file` or `rollback_lock_file` -might close that unrelated file descriptor. +purpose, then a commit or rollback might close that unrelated file +descriptor. Error handling @@ -100,9 +99,9 @@ unable_to_lock_die:: Emit an appropriate error message and `die()`. -Similarly, `commit_lock_file` and `close_lock_file` return 0 on -success. On failure they set `errno` appropriately, do their best to -roll back the lockfile, and return -1. +Similarly, `commit_lock_file`, `commit_lock_file_to`, and +`close_lock_file` return 0 on success. On failure they set `errno` +appropriately, do their best to roll back the lockfile, and return -1. Flags @@ -156,6 +155,12 @@ commit_lock_file:: `commit_lock_file` for a `lock_file` object that is not currently locked. +commit_lock_file_to:: + + Like `commit_lock_file()`, except that it takes an explicit + `path` argument to which the lockfile should be renamed. The + `path` must be on the same filesystem as the lock file. + rollback_lock_file:: Take a pointer to the `struct lock_file` initialized with an @@ -172,8 +177,9 @@ close_lock_file:: `hold_lock_file_for_append`, and close the file descriptor. Return 0 upon success. On failure to `close(2)`, return a negative value and roll back the lock file. Usually - `commit_lock_file` or `rollback_lock_file` should eventually - be called if `close_lock_file` succeeds. + `commit_lock_file`, `commit_lock_file_to`, or + `rollback_lock_file` should eventually be called if + `close_lock_file` succeeds. reopen_lock_file:: diff --git a/cache.h b/cache.h index f81d95fc3c..414e93ca6b 100644 --- a/cache.h +++ b/cache.h @@ -590,6 +590,7 @@ extern void unable_to_lock_message(const char *path, int err, extern NORETURN void unable_to_lock_die(const char *path, int err); extern int hold_lock_file_for_update(struct lock_file *, const char *path, int); extern int hold_lock_file_for_append(struct lock_file *, const char *path, int); +extern int commit_lock_file_to(struct lock_file *, const char *path); extern int commit_lock_file(struct lock_file *); extern int reopen_lock_file(struct lock_file *); extern void update_index_if_able(struct index_state *, struct lock_file *); diff --git a/lockfile.c b/lockfile.c index 56ad7e8a6e..cf7f4d0470 100644 --- a/lockfile.c +++ b/lockfile.c @@ -43,9 +43,9 @@ * Same as the previous state, except that the lockfile is closed * and fd is -1. * - * - Unlocked (after commit_lock_file(), rollback_lock_file(), a - * failed attempt to lock, or a failed close_lock_file()). In this - * state: + * - Unlocked (after commit_lock_file(), commit_lock_file_to(), + * rollback_lock_file(), a failed attempt to lock, or a failed + * close_lock_file()). In this state: * - active is unset * - filename is empty (usually, though there are transitory * states in which this condition doesn't hold). Client code should @@ -284,23 +284,15 @@ int reopen_lock_file(struct lock_file *lk) return lk->fd; } -int commit_lock_file(struct lock_file *lk) +int commit_lock_file_to(struct lock_file *lk, const char *path) { - static struct strbuf result_file = STRBUF_INIT; - int err; - if (!lk->active) - die("BUG: attempt to commit unlocked object"); + die("BUG: attempt to commit unlocked object to \"%s\"", path); if (close_lock_file(lk)) return -1; - /* remove ".lock": */ - strbuf_add(&result_file, lk->filename.buf, - lk->filename.len - LOCK_SUFFIX_LEN); - err = rename(lk->filename.buf, result_file.buf); - strbuf_reset(&result_file); - if (err) { + if (rename(lk->filename.buf, path)) { int save_errno = errno; rollback_lock_file(lk); errno = save_errno; @@ -312,6 +304,26 @@ int commit_lock_file(struct lock_file *lk) return 0; } +int commit_lock_file(struct lock_file *lk) +{ + static struct strbuf result_file = STRBUF_INIT; + int err; + + if (!lk->active) + die("BUG: attempt to commit unlocked object"); + + if (lk->filename.len <= LOCK_SUFFIX_LEN || + strcmp(lk->filename.buf + lk->filename.len - LOCK_SUFFIX_LEN, LOCK_SUFFIX)) + die("BUG: lockfile filename corrupt"); + + /* remove ".lock": */ + strbuf_add(&result_file, lk->filename.buf, + lk->filename.len - LOCK_SUFFIX_LEN); + err = commit_lock_file_to(lk, result_file.buf); + strbuf_reset(&result_file); + return err; +} + int hold_locked_index(struct lock_file *lk, int die_on_error) { return hold_lock_file_for_update(lk, get_index_file(), diff --git a/read-cache.c b/read-cache.c index 91bf876ee6..e887e23fb6 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2041,17 +2041,10 @@ void set_alternate_index_output(const char *name) static int commit_locked_index(struct lock_file *lk) { - if (alternate_index_output) { - if (close_lock_file(lk)) - return -1; - if (rename(lk->filename.buf, alternate_index_output)) - return -1; - lk->active = 0; - strbuf_reset(&lk->filename); - return 0; - } else { + if (alternate_index_output) + return commit_lock_file_to(lk, alternate_index_output); + else return commit_lock_file(lk); - } } static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,