threads / patch / 20896

patch, 4 partsreset: add option "--merge-dirty" to "git reset"

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

## tl;dr

21 messages between Sep 10, 2009 and Sep 15, 2009. Diffs are folded; open one to read it.

replies: 20people: 7as markdown or json

Christian Couder· Sep 10, 2009, 20:23 UTC · lore

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

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
Christian Couder· Sep 10, 2009, 20:23 UTC · re: Christian Couder · lore

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

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
Show changes to t/t7110-reset-merge.sh +70 −0
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
Jakub Narebski· Sep 10, 2009, 20:59 UTC · re: Christian Couder · lore

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

Christian Couder wrote:
Show 14 quoted lines
> 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
Christian Couder· Sep 11, 2009, 05:22 UTC · re: Jakub Narebski · lore

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

On Thursday 10 September 2009, Jakub Narebski wrote:
Show 17 quoted lines
> 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.

Tony Finch· Sep 13, 2009, 22:01 UTC · re: Christian Couder · lore

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

On Fri, 11 Sep 2009, Christian Couder wrote:
Show 8 quoted lines
> 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.
Christian Couder· Sep 10, 2009, 20:23 UTC · re: Christian Couder · lore

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

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(-)
Show changes to builtin-reset.c +40 −11
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
Junio C Hamano· Sep 11, 2009, 05:20 UTC · re: Christian Couder · lore

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

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...?
Christian Couder· Sep 11, 2009, 05:34 UTC · re: Junio C Hamano · lore

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

On Friday 11 September 2009, Junio C Hamano wrote:
Show 7 quoted lines
> 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.

Junio C Hamano· Sep 11, 2009, 05:55 UTC · re: Christian Couder · lore

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

Christian Couder <chriscool@tuxfamily.org> writes:
Show 12 quoted lines
> 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.
Daniel Barkalow· Sep 11, 2009, 06:32 UTC · re: Christian Couder · lore

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

On Fri, 11 Sep 2009, Christian Couder wrote:
Show 12 quoted lines
> 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*
Christian Couder· Sep 10, 2009, 20:23 UTC · re: Christian Couder · lore
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(-)
Show changes to builtin-reset.c +25 −5
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
Linus Torvalds· Sep 10, 2009, 23:24 UTC · re: Christian Couder · lore

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

On Thu, 10 Sep 2009, Christian Couder wrote:
Show 6 quoted lines
> 
> 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
Christian Couder· Sep 11, 2009, 05:29 UTC · re: Linus Torvalds · lore

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

On Friday 11 September 2009, Linus Torvalds wrote:
Show 8 quoted lines
> 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.
Show 9 quoted lines
> 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.

Linus Torvalds· Sep 11, 2009, 16:02 UTC · re: Christian Couder · lore

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

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
Junio C Hamano· Sep 11, 2009, 05:34 UTC · re: Christian Couder · lore

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

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.

Christian Couder· Sep 10, 2009, 20:23 UTC · re: Christian Couder · lore

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

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(-)
Show changes to t/t7110-reset-merge.sh +49 −5
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
Daniel Barkalow· Sep 10, 2009, 22:14 UTC · re: Christian Couder · lore

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

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
?
Show 14 quoted lines
> ---
>  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*
Christian Couder· Sep 11, 2009, 05:05 UTC · re: Daniel Barkalow · lore

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

On Friday 11 September 2009, Daniel Barkalow wrote:
Show 27 quoted lines
> 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

Show 20 quoted lines
> > ---
> >  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.

Daniel Barkalow· Sep 11, 2009, 05:36 UTC · re: Christian Couder · lore

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

On Fri, 11 Sep 2009, Christian Couder wrote:
Show 31 quoted lines
> 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?
Show 23 quoted lines
> > > ---
> > >  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*
Christian Couder· Sep 15, 2009, 04:32 UTC · re: Daniel Barkalow · lore

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

On Friday 11 September 2009, Daniel Barkalow wrote:
Show 44 quoted lines
> 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.

Show 30 quoted lines
> > > > ---
> > > >  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.

Paolo Bonzini· Sep 13, 2009, 22:15 UTC · re: Christian Couder · lore

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

> 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

← back to recent threads