Fwd: Bug report: reset -p HEAD

Fwd: Bug report: reset -p HEAD

[Maarten de Vries]

Hi,

I noticed that reset -p HEAD is inconsistent with checkout -p HEAD.
When running checkout -p you are asked to discard hunks from the index
and worktree, but when running reset -p you are asked to apply hunks
to the index. It would make more sense if reset -p asked to discard
(reversed) hunks from the index.

Digging a bit further, it looks like reset -p is actually intended to
show hunks to discard when resetting to HEAD. The
git-add--interactive.perl script has different cases for resetting to
the head and for resetting to anything else. However, builtin/reset.c
always passes a hash to run_add_interactive, even if HEAD is provided
explicitly on the command line or no revision is given. As a result,
the special case for resetting to the HEAD is never triggered and
git-add--interactive.perl always asks to apply hunks rather than
discard the reverse hunks.

The offending part in builtin/reset.c is on line 307. It's the bit
with sha1_to_hex(sha1):
>     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);
>     }

I'm not familiar enough with the git source, but it's probably a
fairly trivial fix for someone who is.


Kind regards,
Maarten de Vries


P.S.

This bit in git-add--interactive.perl convinced me that resetting to
HEAD interactively should be handled separately:
>     'reset_head' => {
>         DIFF => 'diff-index -p --cached',
>         APPLY => sub { apply_patch 'apply -R --cached', @_; },
>         APPLY_CHECK => 'apply -R --cached',
>         VERB => 'Unstage',
>         TARGET => '',
>         PARTICIPLE => 'unstaging',
>         FILTER => 'index-only',
>         IS_REVERSE => 1,
>     },
>     'reset_nothead' => {
>         DIFF => 'diff-index -R -p --cached',
>         APPLY => sub { apply_patch 'apply --cached', @_; },
>         APPLY_CHECK => 'apply --cached',
>         VERB => 'Apply',
>         TARGET => ' to index',
>         PARTICIPLE => 'applying',
>         FILTER => 'index-only',
>         IS_REVERSE => 0,
>     },

Re: Bug report: reset -p HEAD

[Maarten de Vries]

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

Kinds regards,
Maarten de Vries

On 25 October 2013 01:05, Maarten de Vries <maarten@de-vri.es> wrote:
> Hi,
>
> I noticed that reset -p HEAD is inconsistent with checkout -p HEAD.
> When running checkout -p you are asked to discard hunks from the index
> and worktree, but when running reset -p you are asked to apply hunks
> to the index. It would make more sense if reset -p asked to discard
> (reversed) hunks from the index.
>
> Digging a bit further, it looks like reset -p is actually intended to
> show hunks to discard when resetting to HEAD. The
> git-add--interactive.perl script has different cases for resetting to
> the head and for resetting to anything else. However, builtin/reset.c
> always passes a hash to run_add_interactive, even if HEAD is provided
> explicitly on the command line or no revision is given. As a result,
> the special case for resetting to the HEAD is never triggered and
> git-add--interactive.perl always asks to apply hunks rather than
> discard the reverse hunks.
>
> The offending part in builtin/reset.c is on line 307. It's the bit
> with sha1_to_hex(sha1):
>>     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);
>>     }
>
> I'm not familiar enough with the git source, but it's probably a
> fairly trivial fix for someone who is.
>
>
> Kind regards,
> Maarten de Vries
>
>
> P.S.
>
> This bit in git-add--interactive.perl convinced me that resetting to
> HEAD interactively should be handled separately:
>>     'reset_head' => {
>>         DIFF => 'diff-index -p --cached',
>>         APPLY => sub { apply_patch 'apply -R --cached', @_; },
>>         APPLY_CHECK => 'apply -R --cached',
>>         VERB => 'Unstage',
>>         TARGET => '',
>>         PARTICIPLE => 'unstaging',
>>         FILTER => 'index-only',
>>         IS_REVERSE => 1,
>>     },
>>     'reset_nothead' => {
>>         DIFF => 'diff-index -R -p --cached',
>>         APPLY => sub { apply_patch 'apply --cached', @_; },
>>         APPLY_CHECK => 'apply --cached',
>>         VERB => 'Apply',
>>         TARGET => ' to index',
>>         PARTICIPLE => 'applying',
>>         FILTER => 'index-only',
>>         IS_REVERSE => 0,
>>     },

Re* Bug report: reset -p HEAD

[Junio C Hamano]

Maarten de Vries <maarten@de-vri.es> 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

Re: Re* Bug report: reset -p HEAD

[Jeff King]

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

> Maarten de Vries <maarten@de-vri.es> 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);
>  	}

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

Re: Re* Bug report: reset -p HEAD

[Martin von Zweigbergk]

Sorry about the regression and thanks for report and fixes.

On Thu, Oct 24, 2013 at 9:24 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Oct 24, 2013 at 08:40:13PM -0700, Junio C Hamano wrote:
>
>> Maarten de Vries <maarten@de-vri.es> 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);
>>       }
>
> 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.

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

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.

[PATCH 0/2] reset -p and unborn branches

[Jeff King]

On Thu, Oct 24, 2013 at 10:42:52PM -0700, Martin von Zweigbergk 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:
> 
> 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.

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

[PATCH 1/2] add-interactive: handle unborn branch in patch mode

[Jeff King]

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 <peff@peff.net>
---
 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

[PATCH 2/2] reset: pass real rev name to add--interactive

[Jeff King]

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 <peff@peff.net>
---
 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

Re: Re* Bug report: reset -p HEAD

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Thanks; makes sense after twisting my mind somewhat, exactly for the
reason you stated here ;-)