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

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

[Christian Couder]

As agreed during private discussions, I am trying to refactor and
merge some interesting bits of code from the GSoC sequencer project.

So here is a first round about "git reset".

Patches 1/4 and 2/4 are using sequencer code to speed up "git reset" and
add some test cases for "git reset --merge".

Patches 3/4 and 4/4 are implementing "git reset --merge-dirty" and
showing the differences with "--merge". "--merge-dirty" is a really bad
name for the index reset option that is available using the "allow_dirty"
global variable in the sequencer. These 2 patches are for discussing
this feature.

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

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

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

[PATCH 1/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.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t7110-reset-merge.sh |   70 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 70 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..45714ae
--- /dev/null
+++ b/t/t7110-reset-merge.sh
@@ -0,0 +1,70 @@
+#!/bin/sh
+#
+# Copyright (c) 2009 Christian Couder
+#
+
+test_description='Tests for "git reset --merge"'
+
+exec </dev/null
+
+. ./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 'ok with changes in file not changed by reset' '
+     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 'discard 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 'discard 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 'not ok with changes in file changed by reset' '
+     echo "line 6" >> file1 &&
+     test_tick &&
+     git commit -m "add line 6" 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_done
-- 
1.6.4.271.ge010d

[PATCH 2/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".

The code comes from the sequencer GSoC project:

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

(at commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)

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

diff --git a/builtin-reset.c b/builtin-reset.c
index 73e6022..ddb81f3 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>]",
@@ -52,29 +54,56 @@ static inline int is_merge(void)
 	return !access(git_path("MERGE_HEAD"), F_OK);
 }
 
+static int parse_and_init_tree_desc(const unsigned char *sha1,
+					     struct tree_desc *desc)
+{
+	struct tree *tree = parse_tree_indirect(sha1);
+	if (!tree)
+		return 1;
+	init_tree_desc(desc, tree->buffer, tree->size);
+	return 0;
+}
+
 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 (parse_and_init_tree_desc(sha1, desc + nr - 1))
+		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)
-- 
1.6.4.271.ge010d

[PATCH 3/4] reset: add option "--merge-dirty" to "git reset"

[Christian Couder]

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

This option is nearly like "--merge" except that it is
a little bit safer as it seems that it tries to keep
changes in the index. On the contrary "--merge", only
keep changes in the work tree.

This will be shown in the next patch that adds some
test cases for "--merge-dirty".

In fact with "--merge-dirty", 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
"--merge", changes that are in both the work tree and
the index are discarded after the reset.

So "--merge-dirty" is probably a very bad name for
this new option. Perhaps "--merge-safe" is better?

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

diff --git a/builtin-reset.c b/builtin-reset.c
index ddb81f3..be7aa8d 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 | --merge-dirty] [-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, MERGE_DIRTY, NONE };
+static const char *reset_type_names[] = {
+	"mixed", "soft", "hard", "merge", "merge_dirty", NULL
+};
 
 static char *args_to_str(const char **argv)
 {
@@ -84,6 +86,7 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
 	case MERGE:
 		opts.update = 1;
 		break;
+	case MERGE_DIRTY:
 	case HARD:
 		opts.update = 1;
 		/* fallthrough */
@@ -95,6 +98,16 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
 
 	read_cache_unmerged();
 
+	if (reset_type == MERGE_DIRTY) {
+		unsigned char *head_sha1;
+		if (get_sha1("HEAD", head_sha1))
+			return error("You do not have a valid HEAD.");
+		if (parse_and_init_tree_desc(head_sha1, desc))
+			return error("Failed to find tree of HEAD.");
+		nr++;
+		opts.fn = twoway_merge;
+	}
+
 	if (parse_and_init_tree_desc(sha1, desc + nr - 1))
 		return error("Failed to find tree of %s.", sha1_to_hex(sha1));
 	if (unpack_trees(nr, desc, &opts))
@@ -238,6 +251,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, "merge-dirty", &reset_type,
+				"reset HEAD, index and working tree",
+				MERGE_DIRTY),
 		OPT_BOOLEAN('q', NULL, &quiet,
 				"disable showing new HEAD in hard reset and progress message"),
 		OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
@@ -324,9 +340,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 == MERGE_DIRTY)
+			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.4.271.ge010d

[PATCH 4/4] reset: add test cases for "--merge-dirty" option

[Christian Couder]

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

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

diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
index 45714ae..1e6d634 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 --merge-dirty'
 
 exec </dev/null
 
@@ -19,7 +19,7 @@ test_expect_success 'creating initial files' '
      git commit -m "Initial commit"
 '
 
