docs: fix git stash grammar

From: Quentin Bernet <[email protected]>

Grammar incorrectly did not include git stash -m and other valid git stash push flags

Signed-off-by Quentin Bernet <[email protected]>

---
Fix git stash grammar

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2255%2FSporarum%2Fpatch-1-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2255/Sporarum/patch-1-v1
Pull-Request: https://github.com/git/git/pull/2255

 Documentation/git-stash.adoc | 4 ++--
 builtin/stash.c              | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

Documentation/git-stash.adoc +2 -2

index 235d57ddd8..41086c21a4 100644
@@ -14,10 +14,10 @@ git stash drop [-q | --quiet] [<stash>]
 git stash pop [--index] [-q | --quiet] [<stash>]
 git stash apply [--index] [-q | --quiet] [<stash>]
 git stash branch <branchname> [<stash>]
-git stash [push [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet]
+git stash [push] [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet]
             [-u | --include-untracked] [-a | --all] [(-m | --message) <message>]
             [--pathspec-from-file=<file> [--pathspec-file-nul]]
-            [--] [<pathspec>...]]
+            [--] [<pathspec>...]
 git stash save [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet]
            [-u | --include-untracked] [-a | --all] [<message>]
 git stash clear

builtin/stash.c +2 -2

index 95c5005b0b..0d27b2fb1f 100644
@@ -50,10 +50,10 @@
 #define BUILTIN_STASH_STORE_USAGE \
        N_("git stash store [(-m | --message) <message>] [-q | --quiet] <commit>")
 #define BUILTIN_STASH_PUSH_USAGE \
-       N_("git stash [push [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet]\n" \
+       N_("git stash [push] [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet]\n" \
           "          [-u | --include-untracked] [-a | --all] [(-m | --message) <message>]\n" \
           "          [--pathspec-from-file=<file> [--pathspec-file-nul]]\n" \
-          "          [--] [<pathspec>...]]")
+          "          [--] [<pathspec>...]")
 #define BUILTIN_STASH_SAVE_USAGE \
        N_("git stash save [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet]\n" \
           "          [-u | --include-untracked] [-a | --all] [<message>]")

base-commit: ce74208c2fa13943fffa58f168ac27a76d0eb789 --
gitgitgadget

Now, _is_ the grammar bracketed wrong? "git help stash" says

For quickly making a snapshot, you can omit "push". In this mode, non-option arguments are not allowed to prevent a misspelled subcommand from making an unwanted stash entry. The two exceptions to this are stash -p which acts as alias for stash push -p and pathspec elements, which are allowed after a double hyphen -- for disambiguation.

So _if_ you want to provide options (other than "-p"), the "push" is required. I think the existing brackets indicate that.

Quoted 11 lines · D. Ben Knoble

Now, _is_ the grammar bracketed wrong? "git help stash" says

For quickly making a snapshot, you can omit "push". In this mode, non-option arguments are not allowed to prevent a misspelled subcommand from making an unwanted stash entry. The two exceptions to this are stash -p which acts as alias for stash push -p and pathspec elements, which are allowed after a double hyphen -- for disambiguation.

So _if_ you want to provide options (other than "-p"), the "push" is required. I think the existing brackets indicate that.

When it says "In this mode, non-option arguments are not allowed" wouldn't -m be allowed as it is an option and not a non-option? In fact if we do try to run "git stash -m something" it does correctly stash while if we do something like "git stash pathspec" it does give back "fatal: subcommand wasn't specified; 'push' can't be assumed due to unexpected token 'pathspec'".

If that is the case then there is an issue with the way the usage tooltip shows the optionality of "push".

Quoted 19 lines · Mirko Faina

Quoted 11 lines · D. Ben Knoble

Now, _is_ the grammar bracketed wrong? "git help stash" says

For quickly making a snapshot, you can omit "push". In this mode, non-option arguments are not allowed to prevent a misspelled subcommand from making an unwanted stash entry. The two exceptions to this are stash -p which acts as alias for stash push -p and pathspec elements, which are allowed after a double hyphen -- for disambiguation.

