[PATCH v5 0/7] "git reset --merge" related improvements

[PATCH v5 0/7] "git reset --merge" related improvements

[Christian Couder]

Another reroll with the following changes:

- the name of the new option is now "--keep" instead of "--keep-local-changes",
- the fill_tree_descriptor() function is used instead of adding a new
parse_and_init_tree_desc() function (thanks to Stephen Boyd),
- patch 1/7 was added to only accept soft reset when not in a working tree;
this makes the test suite pass,
- the commit message of patch 4/7 that adds the --keep option has been
improved; it talks more about the use case of this new option.

Christian Couder (6):
  reset: do not accept a mixed reset in a .git dir
  reset: add a few tests for "git reset --merge"
  reset: add option "--keep" to "git reset"
  reset: add test cases for "--keep" option
  Documentation: reset: describe new "--keep" option
  Documentation: reset: add some tables to describe the different
    options

Stephan Beyer (1):
  reset: use "unpack_trees()" directly instead of "git read-tree"

 Documentation/git-reset.txt |   79 +++++++++++++++++++++-
 builtin-reset.c             |   74 +++++++++++++++-----
 t/t7103-reset-bare.sh       |    4 +-
 t/t7110-reset-merge.sh      |  156 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 292 insertions(+), 21 deletions(-)
 create mode 100755 t/t7110-reset-merge.sh

[PATCH v5 1/7] reset: do not accept a mixed reset in a .git dir

[Christian Couder]

It is strange and fragile that a mixed reset is disallowed in a bare
repo but is allowed in a .git dir. So this patch simplifies things
by only allowing soft resets when not in a working tree.

This patch is also needed to speed up "git reset" by using
unpack_tree() directly (instead of execing "git read-tree"). A
following patch will do just that.

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

diff --git a/builtin-reset.c b/builtin-reset.c
index 11d1c6e..14bdb03 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -286,8 +286,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)
-	    && !is_inside_work_tree())
+	if (reset_type != SOFT && !is_inside_work_tree())
 		die("%s reset requires a work tree",
 		    reset_type_names[reset_type]);
 
diff --git a/t/t7103-reset-bare.sh b/t/t7103-reset-bare.sh
index 68041df..13344b2 100755
--- a/t/t7103-reset-bare.sh
+++ b/t/t7103-reset-bare.sh
@@ -21,8 +21,8 @@ test_expect_success 'merge reset requires a worktree' '
 	 test_must_fail git reset --merge)
 '
 
-test_expect_success 'mixed reset is ok' '
-	(cd .git && git reset)
+test_expect_success 'mixed reset requires a worktree' '
+	(cd .git && test_must_fail git reset)
 '
 
 test_expect_success 'soft reset is ok' '
-- 
1.6.6.rc1.8.gd33ec

[PATCH v5 2/7] reset: add a few tests for "git reset --merge"

[Christian Couder]

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 |   94 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 94 insertions(+), 0 deletions(-)
 create mode 100755 t/t7110-reset-merge.sh

diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
new file mode 100755
index 0000000..8190da1
--- /dev/null
+++ b/t/t7110-reset-merge.sh
@@ -0,0 +1,94 @@
+#!/bin/sh
+#
+# Copyright (c) 2009 Christian Couder
+#
+
+test_description='Tests for "git reset --merge"'
+
+. ./test-lib.sh
+
+test_expect_success 'creating initial files' '
+     echo "line 1" >> file1 &&
+     echo "line 2" >> file1 &&
+     echo "line 3" >> file1 &&
+     cp file1 file2 &&
+     git add file1 file2 &&
+     test_tick &&
+     git commit -m "Initial commit"
+'
+
+test_expect_success 'reset --merge is ok with changes in file it does not touch' '
+     echo "line 4" >> file1 &&
+     echo "line 4" >> file2 &&
+     test_tick &&
+     git commit -m "add line 4" file1 &&
+     git reset --merge HEAD^ &&
+     ! grep 4 file1 &&
+     grep 4 file2 &&
+     git reset --merge HEAD@{1} &&
+     grep 4 file1 &&
+     grep 4 file2
+'
+
+test_expect_success 'reset --merge discards changes added to index (1)' '
+     echo "line 5" >> file1 &&
+     git add file1 &&
+     git reset --merge HEAD^ &&
+     ! grep 4 file1 &&
+     ! grep 5 file1 &&
+     grep 4 file2 &&
+     echo "line 5" >> file2 &&
+     git add file2 &&
+     git reset --merge HEAD@{1} &&
+     ! grep 4 file2 &&
+     ! grep 5 file1 &&
+     grep 4 file1
+'
+
+test_expect_success 'reset --merge discards changes added to index (2)' '
+     echo "line 4" >> file2 &&
+     git add file2 &&
+     git reset --merge HEAD^ &&
+     ! grep 4 file2 &&
+     git reset --merge HEAD@{1} &&
+     ! grep 4 file2 &&
+     grep 4 file1
+'
+
+test_expect_success 'reset --merge fails with changes in file it touches' '
+     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 --merge HEAD^ 2>err.log &&
+     grep file1 err.log | grep "not uptodate" &&
+     git reset --hard HEAD^
+'
+
+test_expect_success 'setup 2 different branches' '
+     git branch branch1 &&
+     git branch branch2 &&
+     git checkout branch1 &&
+     echo "line 5 in branch1" >> file1 &&
+     test_tick &&
+     git commit -a -m "change in branch1" &&
+     git checkout branch2 &&
+     echo "line 5 in branch2" >> file1 &&
+     test_tick &&
+     git commit -a -m "change in branch2"
+'
+
+test_expect_success '"reset --merge HEAD^" fails with pending merge' '
+     test_must_fail git merge branch1 &&
+     test_must_fail git reset --merge HEAD^ &&
+     git reset --hard HEAD
+'
+
+test_expect_success '"reset --merge HEAD" fails with pending merge' '
+     test_must_fail git merge branch1 &&
+     test_must_fail git reset --merge HEAD &&
+     git reset --hard HEAD
+'
+
+test_done
-- 
1.6.6.rc1.8.gd33ec

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