-test_expect_success 'ok with changes in file not changed by reset' '
+test_expect_success '--merge: ok if changes in file not touched by reset' '
      echo "line 4" >> file1 &&
      echo "line 4" >> file2 &&
      test_tick &&
@@ -32,7 +32,21 @@ test_expect_success 'ok with changes in file not changed by reset' '
      grep 4 file2
 '
 
-test_expect_success 'discard changes added to index 1' '
+test_expect_success '--merge-dirty: ok if changes in file untouched by reset' '
+     git reset --hard HEAD^ &&
+     echo "line 4" >> file1 &&
+     echo "line 4" >> file2 &&
+     test_tick &&
+     git commit -m "add line 4" file1 &&
+     git reset --merge-dirty HEAD^ &&
+     ! grep 4 file1 &&
+     grep 4 file2 &&
+     git reset --merge-dirty HEAD@{1} &&
+     grep 4 file1 &&
+     grep 4 file2
+'
+
+test_expect_success '--merge: discard changes added to index 1' '
      echo "line 5" >> file1 &&
      git add file1 &&
      git reset --merge HEAD^ &&
@@ -47,7 +61,7 @@ test_expect_success 'discard changes added to index 1' '
      grep 4 file1
 '
 
-test_expect_success 'discard changes added to index 2' '
+test_expect_success '--merge: discard changes added to index 2' '
      echo "line 4" >> file2 &&
      git add file2 &&
      git reset --merge HEAD^ &&
@@ -57,7 +71,37 @@ test_expect_success 'discard changes added to index 2' '
      grep 4 file1
 '
 
-test_expect_success 'not ok with changes in file changed by reset' '
+test_expect_success '--merge-dirty: not ok with touched changes in index' '
+     echo "line 4" >> file2 &&
+     echo "line 5" >> file1 &&
+     git add file1 &&
+     test_must_fail git reset --merge-dirty HEAD^ &&
+     git reset --hard HEAD
+'
+
+test_expect_success '--merge-dirty: keep untouched changes' '
+     echo "line 4" >> file2 &&
+     git add file2 &&
+     git reset --merge-dirty HEAD^ &&
+     grep 4 file2 &&
+     git reset --merge HEAD@{1} &&
+     grep 4 file2 &&
+     grep 4 file1 &&
+     git reset --hard HEAD
+'
+
+test_expect_success '--merge: not ok with changes in file changed by reset' '
+     echo "line 6" >> file1 &&
+     test_tick &&
+     git commit -m "add line 6" 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 '--merge-dirty: not ok with changes in file changed by reset' '
+     git reset --hard HEAD^ &&
      echo "line 6" >> file1 &&
      test_tick &&
      git commit -m "add line 6" file1 &&
-- 
1.6.4.271.ge010d

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

[Jakub Narebski]

Christian Couder wrote:

> diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
> new file mode 100755
> index 0000000..45714ae
> --- /dev/null
> +++ b/t/t7110-reset-merge.sh
> @@ -0,0 +1,70 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2009 Christian Couder
> +#
> +
> +test_description='Tests for "git reset --merge"'
> +
> +exec </dev/null

What does this do?

-- 
Jakub Narebski
Poland

Re: [PATCH 4/4] reset: add test cases for "--merge-dirty" option

[Daniel Barkalow]

On Thu, 10 Sep 2009, Christian Couder wrote:

    This shows that with the "--merge-dirty" 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.

I'm lost here.

If you have:

         working index HEAD target
version     B      B     A     A

You get:

         working index HEAD target
--m-d       B      A     A     A
--merge     A      A     A     A

?

> ---
>  t/t7110-reset-merge.sh |   54 +++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 49 insertions(+), 5 deletions(-)
> 
> diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
> index 45714ae..1e6d634 100755
> --- a/t/t7110-reset-merge.sh
> +++ b/t/t7110-reset-merge.sh
> @@ -19,7 +19,7 @@ test_expect_success 'creating initial files' '
>       git commit -m "Initial commit"
>  '
>  
> -test_expect_success 'ok with changes in file not changed by reset' '
> +test_expect_success '--merge: ok if changes in file not touched by reset' '

Should probably have the "--merge: " from the beginning, since you're 
adding the test in this series anyway. That would make the diff come out 
clearer.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 3/4] reset: add option "--merge-dirty" to "git reset"

[Linus Torvalds]

On Thu, 10 Sep 2009, Christian Couder wrote:
> 
> In fact with "--merge-dirty", 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
> "--merge", changes that are in both the work tree and
> the index are discarded after the reset.

Can you explain that again? That paragraph makes no sense, because it 
boils down to saying two different thing for "changes that are in both the 
work tree and the index". First it says such changes are kept in the work 
tree (but discarded in the index), and then it says that the changes are 
discarded.

