Git reset -p on working tree

Git reset -p on working tree

[Jeenu V]

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

Re: Git reset -p on working tree

[knittl]

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)

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

Re: Git reset -p on working tree

[Jeenu V]

On Tue, Nov 23, 2010 at 2:50 PM, knittl <knittl89@googlemail.com> wrote:
> 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

Re: Git reset -p on working tree

[Jeenu V]

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.

-- 
Jeenu

Re: Git reset -p on working tree

[knittl]

On Tue, Nov 23, 2010 at 11:32 AM, Jeenu V <jeenuv@gmail.com> wrote:
> 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:

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

Re: Git reset -p on working tree

[Jeenu V]

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.

-- 
Jeenu

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

[Štěpán Němec]

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:

> 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

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

[Jeff King]

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

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

[Štěpán Němec]

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

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

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

[Michael J Gruber]

Š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);
>>>  	}
>>
>> 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

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

[Jakub Narebski]

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.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

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

[Michael J Gruber]

Jakub Narebski venit, vidit, dixit 25.11.2010 11:56:
> 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

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

[Matthieu Moy]

Michael J Gruber <git@drmicha.warpmail.net> 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/

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

[Jeff King]

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.