[RFC PATCH] stash: accept options also when subcommand 'save' is omitted

[RFC PATCH] stash: accept options also when subcommand 'save' is omitted

[Matthieu Moy]

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

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 2f5ca7b..6f251e7 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 'git stash' drop [-q|--quiet] [<stash>]
 'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
 'git stash' branch <branchname> [<stash>]
-'git stash' [save [--keep-index] [-q|--quiet] [<message>]]
+'git stash' [save] [--keep-index] [-q|--quiet] [<message>]
 'git stash' clear
 'git stash' create
 
diff --git a/git-stash.sh b/git-stash.sh
index 03e589f..2599410 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -7,7 +7,7 @@ USAGE="list [<options>]
    or: $dashless drop [-q|--quiet] [<stash>]
    or: $dashless ( pop | apply ) [--index] [-q|--quiet] [<stash>]
    or: $dashless branch <branchname> [<stash>]
-   or: $dashless [save [--keep-index] [-q|--quiet] [<message>]]
+   or: $dashless [save] [--keep-index] [-q|--quiet] [<message>]
    or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -302,6 +302,12 @@ apply_to_branch () {
 	drop_stash $stash
 }
 
+case "$1" in
+    -*)
+	set "save" "$@"
+	;;
+esac
+
 # Main command set
 case "$1" in
 list)
-- 
1.6.4.173.g0591

Re: [RFC PATCH] stash: accept options also when subcommand 'save' is omitted

[Matthieu Moy]

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

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

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

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

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

Re: [RFC PATCH] stash: accept options also when subcommand 'save' is omitted

[Jeff King]

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

> 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.
>
> [...]
> 
> 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.

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

> But I may have missed its drawbacks ;-)

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

Re: [RFC PATCH] stash: accept options also when subcommand 'save' is omitted

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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".
>
>> But I may have missed its drawbacks ;-)
>
> 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 '-').

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

[PATCH 0/3] short syntaxes for 'git stash'

[Matthieu Moy]

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.

[PATCH 1/3] stash: accept -k as a shortcut for --keep-index

[Matthieu Moy]

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

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 4b15459..1b5392a 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 'git stash' drop [-q|--quiet] [<stash>]
 'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
 'git stash' branch <branchname> [<stash>]
-'git stash' [save [--patch] [--[no-]keep-index] [-q|--quiet] [<message>]]
+'git stash' [save [--patch] [--[no-]keep-index|-k] [-q|--quiet] [<message>]]
 'git stash' clear
 'git stash' create
 
@@ -42,7 +42,7 @@ is also possible).
 OPTIONS
 -------
 
-save [--patch] [--[no-]keep-index] [-q|--quiet] [<message>]::
+save [--patch] [--[no-]keep-index|-k] [-q|--quiet] [<message>]::
 
 	Save your local modifications to a new 'stash', and run `git reset
 	--hard` to revert them.  This is the default action when no
diff --git a/git-stash.sh b/git-stash.sh
index 567aa5d..856a2d5 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -7,7 +7,7 @@ USAGE="list [<options>]
    or: $dashless drop [-q|--quiet] [<stash>]
    or: $dashless ( pop | apply ) [--index] [-q|--quiet] [<stash>]
    or: $dashless branch <branchname> [<stash>]
-   or: $dashless [save [--keep-index] [-q|--quiet] [<message>]]
+   or: $dashless [save [-k|--keep-index] [-q|--quiet] [<message>]]
    or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -132,7 +132,7 @@ save_stash () {
 	while test $# != 0
 	do
 		case "$1" in
-		--keep-index)
+		-k|--keep-index)
 			keep_index=t
 			;;
 		--no-keep-index)
-- 
1.6.4.rc2.31.g2d7d7

[PATCH 2/3] stash: accept options also when subcommand 'save' is omitted

[Matthieu Moy]

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

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 1b5392a..7e515ce 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 'git stash' drop [-q|--quiet] [<stash>]
 'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
 'git stash' branch <branchname> [<stash>]
-'git stash' [save [--patch] [--[no-]keep-index|-k] [-q|--quiet] [<message>]]
+'git stash' [save] [--patch] [--[no-]keep-index|-k] [-q|--quiet] [<message>]
 'git stash' clear
 'git stash' create
 
