From: Phillip Wood Date: Tue, 05 Nov 2019 16:27:25 GMT Subject: Re: [PATCH 5/5] commit: support the --pathspec-from-file option Message-ID: <9ca7fa57-c438-7243-6ab1-956d8f132d37@gmail.com> In-Reply-To: Hi Alexandr 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. > > 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. > > Decisions taken for simplicity: > 1) The new option is declared incompatible with other options that > could use `stdin`. > 2) It is not allowed to pass some refspecs in args and others in file. > 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. It might be worth tailoring the message to the command rather than having exactly the same message for commit and reset > > Signed-off-by: Alexandr Miloslavskiy > --- > Documentation/git-commit.txt | 14 ++++- > builtin/commit.c | 25 ++++++-- > t/t7526-commit-pathspec-file.sh | 107 ++++++++++++++++++++++++++++++++ > 3 files changed, 141 insertions(+), 5 deletions(-) > create mode 100755 t/t7526-commit-pathspec-file.sh > > diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt > index 4341d0e3ab..ec4752298d 100644 > --- a/Documentation/git-commit.txt > +++ b/Documentation/git-commit.txt > @@ -13,7 +13,8 @@ SYNOPSIS > [-F | -m ] [--reset-author] [--allow-empty] > [--allow-empty-message] [--no-verify] [-e] [--author=] > [--date=] [--cleanup=] [--[no-]status] > - [-i | -o] [-S[]] [--] [...] > + [-i | -o] [--pathspec-from-file= [--pathspec-file-null]] > + [-S[]] [--] [...] > > DESCRIPTION > ----------- > @@ -277,6 +278,17 @@ FROM UPSTREAM REBASE" section in linkgit:git-rebase[1].) > the last commit without committing changes that have > already been staged. If used together with `--allow-empty` > paths are also not required, and an empty commit will be created. > + > +--pathspec-from-file=:: > + Read `` from `` instead. 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. I think my comments from patch 3 about and the option names apply here as well > -u[]:: > --untracked-files[=]:: > diff --git a/builtin/commit.c b/builtin/commit.c > index e588bc6ad3..532f305926 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -107,9 +107,9 @@ static int all, also, interactive, patch_interactive, only, amend, signoff; > static int edit_flag = -1; /* unspecified */ > static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship; > static int config_commit_verbose = -1; /* unspecified */ > -static int no_post_rewrite, allow_empty_message; > +static int no_post_rewrite, allow_empty_message, pathspec_file_null; > static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg; > -static char *sign_commit; > +static char *sign_commit, *pathspec_from_file; > > /* > * The default commit message cleanup mode will remove the lines > @@ -343,6 +343,23 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix > PATHSPEC_PREFER_FULL, > prefix, argv); > > + if (pathspec_from_file) { > + if (interactive) > + die(_("--pathspec-from-file is incompatible with --interactive/--patch")); > + > + 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")); > + > + if (!pathspec.nr && (also || (only && !amend && !allow_empty))) > + die(_("No paths with --include/--only does not make sense.")); I wonder if there is a way of calling parse_pathspec_file() from parse_and_validate_options() instead. Otherwise we end up validating options here instead which is a bit messy. > if (read_cache_preload(&pathspec) < 0) > die(_("index file corrupt")); > > @@ -1198,8 +1215,6 @@ static int parse_and_validate_options(int argc, const char *argv[], > > if (also + only + all + interactive > 1) > die(_("Only one of --include/--only/--all/--interactive/--patch can be used.")); > - if (argc == 0 && (also || (only && !amend && !allow_empty))) > - die(_("No paths with --include/--only does not make sense.")); > cleanup_mode = get_cleanup_mode(cleanup_arg, use_editor); > > handle_untracked_files_arg(s); > @@ -1535,6 +1550,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > OPT_BOOL(0, "amend", &amend, N_("amend previous commit")), > OPT_BOOL(0, "no-post-rewrite", &no_post_rewrite, N_("bypass post-rewrite hook")), > { OPTION_STRING, 'u', "untracked-files", &untracked_files_arg, N_("mode"), N_("show untracked files, optional modes: all, normal, no. (Default: all)"), PARSE_OPT_OPTARG, NULL, (intptr_t)"all" }, > + 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")), > /* end commit contents options */ > > OPT_HIDDEN_BOOL(0, "allow-empty", &allow_empty, > diff --git a/t/t7526-commit-pathspec-file.sh b/t/t7526-commit-pathspec-file.sh > new file mode 100755 > index 0000000000..c5d68e01e6 > --- /dev/null > +++ b/t/t7526-commit-pathspec-file.sh The test comments from patch 3 apply here as well I think. Overall this series is nicely structured and is looking pretty good Best Wishes Phillip > @@ -0,0 +1,107 @@ > +#!/bin/sh > + > +test_description='commit --pathspec-from-file' > + > +. ./test-lib.sh > + > +test_tick > + > +cat > expect.a < +A fileA.t > +EOF > + > +cat > expect.ab < +A fileA.t > +A fileB.t > +EOF > + > +cat > expect.bc < +A fileB.t > +A fileC.t > +EOF > + > +test_expect_success setup ' > + test_commit file0 && > + checkpoint=$(git rev-parse --verify HEAD) && > + > + 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 > +' > + > +restore_checkpoint () { > + git reset --soft "$checkpoint" > +} > + > +verify_state () { > + git diff-tree --no-commit-id --name-status -r HEAD >actual && > + test_cmp "$1" actual > +} > + > +test_expect_success '--pathspec-from-file from stdin' ' > + restore_checkpoint && > + > + echo fileA.t | git commit --pathspec-from-file=- -m "Commit" && > + > + verify_state expect.a > +' > + > +test_expect_success '--pathspec-from-file from file' ' > + restore_checkpoint && > + > + echo fileA.t >list && > + git commit --pathspec-from-file=list -m "Commit" && > + > + verify_state expect.a > +' > + > +test_expect_success 'NUL delimiters' ' > + restore_checkpoint && > + > + printf fileA.tQfileB.t | q_to_nul | git commit --pathspec-from-file=- --pathspec-file-null -m "Commit" && > + > + verify_state expect.ab > +' > + > +test_expect_success 'LF delimiters' ' > + restore_checkpoint && > + > + printf "fileA.t\nfileB.t" | git commit --pathspec-from-file=- -m "Commit" && > + > + verify_state expect.ab > +' > + > +test_expect_success 'CRLF delimiters' ' > + restore_checkpoint && > + > + printf "fileA.t\r\nfileB.t" | git commit --pathspec-from-file=- -m "Commit" && > + > + verify_state expect.ab > +' > + > +test_expect_success 'quotes' ' > + restore_checkpoint && > + > + printf "\"file\\101.t\"" | git commit --pathspec-from-file=- -m "Commit" && > + > + verify_state expect.a > +' > + > +test_expect_success 'quotes not compatible with --pathspec-file-null' ' > + restore_checkpoint && > + > + printf "\"file\\101.t\"" >list && > + test_must_fail git commit --pathspec-from-file=list --pathspec-file-null -m "Commit" > +' > + > +test_expect_success 'only touches what was listed' ' > + restore_checkpoint && > + > + printf "fileB.t\nfileC.t" | git commit --pathspec-from-file=- -m "Commit" && > + > + verify_state expect.bc > +' > + > +test_done >