From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, ZheNing Hu <adlternative@gmail.com>,
Eric Sunshine <sunshine@sunshineco.com>,
Junio C Hamano <gitster@pobox.com>,
Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH v4 2/7] merge-resolve: abort if index does not match HEAD
Date: Fri, 22 Jul 2022 12:27:55 +0200 [thread overview]
Message-ID: <220722.86sfmts9vr.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <b79f44e54b9611fa2b10a4e1cb666992d006951c.1658466942.git.gitgitgadget@gmail.com>
On Fri, Jul 22 2022, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
>
> 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 <newren@gmail.com>
> ---
> 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...?
> + 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?
> # The first parameters up to -- are merge bases; the rest are heads.
> bases= head= remotes= sep_seen=
> for arg
> diff --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
next prev parent reply other threads:[~2022-07-22 10:46 UTC|newest]
Thread overview: 87+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-19 16:26 [PATCH 0/2] Fix merge restore state Elijah Newren via GitGitGadget
2022-05-19 16:26 ` [PATCH 1/2] merge: remove unused variable Elijah Newren via GitGitGadget
2022-05-19 17:45 ` Junio C Hamano
2022-05-19 16:26 ` [PATCH 2/2] merge: make restore_state() do as its name says Elijah Newren via GitGitGadget
2022-05-19 17:44 ` Junio C Hamano
2022-05-19 18:32 ` Junio C Hamano
2022-06-12 6:58 ` [PATCH 0/2] Fix merge restore state Elijah Newren
2022-06-12 8:54 ` ZheNing Hu
2022-06-19 6:50 ` [PATCH v2 0/6] " Elijah Newren via GitGitGadget
2022-06-19 6:50 ` [PATCH v2 1/6] t6424: make sure a failed merge preserves local changes Junio C Hamano via GitGitGadget
2022-06-19 6:50 ` [PATCH v2 2/6] merge: remove unused variable Elijah Newren via GitGitGadget
2022-07-19 23:14 ` Junio C Hamano
2022-06-19 6:50 ` [PATCH v2 3/6] merge: fix save_state() to work when there are racy-dirty files Elijah Newren via GitGitGadget
2022-07-17 16:28 ` ZheNing Hu
2022-07-19 22:49 ` Junio C Hamano
2022-07-21 1:09 ` Elijah Newren
2022-07-19 22:43 ` Junio C Hamano
2022-06-19 6:50 ` [PATCH v2 4/6] merge: make restore_state() restore staged state too Elijah Newren via GitGitGadget
2022-07-17 16:37 ` ZheNing Hu
2022-07-19 23:14 ` Junio C Hamano
2022-07-19 23:28 ` Junio C Hamano
2022-07-21 1:37 ` Elijah Newren
2022-06-19 6:50 ` [PATCH v2 5/6] merge: ensure we can actually restore pre-merge state Elijah Newren via GitGitGadget
2022-07-17 16:41 ` ZheNing Hu
2022-07-19 22:57 ` Junio C Hamano
2022-07-21 2:03 ` Elijah Newren
2022-06-19 6:50 ` [PATCH v2 6/6] merge: do not exit restore_state() prematurely Elijah Newren via GitGitGadget
2022-07-17 16:44 ` ZheNing Hu
2022-07-19 23:13 ` Junio C Hamano
2022-07-20 0:09 ` Eric Sunshine
2022-07-21 2:03 ` Elijah Newren
2022-07-21 3:27 ` Elijah Newren
2022-07-21 8:16 ` [PATCH v3 0/7] Fix merge restore state Elijah Newren via GitGitGadget
2022-07-21 8:16 ` [PATCH v3 1/7] merge-ort-wrappers: make printed message match the one from recursive Elijah Newren via GitGitGadget
2022-07-21 15:47 ` Junio C Hamano
2022-07-21 19:51 ` Elijah Newren
2022-07-21 20:05 ` Junio C Hamano
2022-07-21 21:14 ` Elijah Newren
2022-07-21 8:16 ` [PATCH v3 2/7] merge-resolve: abort if index does not match HEAD Elijah Newren via GitGitGadget
2022-07-21 8:16 ` [PATCH v3 3/7] merge: do not abort early if one strategy fails to handle the merge Elijah Newren via GitGitGadget
2022-07-21 16:09 ` Junio C Hamano
2022-07-25 10:38 ` Ævar Arnfjörð Bjarmason
2022-07-26 1:31 ` Elijah Newren
2022-07-26 6:54 ` Ævar Arnfjörð Bjarmason
2022-07-21 8:16 ` [PATCH v3 4/7] merge: fix save_state() to work when there are stat-dirty files Elijah Newren via GitGitGadget
2022-07-21 8:16 ` [PATCH v3 5/7] merge: make restore_state() restore staged state too Elijah Newren via GitGitGadget
2022-07-21 16:16 ` Junio C Hamano
2022-07-21 16:24 ` Junio C Hamano
2022-07-21 19:52 ` Elijah Newren
2022-07-21 8:16 ` [PATCH v3 6/7] merge: ensure we can actually restore pre-merge state Elijah Newren via GitGitGadget
2022-07-21 16:31 ` Junio C Hamano
2023-03-02 7:17 ` Ben Humphreys
2023-03-02 15:35 ` Elijah Newren
2023-03-02 16:19 ` Junio C Hamano
2023-03-04 16:18 ` Rudy Rigot
2023-03-06 22:19 ` Ben Humphreys
2022-07-21 8:16 ` [PATCH v3 7/7] merge: do not exit restore_state() prematurely Elijah Newren via GitGitGadget
2022-07-21 16:34 ` Junio C Hamano
2022-07-22 5:15 ` [PATCH v4 0/7] Fix merge restore state Elijah Newren via GitGitGadget
2022-07-22 5:15 ` [PATCH v4 1/7] merge-ort-wrappers: make printed message match the one from recursive Elijah Newren via GitGitGadget
2022-07-22 5:15 ` [PATCH v4 2/7] merge-resolve: abort if index does not match HEAD Elijah Newren via GitGitGadget
2022-07-22 10:27 ` Ævar Arnfjörð Bjarmason [this message]
2022-07-23 0:28 ` Elijah Newren
2022-07-23 5:44 ` Ævar Arnfjörð Bjarmason
2022-07-26 1:58 ` Elijah Newren
2022-07-26 6:35 ` Ævar Arnfjörð Bjarmason
2022-07-22 5:15 ` [PATCH v4 3/7] merge: do not abort early if one strategy fails to handle the merge Elijah Newren via GitGitGadget
2022-07-22 10:47 ` Ævar Arnfjörð Bjarmason
2022-07-23 0:36 ` Elijah Newren
2022-07-22 5:15 ` [PATCH v4 4/7] merge: fix save_state() to work when there are stat-dirty files Elijah Newren via GitGitGadget
2022-07-22 5:15 ` [PATCH v4 5/7] merge: make restore_state() restore staged state too Elijah Newren via GitGitGadget
2022-07-22 10:53 ` Ævar Arnfjörð Bjarmason
2022-07-23 1:56 ` Elijah Newren
2022-07-22 5:15 ` [PATCH v4 6/7] merge: ensure we can actually restore pre-merge state Elijah Newren via GitGitGadget
2022-07-22 5:15 ` [PATCH v4 7/7] merge: do not exit restore_state() prematurely Elijah Newren via GitGitGadget
2022-07-23 1:53 ` [PATCH v5 0/8] Fix merge restore state Elijah Newren via GitGitGadget
2022-07-23 1:53 ` [PATCH v5 1/8] merge-ort-wrappers: make printed message match the one from recursive Elijah Newren via GitGitGadget
2022-07-23 1:53 ` [PATCH v5 2/8] merge-resolve: abort if index does not match HEAD Elijah Newren via GitGitGadget
2022-07-23 1:53 ` [PATCH v5 3/8] merge: abort if index does not match HEAD for trivial merges Elijah Newren via GitGitGadget
2022-07-23 1:53 ` [PATCH v5 4/8] merge: do not abort early if one strategy fails to handle the merge Elijah Newren via GitGitGadget
2022-07-23 1:53 ` [PATCH v5 5/8] merge: fix save_state() to work when there are stat-dirty files Elijah Newren via GitGitGadget
2022-07-23 1:53 ` [PATCH v5 6/8] merge: make restore_state() restore staged state too Elijah Newren via GitGitGadget
2022-07-23 1:53 ` [PATCH v5 7/8] merge: ensure we can actually restore pre-merge state Elijah Newren via GitGitGadget
2022-07-23 1:53 ` [PATCH v5 8/8] merge: do not exit restore_state() prematurely Elijah Newren via GitGitGadget
2022-07-25 19:03 ` [PATCH v5 0/8] Fix merge restore state Junio C Hamano
2022-07-26 1:59 ` Elijah Newren
2022-07-26 4:03 ` ZheNing Hu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=220722.86sfmts9vr.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=adlternative@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=newren@gmail.com \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).