[PATCH v6 0/4] "git reset --merge" related improvements

[PATCH v6 0/4] "git reset --merge" related improvements

[Christian Couder]

Another reroll with the following changes:
- patches to add --keep option have been removed for now
- documentation patch has been moved before the core code changes
- commit messages have been improved
- tests have been much improved thanks to Junio's suggestions

Christian Couder (3):
  reset: improve mixed reset error message when in a bare repo
  Documentation: reset: add some tables to describe the different
    options
  reset: add a few tests for "git reset --merge"

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

 Documentation/git-reset.txt |   66 ++++++++++++++++++
 builtin-reset.c             |   45 +++++++++---
 t/t7110-reset-merge.sh      |  161 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 261 insertions(+), 11 deletions(-)
 create mode 100755 t/t7110-reset-merge.sh

[PATCH v6 1/4] reset: improve mixed reset error message when in a bare repo

[Christian Couder]

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

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 because 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

And if "git reset" is ever sped up by using unpack_tree() directly
(instead of execing "git read-tree"), this patch will also make
sure that a mixed reset is still disallowed in a bare repository.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin-reset.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/builtin-reset.c b/builtin-reset.c
index 11d1c6e..3180c2b 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -291,6 +291,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		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. */
-- 
1.6.6.rc2.5.g49666

[PATCH v6 2/4] 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 |   66 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 2d27e40..c9044c9 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -67,6 +67,72 @@ 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)
+
+      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)
+
+      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
+
+      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
+
+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
+--merge target" will put the file in state C in the working tree,
+in the index and in 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 (disallowed)
+
+      working index HEAD target         working index HEAD
+      ----------------------------------------------------
+       X       U     A    A     --soft  (disallowed)
+                                --mixed  X       A     A
+                                --hard   A       A     A
+                                --merge (disallowed)
+
+X means any state and U means an unmerged index.
+
 Examples
 --------
 
-- 
1.6.6.rc2.5.g49666

[PATCH v6 3/4] 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 |  159 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 159 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..4c46083
--- /dev/null
+++ b/t/t7110-reset-merge.sh
@@ -0,0 +1,159 @@
+#!/bin/sh
+#
+# Copyright (c) 2009 Christian Couder
+#
+
+test_description='Tests for "git reset --merge"'
+
+. ./test-lib.sh
+
+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
+'
+
+# 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       D     D    D     --merge  C       D     D
+test_expect_success 'reset --merge is ok with changes in file it does not touch' '
+    git reset --merge 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 --merge is ok when switching back' '
+    git reset --merge 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)' '
+    git reset --hard second &&
+    cat file1 >file2 &&
+    echo "line 5" >> file1 &&
+    git add file1 &&
+    git reset --merge HEAD^ &&
+    ! grep 4 file1 &&
+    ! grep 5 file1 &&
+    grep 4 file2 &&
+    test "$(git rev-parse HEAD)" = "$(git rev-parse initial)" &&
+    test -z "$(git diff --cached)"
+'
+
+test_expect_success 'reset --merge is ok again when switching back (1)' '
+    git reset --hard initial &&
+    echo "line 5" >> file2 &&
+    git add file2 &&
+    git reset --merge second &&
+    ! grep 4 file2 &&
+    ! grep 5 file1 &&
+    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:     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)' '
+    git reset --hard second &&
+    echo "line 4" >> file2 &&
+    git add file2 &&
+    git reset --merge HEAD^ &&
+    ! grep 4 file2 &&
+    test "$(git rev-parse HEAD)" = "$(git rev-parse initial)" &&
+    test -z "$(git diff)" &&
+    test -z "$(git diff --cached)"
+'
+
+test_expect_success 'reset --merge is ok again when switching back (2)' '
+    git reset --hard initial &&
+    git reset --merge 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 &&
+    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"
+'
+
+test_expect_success 'setup 2 different branches' '
+    git reset --hard second &&
+    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" &&
+    git tag third
+'
+
+# The next test will test the following:
+#
+#           working index HEAD target         working index HEAD
+#           ----------------------------------------------------
+# file1:     X       U     B    C     --merge  (disallowed)
+test_expect_success '"reset --merge HEAD^" fails with pending merge' '
+    test_must_fail git merge branch1 &&
+    test_must_fail git reset --merge HEAD^ &&
+    test "$(git rev-parse HEAD)" = "$(git rev-parse third)" &&
+    test -n "$(git diff --cached)"
+'
+
+# The next test will test the following:
+#
+#           working index HEAD target         working index HEAD
+#           ----------------------------------------------------
+# file1:     X       U     B    B     --merge  (disallowed)
+test_expect_success '"reset --merge HEAD" fails with pending merge' '
+    git reset --hard third &&
+    test_must_fail git merge branch1 &&
+    test_must_fail git reset --merge HEAD &&
+    test "$(git rev-parse HEAD)" = "$(git rev-parse third)" &&
+    test -n "$(git diff --cached)"
+'
+
+test_done
-- 
1.6.6.rc2.5.g49666

