Hi Alexandr
On 04/11/2019 19:26, Alexandr Miloslavskiy via GitGitGadget wrote:
From: Alexandr Miloslavskiy <alexandr.miloslavskiy@xxxxxxxxxxx>
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 <alexandr.miloslavskiy@xxxxxxxxxxx>
---
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 <file> | -m <msg>] [--reset-author] [--allow-empty]
[--allow-empty-message] [--no-verify] [-e] [--author=<author>]
[--date=<date>] [--cleanup=<mode>] [--[no-]status]
- [-i | -o] [-S[<keyid>]] [--] [<pathspec>...]
+ [-i | -o] [--pathspec-from-file=<file> [--pathspec-file-null]]
+ [-S[<keyid>]] [--] [<pathspec>...]
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=<file>::
+ Read `<pathspec>` from `<file>` instead. If `<file>` 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 <pathspecs> and the option names
apply here as well
-u[<mode>]::
--untracked-files[=<mode>]::
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 <<EOF
+A fileA.t
+EOF
+
+cat > expect.ab <<EOF
+A fileA.t
+A fileB.t
+EOF
+
+cat > expect.bc <<EOF
+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