[Christian Couder]

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.

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.

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        |   41 ++++++++++++++++++++++++++++++-----------
 t/t7110-reset-merge.sh |    8 +++++---
 2 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/builtin-reset.c b/builtin-reset.c
index 14bdb03..ac38aaa 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -18,6 +18,8 @@
 #include "tree.h"
 #include "branch.h"
 #include "parse-options.h"
+#include "unpack-trees.h"
+#include "cache-tree.h"
 
 static const char * const git_reset_usage[] = {
 	"git reset [--mixed | --soft | --hard | --merge] [-q] [<commit>]",
@@ -54,27 +56,44 @@ static inline int is_merge(void)
 
 static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet)
 {
-	int i = 0;
-	const char *args[6];
+	int nr = 1;
+	int newfd;
+	struct tree_desc desc[2];
+	struct unpack_trees_options opts;
+	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
 
-	args[i++] = "read-tree";
+	memset(&opts, 0, sizeof(opts));
+	opts.head_idx = 1;
+	opts.src_index = &the_index;
+	opts.dst_index = &the_index;
+	opts.fn = oneway_merge;
+	opts.merge = 1;
 	if (!quiet)
-		args[i++] = "-v";
+		opts.verbose_update = 1;
 	switch (reset_type) {
 	case MERGE:
-		args[i++] = "-u";
-		args[i++] = "-m";
+		opts.update = 1;
 		break;
 	case HARD:
-		args[i++] = "-u";
+		opts.update = 1;
 		/* fallthrough */
 	default:
-		args[i++] = "--reset";
+		opts.reset = 1;
 	}
-	args[i++] = sha1_to_hex(sha1);
-	args[i] = NULL;
 
-	return run_command_v_opt(args, RUN_GIT_CMD);
+	newfd = hold_locked_index(lock, 1);
+
+	read_cache_unmerged();
+
+	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))
+		return -1;
+	if (write_cache(newfd, active_cache, active_nr) ||
+	    commit_locked_index(lock))
+		return error("Could not write new index file.");
+
+	return 0;
 }
 
 static void print_new_head_line(struct commit *commit)
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

[PATCH v5 4/7] 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 "--keep" option will do what you want.

The table below shows what happens when running
"git reset --option target" to reset the HEAD to another
commit (as a special case "target" could be the same as
HEAD) in the cases where "--merge" and "--keep" behave
differently.

working index HEAD target         working index HEAD
----------------------------------------------------
  A      B     C     D   --keep    (disallowed)
                         --merge   (disallowed)
  A      B     C     C   --keep     A      C     C
                         --merge   (disallowed)
  B      B     C     D   --keep    (disallowed)
                         --merge    C      C     C
  B      B     C     C   --keep     B      C     C
                         --merge    C      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 --merge target" will put
the file in state C in the working tree, in the index
and in HEAD.

So as can be seen in the table, "--merge" can discard
changes in the working tree, while "--keep" does not.
So if one wants to avoid using "git stash" before and
after using "git reset" to save current changes, it is
better to use "--keep" rather than "--merge".

The following table shows what happens on unmerged entries:

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

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

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

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

diff --git a/builtin-reset.c b/builtin-reset.c
index ac38aaa..859e0d1 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))
@@ -228,6 +241,9 @@ 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('q', NULL, &quiet,
 				"disable showing new HEAD in hard reset and progress message"),
 		OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
@@ -315,9 +331,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.rc1.8.gd33ec

[PATCH v5 5/7] 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). As with the "--merge" option,
changes that are in both the work tree and the index are discarded
after the reset.

In the case of unmerged entries, we can see that
"git reset --merge HEAD" is disallowed while it works using
"--keep" instead.

And this shows that otherwise "--merge" and "--keep" have the same
behavior.

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

diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
index 6afaf73..bd2ef94 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'
 
 . ./test-lib.sh
 
@@ -30,6 +30,20 @@ test_expect_success 'reset --merge is ok with changes in file it does not touch'
      grep 4 file2
 '
 
+test_expect_success 'reset --keep is ok with changes in file it does not touch' '
+     git reset --hard HEAD^ &&
+     echo "line 4" >> file1 &&
+     echo "line 4" >> file2 &&
+     test_tick &&
+     git commit -m "add line 4" file1 &&
+     git reset --keep HEAD^ &&
+     ! grep 4 file1 &&
+     grep 4 file2 &&
+     git reset --keep HEAD@{1} &&
+     grep 4 file1 &&
+     grep 4 file2
+'
+
 test_expect_success 'reset --merge discards changes added to index (1)' '
      echo "line 5" >> file1 &&
      git add file1 &&
@@ -55,6 +69,25 @@ test_expect_success 'reset --merge discards changes added to index (2)' '
      grep 4 file1
 '
 
+test_expect_success 'reset --keep fails with changes in index in files it touches' '
+     echo "line 4" >> file2 &&
+     echo "line 5" >> file1 &&
+     git add file1 &&
+     test_must_fail git reset --keep HEAD^ &&
+     git reset --hard HEAD
+'
+
+test_expect_success 'reset --keep keeps changes it does not touch' '
+     echo "line 4" >> file2 &&
+     git add file2 &&
+     git reset --keep HEAD^ &&
+     grep 4 file2 &&
+     git reset --keep HEAD@{1} &&
+     grep 4 file2 &&
+     grep 4 file1 &&
+     git reset --hard HEAD
+'
+
 test_expect_success 'reset --merge fails with changes in file it touches' '
      echo "line 5" >> file1 &&
      test_tick &&
@@ -66,6 +99,17 @@ test_expect_success 'reset --merge fails with changes in file it touches' '
      git reset --hard HEAD^
 '
 
+test_expect_success 'reset --keep fails with changes in file it touches' '
+     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" &&
+     git reset --hard HEAD^
+'
+
 test_expect_success 'setup 2 different branches' '
      git branch branch1 &&
      git branch branch2 &&
@@ -87,10 +131,26 @@ test_expect_success '"reset --merge HEAD^" is ok with pending merge' '
      git reset --hard HEAD@{1}
 '
 
+test_expect_success '"reset --keep HEAD^" is ok with pending merge' '
+     test_must_fail git merge branch1 &&
+     git reset --keep 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' '
      test_must_fail git merge branch1 &&
      test_must_fail git reset --merge HEAD &&
      git reset --hard HEAD
 '
 
+test_expect_success '"reset --keep HEAD" is ok with pending merge' '
+     test_must_fail git merge branch1 &&
+     git reset --keep HEAD &&
+     test -z "$(git diff --cached)" &&
+     test -n "$(git diff)" &&
+     git reset --hard HEAD
+'
+
 test_done
-- 
1.6.6.rc1.8.gd33ec

[PATCH v5 6/7] Documentation: reset: describe new "--keep" option

[Christian Couder]

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

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 2d27e40..f8724d0 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::
+	This does the same thing as --merge, but it keeps changes in
+	the working tree and the index or it aborts if it is not
+	possible.
+
 -p::
 --patch::
 	Interactively select hunks in the difference between the index
-- 
1.6.6.rc1.8.gd33ec

[PATCH v5 7/7] Documentation: reset: add some tables to describe the different options

[Christian Couder]

This patch adds a DISCUSSION section that contains some tables to
show how the different "git reset" options work depending on the
states of the files in the working tree, the index, HEAD and the
target commit.

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

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index f8724d0..4ded9e3 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -72,6 +72,78 @@ linkgit:git-add[1]).
 <commit>::
 	Commit to make the current HEAD. If not given defaults to HEAD.
 
+DISCUSSION
+----------
+
+The tables below show what happens when running:
+
+----------
+git reset --option target
+----------
+
+to reset the HEAD to another commit (`target`) with the different
+reset options depending on the state of the files.
+
+      working index HEAD target         working index HEAD
+      ----------------------------------------------------
+       A       B     C    D     --soft   A       B     D
+                                --mixed  A       D     D
+                                --hard   D       D     D
+                                --merge (disallowed)
+                                --keep  (disallowed)
+
+      working index HEAD target         working index HEAD
+      ----------------------------------------------------
+       A       B     C    C     --soft   A       B     C
+                                --mixed  A       C     C
+                                --hard   C       C     C
+                                --merge (disallowed)
+                                --keep   A       C     C
+
+      working index HEAD target         working index HEAD
+      ----------------------------------------------------
+       B       B     C    D     --soft   B       B     D
+                                --mixed  B       D     D
+                                --hard   D       D     D
+                                --merge  D       D     D
+                                --keep  (disallowed)
+
+      working index HEAD target         working index HEAD
+      ----------------------------------------------------
+       B       B     C    C     --soft   B       B     C
+                                --mixed  B       C     C
+                                --hard   C       C     C
+                                --merge  C       C     C
+                                --keep   B       C     C
+
+In these tables, A, B, C and D are some different states of a
+file. For example, the last line of the last 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 HEAD.
+
+The following tables show what happens when there are unmerged
+entries:
+
+      working index HEAD target         working index HEAD
+      ----------------------------------------------------
+       X       U     A    B     --soft  (disallowed)
+                                --mixed  X       B     B
+                                --hard   B       B     B
+                                --merge  X       B     B
+                                --keep   X       B     B
+
+      working index HEAD target         working index HEAD
+      ----------------------------------------------------
+       X       U     A    A     --soft  (disallowed)
+                                --mixed  X       A     A
+                                --hard   A       A     A
+                                --merge (disallowed)
+                                --keep   X       A     A
+
+X means any state and U means an unmerged index.
+
 Examples
 --------
 
