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

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

[Christian Couder]

In this new version, some test cases have been added to show that the
behavior of "git reset --merge" when there is a pending merge is changed
by patch 2/4.

And some commit messages have been improved, thanks to Junio.

There is no documentation yet but I am working on it. 

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

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

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

[PATCH v3 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.

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

[PATCH v3 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". So the code is more
efficient.

And it's also easier to see which unpack_tree() options will be used,
as we don't need to follow "git read-tree"'s command line parsing
which is quite complex.

As Daniel Barkalow found, there is a difference between this new
version and the old one. The old version gives an error for
"git reset --merge" with unmerged entries and the new version does
not. But this can be seen as a bug fix, because "--merge" was the
only "git reset" option with this behavior and this behavior was
not documented.

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 +++++++++++++++++++++++++++++++++++++----------
 t/t7110-reset-merge.sh |    7 ++++-
 2 files changed, 45 insertions(+), 13 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)
diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
index 8a28553..6211096 100755
--- a/t/t7110-reset-merge.sh
+++ b/t/t7110-reset-merge.sh
@@ -79,9 +79,12 @@ test_expect_success 'setup 2 different branches' '
      git commit -a -m "change in branch2"
 '
 
-test_expect_success 'reset --merge fails with pending merge' '
+test_expect_success 'reset --merge is ok with pending merge' '
      test_must_fail git merge branch1 &&
-     test_must_fail git reset --merge HEAD^
+     git reset --merge HEAD^ &&
+     test -z "$(git diff --cached)" &&
+     test -n "$(git diff)" &&
+     git reset --hard HEAD@{1}
 '
 
 test_done
-- 
1.6.5.rc0.150.g38fe6

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

[Christian Couder]

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

This option is nearly like "--merge" except that it is
safer.

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

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

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

So as can be seen in the table, "--merge" discards changes
in the index, while "--merge-safe" does not.

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

The "--merge-safe" option is implemented by doing a 2 way
merge between HEAD and the reset target, and if this
succeeds by doing a mixed reset to the target.

The code comes from the sequencer GSoC project:

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

(at commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)

But in the sequencer project the "reset" flag was set
in the "struct unpack_trees_options" passed to
"unpack_trees()". With this flag the changes in the
working tree were discarded if the file was different
between HEAD and the reset target.

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

diff --git a/builtin-reset.c b/builtin-reset.c
index ddb81f3..78d42e6 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-safe] [-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_SAFE, NONE };
+static const char *reset_type_names[] = {
+	"mixed", "soft", "hard", "merge", "merge_safe", NULL
+};
 
 static char *args_to_str(const char **argv)
 {
@@ -81,6 +83,7 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
 	if (!quiet)
 		opts.verbose_update = 1;
 	switch (reset_type) {
+	case MERGE_SAFE:
 	case MERGE:
 		opts.update = 1;
 		break;
@@ -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_SAFE) {
+		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-safe", &reset_type,
+				"reset HEAD, index and working tree",
+				MERGE_SAFE),
 		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_SAFE)
+			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.5.rc0.150.g38fe6

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

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> From: Stephan Beyer <s-beyer@gmx.net>
>
> This option is nearly like "--merge" except that it is
> safer.

Do you still want to have this description after the last round?

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

I'd like to see at least the following rows filled as well.

    X      U     A     A   --m-s      ???    ???   ???
                           --merge    ???    ???   ???
    X      U     B     A   --m-s      ???    ???   ???
                           --merge    ???    ???   ???

> In this table, A, B and C are some different states of a file.

... and X is "don't care", and U is "unmerged index".

> The code comes from the sequencer GSoC project:
>
> git://repo.or.cz/git/sbeyer.git
>
> (at commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)
>
> But in the sequencer project the "reset" flag was set in the "struct
> unpack_trees_options" passed to "unpack_trees()". With this flag the
> changes in the working tree were discarded if the file was different
> between HEAD and the reset target.

If you need to have four lines worth of description here, is this still
Stephan's patch, or would it be more appropriate to say "This is based on
an earlier GSoC work by Stephan in git://repo.or.cz/git/sbeyer.git
repository." and you take all the credit and blame?

>  static const char * const git_reset_usage[] = {
> -	"git reset [--mixed | --soft | --hard | --merge] [-q] [<commit>]",
> +	"git reset [--mixed | --soft | --hard | --merge | --merge-safe] [-q] [<commit>]",
>  	"git reset [--mixed] <commit> [--] <paths>...",
>  	NULL
>  };

