From 8dc3834610b676c2eb64cfe2a08b8b3d05be05df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Thu, 5 Oct 2017 22:32:11 +0200 Subject: cache.h: document `write_locked_index()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The next patches will tweak the behavior of this function. Document it in order to establish a basis for those patches. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- cache.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) (limited to 'cache.h') diff --git a/cache.h b/cache.h index ea6c236e0f..e9d9556e34 100644 --- a/cache.h +++ b/cache.h @@ -601,9 +601,25 @@ extern int do_read_index(struct index_state *istate, const char *path, extern int read_index_from(struct index_state *, const char *path); extern int is_index_unborn(struct index_state *); 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`. + * + * Unless a split index is in use, write the index into the lockfile. + * + * With a split index, write the shared index to a temporary file, + * adjust its permissions and rename it into place, then write the + * split index to the lockfile. If the temporary file for the shared + * index cannot be created, fall back to the behavior described in + * the previous paragraph. + */ extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags); + extern int discard_index(struct index_state *); extern void move_index_extensions(struct index_state *dst, struct index_state *src); extern int unmerged_index(const struct index_state *); -- cgit v1.2.3 From 812d6b00750b56fc4b6a75277a30c628cc7be2ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Fri, 6 Oct 2017 22:12:12 +0200 Subject: read-cache: drop explicit `CLOSE_LOCK`-flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 Signed-off-by: Junio C Hamano --- cache.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'cache.h') diff --git a/cache.h b/cache.h index e9d9556e34..21a6856c5b 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. * -- cgit v1.2.3 From df60cf5789782191b092169f86255aa44525b7d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Fri, 6 Oct 2017 22:12:13 +0200 Subject: read-cache: leave lock in right state in `write_locked_index()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the original version of `write_locked_index()` returned with an error, it didn't roll back the lockfile unless the error occured at the very end, during closing/committing. See commit 03b866477 (read-cache: new API write_locked_index instead of write_index/write_cache, 2014-06-13). In commit 9f41c7a6b (read-cache: close index.lock in do_write_index, 2017-04-26), we learned to close the lock slightly earlier in the callstack. That was mostly a side-effect of lockfiles being implemented using temporary files, but didn't cause any real harm. Recently, commit 076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05) introduced a subtle bug. If the temporary file is deleted (i.e., the lockfile is rolled back), the tempfile-pointer in the `struct lock_file` will be left dangling. Thus, an attempt to reuse the lockfile, or even just to roll it back, will induce undefined behavior -- most likely a crash. Besides not crashing, we clearly want to make things consistent. The guarantees which the lockfile-machinery itself provides is A) if we ask to commit and it fails, roll back, and B) if we ask to close and it fails, do _not_ roll back. Let's do the same for consistency. Do not delete the temporary file in `do_write_index()`. One of its callers, `write_locked_index()` will thereby avoid rolling back the lock. The other caller, `write_shared_index()`, will delete its temporary file anyway. Both of these callers will avoid undefined behavior (crashing). Teach `write_locked_index(..., COMMIT_LOCK)` to roll back the lock before returning. If we have already succeeded and committed, it will be a noop. Simplify the existing callers where we now have a superfluous call to `rollback_lockfile()`. That should keep future readers from wondering why the callers are inconsistent. Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- cache.h | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'cache.h') diff --git a/cache.h b/cache.h index 21a6856c5b..0e26224b9e 100644 --- a/cache.h +++ b/cache.h @@ -616,6 +616,10 @@ extern int read_index_unmerged(struct index_state *); * split index to the lockfile. If the temporary file for the shared * index cannot be created, fall back to the behavior described in * the previous paragraph. + * + * With `COMMIT_LOCK`, the lock is always committed or rolled back. + * Without it, the lock is closed, but neither committed nor rolled + * back. */ extern int write_locked_index(struct index_state *, struct lock_file *lock, unsigned flags); -- cgit v1.2.3 From b74c90fb419b002c664b0236f2941c34786b18b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20=C3=85gren?= Date: Fri, 6 Oct 2017 22:12:14 +0200 Subject: read_cache: roll back lock in `update_index_if_able()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `update_index_if_able()` used to always commit the lock or roll it back. Commit 03b866477 (read-cache: new API write_locked_index instead of write_index/write_cache, 2014-06-13) stopped rolling it back in case a write was not even attempted. This change in behavior is not motivated in the commit message and appears to be accidental: the `else`-path was removed, although that changed the behavior in case the `if` shortcuts. Reintroduce the rollback and document this behavior. While at it, move the documentation on this function from the function definition to the function declaration in cache.h. If `write_locked_index(..., COMMIT_LOCK)` fails, it will roll back the lock for us (see the previous commit). Noticed-by: Junio C Hamano Signed-off-by: Martin Ågren Signed-off-by: Junio C Hamano --- cache.h | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'cache.h') diff --git a/cache.h b/cache.h index 0e26224b9e..8c24937669 100644 --- a/cache.h +++ b/cache.h @@ -734,6 +734,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st); extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg); extern struct cache_entry *refresh_cache_entry(struct cache_entry *, unsigned int); +/* + * Opportunistically update the index but do not complain if we can't. + * The lockfile is always committed or rolled back. + */ extern void update_index_if_able(struct index_state *, struct lock_file *); extern int hold_locked_index(struct lock_file *, int); -- cgit v1.2.3