All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
To: git@vger.kernel.org
Cc: "Phillip Wood" <phillip.wood123@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Elijah Newren" <newren@gmail.com>,
	"Jonathan Nieder" <jrnieder@gmail.com>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Phillip Wood" <phillip.wood@dunelm.org.uk>
Subject: [PATCH 4/4] checkout: prevent losing staged changes with --merge
Date: Fri, 22 Mar 2019 16:31:38 +0700	[thread overview]
Message-ID: <20190322093138.13765-5-pclouds@gmail.com> (raw)
In-Reply-To: <20190322093138.13765-1-pclouds@gmail.com>

When --merge is specified, we may need to do a real merge (instead of
three-way tree unpacking), the steps are best seen in git-checkout.sh
version before it's removed:

    # Match the index to the working tree, and do a three-way.
    git diff-files --name-only | git update-index --remove --stdin &&
    work=`git write-tree` &&
    git read-tree $v --reset -u $new || exit

    git merge-recursive $old -- $new $work

    # Do not register the cleanly merged paths in the index yet.
    # this is not a real merge before committing, but just carrying
    # the working tree changes along.
    unmerged=`git ls-files -u`
    git read-tree $v --reset $new
    case "$unmerged" in
    '')     ;;
    *)
            (
                    z40=0000000000000000000000000000000000000000
                    echo "$unmerged" |
                    sed -e 's/^[0-7]* [0-9a-f]* /'"0 $z40 /"
                    echo "$unmerged"
            ) | git update-index --index-info
            ;;
    esac

Notice the last 'read-tree --reset' step. We restore worktree back to
'new' tree after worktree's messed up by merge-recursive. If there are
staged changes before this whole command sequence is executed, they
are lost because they are unlikely part of the 'new' tree to be
restored.

There is no easy way to fix this. Elijah may have something up his
sleeves [1], but until then, check if there are staged changes and
refuse to run and lose them. The user would need to do "git reset" to
continue in this case.

A note about the test update. 'checkout -m' in that test will fail
because a deletion is staged. This 'checkout -m' was previously needed
to verify quietness behavior of unpack-trees. But a different check
has been put in place in the last patch. We can safely drop
'checkout -m' now.

[1] CABPp-BFoL_U=bzON4SEMaQSKU2TKwnOgNqjt5MUaOejTKGUJxw@mail.gmail.com

Reported-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/checkout.c | 11 ++++++++++-
 t/t7201-co.sh      | 10 +---------
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 22fb6c0cae..7cd01f62be 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -725,7 +725,10 @@ static int merge_working_tree(const struct checkout_opts *opts,
 			 */
 			struct tree *result;
 			struct tree *work;
+			struct tree *old_tree;
 			struct merge_options o;
+			struct strbuf sb = STRBUF_INIT;
+
 			if (!opts->merge)
 				return 1;
 
@@ -735,6 +738,12 @@ static int merge_working_tree(const struct checkout_opts *opts,
 			 */
 			if (!old_branch_info->commit)
 				return 1;
+			old_tree = get_commit_tree(old_branch_info->commit);
+
+			if (repo_index_has_changes(the_repository, old_tree, &sb))
+				die(_("cannot continue with staged changes in "
+				      "the following files:\n%s"), sb.buf);
+			strbuf_release(&sb);
 
 			/* Do more real merge */
 
@@ -772,7 +781,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
 			ret = merge_trees(&o,
 					  get_commit_tree(new_branch_info->commit),
 					  work,
-					  get_commit_tree(old_branch_info->commit),
+					  old_tree,
 					  &result);
 			if (ret < 0)
 				exit(128);
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index f165582019..5990299fc9 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -224,15 +224,7 @@ test_expect_success 'switch to another branch while carrying a deletion' '
 	test_i18ngrep overwritten errs &&
 
 	test_must_fail git read-tree --quiet -m -u HEAD simple 2>errs &&
-	test_must_be_empty errs &&
-
-	git checkout --merge simple 2>errs &&
-	test_i18ngrep ! overwritten errs &&
-	git ls-files -u &&
-	test_must_fail git cat-file -t :0:two &&
-	test "$(git cat-file -t :1:two)" = blob &&
-	test "$(git cat-file -t :2:two)" = blob &&
-	test_must_fail git cat-file -t :3:two
+	test_must_be_empty errs
 '
 
 test_expect_success 'checkout to detach HEAD (with advice declined)' '
-- 
2.21.0.548.gd3c7d92dc2


  parent reply	other threads:[~2019-03-22  9:32 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-22  9:31 [PATCH 0/4] prevent 'checkout -m' from losing staged changes Nguyễn Thái Ngọc Duy
2019-03-22  9:31 ` [PATCH 1/4] unpack-trees: keep gently check inside add_rejected_path Nguyễn Thái Ngọc Duy
2019-03-22  9:31 ` [PATCH 2/4] unpack-trees: rename "gently" flag to "quiet" Nguyễn Thái Ngọc Duy
2019-03-22  9:31 ` [PATCH 3/4] read-tree: add --quiet Nguyễn Thái Ngọc Duy
2019-03-22 17:18   ` Philip Oakley
2019-03-22  9:31 ` Nguyễn Thái Ngọc Duy [this message]
2019-03-25 16:07   ` [PATCH 4/4] checkout: prevent losing staged changes with --merge Elijah Newren

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=20190322093138.13765-5-pclouds@gmail.com \
    --to=pclouds@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    --cc=newren@gmail.com \
    --cc=phillip.wood123@gmail.com \
    --cc=phillip.wood@dunelm.org.uk \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.