-- 
1.6.6.rc1.8.gd33ec

Re: [PATCH v5 1/7] reset: do not accept a mixed reset in a .git dir

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> It is strange and fragile that a mixed reset is disallowed in a bare
> repo but is allowed in a .git dir. So this patch simplifies things
> by only allowing soft resets when not in a working tree.

I would not mind what you said were "I find the difference strange", as it
is a fairly subjective word.  But if you say "fragile", you would need to
defend the use of the word better.  What kind of misuse does it invite,
and what grave consequences such misuses would cause?  I do not see any
fragility and I would want to understand it better so that I can write
about it in the Release Note to warn people and encourage them to upgrade.

More importantly, I think the difference between the two actually makes
sense.  A bare repository by definition does _not_ have any work tree so
there is no point in having the index file in there.  On the other hand,
even though it is not necessary to "cd .git && git reset HEAD^", I don't
see a strong reason why it needs to be disallowed.

On the other hand, I don't see a strong reason why such a use needs to be
supported, other than "we've allowed it for a long time, and people may
have hooks (they typically start in $GIT_DIR) that rely on it", which by
itself is not something strong enough to veto the change.  It is only a
minor incompatibility and I can be persuaded to listen to a pros-and-cons
argument.

An honest justification might have been "This change to disallow a mixed
reset in $GIT_DIR of a repository with a work tree will break existing
scripts, but I think it is not widely used _for such and such reasons_,
and can easily be worked around.  On the other hand, this change vastly
simplifies the reimplementation of 'reset' _because X and Y and Z_".

> This patch is also needed to speed up "git reset" by using
> unpack_tree() directly (instead of execing "git read-tree").

It is very unclear _why_ it is "needed" from this description.

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

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> 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

Re: [PATCH v5 4/7] 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. 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 "--keep" option will do what you want.

Having two paragraphs that describe the use case near the top is a big
improvement from older rounds, but this one line is more irritating than
useful, as it tries to be convincing only by being repetitive.  The only
new information it adds relative to the first two lines is its name.  The
true convincing argument immediately follows in the form of the table
below anyway, so I'd drop this line if I were you.

> The table below shows what happens when running "git reset --option
> target" to reset the HEAD to another commit (as a special case "target"
> could be the same as HEAD) in the cases where "--merge" and "--keep"
> behave differently.
>
> working index HEAD target         working index HEAD
> ----------------------------------------------------
>   A      B     C     D   --keep    (disallowed)
>                          --merge   (disallowed)

Ok. the user has partially updated the index and has further changes; we
don't want to lose either of them.  During a failed merge, this cannot
happen, so it is a good safety measure on "--merge" side as well.

>   A      B     C     C   --keep     A      C     C
>                          --merge   (disallowed)

The user started working based on C, and has partially updated the index.
This cannot have come from a failed automerge because A != B, so using
"reset --merge" is likely to be a mistake and we should disallow it as a
safety measure.

>   B      B     C     D   --keep    (disallowed)
>                          --merge    C      C     C

