From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 17 Dec 2019 20:00:43 GMT
Subject: Re: [PATCH v2 04/18] commit: forbid --pathspec-from-file --all
Message-ID: <195a5b2a-994a-5984-8cc7-280a698df2a0@gmail.com>
In-Reply-To: <deeb860a85d25e0645a5d2e1c82654653ab1e2d5.1576511287.git.gitgitgadget@gmail.com>

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