threads / discuss / 25817

Git reset -p on working tree

Subject: Git reset -p on working tree

## tl;dr

14 messages between Nov 23, 2010 and Nov 25, 2010.

replies: 13people: 7as markdown or json

Jeenu V· Nov 23, 2010, 09:11 UTC · lore

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· Nov 23, 2010, 09:20 UTC · re: Jeenu V · lore

Re: Git reset -p on working tree

On Tue, Nov 23, 2010 at 10:11 AM, Jeenu V <jeenuv@gmail.com> wrote:
Show 9 quoted lines
> 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· Nov 23, 2010, 10:26 UTC · re: knittl · lore

Re: Git reset -p on working tree

On Tue, Nov 23, 2010 at 2:50 PM, knittl <knittl89@googlemail.com> wrote:
Show 14 quoted lines
> On Tue, Nov 23, 2010 at 10:11 AM, Jeenu V <jeenuv@gmail.com> 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· Nov 23, 2010, 10:32 UTC · re: Jeenu V · lore

Re: Git reset -p on working tree

On Tue, Nov 23, 2010 at 3:56 PM, Jeenu V <jeenuv@gmail.com> wrote:
Show 11 quoted lines
> On Tue, Nov 23, 2010 at 2:50 PM, knittl <knittl89@googlemail.com> 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· Nov 23, 2010, 10:58 UTC · re: Jeenu V · lore

Re: Git reset -p on working tree

On Tue, Nov 23, 2010 at 11:32 AM, Jeenu V <jeenuv@gmail.com> wrote:
Show 18 quoted lines
> On Tue, Nov 23, 2010 at 3:56 PM, Jeenu V <jeenuv@gmail.com> wrote:
>> On Tue, Nov 23, 2010 at 2:50 PM, knittl <knittl89@googlemail.com> 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:
Show 5 quoted lines
> After running git reset <paths> 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· Nov 23, 2010, 13:00 UTC · re: knittl · lore

Re: Git reset -p on working tree

On Tue, Nov 23, 2010 at 4:28 PM, knittl <knittl89@googlemail.com> wrote:
Show 8 quoted lines
> `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· Nov 23, 2010, 14:20 UTC · re: Jeenu V · lore

[PATCH] Hint at "checkout -p" in the "reset --patch <type>" error message

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. <http://permalink.gmane.org/gmane.comp.version-control.git/161955>).

Mention "git checkout -p" in the error message to point users in the right direction.

Signed-off-by: Štěpán Němec <stepnem@gmail.com>
---
Jeenu V <jeenuv@gmail.com> writes:
Show 23 quoted lines
> On Tue, Nov 23, 2010 at 4:28 PM, knittl <knittl89@googlemail.com> 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· Nov 23, 2010, 14:59 UTC · re: Štěpán Němec · lore

Re: [PATCH] Hint at "checkout -p" in the "reset --patch <type>" error message

On Tue, Nov 23, 2010 at 03:20:58PM +0100, Štěpán Němec wrote:
Show 13 quoted lines
> 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· Nov 23, 2010, 15:23 UTC · re: Jeff King · lore

Re: [PATCH] Hint at "checkout -p" in the "reset --patch <type>" error message

Jeff King <peff@peff.net> writes:
Show 28 quoted lines
> 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· Nov 25, 2010, 08:55 UTC · re: Štěpán Němec · lore

Re: [PATCH] Hint at "checkout -p" in the "reset --patch <type>" error message

Štěpán Němec venit, vidit, dixit 23.11.2010 16:23:
Show 37 quoted lines
> Jeff King <peff@peff.net> 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· Nov 25, 2010, 10:56 UTC · re: Michael J Gruber · lore

Re: [PATCH] Hint at "checkout -p" in the "reset --patch <type>" error message

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 17 quoted lines
> Štěpán Němec venit, vidit, dixit 23.11.2010 16:23:
>> Jeff King <peff@peff.net> 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);
>>>>  	}
[...]
Show 5 quoted lines
> 
> 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· Nov 25, 2010, 13:10 UTC · re: Jakub Narebski · lore

Re: [PATCH] Hint at "checkout -p" in the "reset --patch <type>" error message

Jakub Narebski venit, vidit, dixit 25.11.2010 11:56:
Show 28 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>> Štěpán Němec venit, vidit, dixit 23.11.2010 16:23:
>>> Jeff King <peff@peff.net> 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· Nov 25, 2010, 15:56 UTC · re: Michael J Gruber · lore

Re: [PATCH] Hint at "checkout -p" in the "reset --patch <type>" error message

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 13 quoted lines
>>>>  	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· Nov 25, 2010, 16:14 UTC · re: Matthieu Moy · lore

Re: [PATCH] Hint at "checkout -p" in the "reset --patch <type>" error message

On Thu, Nov 25, 2010 at 04:56:02PM +0100, Matthieu Moy wrote:
Show 10 quoted lines
> > 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.

← back to recent threads