threads / rfc / 22080

RFC patch, 5 partsadd "--keep" option to "git reset"

Subject: [RFC/PATCH 0/5] add "--keep" option to "git reset"

## tl;dr

12 messages between Jan 2, 2010 and Jan 19, 2010. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Christian Couder· Jan 2, 2010, 05:39 UTC · lore

This patch series is sent to make it easy to test the changes in the first patch by Junio of this series on a new "--keep" reset option.

Christian Couder (4):
  reset: add option "--keep" to "git reset"
  reset: add test cases for "--keep" option
  Documentation: reset: describe new "--keep" option
  reset: disallow "reset --keep" outside a work tree
Junio C Hamano (1):
  reset: make "reset --merge" discard work tree changes on unmerged
    entries
 Documentation/git-reset.txt |   45 ++++++++++++++--
 builtin-reset.c             |   31 +++++++++--
 read-cache.c                |    2 +-
 t/t7103-reset-bare.sh       |   25 ++++++---
 t/t7110-reset-merge.sh      |  119 ++++++++++++++++++++++++++++++++++++++++---
 unpack-trees.c              |   19 ++++---
 6 files changed, 206 insertions(+), 35 deletions(-)
Christian Couder· Jan 2, 2010, 05:39 UTC · re: Christian Couder · lore

[RFC/PATCH 1/5] reset: make "reset --merge" discard work tree changes on unmerged entries

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

Commit 9e8ecea (Add 'merge' mode to 'git reset', 2008-12-01) disallowed "git reset --merge" when there was unmerged entries. It acknowledged that this is not the best possible behavior, and that it would be better if unmerged entries were reset as if --hard (instead of --merge) has been used.

Recently another commit (reset: use "unpack_trees()" directly instead of "git read-tree", 2009-12-30) changed the behavior of --merge to accept resetting unmerged entries if they are reset to a different state than HEAD, but it did not reset the changes in the work tree. So the behavior was kind of improved, but it was not yet as if --hard has been used.

This patch finally makes "git reset --merge" behaves like "git reset --hard" on unmerged entries.