So _if_ you want to provide options (other than "-p"), the "push" is required. I think the existing brackets indicate that.

When it says "In this mode, non-option arguments are not allowed" wouldn't -m be allowed as it is an option and not a non-option? In fact if we do try to run "git stash -m something" it does correctly stash while if we do something like "git stash pathspec" it does give back "fatal: subcommand wasn't specified; 'push' can't be assumed due to unexpected token 'pathspec'".

Quoted 2 lines

If that is the case then there is an issue with the way the usage tooltip shows the optionality of "push".

Yup, you're right. The current SYNOPSIS suggests that you can omit and say "git stash" and it does the "push" thing, but when you want to give any "push" related options, the command name "push" becomes mandatory before them.

If the log message said something like

The "[optionality]" bracket is misplaced on the command line for "git stash push" in the synopsis section. It is not like you can omit "push" only when you do not give any options and arguments.

we wouldn't be having this long thread, I suspect.

My previous, simpler, grammar included "git stash <pathspec>" even though it is not valid.

Quoted 2 lines · Quentin Bernet

My previous, simpler, grammar included "git stash <pathspec>" even though it is not valid.

"git stash -- path1 path2 ..." is valid and is taken as elliding the subcommand name 'push'. So I think your previous one is good enough but the command line description needs to mention that. Double dash is used to signal the start of pathspec when ambiguous, and it is a good example to highlight when it is useful. Without disambiguating "--", "path1" might be misspelt subcommand name but we cannot tell which one. You may even have a file whose name is "pop", and "git stash -- pop" would be a way to save away the changes to the file without having to say "push" explicitly ;-)

Just so we're clear: my previous grammar did allow "git stash path1 path2" (no "push", no "--"), which is not allowed
Do you still think it is good enough ?

Quoted 3 lines · Quentin Bernet

Just so we're clear: my previous grammar did allow "git stash path1 path2" (no "push", no "--"), which is not allowed
Do you still think it is good enough ?

Absolutely.

It is not huge enough issue to warrant separating synopsis form for push into two almost identical pairs, and having to maintain both of them. It would be sufficient to make sure the readers understand that they may have to disambiguate pathspec with "--" like any other command (like "git log foo" when you have both file "foo" and tag "foo") in the description part.

One thing that we may want to fix in the code is that we probably do not want to require "--" when doing something like

git stash -m foo <pathspec>

to assume that it is a lazy "push" that does not say "push". Currently we complain against the lack of subcommand name.

Once "-m" (i.e., a potential option) is seen before seeing any subcommand, we can commit to treat it as a lazy "push", and have the parser pretend as if we got

git stash push -m foo <pathspec>

We may see "git stash -m foo --no-such-option garbage arge" and the option and argument parser of "git stash push" becomes responsible for complaining about such a command line.

#leftoverbits obviously.

I'm assuming "#leftoverbits" means "this is something that would be nice, but which is for another time"
And I'll leave the pleasure of implementing that to someone more well-versed in C and git internals !

Therefore my previous change (current status of the github PR) works, as a reminder, here it is:
...

Documentation/git-stash.adoc +0 -0

...
  git stash clear
...

Is there something else I should do ?

Quoted 4 lines

... It would be sufficient to make sure the readers understand that they may have to disambiguate pathspec with "--" like any other command (like "git log foo" when you have both file "foo" and tag "foo") in the description part.

Quoted 11 lines · D. Ben Knoble

Now, _is_ the grammar bracketed wrong? "git help stash" says

For quickly making a snapshot, you can omit "push". In this mode, non-option arguments are not allowed to prevent a misspelled subcommand from making an unwanted stash entry. The two exceptions to this are stash -p which acts as alias for stash push -p and pathspec elements, which are allowed after a double hyphen -- for disambiguation.

So _if_ you want to provide options (other than "-p"), the "push" is required. I think the existing brackets indicate that.

