diff mbox series

[v5] merge: avoid write merge state when unable to write index

Message ID pull.1731.v5.git.1718173639942.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v5] merge: avoid write merge state when unable to write index | expand

Commit Message

Kyle Zhao June 12, 2024, 6:27 a.m. UTC
From: Kyle Zhao <kylezhao@tencent.com>

When running a merge while the index is locked (presumably by another
process), the merge state is written, the index is not updated, and then
the merge fails. This might cause unexpected results.

E.g., if another running process is "git commit", 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.

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"
    git checkout master
    echo "1" >> 1.txt && git add . && git commit -m "3"
    # another git process runnning
    touch .git/index.lock
    git merge source-branch
    # another git process finished
    rm .git/index.lock
    git commit -m "4"
    
    
    Then the modifications from the source branch are lost.
    
    Regards, Kyle

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

Range-diff vs v4:

 1:  7055dfb82c7 ! 1:  a5156088514 merge: avoid write merge state when unable to write index
     @@ Commit message
          process), the merge state is written, the index is not updated, and then
          the merge fails. This might cause unexpected results.
      
     -    i.g. if another running process is "git commit", 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.
     +    E.g., if another running process is "git commit", 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.
      
          Signed-off-by: Kyle Zhao <kylezhao@tencent.com>
      


 builtin/merge.c  |  2 +-
 t/t7600-merge.sh | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)


base-commit: 19fe900cfce8096b7645ec9611a0b981f6bbd154

Comments

Junio C Hamano June 13, 2024, 5:27 p.m. UTC | #1
"Kyle Zhao via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Kyle Zhao <kylezhao@tencent.com>
>
> When running a merge while the index is locked (presumably by another
> process), the merge state is written, the index is not updated, and then
> the merge fails. This might cause unexpected results.

Failing the merge is good thing.

> E.g., if another running process is "git commit", 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.

If I recall the previous discussion correctly, I think the primary
thing this change achieves is to get us closer to a state where
competing commands (a "git commit" run while we are doing something
else like "git merge") take the index.lock as the first thing (so
others are blocked), before making auxiliary files like MERGE_HEAD
that would affect the behaviour of whoever has index.lock (and thus
making a new commit).  And that is what we need to stress in the
proposed log message, I would think.

But this is probably only half-a-solution.