As we established in the previous round, this is _different_ from --merge,
but *not* in the sense that --merge is more dangerous and users should be
using this new option instead, but in the sense that --merge perfectly
works well for its intended use case, and this new option triggers a mode
of operation that is meant to be used in a completely different use case,
which is unspecified in this series without documentation.

In that light, is --merge-safe still a good name for the option, or merely
a misleading one?

As I said in the previous round, --merge discards the modified index state
when switching, and it is absolutely _the right thing to do_ in the use
case it was intended for.  It is _not_ dangerous, and using --merge-safe
in that scenario is not being _safe_ but is actively a _wrong_ thing to do.

> @@ -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_SAFE) {
> +		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;
> +	}

get_sha1() takes an allocated buffer, does not allocate space on its own.
I think you meant "unsigned char head_sha1[20];" here.

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

[Johannes Sixt]

Junio C Hamano schrieb:
> As we established in the previous round, this is _different_ from --merge,
> but *not* in the sense that --merge is more dangerous and users should be
> using this new option instead, but in the sense that --merge perfectly
> works well for its intended use case, and this new option triggers a mode
> of operation that is meant to be used in a completely different use case,
> which is unspecified in this series without documentation.
> 
> In that light, is --merge-safe still a good name for the option, or merely
> a misleading one?

Do I understand this correctly?

(1) The intended use-case of --merge is to "reset _a_ merge".

(2) The intended use-case of --merge-safe is to point the branch head to a
different commit, but to carry the changes that currently are in the index
and wd over to the new commit, similar to checkout --merge.

I had mistaken that --merge actually performs (2) because of the striking
similarity of the option's name to checkout's --merge. So, IMHO, whatever
the new option is named that performs (2) - it introduces an
inconsistency, because --merge is already taken.

-- Hannes

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

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Junio C Hamano schrieb:
>> As we established in the previous round, this is _different_ from --merge,
>> but *not* in the sense that --merge is more dangerous and users should be
>> using this new option instead, but in the sense that --merge perfectly
>> works well for its intended use case, and this new option triggers a mode
>> of operation that is meant to be used in a completely different use case,
>> which is unspecified in this series without documentation.
>> 
>> In that light, is --merge-safe still a good name for the option, or merely
>> a misleading one?
>
> Do I understand this correctly?
>
> (1) The intended use-case of --merge is to "reset _a_ merge".

See my review comment for the previous round where I described the
intended use case of "reset --merge" and explained why discarding the
changes to the index is _the right thing_.  It is to throw away the
changes that was done to your index by an either completed or conflicted
merge.

> (2) The intended use-case of --merge-safe is to point the branch head to a
> different commit, but to carry the changes that currently are in the index
> and wd over to the new commit, similar to checkout --merge.

I have _no_ idea what the intended use-case of --merge-safe is, and that
was why I asked Christian for clarification in the previous round.  The
answer was still not clear enough so I pointed out --merge-safe could be
still doing a wrong thing even in _his_ use-case.

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

[Johannes Sixt]

Junio C Hamano schrieb:
> Johannes Sixt <j.sixt@viscovery.net> writes:
>> Junio C Hamano schrieb:
>> (2) The intended use-case of --merge-safe is to point the branch head to a
>> different commit, but to carry the changes that currently are in the index
>> and wd over to the new commit, similar to checkout --merge.

This is actually an operation that I need quite often, but I can do it
only by way of git stash.

Clarification: I did not say that I actually meant to carry *only* the
index and wd changes to the new commit. That is, the operation I have in
mind can roughly be done in terms of

  $ git stash
  $ git reset --hard $target
  $ git stash pop

> I have _no_ idea what the intended use-case of --merge-safe is, and that
> was why I asked Christian for clarification in the previous round.  The
> answer was still not clear enough so I pointed out --merge-safe could be
> still doing a wrong thing even in _his_ use-case.

Reading Christian in 200909170554.49416.chriscool@tuxfamily.org, I think
this *is* his use-case? Christian?

-- Hannes

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

[Christian Couder]

On Thu, Sep 17, 2009 at 9:24 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Junio C Hamano schrieb:
>> Johannes Sixt <j.sixt@viscovery.net> writes:
>>> Junio C Hamano schrieb:
>>> (2) The intended use-case of --merge-safe is to point the branch head to a
>>> different commit, but to carry the changes that currently are in the index
>>> and wd over to the new commit, similar to checkout --merge.
>
> This is actually an operation that I need quite often, but I can do it
> only by way of git stash.
>
> Clarification: I did not say that I actually meant to carry *only* the
> index and wd changes to the new commit. That is, the operation I have in
> mind can roughly be done in terms of
>
>  $ git stash
>  $ git reset --hard $target
>  $ git stash pop
>
>> I have _no_ idea what the intended use-case of --merge-safe is, and that
>> was why I asked Christian for clarification in the previous round.  The
>> answer was still not clear enough so I pointed out --merge-safe could be
>> still doing a wrong thing even in _his_ use-case.
>
> Reading Christian in 200909170554.49416.chriscool@tuxfamily.org, I think
> this *is* his use-case? Christian?

