read-cache: drop explicit `CLOSE_LOCK`-flag

`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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Martin Ågren committed Oct 6, 2017 at 22:12 UTC 812d6b00750b56fc4b6a75277a30c628cc7be2ef
3 files changed +15 -14
builtin/commit.c
+5 -5
@@ -355,7 +355,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
355
356 refresh_cache_or_die(refresh_flags);
357
358 - if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))
358 + if (write_locked_index(&the_index, &index_lock, 0))
359 die(_("unable to create temporary index"));
360
361 old_index_env = getenv(INDEX_ENVIRONMENT);
@@ -374,7 +374,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
374 if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {
375 if (reopen_lock_file(&index_lock) < 0)
376 die(_("unable to write index file"));
377 - if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))
377 + if (write_locked_index(&the_index, &index_lock, 0))
378 die(_("unable to update temporary index"));
379 } else
380 warning(_("Failed to update main cache tree"));
@@ -401,7 +401,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
401 add_files_to_cache(also ? prefix : NULL, &pathspec, 0);
402 refresh_cache_or_die(refresh_flags);
403 update_main_cache_tree(WRITE_TREE_SILENT);
404 - if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))
404 + if (write_locked_index(&the_index, &index_lock, 0))
405 die(_("unable to write new_index file"));
406 commit_style = COMMIT_NORMAL;
407 ret = get_lock_file_path(&index_lock);
@@ -474,7 +474,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
474 add_remove_files(&partial);
475 refresh_cache(REFRESH_QUIET);
476 update_main_cache_tree(WRITE_TREE_SILENT);
477 - if (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))
477 + if (write_locked_index(&the_index, &index_lock, 0))
478 die(_("unable to write new_index file"));
479
480 hold_lock_file_for_update(&false_lock,
@@ -486,7 +486,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
486 add_remove_files(&partial);
487 refresh_cache(REFRESH_QUIET);
488
489 - if (write_locked_index(&the_index, &false_lock, CLOSE_LOCK))
489 + if (write_locked_index(&the_index, &false_lock, 0))
490 die(_("unable to write temporary index file"));
491
492 discard_cache();
cache.h
+2 -3
@@ -604,11 +604,10 @@ extern int read_index_unmerged(struct index_state *);
604
605 /* For use with `write_locked_index()`. */
606 #define COMMIT_LOCK (1 << 0)
607 -#define CLOSE_LOCK (1 << 1)
607
608 /*
610 - * Write the index while holding an already-taken lock. The flags may
611 - * contain at most one of `COMMIT_LOCK` and `CLOSE_LOCK`.
609 + * Write the index while holding an already-taken lock. Close the lock,
610 + * and if `COMMIT_LOCK` is given, commit it.
611 *
612 * Unless a split index is in use, write the index into the lockfile.
613 *
read-cache.c
+8 -6
@@ -2187,6 +2187,13 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfile
2187 rollback_lock_file(lockfile);
2188 }
2189
2190 +/*
2191 + * On success, `tempfile` is closed. If it is the temporary file
2192 + * of a `struct lock_file`, we will therefore effectively perform
2193 + * a 'close_lock_file_gently()`. Since that is an implementation
2194 + * detail of lockfiles, callers of `do_write_index()` should not
2195 + * rely on it.
2196 + */
2197 static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
2198 int strip_extensions)
2199 {
@@ -2343,14 +2350,9 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
2350 int ret = do_write_index(istate, lock->tempfile, 0);
2351 if (ret)
2352 return ret;
2346 - assert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=
2347 - (COMMIT_LOCK | CLOSE_LOCK));
2353 if (flags & COMMIT_LOCK)
2354 return commit_locked_index(lock);
2350 - else if (flags & CLOSE_LOCK)
2351 - return close_lock_file_gently(lock);
2352 - else
2353 - return ret;
2355 + return close_lock_file_gently(lock);
2356 }
2357
2358 static int write_split_index(struct index_state *istate,