To do that it does three things:
 - Updates the documentation to match the wish of original "reset
   --merge" better, namely, "An unmerged entry is a sign that the path
   didn't have any local modification and can be safely resetted to
   whatever the new HEAD records";
 - Updates read_index_unmerged(), which reads the index file into the
   cache while dropping any higher-stage entries down to stage #0, not
   to copy the object name recorded in the cache entry.  The code used to
   take the object name from the highest stage entry ("theirs" if you
   happened to have stage #3, or "ours" if they removed while you kept),
   which essentially meant that you are getting random results and
   didn't make sense.
   The _only_ reason we want to keep a previously unmerged entry in the
   index at stage #0 is so that we don't forget the fact that we have
   corresponding file in the work tree in order to be able to remove it
   when the tree we are resetting to does not have the path.  In order
   to differentiate such an entry from ordinary cache entry, the cache
   entry added by read_index_unmerged() records null sha1.
 - Updates merged_entry() and deleted_entry() so that they pay attention
   to cache entries with null sha1 (note that we _might_ want to use a
   new in-core ce->ce_flags instead of using the null-sha1 hack).  They
   are previously unmerged entries, and the files in the work tree that
   correspond to them are resetted away by oneway_merge() to the version
   from the tree we are resetting to.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-reset.txt |    4 ++--
 read-cache.c                |    2 +-
 t/t7110-reset-merge.sh      |   14 +++++++-------
 unpack-trees.c              |   19 ++++++++++++-------
 4 files changed, 22 insertions(+), 17 deletions(-)
Show changes to 4 files +22 −17

Documentation/git-reset.txt, read-cache.c, t/t7110-reset-merge.sh, unpack-trees.c

diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index 2480be5..b78e2e1 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -123,14 +123,14 @@ entries:
        X       U     A    B     --soft  (disallowed)
 				--mixed  X       B     B
 				--hard   B       B     B
-				--merge  X       B     B
+				--merge  B       B     B
 
       working index HEAD target         working index HEAD
       ----------------------------------------------------
        X       U     A    A     --soft  (disallowed)
 				--mixed  X       A     A
 				--hard   A       A     A
-				--merge (disallowed)
+				--merge  A       A     A
 
 X means any state and U means an unmerged index.
 
diff --git a/read-cache.c b/read-cache.c
index 94b92d3..e07a6df 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1635,7 +1635,7 @@ int read_index_unmerged(struct index_state *istate)
 		len = strlen(ce->name);
 		size = cache_entry_size(len);
 		new_ce = xcalloc(1, size);
-		hashcpy(new_ce->sha1, ce->sha1);
+		/* don't copy sha1; this should not match anything */
 		memcpy(new_ce->name, ce->name, len);
 		new_ce->ce_flags = create_ce_flags(len, 0);
 		new_ce->ce_mode = ce->ce_mode;
diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
index ff2875c..249df7c 100755
--- a/t/t7110-reset-merge.sh
+++ b/t/t7110-reset-merge.sh
@@ -135,27 +135,27 @@ test_expect_success 'setup 2 different branches' '
 #
 #           working index HEAD target         working index HEAD
 #           ----------------------------------------------------
-# file1:     X       U     B    C     --merge  X       C     C
+# file1:     X       U     B    C     --merge  C       C     C
 test_expect_success '"reset --merge HEAD^" is ok with pending merge' '
     test_must_fail git merge branch1 &&
-    cat file1 >orig_file1 &&
     git reset --merge HEAD^ &&
     test "$(git rev-parse HEAD)" = "$(git rev-parse second)" &&
     test -z "$(git diff --cached)" &&
-    test_cmp file1 orig_file1
+    test -z "$(git diff)"
 '
 
 # The next test will test the following:
 #
 #           working index HEAD target         working index HEAD
 #           ----------------------------------------------------
-# file1:     X       U     B    B     --merge  (disallowed)
-test_expect_success '"reset --merge HEAD" fails with pending merge' '
+# file1:     X       U     B    B     --merge  B       B     B
+test_expect_success '"reset --merge HEAD" is ok with pending merge' '
     git reset --hard third &&
     test_must_fail git merge branch1 &&
-    test_must_fail git reset --merge HEAD &&
+    git reset --merge HEAD &&
     test "$(git rev-parse HEAD)" = "$(git rev-parse third)" &&
-    test -n "$(git diff --cached)"
+    test -z "$(git diff --cached)" &&
+    test -z "$(git diff)"
 '
 
 test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index 7570475..87f95a1 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -809,7 +809,11 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 {
 	int update = CE_UPDATE;
 
-	if (old) {
+	if (!old) {
+		if (verify_absent(merge, "overwritten", o))
+			return -1;
+		invalidate_ce_path(merge, o);
+	} else if (!is_null_sha1(old->sha1)) {
 		/*
 		 * See if we can re-use the old CE directly?
 		 * That way we get the uptodate stat info.
@@ -827,11 +831,12 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
 				update |= CE_SKIP_WORKTREE;
 			invalidate_ce_path(old, o);
 		}
-	}
-	else {
-		if (verify_absent(merge, "overwritten", o))
-			return -1;
-		invalidate_ce_path(merge, o);
+	} else {
+		/*
+		 * Previously unmerged entry left as an existence
+		 * marker by read_index_unmerged();
+		 */
+		invalidate_ce_path(old, o);
 	}
 
 	add_entry(o, merge, update, CE_STAGEMASK);
@@ -847,7 +852,7 @@ static int deleted_entry(struct cache_entry *ce, struct cache_entry *old,
 			return -1;
 		return 0;
 	}
-	if (verify_uptodate(old, o))
+	if (!is_null_sha1(old->sha1) && verify_uptodate(old, o))
 		return -1;
 	add_entry(o, ce, CE_REMOVE, 0);
 	invalidate_ce_path(ce, o);
-- 
1.6.6.rc2.5.g49666
Junio C Hamano· Jan 2, 2010, 19:46 UTC · re: Christian Couder · lore

Re: [RFC/PATCH 1/5] reset: make "reset --merge" discard work tree changes on unmerged entries

Christian Couder <chriscool@tuxfamily.org> writes:
Show 13 quoted lines
> From: Junio C Hamano <gitster@pobox.com>
>
> Commit 9e8ecea (Add 'merge' mode to 'git reset', 2008-12-01) disallowed
> "git reset --merge" when there was unmerged entries. It acknowledged
> that this is not the best possible behavior, and that it would be better
> if unmerged entries were reset as if --hard (instead of --merge) has
> been used.
>
> Recently another commit (reset: use "unpack_trees()" directly instead of
> "git read-tree", 2009-12-30) changed the behavior of --merge to accept
> resetting unmerged entries if they are reset to a different state than
> HEAD, but it did not reset the changes in the work tree. So the behavior
> was kind of improved, but it was not yet as if --hard has been used.
It would be more honest if we said something like:
	It changed a safer "I can't do as asked, please do it by hand"
	into a more dangerous "I pretend that I did so but I didn't do the
	full job; you need to fix up the result but I am not telling you
	that you have to", which is a lot worse.
here instead (that is one reason why I said my fix-up was "squashable").
But that is a minor issue.
I have been thinking about two issues on this --merge change.  
Show 6 quoted lines
>  - Updates merged_entry() and deleted_entry() so that they pay attention
>    to cache entries with null sha1 (note that we _might_ want to use a
>    new in-core ce->ce_flags instead of using the null-sha1 hack).  They
>    are previously unmerged entries, and the files in the work tree that
>    correspond to them are resetted away by oneway_merge() to the version
>    from the tree we are resetting to.

One is the use of ce_flags instead of relying on the 20-byte comparison I said above, for both performance (minor) and future maintainability (much bigger) concern. I have a feeling that we will regret later that we used the null_sha1 trick here, when we want to express another "special" kind of cache entry in unrelated situations. The use of null_sha1 hack was expedient but I fell victim of the same mentality of declaring that this is the _last_ such kind of special index entry and closing the door to others who want to extend the system later with different kind of special cache entry, which I often complain about myself to patches from other people.

Another is that it _might_ make sense to use two-tree form of read-tree machinery (but using a different version of unpack-trees.c::twoway_merge() function), instead of the one-tree form of "we don't bother checking if the index is consistent with HEAD and assume it is, and jump to the target."

"git reset --merge $there" is about the situation where you started a "mergy" operation (e.g. "git merge", "git am -3", "git rebase", ...) while you had unrelated local changes in the work tree, and you want to go back to the state before that operation $there (which is HEAD if the mergy operation is "merge", but is different from HEAD if it was "am -3" and you have successfully applied a patch or more already). Linus may know and won't use "reset --merge" in a situation where it is not suitable, but not everybody is Linus. Even though "reset --merge" may "correctly" work with respect to the table you added to "git reset" documentation, it would do something that may not "make sense" from the end-user's point of view when used in a situation that it wasn't designed for. Using the two-tree form allows the machinery to inspect the difference between HEAD and the index, and detect cases where "reset --merge" was attempted when it shouldn't. For example, if stage #2 of an unmerged path does not match HEAD, we know there is something wrong.

This latter issue is much bigger and needs a lot more thought, and I don't think it should block the series from going forward at all. But I think it is worth keeping in the back of our heads.

Christian Couder· Jan 2, 2010, 05:39 UTC · re: Christian Couder · lore

[RFC/PATCH 2/5] reset: add option "--keep" to "git reset"

The purpose of this new option is to discard some of the last commits but to keep current changes in the work tree.

The use case is when you work on something and commit that work. And then you work on something else that touches other files, but you don't commit it yet. Then you realize that what you commited when you worked on the first thing is not good or belongs to another branch.

So you want to get rid of the previous commits (at least in the current branch) but you want to make sure that you keep the changes you have in the work tree. And you are pretty sure that your changes are independent from what you previously commited, so you don't want the reset to succeed if the previous commits changed a file that you also changed in your work tree.

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

working index HEAD target         working index HEAD
----------------------------------------------------
  A      B     C     D   --keep    (disallowed)
                         --merge   (disallowed)
  A      B     C     C   --keep     A      C     C
                         --merge   (disallowed)
  B      B     C     D   --keep    (disallowed)
                         --merge    D      D     D
  B      B     C     C   --keep     B      C     C
                         --merge    C      C     C

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

The following table shows what happens on unmerged entries:
working index HEAD target         working index HEAD
----------------------------------------------------
 X       U     A    B     --keep  (disallowed)
                          --merge  B       B     B
 X       U     A    A     --keep   X       A     A
                          --merge  A       A     A

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

Though the error message when "reset --keep" is disallowed on unmerged entries is something like:

error: Entry 'file1' would be overwritten by merge. Cannot merge. fatal: Could not reset index file to revision 'HEAD^'.

which is not very nice.

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

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

The code comes from the sequencer GSoC project, where such an option was developed by Stephan Beyer:

git://repo.or.cz/git/sbeyer.git
(at commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)

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

Mentored-by: Daniel Barkalow <barkalow@iabervon.org>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin-reset.c |   29 ++++++++++++++++++++++++-----
 1 files changed, 24 insertions(+), 5 deletions(-)
Show changes to builtin-reset.c +24 −5
diff --git a/builtin-reset.c b/builtin-reset.c
index 0f5022e..da61f20 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -22,13 +22,15 @@
 #include "cache-tree.h"
 
 static const char * const git_reset_usage[] = {
-	"git reset [--mixed | --soft | --hard | --merge] [-q] [<commit>]",
+	"git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]",
 	"git reset [--mixed] <commit> [--] <paths>...",
 	NULL
 };
 
-enum reset_type { MIXED, SOFT, HARD, MERGE, NONE };
-static const char *reset_type_names[] = { "mixed", "soft", "hard", "merge", NULL };
+enum reset_type { MIXED, SOFT, HARD, MERGE, KEEP, NONE };
+static const char *reset_type_names[] = {
+	"mixed", "soft", "hard", "merge", "keep", NULL
+};
 
 static char *args_to_str(const char **argv)
 {
@@ -71,6 +73,7 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
 	if (!quiet)
 		opts.verbose_update = 1;
 	switch (reset_type) {
+	case KEEP:
 	case MERGE:
 		opts.update = 1;
 		break;
@@ -85,6 +88,16 @@ static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet
 
 	read_cache_unmerged();
 
+	if (reset_type == KEEP) {
+		unsigned char head_sha1[20];
+		if (get_sha1("HEAD", head_sha1))
+			return error("You do not have a valid HEAD.");
+		if (!fill_tree_descriptor(desc, head_sha1))
+			return error("Failed to find tree of HEAD.");
+		nr++;
+		opts.fn = twoway_merge;
+	}
+
 	if (!fill_tree_descriptor(desc + nr - 1, sha1))
 		return error("Failed to find tree of %s.", sha1_to_hex(sha1));
 	if (unpack_trees(nr, desc, &opts))
@@ -229,6 +242,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 				"reset HEAD, index and working tree", HARD),
 		OPT_SET_INT(0, "merge", &reset_type,
 				"reset HEAD, index and working tree", MERGE),
+		OPT_SET_INT(0, "keep", &reset_type,
+				"reset HEAD but keep local changes", KEEP),
 		OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
 		OPT_END()
 	};
@@ -317,9 +332,13 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (reset_type == SOFT) {
 		if (is_merge() || read_cache() < 0 || unmerged_cache())
 			die("Cannot do a soft reset in the middle of a merge.");
+	} else {
+		int err = reset_index_file(sha1, reset_type, quiet);
+		if (reset_type == KEEP)
+			err = err || reset_index_file(sha1, MIXED, quiet);
+		if (err)
+			die("Could not reset index file to revision '%s'.", rev);
 	}
-	else if (reset_index_file(sha1, reset_type, quiet))
-		die("Could not reset index file to revision '%s'.", rev);
 
 	/* Any resets update HEAD to the head being switched to,
 	 * saving the previous head in ORIG_HEAD before. */
-- 
1.6.6.rc2.5.g49666
Junio C Hamano· Jan 2, 2010, 19:50 UTC · re: Christian Couder · lore

Re: [RFC/PATCH 2/5] reset: add option "--keep" to "git reset"

Christian Couder <chriscool@tuxfamily.org> writes:
Show 19 quoted lines
> The purpose of this new option is to discard some of the last commits
> but to keep current changes in the work tree.
>
> The use case is when you work on something and commit that work. And
> then you work on something else that touches other files, but you don't
> commit it yet. Then you realize that what you commited when you worked
> on the first thing is not good or belongs to another branch.
>
> So you want to get rid of the previous commits (at least in the current
> branch) but you want to make sure that you keep the changes you have in
> the work tree. And you are pretty sure that your changes are independent
> from what you previously commited, so you don't want the reset to
> succeed if the previous commits changed a file that you also changed in
> your work tree.
>
> The table below shows what happens when running "git reset --option
> target" to reset the HEAD to another commit (as a special case "target"
> could be the same as HEAD) in the cases where "--merge" and "--keep"
> behave differently.

I think this new option is unrelated to "--merge"; iow, the only relation to it is that it is an option to the same command "git reset", so it is related but it is related the same way and to the degree as "--mixed" is.

Thinking about it even more, if the number of commits you are resetting away is zero in your use case (i.e. target is HEAD), shouldn't this new mode of operation degenerate to "--mixed"? So in that sense, it might make sense to contrast it with "--mixed".

But let's try not to contrast it with anything else, and see how well it stands on its own. The below is my attempt.

Show 6 quoted lines
> working index HEAD target         working index HEAD
> ----------------------------------------------------
>   A      B     C     D   --keep    (disallowed)
>   A      B     C     C   --keep     A      C     C
>   B      B     C     D   --keep    (disallowed)
>   B      B     C     C   --keep     B      C     C

Let's give an explanation of the above in terms of what this means to the end users.

    When you have changes to a path that the accumulated commits between
    target..HEAD touch, you don't want to discard them.  It doesn't matter
    if the changes in the work tree has been partially added (A != B) or
    fully added (A == B) to the index.  In both cases, the operation is
    disallowed, just like "checkout $another_branch" stops in such a case.
    But if you have local modifications based on one version and dropping
    the accumulated commits between target..HEAD does not involve that
    path, we can safely "transplant" that change to the target.

Presented this way, a future direction (iow, I am not suggesting you to do this before the current series solidifies) might be to allow users to do something similar to "checkout -m $the_other_branch". IOW, instead of disallowing "I have changed from C to B and switching to D" case, perform a three-way merge to bring the work tree file to (D+(B-C)), transplanting the local change you made on top of the target.

Show 6 quoted lines
> The following table shows what happens on unmerged entries:
>
> working index HEAD target         working index HEAD
> ----------------------------------------------------
>  X       U     A    B     --keep  (disallowed)
>  X       U     A    A     --keep   X       A     A

In a sense, this is consistent with the above; the local change attempted happens to be an unmerged result.

But it is inconsistent with the intended use case you presented, which leaves no room for unmerged entries to enter in the index to begin with. It might be safer to error out on any unmerged entry in the index. I dunno.

Christian Couder· Jan 19, 2010, 04:28 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH 2/5] reset: add option "--keep" to "git reset"

On samedi 02 janvier 2010, Junio C Hamano wrote:
Show 32 quoted lines
> Christian Couder <chriscool@tuxfamily.org> writes:
> > The purpose of this new option is to discard some of the last commits
> > but to keep current changes in the work tree.
> >
> > The use case is when you work on something and commit that work. And
> > then you work on something else that touches other files, but you don't
> > commit it yet. Then you realize that what you commited when you worked
> > on the first thing is not good or belongs to another branch.
> >
> > So you want to get rid of the previous commits (at least in the current
> > branch) but you want to make sure that you keep the changes you have in
> > the work tree. And you are pretty sure that your changes are
> > independent from what you previously commited, so you don't want the
> > reset to succeed if the previous commits changed a file that you also
> > changed in your work tree.
> >
> > The table below shows what happens when running "git reset --option
> > target" to reset the HEAD to another commit (as a special case "target"
> > could be the same as HEAD) in the cases where "--merge" and "--keep"
> > behave differently.
>
> I think this new option is unrelated to "--merge"; iow, the only relation
> to it is that it is an option to the same command "git reset", so it is
> related but it is related the same way and to the degree as "--mixed" is.
>
> Thinking about it even more, if the number of commits you are resetting
> away is zero in your use case (i.e. target is HEAD), shouldn't this new
> mode of operation degenerate to "--mixed"?  So in that sense, it might
> make sense to contrast it with "--mixed".
>
> But let's try not to contrast it with anything else, and see how well it
> stands on its own. 

