RFC patchstash: accept options also when subcommand 'save' is omitted

This allows in particular 'git stash --keep-index' which is shorter than 'git stash save --keep-index', and not ambiguous.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Does this sound right?

I'm so used to 'git stash' (without saying 'save') that I keep typing 'git stash --keep-index', and get a usage string as an error message.

 Documentation/git-stash.txt |    2 +-
 git-stash.sh                |    8 +++++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

Hmm, googling a bit, I just noticed that there's already something in pu: ea41cfc4f (Make 'git stash -k' a short form for 'git stash save --keep-index') which also does the trick, while adding a -k alias for --keep-index.

Not sure which hack is best between my

And the proposed

 *)
-	if test $# -eq 0
-	then
-		save_stash &&
+	case $#,"$1" in
+	0,|1,-k|1,--keep-index)
+		save_stash "$@" &&
 		say '(To restore them type "git stash apply")'
-	else
+		;;
+	*)
 		usage
-	fi
+	esac
 	;;
 esac

Mine has at least two advantages:

* It won't require changing the code again when new options are added
  to 'git stash save'.

* It works with 'git stash -k -q' for example, while the other
  proposal checks that $# == 1, which won't work if there are more
  than one option.

But I may have missed its drawbacks ;-)

-- Matthieu

On Tue, Aug 18, 2009 at 03:01:52PM +0200, Matthieu Moy wrote:

I think yours is nicer, especially as we have just added the '-p|--patch' option, as well. With what is there now, you can do "git stash -p", but not "git stash -p -k".

The only I can think of is that bogus input will provoke 'save'. So something like:

  git stash --apply

will invoke "git stash save --apply", which doesn't even complain. It just tries to make a stash with message --apply. Now of course this input is obviously bogus, but probably the right thing to do is complain.

OTOH, I think it is the fault of "git stash save --apply" for not doing the complaining, so your patch really isn't making it worse. Probably it should barf on anything unrecognized starting with a '-', and allow '--' to separate the message from the rest of the options (in the rare case that you want a message starting with '-').

-Peff

Jeff King <peff@peff.net> writes:

Sounds like a sane approach.

I am Ok with the idea of queuing a patch to update js/stash-dwim ea41cfc (Make 'git stash -k' a short form for 'git stash save --keep-index', 2009-07-27) with an implementation along this line; the "stash -p" one f300fab (DWIM 'git stash save -p' for 'git stash -p', 2009-08-13) also may need to adjusted (it could become no-op, I suspect).

This small patch serie is based on the following commit in pu:

dda1f2a Implement 'git stash save --patch'

It is meant to replace two commits already there:

ea41cfc Make 'git stash -k' a short form for 'git stash save --keep-index' f300fab DWIM 'git stash save -p' for 'git stash -p'

The first (git stash -k) has the drawback of forcing one to use one and only one option when the 'save' command is ommited. My approach is mostly based on the simple hack:

+case "$1" in
+    -*)
+       set "save" "$@"
+       ;;
+esac
+

and accepts an arbitrary number of options.

The second (DWIM git stash save -p) becomes unnecessary afterwards.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-stash.txt |    4 ++--
 git-stash.sh                |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

This allows in particular 'git stash -k which is shorter than 'git stash save -k', and not ambiguous.

Testcase taken from Johannes Schindelin <johannes.schindelin@gmx.de>.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-stash.txt |    2 +-
 git-stash.sh                |    8 +++++++-
 t/t3903-stash.sh            |    8 ++++++++
 3 files changed, 16 insertions(+), 2 deletions(-)

This avoids typos like 'git stash save --invalid-option', particularly nasty since one can omit the 'save' subcommand. The syntax 'git stash save -- "-name starting with dash" still allows such stash name.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Jeff King <peff@peff.net> writes:

So, here it is!

 Documentation/git-stash.txt |    2 +-
 git-stash.sh                |   10 +++++++++-
 t/t3903-stash.sh            |   10 ++++++++++
 3 files changed, 20 insertions(+), 2 deletions(-)

On Tue, Aug 18, 2009 at 11:38:43PM +0200, Matthieu Moy wrote:

Aside from the documentation and usage lines, this one is actually independent of the other two, and I think makes sense regardless of what happens.

Should this actually be "git stash save --invalid-option", since it is really testing the actual save option parsing, and not the behavior to automatically push options to "git stash save"?

Other than that, patch looks good.

-Peff

Jeff King <peff@peff.net> writes:

Yes, and I'd rather see it near 'maint' even if we were to (mind you, we won't) reject all changes in 'next' on this program.

It would be ideal if

	git stash save --invalid-option

failed, while

	git stash --invalid-option should be trapped

and/or

	git stash "--invalid-option should be trapped"

are accepted as valid quickie ways to name a WIP stash before attending to an unrelated emergency.

On Tue, Aug 18, 2009 at 04:45:40PM -0700, Junio C Hamano wrote:

I thought that was the exact DWIM that was rejected previously so that you don't accidentally make commits with "git stash llist" or "git stash --almost-a-valid-optio".

-Peff

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

Meh, let me take it back. I somehow was living in 13 months ago for a short while when I wrote the above.

9488e87 (git-stash: require "save" to be explicit and update documentation, 2007-07-01) made the latter two invalid.

Jeff King <peff@peff.net> writes:

I also do (but I'm not sure this is important enough to be in maint, especially since it'll introduce silly textual conflicts in docs when merging).

It could be, but the most annoying DWIM would be the "implicit save" case, so that's the one I'm testing. One could test both, but that'd probably be a bit overkill.

On Wed, Aug 19, 2009 at 08:57:05AM +0200, Matthieu Moy wrote:

