v2, 4 parts"git reset --merge" related improvements

In this new version, the test cases that were bogus are now fixed. And the new "git reset" option is now called "--merge-safe" (instead of "--merge-dirty").

To make the test cases pass I had to change a little bit the way "--merge-safe" is implemented. It now does not set the "reset" flag in the "struct unpack_trees_options" passed to "unpack_trees()".

And commit messages have been improved, thanks to input from Daniel, Junio and Linus. And there is no more "exec </dev/null" in the test script, thanks to Jakub.

I prefer to keep Stephan as the author of patch 3/4 because he designed and implemented the new feature in the first place.

I am working on the documentation for "--merge-safe" and on improving the existing "git reset" documentation using a table at the same time. So another patch will be added to this series later.

Christian Couder (2):
  reset: add a few tests for "git reset --merge"
  reset: add test cases for "--merge-safe" option

Stephan Beyer (2):
  reset: use "unpack_trees()" directly instead of "git read-tree"
  reset: add option "--merge-safe" to "git reset"

 builtin-reset.c        |   81 ++++++++++++++++++++++++++++-------
 t/t7110-reset-merge.sh |  112 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 177 insertions(+), 16 deletions(-)
 create mode 100755 t/t7110-reset-merge.sh

Commit 9e8eceab ("Add 'merge' mode to 'git reset'", 2008-12-01), added the --merge option to git reset, but there were no test cases for it.

This was not a big problem because "git reset" was just forking and execing "git read-tree", but this will change in a following patch.

So let's add a few test cases to make sure that there will be no regression.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t7110-reset-merge.sh |   68 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 68 insertions(+), 0 deletions(-)
 create mode 100755 t/t7110-reset-merge.sh

From: Stephan Beyer <s-beyer@gmx.net>

This patch makes "reset_index_file()" call "unpack_trees()" directly instead of forking and execing "git read-tree". So the code is more efficient.

And it's also easier to see which unpack_tree() options will be used, as we don't need to follow "git read-tree"'s command line parsing which is quite complex.

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, because "--merge" was the only "git reset" option with this behavior and this behavior was not documented.

The code comes from the sequencer GSoC project:

git://repo.or.cz/git/sbeyer.git

(at commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)

Mentored-by: Daniel Barkalow <barkalow@iabervon.org>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin-reset.c |   51 ++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 40 insertions(+), 11 deletions(-)

From: Stephan Beyer <s-beyer@gmx.net>

This option is nearly like "--merge" except that it is safer. The table below show the differences between these options.

working index HEAD target         working index HEAD
  B      B     A     A   --m-s      B      A     A
                         --merge    A      A     A
  B      B     A     C   --m-s       (disallowed)
                         --merge    C      C     C

In this table, A, B and C are some different states of a file. For example the first 2 lines of the table mean that if a file is in state B in the working tree and the index, and in a different state A in HEAD and in the target, then "git reset --merge-safe target" will put the file in state B in the working tree and in state A in the index and HEAD.

So as can be seen in the table, "--merge" discards changes in the index, while "--merge-safe" does not.

A following patch will add some test cases for "--merge-safe", where the differences between "--merge" and "--merge-safe" can also be seen.

The "--merge-safe" option is implemented by doing a 2 way merge between HEAD and the reset target, and if this succeeds by doing a mixed reset to the target.

The code comes from the sequencer GSoC project:

git://repo.or.cz/git/sbeyer.git

(at commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)

But in the sequencer project the "reset" flag was set in the "struct unpack_trees_options" passed to "unpack_trees()". With this flag the changes in the working tree were discarded if the file was different between HEAD and the reset target.

Mentored-by: Daniel Barkalow <barkalow@iabervon.org>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin-reset.c |   30 +++++++++++++++++++++++++-----
 1 files changed, 25 insertions(+), 5 deletions(-)

Christian Couder <chriscool@tuxfamily.org> writes:

At first, I had to spend a few minutes guessing what you meant by "target" in the table. All the other words are well known and do not need to be explained, but you can make it even easier to understand by updating the sentence before the table, perhaps like:

    When running "git reset --option target" to reset the HEAD to another
    commit (as a special case "target" could be the same as HEAD), here
    is what happens to paths in various state.

As you mentioned in the proposed commit log message of the other entry, you have a different behaviour for unmerged case. Can you add that case to the table as well?