Ok, I removed parts of the commit messages that contrasted it with "--merge".

[...]
Show 14 quoted lines
> > The following table shows what happens on unmerged entries:
> >
> > working index HEAD target         working index HEAD
> > ----------------------------------------------------
> >  X       U     A    B     --keep  (disallowed)
> >  X       U     A    A     --keep   X       A     A
>
> In a sense, this is consistent with the above; the local change attempted
> happens to be an unmerged result.
>
> But it is inconsistent with the intended use case you presented, which
> leaves no room for unmerged entries to enter in the index to begin with.
> It might be safer to error out on any unmerged entry in the index.  I
> dunno.

Yeah I agree it might be safer, so I added a patch to disallow using --keep when there are unmerged entries.

Thanks, Christian.

Christian Couder· Jan 2, 2010, 05:39 UTC · re: Christian Couder · lore

[RFC/PATCH 3/5] reset: add test cases for "--keep" option

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

In the case of unmerged entries, we can see that "git reset --merge" resets everything to the target, while with "--keep" it works only when the target state is the same as HEAD and the work tree is not reset.

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t7110-reset-merge.sh |  105 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 104 insertions(+), 1 deletions(-)
Show changes to t/t7110-reset-merge.sh +104 −1
diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh
index 249df7c..a9b2fba 100755
--- a/t/t7110-reset-merge.sh
+++ b/t/t7110-reset-merge.sh
@@ -3,7 +3,7 @@
 # Copyright (c) 2009 Christian Couder
 #
 