[PATCH v6 4/4] 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 when we reset the entries to some states that differ from HEAD.

But in 9e8ecea (Add 'merge' mode to 'git reset', 2008-12-01) there is
the following:

"
     - 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. But that's not how read-tree has ever worked, so..
"

So the new behavior looks better according to the original
implementation of "git reset --merge".

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>
---
 Documentation/git-reset.txt |    2 +-
 builtin-reset.c             |   41 ++++++++++++++++++++++++++++++-----------
 t/t7110-reset-merge.sh      |   12 +++++++-----
 3 files changed, 38 insertions(+), 17 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index c9044c9..b40999f 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -122,7 +122,7 @@ entries:
        X       U     A    B     --soft  (disallowed)
                                 --mixed  X       B     B
                                 --hard   B       B     B
-                                --merge (disallowed)
+                                --merge  X       B     B
 
       working index HEAD target         working index HEAD
       ----------------------------------------------------
diff --git a/builtin-reset.c b/builtin-reset.c
index 3180c2b..2c880a7 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 4c46083..ff2875c 100755
--- a/t/t7110-reset-merge.sh
+++ b/t/t7110-reset-merge.sh
@@ -135,12 +135,14 @@ test_expect_success 'setup 2 different branches' '
 #
 #           working index HEAD target         working index HEAD
 #           ----------------------------------------------------
-# file1:     X       U     B    C     --merge  (disallowed)
-test_expect_success '"reset --merge HEAD^" fails with pending merge' '
+# file1:     X       U     B    C     --merge  X       C     C
+test_expect_success '"reset --merge HEAD^" is ok with pending merge' '
     test_must_fail git merge branch1 &&
-    test_must_fail git reset --merge HEAD^ &&
-    test "$(git rev-parse HEAD)" = "$(git rev-parse third)" &&
-    test -n "$(git diff --cached)"
+    cat file1 >orig_file1 &&
+    git reset --merge HEAD^ &&
+    test "$(git rev-parse HEAD)" = "$(git rev-parse second)" &&
+    test -z "$(git diff --cached)" &&
+    test_cmp file1 orig_file1
 '
 
 # The next test will test the following:
-- 
1.6.6.rc2.5.g49666

Re: [PATCH v6 0/4] "git reset --merge" related improvements

[Linus Torvalds]

On Wed, 30 Dec 2009, Christian Couder wrote:
>
> Another reroll with the following changes:
> - patches to add --keep option have been removed for now
> - documentation patch has been moved before the core code changes
> - commit messages have been improved
> - tests have been much improved thanks to Junio's suggestions
> 
> Christian Couder (3):
>   reset: improve mixed reset error message when in a bare repo
>   Documentation: reset: add some tables to describe the different
>     options
>   reset: add a few tests for "git reset --merge"
> 
> Stephan Beyer (1):
>   reset: use "unpack_trees()" directly instead of "git read-tree"

FWIW, Ack on this version of the whole series - I don't think there is 
anything controversial here, and I think avoiding the execve of read-tree 
for something we have a library function for is a good thing (and the 
change in behavior looks like a bug-fix to me).

And the whole "--keep" discussion can be resurrected later ;)

		Linus

Re: [PATCH v6 4/4] reset: use "unpack_trees()" directly instead of "git read-tree"

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> But in 9e8ecea (Add 'merge' mode to 'git reset', 2008-12-01) there is
> the following:
>
> "
>      - 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. But that's not how read-tree has ever worked, so..
> "
>
> So the new behavior looks better according to the original
> implementation of "git reset --merge".

This is not really an improvement..  You are swapping an breakage with a
different breakage of a riskier kind.

At least disallowing means that the user _is notified_ and has to manually
deal with the situation.  Pretending it succeeded by resetting only the
index while still leaving the conflicted state in the work tree intact is
a bit worse in that sense.

> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> index c9044c9..b40999f 100644
> --- a/Documentation/git-reset.txt
> +++ b/Documentation/git-reset.txt
> @@ -122,7 +122,7 @@ entries:
>         X       U     A    B     --soft  (disallowed)
>                                  --mixed  X       B     B
>                                  --hard   B       B     B
> -                                --merge (disallowed)
> +                                --merge  X       B     B

IOW, I think the result should be "B B B" instead of "X B B" in this
case.

Re: [PATCH v6 2/4] Documentation: reset: add some tables to describe the different options

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

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

Much nicer.

>  Documentation/git-reset.txt |   66 +++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 66 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> index 2d27e40..c9044c9 100644
> --- a/Documentation/git-reset.txt
> +++ b/Documentation/git-reset.txt
> @@ -67,6 +67,72 @@ 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.

Together with these "mechanical definitions", I think the readers would
benefit from reading why some are disallowed.

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

"reset --merge" is meant to be used when resetting out of a conflicted
merge.  Because any mergy operation guarantees that the work tree file
that is involved in the merge does not have local change wrt the index
before it starts, and that it writes the result out to the work tree, the
fact that we see difference between the index and the HEAD and also
between the index and the work tree means that we are not seeing a state
that a mergy operation left after failing with a conflict.  That is why we
disallow --merge option in this case, and the next one.

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

The same as above, but you are resetting to the same commit.

> +      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

> +      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

As this table is not only about "--merge" but to explain "reset", I think
other interesting cases should also be covered.

w=A	i=B	H=B	t=B

This is "we had local change in the work tree that was unrelated to the
merge", and "reset --merge" should be a no-op for this path.

w=A	i=B	H=B	t=C

This "reset --merge" is like "using checkout to switch to a commit that
has C but we have changes in the work tree", and should fail just like
"checkout branch" in such a situation fails without "-m" option.

Re: [PATCH v6 3/4] reset: add a few tests for "git reset --merge"

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

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

Looks good.

> +# 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       D     D    D     --merge  C       D     D
> +test_expect_success 'reset --merge is ok with changes in file it does not touch' '
> +    git reset --merge HEAD^ &&
> +    ! grep 4 file1 &&
> +    grep 4 file2 &&
> +    test "$(git rev-parse HEAD)" = "$(git rev-parse initial)" &&
> +    test -z "$(git diff --cached)"
> +'
> ...
> +# 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)' '
> +    git reset --hard second &&
> +    echo "line 4" >> file2 &&
> +    git add file2 &&
> +    git reset --merge HEAD^ &&
> +    ! grep 4 file2 &&
> +    test "$(git rev-parse HEAD)" = "$(git rev-parse initial)" &&
> +    test -z "$(git diff)" &&
> +    test -z "$(git diff --cached)"
> +'

These two seem to duplicate the same case for file1; is it necessary?

I am not pointing it out as something that needs to be removed; I am just
puzzled and wondering if there is some interaction between the ways two
paths are handled and the test is trying to check that (which I do not
think is the case).

Re: [PATCH v6 4/4] reset: use "unpack_trees()" directly instead of "git read-tree"

[Junio C Hamano]

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

> At least disallowing means that the user _is notified_ and has to manually
> deal with the situation.  Pretending it succeeded by resetting only the
> index while still leaving the conflicted state in the work tree intact is
> a bit worse in that sense.
>
>> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
>> index c9044c9..b40999f 100644
>> --- a/Documentation/git-reset.txt
>> +++ b/Documentation/git-reset.txt
>> @@ -122,7 +122,7 @@ entries:
>>         X       U     A    B     --soft  (disallowed)
>>                                  --mixed  X       B     B
>>                                  --hard   B       B     B
>> -                                --merge (disallowed)
>> +                                --merge  X       B     B
>
> IOW, I think the result should be "B B B" instead of "X B B" in this
> case.

A squashable fix-up on top of your patch to match the wish in the part you
quoted from 9e8ecea (Add 'merge' mode to 'git reset', 2008-12-01) would
look like this, I think.

