[v2,04/18] commit: forbid --pathspec-from-file --all

Commit Message

Philippe Blain via GitGitGadget Dec. 16, 2019, 3:47 p.m. UTC

From: Alexandr Miloslavskiy <[e-mail]>

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 <[e-mail]>
Signed-off-by: Alexandr Miloslavskiy <[e-mail]>
---
 builtin/commit.c                | 3 +++
 t/t7526-commit-pathspec-file.sh | 3 +++
 2 files changed, 6 insertions(+)

Comments

Phillip Wood Dec. 17, 2019, 8 p.m. UTC | #1

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 <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Alexandr Miloslavskiy <[e-mail]>
> ---
>   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 &&
>   
>

Junio C Hamano Dec. 18, 2019, 10:04 p.m. UTC | #2

Phillip Wood <[e-mail]> 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 <[e-mail]>
>>
>> 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 Dec. 18, 2019, 10:06 p.m. UTC | #3

Junio C Hamano <[e-mail]> writes:

> Phillip Wood <[e-mail]> 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 <[e-mail]>
>>>
>>> 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 Dec. 18, 2019, 10:16 p.m. UTC | #4

Junio C Hamano <[e-mail]> writes:

> Junio C Hamano <[e-mail]> writes:
>
>> Phillip Wood <[e-mail]> 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 <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
Signed-off-by: Alexandr Miloslavskiy <[e-mail]>
[jc: adjusted test not to depend on other patches]
Signed-off-by: Junio C Hamano <[e-mail]>
---
 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

Alexandr Miloslavskiy Dec. 19, 2019, 5:38 p.m. UTC | #5

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.

Patch

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