From: Phillip Wood <phillip.wood123@gmail.com>
To: Alexandr Miloslavskiy via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Cc: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>,
Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 3/5] reset: support the --pathspec-from-file option
Date: Tue, 5 Nov 2019 15:03:56 +0000 [thread overview]
Message-ID: <b93592df-561f-36d4-09ce-e02d570c60da@gmail.com> (raw)
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:
> 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
>
next prev parent reply other threads:[~2019-11-05 15:04 UTC|newest]
Thread overview: 80+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-04 19:26 [PATCH 0/5] Add --pathspec-from-file option for reset, commit Alexandr Miloslavskiy via GitGitGadget
2019-11-04 19:26 ` [PATCH 1/5] pathspec: add new function to parse file Alexandr Miloslavskiy via GitGitGadget
2019-11-05 15:02 ` Phillip Wood
2019-11-05 19:14 ` Alexandr Miloslavskiy
2019-11-06 15:56 ` Alexandr Miloslavskiy
2019-11-04 19:26 ` [PATCH 2/5] doc: reset: unify <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2019-11-06 4:01 ` Junio C Hamano
2019-11-06 15:56 ` Alexandr Miloslavskiy
2019-11-07 5:46 ` Junio C Hamano
2019-11-07 11:05 ` Alexandr Miloslavskiy
2019-11-08 3:04 ` Junio C Hamano
2019-11-04 19:26 ` [PATCH 3/5] reset: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2019-11-05 15:03 ` Phillip Wood [this message]
2019-11-05 19:22 ` Phillip Wood
2019-11-05 19:36 ` Alexandr Miloslavskiy
2019-11-06 15:56 ` Alexandr Miloslavskiy
2019-11-05 16:14 ` Phillip Wood
2019-11-05 19:37 ` Alexandr Miloslavskiy
2019-11-06 4:40 ` Junio C Hamano
2019-11-06 15:56 ` Alexandr Miloslavskiy
2019-11-04 19:26 ` [PATCH 4/5] doc: commit: unify <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2019-11-06 4:50 ` Junio C Hamano
2019-11-06 15:56 ` Alexandr Miloslavskiy
2019-11-07 5:54 ` Junio C Hamano
2019-11-07 11:39 ` Alexandr Miloslavskiy
2019-11-04 19:26 ` [PATCH 5/5] commit: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2019-11-05 16:27 ` Phillip Wood
2019-11-05 19:42 ` Alexandr Miloslavskiy
2019-11-06 15:56 ` Alexandr Miloslavskiy
2019-12-10 10:42 ` Phillip Wood
2019-12-11 11:43 ` Alexandr Miloslavskiy
2019-12-11 14:27 ` Phillip Wood
2019-12-11 15:06 ` Alexandr Miloslavskiy
2019-12-11 16:14 ` Junio C Hamano
2019-12-11 16:20 ` Alexandr Miloslavskiy
2019-12-12 14:56 ` Alexandr Miloslavskiy
2019-11-06 4:51 ` Junio C Hamano
2019-11-06 15:51 ` [PATCH v2 0/6] Add --pathspec-from-file option for reset, commit Alexandr Miloslavskiy via GitGitGadget
2019-11-06 15:51 ` [PATCH v2 1/6] parse-options.h: add new options `--pathspec-from-file`, `--pathspec-file-nul` Alexandr Miloslavskiy via GitGitGadget
2019-11-06 15:51 ` [PATCH v2 2/6] pathspec: add new function to parse file Alexandr Miloslavskiy via GitGitGadget
2019-11-19 5:59 ` Junio C Hamano
2019-11-19 16:50 ` Alexandr Miloslavskiy
2019-11-06 15:51 ` [PATCH v2 3/6] doc: reset: unify <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2019-11-19 6:05 ` Junio C Hamano
2019-11-19 16:52 ` Alexandr Miloslavskiy
2019-11-06 15:51 ` [PATCH v2 4/6] reset: support the `--pathspec-from-file` option Alexandr Miloslavskiy via GitGitGadget
2019-11-06 15:51 ` [PATCH v2 5/6] doc: commit: unify <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2019-11-19 6:16 ` Junio C Hamano
2019-11-19 16:53 ` Alexandr Miloslavskiy
2019-11-19 17:02 ` Alexandr Miloslavskiy
2019-11-06 15:51 ` [PATCH v2 6/6] commit: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2019-11-19 6:10 ` Junio C Hamano
2019-11-19 16:56 ` Alexandr Miloslavskiy
2019-11-19 16:48 ` [PATCH v3 0/6] Add --pathspec-from-file option for reset, commit Alexandr Miloslavskiy via GitGitGadget
2019-11-19 16:48 ` [PATCH v3 1/6] parse-options.h: add new options `--pathspec-from-file`, `--pathspec-file-nul` Alexandr Miloslavskiy via GitGitGadget
2019-11-19 16:48 ` [PATCH v3 2/6] pathspec: add new function to parse file Alexandr Miloslavskiy via GitGitGadget
2019-11-19 16:48 ` [PATCH v3 3/6] doc: reset: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2019-11-19 16:48 ` [PATCH v3 4/6] reset: support the `--pathspec-from-file` option Alexandr Miloslavskiy via GitGitGadget
2019-11-19 16:48 ` [PATCH v3 5/6] doc: commit: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2019-11-19 16:48 ` [PATCH v3 6/6] commit: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2019-11-20 4:04 ` [PATCH v3 0/6] Add --pathspec-from-file option for reset, commit Junio C Hamano
2019-11-20 9:22 ` Alexandr Miloslavskiy
2019-12-03 14:02 ` [PATCH v4 00/13] Add --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2019-12-03 14:02 ` [PATCH v4 01/13] parse-options.h: add new options `--pathspec-from-file`, `--pathspec-file-nul` Alexandr Miloslavskiy via GitGitGadget
2019-12-03 14:02 ` [PATCH v4 02/13] pathspec: add new function to parse file Alexandr Miloslavskiy via GitGitGadget
2019-12-03 14:02 ` [PATCH v4 03/13] doc: reset: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2019-12-03 14:02 ` [PATCH v4 04/13] reset: support the `--pathspec-from-file` option Alexandr Miloslavskiy via GitGitGadget
2019-12-03 14:02 ` [PATCH v4 05/13] doc: commit: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2019-12-03 14:02 ` [PATCH v4 06/13] commit: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2019-12-03 14:02 ` [PATCH v4 07/13] cmd_add: prepare for next patch Alexandr Miloslavskiy via GitGitGadget
2019-12-03 14:02 ` [PATCH v4 08/13] add: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2019-12-03 14:02 ` [PATCH v4 09/13] doc: checkout: remove duplicate synopsis Alexandr Miloslavskiy via GitGitGadget
2019-12-03 14:02 ` [PATCH v4 10/13] doc: checkout: fix broken text reference Alexandr Miloslavskiy via GitGitGadget
2019-12-03 14:02 ` [PATCH v4 11/13] doc: checkout: synchronize <pathspec> description Alexandr Miloslavskiy via GitGitGadget
2019-12-03 14:02 ` [PATCH v4 12/13] doc: restore: " Alexandr Miloslavskiy via GitGitGadget
2019-12-03 14:02 ` [PATCH v4 13/13] checkout, restore: support the --pathspec-from-file option Alexandr Miloslavskiy via GitGitGadget
2019-12-03 16:55 ` [PATCH v4 00/13] Add " Junio C Hamano
2019-12-03 17:06 ` Alexandr Miloslavskiy
2019-12-04 19:25 ` Junio C Hamano
2019-12-05 10:43 ` Alexandr Miloslavskiy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b93592df-561f-36d4-09ce-e02d570c60da@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=alexandr.miloslavskiy@syntevo.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).