reproducible unexpected behavior for 'git reset'

reproducible unexpected behavior for 'git reset'

[John Nowak]

I am able to reproduce a scenario where, after a 'commit' and a 'stash pop' that results in a merge conflict, I need to 'reset' a file twice in order to get the index back to HEAD. It appears that the first 'reset' sets the index to the merge base version instead of HEAD which was, for me, rather unexpected. I encountered this on 1.7.4 but others have reproduced it on the latest master. If this behavior is documented, I cannot find it.

A transcript to reproduce this oddity is below; note how the file is partially staged after the first 'reset' and unstaged after the second:

---

$ git init
Initialized empty Git repository in /Users/jn/test/.git/

$ echo "a" > foo

$ git add foo

$ git commit -a -m "Initial"
[master (root-commit) 5214837] Initial
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 foo

$ echo "b" >> foo

$ git stash
Saved working directory and index state WIP on master: 5214837 Initial
HEAD is now at 5214837 Initial

$ echo "c" >> foo

$ git add foo

$ git commit -a -m "Added c"
[master 69eef48] Added c
 1 files changed, 1 insertions(+), 0 deletions(-)

$ git stash pop
Auto-merging foo
CONFLICT (content): Merge conflict in foo

$ git status
# On branch master
# Unmerged paths:
#   (use "git reset HEAD <file>..." to unstage)
#   (use "git add/rm <file>..." as appropriate to mark resolution)
#
#	both modified:      foo
#
no changes added to commit (use "git add" and/or "git commit -a")

$ git reset foo
Unstaged changes after reset:
M	foo

$ git status
# On branch master
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#	modified:   foo
#
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	modified:   foo
#

$ git reset foo
Unstaged changes after reset:
M	foo

$ git status
# On branch master
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	modified:   foo
#
no changes added to commit (use "git add" and/or "git commit -a")

reproducible unexpected behavior for 'git reset'

[John Nowak]

