Re: [PATCH] t2028: fix minor error and issues in newly-added "worktree move" tests

2018-03-05 Thread Eric Sunshine 
On Mon, Mar 5, 2018 at 1:37 PM, Junio C Hamano  wrote:
> SZEDER Gábor  writes:
>
>> Could you please save 'git worktree's output into an intermediate
>> file, and run 'grep' on the file's contents?
>
> Here is what I tentatively came up with, while deciding what should
> be queued based on Eric's patch, as a possible squash/fixup.

Thanks for saving a round-trip. One comment below...

> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> @@ -74,8 +75,10 @@ test_expect_success 'move worktree' '
> toplevel="$(pwd)" &&
> git worktree move source destination &&
> test_path_is_missing source &&
> -   git worktree list --porcelain | grep "^worktree.*/destination" &&
> -   ! git worktree list --porcelain | grep "^worktree.*/source" &&
> +   git worktree list --porcelain >out &&
> +   grep "^worktree.*/destination" out &&
> +   git worktree list --porcelain >out &&
> +   ! grep "^worktree.*/source" out &&

The second "git worktree list --porcelain >out" can be dropped,
leaving the two grep's back-to-back since it they can test the same
'out' file

> git -C destination log --format=%s >actual2 &&
> echo init >expected2 &&
> test_cmp expected2 actual2

Re: [PATCH] t2028: fix minor error and issues in newly-added "worktree move" tests

2018-03-05 Thread Junio C Hamano 
SZEDER Gábor  writes:

> There is one more issue in these tests.
> ...
> The main purpose of this test script is to test the 'git worktree'
> command, but these pipes hide its exit code.
> Could you please save 'git worktree's output into an intermediate
> file, and run 'grep' on the file's contents?

Here is what I tentatively came up with, while deciding what should
be queued based on Eric's patch, as a possible squash/fixup.

 t/t2028-worktree-move.sh | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index d70d13dabe..1c391f370e 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -7,7 +7,8 @@ test_description='test git worktree move, remove, lock and 
unlock'
 test_expect_success 'setup' '
test_commit init &&
git worktree add source &&
-   git worktree list --porcelain | grep "^worktree" >actual &&
+   git worktree list --porcelain >out &&
+   grep "^worktree" out >actual &&
cat <<-EOF >expected &&
worktree $(pwd)
worktree $(pwd)/source
@@ -74,8 +75,10 @@ test_expect_success 'move worktree' '
toplevel="$(pwd)" &&
git worktree move source destination &&
test_path_is_missing source &&
-   git worktree list --porcelain | grep "^worktree.*/destination" &&
-   ! git worktree list --porcelain | grep "^worktree.*/source" &&
+   git worktree list --porcelain >out &&
+   grep "^worktree.*/destination" out &&
+   git worktree list --porcelain >out &&
+   ! grep "^worktree.*/source" out &&
git -C destination log --format=%s >actual2 &&
echo init >expected2 &&
test_cmp expected2 actual2
@@ -90,7 +93,8 @@ test_expect_success 'move worktree to another dir' '
git worktree move destination some-dir &&
test_when_finished "git worktree move some-dir/destination destination" 
&&
test_path_is_missing destination &&
-   git worktree list --porcelain | grep "^worktree.*/some-dir/destination" 
&&
+   git worktree list --porcelain >out &&
+   grep "^worktree.*/some-dir/destination" out &&
git -C some-dir/destination log --format=%s >actual2 &&
echo init >expected2 &&
test_cmp expected2 actual2

Re: [PATCH] t2028: fix minor error and issues in newly-added "worktree move" tests

2018-03-05 Thread SZEDER Gábor 
> Recently-added "git worktree move" tests include a minor error and a few
> small issues. Specifically:
> 
> * checking non-existence of wrong file ("source" instead of
>   "destination")
> 
> * unneeded redirect (">empty")
> 
> * unused variable ("toplevel")
> 
> * restoring a worktree location by means of a separate test somewhat
>   distant from the test which moved it rather than using
>   test_when_finished() to restore it in a self-contained fashion