-test_description='Tests for "git reset --merge"'
+test_description='Tests for "git reset" with "--merge" and "--keep" options'
 
 . ./test-lib.sh
 
@@ -47,6 +47,30 @@ test_expect_success 'reset --merge is ok when switching back' '
 #
 #           working index HEAD target         working index HEAD
 #           ----------------------------------------------------
+# file1:     C       C     C    D     --keep   D       D     D
+# file2:     C       D     D    D     --keep   C       D     D
+test_expect_success 'reset --keep is ok with changes in file it does not touch' '
+    git reset --hard second &&
+    cat file1 >file2 &&
+    git reset --keep HEAD^ &&
+    ! grep 4 file1 &&
+    grep 4 file2 &&
+    test "$(git rev-parse HEAD)" = "$(git rev-parse initial)" &&
+    test -z "$(git diff --cached)"
+'
+
+test_expect_success 'reset --keep is ok when switching back' '
+    git reset --keep second &&
+    grep 4 file1 &&
+    grep 4 file2 &&
+    test "$(git rev-parse HEAD)" = "$(git rev-parse second)" &&
+    test -z "$(git diff --cached)"
+'
+
+# The next test will test the following:
+#
+#           working index HEAD target         working index HEAD
+#           ----------------------------------------------------
 # file1:     B       B     C    D     --merge  D       D     D
 # file2:     C       D     D    D     --merge  C       D     D
 test_expect_success 'reset --merge discards changes added to index (1)' '
