patchreset: Better warning message on git reset --mixed <paths>

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:

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(-)

Ævar Arnfjörð Bjarmason wrote:

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> --”.

Maybe:

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

Jonathan Nieder <jrnieder@gmail.com> writes:

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.

Junio C Hamano <gitster@pobox.com> writes:

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

On Sun, Aug 15, 2010 at 02:43, Miles Bader <miles@gnu.org> wrote:

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.

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?

Junio C Hamano wrote:

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.

Jonathan Nieder <jrnieder@gmail.com> writes:

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.

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

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.

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

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.

On Mon, Aug 16, 2010 at 5:51 AM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

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

On Mon, Aug 16, 2010 at 00:59, Miles Bader <miles@gnu.org> wrote:

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.

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

Ralf Ebert wrote:

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.

On Sun, Aug 15, 2010 at 11:43:59AM +0900, Miles Bader wrote:

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

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:

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

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

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.

Huh?

On Mon, Aug 16, 2010 at 03:39, Junio C Hamano <gitster@pobox.com> wrote:

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.

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.

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