git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Nov 5, 2019, 15:03 UTC
Message-ID
<b93592df-561f-36d4-09ce-e02d570c60da@gmail.com>
In-Reply-To
<8d9f1fbc18346144a0c866a59891b652dcfe9b7f.1572895605.git.gitgitgadget@gmail.com>
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:
Show 11 quoted lines
> 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

Show 6 quoted lines
> 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).

Show 19 quoted lines
> 
> 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?

Show 15 quoted lines
>   '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
Show 9 quoted lines
>   	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".

Show 53 quoted lines
> 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

Show 9 quoted lines
> +
> +		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.

Show 31 quoted lines
> +
>   	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)

Show 72 quoted lines
> +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.

Show 14 quoted lines
> +'
> +
> +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/
Show 9 quoted lines
> +	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
Show 10 quoted lines
> +	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
>
Previous: Alexandr Miloslavskiy via GitGitGadgetNext: Phillip Wood
Message 7 of 80 in “Add --pathspec-from-file option for reset, commit”
  1. 0/5 Add --pathspec-from-file option for reset, commitAlexandr Miloslavskiy via GitGitGadget, Nov 4, 2019
  2. 1/5 pathspec: add new function to parse fileAlexandr Miloslavskiy via GitGitGadget, Nov 4, 2019
  3. Phillip WoodNov 5, 2019
  4. Alexandr MiloslavskiyNov 5, 2019
  5. Alexandr MiloslavskiyNov 6, 2019
  6. 3/5 reset: support the --pathspec-from-file optionAlexandr Miloslavskiy via GitGitGadget, Nov 4, 2019
  7. Phillip WoodNov 5, 2019
  8. Phillip WoodNov 5, 2019
  9. Alexandr MiloslavskiyNov 5, 2019
  10. Alexandr MiloslavskiyNov 6, 2019
  11. Phillip WoodNov 5, 2019
  12. Alexandr MiloslavskiyNov 5, 2019
  13. Junio C HamanoNov 6, 2019
  14. Alexandr MiloslavskiyNov 6, 2019
  15. 4/5 doc: commit: unify <pathspec> descriptionAlexandr Miloslavskiy via GitGitGadget, Nov 4, 2019
  16. Junio C HamanoNov 6, 2019
  17. Alexandr MiloslavskiyNov 6, 2019
  18. Junio C HamanoNov 7, 2019
  19. Alexandr MiloslavskiyNov 7, 2019
  20. 5/5 commit: support the --pathspec-from-file optionAlexandr Miloslavskiy via GitGitGadget, Nov 4, 2019
  21. Phillip WoodNov 5, 2019
  22. Alexandr MiloslavskiyNov 5, 2019
  23. Alexandr MiloslavskiyNov 6, 2019
  24. Phillip WoodDec 10, 2019
  25. Alexandr MiloslavskiyDec 11, 2019
  26. Phillip WoodDec 11, 2019
  27. Alexandr MiloslavskiyDec 11, 2019
  28. Junio C HamanoDec 11, 2019
  29. Alexandr MiloslavskiyDec 11, 2019
  30. Alexandr MiloslavskiyDec 12, 2019
  31. Junio C HamanoNov 6, 2019
  32. 2/5 doc: reset: unify <pathspec> descriptionAlexandr Miloslavskiy via GitGitGadget, Nov 4, 2019
  33. Junio C HamanoNov 6, 2019
  34. Alexandr MiloslavskiyNov 6, 2019
  35. Junio C HamanoNov 7, 2019
  36. Alexandr MiloslavskiyNov 7, 2019
  37. Junio C HamanoNov 8, 2019
  38. 0/6 Add --pathspec-from-file option for reset, commitAlexandr Miloslavskiy via GitGitGadget, Nov 6, 2019
  39. 1/6 parse-options.h: add new options `--pathspec-from-file`, `--pathspec-file-nul`Alexandr Miloslavskiy via GitGitGadget, Nov 6, 2019
  40. 2/6 pathspec: add new function to parse fileAlexandr Miloslavskiy via GitGitGadget, Nov 6, 2019
  41. Junio C HamanoNov 19, 2019
  42. Alexandr MiloslavskiyNov 19, 2019
  43. 3/6 doc: reset: unify <pathspec> descriptionAlexandr Miloslavskiy via GitGitGadget, Nov 6, 2019
  44. Junio C HamanoNov 19, 2019
  45. Alexandr MiloslavskiyNov 19, 2019
  46. 6/6 commit: support the --pathspec-from-file optionAlexandr Miloslavskiy via GitGitGadget, Nov 6, 2019
  47. Junio C HamanoNov 19, 2019
  48. Alexandr MiloslavskiyNov 19, 2019
  49. 5/6 doc: commit: unify <pathspec> descriptionAlexandr Miloslavskiy via GitGitGadget, Nov 6, 2019
  50. Junio C HamanoNov 19, 2019
  51. Alexandr MiloslavskiyNov 19, 2019
  52. Alexandr MiloslavskiyNov 19, 2019
  53. 4/6 reset: support the `--pathspec-from-file` optionAlexandr Miloslavskiy via GitGitGadget, Nov 6, 2019
  54. 0/6 Add --pathspec-from-file option for reset, commitAlexandr Miloslavskiy via GitGitGadget, Nov 19, 2019
  55. 3/6 doc: reset: synchronize <pathspec> descriptionAlexandr Miloslavskiy via GitGitGadget, Nov 19, 2019
  56. 2/6 pathspec: add new function to parse fileAlexandr Miloslavskiy via GitGitGadget, Nov 19, 2019
  57. 5/6 doc: commit: synchronize <pathspec> descriptionAlexandr Miloslavskiy via GitGitGadget, Nov 19, 2019
  58. 4/6 reset: support the `--pathspec-from-file` optionAlexandr Miloslavskiy via GitGitGadget, Nov 19, 2019
  59. 1/6 parse-options.h: add new options `--pathspec-from-file`, `--pathspec-file-nul`Alexandr Miloslavskiy via GitGitGadget, Nov 19, 2019
  60. 6/6 commit: support the --pathspec-from-file optionAlexandr Miloslavskiy via GitGitGadget, Nov 19, 2019
  61. Junio C HamanoNov 20, 2019
  62. Alexandr MiloslavskiyNov 20, 2019
  63. 00/13 Add --pathspec-from-file optionAlexandr Miloslavskiy via GitGitGadget, Dec 3, 2019
  64. 01/13 parse-options.h: add new options `--pathspec-from-file`, `--pathspec-file-nul`Alexandr Miloslavskiy via GitGitGadget, Dec 3, 2019
  65. 03/13 doc: reset: synchronize <pathspec> descriptionAlexandr Miloslavskiy via GitGitGadget, Dec 3, 2019
  66. 02/13 pathspec: add new function to parse fileAlexandr Miloslavskiy via GitGitGadget, Dec 3, 2019
  67. 05/13 doc: commit: synchronize <pathspec> descriptionAlexandr Miloslavskiy via GitGitGadget, Dec 3, 2019
  68. 06/13 commit: support the --pathspec-from-file optionAlexandr Miloslavskiy via GitGitGadget, Dec 3, 2019
  69. 10/13 doc: checkout: fix broken text referenceAlexandr Miloslavskiy via GitGitGadget, Dec 3, 2019
  70. 07/13 cmd_add: prepare for next patchAlexandr Miloslavskiy via GitGitGadget, Dec 3, 2019
  71. 09/13 doc: checkout: remove duplicate synopsisAlexandr Miloslavskiy via GitGitGadget, Dec 3, 2019
  72. 04/13 reset: support the `--pathspec-from-file` optionAlexandr Miloslavskiy via GitGitGadget, Dec 3, 2019
  73. 08/13 add: support the --pathspec-from-file optionAlexandr Miloslavskiy via GitGitGadget, Dec 3, 2019
  74. 13/13 checkout, restore: support the --pathspec-from-file optionAlexandr Miloslavskiy via GitGitGadget, Dec 3, 2019
  75. 11/13 doc: checkout: synchronize <pathspec> descriptionAlexandr Miloslavskiy via GitGitGadget, Dec 3, 2019
  76. 12/13 doc: restore: synchronize <pathspec> descriptionAlexandr Miloslavskiy via GitGitGadget, Dec 3, 2019
  77. Junio C HamanoDec 3, 2019
  78. Alexandr MiloslavskiyDec 3, 2019
  79. Junio C HamanoDec 4, 2019
  80. Alexandr MiloslavskiyDec 5, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.