Re: [PATCH v4 2/7] merge-resolve: abort if index does not match HEAD
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-07-22 10:46:23
On Fri, Jul 22 2022, Elijah Newren via GitGitGadget wrote:
quoted hunk ↗ jump to hunk
From: Elijah Newren <redacted> As noted in commit 9822175d2b ("Ensure index matches head before invoking merge machinery, round N", 2019-08-17), we have had a very long history of problems with failing to enforce the requirement that index matches HEAD when starting a merge. One of the commits referenced in the long tale of issues arising from lax enforcement of this requirement was commit 55f39cf755 ("merge: fix misleading pre-merge check documentation", 2018-06-30), which tried to document the requirement and noted there were some exceptions. As mentioned in that commit message, the `resolve` strategy was the one strategy that did not have an explicit index matching HEAD check, and the reason it didn't was that I wasn't able to discover any cases where the implementation would fail to catch the problem and abort, and didn't want to introduce unnecessary performance overhead of adding another check. Well, today I discovered a testcase where the implementation does not catch the problem and so an explicit check is needed. Add a testcase that previously would have failed, and update git-merge-resolve.sh to have an explicit check. Note that the code is copied from 3ec62ad9ff ("merge-octopus: abort if index does not match HEAD", 2016-04-09), so that we reuse the same message and avoid making translators need to translate some new message. Signed-off-by: Elijah Newren <redacted> --- builtin/merge.c | 20 ++++++++++++++++++ git-merge-resolve.sh | 10 +++++++++ t/t6424-merge-unrelated-index-changes.sh | 26 ++++++++++++++++++++++++ 3 files changed, 56 insertions(+)diff --git a/builtin/merge.c b/builtin/merge.c index 23170f2d2a6..13884b8e836 100644 --- a/builtin/merge.c +++ b/builtin/merge.c@@ -1599,6 +1599,26 @@ int cmd_merge(int argc, const char **argv, const char *prefix) */ refresh_cache(REFRESH_QUIET); if (allow_trivial && fast_forward != FF_ONLY) { + /* + * Must first ensure that index matches HEAD before + * attempting a trivial merge. + */ + struct tree *head_tree = get_commit_tree(head_commit); + struct strbuf sb = STRBUF_INIT; + + if (repo_index_has_changes(the_repository, head_tree, + &sb)) { + struct strbuf err = STRBUF_INIT; + strbuf_addstr(&err, "error: "); + strbuf_addf(&err, _("Your local changes to the following files would be overwritten by merge:\n %s"), + sb.buf); + strbuf_addch(&err, '\n');
At first glance I was expecting this to construct an error message to emit it somewhere else that stderr, so I wondered if you couldn't use the "error_routine" facility to avoid re-inventing "error: " etc., but...
+ fputs(err.buf, stderr);
...we emit it to stderr anyway...?
quoted hunk ↗ jump to hunk
+ strbuf_release(&err); + strbuf_release(&sb); + return -1; + } + /* See if it is really trivial. */ git_committer_info(IDENT_STRICT); printf(_("Trying really trivial in-index merge...\n"));diff --git a/git-merge-resolve.sh b/git-merge-resolve.sh index 343fe7bccd0..77e93121bf8 100755 --- a/git-merge-resolve.sh +++ b/git-merge-resolve.sh@@ -5,6 +5,16 @@ # # Resolve two trees, using enhanced multi-base read-tree. +. git-sh-setup + +# Abort if index does not match HEAD +if ! git diff-index --quiet --cached HEAD -- +then + gettextln "Error: Your local changes to the following files would be overwritten by merge" + git diff-index --cached --name-only HEAD -- | sed -e 's/^/ /' + exit 2 +fi
(The "..." continued below)
Just in trying to poke holes in this I made this an "exit 0", and
neither of the tests you added failed, but the last one ("resolve &&
recursive && ort") in the t6424*.sh will fail, is that intentional?
I don't know enough about the context here, but given our *.sh->C
migration elsewhere it's a bit unfortunate to see more *.sh code added
back. We have "git merge" driving this, isn't it OK to have it make this
check before invoking "resolve" (may be a stupid question).
For this code in particular it:
* Uses spaces, not tabs
* We lose the diff-index .. --name-only exit code (segfault), but so
did the older version
* I wonder if bending over backwards to emit the exact message we
emitted before is worth it
If you just make this something like (untested):
{
gettext "error: " &&
gettextln "Your local..."
}
You could re-use the translation from the *.c one (and the "error: " one
we'll get from usage.c).
That leaves "\n %s" as the difference, but we could just remove that
from the _() and emit it unconditionally, no?
quoted hunk ↗ jump to hunk
# The first parameters up to -- are merge bases; the rest are heads. bases= head= remotes= sep_seen= for argdiff --git a/t/t6424-merge-unrelated-index-changes.sh b/t/t6424-merge-unrelated-index-changes.sh index b6e424a427b..f35d3182b86 100755 --- a/t/t6424-merge-unrelated-index-changes.sh +++ b/t/t6424-merge-unrelated-index-changes.sh@@ -114,6 +114,32 @@ test_expect_success 'resolve, non-trivial' ' test_path_is_missing .git/MERGE_HEAD ' +test_expect_success 'resolve, trivial, related file removed' ' + git reset --hard && + git checkout B^0 && + + git rm a && + test_path_is_missing a && + + test_must_fail git merge -s resolve C^0 && + + test_path_is_missing a && + test_path_is_missing .git/MERGE_HEAD +' + +test_expect_success 'resolve, non-trivial, related file removed' ' + git reset --hard && + git checkout B^0 && + + git rm a && + test_path_is_missing a && + + test_must_fail git merge -s resolve D^0 && + + test_path_is_missing a && + test_path_is_missing .git/MERGE_HEAD +' + test_expect_success 'recursive' ' git reset --hard && git checkout B^0 &&
...I tried with this change on top, it seems to me like you'd want this
in any case, it passes the tests both with & without the C code change,
so can't we just use error() here?
diff --git a/builtin/merge.c b/builtin/merge.c
index 7fb4414ebb7..64def49734a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1621,13 +1621,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
if (repo_index_has_changes(the_repository, head_tree,
&sb)) {
- struct strbuf err = STRBUF_INIT;
- strbuf_addstr(&err, "error: ");
- strbuf_addf(&err, _("Your local changes to the following files would be overwritten by merge:\n %s"),
- sb.buf);
- strbuf_addch(&err, '\n');
- fputs(err.buf, stderr);
- strbuf_release(&err);
+ error(_("Your local changes to the following files would be overwritten by merge:\n %s"),
+ sb.buf);
strbuf_release(&sb);
return -1;
}
diff --git a/t/t6424-merge-unrelated-index-changes.sh b/t/t6424-merge-unrelated-index-changes.sh
index c96649448fa..1df130b9ee6 100755
--- a/t/t6424-merge-unrelated-index-changes.sh
+++ b/t/t6424-merge-unrelated-index-changes.sh
@@ -96,7 +96,12 @@ test_expect_success 'resolve, trivial' '
touch random_file && git add random_file &&
- test_must_fail git merge -s resolve C^0 &&
+ sed -e "s/^> //g" >expect <<-\EOF &&
+ > error: Your local changes to the following files would be overwritten by merge:
+ > random_file
+ EOF
+ test_must_fail git merge -s resolve C^0 2>actual &&
+ test_cmp expect actual &&
test_path_is_file random_file &&
git rev-parse --verify :random_file &&
test_path_is_missing .git/MERGE_HEAD