@@ -78,6 +102,18 @@ test_expect_success 'reset --merge is ok again when switching back (1)' '
 #
 #           working index HEAD target         working index HEAD
 #           ----------------------------------------------------
+# file1:     B       B     C    D     --keep   (disallowed)
+test_expect_success 'reset --keep fails with changes in index in files it touches' '
+    git reset --hard second &&
+    echo "line 5" >> file1 &&
+    git add file1 &&
+    test_must_fail git reset --keep HEAD^
+'
+
+# The next test will test the following:
+#
+#           working index HEAD target         working index HEAD
+#           ----------------------------------------------------
 # file1:     C       C     C    D     --merge  D       D     D
 # file2:     C       C     D    D     --merge  D       D     D
 test_expect_success 'reset --merge discards changes added to index (2)' '
@@ -104,6 +140,30 @@ test_expect_success 'reset --merge is ok again when switching back (2)' '
 #
 #           working index HEAD target         working index HEAD
 #           ----------------------------------------------------
+# file1:     C       C     C    D     --keep   D       D     D
+# file2:     C       C     D    D     --keep   C       D     D
+test_expect_success 'reset --keep keeps changes it does not touch' '
+    git reset --hard second &&
+    echo "line 4" >> file2 &&
+    git add file2 &&
+    git reset --keep HEAD^ &&
+    grep 4 file2 &&
+    test "$(git rev-parse HEAD)" = "$(git rev-parse initial)" &&
+    test -z "$(git diff --cached)"
+'
+
+test_expect_success 'reset --keep keeps changes when switching back' '
+    git reset --keep second &&
+    grep 4 file2 &&
+    grep 4 file1 &&
+    test "$(git rev-parse HEAD)" = "$(git rev-parse second)" &&
+    test -z "$(git diff --cached)"
+'
+
+# The next test will test the following:
+#
+#           working index HEAD target         working index HEAD
+#           ----------------------------------------------------
 # file1:     A       B     B    C     --merge  (disallowed)
 test_expect_success 'reset --merge fails with changes in file it touches' '
     git reset --hard second &&
