Re* Bug report: reset -p HEAD

Maarten de Vries [off-list ref] writes:

Some more info: It used to work as intended. Using a bisect shows it
has been broken by commit 166ec2e9.

Thanks.

A knee-jerk change without thinking what side-effect it has for you
to try out.

 builtin/reset.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index f2f9d55..a3088d9 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -304,7 +304,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (patch_mode) {
 		if (reset_type != NONE)
 			die(_("--patch is incompatible with --{hard,mixed,soft}"));
-		return run_add_interactive(sha1_to_hex(sha1), "--patch=reset", &pathspec);
+		return run_add_interactive(
+			(unborn || strcmp(rev, "HEAD"))
+			? sha1_to_hex(sha1)
+			: "HEAD", "--patch=reset", &pathspec);
 	}
 
 	/* git reset tree [--] paths... can be used to

On Thu, Oct 24, 2013 at 08:40:13PM -0700, Junio C Hamano wrote:

I think that's the correct fix for the regression.  You are restoring
the original, pre-166ec2e9 behavior for just the HEAD case. I do not
think add--interactive does any other magic between a symbolic rev and
its sha1, except for recognizing HEAD specially. However, if you wanted
to minimize the potential impact of 166ec2e9, you could pass the sha1
_only_ in the unborn case, like this:

diff --git a/builtin/reset.c b/builtin/reset.c
index f2f9d55..bfdd8d3 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -283,6 +283,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (unborn) {
 		/* reset on unborn branch: treat as reset to empty tree */
 		hashcpy(sha1, EMPTY_TREE_SHA1_BIN);
