{"thread":{"id":"35196","subject":"Fwd: Bug report: reset -p HEAD","startedAt":"2013-10-24T23:05:03Z","lastAt":"2013-10-25T16:54:52Z","messageCount":9,"participants":["Maarten de Vries","Junio C Hamano","Jeff King","Martin von Zweigbergk"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"229450","messageId":"CAPWpf+x5KUjD2K81pJAsV_XuPkd=BT06qPjvr8s7tEe5YqBt=w@mail.gmail.com","threadId":"35196","inReplyTo":"CAPWpf+xqutvhq1jyVkxr6LyKsANTCS6M=vj5XY=EgUfiS3Z8xg@mail.gmail.com","subject":"Fwd: Bug report: reset -p HEAD","fromName":"Maarten de Vries","fromEmail":"maarten@de-vri.es","sentAt":"2013-10-24T23:05:03Z","receivedAt":"2013-10-24T23:05:03Z","isPatch":false,"sender":{"key":"maarten@de-vri.es","avatar":null},"body":"Hi,\n\nI noticed that reset -p HEAD is inconsistent with checkout -p HEAD.\nWhen running checkout -p you are asked to discard hunks from the index\nand worktree, but when running reset -p you are asked to apply hunks\nto the index. It would make more sense if reset -p asked to discard\n(reversed) hunks from the index.\n\nDigging a bit further, it looks like reset -p is actually intended to\nshow hunks to discard when resetting to HEAD. The\ngit-add--interactive.perl script has different cases for resetting to\nthe head and for resetting to anything else. However, builtin/reset.c\nalways passes a hash to run_add_interactive, even if HEAD is provided\nexplicitly on the command line or no revision is given. As a result,\nthe special case for resetting to the HEAD is never triggered and\ngit-add--interactive.perl always asks to apply hunks rather than\ndiscard the reverse hunks.\n\nThe offending part in builtin/reset.c is on line 307. It's the bit\nwith sha1_to_hex(sha1):\n>     if (patch_mode) {\n>         if (reset_type != NONE)\n>             die(_(\"--patch is incompatible with --{hard,mixed,soft}\"));\n>         return run_add_interactive(sha1_to_hex(sha1), \"--patch=reset\", &pathspec);\n>     }\n\nI'm not familiar enough with the git source, but it's probably a\nfairly trivial fix for someone who is.\n\n\nKind regards,\nMaarten de Vries\n\n\nP.S.\n\nThis bit in git-add--interactive.perl convinced me that resetting to\nHEAD interactively should be handled separately:\n>     'reset_head' => {\n>         DIFF => 'diff-index -p --cached',\n>         APPLY => sub { apply_patch 'apply -R --cached', @_; },\n>         APPLY_CHECK => 'apply -R --cached',\n>         VERB => 'Unstage',\n>         TARGET => '',\n>         PARTICIPLE => 'unstaging',\n>         FILTER => 'index-only',\n>         IS_REVERSE => 1,\n>     },\n>     'reset_nothead' => {\n>         DIFF => 'diff-index -R -p --cached',\n>         APPLY => sub { apply_patch 'apply --cached', @_; },\n>         APPLY_CHECK => 'apply --cached',\n>         VERB => 'Apply',\n>         TARGET => ' to index',\n>         PARTICIPLE => 'applying',\n>         FILTER => 'index-only',\n>         IS_REVERSE => 0,\n>     },\n"},{"id":"229451","messageId":"CAPWpf+zkA68pzR3qx5xma8BNiONwt5o8iNba0x8fmmiDAWpHuw@mail.gmail.com","threadId":"35196","inReplyTo":"CAPWpf+x5KUjD2K81pJAsV_XuPkd=BT06qPjvr8s7tEe5YqBt=w@mail.gmail.com","subject":"Re: Bug report: reset -p HEAD","fromName":"Maarten de Vries","fromEmail":"maarten@de-vri.es","sentAt":"2013-10-24T23:16:55Z","receivedAt":"2013-10-24T23:16:55Z","isPatch":false,"sender":{"key":"maarten@de-vri.es","avatar":null},"body":"Some more info: It used to work as intended. Using a bisect shows it\nhas been broken by commit 166ec2e9.\n\nKinds regards,\nMaarten de Vries\n\nOn 25 October 2013 01:05, Maarten de Vries <maarten@de-vri.es> wrote:\n> Hi,\n>\n> I noticed that reset -p HEAD is inconsistent with checkout -p HEAD.\n> When running checkout -p you are asked to discard hunks from the index\n> and worktree, but when running reset -p you are asked to apply hunks\n> to the index. It would make more sense if reset -p asked to discard\n> (reversed) hunks from the index.\n>\n> Digging a bit further, it looks like reset -p is actually intended to\n> show hunks to discard when resetting to HEAD. The\n> git-add--interactive.perl script has different cases for resetting to\n> the head and for resetting to anything else. However, builtin/reset.c\n> always passes a hash to run_add_interactive, even if HEAD is provided\n> explicitly on the command line or no revision is given. As a result,\n> the special case for resetting to the HEAD is never triggered and\n> git-add--interactive.perl always asks to apply hunks rather than\n> discard the reverse hunks.\n>\n> The offending part in builtin/reset.c is on line 307. It's the bit\n> with sha1_to_hex(sha1):\n>>     if (patch_mode) {\n>>         if (reset_type != NONE)\n>>             die(_(\"--patch is incompatible with --{hard,mixed,soft}\"));\n>>         return run_add_interactive(sha1_to_hex(sha1), \"--patch=reset\", &pathspec);\n>>     }\n>\n> I'm not familiar enough with the git source, but it's probably a\n> fairly trivial fix for someone who is.\n>\n>\n> Kind regards,\n> Maarten de Vries\n>\n>\n> P.S.\n>\n> This bit in git-add--interactive.perl convinced me that resetting to\n> HEAD interactively should be handled separately:\n>>     'reset_head' => {\n>>         DIFF => 'diff-index -p --cached',\n>>         APPLY => sub { apply_patch 'apply -R --cached', @_; },\n>>         APPLY_CHECK => 'apply -R --cached',\n>>         VERB => 'Unstage',\n>>         TARGET => '',\n>>         PARTICIPLE => 'unstaging',\n>>         FILTER => 'index-only',\n>>         IS_REVERSE => 1,\n>>     },\n>>     'reset_nothead' => {\n>>         DIFF => 'diff-index -R -p --cached',\n>>         APPLY => sub { apply_patch 'apply --cached', @_; },\n>>         APPLY_CHECK => 'apply --cached',\n>>         VERB => 'Apply',\n>>         TARGET => ' to index',\n>>         PARTICIPLE => 'applying',\n>>         FILTER => 'index-only',\n>>         IS_REVERSE => 0,\n>>     },\n"},{"id":"229464","messageId":"xmqqvc0mj9wi.fsf_-_@gitster.dls.corp.google.com","threadId":"35196","inReplyTo":"CAPWpf+zkA68pzR3qx5xma8BNiONwt5o8iNba0x8fmmiDAWpHuw@mail.gmail.com","subject":"Re* Bug report: reset -p HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-25T03:40:13Z","receivedAt":"2013-10-25T03:40:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Maarten de Vries <maarten@de-vri.es> writes:\n\n> Some more info: It used to work as intended. Using a bisect shows it\n> has been broken by commit 166ec2e9.\n\nThanks.\n\nA knee-jerk change without thinking what side-effect it has for you\nto try out.\n\n builtin/reset.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex f2f9d55..a3088d9 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -304,7 +304,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \tif (patch_mode) {\n \t\tif (reset_type != NONE)\n \t\t\tdie(_(\"--patch is incompatible with --{hard,mixed,soft}\"));\n-\t\treturn run_add_interactive(sha1_to_hex(sha1), \"--patch=reset\", &pathspec);\n+\t\treturn run_add_interactive(\n+\t\t\t(unborn || strcmp(rev, \"HEAD\"))\n+\t\t\t? sha1_to_hex(sha1)\n+\t\t\t: \"HEAD\", \"--patch=reset\", &pathspec);\n \t}\n \n \t/* git reset tree [--] paths... can be used to\n"},{"id":"229470","messageId":"20131025042421.GB11810@sigill.intra.peff.net","threadId":"35196","inReplyTo":"xmqqvc0mj9wi.fsf_-_@gitster.dls.corp.google.com","subject":"Re: Re* Bug report: reset -p HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-25T04:24:21Z","receivedAt":"2013-10-25T04:24:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 24, 2013 at 08:40:13PM -0700, Junio C Hamano wrote:\n\n> Maarten de Vries <maarten@de-vri.es> writes:\n> \n> > Some more info: It used to work as intended. Using a bisect shows it\n> > has been broken by commit 166ec2e9.\n> \n> Thanks.\n> \n> A knee-jerk change without thinking what side-effect it has for you\n> to try out.\n> \n>  builtin/reset.c | 5 ++++-\n>  1 file changed, 4 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index f2f9d55..a3088d9 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -304,7 +304,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n>  \tif (patch_mode) {\n>  \t\tif (reset_type != NONE)\n>  \t\t\tdie(_(\"--patch is incompatible with --{hard,mixed,soft}\"));\n> -\t\treturn run_add_interactive(sha1_to_hex(sha1), \"--patch=reset\", &pathspec);\n> +\t\treturn run_add_interactive(\n> +\t\t\t(unborn || strcmp(rev, \"HEAD\"))\n> +\t\t\t? sha1_to_hex(sha1)\n> +\t\t\t: \"HEAD\", \"--patch=reset\", &pathspec);\n>  \t}\n\nI think that's the correct fix for the regression.  You are restoring\nthe original, pre-166ec2e9 behavior for just the HEAD case. I do not\nthink add--interactive does any other magic between a symbolic rev and\nits sha1, except for recognizing HEAD specially. However, if you wanted\nto minimize the potential impact of 166ec2e9, you could pass the sha1\n_only_ in the unborn case, like this:\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex f2f9d55..bfdd8d3 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -283,6 +283,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \tif (unborn) {\n \t\t/* reset on unborn branch: treat as reset to empty tree */\n \t\thashcpy(sha1, EMPTY_TREE_SHA1_BIN);\n+\t\trev = EMPTY_TREE_SHA1_HEX;\n \t} else if (!pathspec.nr) {\n \t\tstruct commit *commit;\n \t\tif (get_sha1_committish(rev, sha1))\n@@ -304,7 +305,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \tif (patch_mode) {\n \t\tif (reset_type != NONE)\n \t\t\tdie(_(\"--patch is incompatible with --{hard,mixed,soft}\"));\n-\t\treturn run_add_interactive(sha1_to_hex(sha1), \"--patch=reset\", &pathspec);\n+\t\treturn run_add_interactive(rev, \"--patch=reset\", &pathspec);\n \t}\n \n \t/* git reset tree [--] paths... can be used to\n\nThat fixes any possible regression from add--interactive treating the\ntwo cases differently. On an unborn branch, we will still say \"apply\nthis hunk\" rather than \"unstage this hunk\". That's not a regression,\nbecause it simply didn't work before, but it's not ideal. To fix that,\nwe need to somehow tell add--interactive \"this is HEAD, but use the\nempty tree because it's unborn\". I can think of a few simple-ish ways:\n\n  1. Pass the head/not-head flag as a separate option.\n\n  2. Pass HEAD even in the unborn case; teach add--interactive to\n     convert an unborn HEAD to the empty tree.\n\n  3. Teach add--interactive to recognize the empty tree sha1 as an\n     \"unstage\" path.\n\nI kind of like (3). At first glance, it is wrong; we will also treat\n\"git reset -p $(git hash-object -t tree /dev/null)\" as if \"HEAD\" had\nbeen passed. But if you are explicitly passing the empty tree like that,\nI think saying \"unstage\" makes a lot of sense.\n\n-Peff\n"},{"id":"229474","messageId":"CANiSa6iYLp-iNdcv_qbFTitrfFxaDBVUy9YyUAF4QKM+-35P4A@mail.gmail.com","threadId":"35196","inReplyTo":"20131025042421.GB11810@sigill.intra.peff.net","subject":"Re: Re* Bug report: reset -p HEAD","fromName":"Martin von Zweigbergk","fromEmail":"martinvonz@gmail.com","sentAt":"2013-10-25T05:42:52Z","receivedAt":"2013-10-25T05:42:52Z","isPatch":false,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"Sorry about the regression and thanks for report and fixes.\n\nOn Thu, Oct 24, 2013 at 9:24 PM, Jeff King <peff@peff.net> wrote:\n> On Thu, Oct 24, 2013 at 08:40:13PM -0700, Junio C Hamano wrote:\n>\n>> Maarten de Vries <maarten@de-vri.es> writes:\n>>\n>> > Some more info: It used to work as intended. Using a bisect shows it\n>> > has been broken by commit 166ec2e9.\n>>\n>> Thanks.\n>>\n>> A knee-jerk change without thinking what side-effect it has for you\n>> to try out.\n>>\n>>  builtin/reset.c | 5 ++++-\n>>  1 file changed, 4 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/reset.c b/builtin/reset.c\n>> index f2f9d55..a3088d9 100644\n>> --- a/builtin/reset.c\n>> +++ b/builtin/reset.c\n>> @@ -304,7 +304,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n>>       if (patch_mode) {\n>>               if (reset_type != NONE)\n>>                       die(_(\"--patch is incompatible with --{hard,mixed,soft}\"));\n>> -             return run_add_interactive(sha1_to_hex(sha1), \"--patch=reset\", &pathspec);\n>> +             return run_add_interactive(\n>> +                     (unborn || strcmp(rev, \"HEAD\"))\n>> +                     ? sha1_to_hex(sha1)\n>> +                     : \"HEAD\", \"--patch=reset\", &pathspec);\n>>       }\n>\n> I think that's the correct fix for the regression.  You are restoring\n> the original, pre-166ec2e9 behavior for just the HEAD case. I do not\n> think add--interactive does any other magic between a symbolic rev and\n> its sha1, except for recognizing HEAD specially. However, if you wanted\n> to minimize the potential impact of 166ec2e9, you could pass the sha1\n> _only_ in the unborn case, like this:\n\nPlus, the end result is more readable, IMHO.\n\n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index f2f9d55..bfdd8d3 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -283,6 +283,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n>         if (unborn) {\n>                 /* reset on unborn branch: treat as reset to empty tree */\n>                 hashcpy(sha1, EMPTY_TREE_SHA1_BIN);\n> +               rev = EMPTY_TREE_SHA1_HEX;\n>         } else if (!pathspec.nr) {\n>                 struct commit *commit;\n>                 if (get_sha1_committish(rev, sha1))\n> @@ -304,7 +305,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n>         if (patch_mode) {\n>                 if (reset_type != NONE)\n>                         die(_(\"--patch is incompatible with --{hard,mixed,soft}\"));\n> -               return run_add_interactive(sha1_to_hex(sha1), \"--patch=reset\", &pathspec);\n> +               return run_add_interactive(rev, \"--patch=reset\", &pathspec);\n>         }\n>\n>         /* git reset tree [--] paths... can be used to\n>\n> That fixes any possible regression from add--interactive treating the\n> two cases differently. On an unborn branch, we will still say \"apply\n> this hunk\" rather than \"unstage this hunk\". That's not a regression,\n> because it simply didn't work before, but it's not ideal. To fix that,\n> we need to somehow tell add--interactive \"this is HEAD, but use the\n> empty tree because it's unborn\". I can think of a few simple-ish ways:\n>\n>   1. Pass the head/not-head flag as a separate option.\n>\n>   2. Pass HEAD even in the unborn case; teach add--interactive to\n>      convert an unborn HEAD to the empty tree.\n>\n>   3. Teach add--interactive to recognize the empty tree sha1 as an\n>      \"unstage\" path.\n>\n> I kind of like (3). At first glance, it is wrong; we will also treat\n> \"git reset -p $(git hash-object -t tree /dev/null)\" as if \"HEAD\" had\n> been passed. But if you are explicitly passing the empty tree like that,\n> I think saying \"unstage\" makes a lot of sense.\n\nMakes sense to me. I'm sure others can implement that much faster than\nI can, but I feel a little guilty, so I'm happy to do it if no one\nelse wants to, as long as we agree this is the way we want to go.\n"},{"id":"229498","messageId":"20131025065117.GA15192@sigill.intra.peff.net","threadId":"35196","inReplyTo":"CANiSa6iYLp-iNdcv_qbFTitrfFxaDBVUy9YyUAF4QKM+-35P4A@mail.gmail.com","subject":"[PATCH 0/2] reset -p and unborn branches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-25T06:51:17Z","receivedAt":"2013-10-25T06:51:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 24, 2013 at 10:42:52PM -0700, Martin von Zweigbergk wrote:\n\n> > I think that's the correct fix for the regression.  You are restoring\n> > the original, pre-166ec2e9 behavior for just the HEAD case. I do not\n> > think add--interactive does any other magic between a symbolic rev and\n> > its sha1, except for recognizing HEAD specially. However, if you wanted\n> > to minimize the potential impact of 166ec2e9, you could pass the sha1\n> > _only_ in the unborn case, like this:\n> \n> Plus, the end result is more readable, IMHO.\n\nAgreed. Unfortunately it is slightly wrong, because for the non-patch\ncases, we may look at \"rev\" later, and we would want it to still say\n\"HEAD\" rather than a sha1. This is fixed in my patches below.\n\n> >   1. Pass the head/not-head flag as a separate option.\n> >\n> >   2. Pass HEAD even in the unborn case; teach add--interactive to\n> >      convert an unborn HEAD to the empty tree.\n> >\n> >   3. Teach add--interactive to recognize the empty tree sha1 as an\n> >      \"unstage\" path.\n> [...]\n> Makes sense to me. I'm sure others can implement that much faster than\n> I can, but I feel a little guilty, so I'm happy to do it if no one\n> else wants to, as long as we agree this is the way we want to go.\n\nAs it turns out, add--interactive already _does_ know how to handle an\nunborn HEAD. It just didn't use it for this particular code path. So I\nthink doing (2) makes the most sense, and the result is that the patch\nin reset.c ends up nice and simple.\n\n  [1/2]: add-interactive: handle unborn branch in patch mode\n  [2/2]: reset: pass real rev name to add--interactive\n\n-Peff\n"},{"id":"229499","messageId":"20131025065229.GA15253@sigill.intra.peff.net","threadId":"35196","inReplyTo":"20131025065117.GA15192@sigill.intra.peff.net","subject":"[PATCH 1/2] add-interactive: handle unborn branch in patch mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-25T06:52:30Z","receivedAt":"2013-10-25T06:52:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The list_modified function already knows how to handle an\nunborn branch by diffing against the empty tree. However,\nthe diff we perform to get the actual hunks does not. Let's\nuse the same logic for both diffs.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n git-add--interactive.perl | 22 +++++++++++++---------\n 1 file changed, 13 insertions(+), 9 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 5156384..24bb1ab 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -263,6 +263,17 @@ sub get_empty_tree {\n \treturn '4b825dc642cb6eb9a060e54bf8d69288fbee4904';\n }\n \n+sub get_diff_reference {\n+\tmy $ref = shift;\n+\tif (defined $ref and $ref ne 'HEAD') {\n+\t\treturn $ref;\n+\t} elsif (is_initial_commit()) {\n+\t\treturn get_empty_tree();\n+\t} else {\n+\t\treturn 'HEAD';\n+\t}\n+}\n+\n # Returns list of hashes, contents of each of which are:\n # VALUE:\tpathname\n # BINARY:\tis a binary path\n@@ -286,14 +297,7 @@ sub list_modified {\n \t\treturn if (!@tracked);\n \t}\n \n-\tmy $reference;\n-\tif (defined $patch_mode_revision and $patch_mode_revision ne 'HEAD') {\n-\t\t$reference = $patch_mode_revision;\n-\t} elsif (is_initial_commit()) {\n-\t\t$reference = get_empty_tree();\n-\t} else {\n-\t\t$reference = 'HEAD';\n-\t}\n+\tmy $reference = get_diff_reference($patch_mode_revision);\n \tfor (run_cmd_pipe(qw(git diff-index --cached\n \t\t\t     --numstat --summary), $reference,\n \t\t\t     '--', @tracked)) {\n@@ -737,7 +741,7 @@ sub parse_diff {\n \t\tsplice @diff_cmd, 1, 0, \"--diff-algorithm=${diff_algorithm}\";\n \t}\n \tif (defined $patch_mode_revision) {\n-\t\tpush @diff_cmd, $patch_mode_revision;\n+\t\tpush @diff_cmd, get_diff_reference($patch_mode_revision);\n \t}\n \tmy @diff = run_cmd_pipe(\"git\", @diff_cmd, \"--\", $path);\n \tmy @colored = ();\n-- \n1.8.4.1.898.g8bf8a41.dirty\n"},{"id":"229500","messageId":"20131025065406.GB15253@sigill.intra.peff.net","threadId":"35196","inReplyTo":"20131025065117.GA15192@sigill.intra.peff.net","subject":"[PATCH 2/2] reset: pass real rev name to add--interactive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-25T06:54:06Z","receivedAt":"2013-10-25T06:54:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The add--interactive --patch mode adjusts the UI based on\nwhether we are pulling changes from HEAD or elsewhere (in\nthe former case it asks to unstage the reverse hunk, rather\nthan apply the forward hunk).\n\nCommit 166ec2e taught reset to work on an unborn branch, but\nin doing so, switched to always providing add--interactive\nwith the sha1 rather than the symbolic name. This meant we\nalways used the \"apply\" interface, even for \"git reset -p\nHEAD\".\n\nWe can fix this by passing the symbolic name to\nadd--interactive.  Since it understands unborn branches\nthese days, we do not even have to cover this special case\nourselves; we can simply pass HEAD.\n\nThe tests in t7105 now check that the right interface is\nused in each circumstance (and notice the regression from\n166ec2e we are fixing). The test in t7106 checks that we\nget this right for the unborn case, too (not a regression,\nsince it didn't work at all before, but a nice improvement).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/reset.c                |  2 +-\n t/t7105-reset-patch.sh         | 10 ++++++----\n t/t7106-reset-unborn-branch.sh |  5 +++--\n 3 files changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex f2f9d55..6004803 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -304,7 +304,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \tif (patch_mode) {\n \t\tif (reset_type != NONE)\n \t\t\tdie(_(\"--patch is incompatible with --{hard,mixed,soft}\"));\n-\t\treturn run_add_interactive(sha1_to_hex(sha1), \"--patch=reset\", &pathspec);\n+\t\treturn run_add_interactive(rev, \"--patch=reset\", &pathspec);\n \t}\n \n \t/* git reset tree [--] paths... can be used to\ndiff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh\nindex 95fab20..98b7d7b 100755\n--- a/t/t7105-reset-patch.sh\n+++ b/t/t7105-reset-patch.sh\n@@ -25,15 +25,17 @@ test_expect_success PERL 'saying \"n\" does nothing' '\n '\n \n test_expect_success PERL 'git reset -p' '\n-\t(echo n; echo y) | git reset -p &&\n+\t(echo n; echo y) | git reset -p >output &&\n \tverify_state dir/foo work head &&\n-\tverify_saved_state bar\n+\tverify_saved_state bar &&\n+\ttest_i18ngrep \"Unstage\" output\n '\n \n test_expect_success PERL 'git reset -p HEAD^' '\n-\t(echo n; echo y) | git reset -p HEAD^ &&\n+\t(echo n; echo y) | git reset -p HEAD^ >output &&\n \tverify_state dir/foo work parent &&\n-\tverify_saved_state bar\n+\tverify_saved_state bar &&\n+\ttest_i18ngrep \"Apply\" output\n '\n \n # The idea in the rest is that bar sorts first, so we always say 'y'\ndiff --git a/t/t7106-reset-unborn-branch.sh b/t/t7106-reset-unborn-branch.sh\nindex af00ab4..0f95f00 100755\n--- a/t/t7106-reset-unborn-branch.sh\n+++ b/t/t7106-reset-unborn-branch.sh\n@@ -37,11 +37,12 @@ test_expect_success PERL 'reset -p' '\n \trm .git/index &&\n \tgit add a &&\n \techo y >yes &&\n-\tgit reset -p <yes &&\n+\tgit reset -p <yes >output &&\n \n \t>expect &&\n \tgit ls-files >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\ttest_i18ngrep \"Unstage\" output\n '\n \n test_expect_success 'reset --soft is a no-op' '\n-- \n1.8.4.1.898.g8bf8a41.dirty\n"},{"id":"229522","messageId":"xmqqiowljnoj.fsf@gitster.dls.corp.google.com","threadId":"35196","inReplyTo":"20131025042421.GB11810@sigill.intra.peff.net","subject":"Re: Re* Bug report: reset -p HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-25T16:54:52Z","receivedAt":"2013-10-25T16:54:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>   3. Teach add--interactive to recognize the empty tree sha1 as an\n>      \"unstage\" path.\n>\n> I kind of like (3). At first glance, it is wrong; we will also treat\n> \"git reset -p $(git hash-object -t tree /dev/null)\" as if \"HEAD\" had\n> been passed. But if you are explicitly passing the empty tree like that,\n> I think saying \"unstage\" makes a lot of sense.\n\nThanks; makes sense after twisting my mind somewhat, exactly for the\nreason you stated here ;-)\n"}]}