(Apologies if you receive this twice. I sent the first copy before confirming my list subscription and I'm not sure if it went through.)

I am able to reproduce a scenario where, after a 'commit' and a 'stash pop' that results in a merge conflict, I need to 'reset' a file twice in order to get the index back to HEAD. It appears that the first 'reset' sets the index to the merge base version instead of HEAD which was, for me, rather unexpected. I encountered this on 1.7.4 but others have reproduced it on the latest master. If this behavior is documented, I cannot find it.

A transcript to reproduce this oddity is below; note how the file is partially staged after the first 'reset' and unstaged after the second:

---

$ git init
Initialized empty Git repository in /Users/jn/test/.git/

$ echo "a" > foo

$ git add foo

$ git commit -a -m "Initial"
[master (root-commit) 5214837] Initial
1 files changed, 1 insertions(+), 0 deletions(-)
create mode 100644 foo

$ echo "b" >> foo

$ git stash
Saved working directory and index state WIP on master: 5214837 Initial
HEAD is now at 5214837 Initial

$ echo "c" >> foo

$ git add foo

$ git commit -a -m "Added c"
[master 69eef48] Added c
1 files changed, 1 insertions(+), 0 deletions(-)

$ git stash pop
Auto-merging foo
CONFLICT (content): Merge conflict in foo

$ git status
# On branch master
# Unmerged paths:
#   (use "git reset HEAD <file>..." to unstage)
#   (use "git add/rm <file>..." as appropriate to mark resolution)
#
#	both modified:      foo
#
no changes added to commit (use "git add" and/or "git commit -a")

$ git reset foo
Unstaged changes after reset:
M	foo

$ git status
# On branch master
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#	modified:   foo
#
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	modified:   foo
#

$ git reset foo
Unstaged changes after reset:
M	foo

$ git status
# On branch master
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	modified:   foo
#
no changes added to commit (use "git add" and/or "git commit -a")

Re: reproducible unexpected behavior for 'git reset'

[Junio C Hamano]

John Nowak <john@johnnowak.com> writes:

> I am able to reproduce a scenario where, after a 'commit' and a 'stash
> pop' that results in a merge conflict, I need to 'reset' a file twice in
> order to get the index back to HEAD.

Thanks, you found a bug in "git reset [<commit>] -- path" codepath, it
seems, when dealing with an unmerged path.

Re: reproducible unexpected behavior for 'git reset'

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> I am able to reproduce a scenario where, after a 'commit' and a 'stash
>> pop' that results in a merge conflict, I need to 'reset' a file twice in
>> order to get the index back to HEAD.
>
> Thanks, you found a bug in "git reset [<commit>] -- path" codepath, it
> seems, when dealing with an unmerged path.

This patch is probably a wrong way to fix this issue; I have this
suspicion that the original code for "reset [<commit>] -- paths..."
codepath is correctly designed to deal with unmerged index, and it would
probably need a deeper surgery.

-- >8 --
Subject: [PATCH] reset [<commit>] paths...: do not mishandle unmerged paths

By reading the index with unmerged content before running diff-index
with the tree-ish we are reading from, read_from_tree() function ended
up stuffing the object name from a wrong stage to the resulting index.

Noticed by John Nowak.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 builtin/reset.c  |    2 +-
 t/t7102-reset.sh |   15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 98bca04..09c8d2a 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -202,7 +202,7 @@ static int read_from_tree(const char *prefix, const char **argv,
 
 	index_fd = hold_locked_index(lock, 1);
 	index_was_discarded = 0;
-	read_cache();
+	read_cache_unmerged();
 	if (do_diff_cache(tree_sha1, &opt))
 		return 1;
 	diffcore_std(&opt);
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index f1cfc9a..1784412 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -429,6 +429,21 @@ test_expect_success '--mixed refreshes the index' '
 	test_i18ncmp expect output
 '
 
+test_expect_success 'resetting specific path that is unmerged' '
+	F1=$(git rev-parse HEAD:file1) &&
+	F2=$(git rev-parse HEAD:file2) &&
+	F3=$(git rev-parse HEAD:secondfile) &&
+	{
+		echo "000000 $_z40 0	file2" &&
+		echo "100644 $F1 1	file2" &&
+		echo "100644 $F2 2	file2" &&
+		echo "100644 $F3 3	file2"
+	} | git update-index --index-info &&
+	git ls-files -u &&
+	test_must_fail git reset HEAD file2 &&
+	git diff-index --exit-code --cached HEAD
+'
+
 test_expect_success 'disambiguation (1)' '
 
 	git reset --hard &&

Re: reproducible unexpected behavior for 'git reset'

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> This patch is probably a wrong way to fix this issue...

Yes, it was a wrong way. Here is the hopefully right one ;-)

Funny thing is that this is a rather ancient bug, dating back to d1f2d7e
(Make run_diff_index() use unpack_trees(), not read_tree(), 2008-01-19)
and survived a fix for that commit at 29796c6 (diff-index: report unmerged
new entries, 2009-08-04). The reason it survived is probably because
people care about the fact that the path is unmerged, but not the exact
object name on the LHS of the diff (which comes from the tree).

-- >8 --
Subject: [PATCH] reset [<commit>] paths...: do not mishandle unmerged paths

Because "diff --cached HEAD" showed an incorrect blob object name on the
LHS of the diff, we ended up updating the index entry with bogus value,
not what we read from the tree.

Noticed by John Nowak.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/reset.c  |    2 +-
 diff-lib.c       |    3 ++-
 t/t7102-reset.sh |   15 +++++++++++++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 98bca04..777e7c6 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -162,7 +162,7 @@ static void update_index_from_diff(struct diff_queue_struct *q,
 
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filespec *one = q->queue[i]->one;
-		if (one->mode) {
+		if (one->mode && !is_null_sha1(one->sha1)) {
 			struct cache_entry *ce;
 			ce = make_cache_entry(one->mode, one->sha1, one->path,
 				0, 0);
diff --git a/diff-lib.c b/diff-lib.c
index 9c29293..fd61acb 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -379,7 +379,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	if (cached && idx && ce_stage(idx)) {
 		struct diff_filepair *pair;
 		pair = diff_unmerge(&revs->diffopt, idx->name);
-		fill_filespec(pair->one, idx->sha1, idx->ce_mode);
+		if (tree)
+			fill_filespec(pair->one, tree->sha1, tree->ce_mode);
 		return;
 	}
 
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index f1cfc9a..b096dc8 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -429,6 +429,21 @@ test_expect_success '--mixed refreshes the index' '
 	test_i18ncmp expect output
 '
 
+test_expect_success 'resetting specific path that is unmerged' '
+	git rm --cached file2 &&
+	F1=$(git rev-parse HEAD:file1) &&
+	F2=$(git rev-parse HEAD:file2) &&
+	F3=$(git rev-parse HEAD:secondfile) &&
+	{
+		echo "100644 $F1 1	file2" &&
+		echo "100644 $F2 2	file2" &&
+		echo "100644 $F3 3	file2"
+	} | git update-index --index-info &&
+	git ls-files -u &&
+	test_must_fail git reset HEAD file2 &&
+	git diff-index --exit-code --cached HEAD
+'
+
 test_expect_success 'disambiguation (1)' '
 
 	git reset --hard &&
-- 
1.7.6.178.g55272