Which seems very odd.

Or are my reading comprehension skills lacking?

The patch also seems to imply that it's always about HEAD. True?

		Linus

Re: [PATCH 4/4] reset: add test cases for "--merge-dirty" option

[Christian Couder]

On Friday 11 September 2009, Daniel Barkalow wrote:
> On Thu, 10 Sep 2009, Christian Couder wrote:
>
>     This shows that with the "--merge-dirty" 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.
>
> I'm lost here.
>
> If you have:
>
>          working index HEAD target
> version     B      B     A     A
>
> You get:
>
>          working index HEAD target
> --m-d       B      A     A     A
> --merge     A      A     A     A
>
> ?

Yes, files that are not different between HEAD and target are changed like 
that. Thanks for explaining it better than I could!

And also when files are different between HEAD and target (and when the 
index is the same as the working tree but different than HEAD), the result 
from -m-d and --merge are different:

If you have:

          working index HEAD target
 version     B      B     A     C

You get:

          working index HEAD target
 --m-d       B      B     A     C   : --m-d fails (so no change)
 --merge     C      C     C     C   : success but changes B discarded

In t/t7110-reset-merge.sh these differences are shown by test cases 3, 4, 5 
and 6:

--merge: discard changes added to index 1
--merge: discard changes added to index 2
--merge-dirty: not ok with touched changes in index
--merge-dirty: keep untouched changes

> > ---
> >  t/t7110-reset-merge.sh |   54
> > +++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 49
> > insertions(+), 5 deletions(-)
> >
> > diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
> > index 45714ae..1e6d634 100755
> > --- a/t/t7110-reset-merge.sh
> > +++ b/t/t7110-reset-merge.sh
> > @@ -19,7 +19,7 @@ test_expect_success 'creating initial files' '
> >       git commit -m "Initial commit"
> >  '
> >
> > -test_expect_success 'ok with changes in file not changed by reset' '
> > +test_expect_success '--merge: ok if changes in file not touched by
> > reset' '
>
> Should probably have the "--merge: " from the beginning, since you're
> adding the test in this series anyway. That would make the diff come out
> clearer.

Yeah, but I am not sure that patches 3/4 and 4/4 will get merged in the end.
If they are not merged it will be better if there is no "--merge: ". 

If they get merged, the option name '--merge-dirty" will probably be changed 
to something else like "--merge-safe" so I will have to change some patches 
anyway.

Thanks,
Christian.

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

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

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

And the reason why it is a good idea is...?

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

[Christian Couder]

On Thursday 10 September 2009, Jakub Narebski wrote:
> Christian Couder wrote:
> > diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
> > new file mode 100755
> > index 0000000..45714ae
> > --- /dev/null
> > +++ b/t/t7110-reset-merge.sh
> > @@ -0,0 +1,70 @@
> > +#!/bin/sh
> > +#
> > +# Copyright (c) 2009 Christian Couder
> > +#
> > +
> > +test_description='Tests for "git reset --merge"'
> > +
> > +exec </dev/null
>
> What does this do?

Nothing! Yeah, this is a mistake. 

Thanks for spoting it,
Christian.

Re: [PATCH 3/4] reset: add option "--merge-dirty" to "git reset"

[Christian Couder]

On Friday 11 September 2009, Linus Torvalds wrote:
> On Thu, 10 Sep 2009, Christian Couder wrote:
> > In fact with "--merge-dirty", 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
> > "--merge", changes that are in both the work tree and
> > the index are discarded after the reset.
>
> Can you explain that again?

Yeah, I tried to explain it better in my reply to Daniel.

> That paragraph makes no sense, because it 
> boils down to saying two different thing for "changes that are in both
> the work tree and the index". First it says such changes are kept in the
> work tree (but discarded in the index), and then it says that the changes
> are discarded.
>
> Which seems very odd.
>
> Or are my reading comprehension skills lacking?

No, I realize it was my writing skills that were lacking again. I will 
rework the commit message using some tables like in Daniel's message.

> The patch also seems to imply that it's always about HEAD. True?

Yes.

Best regards,
Christian.

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

[Christian Couder]

On Friday 11 September 2009, Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
> > 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".
>
> And the reason why it is a good idea is...?

...that it should be faster.

Ok, I will add that to the commit message in the next version.

Thanks,
Christian.

Re: [PATCH 3/4] reset: add option "--merge-dirty" to "git reset"

[Junio C Hamano]

