[PATCH v3 10/12] read-cache: drop explicit `CLOSE_LOCK`-flag
- From
- Martin Ågren <martin.agren@gmail.com>
- Date
- Oct 6, 2017, 20:12 UTC
- Message-ID
- <db718624ece17a7bed4c956d581dab83b579db0b.1507320376.git.martin.agren@gmail.com>
- In-Reply-To
- <cover.1507320376.git.martin.agren@gmail.com>
`write_locked_index()` takes two flags: `COMMIT_LOCK` and `CLOSE_LOCK`. At most one is allowed. But it is also possible to use no flag, i.e., `0`. But when `write_locked_index()` calls `do_write_index()`, the temporary file, a.k.a. the lockfile, will be closed. So passing `0` is effectively the same as `CLOSE_LOCK`, which seems like a bug.
We might feel tempted to restructure the code in order to close the file later, or conditionally. It also feels a bit unfortunate that we simply "happen" to close the lock by way of an implementation detail of lockfiles. But note that we need to close the temporary file before `stat`-ing it, at least on Windows. See 9f41c7a6b (read-cache: close index.lock in do_write_index, 2017-04-26).
Drop `CLOSE_LOCK` and make it explicit that `write_locked_index()` always closes the lock. Whether it is also committed is governed by the remaining flag, `COMMIT_LOCK`.
This means we neither have nor suggest that we have a mode to write the index and leave the file open. Whatever extra contents we might eventually want to write, we should probably write it from within `write_locked_index()` itself anyway.
Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- v3: Dropped the comment in `do_write_locked_index()` and documented how `do_write_index()` handles the temporary file instead.
builtin/commit.c | 10 +++++----- cache.h | 5 ++--- read-cache.c | 14 ++++++++------ 3 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c index 0f8ddb686..32dc2101f 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -355,7 +355,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix refresh_cache_or_die(refresh_flags); - if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK)) + if (write_locked_index(&the_index, &index_lock, 0)) die(_("unable to create temporary index")); old_index_env = getenv(INDEX_ENVIRONMENT); @@ -374,7 +374,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) { if (reopen_lock_file(&index_lock) < 0) die(_("unable to write index file")); - if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK)) + if (write_locked_index(&the_index, &index_lock, 0)) die(_("unable to update temporary index")); } else warning(_("Failed to update main cache tree")); @@ -401,7 +401,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix add_files_to_cache(also ? prefix : NULL, &pathspec, 0); refresh_cache_or_die(refresh_flags); update_main_cache_tree(WRITE_TREE_SILENT); - if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK)) + if (write_locked_index(&the_index, &index_lock, 0)) die(_("unable to write new_index file")); commit_style = COMMIT_NORMAL; ret = get_lock_file_path(&index_lock); @@ -474,7 +474,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix add_remove_files(&partial); refresh_cache(REFRESH_QUIET); update_main_cache_tree(WRITE_TREE_SILENT); - if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK)) + if (write_locked_index(&the_index, &index_lock, 0)) die(_("unable to write new_index file")); hold_lock_file_for_update(&false_lock, @@ -486,7 +486,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix add_remove_files(&partial); refresh_cache(REFRESH_QUIET); - if (write_locked_index(&the_index, &false_lock, CLOSE_LOCK)) + if (write_locked_index(&the_index, &false_lock, 0)) die(_("unable to write temporary index file")); discard_cache(); diff --git a/cache.h b/cache.h index e9d9556e3..21a6856c5 100644 --- a/cache.h +++ b/cache.h @@ -604,11 +604,10 @@ extern int read_index_unmerged(struct index_state *); /* For use with `write_locked_index()`. */ #define COMMIT_LOCK (1 << 0) -#define CLOSE_LOCK (1 << 1) /* - * Write the index while holding an already-taken lock. The flags may - * contain at most one of `COMMIT_LOCK` and `CLOSE_LOCK`. + * Write the index while holding an already-taken lock. Close the lock, + * and if `COMMIT_LOCK` is given, commit it. * * Unless a split index is in use, write the index into the lockfile. * diff --git a/read-cache.c b/read-cache.c index 65f4fe837..c7aa3632a 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2187,6 +2187,13 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfile rollback_lock_file(lockfile); } +/* + * On success, `tempfile` is closed. If it is the temporary file + * of a `struct lock_file`, we will therefore effectively perform + * a 'close_lock_file_gently()`. Since that is an implementation + * detail of lockfiles, callers of `do_write_index()` should not + * rely on it. + */ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, int strip_extensions) { @@ -2343,14 +2350,9 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l int ret = do_write_index(istate, lock->tempfile, 0); if (ret) return ret; - assert((flags & (COMMIT_LOCK | CLOSE_LOCK)) != - (COMMIT_LOCK | CLOSE_LOCK)); if (flags & COMMIT_LOCK) return commit_locked_index(lock); - else if (flags & CLOSE_LOCK) - return close_lock_file_gently(lock); - else - return ret; + return close_lock_file_gently(lock); } static int write_split_index(struct index_state *istate,
-- 2.15.0.rc0