Message ID | 5611e3ae326bb7f61abf870e3b2851226b6af1d8.1579190965.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Support --pathspec-from-file in rm, stash | expand |
"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> > > 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.
Sorry for late reply, I was on vacation. Now I'm back and ready to continue :) Thanks for your review! On 21.01.2020 20:36, Junio C Hamano wrote: >> 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. I'm not exactly sure what are you suggesting. My best guess is that you want to remove "`if (!argc)` block was adapted" paragraph from commit message? I thought about it and it feels wrong to leave this change unexplained. Or are you suggesting to reword it? If so, please give a hint. >> 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). What feels wrong to me is when I make a mistake and git just slams me with usage, and then it's up to me to figure what could be wrong. I myself struggled to find a mistake a couple times (in similar cases, not in this specific one) and didn't like the experience. This could be a lot worse when there's no mistake, just the file was empty - but you already agreed that showing a new error message is reasonable with '--pathspec-from-file'. Still, without '--pathspec-from-file', it should still be better to point to a specific error rather then "here's usage and try to find a difference". I have reworded the error message in V2 in hopes that it will be less controversial. If you still don't like it, I will change it to only show the new error with '--pathspec-from-file'. >> +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. Changed in V2.
Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> writes: > Sorry for late reply, I was on vacation. Now I'm back and ready to > continue :) > > Thanks for your review! > > On 21.01.2020 20:36, Junio C Hamano wrote: >>> 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. > > I'm not exactly sure what are you suggesting. I expected that the proposed log message to explain and justify why a change (in behaviour, in design, etc.) is made. "There is this limitation" is not a justification---"because of such and such reasons, there is this limitation, otherwise such and such bad things happen" is.
On 10.02.2020 19:48, Junio C Hamano wrote: > I expected that the proposed log message to explain and justify why > a change (in behaviour, in design, etc.) is made. "There is this > limitation" is not a justification---"because of such and such > reasons, there is this limitation, otherwise such and such bad > things happen" is. I have rewritten the commit message in V3. Is that better?
diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt index e02a08e5ef..ab750367fd 100644 --- a/Documentation/git-rm.txt +++ b/Documentation/git-rm.txt @@ -8,7 +8,9 @@ git-rm - Remove files from the working tree and from the index SYNOPSIS -------- [verse] -'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>...] DESCRIPTION ----------- @@ -73,6 +75,19 @@ For more details, see the 'pathspec' entry in linkgit:gitglossary[7]. `git rm` normally outputs one line (in the form of an `rm` command) for each file removed. This option suppresses that output. +--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). + REMOVING FILES THAT HAVE DISAPPEARED FROM THE FILESYSTEM -------------------------------------------------------- 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; 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(_("Nothing specified, nothing removed")); 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); diff --git a/t/t3601-rm-pathspec-file.sh b/t/t3601-rm-pathspec-file.sh new file mode 100755 index 0000000000..0f69ae8478 --- /dev/null +++ b/t/t3601-rm-pathspec-file.sh @@ -0,0 +1,79 @@ +#!/bin/sh + +test_description='rm --pathspec-from-file' + +. ./test-lib.sh + +test_tick + +test_expect_success setup ' + echo A >fileA.t && + echo B >fileB.t && + echo C >fileC.t && + echo D >fileD.t && + git add fileA.t fileB.t fileC.t fileD.t && + git commit -m "files" && + + git tag checkpoint +' + +restore_checkpoint () { + git reset --hard checkpoint +} + +verify_expect () { + git status --porcelain --untracked-files=no -- fileA.t fileB.t fileC.t fileD.t >actual && + test_cmp expect actual +} + +test_expect_success 'simplest' ' + restore_checkpoint && + + cat >expect <<-\EOF && + D fileA.t + EOF + + echo fileA.t | git rm --pathspec-from-file=- && + verify_expect +' + +test_expect_success '--pathspec-file-nul' ' + restore_checkpoint && + + cat >expect <<-\EOF && + D fileA.t + D fileB.t + EOF + + printf "fileA.t\0fileB.t\0" | git rm --pathspec-from-file=- --pathspec-file-nul && + verify_expect +' + +test_expect_success 'only touches what was listed' ' + restore_checkpoint && + + cat >expect <<-\EOF && + D fileB.t + D fileC.t + EOF + + printf "fileB.t\nfileC.t\n" | git rm --pathspec-from-file=- && + verify_expect +' + +test_expect_success 'error conditions' ' + restore_checkpoint && + echo fileA.t >list && + + test_must_fail git rm --pathspec-from-file=list -- fileA.t 2>err && + test_i18ngrep -e "--pathspec-from-file is incompatible with pathspec arguments" err && + + test_must_fail git rm --pathspec-file-nul 2>err && + test_i18ngrep -e "--pathspec-file-nul requires --pathspec-from-file" err && + + >empty_list && + test_must_fail git rm --pathspec-from-file=empty_list 2>err && + test_i18ngrep -e "Nothing specified, nothing removed" err +' + +test_done