@@ -116,6 +176,22 @@ test_expect_success 'reset --merge fails with changes in file it touches' '
     grep file1 err.log | grep "not uptodate"
 '
 
+# The next test will test the following:
+#
+#           working index HEAD target         working index HEAD
+#           ----------------------------------------------------
+# file1:     A       B     B    C     --keep   (disallowed)
+test_expect_success 'reset --keep fails with changes in file it touches' '
+    git reset --hard second &&
+    echo "line 5" >> file1 &&
+    test_tick &&
+    git commit -m "add line 5" file1 &&
+    sed -e "s/line 1/changed line 1/" <file1 >file3 &&
+    mv file3 file1 &&
+    test_must_fail git reset --keep HEAD^ 2>err.log &&
+    grep file1 err.log | grep "not uptodate"
+'
+
 test_expect_success 'setup 2 different branches' '
     git reset --hard second &&
     git branch branch1 &&
@@ -148,6 +224,18 @@ test_expect_success '"reset --merge HEAD^" is ok with pending merge' '
 #
 #           working index HEAD target         working index HEAD
 #           ----------------------------------------------------
+# file1:     X       U     B    C     --keep   (disallowed)
+test_expect_success '"reset --keep HEAD^" fails with pending merge' '
+    git reset --hard third &&
+    test_must_fail git merge branch1 &&
+    test_must_fail git reset --keep HEAD^ 2>err.log &&
+    grep file1 err.log | grep "overwritten by merge"
+'
+
+# The next test will test the following:
+#
+#           working index HEAD target         working index HEAD
+#           ----------------------------------------------------
 # file1:     X       U     B    B     --merge  B       B     B
 test_expect_success '"reset --merge HEAD" is ok with pending merge' '
     git reset --hard third &&
@@ -158,4 +246,19 @@ test_expect_success '"reset --merge HEAD" is ok with pending merge' '
     test -z "$(git diff)"
 '
 
+# The next test will test the following:
+#
+#           working index HEAD target         working index HEAD
+#           ----------------------------------------------------
+# file1:     X       U     B    B     --keep  X       B     B
+test_expect_success '"reset --keep HEAD" is ok with pending merge' '
+    git reset --hard third &&
+    test_must_fail git merge branch1 &&
+    cat file1 >orig_file1 &&
+    git reset --keep HEAD &&
+    test "$(git rev-parse HEAD)" = "$(git rev-parse third)" &&
+    test -z "$(git diff --cached)" &&
+    test_cmp file1 orig_file1
+'
+
 test_done
-- 
1.6.6.rc2.5.g49666
Christian Couder· Jan 2, 2010, 05:39 UTC · re: Christian Couder · lore

[RFC/PATCH 4/5] Documentation: reset: describe new "--keep" option

and give an example to show how it can be used.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-reset.txt |   41 ++++++++++++++++++++++++++++++++++++++---
 1 files changed, 38 insertions(+), 3 deletions(-)
