# Git reset -p on working tree 14 messages from 2010-11-23 to 2010-11-25. Participants: Jeenu V, knittl, Štěpán Němec, Jeff King, Michael J Gruber, Jakub Narebski, Matthieu Moy. Thread: https://gitlist.dev/t/25817 ## Jeenu V, 2010-11-23 09:11 Subject: Git reset -p on working tree Message-ID: URL: https://gitlist.dev/e/AANLkTimc%3DDt9YSu%3DJ%3D7C-f1hZ9UODU7VHu6oD8dmFX3N%40mail.gmail.com ``` Often I find it useful to selectively undo the modifications that I did to my working tree. The -p option to 'reset' that selectively resets changes back to a given commit. Unfortunately the option reports itself to be incompatible with --{hard,soft,mixed} and thus can't be applied to working tree. Right now I do this with vimdiff, by applying 'git diff' in reverse. Wish reset supports -p on working tree operations as well because the -p framework (of add, and reset) feels good to use. -- Jeenu ``` ## knittl, 2010-11-23 09:20 Subject: Re: Git reset -p on working tree Message-ID: URL: https://gitlist.dev/e/AANLkTimP9zr%3DwQDYeBxtvYCE1mZ1aHXQ_nLOxGFfR9YD%40mail.gmail.com In-Reply-To: ``` On Tue, Nov 23, 2010 at 10:11 AM, Jeenu V wrote: > Often I find it useful to selectively undo the modifications that I > did to my working tree. The -p option to 'reset' that selectively > resets changes back to a given commit. Unfortunately the option > reports itself to be incompatible with --{hard,soft,mixed} and thus > can't be applied to working tree. > > Right now I do this with vimdiff, by applying 'git diff' in reverse. > Wish reset supports -p on working tree operations as well because the > -p framework (of add, and reset) feels good to use. there's `git checkout -p` to selectively remove changes from your working tree. (also `git stash -p` to selectively stash changes temporarily) `git reset -p` will only undo staged changes from your index -- typed with http://neo-layout.org myFtPhp -- visit http://myftphp.sf.net -- v. 0.4.7 released! ``` ## Jeenu V, 2010-11-23 10:26 Subject: Re: Git reset -p on working tree Message-ID: URL: https://gitlist.dev/e/AANLkTimQ644C-dEoJTj8bNd9y%2BYzpMez7D1KLZFNEe7c%40mail.gmail.com In-Reply-To: ``` On Tue, Nov 23, 2010 at 2:50 PM, knittl wrote: > On Tue, Nov 23, 2010 at 10:11 AM, Jeenu V wrote: >> Often I find it useful to selectively undo the modifications that I >> did to my working tree. The -p option to 'reset' that selectively >> resets changes back to a given commit. Unfortunately the option >> reports itself to be incompatible with --{hard,soft,mixed} and thus >> can't be applied to working tree. >> >> Right now I do this with vimdiff, by applying 'git diff' in reverse. >> Wish reset supports -p on working tree operations as well because the >> -p framework (of add, and reset) feels good to use. > > > there's `git checkout -p` to selectively remove changes from your working tree. > (also `git stash -p` to selectively stash changes temporarily) Surprised to know that checkout and stash supports it! Thanks a lot for pointing out. -- Jeenu ``` ## Jeenu V, 2010-11-23 10:32 Subject: Re: Git reset -p on working tree Message-ID: URL: https://gitlist.dev/e/AANLkTindHnjeXEGbKWiQkWgoUBwqZbdx71sQwNr_gFp0%40mail.gmail.com In-Reply-To: ``` On Tue, Nov 23, 2010 at 3:56 PM, Jeenu V wrote: > On Tue, Nov 23, 2010 at 2:50 PM, knittl wrote: >>> Right now I do this with vimdiff, by applying 'git diff' in reverse. >>> Wish reset supports -p on working tree operations as well because the >>> -p framework (of add, and reset) feels good to use. >> >> >> there's `git checkout -p` to selectively remove changes from your working tree. >> (also `git stash -p` to selectively stash changes temporarily) > > Surprised to know that checkout and stash supports it! Thanks a lot > for pointing out. I think Git could suggest 'checkout -p' and/or 'stash -p' when user tries to use -p with --{hard,soft,mixed} with reset command (in addition to mentioning that they're incompatible), since relatively new users are used to 'reset' than other commands to reset their working trees. -- Jeenu ``` ## knittl, 2010-11-23 10:58 Subject: Re: Git reset -p on working tree Message-ID: URL: https://gitlist.dev/e/AANLkTi%3D5w3YpYu0_WbmHud0BUkO2BqYNVezWDMkCaSj2%40mail.gmail.com In-Reply-To: ``` On Tue, Nov 23, 2010 at 11:32 AM, Jeenu V wrote: > On Tue, Nov 23, 2010 at 3:56 PM, Jeenu V wrote: >> On Tue, Nov 23, 2010 at 2:50 PM, knittl wrote: >>>> Right now I do this with vimdiff, by applying 'git diff' in reverse. >>>> Wish reset supports -p on working tree operations as well because the >>>> -p framework (of add, and reset) feels good to use. >>> >>> >>> there's `git checkout -p` to selectively remove changes from your working tree. >>> (also `git stash -p` to selectively stash changes temporarily) >> >> Surprised to know that checkout and stash supports it! Thanks a lot >> for pointing out. > > I think Git could suggest 'checkout -p' and/or 'stash -p' when user > tries to use -p with --{hard,soft,mixed} with reset command (in > addition to mentioning that they're incompatible), since relatively > new users are used to 'reset' than other commands to reset their > working trees. `git reset` seldomly affects the working tree, unless using `--hard`, `--merge` or `--keep`, and those switches are potentially dangerous if you are not aware of the implications to your worktree and index. without options reset undoes a previous add, i.e. remove changes from the index, but not the working tree. git reset manpage mentions git-checkout: > After running git reset to update the index entry, you can > use git-checkout(1) to check the contents out of the index to the > working tree. Alternatively, using git-checkout(1) and specifying a > commit, you can copy the contents of a path out of a commit to the > index and to the working tree in one go. -- typed with http://neo-layout.org myFtPhp -- visit http://myftphp.sf.net -- v. 0.4.7 released! ``` ## Jeenu V, 2010-11-23 13:00 Subject: Re: Git reset -p on working tree Message-ID: URL: https://gitlist.dev/e/AANLkTimEOULR8_PAs5iPu7z%2Bn4DQPSyDKYg8RghZxosz%40mail.gmail.com In-Reply-To: ``` On Tue, Nov 23, 2010 at 4:28 PM, knittl wrote: > `git reset` seldomly affects the working tree, unless using `--hard`, > `--merge` or `--keep`, and those switches are potentially dangerous if > you are not aware of the implications to your worktree and index. > > without options reset undoes a previous add, i.e. remove changes from > the index, but not the working tree. > > git reset manpage mentions git-checkout: Agreed. What I'm saying is most manuals/guides out there tips user to use 'git reset --hard' to discard changes in the working directory and restore the pristine copy of files. 'reset' then becomes synonymous to discard, and it kind of is but for a different reason. Because other commands offer -p for selective operations, one might try to discard selectively, and end up using 'git reset -p --hard', which obviously isn't going to work. Knowing the users intention in such cases, I was suggesting the error message could hint the right command (just like git commit hints user to use git-add when one does 'git commit' with a dirty working tree). But that's just me. -- Jeenu ``` ## Štěpán Němec, 2010-11-23 14:20 Subject: [PATCH] Hint at "checkout -p" in the "reset --patch " error message Message-ID: <87mxp086k5.fsf_-_@gmail.com> URL: https://gitlist.dev/e/87mxp086k5.fsf_-_%40gmail.com In-Reply-To: ``` The --patch option to git-reset is not compatible with the type specifiers (--hard et al.), but some users try that first when what they really want is "checkout -p" (cf. ). Mention "git checkout -p" in the error message to point users in the right direction. Signed-off-by: Štěpán Němec --- Jeenu V writes: > On Tue, Nov 23, 2010 at 4:28 PM, knittl wrote: >> `git reset` seldomly affects the working tree, unless using `--hard`, >> `--merge` or `--keep`, and those switches are potentially dangerous if >> you are not aware of the implications to your worktree and index. >> >> without options reset undoes a previous add, i.e. remove changes from >> the index, but not the working tree. >> >> git reset manpage mentions git-checkout: > > Agreed. What I'm saying is most manuals/guides out there tips user to > use 'git reset --hard' to discard changes in the working directory and > restore the pristine copy of files. 'reset' then becomes synonymous to > discard, and it kind of is but for a different reason. Because other > commands offer -p for selective operations, one might try to discard > selectively, and end up using 'git reset -p --hard', which obviously > isn't going to work. > > Knowing the users intention in such cases, I was suggesting the error > message could hint the right command (just like git commit hints user > to use git-add when one does 'git commit' with a dirty working tree). > > But that's just me. Well, maybe something like the patch below wouldn't hurt? builtin/reset.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index 0037be4..fc530d2 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -309,7 +309,8 @@ 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}"); + die("--patch is incompatible with --{hard,mixed,soft}\n" + "(use \"git checkout -p\" to selectively discard changes in working directory)"); return interactive_reset(rev, argv + i, prefix); } -- 1.7.3.rc2.221.gbf93f.dirty ``` ## Jeff King, 2010-11-23 14:59 Subject: Re: [PATCH] Hint at "checkout -p" in the "reset --patch " error message Message-ID: <20101123145921.GB3145@sigill.intra.peff.net> URL: https://gitlist.dev/e/20101123145921.GB3145%40sigill.intra.peff.net In-Reply-To: <87mxp086k5.fsf_-_@gmail.com> ``` On Tue, Nov 23, 2010 at 03:20:58PM +0100, Štěpán Němec wrote: > diff --git a/builtin/reset.c b/builtin/reset.c > index 0037be4..fc530d2 100644 > --- a/builtin/reset.c > +++ b/builtin/reset.c > @@ -309,7 +309,8 @@ 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}"); > + die("--patch is incompatible with --{hard,mixed,soft}\n" > + "(use \"git checkout -p\" to selectively discard changes in working directory)"); > return interactive_reset(rev, argv + i, prefix); > } Should this actually print something different for --hard versus --mixed versus --soft? For --soft, "-p" simply makes no sense as you are not changing the index or the working directory. For --mixed, shouldn't we support "-p" already, as that is the default mode? For --hard, suggesting "checkout -p" does make sense. Also, what about "--merge" and "--keep" modes? -Peff ``` ## Štěpán Němec, 2010-11-23 15:23 Subject: Re: [PATCH] Hint at "checkout -p" in the "reset --patch " error message Message-ID: <87ipzo83o9.fsf@gmail.com> URL: https://gitlist.dev/e/87ipzo83o9.fsf%40gmail.com In-Reply-To: <20101123145921.GB3145@sigill.intra.peff.net> ``` Jeff King writes: > On Tue, Nov 23, 2010 at 03:20:58PM +0100, Štěpán Němec wrote: > >> diff --git a/builtin/reset.c b/builtin/reset.c >> index 0037be4..fc530d2 100644 >> --- a/builtin/reset.c >> +++ b/builtin/reset.c >> @@ -309,7 +309,8 @@ 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}"); >> + die("--patch is incompatible with --{hard,mixed,soft}\n" >> + "(use \"git checkout -p\" to selectively discard changes in working directory)"); >> return interactive_reset(rev, argv + i, prefix); >> } > > Should this actually print something different for --hard versus --mixed > versus --soft? > > For --soft, "-p" simply makes no sense as you are not changing the index > or the working directory. > > For --mixed, shouldn't we support "-p" already, as that is the default > mode? > > For --hard, suggesting "checkout -p" does make sense. > > Also, what about "--merge" and "--keep" modes? All good questions, most of which occured to me, too. What I was going to do based on the outcome of (the discussion of) this patch was at least fix the currently out-of-date "--{hard,mixed,soft}", as the error is signalled with the newer options (--keep and --merge) as well. I'm not sure I could cope with anything much more complicated than that, esp. if it involved adding some new reset functionality. :-) What about this instead: diff --git a/builtin/reset.c b/builtin/reset.c index 0037be4..7779302 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -308,8 +308,11 @@ int cmd_reset(int argc, const char **argv, const char *prefix) hashcpy(sha1, commit->object.sha1); if (patch_mode) { - if (reset_type != NONE) - die("--patch is incompatible with --{hard,mixed,soft}"); + if (reset_type == HARD) + die("--patch is incompatible with --hard\n" + "(use \"git checkout -p\" to selectively discard changes in working directory)"); + if (reset_type != MIXED) + die("--patch is incompatible with --{hard,soft,keep,merge}"); return interactive_reset(rev, argv + i, prefix); } Thanks, Štěpán ``` ## Michael J Gruber, 2010-11-25 08:55 Subject: Re: [PATCH] Hint at "checkout -p" in the "reset --patch " error message Message-ID: <4CEE2472.5020901@drmicha.warpmail.net> URL: https://gitlist.dev/e/4CEE2472.5020901%40drmicha.warpmail.net In-Reply-To: <87ipzo83o9.fsf@gmail.com> ``` Štěpán Němec venit, vidit, dixit 23.11.2010 16:23: > Jeff King writes: > >> On Tue, Nov 23, 2010 at 03:20:58PM +0100, Štěpán Němec wrote: >> >>> diff --git a/builtin/reset.c b/builtin/reset.c >>> index 0037be4..fc530d2 100644 >>> --- a/builtin/reset.c >>> +++ b/builtin/reset.c >>> @@ -309,7 +309,8 @@ 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}"); >>> + die("--patch is incompatible with --{hard,mixed,soft}\n" >>> + "(use \"git checkout -p\" to selectively discard changes in working directory)"); >>> return interactive_reset(rev, argv + i, prefix); >>> } >> >> Should this actually print something different for --hard versus --mixed >> versus --soft? >> >> For --soft, "-p" simply makes no sense as you are not changing the index >> or the working directory. >> >> For --mixed, shouldn't we support "-p" already, as that is the default >> mode? >> >> For --hard, suggesting "checkout -p" does make sense. >> >> Also, what about "--merge" and "--keep" modes? > > All good questions, most of which occured to me, too. What I was going > to do based on the outcome of (the discussion of) this patch was at > least fix the currently out-of-date "--{hard,mixed,soft}", as the > error is signalled with the newer options (--keep and --merge) as well. > I'm not sure I could cope with anything much more complicated than that, > esp. if it involved adding some new reset functionality. :-) The man page for git-reset points at git-checkout already. Until we have a "verbosity level" config across all commands, I don't favor adding a lot of hints to command error messages. Michael ``` ## Jakub Narebski, 2010-11-25 10:56 Subject: Re: [PATCH] Hint at "checkout -p" in the "reset --patch " error message Message-ID: URL: https://gitlist.dev/e/m3mxox4qr3.fsf%40localhost.localdomain In-Reply-To: <4CEE2472.5020901@drmicha.warpmail.net> ``` Michael J Gruber writes: > Štěpán Němec venit, vidit, dixit 23.11.2010 16:23: >> Jeff King writes: >>> On Tue, Nov 23, 2010 at 03:20:58PM +0100, Štěpán Němec wrote: >>> >>>> diff --git a/builtin/reset.c b/builtin/reset.c >>>> index 0037be4..fc530d2 100644 >>>> --- a/builtin/reset.c >>>> +++ b/builtin/reset.c >>>> @@ -309,7 +309,8 @@ 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}"); >>>> + die("--patch is incompatible with --{hard,mixed,soft}\n" >>>> + "(use \"git checkout -p\" to selectively discard changes in working directory)"); >>>> return interactive_reset(rev, argv + i, prefix); >>>> } [...] > > The man page for git-reset points at git-checkout already. > > Until we have a "verbosity level" config across all commands, I don't > favor adding a lot of hints to command error messages. Errrr... but we do have "verbosity level" config in the form of advice.* configuration variables. -- Jakub Narebski Poland ShadeHawk on #git ``` ## Michael J Gruber, 2010-11-25 13:10 Subject: Re: [PATCH] Hint at "checkout -p" in the "reset --patch " error message Message-ID: <4CEE604E.2030208@drmicha.warpmail.net> URL: https://gitlist.dev/e/4CEE604E.2030208%40drmicha.warpmail.net In-Reply-To: ``` Jakub Narebski venit, vidit, dixit 25.11.2010 11:56: > Michael J Gruber writes: >> Štěpán Němec venit, vidit, dixit 23.11.2010 16:23: >>> Jeff King writes: >>>> On Tue, Nov 23, 2010 at 03:20:58PM +0100, Štěpán Němec wrote: >>>> >>>>> diff --git a/builtin/reset.c b/builtin/reset.c >>>>> index 0037be4..fc530d2 100644 >>>>> --- a/builtin/reset.c >>>>> +++ b/builtin/reset.c >>>>> @@ -309,7 +309,8 @@ 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}"); >>>>> + die("--patch is incompatible with --{hard,mixed,soft}\n" >>>>> + "(use \"git checkout -p\" to selectively discard changes in working directory)"); >>>>> return interactive_reset(rev, argv + i, prefix); >>>>> } > > [...] >> >> The man page for git-reset points at git-checkout already. >> >> Until we have a "verbosity level" config across all commands, I don't >> favor adding a lot of hints to command error messages. > > Errrr... but we do have "verbosity level" config in the form of > advice.* configuration variables. ...which is why I wrote "across all commands". Introducing one config per advice does not scale at all. (Well, it scales linearly...) If we want our commands to give advice we need something like "verbosity=silent|quiet|moderate|instructive|chatty" working across all commands. Michael ``` ## Matthieu Moy, 2010-11-25 15:56 Subject: Re: [PATCH] Hint at "checkout -p" in the "reset --patch " error message Message-ID: URL: https://gitlist.dev/e/vpqbp5diei5.fsf%40bauges.imag.fr In-Reply-To: <4CEE2472.5020901@drmicha.warpmail.net> ``` Michael J Gruber writes: >>>> if (patch_mode) { >>>> if (reset_type != NONE) >>>> - die("--patch is incompatible with --{hard,mixed,soft}"); >>>> + die("--patch is incompatible with --{hard,mixed,soft}\n" >>>> + "(use \"git checkout -p\" to selectively discard changes in working directory)"); >>>> return interactive_reset(rev, argv + i, prefix); >>>> } >>> > > The man page for git-reset points at git-checkout already. > > Until we have a "verbosity level" config across all commands, I don't > favor adding a lot of hints to command error messages. I agree for "regular output", but this case is an error message that users should never see. If you see the message, it means you tried a command that doesn't exist, and I can't imagine a case where this would be disturbing to get an informative message pointing to the right command. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Jeff King, 2010-11-25 16:14 Subject: Re: [PATCH] Hint at "checkout -p" in the "reset --patch " error message Message-ID: <20101125161446.GA24426@sigill.intra.peff.net> URL: https://gitlist.dev/e/20101125161446.GA24426%40sigill.intra.peff.net In-Reply-To: ``` On Thu, Nov 25, 2010 at 04:56:02PM +0100, Matthieu Moy wrote: > > The man page for git-reset points at git-checkout already. > > > > Until we have a "verbosity level" config across all commands, I don't > > favor adding a lot of hints to command error messages. > > I agree for "regular output", but this case is an error message that > users should never see. If you see the message, it means you tried a > command that doesn't exist, and I can't imagine a case where this > would be disturbing to get an informative message pointing to the > right command. I have to agree. When I introduced advice.*, I had initially intended to have "advice.all" or similar, so expert users could shut off the verbose warnings. But I found that I didn't really want it. Most of the advice is for messages like these that you just don't see that often, and the extra line of output is simply not that big a deal. For messages that you _do_ see every day (e.g., the help in "git status"), it is not so cumbersome to set the proper config option. Which isn't to say I would be against "advice.all" (or even advice.commandSuggestions) if somebody submitted a patch for it. I just don't want it personally. And I don't think any of this needs to hold up Štěpán's patch[1]. What has happened in the past is something like: 1. Somebody introduces new messages. The patch makes it into next. 2. A developer gets annoyed and creates a patch for advice.whatever. 3. That patch goes on the appropriate topic and they make it into the release together. which makes sense to me. Then we know the patch in (2) is not just bikeshedding, but actually fixes somebody's annoyance. :) -Peff [1] Don't take this as an endorsement of the patch. I think the intent is good, but I haven't reviewed it carefully to make sure "git reset -p --mixed" actually works as advertised. Today is American Thanksgiving, so I probably won't get to it for a few days. ```