@matheusbsbs / matheus-bagliano / commits / 8c3713cede
Fork 0

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>

Alexandr Miloslavskiy committed Feb 17, 2020 at 17:25 UTC 8c3713cede71e4741dac546f787734627329f55b
2 files changed +26 -38
builtin/stash.c
+21 -38
@@ -1448,8 +1448,10 @@ done:
1448 return ret;
1449 }
1450
1451 -static int push_stash(int argc, const char **argv, const char *prefix)
1451 +static int push_stash(int argc, const char **argv, const char *prefix,
1452 + int push_assumed)
1453 {
1454 + int force_assume = 0;
1455 int keep_index = -1;
1456 int patch_mode = 0;
1457 int include_untracked = 0;
@@ -1471,10 +1473,22 @@ static int push_stash(int argc, const char **argv, const char *prefix)
1473 OPT_END()
1474 };
1475
1474 - if (argc)
1476 + if (argc) {
1477 + force_assume = !strcmp(argv[0], "-p");
1478 argc = parse_options(argc, argv, prefix, options,
1479 git_stash_push_usage,
1477 - 0);
1480 + PARSE_OPT_KEEP_DASHDASH);
1481 + }
1482 +
1483 + if (argc) {
1484 + if (!strcmp(argv[0], "--")) {
1485 + argc--;
1486 + argv++;
1487 + } else if (push_assumed && !force_assume) {
1488 + die("subcommand wasn't specified; 'push' can't be assumed due to unexpected token '%s'",
1489 + argv[0]);
1490 + }
1491 + }
1492
1493 parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
1494 prefix, argv);
@@ -1547,7 +1561,6 @@ static int use_builtin_stash(void)
1561
1562 int cmd_stash(int argc, const char **argv, const char *prefix)
1563 {
1550 - int i = -1;
1564 pid_t pid = getpid();
1565 const char *index_file;
1566 struct argv_array args = ARGV_ARRAY_INIT;
@@ -1580,7 +1593,7 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
1593 (uintmax_t)pid);
1594
1595 if (!argc)
1583 - return !!push_stash(0, NULL, prefix);
1596 + return !!push_stash(0, NULL, prefix, 0);
1597 else if (!strcmp(argv[0], "apply"))
1598 return !!apply_stash(argc, argv, prefix);
1599 else if (!strcmp(argv[0], "clear"))
@@ -1600,45 +1613,15 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
1613 else if (!strcmp(argv[0], "create"))
1614 return !!create_stash(argc, argv, prefix);
1615 else if (!strcmp(argv[0], "push"))
1603 - return !!push_stash(argc, argv, prefix);
1616 + return !!push_stash(argc, argv, prefix, 0);
1617 else if (!strcmp(argv[0], "save"))
1618 return !!save_stash(argc, argv, prefix);
1619 else if (*argv[0] != '-')
1620 usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
1621 git_stash_usage, options);
1622
1610 - if (strcmp(argv[0], "-p")) {
1611 - while (++i < argc && strcmp(argv[i], "--")) {
1612 - /*
1613 - * `akpqu` is a string which contains all short options,
1614 - * except `-m` which is verified separately.
1615 - */
1616 - if ((strlen(argv[i]) == 2) && *argv[i] == '-' &&
1617 - strchr("akpqu", argv[i][1]))
1618 - continue;
1619 -
1620 - if (!strcmp(argv[i], "--all") ||
1621 - !strcmp(argv[i], "--keep-index") ||
1622 - !strcmp(argv[i], "--no-keep-index") ||
1623 - !strcmp(argv[i], "--patch") ||
1624 - !strcmp(argv[i], "--quiet") ||
1625 - !strcmp(argv[i], "--include-untracked"))
1626 - continue;
1627 -
1628 - /*
1629 - * `-m` and `--message=` are verified separately because
1630 - * they need to be immediately followed by a string
1631 - * (i.e.`-m"foobar"` or `--message="foobar"`).
1632 - */
1633 - if (starts_with(argv[i], "-m") ||
1634 - starts_with(argv[i], "--message="))
1635 - continue;
1636 -
1637 - usage_with_options(git_stash_usage, options);
1638 - }
1639 - }
1640 -
1623 + /* Assume 'stash push' */
1624 argv_array_push(&args, "push");
1625 argv_array_pushv(&args, argv);
1643 - return !!push_stash(args.argc, args.argv, prefix);
1626 + return !!push_stash(args.argc, args.argv, prefix, 1);
1627 }
t/t3903-stash.sh
+5
@@ -285,6 +285,11 @@ test_expect_success 'stash --no-keep-index' '
285 test bar,bar2 = $(cat file),$(cat file2)
286 '
287
288 +test_expect_success 'dont assume push with non-option args' '
289 + test_must_fail git stash -q drop 2>err &&
290 + test_i18ngrep -e "subcommand wasn'\''t specified; '\''push'\'' can'\''t be assumed due to unexpected token '\''drop'\''" err
291 +'
292 +
293 test_expect_success 'stash --invalid-option' '
294 echo bar5 >file &&
295 echo bar6 >file2 &&