Show changes to Documentation/git-reset.txt +38 −2
diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt
index b78e2e1..3d7c206 100644
--- a/Documentation/git-reset.txt
+++ b/Documentation/git-reset.txt
@@ -8,7 +8,7 @@ git-reset - Reset current HEAD to the specified state
 SYNOPSIS
 --------
 [verse]
-'git reset' [--mixed | --soft | --hard | --merge] [-q] [<commit>]
+'git reset' [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]
 'git reset' [-q] [<commit>] [--] <paths>...
 'git reset' --patch [<commit>] [--] [<paths>...]
 
@@ -52,6 +52,11 @@ OPTIONS
 	and updates the files that are different between the named commit
 	and the current commit in the working tree.
 
+--keep::
+	Resets the index to match the tree recorded by the named commit,
+	but keep changes in the working tree. Aborts if the reset would
+	change files that are already changes in the working tree.
+
 -p::
 --patch::
 	Interactively select hunks in the difference between the index
@@ -86,6 +91,7 @@ reset options depending on the state of the files.
 				--mixed  A       D     D
 				--hard   D       D     D
 				--merge (disallowed)
+				--keep  (disallowed)
 
       working index HEAD target         working index HEAD
       ----------------------------------------------------
@@ -93,6 +99,7 @@ reset options depending on the state of the files.
 				--mixed  A       C     C
 				--hard   C       C     C
 				--merge (disallowed)
+				--keep   A       C     C
 
       working index HEAD target         working index HEAD
       ----------------------------------------------------
@@ -100,6 +107,7 @@ reset options depending on the state of the files.
 				--mixed  B       D     D
 				--hard   D       D     D
 				--merge  D       D     D
+				--keep  (disallowed)
 
       working index HEAD target         working index HEAD
       ----------------------------------------------------
@@ -107,13 +115,14 @@ reset options depending on the state of the files.
 				--mixed  B       C     C
 				--hard   C       C     C
 				--merge  C       C     C
+				--keep   B       C     C
 
 In these tables, A, B, C and D are some different states of a
 file. For example, the last line of the last table means that if a
 file is in state B in the working tree and the index, and in a
 different state C in HEAD and in the target, then "git reset
---merge target" will put the file in state C in the working tree,
-in the index and in HEAD.
+--keep target" will put the file in state B in the working tree,
+and in state C in the index and in HEAD.
 
 The following tables show what happens when there are unmerged
 entries:
@@ -124,6 +133,7 @@ entries:
 				--mixed  X       B     B
 				--hard   B       B     B
 				--merge  B       B     B
+				--keep  (disallowed)
 
       working index HEAD target         working index HEAD
       ----------------------------------------------------
@@ -131,6 +141,7 @@ entries:
 				--mixed  X       A     A
 				--hard   A       A     A
 				--merge  A       A     A
+				--keep   X       A     A
 
 X means any state and U means an unmerged index.
 
@@ -302,6 +313,30 @@ $ git add frotz.c                           <3>
 <2> This commits all other changes in the index.
 <3> Adds the file to the index again.
 
+Keep changes in working tree while discarding some previous commits::
++
+Suppose you are working on something and you commit it, and then you
+continue working a bit more, but now you think that what you have in
+your working tree should be in another branch that has nothing to do
+with what you commited previously. You can start a new branch and
+reset it while keeping the changes in your work tree.
++
+------------
+$ git tag start
+$ git branch branch1
+$ edit
+$ git commit ...                            <1>
+$ edit
+$ git branch branch2                        <2>
+$ git reset --keep start                    <3>
+------------
++
+<1> This commits your first edits in branch1.
+<2> This creates branch2, but unfortunately it contains the previous
+commit that you don't want in this branch.
+<3> This removes the unwanted previous commit, but this keeps the
+changes in your working tree.
+
 Author
 ------
 Written by Junio C Hamano <gitster@pobox.com> and Linus Torvalds <torvalds@osdl.org>
-- 
1.6.6.rc2.5.g49666
Andreas Schwab· Jan 2, 2010, 09:06 UTC · re: Christian Couder · lore

Re: [RFC/PATCH 4/5] Documentation: reset: describe new "--keep" option

Christian Couder <chriscool@tuxfamily.org> writes:
> +--keep::
> +	Resets the index to match the tree recorded by the named commit,
> +	but keep changes in the working tree. Aborts if the reset would
> +	change files that are already changes in the working tree.
s/changes/changed/ in the last sentence.
Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Daniel Convissor· Jan 2, 2010, 17:14 UTC · re: Christian Couder · lore

Re: [RFC/PATCH 4/5] Documentation: reset: describe new "--keep" option

On Sat, Jan 02, 2010 at 06:39:32AM +0100, Christian Couder wrote:
> +++ b/Documentation/git-reset.txt
...
> +--keep::
> +	Resets the index to match the tree recorded by the named commit,
> +	but keep changes in the working tree. Aborts if the reset would
> +	change files that are already changes in the working tree.

