[PATCH v2 0/5] add "--keep" option to "git reset"

[PATCH v2 0/5] add "--keep" option to "git reset"

[Christian Couder]

Changes since the previous series are the following:

- removed parts of the commit message that contrasted "--keep" with "--merge"
- improved wording in the documentation
- added a new patch to disallow "--keep" when there are unmerged entries
- added some tests for --keep in t7111-reset-table.sh

Christian Couder (5):
  reset: add option "--keep" to "git reset"
  reset: add test cases for "--keep" option
  Documentation: reset: describe new "--keep" option
  reset: disallow "reset --keep" outside a work tree
  reset: disallow using --keep when there are unmerged entries

 Documentation/git-reset.txt |   48 +++++++++++++++++-
 builtin-reset.c             |   46 ++++++++++++++---
 t/t7103-reset-bare.sh       |   25 ++++++---
 t/t7110-reset-merge.sh      |  116 ++++++++++++++++++++++++++++++++++++++++++-
 t/t7111-reset-table.sh      |    8 +++
 5 files changed, 223 insertions(+), 20 deletions(-)

[PATCH v2 1/5] reset: add option "--keep" to "git reset"

[Christian Couder]

The purpose of this new option is to discard some of the
last commits but to keep current changes in the work tree.

The use case is when you work on something and commit
that work. And then you work on something else that touches
other files, but you don't commit it yet. Then you realize
that what you commited when you worked on the first thing
is not good or belongs to another branch.

So you want to get rid of the previous commits (at least in
the current branch) but you want to make sure that you keep
the changes you have in the work tree. And you are pretty
sure that your changes are independent from what you
previously commited, so you don't want the reset to succeed
if the previous commits changed a file that you also
changed in your work tree.

The table below shows what happens when running
"git reset --keep target" to reset the HEAD to another
commit (as a special case "target" could be the same as
HEAD).

working index HEAD target         working index HEAD
----------------------------------------------------
  A      B     C     D   --keep    (disallowed)
  A      B     C     C   --keep     A      C     C
  B      B     C     D   --keep    (disallowed)
  B      B     C     C   --keep     B      C     C

In this table, A, B and C are some different states of
a file. For example the last line of the table means
that if a file is in state B in the working tree and
the index, and in a different state C in HEAD and in
the target, then "git reset --keep target" will put
the file in state B in the working tree, and in state
C in the index and in HEAD.

The following table shows what happens on unmerged entries:

working index HEAD target         working index HEAD
----------------------------------------------------
 X       U     A    B     --keep  (disallowed)
 X       U     A    A     --keep   X       A     A

In this table X can be any state and U means an unmerged
entry.

Though the error message when "reset --keep" is disallowed
on unmerged entries is something like:

error: Entry 'file1' would be overwritten by merge. Cannot merge.
fatal: Could not reset index file to revision 'HEAD^'.

which is not very nice.

A following patch will add some test cases for "--keep".

The "--keep" 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, where
such an option was developed by Stephan Beyer:

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 |   29 ++++++++++++++++++++++++-----
 1 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/builtin-reset.c b/builtin-reset.c
index 0f5022e..da61f20 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -22,13 +22,15 @@
 #include "cache-tree.h"
 
 static const char * const git_reset_usage[] = {
-	"git reset [--mixed | --soft | --hard | --merge] [-q] [<commit>]",
+	"git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]",
 	"git reset [--mixed] <commit> [--] <paths>...",
 	NULL
 };
 
-enum reset_type { MIXED, SOFT, HARD, MERGE, NONE };
-static const char *reset_type_names[] = { "mixed", "soft", "hard", "merge", NULL };
+enum reset_type { MIXED, SOFT, HARD, MERGE, KEEP, NONE };
+static const char *reset_type_names[] = {
+	"mixed", "soft", "hard", "merge", "keep", NULL
+};
 
 static char *args_to_str(const char **argv)
 {
@@ -71,6 +73,7 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
 	if (!quiet)
 		opts.verbose_update = 1;
 	switch (reset_type) {
+	case KEEP:
 	case MERGE:
 		opts.update = 1;
 		break;
@@ -85,6 +88,16 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
 
 	read_cache_unmerged();
 
+	if (reset_type == KEEP) {
+		unsigned char head_sha1[20];
+		if (get_sha1("HEAD", head_sha1))
+			return error("You do not have a valid HEAD.");
+		if (!fill_tree_descriptor(desc, head_sha1))
+			return error("Failed to find tree of HEAD.");
+		nr++;
+		opts.fn = twoway_merge;
+	}
+
 	if (!fill_tree_descriptor(desc + nr - 1, sha1))
 		return error("Failed to find tree of %s.", sha1_to_hex(sha1));
 	if (unpack_trees(nr, desc, &opts))
@@ -229,6 +242,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				"reset HEAD, index and working tree", HARD),
 		OPT_SET_INT(0, "merge", &reset_type,
 				"reset HEAD, index and working tree", MERGE),
+		OPT_SET_INT(0, "keep", &reset_type,
+				"reset HEAD but keep local changes", KEEP),
 		OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
 		OPT_END()
 	};
