From 5f393dc3aa62ba3e13a9890af4e769ad84812fbc Mon Sep 17 00:00:00 2001 From: Alexandr Miloslavskiy Date: Mon, 17 Feb 2020 17:25:16 +0000 Subject: rm: support the --pathspec-from-file option Decisions taken for simplicity: 1) It is not allowed to pass pathspec in both args and file. Adjustments were needed for `if (!argc)` block: This code actually means "pathspec is not present". Previously, pathspec could only come from commandline arguments, so testing for `argc` was a valid way of testing for the presence of pathspec. But this is no longer true with `--pathspec-from-file`. During the entire `--pathspec-from-file` story, I tried to keep its behavior very close to giving pathspec on commandline, so that switching from one to another doesn't involve any surprises. However, throwing usage at user in the case of empty `--pathspec-from-file` would puzzle because there's nothing wrong with "usage" (that is, argc/argv array). On the other hand, throwing usage in the old case also feels bad to me. While it's less of a puzzle, I (as user) never liked the experience of comparing my commandline to "usage", trying to spot a difference. Since it's already known what the error is, it feels a lot better to give that specific error to user. Judging from [1] it doesn't seem that showing usage in this case was important (the patch was to avoid segfault), and it doesn't fit into how other commands react to empty pathspec (see for example `git add` with a custom message). Therefore, I decided to show new error text in both cases. In order to continue testing for error early, I moved `parse_pathspec()` higher. Now it happens before `read_cache()` / `hold_locked_index()` / `setup_work_tree()`, which shouldn't cause any issues. [1] Commit 7612a1ef ("git-rm: honor -n flag" 2006-06-09) Signed-off-by: Alexandr Miloslavskiy Signed-off-by: Junio C Hamano --- builtin/rm.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) (limited to 'builtin') diff --git a/builtin/rm.c b/builtin/rm.c index 19ce95a901..4858631e0f 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -235,7 +235,8 @@ static int check_local_mod(struct object_id *head, int index_only) } static int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0; -static int ignore_unmatch = 0; +static int ignore_unmatch = 0, pathspec_file_nul; +static char *pathspec_from_file; static struct option builtin_rm_options[] = { OPT__DRY_RUN(&show_only, N_("dry run")), @@ -245,6 +246,8 @@ static struct option builtin_rm_options[] = { OPT_BOOL('r', NULL, &recursive, N_("allow recursive removal")), OPT_BOOL( 0 , "ignore-unmatch", &ignore_unmatch, N_("exit with a zero status even if nothing matched")), + OPT_PATHSPEC_FROM_FILE(&pathspec_from_file), + OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul), OPT_END(), }; @@ -259,8 +262,24 @@ int cmd_rm(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, builtin_rm_options, builtin_rm_usage, 0); - if (!argc) - usage_with_options(builtin_rm_usage, builtin_rm_options); + + parse_pathspec(&pathspec, 0, + PATHSPEC_PREFER_CWD, + prefix, argv); + + if (pathspec_from_file) { + if (pathspec.nr) + die(_("--pathspec-from-file is incompatible with pathspec arguments")); + + parse_pathspec_file(&pathspec, 0, + PATHSPEC_PREFER_CWD, + prefix, pathspec_from_file, pathspec_file_nul); + } else if (pathspec_file_nul) { + die(_("--pathspec-file-nul requires --pathspec-from-file")); + } + + if (!pathspec.nr) + die(_("No pathspec was given. Which files should I remove?")); if (!index_only) setup_work_tree(); @@ -270,9 +289,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix) if (read_cache() < 0) die(_("index file corrupt")); - parse_pathspec(&pathspec, 0, - PATHSPEC_PREFER_CWD, - prefix, argv); refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, &pathspec, NULL, NULL); seen = xcalloc(pathspec.nr, 1); -- cgit v1.2.3 From 8c3713cede71e4741dac546f787734627329f55b Mon Sep 17 00:00:00 2001 From: Alexandr Miloslavskiy Date: Mon, 17 Feb 2020 17:25:21 +0000 Subject: 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 Signed-off-by: Junio C Hamano --- builtin/stash.c | 59 ++++++++++++++++++++------------------------------------- 1 file changed, 21 insertions(+), 38 deletions(-) (limited to 'builtin') diff --git a/builtin/stash.c b/builtin/stash.c index 4ad3adf4ba..7bc4c5d696 100644 --- 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); } -- cgit v1.2.3 From 8a98758a8d9dbd807fc2f1576e9141c1b720c5c1 Mon Sep 17 00:00:00 2001 From: Alexandr Miloslavskiy Date: Mon, 17 Feb 2020 17:25:22 +0000 Subject: stash push: support the --pathspec-from-file option Decisions taken for simplicity: 1) For now, `--pathspec-from-file` is declared incompatible with `--patch`, even when is not `-`. Such use case is not really expected. 2) It is not allowed to pass pathspec in both args and file. Signed-off-by: Alexandr Miloslavskiy Signed-off-by: Junio C Hamano --- builtin/stash.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) (limited to 'builtin') diff --git a/builtin/stash.c b/builtin/stash.c index 7bc4c5d696..74d92595a2 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -27,6 +27,7 @@ static const char * const git_stash_usage[] = { N_("git stash clear"), N_("git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n" " [-u|--include-untracked] [-a|--all] [-m|--message ]\n" + " [--pathspec-from-file= [--pathspec-file-nul]]\n" " [--] [...]]"), N_("git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n" " [-u|--include-untracked] [-a|--all] []"), @@ -1456,7 +1457,9 @@ static int push_stash(int argc, const char **argv, const char *prefix, int patch_mode = 0; int include_untracked = 0; int quiet = 0; + int pathspec_file_nul = 0; const char *stash_msg = NULL; + const char *pathspec_from_file = NULL; struct pathspec ps; struct option options[] = { OPT_BOOL('k', "keep-index", &keep_index, @@ -1470,6 +1473,8 @@ static int push_stash(int argc, const char **argv, const char *prefix, N_("include ignore files"), 2), OPT_STRING('m', "message", &stash_msg, N_("message"), N_("stash message")), + OPT_PATHSPEC_FROM_FILE(&pathspec_from_file), + OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul), OPT_END() }; @@ -1492,6 +1497,21 @@ static int push_stash(int argc, const char **argv, const char *prefix, parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN, prefix, argv); + + if (pathspec_from_file) { + if (patch_mode) + die(_("--pathspec-from-file is incompatible with --patch")); + + if (ps.nr) + die(_("--pathspec-from-file is incompatible with pathspec arguments")); + + parse_pathspec_file(&ps, 0, + PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN, + prefix, pathspec_from_file, pathspec_file_nul); + } else if (pathspec_file_nul) { + die(_("--pathspec-file-nul requires --pathspec-from-file")); + } + return do_push_stash(&ps, stash_msg, quiet, keep_index, patch_mode, include_untracked); } -- cgit v1.2.3