diff --git a/git-stash.sh b/git-stash.sh
index 856a2d5..bb36bc7 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -7,7 +7,7 @@ USAGE="list [<options>]
    or: $dashless drop [-q|--quiet] [<stash>]
    or: $dashless ( pop | apply ) [--index] [-q|--quiet] [<stash>]
    or: $dashless branch <branchname> [<stash>]
-   or: $dashless [save [-k|--keep-index] [-q|--quiet] [<message>]]
+   or: $dashless [save] [-k|--keep-index] [-q|--quiet] [<message>]
    or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -354,6 +354,12 @@ apply_to_branch () {
 	drop_stash $stash
 }
 
+case "$1" in
+    -*)
+	set "save" "$@"
+	;;
+esac
+
 # Main command set
 case "$1" in
 list)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 7a3fb67..0e831e0 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -200,4 +200,12 @@ test_expect_success 'drop -q is quiet' '
 	test ! -s output.out
 '
 
+test_expect_success 'stash -k' '
+       echo bar3 > file &&
+       echo bar4 > file2 &&
+       git add file2 &&
+       git stash -k &&
+       test bar,bar4 = $(cat file),$(cat file2)
+'
+
 test_done
-- 
1.6.4.rc2.31.g2d7d7

[PATCH 3/3] stash: reject stash name starting with a dash.

[Matthieu Moy]

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:

>> But I may have missed its drawbacks ;-)
>
> 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

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

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 7e515ce..ded62e0 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 'git stash' drop [-q|--quiet] [<stash>]
 'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
 'git stash' branch <branchname> [<stash>]
-'git stash' [save] [--patch] [--[no-]keep-index|-k] [-q|--quiet] [<message>]
+'git stash' [save] [--patch] [--[no-]keep-index|-k] [-q|--quiet] [--] [<message>]
 'git stash' clear
 'git stash' create
 
diff --git a/git-stash.sh b/git-stash.sh
index bb36bc7..642e265 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -7,7 +7,7 @@ USAGE="list [<options>]
    or: $dashless drop [-q|--quiet] [<stash>]
    or: $dashless ( pop | apply ) [--index] [-q|--quiet] [<stash>]
    or: $dashless branch <branchname> [<stash>]
-   or: $dashless [save] [-k|--keep-index] [-q|--quiet] [<message>]
+   or: $dashless [save] [-k|--keep-index] [-q|--quiet] [--] [<message>]
    or: $dashless clear"
 
 SUBDIRECTORY_OK=Yes
@@ -145,6 +145,14 @@ save_stash () {
 		-q|--quiet)
 			GIT_QUIET=t
 			;;
+		--)
+			shift
+			break
+			;;
+		-*)
+			echo "error: unknown option for 'stash save': $1"
+			usage
+			;;
 		*)
 			break
 			;;
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 0e831e0..87e5a14 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -208,4 +208,14 @@ test_expect_success 'stash -k' '
        test bar,bar4 = $(cat file),$(cat file2)
 '
 
+test_expect_success 'stash --invalid-option' '
+       echo bar5 > file &&
+       echo bar6 > file2 &&
+       git add file2 &&
+       ! git stash --invalid-option &&
+       test bar5,bar6 = $(cat file),$(cat file2) &&
+       git stash -- -message-starting-with-dash &&
+       test bar,bar2 = $(cat file),$(cat file2)
+'
+
 test_done
-- 
1.6.4.rc2.31.g2d7d7

Re: [RFC PATCH] stash: accept options also when subcommand 'save' is omitted

[Johannes Schindelin]

Hi,

On Tue, 18 Aug 2009, Jeff King wrote:

> On Tue, Aug 18, 2009 at 03:01:52PM +0200, Matthieu Moy wrote:
> 
> > 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.
> >
> > [...]
> > 
> > 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.
> 
> 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".

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

Re: [RFC PATCH] stash: accept options also when subcommand 'save' is omitted

[Matthieu Moy]

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

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

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.

> So please register my objection.

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

Re: [RFC PATCH] stash: accept options also when subcommand 'save' is omitted

[Jeff King]

On Tue, Aug 18, 2009 at 11:42:58PM +0200, Johannes Schindelin 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".
> 
> 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.

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

Re: [RFC PATCH] stash: accept options also when subcommand 'save' is omitted

[Johannes Schindelin]

Hi,