The original use case of Linus's "reset --merge" was:

    $ edit ... ;# you may have some local changes to the work tree
    $ git merge/pull ...
    ... (1) it merges cleanly;
    ... (2) you see conflicts and do not commit, or
    ... (3) you resolve conflicts and commit, while leaving the earlier
    ...     modified paths alone.
    ... In any of the three cases, you inspect the result, and say
    ... "ah, crap!"
    ... You want to go back to the state before the merge, i.e.
    ... target = HEAD^ in (1) or (3) above and target = HEAD in (2).
    $ git reset --merge $target

Recall that "git merge/pull ..." step does not even touch anything if you have a dirty index (i.e. "diff --cached" is non-empty), so any difference between the index and HEAD to reset the failed merge away must come from the merge itself

Immediately before you run "reset --merge" in the above sequence, you can categorize the paths in various states this way:

  work   index  HEAD  how that path got into this state...
    A      A     A    not involved in this merge.
    B      A     A    not involved in this merge, originally modified.
    B      B     A    cleanly merged.
    B      B     B    cleanly merged (and committed if (1) or (3)).
    C      U     A    merge left conflicts

where U denotes unmerged path in the index, and C is a file in the work tree with conflict markers. The path had content A in HEAD before the start of the merge that you are resetting away.

Note that the target is A in all cases in the above table.

We would want to go back to HEAD == index == target for all of these cases, and discarding B in "cleanly merged" entries is absolutely the right thing to do. Clearly, --merge-safe is _not_ designed to work in this scenario.

Don't get me wrong.  I am not saying --merge-safe is unsafe nor useless.

I am trying to show a way with an intended use-case (and the mechanical notation you and Daniel wrote up, which is very nice to illustrate what exactly happens) to explain how --merge works, and more importantly why it works that way.

That is because I would like to see an intended use case of this new feature in a bigger picture. With the table, I can see what it does (or at least what you wanted it to do), but it does not clearly explain what this new mode of operation is good for, in what context it is designed to be used, and what problem it intends to solve. I think you find it very clear, by reading my explanation above, what Linus's --merge is intended to solve:

	After a (possibly conflicted) merge modified my index and my work
        tree, "reset --hard" to the commit before I started the merge will
        discard the local modifications I had in the work tree before the
        merge.  "reset --merge" was invented to let me go back to the
        state before the merge, without discarding such local changes I
        had before the merge.

I want to be able to explain to others what --merge-safe is for in a similar way myself before I have it in my tree, and I can't (yet).

On Wednesday 16 September 2009, Junio C Hamano wrote:

Ok, I will update the sentence like this.

The behavior is not different between --merge and --merge-safe, the behavior is different between --merge before patch 2/4 and --merge after patch 2/4. I will add a test case to show this.

I think that Daniel found out that the above "reset --merge" command did not worked well in case (2) before patch 2/4.

Yes.

I understand, and I think that Stephan designed the "allow_dirty" feature (that became --merge-safe in this patch series) because he wanted to let the user do a cherry-pick or an improved cherry-pick (or even run the sequencer) with a dirty working tree or index without the risk of losing some work.

But I think that it can be usefull in case like:

$ "hack something" $ git commit ... $ "hack something else" $ git add "some of the files" $ "find out previous commit was crap" $ git reset --merge-safe HEAD^

Here using "--merge-safe" can be usefull because you don't want to lose stuff in your current index and work tree.

Best regards, Christian.

Christian Couder <chriscool@tuxfamily.org> writes:

That depends on the reason why you are resetting away the "crap commit" and what your next move is.

If you are going to run "git commit", perhaps after some further edit, in order to fix what the "crap commit" did not quite do right, what your new option does is actively a wrong thing to do. "Some of the files" you added after that "crap commit" are about "hack something else", i.e. not part of the work to fix the "crap commit", and their changes should not be included in the amended commit, no?

In general, "reset" should be about matching the index entries to the named commit, so that you can start from the clean, known slate to redo the commit you wanted to make on top of it. You would need a very good use case to deviate from it and leave the index entry different from that of the commit you are switching to; otherwise you would greatly confuse the end users.

This shows that with the "--merge-safe" option, changes that are both in the work tree and the index are kept in the work tree after the reset (but discarded in the index). As with the "--merge" option, changes that are in both the work tree and the index are discarded after the reset.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t7110-reset-merge.sh |   50 +++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 47 insertions(+), 3 deletions(-)