git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] merge: avoid write merge state when unable to write index
@ 2024-05-16  5:08 Kyle Zhao via GitGitGadget
  2024-05-16  5:18 ` [PATCH v2] " Kyle Zhao via GitGitGadget
  0 siblings, 1 reply; 12+ messages in thread
From: Kyle Zhao via GitGitGadget @ 2024-05-16  5:08 UTC (permalink / raw)
  To: git; +Cc: Kyle Zhao, Kyle Zhao

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.

If the user exec "git commit" instead of "git merge --abort" after this,
the commit will be successful and all modifications from the source
branch will be lost.

Signed-off-by: Kyle Zhao <kylezhao@tencent.com>
---
    merge: avoid write merge state when unable to write index
    
    In some of our monorepos, code is sometimes lost after merging.
    
    After investigation, we discovered the problem.
    
    This happens if we perform "git pull" or "git merge" when another git
    process is writing to the index, especially in a monorepo (because its
    index will be larger).
    
    How to reproduce:
    
    git init demo
    cd demo
    touch 1.txt && git add . && git commit -m "1"
    git checkout -b source-branch
    touch 2.txt && git add . && git commit -m "2"
    echo "1" >> 1.txt && git add . && git commit -m "3"
    touch .git/index.lock
    git merge source-branch
    rm .git/index.lock
    git commit -m "4"
    
    
    Then the modifications from the source branch are lost.
    
    Regards, Kyle
    
    cc: Elijah Newren newren@gmail.com cc: Ævar Arnfjörð Bjarmason
    avarab@gmail.com cc: Junio C Hamano gitster@pobox.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1731%2Fkeyu98%2Fkz%2Ffix-merge-when-index-lock-exists-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1731

 builtin/merge.c  |  6 ++++--
 t/t7600-merge.sh | 10 ++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

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 &&

base-commit: 19fe900cfce8096b7645ec9611a0b981f6bbd154
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [PATCH v2] merge: avoid write merge state when unable to write index
@ 2024-05-17  3:59 Kyle Zhao
  0 siblings, 0 replies; 12+ messages in thread
From: Kyle Zhao @ 2024-05-17  3:59 UTC (permalink / raw)
  To: gitster, gitgitgadget; +Cc: git

> "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.

Done.

> 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.

Yes, your understanding is very correct.

In fact, there are many other situations besides the example I give
that will be affected.

> 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.

In the latest version I modified it to:
    die(_("Unable to write index."));

Maybe it's better. (Refer to the code elsewhere)

> 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).

Done.

Thanks.

^ permalink raw reply	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-06-17  9:17 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-16  5:08 [PATCH] merge: avoid write merge state when unable to write index Kyle Zhao via GitGitGadget
2024-05-16  5:18 ` [PATCH v2] " Kyle Zhao via GitGitGadget
2024-05-16 16:18   ` Junio C Hamano
2024-05-17  3:47   ` [PATCH v3] " Kyle Zhao via GitGitGadget
2024-05-17  4:41     ` [PATCH v4] " Kyle Zhao via GitGitGadget
2024-06-12  2:40       ` goldofzky
2024-06-12  6:27       ` [PATCH v5] " Kyle Zhao via GitGitGadget
2024-06-13 17:27         ` Junio C Hamano
2024-06-17  3:02           ` [Internet]Re: " goldofzky
2024-06-17  9:17             ` goldofzky
2024-06-17  3:08         ` [PATCH v6] " Kyle Zhao via GitGitGadget
  -- strict thread matches above, loose matches on Subject: below --
2024-05-17  3:59 [PATCH v2] " Kyle Zhao

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).