For --keep this is the same as the first case (except that the "partially
updated the index" happened to be "100% pertially") and it makes sense to
disallow it.

However, I think --merge _should_ have D (target) in all of the three in
the result in this case, as I mentioned in my response to [PATCH 3/7].  Is
that "the bug" you talked about there?

>   B      B     C     C   --keep     B      C     C
>                          --merge    C      C     C

A special case of the second one for --keep.  For --merge, B can only have
come from the previous merge operation we are resetting away, and matching
all three result to the "C", which is the target, is the right thing to do.

> So as can be seen in the table, "--merge" can discard changes in the
> working tree, while "--keep" does not.  So if one wants to avoid using
> "git stash" before and after using "git reset" to save current changes,
> it is better to use "--keep" rather than "--merge".

This is a very flawed and misleading description.  These two options serve
two entirely different purposes and use cases, but your "if one wants to
avoid using ... to save current changes" is too broad and does not
distinguish between the two. "--keep" option is valid (or "better") for
only one of them, while it is absolutely the wrong option to use for the
other one.  So it may be "better" only half the time and it is wrong in
the other half.

The precondition of using a --merge is the scenario Linus described in
9e8ecea (Add 'merge' mode to 'git reset', 2008-12-01).  You started some
mergy operation and ended up in a state where all the differences, either
merged or unmerged, between the HEAD and the index came from the mergy
operation.  Also the paths in the work tree that correspond to the index
entries with differences from HEAD are modified by the mergy operation
(either updated with a cleanly resolved merge, or with conflict markers)
and do not match HEAD when "reset --merge" is run, but it is guaranteed
that you didn't have any local modification to them before the mergy
operation.  You do want to reset these changes to the work tree away,
while keeping the changes you had before the mergy operation.

"--merge" not only _can_ (as in "has the risk to") discard changes, but in
the "failed merge" scenario, it is absolutely the right thing to do to
discard the changes in the work tree for the paths that have differences
between HEAD and index and to update them to that of the target.  Not
updating like "--keep" does is not "better" but is simply _wrong_.

The use case "--keep" wants to support is very different (and you have a
good description near the beginning of the commit log message for people
to judge if the intended use case makes sense).  For it, discarding the
changes to the work tree is a wrong thing to do.

> The following table shows what happens on unmerged entries:
>
> working index HEAD target         working index HEAD
> ----------------------------------------------------
>  X       U     A    B     --keep   X       B     B
>                           --merge  X       B     B
>  X       U     A    A     --keep   X       A     A
>                           --merge (disallowed)
>
> In this table X can be any state and U means an unmerged entry.

I am wondering if we should disallow "--keep" if we see unmerged entries
in the index as a minimum safety measure.  Failing "reset --keep" when an
unmerged entry exists in the index will save people who are trying to
discard a failed merge (i.e. who should have used --merge) but was somehow
fooled into thinking that "--keep" is a better "--merge".

It also is tempting to say that we should forbid "reset --merge" without
an unmerged entry in the index, but that wouldn't work.  A mergy operation
would have left unmerged entries in the index initially before giving the
control back to the user, but the user may have used "edit && git add" to
resolve them, and then decided that it is not worth committing.  By the
time "reset --merge" is run, there may not be any unmerged path left in
the index.

I am suggesting extra safety measures primarily because I am worried that
people will get confused by these two similar looking options that are
meant for entirely different use cases.  An obvious alternative solution
to avoid the confusion is not to add "--keep" in the first place.  While I
think that is rather a cop-out than a solution, it might make more sense.
It is hopeless to educate users to pick the right one, if even the author
of the new option mistakenly thinks that "--keep is merely a better
version of --merge".

My preference is at this point to first have patches [1/7] to [3/7] to
update "reset --merge" (I am not sure about the "--mixed in $GIT_DIR"
change, though), with a new follow-up patch [3.5/7] to fix "reset --merge"
to reset paths that were cloberred by the merge to the target (not HEAD),
and start cooking these changes in 'pu' and then 'next'.

That will lay groundwork of using unpack_trees() in "reset" and after they
stabilize, build new modes like "--keep" on top of it.

Re: [PATCH v5 2/7] reset: add a few tests for "git reset --merge"

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> +test_description='Tests for "git reset --merge"'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'creating initial files' '
> +     echo "line 1" >> file1 &&
> +     echo "line 2" >> file1 &&
> +     echo "line 3" >> file1 &&
> +     cp file1 file2 &&
> +     git add file1 file2 &&
> +     test_tick &&
> +     git commit -m "Initial commit"
> +'

5-char indent is somewhat unusual; please fix it.  Also you may want to
tag the initial one and other key commits for later use.

> +test_expect_success 'reset --merge is ok with changes in file it does not touch' '
> +     echo "line 4" >> file1 &&
> +     echo "line 4" >> file2 &&
> +     test_tick &&
> +     git commit -m "add line 4" file1 &&

IOW, move the above four lines to the end of the first "setup", and tag as
necessary like this:

    test_expect_success setup '
        for i in 1 2 3; do echo line $i; done >file1 &&
        cat file1 >file2 &&
        git add file1 file2 &&
        test_tick &&
        git commit -m "Initial commit" &&
        git tag initial &&
        echo line 4 >>file1 &&
        cat file1 >file2 &&
        test_tick &&
        git commit -m "add line 4 to file1" file1 &&
        git tag second
    '

And the rest should become its own test, starting from a known state.

> +     git reset --merge HEAD^ &&
> +     ! grep 4 file1 &&
> +     grep 4 file2 &&

Switching from the second commit to the initial one should succeed because
the only local change is in file2 that is unrelated.  This should also check
if HEAD points at the "initial" commit after "reset", and the index
matches the HEAD commit.

> +     git reset --merge HEAD@{1} &&
> +     grep 4 file1 &&
> +     grep 4 file2
> +'

And switching back to the second commit should recover.  This should be a
separate test, and should make sure it begins in a known state even if the
"reset --merge HEAD^" in the previous test failed.  This should also
check:

	test $(git rev-parse HEAD) = $(git rev-parse second) &&
	test -z "$(git diff --cached)"

If any of these two "reset --merge" is broken, the next test will start at
an unknown commit, hence you should add

	git reset --hard second &&

at the beginning of the next test.  You may also need to copy file1 to
file2 to recreate the local change as well.

The idea is not to assume that tests will succeed and 'recovery' step at
the end will run; instead, make sure each test starts itself in a known
state.  The way you wrote "reset --hard" at the end of some tests for
recovery is fragile.  There is no guarantee that each test run to the end,
executing these recovery parts.  Instead, have a corresponding set-up at
the beginning of each test as necessary.

> +test_expect_success 'reset --merge discards changes added to index (1)' '
> +     echo "line 5" >> file1 &&
> +     git add file1 &&

So at this point, file2 has an extra "line 4", and file1 has a change to
be committed.  And we reset to the initial commit.

> +     git reset --merge HEAD^ &&
> +     ! grep 4 file1 &&
> +     ! grep 5 file1 &&
> +     grep 4 file2 &&

And that will make file1 match that of initial, while keeping the change
to file2 intact.  The same comment about missing checks applies.  The
remainder of this test should be a separate test.

> +     echo "line 5" >> file2 &&
> +     git add file2 &&
> +     git reset --merge HEAD@{1} &&
> +     ! grep 4 file2 &&
> +     ! grep 5 file1 &&
> +     grep 4 file1
> +'

Starting at 'initial' but with two lines added to file2 and the index
updated to it, we reset to 'second' (spell these out with tags, instead of
relying on reflog, so that you do not assume all the previous tests have
passed).  Both files should match those of 'second'.  The same comment
about missing checks applies.

> +test_expect_success 'reset --merge discards changes added to index (2)' '
> +     echo "line 4" >> file2 &&
> +     git add file2 &&
> +     git reset --merge HEAD^ &&
> +     ! grep 4 file2 &&
> +     git reset --merge HEAD@{1} &&
> +     ! grep 4 file2 &&
> +     grep 4 file1
> +'

The same comment about the missing 'make sure it begins in a known state'
applies, but I don't see the point of this (2).  Is there any fundamental
difference of the set-up this one tests, compared to the earlier test?

> +test_expect_success 'reset --merge fails with changes in file it touches' '
> +     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 --merge HEAD^ 2>err.log &&
> +     grep file1 err.log | grep "not uptodate" &&

Hmm, this is something we had a patch for to give different messages from
plumbing and Porcelain.

> +     git reset --hard HEAD^

Lose this 'recover at the end'.  There is no guarantee the each test run
to the end.   Instead, have a corresponding set-up at the beginning of the
next one.

I'd re-review the rest after this is rerolled.

Re: [PATCH v5 1/7] reset: do not accept a mixed reset in a .git dir

[Christian Couder]

On samedi 12 décembre 2009, Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
> > It is strange and fragile that a mixed reset is disallowed in a bare
> > repo but is allowed in a .git dir. So this patch simplifies things
> > by only allowing soft resets when not in a working tree.
>
> I would not mind what you said were "I find the difference strange", as
> it is a fairly subjective word.  But if you say "fragile", you would need
> to defend the use of the word better.  What kind of misuse does it
> invite, and what grave consequences such misuses would cause?  I do not
> see any fragility and I would want to understand it better so that I can
> write about it in the Release Note to warn people and encourage them to
> upgrade.
>
> More importantly, I think the difference between the two actually makes
> sense.  A bare repository by definition does _not_ have any work tree so
> there is no point in having the index file in there.  On the other hand,
> even though it is not necessary to "cd .git && git reset HEAD^", I don't
> see a strong reason why it needs to be disallowed.

Ok. I have the following patch now:

---------- 8< ---------------
commit c20f969db6e565f2fe854b95202c3ef95ad0ff42
Author: Christian Couder <chriscool@tuxfamily.org>
Date:   Thu Dec 10 22:10:07 2009 +0100

    reset: improve mixed reset error message when in a bare repo

    When running a "git reset --mixed" in a bare repository, the
    message displayed is:

    fatal: This operation must be run in a work tree
    fatal: Could not reset index file to revision 'HEAD^'.

    This message is a little bit misleading as a mixed reset is ok in
    a git directory, so it is not absolutely needed to run it in a
    work tree.

    So this patch improves upon the above by changing the message to:

    fatal: mixed reset is not allowed in a bare repository

    This patch is also needed to speed up "git reset" by using
    unpack_tree() directly (instead of execing "git read-tree"). A
    following patch will do just that.

    While at it, instead of disallowing "git reset --option" outside
    a work tree only when option is "hard" or "merge", we now disallow
    it except when option is "soft" or "mixed", as it is safer if we
    ever add options to "git reset".

diff --git a/builtin-reset.c b/builtin-reset.c
index 11d1c6e..ac3505b 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -286,11 +286,15 @@ int cmd_reset(int argc, const char **argv, const char 
*pre
        if (reset_type == NONE)
                reset_type = MIXED; /* by default */

-       if ((reset_type == HARD || reset_type == MERGE)
-           && !is_inside_work_tree())
+       if (reset_type != SOFT && reset_type != MIXED
+            && !is_inside_work_tree())
                die("%s reset requires a work tree",
                    reset_type_names[reset_type]);

+       if (reset_type == MIXED && is_bare_repository())
+               die("%s reset is not allowed in a bare repository",
+                   reset_type_names[reset_type]);
+
        /* 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. */
---------- 8< ---------------

> On the other hand, I don't see a strong reason why such a use needs to be
> supported, other than "we've allowed it for a long time, and people may
> have hooks (they typically start in $GIT_DIR) that rely on it", which by
> itself is not something strong enough to veto the change.  It is only a
> minor incompatibility and I can be persuaded to listen to a pros-and-cons
> argument.
>
> An honest justification might have been "This change to disallow a mixed
> reset in $GIT_DIR of a repository with a work tree will break existing
> scripts, but I think it is not widely used _for such and such reasons_,
> and can easily be worked around.  On the other hand, this change vastly
> simplifies the reimplementation of 'reset' _because X and Y and Z_".

My opinion is that it works this way just by accident not by design (that's 
why I said "fragile"). But if we don't want to take any risk of regression, 
then let's use the new patch above.

> > This patch is also needed to speed up "git reset" by using
> > unpack_tree() directly (instead of execing "git read-tree").
>
> It is very unclear _why_ it is "needed" from this description.

The reason is that after the next patch, it will not fail in a bare 
repository, so if we don't change anything, the test that checks that it 
fails in a bare repo will fail. I will add this explanation to the new 
patch.

Best regards,
Christian.

Re: [PATCH v5 1/7] reset: do not accept a mixed reset in a .git dir

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

>     This patch is also needed to speed up "git reset" by using
>     unpack_tree() directly (instead of execing "git read-tree"). A
>     following patch will do just that.

This still doesn't seem to explain anything that the part you added after
your patch.

>     While at it, instead of disallowing "git reset --option" outside
>     a work tree only when option is "hard" or "merge", we now disallow
>     it except when option is "soft" or "mixed", as it is safer if we
>     ever add options to "git reset".

I fail to see any sane logic behind this reasoning; you cannot decide if
you need to allow or disallow the new --option with unspecified semantics
until you have that --option, and you are saying 

Hmm, "reset --option" that does not work when it should work is a bug,
just like "reset --option" that does not refuse to work when it should
refuse is, and you cannot decide if you should allow a new --option until
you have it.  Your "disallowing everything the code does not know about by
default" doesn't particularly sound safer to me.  I'd suggest dropping it
from this patch.

It is perfectly fine to have a change like that, if it makes the logic
easier to follow with the updated repertoire when a new --option is added,
but not before.

>> An honest justification might have been "This change to disallow a mixed
>> reset in $GIT_DIR of a repository with a work tree will break existing
>> scripts, but I think it is not widely used _for such and such reasons_,
>> and can easily be worked around.  On the other hand, this change vastly
>> simplifies the reimplementation of 'reset' _because X and Y and Z_".
>
> My opinion is that it works this way just by accident not by design (that's 
> why I said "fragile").

And do you still think it is accident after I explained the difference
between the two for you (or perhaps you didn't read it)?

>> > This patch is also needed to speed up "git reset" by using
>> > unpack_tree() directly (instead of execing "git read-tree").
>>
>> It is very unclear _why_ it is "needed" from this description.
>
> The reason is that after the next patch, it will not fail in a bare 
> repository,...

That sounds as if you want to change the definition of what the expected
behaviour is early, because you want to claim a regression you will later
introduce is not a regression.  I hope that is not the case.

Re: [PATCH v5 1/7] reset: do not accept a mixed reset in a .git dir

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> diff --git a/builtin-reset.c b/builtin-reset.c
> index 11d1c6e..ac3505b 100644
> --- a/builtin-reset.c
> +++ b/builtin-reset.c
> @@ -286,11 +286,15 @@ int cmd_reset(int argc, const char **argv, const char 
> *pre
>         if (reset_type == NONE)
>                 reset_type = MIXED; /* by default */
>
> -       if ((reset_type == HARD || reset_type == MERGE)
> -           && !is_inside_work_tree())
> +       if (reset_type != SOFT && reset_type != MIXED
> +            && !is_inside_work_tree())
>                 die("%s reset requires a work tree",
>                     reset_type_names[reset_type]);
>
> +       if (reset_type == MIXED && is_bare_repository())
> +               die("%s reset is not allowed in a bare repository",
> +                   reset_type_names[reset_type]);

This patch text itself makes sense, I think, except the first part.

Re: [PATCH v5 1/7] reset: do not accept a mixed reset in a .git dir

[Junio C Hamano]

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

>> The reason is that after the next patch, it will not fail in a bare 
>> repository,...
>
> That sounds as if you want to change the definition of what the expected
> behaviour is early, because you want to claim a regression you will later
> introduce is not a regression.  I hope that is not the case.

Hmm...

By "after the next patch, it will not fail in a bare repository",
did you mean "if the next patch blindly replaced an external call to
read-tree with an internal call to unpack_trees(), it will change the
behaviour, and we will end up allowing '--mixed in bare'.  To prevent it
from happening, cmd_reset() should check that condition upfront"?

Then you were not trying to hide regressions (which makes me happier).
But then doesn't the change belong to the next patch, not this one?

Re: [PATCH v5 1/7] reset: do not accept a mixed reset in a .git dir

[Christian Couder]

On mardi 15 décembre 2009, Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
> >     While at it, instead of disallowing "git reset --option" outside
> >     a work tree only when option is "hard" or "merge", we now disallow
> >     it except when option is "soft" or "mixed", as it is safer if we
> >     ever add options to "git reset".
>
> I fail to see any sane logic behind this reasoning; you cannot decide if
> you need to allow or disallow the new --option with unspecified semantics
> until you have that --option, and you are saying
>
> Hmm, "reset --option" that does not work when it should work is a bug,
> just like "reset --option" that does not refuse to work when it should
> refuse is, and you cannot decide if you should allow a new --option until
> you have it.  Your "disallowing everything the code does not know about
> by default" doesn't particularly sound safer to me.  I'd suggest dropping
> it from this patch.

Ok, I will drop it.

> It is perfectly fine to have a change like that, if it makes the logic
> easier to follow with the updated repertoire when a new --option is
> added, but not before.

Ok.

[...]

> By "after the next patch, it will not fail in a bare repository",
> did you mean "if the next patch blindly replaced an external call to
> read-tree with an internal call to unpack_trees(), it will change the
> behaviour, and we will end up allowing '--mixed in bare'.  To prevent it
> from happening, cmd_reset() should check that condition upfront"?

Yes, that's what I meant.

> Then you were not trying to hide regressions (which makes me happier).
> But then doesn't the change belong to the next patch, not this one?

I can put it in the patch that calls unpack_trees() directly, but on the 
other hand it can also be seen as an improvement that could be applied 
to "maint" as it improves the error message.

Best regards,
Christian.

Re: [PATCH v5 4/7] reset: add option "--keep" to "git reset"

[Christian Couder]

On samedi 12 décembre 2009, Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
>
> >   B      B     C     D   --keep    (disallowed)
> >                          --merge    C      C     C
>
> For --keep this is the same as the first case (except that the "partially
> updated the index" happened to be "100% pertially") and it makes sense to
> disallow it.
>
> However, I think --merge _should_ have D (target) in all of the three in
> the result in this case, as I mentioned in my response to [PATCH 3/7]. 
> Is that "the bug" you talked about there?

No it is not the bug I talked about. It was a bug in the documentation 
patch. Thanks for finding it! I left some "C" on the merge line from an 
earlier documentation patch but I should have changed them to "D" like 
that:

   B      B     C     D   --keep    (disallowed)
                          --merge    D     D     D

[...]

> It also is tempting to say that we should forbid "reset --merge" without
> an unmerged entry in the index, but that wouldn't work.  A mergy
> operation would have left unmerged entries in the index initially before
> giving the control back to the user, but the user may have used "edit &&
> git add" to resolve them, and then decided that it is not worth
> committing.  By the time "reset --merge" is run, there may not be any
> unmerged path left in the index.
>
> I am suggesting extra safety measures primarily because I am worried that
> people will get confused by these two similar looking options that are
> meant for entirely different use cases.  An obvious alternative solution
> to avoid the confusion is not to add "--keep" in the first place.  While
> I think that is rather a cop-out than a solution, it might make more
> sense. It is hopeless to educate users to pick the right one, if even the
> author of the new option mistakenly thinks that "--keep is merely a
> better version of --merge".

I just think it is better in some cases not always.

> My preference is at this point to first have patches [1/7] to [3/7] to
> update "reset --merge" (I am not sure about the "--mixed in $GIT_DIR"
> change, though), with a new follow-up patch [3.5/7] to fix "reset
> --merge" to reset paths that were cloberred by the merge to the target
> (not HEAD), and start cooking these changes in 'pu' and then 'next'.

Ok, I will send a patch series to do that except that I I don't understand 
exactly what the follow up patch [3.5/7] shoud do. Perhaps you asked for 
this additional patch because of the documentation bug above?

> That will lay groundwork of using unpack_trees() in "reset" and after
> they stabilize, build new modes like "--keep" on top of it.

Ok.

Thanks,
Christian.