There is one more issue in these tests.
 

> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> index 082368d8c6..d70d13dabe 100755
> --- a/t/t2028-worktree-move.sh
> +++ b/t/t2028-worktree-move.sh
> @@ -75,7 +75,7 @@ test_expect_success 'move worktree' '
>   git worktree move source destination &&
>   test_path_is_missing source &&
>   git worktree list --porcelain | grep "^worktree.*/destination" &&
> - ! git worktree list --porcelain | grep "^worktree.*/source" >empty &&
> + ! git worktree list --porcelain | grep "^worktree.*/source" &&

The main purpose of this test script is to test the 'git worktree'
command, but these pipes hide its exit code.
Could you please save 'git worktree's output into an intermediate
file, and run 'grep' on the file's contents?

This also applies to two other tests in this test script.

>   git -C destination log --format=%s >actual2 &&
>   echo init >expected2 &&
>   test_cmp expected2 actual2
> @@ -86,10 +86,10 @@ test_expect_success 'move main worktree' '
>  '
>  
>  test_expect_success 'move worktree to another dir' '
> - toplevel="$(pwd)" &&
>   mkdir some-dir &&
>   git worktree move destination some-dir &&
> - test_path_is_missing source &&
> + test_when_finished "git worktree move some-dir/destination destination" 
> &&
> + test_path_is_missing destination &&
>   git worktree list --porcelain | grep "^worktree.*/some-dir/destination" 
> &&
>   git -C some-dir/destination log --format=%s >actual2 &&
>   echo init >expected2 &&
> @@ -100,10 +100,6 @@ test_expect_success 'remove main worktree' '
>   test_must_fail git worktree remove .
>  '
>  
> -test_expect_success 'move some-dir/destination back' '
> - git worktree move some-dir/destination destination
> -'
> -
>  test_expect_success 'remove locked worktree' '
>   git worktree lock destination &&
>   test_when_finished "git worktree unlock destination" &&
> -- 
> 2.16.2.660.g709887971b
> 
>

Re: [PATCH] t2028: fix minor error and issues in newly-added "worktree move" tests

2018-03-05 Thread Duy Nguyen 
On Sun, Mar 4, 2018 at 12:26 PM, Eric Sunshine  wrote:
> Recently-added "git worktree move" tests include a minor error and a few
> small issues. Specifically:
>
> * checking non-existence of wrong file ("source" instead of
>   "destination")
>
> * unneeded redirect (">empty")
>
> * unused variable ("toplevel")
>
> * restoring a worktree location by means of a separate test somewhat
>   distant from the test which moved it rather than using
>   test_when_finished() to restore it in a self-contained fashion

Argh... You're right again :) This looks good.

