[5/5] commit: support the --pathspec-from-file option
diff mbox series

Message ID f4847046896848d3f16bc5f3cb7a26271cefd97c.1572895605.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • Add --pathspec-from-file option for reset, commit
Related show

Commit Message

Johannes Schindelin via GitGitGadget Nov. 4, 2019, 7:26 p.m. UTC
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.

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

Comments

Phillip Wood Nov. 5, 2019, 4:27 p.m. UTC | #1
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
>
Alexandr Miloslavskiy Nov. 5, 2019, 7:42 p.m. UTC | #2
On 05.11.2019 17:27, Phillip Wood wrote:

>> 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

I also was somewhat unhappy about duplication. But I didn't figure how 
to do that correctly. Currently the messages for 'git reset' and 'git 
commit' are almost identical.

Maybe in 2nd commit I should say something like "Extend 
`--pathspec-file-null` support to `git commit` (see previous patch for 
`git reset`)" ?

> I think my comments from patch 3 about <pathspecs> and the option names 
> apply here as well

Yes, sure, I will try to apply your suggestions to all patches. 
Hopefully without forgetting things :)

>> +    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.

Yes, I was also somewhat unhappy about that. I will give it more thought.

> Overall this series is nicely structured and is looking pretty good

Thanks, and also thanks for reviewing my patches!
Junio C Hamano Nov. 6, 2019, 4:51 a.m. UTC | #3
"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> 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.

I think all the comments I made on "reset" with these two options,
both to the proposed log message and to the patch text, applies to
this this step, too, so I won't repeat them.

Thanks for working on this topic.
Alexandr Miloslavskiy Nov. 6, 2019, 3:56 p.m. UTC | #4
I think I have implemented most suggestions in PatchV2. Thanks!

> It might be worth tailoring the message to the command rather than 
> having exactly the same message for commit and reset

I decided to move the general comment to base commit where options are 
introduced and not repeat it where options are supported.

> 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.

The code looks a bit too entangled to support that without making it 
worse. The biggest concern I have is that parse_and_validate_options() 
will populate `pathspec` and some other function will need to remember 
to clean it up. I like it better when `pathspec` is handled in one place.

I agree that things are not perfect, but this seems to be a consequence 
of other existing problems. For example, I would have expected a 
structure instead of a handful of global variables. That would have 
solved many problems. However, I didn't have the bravery to dive into 
this refactoring.

Patch
diff mbox series

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.
 
 -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."));
+
 	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
@@ -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