path: root/merge.c
diff options
authorMartin Ågren <>2017-10-06 20:12:13 (GMT)
committerJunio C Hamano <>2017-10-07 01:20:56 (GMT)
commitdf60cf5789782191b092169f86255aa44525b7d1 (patch)
treee95d78996249c97562a3249fe0dc4668d0d5dfcd /merge.c
parent812d6b00750b56fc4b6a75277a30c628cc7be2ef (diff)
read-cache: leave lock in right state in `write_locked_index()`
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 <>
Diffstat (limited to 'merge.c')
1 files changed, 1 insertions, 3 deletions
diff --git a/merge.c b/merge.c
index a18a452..e5d796c 100644
--- a/merge.c
+++ b/merge.c
@@ -91,9 +91,7 @@ int checkout_fast_forward(const struct object_id *head,
if (unpack_trees(nr_trees, t, &opts))
return -1;
- if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) {
- rollback_lock_file(&lock_file);
+ if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
return error(_("unable to write new index file"));
- }
return 0;