But if your proposal to accept any "-*' is not accepted, then it is not testing your added code at all, is it? Even without the rest of the patch, the test would pass.

-Peff

On Tue, Aug 18, 2009 at 11:38:40PM +0200, Matthieu Moy wrote:

Actually, these are already in 'next', so they can't be simply replaced. So you would need to re-roll patches 1 and 2 at the very least.

However, thinking more on it, I think we can address Dscho's concern with your proposal to accept only a limited set of options. And looking at what's in f300fab, it actually does make an attempt to allow multiple options, but it doesn't cover all cases (e.g., I can use "-p --no-keep-index" but not "--no-keep-index -p". Nor can I do "-p -k"; even though "-k" is implied by "-p", you will get a very strange usage mention instead of it being a silent no-op).

So there are two issues:

  - refactoring to allow arbitrary combinations of -k/-p and variants.

  - allowing other options; I believe "-q" is the only one. That seems
    to be specific to Dscho's objection, as it is ambiguous with other
    subcommands. Though "-p" may also become ambiguous, if we get "stash
    apply -p" soon.

I think the first one should be fairly uncontroversial. I'm not sure about the second.

-Peff

Hi,

On Tue, 18 Aug 2009, Jeff King wrote:

But it is sloppy, in that it blindly accepts options that might be valid for several subcommands, not just "save".

That was the reason I did not implement it this way.

But we do not have such ambiguous options yet.

Or do we?  Look at what "list" accepts!

So please register my objection.

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

I wouldn't call that sloppy. 'save' is the default command, if you don't provide any command, then 'save' will be used.

So, "git stash -p" means "git stash save -p" regardless of the fact that there exists somewhere else a "git stash list -p". Actually, in the current form of pu, it is already the case since

f300fab (Thomas Rast, DWIM 'git stash save -p' for 'git stash -p')

which came right after your patch.

I will if you register my objection to yours ;-).

Jokes aside, if you insist in rejecting 'git stash -p', then my patch can be slightly modified to say something like

+case "$1" in
+    -k|--keep-index|--patch)
+       set "save" "$@"
+       ;;
+esac

instead, which also allows multiple arguments (unlike your initial proposal), and can control more precisely the list of options for which 'save' is a default.

-- Matthieu

Hi,

On Tue, 18 Aug 2009, Matthieu Moy wrote:

'save' might be the default command, but we don't trigger it with _any_ crap, for a very good reason. Read the commit log for git-stash.sh to know why.

If you think that's funny, I don't. I actually put a lot of thought into the issue whether to allow _any_ parameter with looks like an option to trigger "save". And I very much had to come to the decision that no, that is too dangerous.

So I cannot take your objection seriously.

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

The good reason was people doing a typo when typing a command, like 'git stash aply' or so. And yes, I did find this annoying. The question of someone omitting the subcommand is very different to me. On can hardly type 'git stash -q' and claim he explicitely wanted to run 'pop'.

Your proposal will almost certainly trigger complains from users:

git stash -k => works git stash -k -q => doesn't work git stash -k "name of stash" => doesn't work git stash save -k "name of stash" => works git stash -p => works with another patch merged in next git stash -q => doesn't work git stash --patch --no-keep-index => works git stash --no-keep-index --patch => doesn't work

You'll have a hard time explaining that to bare mortals.

Look at what's the code's becomming:

+       0,,|1,-k,|1,--keep-index,|1,-p,|1,--patch,|2,-p,--no-keep-index|2,--patch,--no-keep-index)

when the code starts having to enumerate so many possibilities instead of having a simple logic, something's going wrong.

On Tue, Aug 18, 2009 at 11:42:58PM +0200, Johannes Schindelin wrote:

I don't see the problem. Either the option works for "stash save" or it does not. If I say "git stash --quiet", then it _must_ be "git stash save --quiet", and not "git stash pop --quiet", because "save" is the only default command. If I say "git stash --foobar", it is translated to "git stash save --foobar", which should generate an error (it doesn't right now, but that is a separate issue). I don't see any confusion: "save" is always the default command, unless one is given as the first argument.

The place where I would see a potential problem is if stash grew any "global" options (e.g., in the same way that "git" can take options before its subcommand name).

-Peff

Hi,

On Tue, 18 Aug 2009, Jeff King wrote:

So if I say "git stash -q" by mistake, but wanted to say "git stash drop -q", then I am borked?

Bah!  I say: bah!

You're basically reintroducing at least part of the DWIMery that was reverted in 9488e875 and I have the distinct feeling that some people in this thread do not think hard enough about what would adher to the principle of least surprise even in the future (or even for people introducing other stash save options).

Well, you just go ahead and push through your patch, and I do what I promised on my blog.

On Wed, Aug 19, 2009 at 12:46:27AM +0200, Johannes Schindelin wrote:

Yep, though it is only one of many borkings that currently exist. Try:

  # oops, what was the name of that option?
  git stash save --index

  # does apply take --patch, too?
  git stash apply --patch

Still, other parts of the option parsing being awful aren't an excuse to mess this one up. So I see your point.

I don't think it is quite as bad as that, as arbitrary crap will not get passed through, only crap that looks like options. Which is perhaps a step up, but it is debatable how much.

I think Matthieu's proposal to be strict about matching the options, but to take multiple options is probably the best bet for now. As it is now, accepting "git stash -k" or "git stash -p" but not "git stash -k -p" is pretty counterintuitive. But with explicit matching it should be no less safe than it is now.

I don't know why you need to be so confrontational. I don't even have a patch to "push through". I just said "I don't see the problem", and I'm glad you brought up the previous stash behavior, because I had forgotten the pain it caused. And that is why discussing on a public forum is nice; it lets people contribute things that others might have missed.

-Peff