@@ -317,9 +332,13 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (reset_type == SOFT) {
 		if (is_merge() || read_cache() < 0 || unmerged_cache())
 			die("Cannot do a soft reset in the middle of a merge.");
+	} else {
+		int err = reset_index_file(sha1, reset_type, quiet);
+		if (reset_type == KEEP)
+			err = err || reset_index_file(sha1, MIXED, quiet);
+		if (err)
+			die("Could not reset index file to revision '%s'.", rev);
 	}
-	else if (reset_index_file(sha1, reset_type, quiet))
-		die("Could not reset index file to revision '%s'.", rev);
 
 	/* Any resets update HEAD to the head being switched to,
 	 * saving the previous head in ORIG_HEAD before. */
-- 
1.6.6.271.gc8799

[PATCH v2 2/5] reset: add test cases for "--keep" option

[Christian Couder]

This shows that with the "--keep" 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).

In the case of unmerged entries, we can see that "git reset --keep"
works only when the target state is the same as HEAD. And then the
work tree is not reset.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t7110-reset-merge.sh |  119 +++++++++++++++++++++++++++++++++++++++++++++++-
 t/t7111-reset-table.sh |    8 +++
 2 files changed, 126 insertions(+), 1 deletions(-)

diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
index 8704d00..1a9c1c7 100755
--- a/t/t7110-reset-merge.sh
+++ b/t/t7110-reset-merge.sh
@@ -3,7 +3,7 @@
 # Copyright (c) 2009 Christian Couder
 #
 
-test_description='Tests for "git reset --merge"'
+test_description='Tests for "git reset" with "--merge" and "--keep" options'
 
 . ./test-lib.sh
 
@@ -47,6 +47,30 @@ test_expect_success 'reset --merge is ok when switching back' '
 #
 #           working index HEAD target         working index HEAD
 #           ----------------------------------------------------
+# file1:     C       C     C    D     --keep   D       D     D
+# file2:     C       D     D    D     --keep   C       D     D
+test_expect_success 'reset --keep is ok with changes in file it does not touch' '
+    git reset --hard second &&
+    cat file1 >file2 &&
+    git reset --keep HEAD^ &&
+    ! grep 4 file1 &&
+    grep 4 file2 &&
+    test "$(git rev-parse HEAD)" = "$(git rev-parse initial)" &&
+    test -z "$(git diff --cached)"
+'
+
+test_expect_success 'reset --keep is ok when switching back' '
+    git reset --keep second &&
+    grep 4 file1 &&
+    grep 4 file2 &&
+    test "$(git rev-parse HEAD)" = "$(git rev-parse second)" &&
+    test -z "$(git diff --cached)"
+'
+
+# The next test will test the following:
+#
+#           working index HEAD target         working index HEAD
+#           ----------------------------------------------------
 # file1:     B       B     C    D     --merge  D       D     D
 # file2:     C       D     D    D     --merge  C       D     D
 test_expect_success 'reset --merge discards changes added to index (1)' '
@@ -78,6 +102,18 @@ test_expect_success 'reset --merge is ok again when switching back (1)' '
 #
 #           working index HEAD target         working index HEAD
 #           ----------------------------------------------------
+# file1:     B       B     C    D     --keep   (disallowed)
+test_expect_success 'reset --keep fails with changes in index in files it touches' '
+    git reset --hard second &&
+    echo "line 5" >> file1 &&
+    git add file1 &&
+    test_must_fail git reset --keep HEAD^
+'
+
+# The next test will test the following:
+#
+#           working index HEAD target         working index HEAD
+#           ----------------------------------------------------
 # file1:     C       C     C    D     --merge  D       D     D
 # file2:     C       C     D    D     --merge  D       D     D
 test_expect_success 'reset --merge discards changes added to index (2)' '
