| Message ID | pull.1731.v2.git.1715836731784.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] merge: avoid write merge state when unable to write index | expand |
"Kyle Zhao via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Kyle Zhao <kylezhao@tencent.com> > > Currently, when index.lock exists, if a merge occurs, the merge state > will be written and the index will be unchanged. "Currently, " is superfluous in this project, as by convention our proposed log message begins with an observation of the current status (and "what's wrong in it" follows) in the present tense. My guess, from reading the tests, of the situation this patch attempts to handle is something like this? When running a merge while the index is locked (presumably by another process), the merge state is written in SUCH and SUCH files, the index is not updated, and then the merge fails. This leaves the resulting state inconsistent. > If the user exec "git commit" instead of "git merge --abort" after this, > a merge commit will be generated and all modifications from the source > branch will be lost. I do not think this is accurate description of the "an user action can make it worse". In reality, if the user runs "git commit", a safety kicks in and they get: fatal: Unable to create '.../.git/index.lock': file exists. In fact, your "How to reproduce" below the three-dash line removes the stale index.lock file before running "git commit". From this state, if the index.lock file gets removed and the user runs "git commit", a merge commit is created, recording the tree from the inconsistent state the merge left. may be a better description of the breakage. But stepping back a bit, I do not think this extra paragraph is needed at all. If there were a competing process holding the index.lock, it were in the process of updating the index, possibly even to create a new commit. If that process were indeed running the "git commit" command, MERGE_HEAD and other state files we write on our side will be taken into account by them and cause them to record a merge, even though they may have been trying to record something entirely different. So regardless of what _this_ user, whose merge failed due to a competing process that held the index.lock file, does after the merge failure, the recorded history can be hosed by the other process. In any case, to prevent the other "git commit" process from using "our" MERGE_HEAD and other state files to record a wrong contents, the right fix is to make sure everybody who takes the lock on the index file does *not* create any other state files to be read by others before it successfully takes the lock. That will also prevent "git commit" we run after a failed merge (and removing the lockfile) from recording an incorrect merge. I do not offhand know if returning 2 (aka "I am not equipped to handle this merg e at all") is a good way to do so, but if it is, then the patch given here is absolutely the right thing to do. >... > How to reproduce: > ... > git merge source-branch > rm .git/index.lock > git commit -m "4" > diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh > index e5ff073099a..f03709ea4be 100755 > --- a/t/t7600-merge.sh > +++ b/t/t7600-merge.sh > @@ -236,6 +236,16 @@ test_expect_success 'merge c1 with c2' ' > verify_parents $c1 $c2 > ' > > +test_expect_success 'merge c1 with c2 when index.lock exists' ' > + test_when_finished rm .git/index.lock && > + git reset --hard c1 && > + touch .git/index.lock && Do not use "touch" when the timestamp is not the thing we care about. In this case, you want that file to _exist_, and the right way to do so is >.git/index.lock && which already appears in t7400 (it uses a no-op ":" command, but that is not needed). Other than that, the patch looks good to me. Thanks.
diff --git a/builtin/merge.c b/builtin/merge.c index 6a6d3798858..80e3438be25 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -698,8 +698,10 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, SKIP_IF_UNCHANGED, 0, NULL, NULL, - NULL) < 0) - return error(_("Unable to write index.")); + NULL) < 0) { + error(_("Unable to write index.")); + return 2; + } if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree") || !strcmp(strategy, "ort")) { diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh index e5ff073099a..f03709ea4be 100755 --- a/t/t7600-merge.sh +++ b/t/t7600-merge.sh @@ -236,6 +236,16 @@ test_expect_success 'merge c1 with c2' ' verify_parents $c1 $c2 ' +test_expect_success 'merge c1 with c2 when index.lock exists' ' + test_when_finished rm .git/index.lock && + git reset --hard c1 && + touch .git/index.lock && + test_must_fail git merge c2 && + test_path_is_missing .git/MERGE_HEAD && + test_path_is_missing .git/MERGE_MODE && + test_path_is_missing .git/MERGE_MSG +' + test_expect_success 'merge --squash c3 with c7' ' git reset --hard c3 && test_must_fail git merge --squash c7 &&