Re: [PATCH 1/3] t7001: reformat to newer style

From: Derrick Stolee <stolee@gmail.com>
Date: Sep 24, 2018, 13:31 UTC
Message-ID: <7f77be2b-a084-5b8d-df65-1bb4b9c5da82@gmail.com>
In-Reply-To: <20180921235833.99045-2-sbeller@google.com>

On 9/21/2018 7:58 PM, Stefan Beller wrote:

> The old formatting style is a real hindrance of getting people up to speed
> contributing as they use existing code as an example and follow that style.
> So let's get rid of the old style and reformat it in our current style.

I was suspicious of your automated approach catching everything, so I looked carefully at this patch. There are still a lot of things happening that we would not recommend doing in new tests.

Show 35 quoted lines

>
> Reported-by: Derrick Stolee <stolee@gmail.com>
> Reported-by: Jeff Hostetler <git@jeffhostetler.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>   t/t7001-mv.sh | 268 +++++++++++++++++++++++++-------------------------
>   1 file changed, 134 insertions(+), 134 deletions(-)
>
> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
> index 36b50d0b4c1..2251d24735c 100755
> --- a/t/t7001-mv.sh
> +++ b/t/t7001-mv.sh
> @@ -3,74 +3,74 @@
>   test_description='git mv in subdirs'
>   . ./test-lib.sh
>   
> -test_expect_success \
> -    'prepare reference tree' \
> -    'mkdir path0 path1 &&
> -     cp "$TEST_DIRECTORY"/../COPYING path0/COPYING &&
> -     git add path0/COPYING &&
> -     git commit -m add -a'
> +test_expect_success 'prepare reference tree' '
> +	mkdir path0 path1 &&
> +	cp "$TEST_DIRECTORY"/../COPYING path0/COPYING &&
> +	git add path0/COPYING &&
> +	git commit -m add -a
> +'
>   
> -test_expect_success \
> -    'moving the file out of subdirectory' \
> -    'cd path0 && git mv COPYING ../path1/COPYING'
> +test_expect_success 'moving the file out of subdirectory' '
> +	cd path0 && git mv COPYING ../path1/COPYING
> +'

Perhaps split this line on the &&?

Show 8 quoted lines

>   
>   # in path0 currently
> -test_expect_success \
> -    'commiting the change' \
> -    'cd .. && git commit -m move-out -a'
> +test_expect_success 'commiting the change' '
> +	cd .. && git commit -m move-out -a
> +'

This "cd .." should probably be removed and use a subshell in the test above where we "cd path0".

Show 16 quoted lines

>   
> -test_expect_success \
> -    'checking the commit' \
> -    'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
> -    grep "^R100..*path0/COPYING..*path1/COPYING" actual'
> +test_expect_success 'checking the commit' '
> +	git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
> +	grep "^R100..*path0/COPYING..*path1/COPYING" actual
> +'
>   
> -test_expect_success \
> -    'moving the file back into subdirectory' \
> -    'cd path0 && git mv ../path1/COPYING COPYING'
> +test_expect_success 'moving the file back into subdirectory' '
> +	cd path0 && git mv ../path1/COPYING COPYING
> +'

Split at &&, use subshell?

> +test_expect_success 'commiting the change' '
> +	cd .. && git commit -m move-in -a
> +'

Drop "cd .." (and the comments about being in path0)

[big snip]

> +test_expect_success 'moving to existing tracked target with trailing slash' '
> +	mkdir path2 &&
> +	>path2/file && git add path2/file &&

This line in particular looks a bit strange. What is this doing? At least we should split the &&.

Show 49 quoted lines

> +	git mv path1/path0/ path2/ &&
> +	test_path_is_dir path2/path0/
> +'
> +
> +test_expect_success 'clean up' '
> +	git reset --hard
> +'
> +
> +test_expect_success 'adding another file' '
> +	cp "$TEST_DIRECTORY"/../README.md path0/README &&
> +	git add path0/README &&
> +	git commit -m add2 -a
> +'
> +
> +test_expect_success 'moving whole subdirectory' '
> +	git mv path0 path2
> +'
> +
> +test_expect_success 'commiting the change' '
> +	git commit -m dir-move -a
> +'
> +
> +test_expect_success 'checking the commit' '
> +	git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
> +	grep "^R100..*path0/COPYING..*path2/COPYING" actual &&
> +	grep "^R100..*path0/README..*path2/README" actual
> +'
> +
> +test_expect_success 'succeed when source is a prefix of destination' '
> +	git mv path2/COPYING path2/COPYING-renamed
> +'
> +
> +test_expect_success 'moving whole subdirectory into subdirectory' '
> +	git mv path2 path1
> +'
> +
> +test_expect_success 'commiting the change' '
> +	git commit -m dir-move -a
> +'
> +
> +test_expect_success 'checking the commit' '
> +	git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
> +	grep "^R100..*path2/COPYING..*path1/path2/COPYING" actual &&
> +	grep "^R100..*path2/README..*path1/path2/README" actual
> +'
> +
> +test_expect_success 'do not move directory over existing directory' '
> +	mkdir path0 && mkdir path0/path2 && test_must_fail git mv path2 path0
> +'

Split this line.

Thanks,

-Stolee