> Signed-off-by: Kyle Zhao <kylezhao@tencent.com>
> ---
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 6a6d3798858..12c1b048fe1 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -699,7 +699,7 @@ 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."));
> +		die(_("Unable to write index."));
>  
>  	if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree") ||
>  	    !strcmp(strategy, "ort")) {

If we fail to write the index here, even if we have other strategies
to try after the current one fails, it probably is a good idea to die
and stop the other ones from being tried, not because their attempt
to write the index might fail the same way, but because it is likely
that we are really in a weird situation and the user would want to
inspect the situation before this process makes too much damage to
the working tree and the index.

But this is probably only half-a-solution.  Because we release the
index.lock when the refresh-and-write call returns, and the
index.lock is free for the other process to grab, do whatever they
want to do to the index and the working tree (including making a new
commit out of it and update the HEAD), before or after we write out
the merge state files. So in that sense, this patch is *not* solving
the "E.g., if another running process is ..."  problem at all.

So ...

> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index e5ff073099a..ef54cff4faa 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 &&
> +	>.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

... I do not quite see the point of this exercise.  It is good to
make sure that "git merge c2" fails while it is clear that somebody
else is mucking with the same repository touching the index.  But it
does not help the other process all that much if we stop only when
they happen to be holding lock at the point we try to refresh the
index.  It is making the race window smaller by a tiny bit.

So, I am not sure if this is worth doing.
goldofzky@gmail.com June 17, 2024, 3:02 a.m. UTC | #2
On Fri, Jun 14, 2024 at 1:27 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Kyle Zhao via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Kyle Zhao <kylezhao@tencent.com>
> >
> > When running a merge while the index is locked (presumably by another
> > process), the merge state is written, the index is not updated, and then
> > the merge fails. This might cause unexpected results.
>
> Failing the merge is good thing.
>
> > E.g., if another running process is "git commit", 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.
>
> If I recall the previous discussion correctly, I think the primary
> thing this change achieves is to get us closer to a state where
> competing commands (a "git commit" run while we are doing something
> else like "git merge") take the index.lock as the first thing (so
> others are blocked), before making auxiliary files like MERGE_HEAD
> that would affect the behaviour of whoever has index.lock (and thus
> making a new commit).  And that is what we need to stress in the
> proposed log message, I would think.

I agree.

>
> But this is probably only half-a-solution.


>
> > Signed-off-by: Kyle Zhao <kylezhao@tencent.com>
> > ---
> > diff --git a/builtin/merge.c b/builtin/merge.c
> > index 6a6d3798858..12c1b048fe1 100644
> > --- a/builtin/merge.c
> > +++ b/builtin/merge.c
> > @@ -699,7 +699,7 @@ 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."));
> > +             die(_("Unable to write index."));
> >
> >       if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree") ||
> >           !strcmp(strategy, "ort")) {
>
> If we fail to write the index here, even if we have other strategies
> to try after the current one fails, it probably is a good idea to die
> and stop the other ones from being tried, not because their attempt
> to write the index might fail the same way, but because it is likely
> that we are really in a weird situation and the user would want to
> inspect the situation before this process makes too much damage to
> the working tree and the index.
>
> But this is probably only half-a-solution.  Because we release the
> index.lock when the refresh-and-write call returns, and the
> index.lock is free for the other process to grab, do whatever they
> want to do to the index and the working tree (including making a new
> commit out of it and update the HEAD), before or after we write out
> the merge state files. So in that sense, this patch is *not* solving
> the "E.g., if another running process is ..."  problem at all.
>
> So ...
>

Oops!
Thank you for the reminder, I indeed did not consider this point. 
Even if the index is written successfully, another running "git commit" may 
record a merge (if it generates the commit before merge state is removed).

> > diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> > index e5ff073099a..ef54cff4faa 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 &&
> > +     >.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
>
> ... I do not quite see the point of this exercise.  It is good to
> make sure that "git merge c2" fails while it is clear that somebody
> else is mucking with the same repository touching the index.  But it
> does not help the other process all that much if we stop only when
> they happen to be holding lock at the point we try to refresh the
> index.  It is making the race window smaller by a tiny bit.

This test is only used to verify whether the merge state is generated after a merge fails 
due to the index writing. 
To my knowledge, for a merge, writing the merge state and the index is not an atomic operation. 
Simply modifying the logic of merge is useless and we need to think about other solutions.

>
> So, I am not sure if this is worth doing.
>

[1] As explained in the commit message of v1, we are particularly concerned about the
issue of source code loss. Once this problem occurs, the consequences will be immeasurable 
and detecting these abnormal merge points will be very difficult.

[2] From a usability perspective, the merge state should not be written when the index is being
 written (merge conflicts are not considered failures). To avoid losing changes in the source branch, 
users can only execute 'git merge --abort' and try 'git merge' again. However, if the merge state is
not written in the first place, the user only needs to retry 'git merge'.

In other words, writing the merge state after the index write fails is meaningless and could 
potentially cause Git to lose changes.
goldofzky@gmail.com June 17, 2024, 9:17 a.m. UTC | #3
> [2] From a usability perspective, the merge state should not be written when the index is being
> written (merge conflicts are not considered failures). To avoid losing changes in the source branch,

"the merge state should not be written when the index is being written" should be:
the merge state should not be written if the index write fails.



> users can only execute 'git merge --abort' and try 'git merge' again. However, if the merge state is
> not written in the first place, the user only needs to retry 'git merge'.


>In other words, writing the merge state after the index write fails is meaningless and could
>potentially cause Git to lose changes.
diff mbox series

Patch

diff --git a/builtin/merge.c b/builtin/merge.c
index 6a6d3798858..12c1b048fe1 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -699,7 +699,7 @@  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."));
+		die(_("Unable to write index."));
 
 	if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree") ||
 	    !strcmp(strategy, "ort")) {
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index e5ff073099a..ef54cff4faa 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 &&
+	>.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 &&