Re: [PATCH v5 3/7] reset: use "unpack_trees()" directly instead of "git read-tree"

Christian Couder <chriscool@tuxfamily.org> writes:

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 ;-)

"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.