@@ -104,6 +140,30 @@ test_expect_success 'reset --merge is ok again when switching back (2)' '
 #
 #           working index HEAD target         working index HEAD
 #           ----------------------------------------------------
+# file1:     C       C     C    D     --keep   D       D     D
+# file2:     C       C     D    D     --keep   C       D     D
+test_expect_success 'reset --keep keeps changes it does not touch' '
+    git reset --hard second &&
+    echo "line 4" >> file2 &&
+    git add file2 &&
+    git reset --keep HEAD^ &&
+    grep 4 file2 &&
+    test "$(git rev-parse HEAD)" = "$(git rev-parse initial)" &&
+    test -z "$(git diff --cached)"
+'
+
+test_expect_success 'reset --keep keeps changes when switching back' '
+    git reset --keep second &&
+    grep 4 file2 &&
+    grep 4 file1 &&
+    test "$(git rev-parse HEAD)" = "$(git rev-parse second)" &&
+    test -z "$(git diff --cached)"
+'
+
+# The next test will test the following:
+#
+#           working index HEAD target         working index HEAD
+#           ----------------------------------------------------
 # file1:     A       B     B    C     --merge  (disallowed)
 test_expect_success 'reset --merge fails with changes in file it touches' '
     git reset --hard second &&
@@ -116,6 +176,22 @@ test_expect_success 'reset --merge fails with changes in file it touches' '
     grep file1 err.log | grep "not uptodate"
 '
 
+# The next test will test the following:
+#
+#           working index HEAD target         working index HEAD
+#           ----------------------------------------------------
+# file1:     A       B     B    C     --keep   (disallowed)
+test_expect_success 'reset --keep fails with changes in file it touches' '
+    git reset --hard second &&
+    echo "line 5" >> file1 &&
+    test_tick &&
+    git commit -m "add line 5" file1 &&
+    sed -e "s/line 1/changed line 1/" <file1 >file3 &&
+    mv file3 file1 &&
+    test_must_fail git reset --keep HEAD^ 2>err.log &&
+    grep file1 err.log | grep "not uptodate"
+'
+
 test_expect_success 'setup 3 different branches' '
     git reset --hard second &&
     git branch branch1 &&
@@ -156,6 +232,18 @@ test_expect_success '"reset --merge HEAD^" is ok with pending merge' '
 #
 #           working index HEAD target         working index HEAD
 #           ----------------------------------------------------
+# file1:     X       U     B    C     --keep   (disallowed)
+test_expect_success '"reset --keep HEAD^" fails with pending merge' '
+    git reset --hard third &&
+    test_must_fail git merge branch1 &&
+    test_must_fail git reset --keep HEAD^ 2>err.log &&
+    grep file1 err.log | grep "overwritten by merge"
+'
+
+# The next test will test the following:
+#
+#           working index HEAD target         working index HEAD
+#           ----------------------------------------------------
 # file1:     X       U     B    B     --merge  B       B     B
 test_expect_success '"reset --merge HEAD" is ok with pending merge' '
     git reset --hard third &&
@@ -166,6 +254,21 @@ test_expect_success '"reset --merge HEAD" is ok with pending merge' '
     test -z "$(git diff)"
 '
 
+# The next test will test the following:
+#
+#           working index HEAD target         working index HEAD
+#           ----------------------------------------------------
+# file1:     X       U     B    B     --keep  X       B     B
+test_expect_success '"reset --keep HEAD" is ok with pending merge' '
+    git reset --hard third &&
+    test_must_fail git merge branch1 &&
+    cat file1 >orig_file1 &&
+    git reset --keep HEAD &&
+    test "$(git rev-parse HEAD)" = "$(git rev-parse third)" &&
+    test -z "$(git diff --cached)" &&
+    test_cmp file1 orig_file1
+'
+
 test_expect_success '--merge with added/deleted' '
     git reset --hard third &&
     rm -f file2 &&
@@ -180,4 +283,18 @@ test_expect_success '--merge with added/deleted' '
     git diff --exit-code --cached
 '
 
