search

gitgitgadget / git

GitGitGadget's Git fork. Open Pull Requests here to submit them to the Git mailing list
https://gitgitgadget.github.io/
Other
213 stars 134 forks source link

merge: avoid write merge state when unable to write index #1731

Closed keyu98 closed 3 months ago

keyu98 commented 5 months ago

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

keyu98 commented 5 months ago

/submit

gitgitgadget[bot] commented 5 months ago

Submitted as pull.1731.git.1715836120584.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v1

To fetch this version to local tag pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v1
keyu98 commented 5 months ago

/submit

gitgitgadget[bot] commented 5 months ago

Error: aab0be559898a9c0f142846adaa8b0388c27b1d9 was already submitted

keyu98 commented 5 months ago

/submit

gitgitgadget[bot] commented 5 months ago

Submitted as pull.1731.v2.git.1715836731784.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v2

To fetch this version to local tag pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v2
gitgitgadget[bot] commented 5 months ago

On the Git mailing list, Junio C Hamano wrote (reply to this):

"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.
keyu98 commented 5 months ago

/submit

gitgitgadget[bot] commented 5 months ago

Submitted as pull.1731.v3.git.1715917639587.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v3

To fetch this version to local tag pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v3:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v3
keyu98 commented 5 months ago

/submit

gitgitgadget[bot] commented 5 months ago

Submitted as pull.1731.v4.git.1715920862420.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v4

To fetch this version to local tag pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v4:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v4
keyu98 commented 4 months ago

/submit

gitgitgadget[bot] commented 4 months ago

Submitted as pull.1731.v5.git.1718173639942.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v5

To fetch this version to local tag pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v5:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v5
gitgitgadget[bot] commented 4 months ago

On the Git mailing list, Junio C Hamano wrote (reply to this):

"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.
keyu98 commented 4 months ago

/submit

gitgitgadget[bot] commented 4 months ago

Submitted as pull.1731.v6.git.1718593717745.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v6

To fetch this version to local tag pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v6:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1731/keyu98/kz/fix-merge-when-index-lock-exists-v6
gitgitgadget[bot] commented 4 months ago

This branch is now known as kz/merge-fail-early-upon-refresh-failure.

gitgitgadget[bot] commented 4 months ago

This patch series was integrated into seen via https://github.com/git/git/commit/d2c001ca14bea79c50220f9d3f33d2bdba018ca3.

gitgitgadget[bot] commented 4 months ago

This patch series was integrated into seen via https://github.com/git/git/commit/c3aed14b89d116ef2ce677b95f0f13c7af6b209b.

gitgitgadget[bot] commented 4 months ago

This patch series was integrated into next via https://github.com/git/git/commit/01d4bdd0197493f8391628ea498d06cddbf8f494.

gitgitgadget[bot] commented 4 months ago

There was a status update in the "New Topics" section about the branch kz/merge-fail-early-upon-refresh-failure on the Git mailing list:

When "git merge" sees that the index cannot be refreshed (e.g. due
to another process doing the same in the background), it died but
after writing MERGE_HEAD etc. files, which was useless for the
purpose to recover from the failure.

Will merge to 'master'.
source: <pull.1731.v6.git.1718593717745.gitgitgadget@gmail.com>
gitgitgadget[bot] commented 4 months ago

This patch series was integrated into seen via https://github.com/git/git/commit/5a2d67a9a2b087ae33458c9287b898f4a348cf8e.

gitgitgadget[bot] commented 4 months ago

This patch series was integrated into seen via https://github.com/git/git/commit/cc4f30f39d64f149accda8fa822eb25cc156b073.

gitgitgadget[bot] commented 4 months ago

There was a status update in the "Cooking" section about the branch kz/merge-fail-early-upon-refresh-failure on the Git mailing list:

When "git merge" sees that the index cannot be refreshed (e.g. due
to another process doing the same in the background), it died but
after writing MERGE_HEAD etc. files, which was useless for the
purpose to recover from the failure.

Will merge to 'master'.
source: <pull.1731.v6.git.1718593717745.gitgitgadget@gmail.com>
gitgitgadget[bot] commented 3 months ago

This patch series was integrated into seen via https://github.com/git/git/commit/453fbbf2e486b0342b8179121959a6b993630a14.

gitgitgadget[bot] commented 3 months ago

This patch series was integrated into seen via https://github.com/git/git/commit/4d597962132bc8ac0caa654c2ca3b1238ad5887d.

gitgitgadget[bot] commented 3 months ago

This patch series was integrated into seen via https://github.com/git/git/commit/697fd878ad0c17f00e33faa7984989095b20a11f.

gitgitgadget[bot] commented 3 months ago

This patch series was integrated into seen via https://github.com/git/git/commit/6c0bfce914f951d673af91f0cad733b0d465fafb.

gitgitgadget[bot] commented 3 months ago

This patch series was integrated into master via https://github.com/git/git/commit/6c0bfce914f951d673af91f0cad733b0d465fafb.

gitgitgadget[bot] commented 3 months ago

This patch series was integrated into next via https://github.com/git/git/commit/6c0bfce914f951d673af91f0cad733b0d465fafb.

gitgitgadget[bot] commented 3 months ago

Closed via 6c0bfce914f951d673af91f0cad733b0d465fafb.