From: Phillip Wood Date: Tue, 05 Nov 2019 15:03:56 GMT Subject: Re: [PATCH 3/5] reset: support the --pathspec-from-file option Message-ID: In-Reply-To: <8d9f1fbc18346144a0c866a59891b652dcfe9b7f.1572895605.git.gitgitgadget@gmail.com> Hi Alexandr This also looks good, I've got some minor comments below. If I'm complaining about whitespace and style issues you know the patch is quite good! On 04/11/2019 19:26, Alexandr Miloslavskiy via GitGitGadget wrote: > From: Alexandr Miloslavskiy > > This option solves the problem of commandline length limit for UI's > built on top of git. Plumbing commands are not always a good fit, for > two major reasons: > 1) Some UI's serve as assistants that help user run git commands. In > this case, replacing familiar commands with plumbing commands will > confuse most users. > 2) Some UI's have started and grown with porcelain commands. Replacing > existing logic with plumbing commands could be cumbersome and prone > to various new problems. I think reset is a good example of the second reason - it's not straight forward to implement it in plumbing commands > The new option is designed to behave very close to pathspecs passed in > commandline args, so that switching from one to another is simple. > > The new option allows to read either a specified file or `stdin`. > Reading from file is a good way to avoid competing for stdin, and > also gives some extra flexibility. I think this flexibility is a good idea > Decisions taken for simplicity: > 1) The new option is declared incompatible with other options that > could use `stdin`. I'm confused reset does not use stdin does it? > 2) It is not allowed to pass some refspecs in args and others in file. s/refspecs/pathspecs/ ? > 3) New options do not have shorthands to avoid shorthand conflicts. > Also add new '--pathspec-file-null' switch that mirrors '-z' used in > various places. Some porcelain commands, such as `git commit`, already > use '-z', therefore it needed a new unambiguous name. As the 'lines' in the file are nul terminated perhaps it would be better to call this --pathspec-file-nul or --nul-termination. I think the use of --null to mean nul termination for config was a mistake (for grep it matches what GUN grep does but it's still unfortunate). > > Co-authored-by: Johannes Schindelin > Signed-off-by: Alexandr Miloslavskiy > --- > Documentation/git-reset.txt | 18 ++++- > builtin/reset.c | 22 +++++- > t/t7107-reset-pathspec-file.sh | 126 +++++++++++++++++++++++++++++++++ > 3 files changed, 161 insertions(+), 5 deletions(-) > create mode 100755 t/t7107-reset-pathspec-file.sh > > diff --git a/Documentation/git-reset.txt b/Documentation/git-reset.txt > index b0ea6e0ce5..d484cd2827 100644 > --- a/Documentation/git-reset.txt > +++ b/Documentation/git-reset.txt > @@ -9,18 +9,20 @@ SYNOPSIS > -------- > [verse] > 'git reset' [-q] [] [--] ... > +'git reset' [-q] [--pathspec-from-file= [--pathspec-file-null]] [] --pathspec-file would be shorter and still conveys the intent of the option. Is this line missing a leading space? > 'git reset' (--patch | -p) [] [--] [...] > 'git reset' [--soft | --mixed [-N] | --hard | --merge | --keep] [-q] [] > > DESCRIPTION > ----------- > -In the first and second form, copy entries from `` to the index. > -In the third form, set the current branch head (`HEAD`) to ``, > +In the first three forms, copy entries from `` to the index. > +In the last form, set the current branch head (`HEAD`) to ``, > optionally modifying index and working tree to match. > The ``/`` defaults to `HEAD` in all forms. > > 'git reset' [-q] [] [--] ...:: > - This form resets the index entries for all `` to their > +'git reset' [-q] [--pathspec-from-file= [--pathspec-file-null]] []:: Alignment again > + These forms reset the index entries for all `` to their The new form does not mention so this could be confusing > state at ``. (It does not affect the working tree or > the current branch.) > + > @@ -107,6 +109,16 @@ OPTIONS > `reset.quiet` config option. `--quiet` and `--no-quiet` will > override the default behavior. > > +--pathspec-from-file=:: > + Read `` from `` instead. As we have a separate synopsis line for --pathspec-from-file which does not mention it might be better just to say "read pathspecs from `` instead of the command line". > If `` is exactly `-` > + then read from standard input. Pathspecs are separated by LF or > + CR/LF. Pathspecs can be quoted as explained for the configuration > + variable `core.quotePath` (see linkgit:git-config[1]). See also > + `--pathspec-file-null` and global `--literal-pathspecs`. > + > +--pathspec-file-null:: > + Only meaningful with `--pathspec-from-file`. Pathspecs are > + separated with NUL character and are not expected to be quoted. > > EXAMPLES > -------- > diff --git a/builtin/reset.c b/builtin/reset.c > index fdd572168b..0eaa6b0bca 100644 > --- a/builtin/reset.c > +++ b/builtin/reset.c > @@ -31,6 +31,7 @@ > static const char * const git_reset_usage[] = { > N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] []"), > N_("git reset [-q] [] [--] ..."), > + N_("git reset [-q] [--pathspec-from-file [--pathspec-file-null]] []"), > N_("git reset --patch [] [--] [...]"), > NULL > }; > @@ -284,8 +285,8 @@ static int git_reset_config(const char *var, const char *value, void *cb) > int cmd_reset(int argc, const char **argv, const char *prefix) > { > int reset_type = NONE, update_ref_status = 0, quiet = 0; > - int patch_mode = 0, unborn; > - const char *rev; > + int patch_mode = 0, pathspec_file_null = 0, unborn; > + const char *rev, *pathspec_from_file = NULL; > struct object_id oid; > struct pathspec pathspec; > int intent_to_add = 0; > @@ -306,6 +307,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix) > OPT_BOOL('p', "patch", &patch_mode, N_("select hunks interactively")), > OPT_BOOL('N', "intent-to-add", &intent_to_add, > N_("record only the fact that removed paths will be added later")), > + OPT_FILENAME(0, "pathspec-from-file", &pathspec_from_file, > + N_("read pathspecs from file")), > + OPT_BOOL(0, "pathspec-file-null", &pathspec_file_null, > + N_("with --pathspec-from-file, pathspecs are separated with NUL character")), > OPT_END() > }; > > @@ -316,6 +321,19 @@ int cmd_reset(int argc, const char **argv, const char *prefix) > PARSE_OPT_KEEP_DASHDASH); > parse_args(&pathspec, argv, prefix, patch_mode, &rev); > > + if (pathspec_from_file) { > + if (patch_mode) > + die(_("--pathspec-from-file is incompatible with --patch")); This is sensible as -p is interactive so we wouldn't expect command line length to be an issue > + > + if (pathspec.nr) > + die(_("--pathspec-from-file is incompatible with path arguments")); > + > + parse_pathspec_file(&pathspec, 0, > + PATHSPEC_PREFER_FULL, > + prefix, pathspec_from_file, pathspec_file_null); > + } else if (pathspec_file_null) > + die(_("--pathspec-file-null requires --pathspec-from-file")); Style nit: the coding guidelines state that if any branch of an if statement requires braces then all the branches should be braced. This is widely ignored though. > + > unborn = !strcmp(rev, "HEAD") && get_oid("HEAD", &oid); > if (unborn) { > /* reset on unborn branch: treat as reset to empty tree */ > diff --git a/t/t7107-reset-pathspec-file.sh b/t/t7107-reset-pathspec-file.sh > new file mode 100755 > index 0000000000..cf7f085ad5 > --- /dev/null > +++ b/t/t7107-reset-pathspec-file.sh > @@ -0,0 +1,126 @@ > +#!/bin/sh > + > +test_description='reset --pathspec-from-file' > + > +. ./test-lib.sh > + > +cat > expect.a < + D fileA.t > +EOF > + > +cat > expect.ab < + D fileA.t > + D fileB.t > +EOF > + > +cat > expect.a_bc_d < +D fileA.t > + D fileB.t > + D fileC.t > +D fileD.t > +EOF These days we tend to set up the expected files within the relevant test case using <<-\EOF to allow indentation and disallow substitution (unless it's needed of course) > +test_expect_success setup ' > + echo A >fileA.t && > + echo B >fileB.t && > + echo C >fileC.t && > + echo D >fileD.t && > + git add . && > + git commit --include . -m "Commit" && > + checkpoint=$(git rev-parse --verify HEAD) > +' > + > +restore_checkpoint () { > + git reset --hard "$checkpoint" > +} > + > +verify_state () { > + git status --porcelain -- fileA.t fileB.t fileC.t fileD.t >actual && > + test_cmp "$1" actual > +} > + > +test_expect_success '--pathspec-from-file from stdin' ' > + restore_checkpoint && > + > + git rm fileA.t && > + echo fileA.t | git reset --pathspec-from-file=- && > + > + verify_state expect.a > +' > + > +test_expect_success '--pathspec-from-file from file' ' > + restore_checkpoint && > + > + git rm fileA.t && > + echo fileA.t >list && > + git reset --pathspec-from-file=list && > + > + verify_state expect.a > +' > + > +test_expect_success 'NUL delimiters' ' > + restore_checkpoint && > + > + git rm fileA.t fileB.t && > + printf fileA.tQfileB.t | q_to_nul | git reset --pathspec-from-file=- --pathspec-file-null && > + > + verify_state expect.ab > +' > + > +test_expect_success 'LF delimiters' ' > + restore_checkpoint && > + > + git rm fileA.t fileB.t && > + printf "fileA.t\nfileB.t" | git reset --pathspec-from-file=- && > + > + verify_state expect.ab > +' > + > +test_expect_success 'CRLF delimiters' ' > + restore_checkpoint && > + > + git rm fileA.t fileB.t && > + printf "fileA.t\r\nfileB.t" | git reset --pathspec-from-file=- && > + > + verify_state expect.ab > +' > + > +test_expect_success 'quotes' ' > + restore_checkpoint && > + > + git rm fileA.t && > + printf "\"file\\101.t\"" | git reset --pathspec-from-file=- && > + > + verify_state expect.a If I've understood correctly this doesn't test if a path is correctly unquoted, only that it is accepted. > +' > + > +test_expect_success 'quotes not compatible with --pathspec-file-null' ' > + restore_checkpoint && > + > + git rm fileA.t && > + printf "\"file\\101.t\"" >list && > + # Note: "git reset" has not yet learned to fail on wrong pathspecs > + git reset --pathspec-from-file=list --pathspec-file-null && > + > + test_must_fail verify_state expect.a > +' > + > +test_expect_success '--pathspec-from-file is not compatible with --soft --hard' ' s/--soft --hard/--soft or --hard/ > + restore_checkpoint && > + > + git rm fileA.t && > + echo fileA.t >list && > + test_must_fail git reset --soft --pathspec-from-file=list && > + test_must_fail git reset --hard --pathspec-from-file=list > +' > + > +test_expect_success 'only touches what was listed' ' s/^/--pathspec-from-file / ? Best Wishes Phillip > + restore_checkpoint && > + > + git rm fileA.t fileB.t fileC.t fileD.t && > + printf "fileB.t\nfileC.t" | git reset --pathspec-from-file=- && > + > + verify_state expect.a_bc_d > +' > + > +test_done >