From 668053befecfd0979ff790bc9a2a3c308c38a9be Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= <cmn@dwim.me>
Date: Fri, 24 Jul 2015 18:44:29 +0200
Subject: [PATCH 1/2] filebuf: failing test for leaving the lockfile when
 failing to rename

When we fail to rename, we currently leave the lockfile laying
around. This shows that behaviour.
---
 tests/core/filebuf.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/tests/core/filebuf.c b/tests/core/filebuf.c
index 5a3e7510f..9fd52b179 100644
--- a/tests/core/filebuf.c
+++ b/tests/core/filebuf.c
@@ -124,3 +124,30 @@ void test_core_filebuf__umask(void)
 	cl_must_pass(p_unlink(test));
 }
 
+void test_core_filebuf__rename_error(void)
+{
+	git_filebuf file = GIT_FILEBUF_INIT;
+	char *dir = "subdir",  *test = "subdir/test", *test_lock = "subdir/test.lock";
+	int fd;
+
+#ifndef GIT_WIN32
+	cl_skip();
+#endif
+
+	cl_git_pass(p_mkdir(dir, 0666));
+	cl_git_mkfile(test, "dummy content");
+	fd = p_open(test, O_RDONLY);
+	cl_assert(fd > 0);
+	cl_git_pass(git_filebuf_open(&file, test, 0, 0666));
+
+	cl_git_pass(git_filebuf_printf(&file, "%s\n", "libgit2 rocks"));
+
+	cl_assert_equal_i(true, git_path_exists(test_lock));
+
+	cl_git_fail(git_filebuf_commit(&file));
+	p_close(fd);
+
+	git_filebuf_cleanup(&file);
+	cl_assert_equal_i(false, git_path_exists(test_lock));
+
+}

From 19d9beb7ffbde3e171c17fc3544d508304a30fbf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= <cmn@dwim.me>
Date: Fri, 24 Jul 2015 19:22:41 +0200
Subject: [PATCH 2/2] filebuf: remove lockfile upon rename errors

When we have an error renaming the lockfile, we need to make sure
that we remove it upon cleanup. For this, we need to keep track of
whether we opened the file and whether the rename succeeded.

If we did create the lockfile but the rename did not succeed, we
remove the lockfile. This won't protect against all errors, but
the most common ones (target file is open) does get handled.
---
 src/filebuf.c        | 7 ++++++-
 src/filebuf.h        | 2 ++
 tests/core/filebuf.c | 2 +-
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/filebuf.c b/src/filebuf.c
index 848ac343b..838f4b4d2 100644
--- a/src/filebuf.c
+++ b/src/filebuf.c
@@ -101,7 +101,7 @@ void git_filebuf_cleanup(git_filebuf *file)
 	if (file->fd_is_open && file->fd >= 0)
 		p_close(file->fd);
 
-	if (file->fd_is_open && file->path_lock && git_path_exists(file->path_lock))
+	if (file->created_lock && !file->did_rename && file->path_lock && git_path_exists(file->path_lock))
 		p_unlink(file->path_lock);
 
 	if (file->compute_digest) {
@@ -258,6 +258,7 @@ int git_filebuf_open(git_filebuf *file, const char *path, int flags, mode_t mode
 			goto cleanup;
 		}
 		file->fd_is_open = true;
+		file->created_lock = true;
 
 		/* No original path */
 		file->path_original = NULL;
@@ -281,6 +282,8 @@ int git_filebuf_open(git_filebuf *file, const char *path, int flags, mode_t mode
 		/* open the file for locking */
 		if ((error = lock_file(file, flags, mode)) < 0)
 			goto cleanup;
+
+		file->created_lock = true;
 	}
 
 	return 0;
@@ -340,6 +343,8 @@ int git_filebuf_commit(git_filebuf *file)
 		goto on_error;
 	}
 
+	file->did_rename = true;
+
 	git_filebuf_cleanup(file);
 	return 0;
 
diff --git a/src/filebuf.h b/src/filebuf.h
index 2bd18dc35..f4d255b0a 100644
--- a/src/filebuf.h
+++ b/src/filebuf.h
@@ -44,6 +44,8 @@ struct git_filebuf {
 	size_t buf_size, buf_pos;
 	git_file fd;
 	bool fd_is_open;
+	bool created_lock;
+	bool did_rename;
 	bool do_not_buffer;
 	int last_error;
 };
diff --git a/tests/core/filebuf.c b/tests/core/filebuf.c
index 9fd52b179..3f7dc8569 100644
--- a/tests/core/filebuf.c
+++ b/tests/core/filebuf.c
@@ -148,6 +148,6 @@ void test_core_filebuf__rename_error(void)
 	p_close(fd);
 
 	git_filebuf_cleanup(&file);
-	cl_assert_equal_i(false, git_path_exists(test_lock));
 
+	cl_assert_equal_i(false, git_path_exists(test_lock));
 }