summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSebastian Henke <s.henke@henke-informatik.de>2019-10-10 15:28:46 +0200
committerPatrick Steinhardt <ps@pks.im>2020-03-26 16:20:18 +0100
commit5aca2444e670df0d32925a8f0c97866ac121d618 (patch)
tree9cb4dc6406342a4b4db86df6e3f594343dc3833c
parent85ab27c8108015368a1597dde484ae85ed4b06c9 (diff)
downloadlibgit2-5aca2444e670df0d32925a8f0c97866ac121d618.tar.gz
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.
-rw-r--r--src/cherrypick.c4
-rw-r--r--src/filebuf.c14
-rw-r--r--src/filebuf.h2
-rw-r--r--src/merge.c6
-rw-r--r--src/refdb_fs.c2
-rw-r--r--src/repository.c2
-rw-r--r--src/revert.c4
-rw-r--r--tests/refs/transactions.c21
8 files changed, 36 insertions, 19 deletions
diff --git a/src/cherrypick.c b/src/cherrypick.c
index 26935c6df..4a07c9a43 100644
--- a/src/cherrypick.c
+++ b/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;
diff --git a/src/filebuf.c b/src/filebuf.c
index 8a70bcd66..182a13687 100644
--- a/src/filebuf.c
+++ b/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 {
diff --git a/src/filebuf.h b/src/filebuf.h
index f51ff230f..446308403 100644
--- a/src/filebuf.h
+++ b/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)
diff --git a/src/merge.c b/src/merge.c
index 14b76fa2e..59c825f6e 100644
--- a/src/merge.c
+++ b/src/merge.c
@@ -2430,7 +2430,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++) {
@@ -2458,7 +2458,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)
@@ -2689,7 +2689,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;
diff --git a/src/refdb_fs.c b/src/refdb_fs.c
index d582cb8a2..b9b643e22 100644
--- a/src/refdb_fs.c
+++ b/src/refdb_fs.c
@@ -794,7 +794,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;
diff --git a/src/repository.c b/src/repository.c
index ddbf2626d..7de195c92 100644
--- a/src/repository.c
+++ b/src/repository.c
@@ -2476,7 +2476,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);
diff --git a/src/revert.c b/src/revert.c
index eb71a68db..2ce7d2219 100644
--- a/src/revert.c
+++ b/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;
diff --git a/tests/refs/transactions.c b/tests/refs/transactions.c
index 39ea1cae5..d4ddf459f 100644
--- a/tests/refs/transactions.c
+++ b/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);
+}