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

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

Commit Message

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

Co-authored-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
---
 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

Comments

Phillip Wood Nov. 5, 2019, 3:03 p.m. UTC | #1
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 <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 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 <johannes.schindelin@gmx.de>
> Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
> ---
>   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] [<tree-ish>] [--] <pathspec>...
> +'git reset' [-q] [--pathspec-from-file=<file> [--pathspec-file-null]] [<tree-ish>]

--pathspec-file would be shorter and still conveys the intent of the 
option. Is this line missing a leading space?

>   'git reset' (--patch | -p) [<tree-ish>] [--] [<pathspec>...]
>   'git reset' [--soft | --mixed [-N] | --hard | --merge | --keep] [-q] [<commit>]
>   
>   DESCRIPTION
>   -----------
> -In the first and second form, copy entries from `<tree-ish>` to the index.
> -In the third form, set the current branch head (`HEAD`) to `<commit>`,
> +In the first three forms, copy entries from `<tree-ish>` to the index.
> +In the last form, set the current branch head (`HEAD`) to `<commit>`,
>   optionally modifying index and working tree to match.
>   The `<tree-ish>`/`<commit>` defaults to `HEAD` in all forms.
>   
>   'git reset' [-q] [<tree-ish>] [--] <pathspec>...::
> -	This form resets the index entries for all `<pathspec>` to their
> +'git reset' [-q] [--pathspec-from-file=<file> [--pathspec-file-null]] [<tree-ish>]::

Alignment again

> +	These forms reset the index entries for all `<pathspec>` to their

The new form does not mention <pathspec> so this could be confusing

>   	state at `<tree-ish>`.  (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=<file>::
> +	Read `<pathspec>` from `<file>` instead. 

As we have a separate synopsis line for --pathspec-from-file which does 
not mention <pathspec> it might be better just to say "read pathspecs 
from `<file>` instead of the command line".

> 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.
>   
>   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] [<commit>]"),
>   	N_("git reset [-q] [<tree-ish>] [--] <paths>..."),
> +	N_("git reset [-q] [--pathspec-from-file [--pathspec-file-null]] [<tree-ish>]"),
>   	N_("git reset --patch [<tree-ish>] [--] [<paths>...]"),
>   	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 <<EOF
> + D fileA.t
> +EOF
> +
> +cat > expect.ab <<EOF
> + D fileA.t
> + D fileB.t
> +EOF
> +
> +cat > expect.a_bc_d <<EOF
> +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
>
Phillip Wood Nov. 5, 2019, 4:14 p.m. UTC | #2
Hi Alexandr

On 04/11/2019 19:26, Alexandr Miloslavskiy via GitGitGadget wrote:
> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
> [...]
> 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] [<commit>]"),
>   	N_("git reset [-q] [<tree-ish>] [--] <paths>..."),
> +	N_("git reset [-q] [--pathspec-from-file [--pathspec-file-null]] [<tree-ish>]"),
>   	N_("git reset --patch [<tree-ish>] [--] [<paths>...]"),
>   	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")),

One thing I forgot to mention before is that if you're going to add 
these options to a number of commands then maybe it is worth defining a 
macro for each one in parse-options.h. There are a couple of examples of 
this at the end of that file.

Best Wishes

Phillip

>   		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"));
> +
> +		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"));
> +
>   	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 <<EOF
> + D fileA.t
> +EOF
> +
> +cat > expect.ab <<EOF
> + D fileA.t
> + D fileB.t
> +EOF
> +
> +cat > expect.a_bc_d <<EOF
> +D  fileA.t
> + D fileB.t
> + D fileC.t
> +D  fileD.t
> +EOF
> +
> +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
> +'
> +
> +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' '
> +	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' '
> +	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
>
Phillip Wood Nov. 5, 2019, 7:22 p.m. UTC | #3
Hi Alexandr

On 05/11/2019 15:03, Phillip Wood wrote:
> [...]
>> +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.

Oh I think I've misunderstood. The '\\' in unquoted by printf, so git 
sees '\101' which is A so that is a real file.