Yes, I agree, it can be used instead of git stash. By the way Linus, in his
patch that added the --merge option, said that --merge could be used like
that.

So for this use case --merge-safe is safer than --merge, and that's why I
suggested that name. But I know I suck at naming things, so there is
probably a better name.

Thanks,
Christian.

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

[Johannes Sixt]

Christian Couder schrieb:
> On Thu, Sep 17, 2009 at 9:24 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> Junio C Hamano schrieb:
>>> Johannes Sixt <j.sixt@viscovery.net> writes:
>>  $ git stash
>>  $ git reset --hard $target
>>  $ git stash pop
>>
>>> I have _no_ idea what the intended use-case of --merge-safe is, and that
>>> was why I asked Christian for clarification in the previous round.  The
>>> answer was still not clear enough so I pointed out --merge-safe could be
>>> still doing a wrong thing even in _his_ use-case.
>> Reading Christian in 200909170554.49416.chriscool@tuxfamily.org, I think
>> this *is* his use-case? Christian?
> 
> Yes, I agree, it can be used instead of git stash.

It "can"? Do you say that you intend --merge-safe for something else in
addition to the above stash + reset --hard + stash pop sequence? What?

> By the way Linus, in his
> patch that added the --merge option, said that --merge could be used like
> that.

But that use-case has one important difference: You can't use stash right
before the reset:

   # work tree is dirty
   $ git pull $there $topic  # assume we have conflicts

   # investigate result ...
   # oh no, that's crap, scratch it

   $ git stash what? conflicted changes?
   $ git reset what? --hard would remove my dirty state, too

You are screwed. 'git reset --merge' comes to rescue.

I'm pretty sure you don't mean --merge-safe to provide extra safety in
*this* use-case, but that you have a very different use-case in mind.

-- Hannes

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

[Christian Couder]

On Thu, Sep 17, 2009 at 3:05 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Christian Couder schrieb:
>> On Thu, Sep 17, 2009 at 9:24 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>> Junio C Hamano schrieb:
>>>> Johannes Sixt <j.sixt@viscovery.net> writes:
>>>  $ git stash
>>>  $ git reset --hard $target
>>>  $ git stash pop
>>>
>>>> I have _no_ idea what the intended use-case of --merge-safe is, and that
>>>> was why I asked Christian for clarification in the previous round.  The
>>>> answer was still not clear enough so I pointed out --merge-safe could be
>>>> still doing a wrong thing even in _his_ use-case.
>>> Reading Christian in 200909170554.49416.chriscool@tuxfamily.org, I think
>>> this *is* his use-case? Christian?
>>
>> Yes, I agree, it can be used instead of git stash.
>
> It "can"? Do you say that you intend --merge-safe for something else in
> addition to the above stash + reset --hard + stash pop sequence? What?

As I said to Junio, another "use case" is to enable more commands (like
an improved cherry-pick) to be used with a dirty work tree or index.

>> By the way Linus, in his
>> patch that added the --merge option, said that --merge could be used like
>> that.
>
> But that use-case has one important difference: You can't use stash right
> before the reset:
>
>   # work tree is dirty
>   $ git pull $there $topic  # assume we have conflicts
>
>   # investigate result ...
>   # oh no, that's crap, scratch it
>
>   $ git stash what? conflicted changes?
>   $ git reset what? --hard would remove my dirty state, too
>
> You are screwed. 'git reset --merge' comes to rescue.
>
> I'm pretty sure you don't mean --merge-safe to provide extra safety in
> *this* use-case,

You are right, I don't think it is usefull in this use case.

> but that you have a very different use-case in mind.

I don't have other use cases I didn't already talked about in mind.

Best regards,
Christian.

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

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Christian Couder schrieb:
>> On Thu, Sep 17, 2009 at 9:24 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>> Junio C Hamano schrieb:
>>>> Johannes Sixt <j.sixt@viscovery.net> writes:
>>>  $ git stash
>>>  $ git reset --hard $target
>>>  $ git stash pop
>>
> It "can"? Do you say that you intend --merge-safe for something else in
> addition to the above stash + reset --hard + stash pop sequence? What?

