Re: [PATCH 5/5] commit: support the --pathspec-from-file option

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Alexandr

On 04/11/2019 19:26, Alexandr Miloslavskiy via GitGitGadget wrote:
> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
> 
> 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@syntevo.com>
> ---
>   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
>