[PATCH] reset: Better warning message on git reset --mixed <paths>

[PATCH] reset: Better warning message on git reset --mixed <paths>

[Ævar Arnfjörð Bjarmason]

When you call "git reset --mixed <paths>" git will complain that using
mixed with paths is deprecated:

    warning: --mixed option is deprecated with paths.

That doesn't tell the user why it's deprecated, or what he should use
instead. Expand on the warning and tell the user to just omit --mixed:

    warning: --mixed is redundant with paths, use 'git reset <paths>' instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Sat, Aug 14, 2010 at 19:12, Ralf Ebert <info@ralfebert.de> wrote:
> On 14.08.2010 20:55, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Sat, Aug 14, 2010 at 18:40, Ralf Ebert<info@ralfebert.de>  wrote:
>>>
>>> On 14.08.2010 17:23, Ævar Arnfjörð Bjarmason wrote:
>>>>
>>>> So what should I use instead? 0e5a7faa which introduced it doesn't say.
>>>
>>> It doesn't currently exist, but I'm pretty sure it should be called 'git
>>> unstage' :)
>>
>> Then perhaps it should be undeprecated? If what you say is correct
>> it's been whining about that since 2007 with no alternative, while
>> doing what I mean when I run it.
>
> Sorry, I misremembered '--mixed' again, '--mixed' is the default and it
> doesn't complain if you just omit it, and if I read&tried right, 'git reset
> <file>' does the same as 'git reset --mixed <file>'.

Right you are, if only we had a manpage to explain this or something >:)