Sorry for the confusion

Phillip

> 
>> +'
>> +
>> +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
>>
Alexandr Miloslavskiy Nov. 5, 2019, 7:36 p.m. UTC | #4
On 05.11.2019 16:03, Phillip Wood wrote:

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

Thanks for your support :) Previously the opinions were mixed and I was 
a bit afraid that this will invoke a new round of discussions.

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

I understand that '--patch' interacts with user via stdin. Will 
double-check tomorrow.

>> 2) It is not allowed to pass some refspecs in args and others in file.
> 
> s/refspecs/pathspecs/ ?

Thanks! Not quite used to git speak and mix up things sometimes.

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

OK, will change to '--pathspec-file-nul'

>> +'git reset' [-q] [--pathspec-from-file=<file> [--pathspec-file-null]] 
>> [<tree-ish>]
> 
> --pathspec-file would be shorter and still conveys the intent of the 
> option. Is this line missing a leading space?

'--pathspec-from-file' was kind of suggested instead of '--paths-file' 
by Junio here:
https://public-inbox.org/git/xmqqtv9qr82q.fsf@gitster-ct.c.googlers.com/

so Junio added '-from' to previous writing. Hmm. What do you think, 
taking Junio's message into account?

As for whitespaces, sorry, will fix.

>> +'git reset' [-q] [--pathspec-from-file=<file> [--pathspec-file-null]] 
>> [<tree-ish>]::
> 
> Alignment again

Will fix

>> +    These forms reset the index entries for all `<pathspec>` to their
> 
> The new form does not mention <pathspec> so this could be confusing

Shall I replace '<pathspec>' with 'pathspecs' ?

> As we have a separate synopsis line for --pathspec-from-file which does 
> not mention <pathspec> it might be better just to say "read pathspecs 
> from `<file>` instead of the command line".

OK

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

Yes, I also thought so. I doubt that user is willing to interactively 
decide on hundreds of files.

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

Took this code from a guy who ignored it :) But sure, will change.

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

I'll try to change this.

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

In my understanding, 'verify_state expect.a' should test that it's 
correctly understood. Am I wrong?

>> +test_expect_success '--pathspec-from-file is not compatible with 
>> --soft --hard' '
> 
> s/--soft --hard/--soft or --hard/

Good idea.

>> +test_expect_success 'only touches what was listed' '
> 
> s/^/--pathspec-from-file / ?

I thought that whole test package is about '--pathspec-from-file' so I'd 
rather not repeat that in every test name. Shall I change that?
Alexandr Miloslavskiy Nov. 5, 2019, 7:37 p.m. UTC | #5
On 05.11.2019 17:14, Phillip Wood wrote:

>> +        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")),
> 
> One thing I forgot to mention before is that if you're going to add 
> these options to a number of commands then maybe it is worth defining a 
> macro for each one in parse-options.h. There are a couple of examples of 
> this at the end of that file.

Thanks for the pointer!
Junio C Hamano Nov. 6, 2019, 4:40 a.m. UTC | #6
"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.

"UI's that serve as assistants that help user run git commands" does
not have to avoid plumbing commands at all.  Only the ones that "show"
the commands that are run on behalf of the users (perhaps so that the
users can learn from such examples?) do, and I think I learned that
it was your motivating use case from an earlier discussion.  Perhaps

	UIs that help users to formulate git commands to run need to
	present Porcelain commands to be used, as it is not reasonable
	to demonstrate arcane combination of plumbing commands as an
	example for their interactive use.

would probably be more readable without bending what you wanted to
say too much?

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

There is not a lot of sympathy for such argument ;-)

> 2) It is not allowed to pass some refspecs in args and others in file.

Did you mean refspec, not pathspec?

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

I do not understand this part.  Wouldn't it natural to expect that
"-z", when used with "--pathspec-from-file", tell us that the named
file is NUL delimited collection of records?  What's different about
"--pathspec-file-null" that it is confusing to all it "-z"?