It does three things:

 - Updates the documentation to match the wish of original "reset --merge"
   better, namely, "An unmerged entry is a sign that the path didn't have
   any local modification and can be safely resetted to whatever the new
   HEAD records";

 - Updates read_index_unmerged(), which reads the index file into the
   cache while dropping any higher-stage entries down to stage #0, not to
   copy the object name recorded in the cache entry.  The code used to
   take the object name from the highest stage entry ("theirs" if you
   happened to have stage #3, or "ours" if they removed while you kept),
   which essentially meant that you are getting random results and didn't
   make sense.

   The _only_ reason we want to keep a previously unmerged entry in the
   index at stage #0 is so that we don't forget the fact that we have
   corresponding file in the work tree in order to be able to remove it
   when the tree we are resetting to does not have the path.  In order to
   differentiate such an entry from ordinary cache entry, the cache entry
   added by read_index_unmerged() records null sha1.

 - Updates merged_entry() and deleted_entry() so that they pay attention to
   cache entries with null sha1 (note that we _might_ want to use a new
   in-core ce->ce_flags instead of using the null-sha1 hack).  They are
   previously unmerged entries, and the files in the work tree that
   correspond to them are resetted away by oneway_merge() to the version
   from the tree we are resetting to.

Please take this with a grain of salt as I am under slight influence of
CH3-CH2-OH while writing it, and I usually almost never drink.

---

 Documentation/git-reset.txt |    4 ++--
 read-cache.c                |    2 +-
 t/t7110-reset-merge.sh      |   14 +++++++-------
 unpack-trees.c              |   19 ++++++++++++-------
 4 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index cf2433d..dc73dca 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -122,14 +122,14 @@ entries:
        X       U     A    B     --soft  (disallowed)
 				--mixed  X       B     B
 				--hard   B       B     B
-				--merge  X       B     B
+				--merge  B       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)
+				--merge  A       A     A
 
 X means any state and U means an unmerged index.
 
diff --git a/read-cache.c b/read-cache.c
index 1bbaf1c..616dd03 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1606,7 +1606,7 @@ int read_index_unmerged(struct index_state *istate)
 		len = strlen(ce->name);
 		size = cache_entry_size(len);
 		new_ce = xcalloc(1, size);
-		hashcpy(new_ce->sha1, ce->sha1);
+		/* don't copy sha1; this should not match anything */
 		memcpy(new_ce->name, ce->name, len);
 		new_ce->ce_flags = create_ce_flags(len, 0);
 		new_ce->ce_mode = ce->ce_mode;
diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
index ff2875c..249df7c 100755
--- a/t/t7110-reset-merge.sh
+++ b/t/t7110-reset-merge.sh
@@ -135,27 +135,27 @@ test_expect_success 'setup 2 different branches' '
 #
 #           working index HEAD target         working index HEAD
 #           ----------------------------------------------------
-# file1:     X       U     B    C     --merge  X       C     C
+# file1:     X       U     B    C     --merge  C       C     C
 test_expect_success '"reset --merge HEAD^" is ok with pending merge' '
     test_must_fail git merge branch1 &&
-    cat file1 >orig_file1 &&
     git reset --merge HEAD^ &&
     test "$(git rev-parse HEAD)" = "$(git rev-parse second)" &&
     test -z "$(git diff --cached)" &&
-    test_cmp file1 orig_file1
+    test -z "$(git diff)"
 '
 
 # The next test will test the following:
 #
 #           working index HEAD target         working index HEAD
 #           ----------------------------------------------------
-# file1:     X       U     B    B     --merge  (disallowed)
-test_expect_success '"reset --merge HEAD" fails with pending merge' '
+# file1:     X       U     B    B     --merge  B       B     B
+test_expect_success '"reset --merge HEAD" is ok with pending merge' '
     git reset --hard third &&
     test_must_fail git merge branch1 &&
-    test_must_fail git reset --merge HEAD &&
+    git reset --merge HEAD &&
     test "$(git rev-parse HEAD)" = "$(git rev-parse third)" &&
