refs: fix locks getting forcibly removed

The flag GIT_FILEBUF_FORCE currently does two things:
     1. It will cause the filebuf to create non-existing leading
        directories for the file that is about to be written.
     2. It will forcibly remove any pre-existing locks.
While most call sites actually do want (1), they do not want to
remove pre-existing locks, as that renders the locking mechanisms
effectively useless.
Introduce a new flag `GIT_FILEBUF_CREATE_LEADING_DIRS` to
separate both behaviours cleanly from each other and convert
callers to use it instead of `GIT_FILEBUF_FORCE` to have them
honor locked files correctly.

As this conversion removes all current users of `GIT_FILEBUF_FORCE`,
this commit removes the flag altogether.

Author: henkesn

src/cherrypick.c

@@ -30,7 +30,7 @@ static int write_cherrypick_head(
 	int error = 0;
 
 	if ((error = git_buf_joinpath(&file_path, repo->gitdir, GIT_CHERRYPICK_HEAD_FILE)) >= 0 &&
-		(error = git_filebuf_open(&file, file_path.ptr, GIT_FILEBUF_FORCE, GIT_CHERRYPICK_FILE_MODE)) >= 0 &&
+		(error = git_filebuf_open(&file, file_path.ptr, GIT_FILEBUF_CREATE_LEADING_DIRS, GIT_CHERRYPICK_FILE_MODE)) >= 0 &&
 		(error = git_filebuf_printf(&file, "%s\n", commit_oidstr)) >= 0)
 		error = git_filebuf_commit(&file);
 
@@ -51,7 +51,7 @@ static int write_merge_msg(
 	int error = 0;
 
 	if ((error = git_buf_joinpath(&file_path, repo->gitdir, GIT_MERGE_MSG_FILE)) < 0 ||
-		(error = git_filebuf_open(&file, file_path.ptr, GIT_FILEBUF_FORCE, GIT_CHERRYPICK_FILE_MODE)) < 0 ||
+		(error = git_filebuf_open(&file, file_path.ptr, GIT_FILEBUF_CREATE_LEADING_DIRS, GIT_CHERRYPICK_FILE_MODE)) < 0 ||
 		(error = git_filebuf_printf(&file, "%s", commit_msg)) < 0)
 		goto cleanup;
 

src/filebuf.c

@@ -44,18 +44,14 @@ static int verify_last_error(git_filebuf *file)
 static int lock_file(git_filebuf *file, int flags, mode_t mode)
 {
 	if (git_path_exists(file->path_lock) == true) {
-		if (flags & GIT_FILEBUF_FORCE)
-			p_unlink(file->path_lock);
-		else {
-			git_error_clear(); /* actual OS error code just confuses */
-			git_error_set(GIT_ERROR_OS,
-				"failed to lock file '%s' for writing", file->path_lock);
-			return GIT_ELOCKED;
-		}
+		git_error_clear(); /* actual OS error code just confuses */
+		git_error_set(GIT_ERROR_OS,
+			"failed to lock file '%s' for writing", file->path_lock);
+		return GIT_ELOCKED;
 	}
 
 	/* create path to the file buffer is required */
-	if (flags & GIT_FILEBUF_FORCE) {
+	if (flags & GIT_FILEBUF_CREATE_LEADING_DIRS) {
 		/* XXX: Should dirmode here be configurable? Or is 0777 always fine? */
 		file->fd = git_futils_creat_locked_withpath(file->path_lock, 0777, mode);
 	} else {

src/filebuf.h

@@ -19,7 +19,7 @@
 
 #define GIT_FILEBUF_HASH_CONTENTS		(1 << 0)
 #define GIT_FILEBUF_APPEND				(1 << 2)
-#define GIT_FILEBUF_FORCE				(1 << 3)
+#define GIT_FILEBUF_CREATE_LEADING_DIRS	(1 << 3)
 #define GIT_FILEBUF_TEMPORARY			(1 << 4)
 #define GIT_FILEBUF_DO_NOT_BUFFER		(1 << 5)
 #define GIT_FILEBUF_FSYNC				(1 << 6)

src/merge.c

@@ -2431,7 +2431,7 @@ static int write_merge_head(
 	assert(repo && heads);
 
 	if ((error = git_buf_joinpath(&file_path, repo->gitdir, GIT_MERGE_HEAD_FILE)) < 0 ||
-		(error = git_filebuf_open(&file, file_path.ptr, GIT_FILEBUF_FORCE, GIT_MERGE_FILE_MODE)) < 0)
+		(error = git_filebuf_open(&file, file_path.ptr, GIT_FILEBUF_CREATE_LEADING_DIRS, GIT_MERGE_FILE_MODE)) < 0)
 		goto cleanup;
 
 	for (i = 0; i < heads_len; i++) {
@@ -2459,7 +2459,7 @@ static int write_merge_mode(git_repository *repo)
 	assert(repo);
 
 	if ((error = git_buf_joinpath(&file_path, repo->gitdir, GIT_MERGE_MODE_FILE)) < 0 ||
-		(error = git_filebuf_open(&file, file_path.ptr, GIT_FILEBUF_FORCE, GIT_MERGE_FILE_MODE)) < 0)
+		(error = git_filebuf_open(&file, file_path.ptr, GIT_FILEBUF_CREATE_LEADING_DIRS, GIT_MERGE_FILE_MODE)) < 0)
 		goto cleanup;
 
 	if ((error = git_filebuf_write(&file, "no-ff", 5)) < 0)
@@ -2690,7 +2690,7 @@ static int write_merge_msg(
 		entries[i].merge_head = heads[i];
 
 	if ((error = git_buf_joinpath(&file_path, repo->gitdir, GIT_MERGE_MSG_FILE)) < 0 ||
-		(error = git_filebuf_open(&file, file_path.ptr, GIT_FILEBUF_FORCE, GIT_MERGE_FILE_MODE)) < 0 ||
+		(error = git_filebuf_open(&file, file_path.ptr, GIT_FILEBUF_CREATE_LEADING_DIRS, GIT_MERGE_FILE_MODE)) < 0 ||
 		(error = git_filebuf_write(&file, "Merge ", 6)) < 0)
 		goto cleanup;
 

src/refdb_fs.c

@@ -804,7 +804,7 @@ static int loose_lock(git_filebuf *file, refdb_fs_backend *backend, const char *
 	if (git_buf_joinpath(&ref_path, basedir, name) < 0)
 		return -1;
 
-	filebuf_flags = GIT_FILEBUF_FORCE;
+	filebuf_flags = GIT_FILEBUF_CREATE_LEADING_DIRS;
 	if (backend->fsync)
 		filebuf_flags |= GIT_FILEBUF_FSYNC;
 

src/repository.c

@@ -2485,7 +2485,7 @@ int git_repository__set_orig_head(git_repository *repo, const git_oid *orig_head
 	git_oid_fmt(orig_head_str, orig_head);
 
 	if ((error = git_buf_joinpath(&file_path, repo->gitdir, GIT_ORIG_HEAD_FILE)) == 0 &&
-		(error = git_filebuf_open(&file, file_path.ptr, GIT_FILEBUF_FORCE, GIT_MERGE_FILE_MODE)) == 0 &&
+		(error = git_filebuf_open(&file, file_path.ptr, GIT_FILEBUF_CREATE_LEADING_DIRS, GIT_MERGE_FILE_MODE)) == 0 &&
 		(error = git_filebuf_printf(&file, "%.*s\n", GIT_OID_HEXSZ, orig_head_str)) == 0)
 		error = git_filebuf_commit(&file);
 

src/revert.c

@@ -29,7 +29,7 @@ static int write_revert_head(
 	int error = 0;
 
 	if ((error = git_buf_joinpath(&file_path, repo->gitdir, GIT_REVERT_HEAD_FILE)) >= 0 &&
-		(error = git_filebuf_open(&file, file_path.ptr, GIT_FILEBUF_FORCE, GIT_REVERT_FILE_MODE)) >= 0 &&
+		(error = git_filebuf_open(&file, file_path.ptr, GIT_FILEBUF_CREATE_LEADING_DIRS, GIT_REVERT_FILE_MODE)) >= 0 &&
 		(error = git_filebuf_printf(&file, "%s\n", commit_oidstr)) >= 0)
 		error = git_filebuf_commit(&file);
 
@@ -51,7 +51,7 @@ static int write_merge_msg(
 	int error = 0;
 
 	if ((error = git_buf_joinpath(&file_path, repo->gitdir, GIT_MERGE_MSG_FILE)) < 0 ||
-		(error = git_filebuf_open(&file, file_path.ptr, GIT_FILEBUF_FORCE, GIT_REVERT_FILE_MODE)) < 0 ||
+		(error = git_filebuf_open(&file, file_path.ptr, GIT_FILEBUF_CREATE_LEADING_DIRS, GIT_REVERT_FILE_MODE)) < 0 ||
 		(error = git_filebuf_printf(&file, "Revert \"%s\"\n\nThis reverts commit %s.\n",
 		commit_msgline, commit_oidstr)) < 0)
 		goto cleanup;

tests/refs/transactions.c

@@ -108,3 +108,24 @@ void test_refs_transactions__unlocked_set(void)
 	cl_git_fail_with(GIT_ENOTFOUND, git_transaction_set_target(g_tx, "refs/heads/foo", &id, NULL, NULL));
 	cl_git_pass(git_transaction_commit(g_tx));
 }
+
+void test_refs_transactions__error_on_locking_locked_ref(void)
+{
+	git_oid id;
+	git_transaction *g_tx_with_lock;
+	git_repository *g_repo_with_locking_tx;
+	const char *g_repo_path = git_repository_path(g_repo);
+	
+	/* prepare a separate transaction in another instance of testrepo and lock master */
+	cl_git_pass(git_repository_open(&g_repo_with_locking_tx, g_repo_path));
+	cl_git_pass(git_transaction_new(&g_tx_with_lock, g_repo_with_locking_tx));
+	cl_git_pass(git_transaction_lock_ref(g_tx_with_lock, "refs/heads/master"));
+
+	/* lock reference for set_target */
+	cl_git_pass(git_oid_fromstr(&id, "a65fedf39aefe402d3bb6e24df4d4f5fe4547750"));
+	cl_git_fail_with(GIT_ELOCKED, git_transaction_lock_ref(g_tx, "refs/heads/master"));
+	cl_git_fail_with(GIT_ENOTFOUND, git_transaction_set_target(g_tx, "refs/heads/master", &id, NULL, NULL));
+
+	git_transaction_free(g_tx_with_lock);
+	git_repository_free(g_repo_with_locking_tx);
+}