I actually do not mind not having "-z" and using only
"--pathspec-file-null".  A new option with long name that feels
similar to existing "-z" but does sufficiently different things so
that it needs a different name, however, feels counter-productive
for the purpose of using it in the UI that produces commands to be
learned by end-users.

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

When I invented the terms "pathspec", "refspec", etc. for this
project, I meant them to be collective nouns.  A pathspec is a set
of pathspec elements, each of which is usually given as a separate
argument on the command line.  So "Read pathspec from file" is good
(not "Read pathspecs from file").

And "Pathspec elements" are separated by LF or CR/LF.

A tangent.  Since we do not allow NUL in a pathspec element, we do
not even need the "-z" option.  When we read pathspec from a file,
we can take any of CRLF, LF or NUL as the separator.

Ah, sorry, that would not help very much.  With "-z" we are allowing
to express pathspec elements inside which there are embedded LF or
CR/LF.  Sorry about the noise.

> +--pathspec-file-null::
> +	Only meaningful with `--pathspec-from-file`. Pathspecs are
> +	separated with NUL character and are not expected to be quoted.

OK.

> +	if (pathspec_from_file) {
> +		if (patch_mode)
> +			die(_("--pathspec-from-file is incompatible with --patch"));
> +
> +		if (pathspec.nr)
> +			die(_("--pathspec-from-file is incompatible with path arguments"));

Shouldn't the error message say pathspec arguments instead?

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

Style:

 - Modern test scripts strive to perform these set-up procedure
   inside (the first) test_expect_success.

 - No SP between the redirection operator and its source/destination
   filename.

 - Quote end-of-here-document (EOF in the above) if you do not rely
   on parameter interpolation inside the here document; this helps
   readers by telling them that what is presented is used verbatim.

> + D fileA.t
> +EOF
> +
> +cat > expect.ab <<EOF
> + D fileA.t
> + D fileB.t
> +EOF
> +
> +cat > expect.a_bc_d <<EOF
> +D  fileA.t
> + D fileB.t
> + D fileC.t
> +D  fileD.t
> +EOF
> +
> +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"
> +}

Hmm, wouldn't it be cleaner to use a lightweight tag or something to
keep checkpoint, instead of a variable that is hard to examine when
tests break and needs debugging?

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

This feeds "fileA.t<NUL>fileB.t" without <NUL> after "fileB.t" to
the command.  Intended?

Rather, perhaps

	printf "%s\0" fileA.t fileB.t

without q-to-nul, once you use printf anyway?

If you truly mean "delimiter" (as opposed to "terminator"),

	printf "fileA.t\0fileB.t"

can also lose " | q_to_nul".

Thanks.
Alexandr Miloslavskiy Nov. 6, 2019, 3:56 p.m. UTC | #7
I think I have implemented most suggestions in PatchV2. Thanks!

On 05.11.2019 16:03, Phillip Wood wrote:
> I'm confused reset does not use stdin does it?

I have improved commit message to clarify.

> --pathspec-file would be shorter and still conveys the intent of the 
> option.

I decided to stay with writing suggested by Junio.

> Is this line missing a leading space?

I think it's good. Maybe you were confused by patch formatting?

> Alignment again

I think it's good again :)

> If I've understood correctly this doesn't test if a path is correctly 
> unquoted, only that it is accepted.

Already agreed that test is good, just a bit non-obvious.

> s/^/--pathspec-from-file / ?

I decided not to repeat '--pathspec-from-file' in every test, because 
test package already has it in description. Is that good?
Alexandr Miloslavskiy Nov. 6, 2019, 3:56 p.m. UTC | #8
I think I have implemented most suggestions in PatchV2. Thanks!

>> 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.
> 
> There is not a lot of sympathy for such argument ;-)

Let's think that this merely reinforces the first point :)

