[v6,0/2] add-patch: classify '@' as a synonym for 'HEAD'

Commit Message

Ghanshyam Thakkar Feb. 13, 2024, 12:05 a.m. UTC

I just noticed after sending v5 that, in one of the tests, while
moving it inside the loop for also testing '@', set_and_save_state was
not used therefore the subsequent tests after the first $opt will not have
the changes for which we prove 'y' thereby making the tests useless.
(Although it would not fail regardless of this). This got unnoticed by
the previous reviews since v1 and me as well.

Apologies for the oversight and noise.

Ghanshyam Thakkar (2):
  add-patch: classify '@' as a synonym for 'HEAD'
  add -p tests: remove PERL prerequisites

 add-patch.c                    |  8 ------
 builtin/checkout.c             |  4 ++-
 builtin/reset.c                |  4 ++-
 t/t2016-checkout-patch.sh      | 46 +++++++++++++++++++---------------
 t/t2020-checkout-detach.sh     | 12 +++++++++
 t/t2024-checkout-dwim.sh       |  2 +-
 t/t2071-restore-patch.sh       | 46 ++++++++++++++++++----------------
 t/t3904-stash-patch.sh         |  6 -----
 t/t7105-reset-patch.sh         | 38 +++++++++++++++-------------
 t/t7106-reset-unborn-branch.sh |  2 +-
 t/t7514-commit-patch.sh        |  6 -----
 11 files changed, 92 insertions(+), 82 deletions(-)

change since v5:

Comments

Phillip Wood Feb. 14, 2024, 11:06 a.m. UTC | #1

Hi Ghanshyam

On 13/02/2024 00:05, Ghanshyam Thakkar wrote:
> I just noticed after sending v5 that, in one of the tests, while
> moving it inside the loop for also testing '@', set_and_save_state was
> not used therefore the subsequent tests after the first $opt will not have
> the changes for which we prove 'y' thereby making the tests useless.
> (Although it would not fail regardless of this). This got unnoticed by
> the previous reviews since v1 and me as well.

This version all looks fine to me, thanks for working on it. Thanks for 
removing the PERL prerequisite from the remaining tests. I think the 
change to "git checkout @" is a good idea as it makes "@" act like 
"HEAD", hopefully there aren't too many users relying on "git checkout 
@" to detach HEAD.

Best Wishes

Phillip

> Apologies for the oversight and noise.
> 
> Ghanshyam Thakkar (2):
>    add-patch: classify '@' as a synonym for 'HEAD'
>    add -p tests: remove PERL prerequisites
> 
>   add-patch.c                    |  8 ------
>   builtin/checkout.c             |  4 ++-
>   builtin/reset.c                |  4 ++-
>   t/t2016-checkout-patch.sh      | 46 +++++++++++++++++++---------------
>   t/t2020-checkout-detach.sh     | 12 +++++++++
>   t/t2024-checkout-dwim.sh       |  2 +-
>   t/t2071-restore-patch.sh       | 46 ++++++++++++++++++----------------
>   t/t3904-stash-patch.sh         |  6 -----
>   t/t7105-reset-patch.sh         | 38 +++++++++++++++-------------
>   t/t7106-reset-unborn-branch.sh |  2 +-
>   t/t7514-commit-patch.sh        |  6 -----
>   11 files changed, 92 insertions(+), 82 deletions(-)
> 
> change since v5:
> diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
> index 0f597416d8..453872c8ba 100755
> --- a/t/t7105-reset-patch.sh
> +++ b/t/t7105-reset-patch.sh
> @@ -29,6 +29,7 @@ test_expect_success PERL 'saying "n" does nothing' '
>   for opt in "HEAD" "@" ""
>   do
>          test_expect_success PERL "git reset -p $opt" '
> +               set_and_save_state dir/foo work work &&
>                  test_write_lines n y | git reset -p $opt >output &&
>                  verify_state dir/foo work head &&
>                  verify_saved_state bar &&
>

Patch

diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
index 0f597416d8..453872c8ba 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -29,6 +29,7 @@  test_expect_success PERL 'saying "n" does nothing' '
 for opt in "HEAD" "@" ""
 do
        test_expect_success PERL "git reset -p $opt" '
+               set_and_save_state dir/foo work work &&
                test_write_lines n y | git reset -p $opt >output &&
                verify_state dir/foo work head &&
                verify_saved_state bar &&