"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@xxxxxxxxx>
writes:
> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@xxxxxxxxxxx>
>
> Decisions taken for simplicity:
> 1) It is not allowed to pass pathspec in both args and file.
>
> `if (!argc)` block was adapted to work with --pathspec-from-file. For
> that, I also had to parse pathspec earlier. Now it happens before
> `read_cache()` / `hold_locked_index()` / `setup_work_tree()`, which
> sounds fine to me.
That is not an explanation nor justification.
> In case of empty pathspec, there is now a clear error message instead
> of showing usage.
Hmph, "git rm --pathspec-from-file=/dev/null" would say "nothing
specified, nothing removed" and it makes perfect sense, but I am not
sure "git rm" that gives the same message is better than the output
by usage_with_options(builtin_rm_usage, builtin_rm_options).
> -'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch] [--quiet] [--] <pathspec>...
> +'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch]
> + [--quiet] [--pathspec-from-file=<file> [--pathspec-file-nul]]
> + [--] [<pathspec>...]
OK.
> +--pathspec-from-file=<file>::
> + Pathspec is passed in `<file>` instead of commandline args. If
> + `<file>` is exactly `-` then standard input is used. Pathspec
> + elements are separated by LF or CR/LF. Pathspec elements can be
> + quoted as explained for the configuration variable `core.quotePath`
> + (see linkgit:git-config[1]). See also `--pathspec-file-nul` and
> + global `--literal-pathspecs`.
> +
> +--pathspec-file-nul::
> + Only meaningful with `--pathspec-from-file`. Pathspec elements are
> + separated with NUL character and all other characters are taken
> + literally (including newlines and quotes).
> +
OK.
> diff --git a/builtin/rm.c b/builtin/rm.c
> index 19ce95a901..8e40795751 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 = 0;
> +static char *pathspec_from_file = NULL;
We may want to clean these "explicitly initialize to 0/NULL" up at
some point. The clean-up itself would not be in the scope of this
patch, of course, but not making it worse is something this patch
can do to help.
> @@ -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(_("Nothing specified, nothing removed"));
I wonder if doing these in this order instead would make more sense
without making unnecessary behaviour change.
- parse the options (which would make pathspec_f_f available to
us)
- if pathspec_f_f is given, call parse_pathspec_file()
- otherwise complain if pathspec_file_nul is set
- otherwise check argc and give the usage_with_options()
I dunno.
Thanks.