> I do not understand this part.  Wouldn't it natural to expect that
> "-z", when used with "--pathspec-from-file", tell us that the named
> file is NUL delimited collection of records?  What's different about
> "--pathspec-file-null" that it is confusing to all it "-z"?
> 
> I actually do not mind not having "-z" and using only
> "--pathspec-file-null".  A new option with long name that feels
> similar to existing "-z" but does sufficiently different things so
> that it needs a different name, however, feels counter-productive
> for the purpose of using it in the UI that produces commands to be
> learned by end-users.

I didn't want to run into a situation where '-z' suddenly applies to 
multiple things (for example, if later it's decided to allow --patch 
with --pathspec-from-file).

Regarding UI, I think that (insert our UI name here) will not use 
'--pathspec-from-file' unless there are really many files. When it does 
happen, user will probably have an easier time understanding 
'--pathspec-file-nul' then '-z'.

> When I invented the terms "pathspec", "refspec", etc. for this
> project, I meant them to be collective nouns.  A pathspec is a set
> of pathspec elements, each of which is usually given as a separate
> argument on the command line.  So "Read pathspec from file" is good
> (not "Read pathspecs from file").
> 
> And "Pathspec elements" are separated by LF or CR/LF.

I changed that. However, `git grep pathspecs` is quite interesting. My 
own taste says that "pathspecs" is more expected then "pathspec".

> A tangent.  Since we do not allow NUL in a pathspec element, we do
> not even need the "-z" option.  When we read pathspec from a file,
> we can take any of CRLF, LF or NUL as the separator.
> 
> Ah, sorry, that would not help very much.  With "-z" we are allowing
> to express pathspec elements inside which there are embedded LF or
> CR/LF.  Sorry about the noise.

Yes, NUL delimiter allows to avoid escaping.

>   - Quote end-of-here-document (EOF in the above) if you do not rely
>     on parameter interpolation inside the here document; this helps
>     readers by telling them that what is presented is used verbatim.

Tests overwhelmingly prefer escaped form (\EOF) instead, so I used that.

> This feeds "fileA.t<NUL>fileB.t" without <NUL> after "fileB.t" to
> the command.  Intended?

Kind of intended. I changed tests to always use trailing delimiters, and 
also added a new dedicated test without trailing delimiter.

Patch
diff mbox series

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] [<tree-ish>] [--] <pathspec>...
+'git reset' [-q] [--pathspec-from-file=<file> [--pathspec-file-null]] [<tree-ish>]
 'git reset' (--patch | -p) [<tree-ish>] [--] [<pathspec>...]
 'git reset' [--soft | --mixed [-N] | --hard | --merge | --keep] [-q] [<commit>]
 
 DESCRIPTION
 -----------
-In the first and second form, copy entries from `<tree-ish>` to the index.
-In the third form, set the current branch head (`HEAD`) to `<commit>`,
+In the first three forms, copy entries from `<tree-ish>` to the index.
+In the last form, set the current branch head (`HEAD`) to `<commit>`,
 optionally modifying index and working tree to match.
 The `<tree-ish>`/`<commit>` defaults to `HEAD` in all forms.
 
 'git reset' [-q] [<tree-ish>] [--] <pathspec>...::
-	This form resets the index entries for all `<pathspec>` to their
+'git reset' [-q] [--pathspec-from-file=<file> [--pathspec-file-null]] [<tree-ish>]::
+	These forms reset the index entries for all `<pathspec>` to their
 	state at `<tree-ish>`.  (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=<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.
 
 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] [<commit>]"),
 	N_("git reset [-q] [<tree-ish>] [--] <paths>..."),
+	N_("git reset [-q] [--pathspec-from-file [--pathspec-file-null]] [<tree-ish>]"),
 	N_("git reset --patch [<tree-ish>] [--] [<paths>...]"),
 	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"));
+
+		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"));
+
 	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 <<EOF
+ D fileA.t
+EOF
+
+cat > expect.ab <<EOF
+ D fileA.t
+ D fileB.t
+EOF
+
+cat > expect.a_bc_d <<EOF
+D  fileA.t
+ D fileB.t
+ D fileC.t
+D  fileD.t
+EOF
+
+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
+'
+
+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' '
+	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' '
+	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