From 8b0ddd5dd90e1d62f32faf14d5cb4bf88f04a095 Mon Sep 17 00:00:00 2001 From: Edward Thomson Date: Sat, 17 Jan 2015 23:28:53 -0600 Subject: [PATCH] merge: lock the index at the start of the merge Always lock the index when we begin the merge, before we write any of the metdata files. This prevents a race where another client may run a commit after we have written the MERGE_HEAD but before we have updated the index, which will produce a merge commit that is treesame to one parent. The merge will finish and update the index and the resultant commit would not be a merge at all. --- src/merge.c | 21 ++++++++++++++-- tests/merge/workdir/setup.c | 49 +++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/src/merge.c b/src/merge.c index 7c38b5692..8201c0c1c 100644 --- a/src/merge.c +++ b/src/merge.c @@ -2651,7 +2651,9 @@ int git_merge( git_checkout_options checkout_opts; git_annotated_commit *ancestor_head = NULL, *our_head = NULL; git_tree *ancestor_tree = NULL, *our_tree = NULL, **their_trees = NULL; - git_index *index_new = NULL; + git_index *index = NULL, *index_new = NULL; + git_indexwriter indexwriter = GIT_INDEXWRITER_INIT; + bool write_index = false; size_t i; int error = 0; @@ -2672,6 +2674,17 @@ int git_merge( ancestor_head, our_head, their_heads_len, their_heads)) < 0) goto on_error; + write_index = (checkout_opts.checkout_strategy & GIT_CHECKOUT_DONT_WRITE_INDEX) == 0; + + if (write_index) { + /* Never let checkout update the index, we'll update it ourselves. */ + checkout_opts.checkout_strategy |= GIT_CHECKOUT_DONT_WRITE_INDEX; + + if ((error = git_repository_index(&index, repo)) < 0 || + (error = git_indexwriter_init(&indexwriter, index)) < 0) + goto on_error; + } + /* Write the merge files to the repository. */ if ((error = git_merge__setup(repo, our_head, their_heads, their_heads_len)) < 0) goto on_error; @@ -2693,7 +2706,8 @@ int git_merge( if ((error = git_merge_trees(&index_new, repo, ancestor_tree, our_tree, their_trees[0], merge_opts)) < 0 || (error = git_merge__check_result(repo, index_new)) < 0 || (error = git_merge__append_conflicts_to_merge_msg(repo, index_new)) < 0 || - (error = git_checkout_index(repo, index_new, &checkout_opts)) < 0) + (error = git_checkout_index(repo, index_new, &checkout_opts)) < 0 || + (write_index && (error = git_indexwriter_commit(&indexwriter)) < 0)) goto on_error; goto done; @@ -2702,6 +2716,9 @@ on_error: merge_state_cleanup(repo); done: + git_indexwriter_cleanup(&indexwriter); + + git_index_free(index); git_index_free(index_new); git_tree_free(ancestor_tree); diff --git a/tests/merge/workdir/setup.c b/tests/merge/workdir/setup.c index 31ffd5702..4aebf8701 100644 --- a/tests/merge/workdir/setup.c +++ b/tests/merge/workdir/setup.c @@ -1045,3 +1045,52 @@ void test_merge_workdir_setup__removed_after_failure(void) git_annotated_commit_free(our_head); git_annotated_commit_free(their_heads[0]); } + +void test_merge_workdir_setup__unlocked_after_success(void) +{ + git_oid our_oid; + git_reference *octo1_ref; + git_annotated_commit *our_head, *their_heads[1]; + + cl_git_pass(git_oid_fromstr(&our_oid, ORIG_HEAD)); + cl_git_pass(git_annotated_commit_lookup(&our_head, repo, &our_oid)); + + cl_git_pass(git_reference_lookup(&octo1_ref, repo, GIT_REFS_HEADS_DIR OCTO1_BRANCH)); + cl_git_pass(git_annotated_commit_from_ref(&their_heads[0], repo, octo1_ref)); + + cl_git_pass(git_merge( + repo, (const git_annotated_commit **)&their_heads[0], 1, NULL, NULL)); + + cl_assert(!git_path_exists("merge-resolve/.git/index.lock")); + + git_reference_free(octo1_ref); + + git_annotated_commit_free(our_head); + git_annotated_commit_free(their_heads[0]); +} + +void test_merge_workdir_setup__unlocked_after_conflict(void) +{ + git_oid our_oid; + git_reference *octo1_ref; + git_annotated_commit *our_head, *their_heads[1]; + + cl_git_pass(git_oid_fromstr(&our_oid, ORIG_HEAD)); + cl_git_pass(git_annotated_commit_lookup(&our_head, repo, &our_oid)); + + cl_git_pass(git_reference_lookup(&octo1_ref, repo, GIT_REFS_HEADS_DIR OCTO1_BRANCH)); + cl_git_pass(git_annotated_commit_from_ref(&their_heads[0], repo, octo1_ref)); + + cl_git_rewritefile("merge-resolve/new-in-octo1.txt", + "Conflicting file!\n\nMerge will fail!\n"); + + cl_git_fail(git_merge( + repo, (const git_annotated_commit **)&their_heads[0], 1, NULL, NULL)); + + cl_assert(!git_path_exists("merge-resolve/.git/index.lock")); + + git_reference_free(octo1_ref); + + git_annotated_commit_free(our_head); + git_annotated_commit_free(their_heads[0]); +}