+test_expect_success '--keep with added/deleted' '
+    git reset --hard third &&
+    rm -f file2 &&
+    test_must_fail git merge branch3 &&
+    ! test -f file2 &&
+    test -f file3 &&
+    git diff --exit-code file3 &&
+    git diff --exit-code branch3 file3 &&
+    git reset --keep HEAD &&
+    test -f file3 &&
+    ! test -f file2 &&
+    git diff --exit-code --cached
+'
+
 test_done
diff --git a/t/t7111-reset-table.sh b/t/t7111-reset-table.sh
index de896c9..2ebda97 100755
--- a/t/t7111-reset-table.sh
+++ b/t/t7111-reset-table.sh
@@ -44,26 +44,32 @@ A B C D soft   A B D
 A B C D mixed  A D D
 A B C D hard   D D D
 A B C D merge  XXXXX
+A B C D keep   XXXXX
 A B C C soft   A B C
 A B C C mixed  A C C
 A B C C hard   C C C
 A B C C merge  XXXXX
+A B C C keep   A C C
 B B C D soft   B B D
 B B C D mixed  B D D
 B B C D hard   D D D
 B B C D merge  D D D
+B B C D keep   XXXXX
 B B C C soft   B B C
 B B C C mixed  B C C
 B B C C hard   C C C
 B B C C merge  C C C
+B B C C keep   B C C
 B C C D soft   B C D
 B C C D mixed  B D D
 B C C D hard   D D D
 B C C D merge  XXXXX
+B C C D keep   XXXXX
 B C C C soft   B C C
 B C C C mixed  B C C
 B C C C hard   C C C
 B C C C merge  B C C
+B C C C keep   B C C
 EOF
 
 test_expect_success 'setting up branches to test with unmerged entries' '
@@ -104,10 +110,12 @@ X U B C soft   XXXXX
 X U B C mixed  X C C
 X U B C hard   C C C
 X U B C merge  C C C
+X U B C keep   XXXXX
 X U B B soft   XXXXX
 X U B B mixed  X B B
 X U B B hard   B B B
 X U B B merge  B B B
+X U B B keep   X B B
 EOF
 
 test_done
-- 
1.6.6.271.gc8799

[PATCH v2 3/5] Documentation: reset: describe new "--keep" option

[Christian Couder]

and give an example to show how it can be used.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-reset.txt |   47 ++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 46 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 168db08..90239f5 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -8,7 +8,7 @@ git-reset - Reset current HEAD to the specified state
 SYNOPSIS
 --------
 [verse]
-'git reset' [--mixed | --soft | --hard | --merge] [-q] [<commit>]
+'git reset' [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]
 'git reset' [-q] [<commit>] [--] <paths>...
 'git reset' --patch [<commit>] [--] [<paths>...]
 
@@ -52,6 +52,11 @@ OPTIONS
 	and updates the files that are different between the named commit
 	and the current commit in the working tree.
 
+--keep::
+	Resets the index to match the tree recorded by the named commit,
+	but keep changes in the working tree. Aborts if the reset would
+	change files that are already modified in the working tree.
+
 -p::
 --patch::
 	Interactively select hunks in the difference between the index
@@ -93,6 +98,7 @@ in the index and in state D in HEAD.
 				--mixed  A       D     D
 				--hard   D       D     D
 				--merge (disallowed)
+				--keep  (disallowed)
 
       working index HEAD target         working index HEAD
       ----------------------------------------------------
@@ -100,6 +106,7 @@ in the index and in state D in HEAD.
 				--mixed  A       C     C
 				--hard   C       C     C
 				--merge (disallowed)
+				--keep   A       C     C
 
       working index HEAD target         working index HEAD
       ----------------------------------------------------
@@ -107,6 +114,7 @@ in the index and in state D in HEAD.
 				--mixed  B       D     D
 				--hard   D       D     D
 				--merge  D       D     D
+				--keep  (disallowed)
 
       working index HEAD target         working index HEAD
       ----------------------------------------------------
@@ -114,6 +122,7 @@ in the index and in state D in HEAD.
 				--mixed  B       C     C
 				--hard   C       C     C
 				--merge  C       C     C
+				--keep   B       C     C
 
       working index HEAD target         working index HEAD
       ----------------------------------------------------
@@ -121,6 +130,7 @@ in the index and in state D in HEAD.
 				--mixed  B       D     D
 				--hard   D       D     D
 				--merge (disallowed)