>
> Signed-off-by: Eric Sunshine 
> ---
>
> This patch is built atop nd/worktree-move-reboot in 'next'.
>
> I didn't get around to doing a proper review of nd/worktree-move-reboot
> v2 [1] until after it had graduated to 'next'. Although v2 fixed all the
> issues identified in my review of v1 [2], it introduced a few minor
> issues of its own. This patch addresses those issues.
>
> [1]: https://public-inbox.org/git/20180212094940.23834-1-pclo...@gmail.com/
> [2]: https://public-inbox.org/git/20180124095357.19645-1-pclo...@gmail.com/
>
>  t/t2028-worktree-move.sh | 10 +++---
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> index 082368d8c6..d70d13dabe 100755
> --- a/t/t2028-worktree-move.sh
> +++ b/t/t2028-worktree-move.sh
> @@ -75,7 +75,7 @@ test_expect_success 'move worktree' '
> git worktree move source destination &&
> test_path_is_missing source &&
> git worktree list --porcelain | grep "^worktree.*/destination" &&
> -   ! git worktree list --porcelain | grep "^worktree.*/source" >empty &&
> +   ! git worktree list --porcelain | grep "^worktree.*/source" &&
> git -C destination log --format=%s >actual2 &&
> echo init >expected2 &&
> test_cmp expected2 actual2
> @@ -86,10 +86,10 @@ test_expect_success 'move main worktree' '
>  '
>
>  test_expect_success 'move worktree to another dir' '
> -   toplevel="$(pwd)" &&
> mkdir some-dir &&
> git worktree move destination some-dir &&
> -   test_path_is_missing source &&
> +   test_when_finished "git worktree move some-dir/destination 
> destination" &&
> +   test_path_is_missing destination &&
> git worktree list --porcelain | grep 
> "^worktree.*/some-dir/destination" &&
> git -C some-dir/destination log --format=%s >actual2 &&
> echo init >expected2 &&
> @@ -100,10 +100,6 @@ test_expect_success 'remove main worktree' '
> test_must_fail git worktree remove .
>  '
>
> -test_expect_success 'move some-dir/destination back' '
> -   git worktree move some-dir/destination destination
> -'
> -
>  test_expect_success 'remove locked worktree' '
> git worktree lock destination &&
> test_when_finished "git worktree unlock destination" &&
> --
> 2.16.2.660.g709887971b
>
-- 
Duy

[PATCH] t2028: fix minor error and issues in newly-added "worktree move" tests

2018-03-03 Thread Eric Sunshine 
Recently-added "git worktree move" tests include a minor error and a few
small issues. Specifically:

* checking non-existence of wrong file ("source" instead of
  "destination")

* unneeded redirect (">empty")

* unused variable ("toplevel")

* restoring a worktree location by means of a separate test somewhat
  distant from the test which moved it rather than using
  test_when_finished() to restore it in a self-contained fashion

Signed-off-by: Eric Sunshine 
---

This patch is built atop nd/worktree-move-reboot in 'next'.

I didn't get around to doing a proper review of nd/worktree-move-reboot
v2 [1] until after it had graduated to 'next'. Although v2 fixed all the
issues identified in my review of v1 [2], it introduced a few minor
issues of its own. This patch addresses those issues.

[1]: https://public-inbox.org/git/20180212094940.23834-1-pclo...@gmail.com/
[2]: https://public-inbox.org/git/20180124095357.19645-1-pclo...@gmail.com/

 t/t2028-worktree-move.sh | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 082368d8c6..d70d13dabe 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -75,7 +75,7 @@ test_expect_success 'move worktree' '
git worktree move source destination &&
test_path_is_missing source &&
git worktree list --porcelain | grep "^worktree.*/destination" &&
-   ! git worktree list --porcelain | grep "^worktree.*/source" >empty &&
+   ! git worktree list --porcelain | grep "^worktree.*/source" &&
git -C destination log --format=%s >actual2 &&
echo init >expected2 &&
test_cmp expected2 actual2
@@ -86,10 +86,10 @@ test_expect_success 'move main worktree' '
 '
 
 test_expect_success 'move worktree to another dir' '
-   toplevel="$(pwd)" &&
mkdir some-dir &&
git worktree move destination some-dir &&
-   test_path_is_missing source &&
+   test_when_finished "git worktree move some-dir/destination destination" 
&&
+   test_path_is_missing destination &&
git worktree list --porcelain | grep "^worktree.*/some-dir/destination" 
&&
git -C some-dir/destination log --format=%s >actual2 &&
echo init >expected2 &&
@@ -100,10 +100,6 @@ test_expect_success 'remove main worktree' '
test_must_fail git worktree remove .
 '
 
-test_expect_success 'move some-dir/destination back' '
-   git worktree move some-dir/destination destination
-'
-
 test_expect_success 'remove locked worktree' '
git worktree lock destination &&
test_when_finished "git worktree unlock destination" &&
-- 
2.16.2.660.g709887971b