+		rev = EMPTY_TREE_SHA1_HEX;
 	} else if (!pathspec.nr) {
 		struct commit *commit;
 		if (get_sha1_committish(rev, sha1))

@@ -304,7 +305,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (patch_mode) {
 		if (reset_type != NONE)
 			die(_("--patch is incompatible with --{hard,mixed,soft}"));
-		return run_add_interactive(sha1_to_hex(sha1), "--patch=reset", &pathspec);
+		return run_add_interactive(rev, "--patch=reset", &pathspec);
 	}
 
 	/* git reset tree [--] paths... can be used to

That fixes any possible regression from add--interactive treating the
two cases differently. On an unborn branch, we will still say "apply
this hunk" rather than "unstage this hunk". That's not a regression,
because it simply didn't work before, but it's not ideal. To fix that,
we need to somehow tell add--interactive "this is HEAD, but use the
empty tree because it's unborn". I can think of a few simple-ish ways:

  1. Pass the head/not-head flag as a separate option.

  2. Pass HEAD even in the unborn case; teach add--interactive to
     convert an unborn HEAD to the empty tree.

  3. Teach add--interactive to recognize the empty tree sha1 as an
     "unstage" path.

I kind of like (3). At first glance, it is wrong; we will also treat
"git reset -p $(git hash-object -t tree /dev/null)" as if "HEAD" had
been passed. But if you are explicitly passing the empty tree like that,
I think saying "unstage" makes a lot of sense.

-Peff

Sorry about the regression and thanks for report and fixes.

On Thu, Oct 24, 2013 at 9:24 PM, Jeff King [off-list ref] wrote:

On Thu, Oct 24, 2013 at 08:40:13PM -0700, Junio C Hamano wrote:

quoted

Maarten de Vries [off-list ref] writes:

quoted

Some more info: It used to work as intended. Using a bisect shows it
has been broken by commit 166ec2e9.

Thanks.

A knee-jerk change without thinking what side-effect it has for you
to try out.

 builtin/reset.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index f2f9d55..a3088d9 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -304,7 +304,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
      if (patch_mode) {
              if (reset_type != NONE)
                      die(_("--patch is incompatible with --{hard,mixed,soft}"));
-             return run_add_interactive(sha1_to_hex(sha1), "--patch=reset", &pathspec);
+             return run_add_interactive(
+                     (unborn || strcmp(rev, "HEAD"))
+                     ? sha1_to_hex(sha1)
+                     : "HEAD", "--patch=reset", &pathspec);
      }

I think that's the correct fix for the regression.  You are restoring
the original, pre-166ec2e9 behavior for just the HEAD case. I do not
think add--interactive does any other magic between a symbolic rev and
its sha1, except for recognizing HEAD specially. However, if you wanted
to minimize the potential impact of 166ec2e9, you could pass the sha1
_only_ in the unborn case, like this:

Plus, the end result is more readable, IMHO.

Makes sense to me. I'm sure others can implement that much faster than
I can, but I feel a little guilty, so I'm happy to do it if no one
else wants to, as long as we agree this is the way we want to go.

On Thu, Oct 24, 2013 at 10:42:52PM -0700, Martin von Zweigbergk wrote:

quoted

I think that's the correct fix for the regression.  You are restoring
the original, pre-166ec2e9 behavior for just the HEAD case. I do not
think add--interactive does any other magic between a symbolic rev and
its sha1, except for recognizing HEAD specially. However, if you wanted
to minimize the potential impact of 166ec2e9, you could pass the sha1
_only_ in the unborn case, like this:

Plus, the end result is more readable, IMHO.

Agreed. Unfortunately it is slightly wrong, because for the non-patch
cases, we may look at "rev" later, and we would want it to still say
"HEAD" rather than a sha1. This is fixed in my patches below.

quoted

  1. Pass the head/not-head flag as a separate option.

  2. Pass HEAD even in the unborn case; teach add--interactive to
     convert an unborn HEAD to the empty tree.

  3. Teach add--interactive to recognize the empty tree sha1 as an
     "unstage" path.

[...]
Makes sense to me. I'm sure others can implement that much faster than
I can, but I feel a little guilty, so I'm happy to do it if no one
else wants to, as long as we agree this is the way we want to go.

As it turns out, add--interactive already _does_ know how to handle an
unborn HEAD. It just didn't use it for this particular code path. So I
think doing (2) makes the most sense, and the result is that the patch
in reset.c ends up nice and simple.

  [1/2]: add-interactive: handle unborn branch in patch mode
  [2/2]: reset: pass real rev name to add--interactive

-Peff

The list_modified function already knows how to handle an
unborn branch by diffing against the empty tree. However,
the diff we perform to get the actual hunks does not. Let's
use the same logic for both diffs.

Signed-off-by: Jeff King <redacted>
---
 git-add--interactive.perl | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 5156384..24bb1ab 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl

@@ -263,6 +263,17 @@ sub get_empty_tree {
 	return '4b825dc642cb6eb9a060e54bf8d69288fbee4904';
 }
 
+sub get_diff_reference {
+	my $ref = shift;
+	if (defined $ref and $ref ne 'HEAD') {
+		return $ref;
+	} elsif (is_initial_commit()) {
+		return get_empty_tree();
+	} else {
+		return 'HEAD';
+	}
+}
+
 # Returns list of hashes, contents of each of which are:
 # VALUE:	pathname
 # BINARY:	is a binary path

@@ -286,14 +297,7 @@ sub list_modified {
 		return if (!@tracked);
 	}
 
-	my $reference;
-	if (defined $patch_mode_revision and $patch_mode_revision ne 'HEAD') {
-		$reference = $patch_mode_revision;
-	} elsif (is_initial_commit()) {
-		$reference = get_empty_tree();
-	} else {
-		$reference = 'HEAD';
-	}
+	my $reference = get_diff_reference($patch_mode_revision);
 	for (run_cmd_pipe(qw(git diff-index --cached
 			     --numstat --summary), $reference,
 			     '--', @tracked)) {

@@ -737,7 +741,7 @@ sub parse_diff {
 		splice @diff_cmd, 1, 0, "--diff-algorithm=${diff_algorithm}";
 	}
 	if (defined $patch_mode_revision) {
-		push @diff_cmd, $patch_mode_revision;
+		push @diff_cmd, get_diff_reference($patch_mode_revision);
 	}
 	my @diff = run_cmd_pipe("git", @diff_cmd, "--", $path);
 	my @colored = ();

-- 
1.8.4.1.898.g8bf8a41.dirty

The add--interactive --patch mode adjusts the UI based on
whether we are pulling changes from HEAD or elsewhere (in
the former case it asks to unstage the reverse hunk, rather
than apply the forward hunk).

Commit 166ec2e taught reset to work on an unborn branch, but
in doing so, switched to always providing add--interactive
with the sha1 rather than the symbolic name. This meant we
always used the "apply" interface, even for "git reset -p
HEAD".

We can fix this by passing the symbolic name to
add--interactive.  Since it understands unborn branches
these days, we do not even have to cover this special case
ourselves; we can simply pass HEAD.

The tests in t7105 now check that the right interface is
used in each circumstance (and notice the regression from
166ec2e we are fixing). The test in t7106 checks that we
get this right for the unborn case, too (not a regression,
since it didn't work at all before, but a nice improvement).

Signed-off-by: Jeff King <redacted>
---
 builtin/reset.c                |  2 +-
 t/t7105-reset-patch.sh         | 10 ++++++----
 t/t7106-reset-unborn-branch.sh |  5 +++--
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index f2f9d55..6004803 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -304,7 +304,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (patch_mode) {
 		if (reset_type != NONE)
 			die(_("--patch is incompatible with --{hard,mixed,soft}"));
-		return run_add_interactive(sha1_to_hex(sha1), "--patch=reset", &pathspec);
+		return run_add_interactive(rev, "--patch=reset", &pathspec);
 	}
 
 	/* git reset tree [--] paths... can be used to

diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
index 95fab20..98b7d7b 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh

@@ -25,15 +25,17 @@ test_expect_success PERL 'saying "n" does nothing' '
 '
 
 test_expect_success PERL 'git reset -p' '
-	(echo n; echo y) | git reset -p &&
+	(echo n; echo y) | git reset -p >output &&
 	verify_state dir/foo work head &&
-	verify_saved_state bar
+	verify_saved_state bar &&
+	test_i18ngrep "Unstage" output
 '
 
 test_expect_success PERL 'git reset -p HEAD^' '
-	(echo n; echo y) | git reset -p HEAD^ &&
+	(echo n; echo y) | git reset -p HEAD^ >output &&
 	verify_state dir/foo work parent &&
-	verify_saved_state bar
+	verify_saved_state bar &&
+	test_i18ngrep "Apply" output
 '
 
 # The idea in the rest is that bar sorts first, so we always say 'y'

diff --git a/t/t7106-reset-unborn-branch.sh b/t/t7106-reset-unborn-branch.sh
index af00ab4..0f95f00 100755
--- a/t/t7106-reset-unborn-branch.sh
+++ b/t/t7106-reset-unborn-branch.sh

@@ -37,11 +37,12 @@ test_expect_success PERL 'reset -p' '
 	rm .git/index &&
 	git add a &&
 	echo y >yes &&
-	git reset -p <yes &&
+	git reset -p <yes >output &&
 
 	>expect &&
 	git ls-files >actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test_i18ngrep "Unstage" output
 '
 
 test_expect_success 'reset --soft is a no-op' '

-- 
1.8.4.1.898.g8bf8a41.dirty