+				--keep  (disallowed)
 
       working index HEAD target         working index HEAD
       ----------------------------------------------------
@@ -128,6 +138,7 @@ in the index and in state D in HEAD.
 				--mixed  B       C     C
 				--hard   C       C     C
 				--merge  B       C     C
+				--keep   B       C     C
 
 "reset --merge" is meant to be used when resetting out of a conflicted
 merge. Any mergy operation guarantees that the work tree file that is
@@ -138,6 +149,14 @@ between the index and the work tree, then it means that we are not
 resetting out from a state that a mergy operation left after failing
 with a conflict. That is why we disallow --merge option in this case.
 
+"reset --keep" is meant to be used when remove some of the last
+commits in the current branch while keep changes in the working tree.
+If there could be conflicts between the changes in the commit we want
+to remove and the changes in the working tree we want to keep, the
+reset is disallowed. That's why it is disallowed if there are both
+changes between the working tree and HEAD, and between HEAD and the
+target.
+
 The following tables show what happens when there are unmerged
 entries:
 
@@ -147,6 +166,7 @@ entries:
 				--mixed  X       B     B
 				--hard   B       B     B
 				--merge  B       B     B
+				--keep  (disallowed)
 
       working index HEAD target         working index HEAD
       ----------------------------------------------------
@@ -154,6 +174,7 @@ entries:
 				--mixed  X       A     A
 				--hard   A       A     A
 				--merge  A       A     A
+				--keep   X       A     A
 
 X means any state and U means an unmerged index.
 
@@ -325,6 +346,30 @@ $ git add frotz.c                           <3>
 <2> This commits all other changes in the index.
 <3> Adds the file to the index again.
 
+Keep changes in working tree while discarding some previous commits::
++
+Suppose you are working on something and you commit it, and then you
+continue working a bit more, but now you think that what you have in
+your working tree should be in another branch that has nothing to do
+with what you commited previously. You can start a new branch and
+reset it while keeping the changes in your work tree.
++
+------------
+$ git tag start
+$ git branch branch1
+$ edit
+$ git commit ...                            <1>
+$ edit
+$ git branch branch2                        <2>
+$ git reset --keep start                    <3>
+------------
++
+<1> This commits your first edits in branch1.
+<2> This creates branch2, but unfortunately it contains the previous
+commit that you don't want in this branch.
+<3> This removes the unwanted previous commit, but this keeps the
+changes in your working tree.
+
 Author
 ------
 Written by Junio C Hamano <gitster@pobox.com> and Linus Torvalds <torvalds@osdl.org>
-- 
1.6.6.271.gc8799

[PATCH v2 4/5] reset: disallow "reset --keep" outside a work tree

[Christian Couder]

It is safer and consistent with "--merge" and "--hard" resets to disallow
"git reset --keep" outside a work tree.

So let's just do that and add some tests while at it.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin-reset.c       |    2 +-
 t/t7103-reset-bare.sh |   25 +++++++++++++++++--------
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/builtin-reset.c b/builtin-reset.c
index da61f20..52584af 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -319,7 +319,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (reset_type == NONE)
 		reset_type = MIXED; /* by default */
 
-	if (reset_type == HARD || reset_type == MERGE)
+	if (reset_type != SOFT && reset_type != MIXED)
 		setup_work_tree();
 
 	if (reset_type == MIXED && is_bare_repository())
diff --git a/t/t7103-reset-bare.sh b/t/t7103-reset-bare.sh
index afb55b3..1eef93c 100755
--- a/t/t7103-reset-bare.sh
+++ b/t/t7103-reset-bare.sh
@@ -11,21 +11,26 @@ test_expect_success 'setup non-bare' '
 	git commit -a -m two
 '
 
-test_expect_success 'hard reset requires a worktree' '
+test_expect_success '"hard" reset requires a worktree' '
 	(cd .git &&
 	 test_must_fail git reset --hard)
 '
 
-test_expect_success 'merge reset requires a worktree' '
+test_expect_success '"merge" reset requires a worktree' '
 	(cd .git &&
 	 test_must_fail git reset --merge)
 '
 
-test_expect_success 'mixed reset is ok' '
+test_expect_success '"keep" reset requires a worktree' '
+	(cd .git &&
+	 test_must_fail git reset --keep)
+'
+
+test_expect_success '"mixed" reset is ok' '
 	(cd .git && git reset)
 '
 