I have two comments (and I share Linus's puzzlement).

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

 - Trying to give credit to Stephan is nice, but comparing this with bits
   from the previous round, I am not sure if you need to keep calling this
   Stephan's code anymore.  Perhaps it would be enough to replace the last
   three lines in your commit log message with "Uses some code from
   Stephan's GSoC work in git://repo.or.cz/git/sbeyer.git repository".

   And I would actually prefer you take the authorship, especially if
   Stephan's series does not add an option to "reset".  Then we know whom
   to forward any bug reports to ;-)

   I did not compare this with the GSoC repository, so maybe this is what
   Stephan did after all, but still I thought it is worth to ask.

> This option is nearly like "--merge" except that it is a little bit
> safer as it seems that it tries to keep changes in the index. On the
> contrary "--merge", only keep changes in the work tree.

Nearly, a little bit, seems and tries?

These words do not build confidence that this new feature is robust and
behaves predictably.

Of course, you are not adding a random number generator, but a useful mode
with a bit more complex behaviour than "--merge that _always_ discards
changes in the index."  So as the salesman, you would need to do a bit
better job to explain what the new behaviour is, and why it is better to
have that new behaviour than the behaviour of --merge, to sell that new
feature.

Re: [PATCH 4/4] reset: add test cases for "--merge-dirty" option

[Daniel Barkalow]

On Fri, 11 Sep 2009, Christian Couder wrote:

> On Friday 11 September 2009, Daniel Barkalow wrote:
> > On Thu, 10 Sep 2009, Christian Couder wrote:
> >
> >     This shows that with the "--merge-dirty" 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.
> >
> > I'm lost here.
> >
> > If you have:
> >
> >          working index HEAD target
> > version     B      B     A     A
> >
> > You get:
> >
> >          working index HEAD target
> > --m-d       B      A     A     A
> > --merge     A      A     A     A
> >
> > ?
> 
> Yes, files that are not different between HEAD and target are changed like 
> that. Thanks for explaining it better than I could!

I worked on the rules for merging way back when, so I've looked at tables 
of cases like that. If there are more cases to cover, it might work 
better to have a table like:

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

Are there other differences?

> > > ---
> > >  t/t7110-reset-merge.sh |   54
> > > +++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 49
> > > insertions(+), 5 deletions(-)
> > >
> > > diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
> > > index 45714ae..1e6d634 100755
> > > --- a/t/t7110-reset-merge.sh
> > > +++ b/t/t7110-reset-merge.sh
> > > @@ -19,7 +19,7 @@ test_expect_success 'creating initial files' '
> > >       git commit -m "Initial commit"
> > >  '
> > >
> > > -test_expect_success 'ok with changes in file not changed by reset' '
> > > +test_expect_success '--merge: ok if changes in file not touched by
> > > reset' '
> >
> > Should probably have the "--merge: " from the beginning, since you're
> > adding the test in this series anyway. That would make the diff come out
> > clearer.
> 
> Yeah, but I am not sure that patches 3/4 and 4/4 will get merged in the end.
> If they are not merged it will be better if there is no "--merge: ". 

Maybe write those lines to mention "reset --merge" naturally? Like:

'ok with changes in file not changed by reset --merge'

'reset --merge discards changes added to index 1'

	-Daniel
*This .sig left intentionally blank*

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

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> On Friday 11 September 2009, Junio C Hamano wrote:
>> Christian Couder <chriscool@tuxfamily.org> writes:
>> > 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".
>>
>> And the reason why it is a good idea is...?
>
> ...that it should be faster.
>
> Ok, I will add that to the commit message in the next version.

Please don't add "should be".  That is not a justification.

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

[Daniel Barkalow]

On Fri, 11 Sep 2009, Christian Couder wrote:

> On Friday 11 September 2009, Junio C Hamano wrote:
> > Christian Couder <chriscool@tuxfamily.org> writes:
> > > 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".
> >
> > And the reason why it is a good idea is...?
> 
> ...that it should be faster.
> 
> Ok, I will add that to the commit message in the next version.

There's also the benefit (IMHO, more significant) that git-read-tree's 
command line parsing is complicated, and using it from git-reset makes it 
hard to see exactly what each option of "git reset" does in terms of 
operations on the index and the working tree.

For example, it's not obvious when reading the code to run read-tree that 
all of the branches lead to the use of the "merge" flag, because some 
branches use "-m" and some use "--reset".

Actually, there's a behavior difference between the old version and the 
new version. The old version gives an error for "git reset --merge" with 
unmerged entries (unlike any other option to "git reset", AFAICT), and the 
new version does not. There's no way you'd know this without a careful 
reading of cmd_read_tree() and cross-reference with reset_index_file(), 
since the documentation doesn't mention it, the code in reset_index_file() 
only replaces "--reset" with "-m", and it seems to be doing that for the 
effect of not ignoring differences in the working tree.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 3/4] reset: add option "--merge-dirty" to "git reset"