Grammar suggestion for the last line there. Change "already changes" to "already changed". Or better yet, use a different word, such as "already modified", since "change" based words have been used so many times already in the sentence.

Thanks,
--Dan
-- 
 T H E   A N A L Y S I S   A N D   S O L U T I O N S   C O M P A N Y
            data intensive web and database programming
                http://www.AnalysisAndSolutions.com/
 4015 7th Ave #4, Brooklyn NY 11232  v: 718-854-0335 f: 718-854-0409
Christian Couder· Jan 19, 2010, 04:28 UTC · re: Daniel Convissor · lore

Re: [RFC/PATCH 4/5] Documentation: reset: describe new "--keep" option

On samedi 02 janvier 2010, Daniel Convissor wrote:
Show 14 quoted lines
> On Sat, Jan 02, 2010 at 06:39:32AM +0100, Christian Couder wrote:
> > +++ b/Documentation/git-reset.txt
>
> ...
>
> > +--keep::
> > +	Resets the index to match the tree recorded by the named commit,
> > +	but keep changes in the working tree. Aborts if the reset would
> > +	change files that are already changes in the working tree.
>
> Grammar suggestion for the last line there.  Change "already changes" to
> "already changed".  Or better yet, use a different word, such as "already
> modified", since "change" based words have been used so many times
> already in the sentence.
Ok I used "already modified".

Thanks, Christian.

Christian Couder· Jan 2, 2010, 05:39 UTC · re: Christian Couder · lore

[RFC/PATCH 5/5] reset: disallow "reset --keep" outside a work tree

It is safer and consistent with "--merge" and "--hard" resets to disallow "git reset --keep" outside a work tree.

So let's just do that and add some tests while at it.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin-reset.c       |    2 +-
 t/t7103-reset-bare.sh |   25 +++++++++++++++++--------
 2 files changed, 18 insertions(+), 9 deletions(-)
Show changes to 2 files +18 −9

builtin-reset.c, t/t7103-reset-bare.sh

diff --git a/builtin-reset.c b/builtin-reset.c
index da61f20..52584af 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -319,7 +319,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (reset_type == NONE)
 		reset_type = MIXED; /* by default */
 
-	if (reset_type == HARD || reset_type == MERGE)
+	if (reset_type != SOFT && reset_type != MIXED)
 		setup_work_tree();
 
 	if (reset_type == MIXED && is_bare_repository())
diff --git a/t/t7103-reset-bare.sh b/t/t7103-reset-bare.sh
index afb55b3..1eef93c 100755
--- a/t/t7103-reset-bare.sh
+++ b/t/t7103-reset-bare.sh
@@ -11,21 +11,26 @@ test_expect_success 'setup non-bare' '
 	git commit -a -m two
 '
 
-test_expect_success 'hard reset requires a worktree' '
+test_expect_success '"hard" reset requires a worktree' '
 	(cd .git &&
 	 test_must_fail git reset --hard)
 '
 
-test_expect_success 'merge reset requires a worktree' '
+test_expect_success '"merge" reset requires a worktree' '
 	(cd .git &&
 	 test_must_fail git reset --merge)
 '
 
-test_expect_success 'mixed reset is ok' '
+test_expect_success '"keep" reset requires a worktree' '
+	(cd .git &&
+	 test_must_fail git reset --keep)
+'
+
+test_expect_success '"mixed" reset is ok' '
 	(cd .git && git reset)
 '
 
-test_expect_success 'soft reset is ok' '
+test_expect_success '"soft" reset is ok' '
 	(cd .git && git reset --soft)
 '
 
@@ -40,19 +45,23 @@ test_expect_success 'setup bare' '
 	cd bare.git
 '
 
-test_expect_success 'hard reset is not allowed in bare' '
+test_expect_success '"hard" reset is not allowed in bare' '
 	test_must_fail git reset --hard HEAD^
 '
 
-test_expect_success 'merge reset is not allowed in bare' '
+test_expect_success '"merge" reset is not allowed in bare' '
 	test_must_fail git reset --merge HEAD^
 '
 
-test_expect_success 'mixed reset is not allowed in bare' '
+test_expect_success '"keep" reset is not allowed in bare' '
+	test_must_fail git reset --keep HEAD^
+'
+
+test_expect_success '"mixed" reset is not allowed in bare' '
 	test_must_fail git reset --mixed HEAD^
 '
 
-test_expect_success 'soft reset is allowed in bare' '
+test_expect_success '"soft" reset is allowed in bare' '
 	git reset --soft HEAD^ &&
 	test "`git show --pretty=format:%s | head -n 1`" = "one"
 '
-- 
1.6.6.rc2.5.g49666

← back to recent threads