-test_expect_success 'soft reset is ok' '
+test_expect_success '"soft" reset is ok' '
 	(cd .git && git reset --soft)
 '
 
@@ -40,19 +45,23 @@ test_expect_success 'setup bare' '
 	cd bare.git
 '
 
-test_expect_success 'hard reset is not allowed in bare' '
+test_expect_success '"hard" reset is not allowed in bare' '
 	test_must_fail git reset --hard HEAD^
 '
 
-test_expect_success 'merge reset is not allowed in bare' '
+test_expect_success '"merge" reset is not allowed in bare' '
 	test_must_fail git reset --merge HEAD^
 '
 
-test_expect_success 'mixed reset is not allowed in bare' '
+test_expect_success '"keep" reset is not allowed in bare' '
+	test_must_fail git reset --keep HEAD^
+'
+
+test_expect_success '"mixed" reset is not allowed in bare' '
 	test_must_fail git reset --mixed HEAD^
 '
 
-test_expect_success 'soft reset is allowed in bare' '
+test_expect_success '"soft" reset is allowed in bare' '
 	git reset --soft HEAD^ &&
 	test "`git show --pretty=format:%s | head -n 1`" = "one"
 '
-- 
1.6.6.271.gc8799

[PATCH v2 5/5] reset: disallow using --keep when there are unmerged entries

[Christian Couder]

The use case for --keep option is to remove previous commits unrelated
to the current changes in the working tree. So in this use case we are
not supposed to have unmerged entries. This is why it seems safer to
just disallow using --keep when there are unmerged entries.

And this patch changes the error message when --keep was disallowed and
there were some unmerged entries from:

    error: Entry 'file1' would be overwritten by merge. Cannot merge.
    fatal: Could not reset index file to revision 'HEAD^'.

to:

    fatal: Cannot do a keep reset in the middle of a merge.

which is nicer.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-reset.txt |    5 +++--
 builtin-reset.c             |   17 +++++++++++++----
 t/t7110-reset-merge.sh      |   23 +++++++++--------------
 t/t7111-reset-table.sh      |    2 +-
 4 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 90239f5..d2f6880 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -155,7 +155,8 @@ If there could be conflicts between the changes in the commit we want
 to remove and the changes in the working tree we want to keep, the
 reset is disallowed. That's why it is disallowed if there are both
 changes between the working tree and HEAD, and between HEAD and the
-target.
+target. To be safe, it is also disallowed when there are unmerged
+entries.
 
 The following tables show what happens when there are unmerged
 entries:
@@ -174,7 +175,7 @@ entries:
 				--mixed  X       A     A
 				--hard   A       A     A
 				--merge  A       A     A
-				--keep   X       A     A
+				--keep  (disallowed)
 
 X means any state and U means an unmerged index.
 
diff --git a/builtin-reset.c b/builtin-reset.c
index 52584af..441ac1b 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -224,6 +224,14 @@ static void prepend_reflog_action(const char *action, char *buf, size_t size)
 		warning("Reflog action message too long: %.*s...", 50, buf);
 }
 