I think I am starting to understand.  The use case in a larger picture
would look like

    $ (edit/add/commit)+ to work towards something
    ... And you _finished_ that something.
    $ (edit/add)+ to work towards something else that is wonderful
    ... Now you notice that all the commits for that earlier something
    ... are crap and you would want to discard them, while still keeping
    ... changes for "something else that is wonderful".
    ... Luckily, you haven't committed anything you would want to keep,
    ... and you can afford to reset the tip commits away.
    $ git reset --keep-local-changes HEAD~7
    ... or howmanyever commits you want to discard.

The reset in this case is done purely to discard the crap, not to redo
them (you have something else you want to work on in your work tree and
index, and they are not fixups---if they were you wouldn't have resetted
but used "commit --amend" or "rebase -i").

It is more like switching branches but in this case you are switching to
your own branch immediately after rewinding that same branch.

In other words, as far as the index, the work tree and where HEAD will
point at are concerned, the new mode of reset works exactly like this:

    $ (edit/add)+ to work towards something wonderful
    ... notice that the work does not belong to the current branch
    ... and you would want to switch to another branch while carrying
    ... your local changes.
    $ git checkout another-branch

The only difference being that reset will stay on the same branch and
remove some commits from the tip of it, while checkout will leave the
original branch intact.

It makes sense that it has the same "safety" as "checkout" has when
switching branches; when you have modification in the index for a path,
and the path is different between switched-to commit and the current
commit, the command errors out with "cannot merge" (or a better message).

One drawback is that you can use this new mode of resetting only until you
make a commit that is part of the new "something else that is wonderful"
topic.  After that "git reset" with this new option is not useful for this
workflow, and you would need to stash then rebase -i then unstash.

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

[Christian Couder]

On Thu, Sep 17, 2009 at 7:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
>
>> From: Stephan Beyer <s-beyer@gmx.net>
>>
>> This option is nearly like "--merge" except that it is
>> safer.
>
> Do you still want to have this description after the last round?
>
>> The table below shows what happens when running
>> "git reset --option target" to reset the HEAD to another
>> commit (as a special case "target" could be the same as
>> HEAD) in the cases where "--merge" and "--merge-safe"
>> (abreviated --m-s) behave differently.
>>
>> working index HEAD target         working index HEAD
>>   B      B     A     A   --m-s      B      A     A
>>                          --merge    A      A     A
>>   B      B     A     C   --m-s       (disallowed)
>>                          --merge    C      C     C
>
> I'd like to see at least the following rows filled as well.
>
>    X      U     A     A   --m-s      ???    ???   ???
>                           --merge    ???    ???   ???
>    X      U     B     A   --m-s      ???    ???   ???
>                           --merge    ???    ???   ???
>
>> In this table, A, B and C are some different states of a file.
>
> ... and X is "don't care", and U is "unmerged index".

I will have a look, but it seems to me that --m-s and --merge
behave the same in these cases.

>> The code comes from the sequencer GSoC project:
>>
>> git://repo.or.cz/git/sbeyer.git
>>
>> (at commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)
>>
>> But in the sequencer project the "reset" flag was set in the "struct
>> unpack_trees_options" passed to "unpack_trees()". With this flag the
>> changes in the working tree were discarded if the file was different
>> between HEAD and the reset target.
>
> If you need to have four lines worth of description here, is this still
> Stephan's patch, or would it be more appropriate to say "This is based on
> an earlier GSoC work by Stephan in git://repo.or.cz/git/sbeyer.git
> repository." and you take all the credit and blame?

As you insist, I will take all the credit and blame.

>>  static const char * const git_reset_usage[] = {
>> -     "git reset [--mixed | --soft | --hard | --merge] [-q] [<commit>]",
>> +     "git reset [--mixed | --soft | --hard | --merge | --merge-safe] [-q] [<commit>]",
>>       "git reset [--mixed] <commit> [--] <paths>...",
>>       NULL
>>  };
>
> As we established in the previous round, this is _different_ from --merge,
> but *not* in the sense that --merge is more dangerous and users should be
> using this new option instead, but in the sense that --merge perfectly
> works well for its intended use case, and this new option triggers a mode
> of operation that is meant to be used in a completely different use case,
> which is unspecified in this series without documentation.
>
> In that light, is --merge-safe still a good name for the option, or merely
> a misleading one?

I agree that it could be misleading. What about "--stash" ?