-    test -n "$(git diff --cached)"
+    test -z "$(git diff --cached)" &&
+    test -z "$(git diff)"
 '
 
 test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index dd5999c..4a4d18c 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -666,7 +666,11 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 {
 	int update = CE_UPDATE;
 
-	if (old) {
+	if (!old) {
+		if (verify_absent(merge, "overwritten", o))
+			return -1;
+		invalidate_ce_path(merge, o);
+	} else if (!is_null_sha1(old->sha1)) {
 		/*
 		 * See if we can re-use the old CE directly?
 		 * That way we get the uptodate stat info.
@@ -682,11 +686,12 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 				return -1;
 			invalidate_ce_path(old, o);
 		}
-	}
-	else {
-		if (verify_absent(merge, "overwritten", o))
-			return -1;
-		invalidate_ce_path(merge, o);
+	} else {
+		/*
+		 * Previously unmerged entry left as an existence
+		 * marker by read_index_unmerged();
+		 */
+		invalidate_ce_path(old, o);
 	}
 
 	add_entry(o, merge, update, CE_STAGEMASK);
@@ -702,7 +707,7 @@ static int deleted_entry(struct cache_entry *ce, struct cache_entry *old,
 			return -1;
 		return 0;
 	}
-	if (verify_uptodate(old, o))
+	if (!is_null_sha1(old->sha1) && verify_uptodate(old, o))
 		return -1;
 	add_entry(o, ce, CE_REMOVE, 0);
 	invalidate_ce_path(ce, o);

Re: [PATCH v6 0/4] "git reset --merge" related improvements

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Wed, 30 Dec 2009, Christian Couder wrote:
>>
>> Another reroll with the following changes:
>> - patches to add --keep option have been removed for now
>> - documentation patch has been moved before the core code changes
>> - commit messages have been improved
>> - tests have been much improved thanks to Junio's suggestions
>> 
>> Christian Couder (3):
>>   reset: improve mixed reset error message when in a bare repo
>>   Documentation: reset: add some tables to describe the different
>>     options
>>   reset: add a few tests for "git reset --merge"
>> 
>> Stephan Beyer (1):
>>   reset: use "unpack_trees()" directly instead of "git read-tree"
>
> FWIW, Ack on this version of the whole series - I don't think there is 
> anything controversial here, and I think avoiding the execve of read-tree 
> for something we have a library function for is a good thing (and the 
> change in behavior looks like a bug-fix to me).

The "bug-fix" needs my follow-up patch [*1*] to be a real "bug-fix", I
think.

By the way, I've always felt it somewhat confusing that "reset --merge"
implementation uses one-tree form of "read-tree -m -u", especially given
that your 9e8ecea (Add 'merge' mode to 'git reset', 2008-12-01) that
describes the "bug" contrasts its behaviour with the behaviour of going to
a different branch with "checkout", that is inherently an operation that
depends on the two-tree "read-tree -m -u" semantics.

In the context of "reset", however, we don't want the check "checkout"
does between the tree that the index came from (the first tree argument,
typically HEAD) and the index before going to the target tree (the second
tree argument), so it is the right thing to do to use the one-tree form
semantics "go from the state in the index to the target tree".


[Refererence]

*1* http://article.gmane.org/gmane.comp.version-control.git/136004

Re: [PATCH v6 4/4] reset: use "unpack_trees()" directly instead of "git read-tree"

[Christian Couder]

Hi and happy new year!

On vendredi 01 janvier 2010, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> > At least disallowing means that the user _is notified_ and has to
> > manually deal with the situation.  Pretending it succeeded by resetting
> > only the index while still leaving the conflicted state in the work
> > tree intact is a bit worse in that sense.
> >
> >> diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> >> index c9044c9..b40999f 100644
> >> --- a/Documentation/git-reset.txt
> >> +++ b/Documentation/git-reset.txt
> >> @@ -122,7 +122,7 @@ entries:
> >>         X       U     A    B     --soft  (disallowed)
> >>                                  --mixed  X       B     B
> >>                                  --hard   B       B     B
> >> -                                --merge (disallowed)
> >> +                                --merge  X       B     B
> >
> > IOW, I think the result should be "B B B" instead of "X B B" in this
> > case.
>
> A squashable fix-up on top of your patch to match the wish in the part
> you quoted from 9e8ecea (Add 'merge' mode to 'git reset', 2008-12-01)
> would look like this, I think.
>
> It does three things:
>
>  - Updates the documentation to match the wish of original "reset
> --merge" better, namely, "An unmerged entry is a sign that the path
> didn't have any local modification and can be safely resetted to whatever
> the new HEAD records";
>
>  - Updates read_index_unmerged(), which reads the index file into the
>    cache while dropping any higher-stage entries down to stage #0, not to
>    copy the object name recorded in the cache entry.  The code used to
>    take the object name from the highest stage entry ("theirs" if you
>    happened to have stage #3, or "ours" if they removed while you kept),
>    which essentially meant that you are getting random results and didn't
>    make sense.
>
>    The _only_ reason we want to keep a previously unmerged entry in the
>    index at stage #0 is so that we don't forget the fact that we have
>    corresponding file in the work tree in order to be able to remove it
>    when the tree we are resetting to does not have the path.  In order to
>    differentiate such an entry from ordinary cache entry, the cache entry
>    added by read_index_unmerged() records null sha1.
>
>  - Updates merged_entry() and deleted_entry() so that they pay attention
>    to cache entries with null sha1 (note that we _might_ want to use a
>    new in-core ce->ce_flags instead of using the null-sha1 hack). They
>    are previously unmerged entries, and the files in the work tree that
>    correspond to them are resetted away by oneway_merge() to the version
>    from the tree we are resetting to.
>
> Please take this with a grain of salt as I am under slight influence of
> CH3-CH2-OH while writing it, and I usually almost never drink.

I like this patch. It seems to improve the behavior of the --keep option for 
unmerged entries too.

The previous behavior was:

    working index HEAD target         working index HEAD
    ----------------------------------------------------
     X       U     A    B     --keep   X       B     B
     X       U     A    A     --keep   X       A     A

and now it is:

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

And I think it is better, as it is more consistent with the behavior when 
there are no unmerged entries.

The only problem is that when it fails the error message 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.

I will send a RFC patch series so people interested can test this.

Thanks,
Christian.

Re: [PATCH v6 2/4] Documentation: reset: add some tables to describe the different options

[Christian Couder]

On vendredi 01 janvier 2010, Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
> > 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>
>
> Much nicer.

Thanks.

> >  Documentation/git-reset.txt |   66
> > +++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 66
> > insertions(+), 0 deletions(-)
> >
> > diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
> > index 2d27e40..c9044c9 100644
> > --- a/Documentation/git-reset.txt
> > +++ b/Documentation/git-reset.txt
> > @@ -67,6 +67,72 @@ 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.
>
> Together with these "mechanical definitions", I think the readers would
> benefit from reading why some are disallowed.

Ok, I will add some explanations along what you write below.

> > +      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)
>
> "reset --merge" is meant to be used when resetting out of a conflicted
> merge.  Because any mergy operation guarantees that the work tree file
> that is involved in the merge does not have local change wrt the index
> before it starts, and that it writes the result out to the work tree, the
> fact that we see difference between the index and the HEAD and also
> between the index and the work tree means that we are not seeing a state
> that a mergy operation left after failing with a conflict.  That is why
> we disallow --merge option in this case, and the next one.
>
> > +      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)
>
> The same as above, but you are resetting to the same commit.
>
> > +      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
> >
> > +      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
>
> As this table is not only about "--merge" but to explain "reset", I think
> other interesting cases should also be covered.
>
> w=A	i=B	H=B	t=B
>
> This is "we had local change in the work tree that was unrelated to the
> merge", and "reset --merge" should be a no-op for this path.
>
> w=A	i=B	H=B	t=C
>
> This "reset --merge" is like "using checkout to switch to a commit that
> has C but we have changes in the work tree", and should fail just like
> "checkout branch" in such a situation fails without "-m" option.

Ok I will add these cases too.

Thanks,
Christian.

Re: [PATCH v6 3/4] reset: add a few tests for "git reset --merge"

[Christian Couder]

On vendredi 01 janvier 2010, Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
> > 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>
>
> Looks good.

Thanks again.

> > +# 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       D     D    D     --merge  C       D     D
> > +test_expect_success 'reset --merge is ok with changes in file it does
> > not touch' ' +    git reset --merge HEAD^ &&
> > +    ! grep 4 file1 &&
> > +    grep 4 file2 &&
> > +    test "$(git rev-parse HEAD)" = "$(git rev-parse initial)" &&
> > +    test -z "$(git diff --cached)"
> > +'
> > ...
> > +# 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)' ' +    git reset --hard second &&
> > +    echo "line 4" >> file2 &&
> > +    git add file2 &&
> > +    git reset --merge HEAD^ &&
> > +    ! grep 4 file2 &&
> > +    test "$(git rev-parse HEAD)" = "$(git rev-parse initial)" &&
> > +    test -z "$(git diff)" &&
> > +    test -z "$(git diff --cached)"
> > +'
>
> These two seem to duplicate the same case for file1; is it necessary?

No. I think I just copied the previous test and added the "git add file2" 
line.

> I am not pointing it out as something that needs to be removed; I am just
> puzzled and wondering if there is some interaction between the ways two
> paths are handled and the test is trying to check that (which I do not
> think is the case).

Best regards,
Christian.