diff options
author | Sebastian Henke <s.henke@henke-informatik.de> | 2019-10-10 15:28:46 +0200 |
---|---|---|
committer | Sebastian Henke <s.henke@henke-informatik.de> | 2019-10-10 15:30:26 +0200 |
commit | 3335a0346a409695ec5ab43448604a51e45a2d08 (patch) | |
tree | f38ba79ddba4abe4335d912dde3089a47384311e | |
parent | f04a58b00c1a350e2cd90bddcdaa7865183c9d2f (diff) | |
download | libgit2-3335a0346a409695ec5ab43448604a51e45a2d08.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.c | 4 | ||||
-rw-r--r-- | src/filebuf.c | 14 | ||||
-rw-r--r-- | src/filebuf.h | 2 | ||||
-rw-r--r-- | src/merge.c | 6 | ||||
-rw-r--r-- | src/refdb_fs.c | 2 | ||||
-rw-r--r-- | src/repository.c | 2 | ||||
-rw-r--r-- | src/revert.c | 4 | ||||
-rw-r--r-- | tests/refs/transactions.c | 21 |
8 files changed, 36 insertions, 19 deletions
diff --git a/src/cherrypick.c b/src/cherrypick.c index d5ba74c66..640d11a7b 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 aaebf82ec..e12a9eabf 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 b5002ecb2..9d53bc307 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 657b0181b..b72b3843b 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 9fb66a8e7..c48afb94f 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 2cb59e0a7..5871e9567 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 b8334c39c..b41a2a131 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); +} |