+static void die_if_unmerged_cache(int reset_type)
+{
+	if (is_merge() || read_cache() < 0 || unmerged_cache())
+		die("Cannot do a %s reset in the middle of a merge.",
+		    reset_type_names[reset_type]);
+
+}
+
 int cmd_reset(int argc, const char **argv, const char *prefix)
 {
 	int i = 0, reset_type = NONE, update_ref_status = 0, quiet = 0;
@@ -329,10 +337,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	/* Soft reset does not touch the index file nor the working tree
 	 * at all, but requires them in a good order.  Other resets reset
 	 * the index file to the tree object we are switching to. */
-	if (reset_type == SOFT) {
-		if (is_merge() || read_cache() < 0 || unmerged_cache())
-			die("Cannot do a soft reset in the middle of a merge.");
-	} else {
+	if (reset_type == SOFT)
+		die_if_unmerged_cache(reset_type);
+	else {
+		if (reset_type == KEEP)
+			die_if_unmerged_cache(reset_type);
 		int err = reset_index_file(sha1, reset_type, quiet);
 		if (reset_type == KEEP)
 			err = err || reset_index_file(sha1, MIXED, quiet);
diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
index 1a9c1c7..70cdd8e 100755
--- a/t/t7110-reset-merge.sh
+++ b/t/t7110-reset-merge.sh
@@ -237,7 +237,7 @@ test_expect_success '"reset --keep HEAD^" fails with pending merge' '
     git reset --hard third &&
     test_must_fail git merge branch1 &&
     test_must_fail git reset --keep HEAD^ 2>err.log &&
-    grep file1 err.log | grep "overwritten by merge"
+    grep "middle of a merge" err.log
 '
 
 # The next test will test the following:
@@ -258,18 +258,15 @@ test_expect_success '"reset --merge HEAD" is ok with pending merge' '
 #
 #           working index HEAD target         working index HEAD
 #           ----------------------------------------------------
-# file1:     X       U     B    B     --keep  X       B     B
-test_expect_success '"reset --keep HEAD" is ok with pending merge' '
+# file1:     X       U     B    B     --keep   (disallowed)
+test_expect_success '"reset --keep HEAD" fails with pending merge' '
     git reset --hard third &&
     test_must_fail git merge branch1 &&
-    cat file1 >orig_file1 &&
-    git reset --keep HEAD &&
-    test "$(git rev-parse HEAD)" = "$(git rev-parse third)" &&
-    test -z "$(git diff --cached)" &&
-    test_cmp file1 orig_file1
+    test_must_fail git reset --keep HEAD 2>err.log &&
+    grep "middle of a merge" err.log
 '
 
-test_expect_success '--merge with added/deleted' '
+test_expect_success '--merge is ok with added/deleted merge' '
     git reset --hard third &&
     rm -f file2 &&
     test_must_fail git merge branch3 &&
@@ -283,7 +280,7 @@ test_expect_success '--merge with added/deleted' '
     git diff --exit-code --cached
 '
 
-test_expect_success '--keep with added/deleted' '
+test_expect_success '--keep fails with added/deleted merge' '
     git reset --hard third &&
     rm -f file2 &&
     test_must_fail git merge branch3 &&
@@ -291,10 +288,8 @@ test_expect_success '--keep with added/deleted' '
     test -f file3 &&
     git diff --exit-code file3 &&
     git diff --exit-code branch3 file3 &&
-    git reset --keep HEAD &&
-    test -f file3 &&
-    ! test -f file2 &&
-    git diff --exit-code --cached
+    test_must_fail git reset --keep HEAD 2>err.log &&
+    grep "middle of a merge" err.log
 '
 
 test_done
diff --git a/t/t7111-reset-table.sh b/t/t7111-reset-table.sh
index 2ebda97..ce421ad 100755
--- a/t/t7111-reset-table.sh
+++ b/t/t7111-reset-table.sh
@@ -115,7 +115,7 @@ X U B B soft   XXXXX
 X U B B mixed  X B B
 X U B B hard   B B B
 X U B B merge  B B B
-X U B B keep   X B B
+X U B B keep   XXXXX
 EOF
 
 test_done
-- 
1.6.6.271.gc8799

Re: [PATCH v2 1/5] reset: add option "--keep" to "git reset"

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> The purpose of this new option is to discard some of the
> last commits but to keep current changes in the work tree.
>
> The use case is when you work on something and commit
> that work. And then you work on something else that touches
> other files, but you don't commit it yet. Then you realize
> that what you commited when you worked on the first thing
> is not good or belongs to another branch.
>
> So you want to get rid of the previous commits (at least in
> the current branch) but you want to make sure that you keep
> the changes you have in the work tree.

You did this:

	git checkout master
	work; git commit ; work; git commit ; work; git commit
	edit ; git add ; ... (but not commit)

and noticed the three commits should not be on master but on a new branch.

I think we currently teach users to do something like this:

	git stash
        git branch topic
        git reset --hard HEAD~3
        git stash pop

Instead you want to do this:

	git branch topic
        git reset --keep HEAD~3

Surely you halved the number of command involved, but is this really an
improvement?  At least I can visualize (and more importantly, explain to
new users) how the "stash, flip and unstash" works, why it is safe, and
how to recover when "pop" stops in conflicts, but I have no confidence in
explaining what "reset --keep" does and how to recover when it refuses to
work to new users.

Another way to accomplish the same thing might be:

	git branch -m topic
        git checkout -b master HEAD~3

and with the same number of commands, conceptually it may be easier to
understand than "reset --keep".  What you committed so far belongs to
another branch "topic", so you name the current history that way, and then
you switch branches with "checkout" that keeps your local modifications.
It also opens the possibility of retrying with "-m" after checkout refuses
to acti, to take the same mix-up risk CVS/SVN users have, if you are very
confident that your local change conflicts only minimally with the change
made on the topic and you can resolve them.

Of course, when you are not interested in keeping the topmost commits at
all, you either

	git stash ; git reset --hard HEAD~3 ; git stash pop

or

	git reset --keep HEAD~3

but even in this case, I think "stash, flip and unstash" is easier to
explain, especially when teaching what to do if things go wrong.

I dunno.  Is this really an useful addition that helps real-world
workflows and is worth a five patch series, or is this just "because we
can" patch?

Re: [PATCH v2 1/5] reset: add option "--keep" to "git reset"

[Christian Couder]

On mardi 19 janvier 2010, Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
> > The purpose of this new option is to discard some of the
> > last commits but to keep current changes in the work tree.
> >
> > The use case is when you work on something and commit
> > that work. And then you work on something else that touches
> > other files, but you don't commit it yet. Then you realize
> > that what you commited when you worked on the first thing
> > is not good or belongs to another branch.
> >
> > So you want to get rid of the previous commits (at least in
> > the current branch) but you want to make sure that you keep
> > the changes you have in the work tree.
>
> You did this:
>
> 	git checkout master
> 	work; git commit ; work; git commit ; work; git commit
> 	edit ; git add ; ... (but not commit)
>
> and noticed the three commits should not be on master but on a new
> branch.
>
> I think we currently teach users to do something like this:
>
> 	git stash
>         git branch topic
>         git reset --hard HEAD~3
>         git stash pop
>
> Instead you want to do this:
>
> 	git branch topic
>         git reset --keep HEAD~3
>
> Surely you halved the number of command involved, but is this really an
> improvement?  At least I can visualize (and more importantly, explain to
> new users) how the "stash, flip and unstash" works, why it is safe, and
> how to recover when "pop" stops in conflicts, but I have no confidence in
> explaining what "reset --keep" does and how to recover when it refuses to
> work to new users.

Of course new users should be told about "git stash" before being told 
about "git reset --keep" and "git reset --merge". These 2 options are 
mostly for advanced users who want shortcuts and are ready to learn a few 
more options to speed up some of their common tasks.

Commit 9e8eceab ("Add 'merge' mode to 'git reset'", 2008-12-01), ended with:

-------------------
    Yes, yes, with a dirty tree you could always do

        git stash
        git reset --hard
        git stash apply

    instead, but isn't "git reset --merge" a nice way to handle one 
particular simple case?
-------------------

and I claim that the same is true for "--keep". It is just a nice way to 
handle one particular simple case.

> Another way to accomplish the same thing might be:
>
> 	git branch -m topic
>         git checkout -b master HEAD~3
>
> and with the same number of commands, conceptually it may be easier to
> understand than "reset --keep".  What you committed so far belongs to
> another branch "topic", so you name the current history that way, and
> then you switch branches with "checkout" that keeps your local
> modifications. It also opens the possibility of retrying with "-m" after
> checkout refuses to acti, to take the same mix-up risk CVS/SVN users
> have, if you are very confident that your local change conflicts only
> minimally with the change made on the topic and you can resolve them.

Sorry but I don't find that easier to understand. On the contrary I find 
awkward to have to rename the current branch first.

My first reaction when I realize that my current work belongs to another 
branch is just to create another branch with a good name, and then I try to 
find a way to make the new branch clean. It would be strange and perhaps a 
little bit unsafe, at least for me, to have to rename the current branch 
and then recreate it with some good content.

> Of course, when you are not interested in keeping the topmost commits at
> all, you either
>
> 	git stash ; git reset --hard HEAD~3 ; git stash pop
>
> or
>
> 	git reset --keep HEAD~3
>
> but even in this case, I think "stash, flip and unstash" is easier to
> explain, especially when teaching what to do if things go wrong.
>
> I dunno.  Is this really an useful addition that helps real-world
> workflows and is worth a five patch series, or is this just "because we
> can" patch?

I know that I will use the new option. At least I will use it much more 
than --merge that I never used at $dayjob, probably because I don't merge a 
lot of stuff.

Best regards,
Christian.