On Tue, 18 Aug 2009, Matthieu Moy wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > But it is sloppy, in that it blindly accepts options that might be 
> > valid for several subcommands, not just "save".
> 
> I wouldn't call that sloppy. 'save' is the default command, if you don't 
> provide any command, then 'save' will be used.

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

> > So please register my objection.
> 
> I will if you register my objection to yours ;-).
> 
> Jokes aside,

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

Re: [RFC PATCH] stash: accept options also when subcommand 'save' is omitted

[Johannes Schindelin]

Hi,

On Tue, 18 Aug 2009, Jeff King wrote:

> On Tue, Aug 18, 2009 at 11:42:58PM +0200, Johannes Schindelin 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".
> > 
> > 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.
> 
> 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.

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.

Re: [RFC PATCH] stash: accept options also when subcommand 'save' is omitted

[Jeff King]

On Wed, Aug 19, 2009 at 12:46:27AM +0200, Johannes Schindelin 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!

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.

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

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.

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

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

Re: [PATCH 0/3] short syntaxes for 'git stash'

[Jeff King]

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

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

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

Re: [PATCH 3/3] stash: reject stash name starting with a dash.

[Jeff King]

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

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

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.

> +test_expect_success 'stash --invalid-option' '
> +       echo bar5 > file &&
> +       echo bar6 > file2 &&
> +       git add file2 &&
> +       ! git stash --invalid-option &&
> +       test bar5,bar6 = $(cat file),$(cat file2) &&
> +       git stash -- -message-starting-with-dash &&
> +       test bar,bar2 = $(cat file),$(cat file2)
> +'

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

Re: [PATCH 3/3] stash: reject stash name starting with a dash.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Aug 18, 2009 at 11:38:43PM +0200, Matthieu Moy wrote:
>
>> 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.
>
> 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.

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.

>> +test_expect_success 'stash --invalid-option' '
>> +       echo bar5 > file &&
>> +       echo bar6 > file2 &&
>> +       git add file2 &&
>> +       ! git stash --invalid-option &&
>> +       test bar5,bar6 = $(cat file),$(cat file2) &&
>> +       git stash -- -message-starting-with-dash &&
>> +       test bar,bar2 = $(cat file),$(cat file2)
>> +'
>
> 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"?

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.

Re: [PATCH 3/3] stash: reject stash name starting with a dash.

[Jeff King]

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

> > 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"?
> 
> 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.

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

Re: [PATCH 3/3] stash: reject stash name starting with a dash.

[Junio C Hamano]

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

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

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.

Re: [PATCH 3/3] stash: reject stash name starting with a dash.

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> On Tue, Aug 18, 2009 at 11:38:43PM +0200, Matthieu Moy wrote:
>
>> 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.
>
> 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.

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

>> +test_expect_success 'stash --invalid-option' '
>> +       echo bar5 > file &&
>> +       echo bar6 > file2 &&
>> +       git add file2 &&
>> +       ! git stash --invalid-option &&
>> +       test bar5,bar6 = $(cat file),$(cat file2) &&
>> +       git stash -- -message-starting-with-dash &&
>> +       test bar,bar2 = $(cat file),$(cat file2)
>> +'
>
> 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"?

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.

-- 
Matthieu

Re: [RFC PATCH] stash: accept options also when subcommand 'save' is omitted

[Matthieu Moy]

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

> Hi,
>
> On Tue, 18 Aug 2009, Matthieu Moy wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > But it is sloppy, in that it blindly accepts options that might be 
>> > valid for several subcommands, not just "save".
>> 
>> I wouldn't call that sloppy. 'save' is the default command, if you don't 
>> provide any command, then 'save' will be used.
>
> '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.

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.

-- 
Matthieu

Re: [PATCH 3/3] stash: reject stash name starting with a dash.

[Jeff King]

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

> >> +test_expect_success 'stash --invalid-option' '
> >> +       echo bar5 > file &&
> >> +       echo bar6 > file2 &&
> >> +       git add file2 &&
> >> +       ! git stash --invalid-option &&
> >> +       test bar5,bar6 = $(cat file),$(cat file2) &&
> >> +       git stash -- -message-starting-with-dash &&
> >> +       test bar,bar2 = $(cat file),$(cat file2)
> >> +'
> >
> > 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"?
> 
> 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.

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