> As I said in the previous round, --merge discards the modified index state
> when switching, and it is absolutely _the right thing to do_ in the use
> case it was intended for.  It is _not_ dangerous, and using --merge-safe
> in that scenario is not being _safe_ but is actively a _wrong_ thing to do.

Ok.

>> @@ -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_SAFE) {
>> +             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;
>> +     }
>
> get_sha1() takes an allocated buffer, does not allocate space on its own.
> I think you meant "unsigned char head_sha1[20];" here.

Yes.

Thanks,
Christian.

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

[Daniel Barkalow]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 914 bytes --]

On Thu, 17 Sep 2009, Christian Couder wrote:

> On Thu, Sep 17, 2009 at 7:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> > I'd like to see at least the following rows filled as well.
> >
> >    X      U     A     A   --m-s      ???    ???   ???
> >                           --merge    ???    ???   ???
> >    X      U     B     A   --m-s      ???    ???   ???
> >                           --merge    ???    ???   ???
> >
> >> In this table, A, B and C are some different states of a file.
> >
> > ... and X is "don't care", and U is "unmerged index".
> 
> I will have a look, but it seems to me that --m-s and --merge
> behave the same in these cases.

It's still worth documenting, since it used to be entirely undocumented 
(and probably not what it should have been), and also because you're 
making it something reliable for the first time in this series.

	-Daniel
*This .sig left intentionally blank*

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

[Christian Couder]

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

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

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

diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
index 6211096..794a506 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-safe'
 
 . ./test-lib.sh
 
@@ -30,6 +30,20 @@ test_expect_success 'reset --merge is ok with changes in file it does not touch'
      grep 4 file2
 '
 
+test_expect_success 'reset --merge-safe is ok with changes in file it does not touch' '
+     git reset --hard HEAD^ &&
+     echo "line 4" >> file1 &&
+     echo "line 4" >> file2 &&
+     test_tick &&
+     git commit -m "add line 4" file1 &&
+     git reset --merge-safe HEAD^ &&
+     ! grep 4 file1 &&
+     grep 4 file2 &&
+     git reset --merge-safe HEAD@{1} &&
+     grep 4 file1 &&
+     grep 4 file2
+'
+
 test_expect_success 'reset --merge discards changes added to index (1)' '
      echo "line 5" >> file1 &&
      git add file1 &&
@@ -55,6 +69,25 @@ test_expect_success 'reset --merge discards changes added to index (2)' '
      grep 4 file1
 '
 
+test_expect_success 'reset --merge-safe fails with changes in index in files it touches' '
+     echo "line 4" >> file2 &&
+     echo "line 5" >> file1 &&
+     git add file1 &&
+     test_must_fail git reset --merge-safe HEAD^ &&
+     git reset --hard HEAD
+'
+
+test_expect_success 'reset --merge-safe keeps changes it does not touch' '
+     echo "line 4" >> file2 &&
+     git add file2 &&
+     git reset --merge-safe HEAD^ &&
+     grep 4 file2 &&
+     git reset --merge-safe HEAD@{1} &&
+     grep 4 file2 &&
+     grep 4 file1 &&
+     git reset --hard HEAD
+'
+
 test_expect_success 'reset --merge fails with changes in file it touches' '
      echo "line 5" >> file1 &&
      test_tick &&
@@ -66,6 +99,17 @@ test_expect_success 'reset --merge fails with changes in file it touches' '
      git reset --hard HEAD^
 '
 
+test_expect_success 'reset --merge-safe fails with changes in file it touches' '
+     echo "line 5" >> file1 &&
+     test_tick &&
+     git commit -m "add line 5" file1 &&
+     sed -e "s/line 1/changed line 1/" <file1 >file3 &&
+     mv file3 file1 &&
+     test_must_fail git reset --merge-safe HEAD^ 2>err.log &&
+     grep file1 err.log | grep "not uptodate" &&
+     git reset --hard HEAD^
+'
+
 test_expect_success 'setup 2 different branches' '
      git branch branch1 &&
      git branch branch2 &&
@@ -87,4 +131,12 @@ test_expect_success 'reset --merge is ok with pending merge' '
      git reset --hard HEAD@{1}
 '
 
+test_expect_success 'reset --merge-safe is ok with pending merge' '
+     test_must_fail git merge branch1 &&
+     git reset --merge-safe HEAD^ &&
+     test -z "$(git diff --cached)" &&
+     test -n "$(git diff)" &&
+     git reset --hard HEAD@{1}
+'
+
 test_done
-- 
1.6.5.rc0.150.g38fe6