stash: eliminate crude option parsing

Eliminate crude option parsing and rely on real parsing instead, because
1) Crude parsing is crude, for example it's not capable of
   handling things like `git stash -m Message`
2) Adding options in two places is inconvenient and prone to bugs

As a side result, the case of `git stash -m Message` gets fixed.
Also give a good error message instead of just throwing usage at user.

----

Some review of what's been happening to this code:

Before [1], `git-stash.sh` only verified that all args begin with `-` :

	# The default command is "push" if nothing but options are given
	seen_non_option=
	for opt
	do
		case "$opt" in
		--) break ;;
		-*) ;;
		*) seen_non_option=t; break ;;
		esac
	done

Later, [1] introduced the duplicate code I'm now removing, also making
the previous test more strict by white-listing options.

----

[1] Commit 40af1468 ("stash: convert `stash--helper.c` into `stash.c`" 2019-02-26)

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/stash.c b/builtin/stash.c
index 4ad3adf4ba5a01d78f75dfc59b7ee99b25880e0c..7bc4c5d69657cc74b65c485d0bf2583cf1c61b9e 100644 (file)
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1448,8 +1448,10 @@ done:
        return ret;
 }
 
-static int push_stash(int argc, const char **argv, const char *prefix)
+static int push_stash(int argc, const char **argv, const char *prefix,
+                     int push_assumed)
 {
+       int force_assume = 0;
        int keep_index = -1;
        int patch_mode = 0;
        int include_untracked = 0;
@@ -1471,10 +1473,22 @@ static int push_stash(int argc, const char **argv, const char *prefix)
                OPT_END()
        };
 
-       if (argc)
+       if (argc) {
+               force_assume = !strcmp(argv[0], "-p");
                argc = parse_options(argc, argv, prefix, options,
                                     git_stash_push_usage,
-                                    0);
+                                    PARSE_OPT_KEEP_DASHDASH);
+       }
+
+       if (argc) {
+               if (!strcmp(argv[0], "--")) {
+                       argc--;
+                       argv++;
+               } else if (push_assumed && !force_assume) {
+                       die("subcommand wasn't specified; 'push' can't be assumed due to unexpected token '%s'",
+                           argv[0]);
+               }
+       }
 
        parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
                       prefix, argv);
@@ -1547,7 +1561,6 @@ static int use_builtin_stash(void)
 
 int cmd_stash(int argc, const char **argv, const char *prefix)
 {
-       int i = -1;
        pid_t pid = getpid();
        const char *index_file;
        struct argv_array args = ARGV_ARRAY_INIT;
@@ -1580,7 +1593,7 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
                    (uintmax_t)pid);
 
        if (!argc)
-               return !!push_stash(0, NULL, prefix);
+               return !!push_stash(0, NULL, prefix, 0);
        else if (!strcmp(argv[0], "apply"))
                return !!apply_stash(argc, argv, prefix);
        else if (!strcmp(argv[0], "clear"))
@@ -1600,45 +1613,15 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
        else if (!strcmp(argv[0], "create"))
                return !!create_stash(argc, argv, prefix);
        else if (!strcmp(argv[0], "push"))
-               return !!push_stash(argc, argv, prefix);
+               return !!push_stash(argc, argv, prefix, 0);
        else if (!strcmp(argv[0], "save"))
                return !!save_stash(argc, argv, prefix);
        else if (*argv[0] != '-')
                usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
                              git_stash_usage, options);
 
-       if (strcmp(argv[0], "-p")) {
-               while (++i < argc && strcmp(argv[i], "--")) {
-                       /*
-                        * `akpqu` is a string which contains all short options,
-                        * except `-m` which is verified separately.
-                        */
-                       if ((strlen(argv[i]) == 2) && *argv[i] == '-' &&
-                           strchr("akpqu", argv[i][1]))
-                               continue;
-
-                       if (!strcmp(argv[i], "--all") ||
-                           !strcmp(argv[i], "--keep-index") ||
-                           !strcmp(argv[i], "--no-keep-index") ||
-                           !strcmp(argv[i], "--patch") ||
-                           !strcmp(argv[i], "--quiet") ||
-                           !strcmp(argv[i], "--include-untracked"))
-                               continue;
-
-                       /*
-                        * `-m` and `--message=` are verified separately because
-                        * they need to be immediately followed by a string
-                        * (i.e.`-m"foobar"` or `--message="foobar"`).
-                        */
-                       if (starts_with(argv[i], "-m") ||
-                           starts_with(argv[i], "--message="))
-                               continue;
-
-                       usage_with_options(git_stash_usage, options);
-               }
-       }
-
+       /* Assume 'stash push' */
        argv_array_push(&args, "push");
        argv_array_pushv(&args, argv);
-       return !!push_stash(args.argc, args.argv, prefix);
+       return !!push_stash(args.argc, args.argv, prefix, 1);
 }

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index ea56e85e70d5d1c2f991689d85a26433674add1a..3ad23e2502bea0e861d21822e5ef6b3559e0f8bd 100755 (executable)
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -285,6 +285,11 @@ test_expect_success 'stash --no-keep-index' '
        test bar,bar2 = $(cat file),$(cat file2)
 '
 
+test_expect_success 'dont assume push with non-option args' '
+       test_must_fail git stash -q drop 2>err &&
+       test_i18ngrep -e "subcommand wasn'\''t specified; '\''push'\'' can'\''t be assumed due to unexpected token '\''drop'\''" err
+'
+
 test_expect_success 'stash --invalid-option' '
        echo bar5 >file &&
        echo bar6 >file2 &&