Commit 3335a0346a409695ec5ab43448604a51e45a2d08
2019-10-10T15:28:46
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.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162 163 164 165 166 167 168 169 170 171
diff --git a/src/cherrypick.c b/src/cherrypick.c
index d5ba74c..640d11a 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 aaebf82..e12a9ea 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 b5002ec..9d53bc3 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 657b018..b72b384 100644
--- a/src/merge.c
+++ b/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;
diff --git a/src/refdb_fs.c b/src/refdb_fs.c
index 9fb66a8..c48afb9 100644
--- a/src/refdb_fs.c
+++ b/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;
diff --git a/src/repository.c b/src/repository.c
index 2cb59e0..5871e95 100644
--- a/src/repository.c
+++ b/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);
diff --git a/src/revert.c b/src/revert.c
index b8334c3..b41a2a1 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 39ea1ca..d4ddf45 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);
+}