When it says "In this mode, non-option arguments are not allowed" wouldn't -m be allowed as it is an option and not a non-option? In fact if we do try to run "git stash -m something" it does correctly stash while if we do something like "git stash pathspec" it does give back "fatal: subcommand wasn't specified; 'push' can't be assumed due to unexpected token 'pathspec'".

If that is the case then there is an issue with the way the usage tooltip shows the optionality of "push".

Quoted 4 lines · Quentin Bernet via GitGitGadget

From: Quentin Bernet <[email protected]>

Grammar incorrectly did not include git stash -m and other valid git stash push flags

Typically commits for Git describe
- the current situation (present tense)
- commands to make the code better

In this case, something like

The grammar for "git stash" is incorrectly bracketed; fix it.

?

Quoted 47 lines

Signed-off-by Quentin Bernet <[email protected]>

---
Fix git stash grammar

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2255%2FSporarum%2Fpatch-1-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2255/Sporarum/patch-1-v1
Pull-Request: https://github.com/git/git/pull/2255

  Documentation/git-stash.adoc | 4 ++--
  builtin/stash.c              | 4 ++--
  2 files changed, 4 insertions(+), 4 deletions(-)

Documentation/git-stash.adoc +2 -2

index 235d57ddd8..41086c21a4 100644
@@ -14,10 +14,10 @@ git stash drop [-q | --quiet] [<stash>]
  git stash pop [--index] [-q | --quiet] [<stash>]
  git stash apply [--index] [-q | --quiet] [<stash>]
  git stash branch <branchname> [<stash>]
-git stash [push [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet]
+git stash [push] [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet]
              [-u | --include-untracked] [-a | --all] [(-m | --message) <message>]
              [--pathspec-from-file=<file> [--pathspec-file-nul]]
-            [--] [<pathspec>...]]
+            [--] [<pathspec>...]
  git stash save [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet]
             [-u | --include-untracked] [-a | --all] [<message>]
  git stash clear

builtin/stash.c +2 -2

index 95c5005b0b..0d27b2fb1f 100644
@@ -50,10 +50,10 @@
  #define BUILTIN_STASH_STORE_USAGE \
         N_("git stash store [(-m | --message) <message>] [-q | --quiet] <commit>")
  #define BUILTIN_STASH_PUSH_USAGE \
-       N_("git stash [push [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet]\n" \
+       N_("git stash [push] [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet]\n" \
            "          [-u | --include-untracked] [-a | --all] [(-m | --message) <message>]\n" \
            "          [--pathspec-from-file=<file> [--pathspec-file-nul]]\n" \
-          "          [--] [<pathspec>...]]")
+          "          [--] [<pathspec>...]")
  #define BUILTIN_STASH_SAVE_USAGE \
         N_("git stash save [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet]\n" \
            "          [-u | --include-untracked] [-a | --all] [<message>]")

Quoted 3 lines

base-commit: ce74208c2fa13943fffa58f168ac27a76d0eb789 --
gitgitgadget

Now, _is_ the grammar bracketed wrong? "git help stash" says

For quickly making a snapshot, you can omit "push". In this mode, non-option arguments are not allowed to prevent a misspelled subcommand from making an unwanted stash entry. The two exceptions to this are stash -p which acts as alias for stash push -p and pathspec elements, which are allowed after a double hyphen -- for disambiguation.

So _if_ you want to provide options (other than "-p"), the "push" is required. I think the existing brackets indicate that.

But it also adds "-p" to the list of exceptions, even though it is an option argument.

#define BUILTIN_STASH_SAVE_USAGE \

builtin/stash.c +0 -0

 	N_("git stash save [-p | --patch] [-S | --staged] [-k | --[no-]keep-index] [-q | --quiet]\n" \
 	   "          [-u | --include-untracked] [-a | --all] [<message>]")

base-commit: ce74208c2fa13943fffa58f168ac27a76d0eb789

Looking ver good.  Does this excatly match what we have in the documentation?  I think we have a test for it, but I do not offhand recall it covers "git stash".

Thanks.  Will queue.