[Linus Torvalds]

On Fri, 11 Sep 2009, Christian Couder wrote:
> 
> No, I realize it was my writing skills that were lacking again. I will 
> rework the commit message using some tables like in Daniel's message.

Ok, thanks.

> > The patch also seems to imply that it's always about HEAD. True?
> 
> Yes.

It might be worth noting/explaining very explicitly. "git reset" uin 
general does not ever try to make HEAD special - except as a _default_ 
location, of course, and as the thing that is actually modified.

I realize _why_ (HEAD is kind of the "base" for the index state), but I 
think it's worth a comment or something. I did a double-take when I saw 
that part of the diff exactly because it's unusual.

		Linus

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

[Tony Finch]

On Fri, 11 Sep 2009, Christian Couder wrote:
> On Thursday 10 September 2009, Jakub Narebski wrote:
> > Christian Couder wrote:
> > >
> > > exec </dev/null
> >
> > What does this do?
>
> Nothing! Yeah, this is a mistake.

It redirects stdin of the current shell.

Tony.
-- 
f.anthony.n.finch  <dot@dotat.at>  http://dotat.at/
GERMAN BIGHT HUMBER: SOUTHWEST 5 TO 7. MODERATE OR ROUGH. SQUALLY SHOWERS.
MODERATE OR GOOD.

Re: [PATCH 4/4] reset: add test cases for "--merge-dirty" option

[Paolo Bonzini]

> If they get merged, the option name '--merge-dirty" will probably be changed
> to something else like "--merge-safe" so I will have to change some patches
> anyway.

--merge-index or --merge-staged?

Also, it would be great if you would prepare tables like these for all 
cases of git-reset.

Paolo

Re: [PATCH 4/4] reset: add test cases for "--merge-dirty" option

[Christian Couder]

On Friday 11 September 2009, Daniel Barkalow wrote:
> On Fri, 11 Sep 2009, Christian Couder wrote:
> > On Friday 11 September 2009, Daniel Barkalow wrote:
> > > On Thu, 10 Sep 2009, Christian Couder wrote:
> > >
> > >     This shows that with the "--merge-dirty" 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.
> > >
> > > I'm lost here.
> > >
> > > If you have:
> > >
> > >          working index HEAD target
> > > version     B      B     A     A
> > >
> > > You get:
> > >
> > >          working index HEAD target
> > > --m-d       B      A     A     A
> > > --merge     A      A     A     A
> > >
> > > ?
> >
> > Yes, files that are not different between HEAD and target are changed
> > like that. Thanks for explaining it better than I could!
>
> I worked on the rules for merging way back when, so I've looked at tables
> of cases like that. If there are more cases to cover, it might work
> better to have a table like:
>
> working index HEAD target         working index HEAD
>    B      B     A     A   --m-d      B      A     A
>                           --merge    A      A     A
>    B      B     A     C   --m-d       (disallowed)
>                           --merge    C      C     C
>
> Are there other differences?

Yes, I found that I messed up the last test in patch 4/4. I forgot to 
replace some --merge with --merge-dirty :-(

In fact while "reset --merge" fails when there are changes in files that are 
changed between HEAD and target, "reset --merge-dirty" will not fail and 
discard these changes. So it is not really safe in this case and I am 
working on trying to make it safer in this case.

> > > > ---
> > > >  t/t7110-reset-merge.sh |   54
> > > > +++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 49
> > > > insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
> > > > index 45714ae..1e6d634 100755
> > > > --- a/t/t7110-reset-merge.sh
> > > > +++ b/t/t7110-reset-merge.sh
> > > > @@ -19,7 +19,7 @@ test_expect_success 'creating initial files' '
> > > >       git commit -m "Initial commit"
> > > >  '
> > > >
> > > > -test_expect_success 'ok with changes in file not changed by reset'
> > > > ' +test_expect_success '--merge: ok if changes in file not touched
> > > > by reset' '
> > >
> > > Should probably have the "--merge: " from the beginning, since you're
> > > adding the test in this series anyway. That would make the diff come
> > > out clearer.
> >
> > Yeah, but I am not sure that patches 3/4 and 4/4 will get merged in the
> > end. If they are not merged it will be better if there is no "--merge:
> > ".
>
> Maybe write those lines to mention "reset --merge" naturally? Like:
>
> 'ok with changes in file not changed by reset --merge'
>
> 'reset --merge discards changes added to index 1'

Ok I will do that.

Thanks,
Christian.