Anyway, I think the warning could be better, here's a patch to
implement that. We could also warn on a plain "git reset --mixed"
since it's also redundant, but that would be adding something new so I
haven't done that here.

 builtin/reset.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 1283068..a7878d4 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -318,7 +318,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	 * affecting the working tree nor HEAD. */
 	if (i < argc) {
 		if (reset_type == MIXED)
-			warning("--mixed option is deprecated with paths.");
+			warning("--mixed is redundant with paths, use 'git reset <paths>' instead.");
 		else if (reset_type != NONE)
 			die("Cannot do %s reset with paths.",
 					reset_type_names[reset_type]);
-- 
1.7.2.1.339.g9c5d4

Re: [PATCH] reset: Better warning message on git reset --mixed <paths>

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> Anyway, I think the warning could be better, here's a patch to
> implement that. We could also warn on a plain "git reset --mixed"
> since it's also redundant, but that would be adding something new so I
> haven't done that here.

That seems backwards --- it is not actually the redundancy that leads
to this warning (and I hope we never add such a warning to save people
typing).

Instead, I had assumed the idea was that some day “git reset --mixed
<path>” will be forbidden, eliminating a potential ambiguity in that
argument and saving high-level scripts from needing -- at the end of
“git reset --mixed <rev> --”.

> +++ b/builtin/reset.c
> @@ -318,7 +318,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>  	 * affecting the working tree nor HEAD. */
>  	if (i < argc) {
>  		if (reset_type == MIXED)
> -			warning("--mixed option is deprecated with paths.");
> +			warning("--mixed is redundant with paths, use 'git reset <paths>' instead.");

Maybe:

 warning: --mixed with paths is deprecated; use 'git reset -- <paths>' instead

Re: [PATCH] reset: Better warning message on git reset --mixed <paths>

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Maybe:
>
>  warning: --mixed with paths is deprecated; use 'git reset -- <paths>' instead

That sounds helpful.

We've been saying this form is deprecated since 1.5.4; a better
alternative may be to make this into an error in the next release,
though.

Re: [PATCH] reset: Better warning message on git reset --mixed <paths>

[Miles Bader]

Junio C Hamano <gitster@pobox.com> writes:
>>  warning: --mixed with paths is deprecated; use 'git reset -- <paths>' instead
>
> That sounds helpful.
>
> We've been saying this form is deprecated since 1.5.4; a better
> alternative may be to make this into an error in the next release,
> though.

On a similar note, how about a more helpful error message for
"git reset --hard PATH"?  It took me _ages_ to figure out that I
needed to do "git checkout PATH" to get that effect...

e.g.:

   $ git reset --hard load-lua.cc
   fatal: --hard cannot be used with paths; use "git checkout -- <paths> instead"

-Miles

-- 
Brain, n. An apparatus with which we think we think.

Re: [PATCH] reset: Better warning message on git reset --mixed <paths>

[Ævar Arnfjörð Bjarmason]

On Sun, Aug 15, 2010 at 02:43, Miles Bader <miles@gnu.org> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>>>  warning: --mixed with paths is deprecated; use 'git reset -- <paths>' instead
>>
>> That sounds helpful.
>>
>> We've been saying this form is deprecated since 1.5.4; a better
>> alternative may be to make this into an error in the next release,
>> though.
>
> On a similar note, how about a more helpful error message for
> "git reset --hard PATH"?  It took me _ages_ to figure out that I
> needed to do "git checkout PATH" to get that effect...
>
> e.g.:
>
>   $ git reset --hard load-lua.cc
>   fatal: --hard cannot be used with paths; use "git checkout -- <paths> instead"

Do you have suggestions for alternatives for "git reset [ --soft |
--merge | --keep ] -- <paths>" too? Since I'm working on this anyway I
might roll that in.

Re: [PATCH] reset: Better warning message on git reset --mixed <paths>

[Junio C Hamano]

Regarding the various modes that are forbidden with "reset", I've been
wondering if we can do things differently.

If you want to reset the index at selected paths, don't use any option, as
nothing but --mixed makes sense.  Hence we deprecated use of --mixed when
used this way.  But if _it_ makes sense, why deprecate it?  What harm
would it do if we took it silently?

IIRC, the reasoning that lead to the deprecation was that allowing --mixed
may give a false impression to confused users that other mode options also
might be usable with pathspec, e.g. "reset --soft [<commit>] <pathspec>".
It obviously should barf loudly, as there is no way to move the HEAD to a
named commit without touching the index and the worktree at only specified
paths, but then the error message belongs to --soft, not --mixed.

Also "reset --hard [<commit>] -- <pathspec>" is forbidden in a repository
with a working tree, but it is clear that what the user wanted to do with
that unsupported command was what "checkout [<commit>] -- <pathspec>"
would have done.

What if we:

 - change "reset --hard [<commit>] -- <pathspec>" to internally run the
   moral equivalent "checkout" without complaining;

 - change "reset --mixed [<commit>] -- <pathspec>" to do the same thing as
   it has always done without complaining; and

 - make sure "reset --soft [<commit>] -- <pathspec>" barfs loudly.

Do people see major downside?

Re: [PATCH] reset: Better warning message on git reset --mixed <paths>

[Jonathan Nieder]

Junio C Hamano wrote:

> If you want to reset the index at selected paths, don't use any option, as
> nothing but --mixed makes sense.  Hence we deprecated use of --mixed when
> used this way.  But if _it_ makes sense, why deprecate it?  What harm
> would it do if we took it silently?

Nothing major.  It would be the same inconsistency as we currently have
between "checkout <commit> -- <pathspec>" and "checkout <commit> --":
one updates the HEAD (or rather, for "reset", the current branch),
while the other doesn’t.

So my preference is to make "reset --foo" not take a pathspec, but it
is not a strong one.

Re: [PATCH] reset: Better warning message on git reset --mixed <paths>

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Nothing major.  It would be the same inconsistency as we currently have
> between "checkout <commit> -- <pathspec>" and "checkout <commit> --":
> one updates the HEAD (or rather, for "reset", the current branch),
> while the other doesn’t.

What do you mean by inconsistency?  One is checking out the whole branch
to work on the named branch, while the other is checking out individual
paths out of branch to work on them in the current branch.

They are entirely different operations.

Re: [PATCH] reset: Better warning message on git reset --mixed <paths>

[Ævar Arnfjörð Bjarmason]

On Sun, Aug 15, 2010 at 18:36, Junio C Hamano <gitster@pobox.com> wrote:

> What if we:
>
>  - change "reset --hard [<commit>] -- <pathspec>" to internally run the
>   moral equivalent "checkout" without complaining;
>
>  - change "reset --mixed [<commit>] -- <pathspec>" to do the same thing as
>   it has always done without complaining; and
>
>  - make sure "reset --soft [<commit>] -- <pathspec>" barfs loudly.
>
> Do people see major downside?

Generally I'd like to see Git move towards have a more internally
consistent UI and less warts.

Doing what you suggest would make git-reset(1) more internally
consistent, while duplicating the git-checkout functionality.

So it's a question of whether git-reset should do all reset-y things
without complaining, even when that infringes on git-checkout's
domain.

I don't know which should be picked, but it's something to
consider.

One thing I'd like to see is some documentation showing how we'd like
Git to act if we didn't have to worry about backwards compatibility. I
sometimes see little warts like mixing of cached/index/stage, but I
haven't seen any plan for these sort of things. Are we planning to
change some of these things in the future? If so when, and what should
be changed?

It's hard to say whether a UI change like this makes sense without the
bigger picture.

Re: [PATCH] reset: Better warning message on git reset --mixed <paths>

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> So it's a question of whether git-reset should do all reset-y things
> without complaining, even when that infringes on git-checkout's
> domain.

I'm not sure about "domain"; do not think we should also forbid for the
same argument "git reset --soft" because it violates update-ref's domain?

I changed my mind; I do not like "git reset --hard <commit> -- <pathspec>"
doing "checkout <commit> -- <pathspec>", unless we limit this when <commit>
is not given (or perhaps when <commit> is the same as HEAD).  Otherwise it
would make things harder to explain.  So let's forget about it now.

Re: [PATCH] reset: Better warning message on git reset --mixed <paths>

[Miles Bader]

On Mon, Aug 16, 2010 at 5:51 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> So it's a question of whether git-reset should do all reset-y things
> without complaining, even when that infringes on git-checkout's
> domain.

Of course one question is:  why is this "git-checkout's domain" in the
first place?

From a UI perspective this functionality doesn't seem to make any more
sense in checkout -- and perhaps _less_ -- than it does in git-reset.
"git-checkout <path>" seems like a tacked-on-to-make-cvs-users-happy
wart rather than a natural part of git-checkout.

I know that as a beginning git user, I always tried to use "git-reset
--hard <path>", because that sort of made sense in my mental model of
git commands, only to be confused when it didn't work.  The fact that
one actually needed to to do git-checkout instead was confusing.

-Miles

-- 
Do not taunt Happy Fun Ball.

Re: [PATCH] reset: Better warning message on git reset --mixed <paths>

[Ævar Arnfjörð Bjarmason]

On Mon, Aug 16, 2010 at 00:59, Miles Bader <miles@gnu.org> wrote:
> On Mon, Aug 16, 2010 at 5:51 AM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> So it's a question of whether git-reset should do all reset-y things
>> without complaining, even when that infringes on git-checkout's
>> domain.
>
> Of course one question is:  why is this "git-checkout's domain" in the
> first place?
>
> From a UI perspective this functionality doesn't seem to make any more
> sense in checkout -- and perhaps _less_ -- than it does in git-reset.
> "git-checkout <path>" seems like a tacked-on-to-make-cvs-users-happy
> wart rather than a natural part of git-checkout.
>
> I know that as a beginning git user, I always tried to use "git-reset
> --hard <path>", because that sort of made sense in my mental model of
> git commands, only to be confused when it didn't work.  The fact that
> one actually needed to to do git-checkout instead was confusing.

Yeah, maybe one command to reset a file to various states makes more
sense than a singular "checking it out".

Anyway, this is the sort of thing I was alluding to, it won't do to
make micro-changes to the UI without considering the bigger picture.

Re: [PATCH] reset: Better warning message on git reset --mixed <paths>

[Ralf Ebert]

> Regarding the various modes that are forbidden with "reset", I've been
> wondering if we can do things differently.

After being very happy about finding the
'soft-mixed-hard-path-cheat-sheet' table in 0e5a7fa (mentioned in Ævars
message that triggered the discussion)

reset:  --soft  --mixed  --hard  -- <paths>
HEAD       X       X        X        -
index      -       X        X        X
files      -       -        X        -

I was wondering:

Wouldn't the common reset soft/mixed/hard with path/commit operations
become a lot more intuitive by separating the "(1) copy [partially] from
HEAD to index [to working tree]" operation and the "(2) change HEAD and
do (1)" operation? Just as a very rough scribble of what I mean, like
this:

git wipe [<path>]            # git reset [<file>]; git checkout [<file>]
git wipe [<path>] --index-only           # git reset [--mixed] [<path>]

git set-head <commit>                    # git reset --soft [<commit>]
git set-head <commit> --wipe             # git reset --hard [<commit>]
git set-head <commit> --wipe-index-only  # git reset --mixed [<commit>]

(I know that git reset can do more than that, but this is everything I
ever wanted as porcelain-only user from git reset, thought maybe the
idea could be of use without being fully elaborated)

--
Ralf

Re: [PATCH] reset: Better warning message on git reset --mixed <paths>

[Jonathan Nieder]

Ralf Ebert wrote:

> git wipe [<path>]            # git reset [<file>]; git checkout [<file>]

git checkout HEAD -- <pathspec>.

So:

repository  ---> index ---> work tree

Limited by paths
================

  r -> i: git reset <rev> -- <pathspec>
  i -> w: git checkout -- <pathspec>
  r -> w: git checkout <rev> -- <pathspec>

Updating head symref
====================

  r -> r: git checkout HEAD^0 && git reset --soft <rev> && git checkout <rev>
  r -> i: git checkout HEAD^0 && git reset --mixed <rev> && git checkout <rev>
  r -> w: git checkout <rev>

Resetting branch tip
====================

  r -> r: git reset --soft <rev>
  r -> i: git reset --mixed <rev>
  r -> w: git reset --hard <rev>

“git checkout --no-update” and “git checkout --no-update --keep-index”
do not sound very useful to me, so I am not suggesting adding them.

Re: [PATCH] reset: Better warning message on git reset --mixed <paths>

[David Fries]

On Sun, Aug 15, 2010 at 11:43:59AM +0900, Miles Bader wrote:
> On a similar note, how about a more helpful error message for
> "git reset --hard PATH"?  It took me _ages_ to figure out that I
> needed to do "git checkout PATH" to get that effect...

There's also the mixed reset in a bare repository message that could
use an update.

-- 
David Fries <david@fries.net>
http://fries.net/~david/ (PGP encryption key available)

>From fe5837d9222a621c081e22685db65adab32207f6 Mon Sep 17 00:00:00 2001
From: David Fries <david@fries.net>
Date: Fri, 2 Jul 2010 09:12:47 -0500
Subject: [PATCH] reset --mixed message update, tell the user what will work

Instead of,
fatal: mixed reset is not allowed in a bare repository
print,
fatal: mixed reset is not allowed in a bare repository, use --soft
To tell the user what they can do, instead of just what they can't.
---
 builtin/reset.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 1283068..ebbefe0 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -332,7 +332,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		setup_work_tree();
 
 	if (reset_type == MIXED && is_bare_repository())
-		die("%s reset is not allowed in a bare repository",
+		die("%s reset is not allowed in a bare repository, use --soft",
 		    reset_type_names[reset_type]);
 
 	/* Soft reset does not touch the index file nor the working tree
-- 
1.7.1

[PATCH v2] reset: Better warning message on git reset --mixed <paths>

[Ævar Arnfjörð Bjarmason]

When you call "git reset --mixed <paths>" git will complain that using
mixed with paths is deprecated:

    warning: --mixed option is deprecated with paths.

That doesn't tell the user why it's deprecated, or what he should use
instead. Expand on the warning and tell the user to just omit --mixed:

    warning: --mixed with paths is deprecated; use 'git reset -- <paths>' instead

The exact wording of the warning was suggested by Jonathan Nieder.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Sat, Aug 14, 2010 at 21:05, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Maybe:
>
>  warning: --mixed with paths is deprecated; use 'git reset -- <paths>' instead

That's better, thanks. Here's an amended version, and with tests this
time.

 builtin/reset.c           |    2 +-
 t/t7112-reset-messages.sh |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletions(-)
 create mode 100755 t/t7112-reset-messages.sh

diff --git a/builtin/reset.c b/builtin/reset.c
index 1283068..0037be4 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -318,7 +318,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	 * affecting the working tree nor HEAD. */
 	if (i < argc) {
 		if (reset_type == MIXED)
-			warning("--mixed option is deprecated with paths.");
+			warning("--mixed with paths is deprecated; use 'git reset -- <paths>' instead.");
 		else if (reset_type != NONE)
 			die("Cannot do %s reset with paths.",
 					reset_type_names[reset_type]);
diff --git a/t/t7112-reset-messages.sh b/t/t7112-reset-messages.sh
new file mode 100755
index 0000000..6f2669b
--- /dev/null
+++ b/t/t7112-reset-messages.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Ævar Arnfjörð Bjarmason
+#
+
+test_description='git reset warning and error messages'
+
+. ./test-lib.sh
+
+test_expect_success 'setup {err,out}-expect' "
+	cat >err-expect <<EOF &&
+warning: --mixed with paths is deprecated; use 'git reset -- <paths>' instead.
+EOF
+	cat >out-expect <<EOF
+Unstaged changes after reset:
+M	hlagh
+EOF
+"
+
+test_expect_success 'git reset --mixed <paths> warning' '
+	# Not test_commit() due to "ambiguous argument [..] both revision
+	# and filename"
+	echo stuff >hlagh &&
+	git add hlagh &&
+	git commit -m"adding stuff" hlagh &&
+	echo more stuff >hlagh &&
+	git add hlagh &&
+	test_must_fail git reset --mixed hlagh >out 2>err &&
+	test_cmp err-expect err &&
+	test_cmp out-expect out
+'
+
+test_done
-- 
1.7.2.1.339.gfad93

Re: [PATCH v2] reset: Better warning message on git reset --mixed <paths>

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> When you call "git reset --mixed <paths>" git will complain that using
> mixed with paths is deprecated:
>
>     warning: --mixed option is deprecated with paths.
>
> That doesn't tell the user why it's deprecated, or what he should use
> instead. Expand on the warning and tell the user to just omit --mixed:
>
>     warning: --mixed with paths is deprecated; use 'git reset -- <paths>' instead
>
> The exact wording of the warning was suggested by Jonathan Nieder.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> On Sat, Aug 14, 2010 at 21:05, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Maybe:
>>
>>  warning: --mixed with paths is deprecated; use 'git reset -- <paths>' instead
>
> That's better, thanks. Here's an amended version, and with tests this
> time.

While the new message is an improvement, I do not think the added test
that checks the exact wording is good.

Think for 5 seconds what the expected code change would be to break that
test.  I can only think of two realistic cases.  (1) Command line parsing
is broken and "reset --mixed <pathspec>" does not go through the codepath
that produces this warning anymore; (2) we deem that we had the
deprecation long enough and error out on this usage; or (3) Somebody comes
up with an even better wording but the string in this test still expects
suboptimal warning.

When we change this to an error, that is a behaviour change (it will
change the exit status as well), and it would be Ok to force the person
who does such a change to update the test.  But (3) shows that this test
is making it harder for people to improve the wording than necessary;
isn't it sufficient to check if any warning is issued at all?

I personally do not think this deserves to consume a new test number,
which is rather a scarce resource.

> diff --git a/t/t7112-reset-messages.sh b/t/t7112-reset-messages.sh
> new file mode 100755
> index 0000000..6f2669b
> --- /dev/null
> +++ b/t/t7112-reset-messages.sh
> @@ -0,0 +1,33 @@
> ...
> +test_expect_success 'git reset --mixed <paths> warning' '
> +	# Not test_commit() due to "ambiguous argument [..] both revision
> +	# and filename"

Huh?

Re: [PATCH v2] reset: Better warning message on git reset --mixed <paths>

[Ævar Arnfjörð Bjarmason]

On Mon, Aug 16, 2010 at 03:39, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> When you call "git reset --mixed <paths>" git will complain that using
>> mixed with paths is deprecated:
>>
>>     warning: --mixed option is deprecated with paths.
>>
>> That doesn't tell the user why it's deprecated, or what he should use
>> instead. Expand on the warning and tell the user to just omit --mixed:
>>
>>     warning: --mixed with paths is deprecated; use 'git reset -- <paths>' instead
>>
>> The exact wording of the warning was suggested by Jonathan Nieder.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>
>> On Sat, Aug 14, 2010 at 21:05, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>> Maybe:
>>>
>>>  warning: --mixed with paths is deprecated; use 'git reset -- <paths>' instead
>>
>> That's better, thanks. Here's an amended version, and with tests this
>> time.
>
> While the new message is an improvement, I do not think the added test
> that checks the exact wording is good.
>
> Think for 5 seconds what the expected code change would be to break that
> test.  I can only think of two realistic cases.  (1) Command line parsing
> is broken and "reset --mixed <pathspec>" does not go through the codepath
> that produces this warning anymore; (2) we deem that we had the
> deprecation long enough and error out on this usage; or (3) Somebody comes
> up with an even better wording but the string in this test still expects
> suboptimal warning.
>
> When we change this to an error, that is a behaviour change (it will
> change the exit status as well), and it would be Ok to force the person
> who does such a change to update the test.  But (3) shows that this test
> is making it harder for people to improve the wording than necessary;
> isn't it sufficient to check if any warning is issued at all?

I thought we might test for deprecation warnings, since they're a step
above general warnings as they imply a deprecation cycle.

I'm just used to a codebase where every single warning message of that
sort is explicitly tested for, so I mostly did that out of habit.

> I personally do not think this deserves to consume a new test number,
> which is rather a scarce resource.

Just drop the test file from the patch, I don't think it's really
needed, and maybe drop the patch altogether. I don't know what we want
to do about this reset behavior in general.

>> diff --git a/t/t7112-reset-messages.sh b/t/t7112-reset-messages.sh
>> new file mode 100755
>> index 0000000..6f2669b
>> --- /dev/null
>> +++ b/t/t7112-reset-messages.sh
>> @@ -0,0 +1,33 @@
>> ...
>> +test_expect_success 'git reset --mixed <paths> warning' '
>> +     # Not test_commit() due to "ambiguous argument [..] both revision
>> +     # and filename"
>
> Huh?

I think I forgot that test_commit could take two args there.