Christian Couder <chriscool@xxxxxxxxxxxxx> writes:
> As Daniel Barkalow found, there is a difference between this new
> version and the old one. The old version gives an error for
> "git reset --merge" with unmerged entries and the new version does
> not. But this can be seen as a bug fix,...
I sense that there is one crucial sentence missing here for a reader to
judge the "can be seen as a bug fix" claim. Instead of giving an error
and stopping, what _does_ this version do? If it ran "rm -rf" on the
entire work tree instead of giving an error, it wouldn't be a bugfix, for
example ;-)
> In fact there is still an error with unmerge entries if we reset
> the unmerge entries to the same state as HEAD. So the bug is not
> completely fixed.
"If we reset to HEAD then it is a bug"---and what the patch actually does
is...?
I agree with the analysis in the log message of 9e8ecea (Add 'merge' mode
to 'git reset', 2008-12-01):
If the index has unmerged entries, "--merge" will currently simply
refuse to reset ("you need to resolve your current index first").
You'll need to use "--hard" or similar in this case.
This is sad, because normally a unmerged index means that the working
tree file should have matched the source tree, so the correct action
is likely to make --merge reset such a path to the target (like --hard),
regardless of dirty state in-tree or in-index.
If I have a local change that I know is unrelated to a merge I am
going to do, I can imagine I'd work like this:
$ git pull some-where
... Some paths conflict, others merge cleanly and update
... the index, but the overall change looks horrible, and
... I decide to discard the whole thing.
$ git reset --merge HEAD
In this case HEAD is the target. Similarly, if I have a local change that
I know is unrelated to a series of patches I am applying:
$ git am -3 topic.mbox
... A few patches apply cleanly, and then the series fail
... with conflict. The overall change looks horrible, and
... I decide to discard the whole thing.
$ git reset --merge @{3.minutes.ago}
In this case, the target is the commit that I was at before starting to
apply the series---i.e. different from HEAD.
In either case, because "merge" (triggered by "pull") and "am -3" honor
the promise with the user that:
(1) no mergy (aka "integration") command stuffs an unmerged entry to an
index that is dirty with respect to HEAD (this should also apply to
"cherry-pick" and "rebase -m/-i" even though the last one is often
stricter than it is absolutely necessary); and
(2) every mergy command verifies that the index entry and the path in the
work tree do not have any local change before they are updated by it;
resetting can safely overwrite both the index entry and the path in the
work tree with contents taken from the commit we are switching to.
The updated test seems to be testing that "reset --merge HEAD^" does make
the index match the target, but it only checks "is there _any_ change",
and does not even test "which path kept the change and which path got
cloberred".
Ideally it should test "Is the change what we expect to have? Did we keep
what we should have kept, instead of clobbering? Did we discard the
changes to the path that the failed merge cloberred?", all of the three.
> diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
> index 8190da1..6afaf73 100755
> --- a/t/t7110-reset-merge.sh
> +++ b/t/t7110-reset-merge.sh
> @@ -79,10 +79,12 @@ test_expect_success 'setup 2 different branches' '
> git commit -a -m "change in branch2"
> '
>
> -test_expect_success '"reset --merge HEAD^" fails with pending merge' '
> +test_expect_success '"reset --merge HEAD^" is ok with pending merge' '
> test_must_fail git merge branch1 &&
> - test_must_fail git reset --merge HEAD^ &&
> - git reset --hard HEAD
> + git reset --merge HEAD^ &&
> + test -z "$(git diff --cached)" &&
> + test -n "$(git diff)" &&
> + git reset --hard HEAD@{1}
> '
>
> test_expect_success '"reset --merge HEAD" fails with pending merge' '
> --
> 1.6.6.rc1.8.gd33ec
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html