| Message ID | deeb860a85d25e0645a5d2e1c82654653ab1e2d5.1576511287.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Extend --pathspec-from-file to git add, checkout | expand |
Hi Alexandr This looks good, thanks for the test Best Wishes Phillip On 16/12/2019 15:47, Alexandr Miloslavskiy via GitGitGadget wrote: > From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> > > I forgot this in my previous patch `--pathspec-from-file` for > `git commit` [1]. When both `--pathspec-from-file` and `--all` were > specified, `--all` took precedence and `--pathspec-from-file` was > ignored. Before `--pathspec-from-file` was implemented, this case was > prevented by this check in `parse_and_validate_options()` : > > die(_("paths '%s ...' with -a does not make sense"), argv[0]); > > It is unfortunate that these two cases are disconnected. This came as > result of how the code was laid out before my patches, where `pathspec` > is parsed outside of `parse_and_validate_options()`. This branch is > already full of refactoring patches and I did not dare to go for another > one. > > Fix by mirroring `die()` for `--pathspec-from-file` as well. > > [1] Commit e440fc58 ("commit: support the --pathspec-from-file option" 2019-11-19) > > Reported-by: Phillip Wood <phillip.wood@dunelm.org.uk> > Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> > --- > builtin/commit.c | 3 +++ > t/t7526-commit-pathspec-file.sh | 3 +++ > 2 files changed, 6 insertions(+) > > diff --git a/builtin/commit.c b/builtin/commit.c > index 2db2ad0de4..893a9f29b2 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -347,6 +347,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix > if (interactive) > die(_("--pathspec-from-file is incompatible with --interactive/--patch")); > > + if (all) > + die(_("--pathspec-from-file with -a does not make sense")); > + > if (pathspec.nr) > die(_("--pathspec-from-file is incompatible with pathspec arguments")); > > diff --git a/t/t7526-commit-pathspec-file.sh b/t/t7526-commit-pathspec-file.sh > index 68920e8ff9..ba769e0e5d 100755 > --- a/t/t7526-commit-pathspec-file.sh > +++ b/t/t7526-commit-pathspec-file.sh > @@ -72,6 +72,9 @@ test_expect_success 'error conditions' ' > test_must_fail git commit --pathspec-from-file=- --patch -m "Commit" <list 2>err && > test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-interactive/\-\-patch" err && > > + test_must_fail git commit --pathspec-from-file=- --all -m "Commit" <list 2>err && > + test_i18ngrep "\-\-pathspec-from-file with \-a does not make sense" err && > + > test_must_fail git commit --pathspec-from-file=- -m "Commit" -- fileA.t <list 2>err && > test_i18ngrep "\-\-pathspec-from-file is incompatible with pathspec arguments" err && > >
Phillip Wood <phillip.wood123@gmail.com> writes: > Hi Alexandr > > This looks good, thanks for the test > > Best Wishes > > Phillip > > On 16/12/2019 15:47, Alexandr Miloslavskiy via GitGitGadget wrote: >> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> >> >> I forgot this in my previous patch `--pathspec-from-file` for >> `git commit` [1]. When both `--pathspec-from-file` and `--all` were >> ... >> [1] Commit e440fc58 ("commit: support the --pathspec-from-file option" 2019-11-19) Thanks, both. I will take this separately and queue directly on top of am/pathspec-from-file to fast-track it, rather than leaving it as a part of larger topic that would take more time to mature.
Junio C Hamano <gitster@pobox.com> writes: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> Hi Alexandr >> >> This looks good, thanks for the test >> >> Best Wishes >> >> Phillip >> >> On 16/12/2019 15:47, Alexandr Miloslavskiy via GitGitGadget wrote: >>> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> >>> >>> I forgot this in my previous patch `--pathspec-from-file` for >>> `git commit` [1]. When both `--pathspec-from-file` and `--all` were >>> ... >>> [1] Commit e440fc58 ("commit: support the --pathspec-from-file option" 2019-11-19) > > Thanks, both. I will take this separately and queue directly on top > of am/pathspec-from-file to fast-track it, rather than leaving it as > a part of larger topic that would take more time to mature. Sigh... the test part of this patch is taken hostage to an earlier patches in this series that are iffy, so I cannot quite apply this fix alone at this moment. Yuck.
Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Phillip Wood <phillip.wood123@gmail.com> writes: >> >>> Hi Alexandr >>> >>> This looks good, thanks for the test >>> >>> Best Wishes >>> >>> Phillip >>> >>> On 16/12/2019 15:47, Alexandr Miloslavskiy via GitGitGadget wrote: >>>> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> >>>> >>>> I forgot this in my previous patch `--pathspec-from-file` for >>>> `git commit` [1]. When both `--pathspec-from-file` and `--all` were >>>> ... >>>> [1] Commit e440fc58 ("commit: support the --pathspec-from-file option" 2019-11-19) >> >> Thanks, both. I will take this separately and queue directly on top >> of am/pathspec-from-file to fast-track it, rather than leaving it as >> a part of larger topic that would take more time to mature. > > Sigh... the test part of this patch is taken hostage to an earlier > patches in this series that are iffy, so I cannot quite apply this > fix alone at this moment. > > Yuck. Here is what I'll queue directly on top of am/pathspec-from-file e440fc58 ("commit: support the --pathspec-from-file option", 2019-11-19), to be fast-tracked. -- >8 -- Subject: [PATCH] commit: forbid --pathspec-from-file --all I forgot this in my previous patch `--pathspec-from-file` for `git commit` [1]. When both `--pathspec-from-file` and `--all` were specified, `--all` took precedence and `--pathspec-from-file` was ignored. Before `--pathspec-from-file` was implemented, this case was prevented by this check in `parse_and_validate_options()` : die(_("paths '%s ...' with -a does not make sense"), argv[0]); It is unfortunate that these two cases are disconnected. This came as result of how the code was laid out before my patches, where `pathspec` is parsed outside of `parse_and_validate_options()`. This branch is already full of refactoring patches and I did not dare to go for another one. Fix by mirroring `die()` for `--pathspec-from-file` as well. [1] Commit e440fc58 ("commit: support the --pathspec-from-file option" 2019-11-19) Reported-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> [jc: adjusted test not to depend on other patches] Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/commit.c | 3 +++ t/t7526-commit-pathspec-file.sh | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/builtin/commit.c b/builtin/commit.c index ed40729355..c040dc92a4 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -347,6 +347,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix if (interactive) die(_("--pathspec-from-file is incompatible with --interactive/--patch")); + if (all) + die(_("--pathspec-from-file with -a does not make sense")); + if (pathspec.nr) die(_("--pathspec-from-file is incompatible with pathspec arguments")); diff --git a/t/t7526-commit-pathspec-file.sh b/t/t7526-commit-pathspec-file.sh index a06b683534..4b58901ed6 100755 --- a/t/t7526-commit-pathspec-file.sh +++ b/t/t7526-commit-pathspec-file.sh @@ -127,4 +127,10 @@ test_expect_success 'only touches what was listed' ' verify_expect ' +test_expect_success '--pathspec-from-file and --all cannot be used together' ' + restore_checkpoint && + test_must_fail git commit --pathspec-from-file=- --all -m "Commit" 2>err && + test_i18ngrep "[-]-pathspec-from-file with -a does not make sense" err +' + test_done
On 18.12.2019 23:16, Junio C Hamano wrote: > Here is what I'll queue directly on top of am/pathspec-from-file > e440fc58 ("commit: support the --pathspec-from-file option", > 2019-11-19), to be fast-tracked. Roger. I will keep my branch as is currently and will rebase it on top of your patch once it's in master.
diff --git a/builtin/commit.c b/builtin/commit.c index 2db2ad0de4..893a9f29b2 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -347,6 +347,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix if (interactive) die(_("--pathspec-from-file is incompatible with --interactive/--patch")); + if (all) + die(_("--pathspec-from-file with -a does not make sense")); + if (pathspec.nr) die(_("--pathspec-from-file is incompatible with pathspec arguments")); diff --git a/t/t7526-commit-pathspec-file.sh b/t/t7526-commit-pathspec-file.sh index 68920e8ff9..ba769e0e5d 100755 --- a/t/t7526-commit-pathspec-file.sh +++ b/t/t7526-commit-pathspec-file.sh @@ -72,6 +72,9 @@ test_expect_success 'error conditions' ' test_must_fail git commit --pathspec-from-file=- --patch -m "Commit" <list 2>err && test_i18ngrep "\-\-pathspec-from-file is incompatible with \-\-interactive/\-\-patch" err && + test_must_fail git commit --pathspec-from-file=- --all -m "Commit" <list 2>err && + test_i18ngrep "\-\-pathspec-from-file with \-a does not make sense" err && + test_must_fail git commit --pathspec-from-file=- -m "Commit" -- fileA.t <list 2>err && test_i18ngrep "\-\-pathspec-from-file is incompatible with pathspec arguments" err &&