Re: [PATCH v2 1/2] modernize t7300

[Junio C Hamano]

James  writes:

> From: James Rouzier 
>
> ---

Missing sign-off.

>  t/t7300-clean.sh | 382 
> +++
>  1 file changed, 190 insertions(+), 192 deletions(-)
>
> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
> index 86ceb38..d555bb6 100755
> --- a/t/t7300-clean.sh
> +++ b/t/t7300-clean.sh
> @@ -28,15 +28,15 @@ test_expect_success 'git clean with skip-worktree 
> .gitignore' '
>   mkdir -p build docs &&
>   touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>   git clean &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test ! -f a.out &&
> - test ! -f src/part3.c &&
> - test -f docs/manual.txt &&
> - test -f obj.o &&
> - test -f build/lib.so &&
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&

OK.

> + test_path_is_missing a.out &&
> + test_path_is_missing src/part3.c &&

This is better than the original, which may have said "OK" upon
seeing a directory called "a.out".

> + test_path_is_file docs/manual.txt &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so &&

OK.

>   git update-index --no-skip-worktree .gitignore &&
>   git checkout .gitignore
>  '
> @@ -46,15 +46,15 @@ test_expect_success 'git clean' '
>   mkdir -p build docs &&
>   touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>   git clean &&
> - test -f Makefile &&
> - test -f README &&
> - test -f src/part1.c &&
> - test -f src/part2.c &&
> - test ! -f a.out &&
> - test ! -f src/part3.c &&
> - test -f docs/manual.txt &&
> - test -f obj.o &&
> - test -f build/lib.so
> + test_path_is_file Makefile &&
> + test_path_is_file README &&
> + test_path_is_file src/part1.c &&
> + test_path_is_file src/part2.c &&
> + test_path_is_missing a.out &&
> + test_path_is_missing src/part3.c &&
> + test_path_is_file docs/manual.txt &&
> + test_path_is_file obj.o &&
> + test_path_is_file build/lib.so

The verbosity of this conversion makes me wonder if we want to have
"test_paths_are_files" and "test_paths_are_missing".  For that
matter, this test does not really care about the distinction between
files and directories (e.g. some tests said "test ! -d docs" and
would have passed if there were a 'docs' regular file, but what we
really care about is the path 'docs' is _gone_), so what we want may
be test_paths_exist and test_paths_are_missing.  With that, the
above hunk would become

test_paths_exist Makefile README src/part1.c src/part2.c \
obj.o build/lib.so &&
test_paths_are_missing a.out src/part3.c

I dunno.

>  test_expect_success 'git clean -e' '
>   rm -fr repo &&
>   mkdir repo &&
> - (
> - cd repo &&
> - git init &&
> - touch known 1 2 3 &&
> - git add known &&
> - git clean -f -e 1 -e 2 &&
> - test -e 1 &&
> - test -e 2 &&
> - ! (test -e 3) &&
> - test -e known
> - )
> + cd repo &&
> + git init &&
> + touch known 1 2 3 &&
> + git add known &&
> + git clean -f -e 1 -e 2 &&
> + test_path_is_file 1 &&
> + test_path_is_file 2 &&
> + test_path_is_missing 3 &&
> + test_path_is_file known
>  '

I think this is wrong.  The next test piece will be run inside
"repo" directory with this patch applied.  Don't lose the subshell
here.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] modernize t7300

[Eric Sunshine]

On Sun, Dec 6, 2015 at 9:58 AM, James  wrote:
> From: James Rouzier 

This would be a good place to explain how you are modernizing the test
script. Right now, you're just updating to take advantage of
test_path_is_foo(), but some other modernizations you could do
include:

* using '>' rather than 'touch' to create empty files when the
timestamp doesn't matter (which is all cases in this script)

* (optional) replace unnecessarily complex 'mkdir -p foo' with simpler
'mkdir foo' (but leave "mkdir -p foo/bar" as is)

* drop blank lines before and after test body; for instance:

test_expect_success 'foo' '

blah &&
bloo

'

becomes:

test_expect_success 'foo' '
blah &&
bloo
'

> ---
> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
> @@ -609,32 +609,30 @@ test_expect_success 'force removal of nested git work 
> tree' '
>  test_expect_success 'git clean -e' '
> rm -fr repo &&
> mkdir repo &&
> -   (
> -   cd repo &&
> -   git init &&
> -   touch known 1 2 3 &&
> -   git add known &&
> -   git clean -f -e 1 -e 2 &&
> -   test -e 1 &&
> -   test -e 2 &&
> -   ! (test -e 3) &&
> -   test -e known
> -   )
> +   cd repo &&

The previous working directory is not automatically restored at the
end of the test, so this "cd" without corresponding "cd .." causes
following tests to execute at the incorrect location. Unfortunately,
you can't just place "cd .." at the end of a test since it will be
skipped if something before the "cd .." fails. The way the existing
code deals with this is by using a subshell:

mkdir repo &&
(
cd repo &&
...
)

The "cd repo" is inside the subshell, so it doesn't affect the parent
shell, and when the subshell exits, the parent shell remains at the
correct working directory (regardless of whether the test succeeded or
failed). Therefore, you don't want to drop the subshell.

> +   git init &&
> +   touch known 1 2 3 &&
> +   git add known &&
> +   git clean -f -e 1 -e 2 &&
> +   test_path_is_file 1 &&
> +   test_path_is_file 2 &&
> +   test_path_is_missing 3 &&
> +   test_path_is_file known
>  '
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] modernize t7300

[Eric Sunshine]

On Mon, Dec 7, 2015 at 4:40 PM, Junio C Hamano  wrote:
> James  writes:
>> @@ -46,15 +46,15 @@ test_expect_success 'git clean' '
>>   mkdir -p build docs &&
>>   touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>>   git clean &&
>> - test -f Makefile &&
>> - test -f README &&
>> - test -f src/part1.c &&
>> - test -f src/part2.c &&
>> - test ! -f a.out &&
>> - test ! -f src/part3.c &&
>> - test -f docs/manual.txt &&
>> - test -f obj.o &&
>> - test -f build/lib.so
>> + test_path_is_file Makefile &&
>> + test_path_is_file README &&
>> + test_path_is_file src/part1.c &&
>> + test_path_is_file src/part2.c &&
>> + test_path_is_missing a.out &&
>> + test_path_is_missing src/part3.c &&
>> + test_path_is_file docs/manual.txt &&
>> + test_path_is_file obj.o &&
>> + test_path_is_file build/lib.so
>
> The verbosity of this conversion makes me wonder if we want to have
> "test_paths_are_files" and "test_paths_are_missing".  For that
> matter, this test does not really care about the distinction between
> files and directories (e.g. some tests said "test ! -d docs" and
> would have passed if there were a 'docs' regular file, but what we
> really care about is the path 'docs' is _gone_), so what we want may
> be test_paths_exist and test_paths_are_missing.  With that, the
> above hunk would become
>
> test_paths_exist Makefile README src/part1.c src/part2.c \
> obj.o build/lib.so &&
> test_paths_are_missing a.out src/part3.c
>
> I dunno.

Alternately, update test_path_foo() functions to accept multiple
pathnames, or is that too ugly?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] modernize t7300

[Junio C Hamano]

Eric Sunshine  writes:

> Alternately, update test_path_foo() functions to accept multiple
> pathnames, or is that too ugly?

That actually would have been my first choice, except (1) that
path_is_missing has a cruft whose usefulness is dubious, and (2)
that "path_exists A B C" and "path_is_missing D E F" would be
gramatically incorrect.

I think we should first see if we can remove that "customized
message" that appears only in path_is_missing and remove it if
we can.  Extending "path_is_missing A B C" and friends to work
would then become trivial.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] modernize t7300

[James]

From: James Rouzier 

---
 t/t7300-clean.sh | 382 +++
 1 file changed, 190 insertions(+), 192 deletions(-)

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 86ceb38..d555bb6 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -28,15 +28,15 @@ test_expect_success 'git clean with skip-worktree 
.gitignore' '
mkdir -p build docs &&
touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
git clean &&
-   test -f Makefile &&
-   test -f README &&
-   test -f src/part1.c &&
-   test -f src/part2.c &&
-   test ! -f a.out &&
-   test ! -f src/part3.c &&
-   test -f docs/manual.txt &&
-   test -f obj.o &&
-   test -f build/lib.so &&
+   test_path_is_file Makefile &&
+   test_path_is_file README &&
+   test_path_is_file src/part1.c &&
+   test_path_is_file src/part2.c &&
+   test_path_is_missing a.out &&
+   test_path_is_missing src/part3.c &&
+   test_path_is_file docs/manual.txt &&
+   test_path_is_file obj.o &&
+   test_path_is_file build/lib.so &&
git update-index --no-skip-worktree .gitignore &&
git checkout .gitignore
 '
@@ -46,15 +46,15 @@ test_expect_success 'git clean' '
mkdir -p build docs &&
touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
git clean &&
-   test -f Makefile &&
-   test -f README &&
-   test -f src/part1.c &&
-   test -f src/part2.c &&
-   test ! -f a.out &&
-   test ! -f src/part3.c &&
-   test -f docs/manual.txt &&
-   test -f obj.o &&
-   test -f build/lib.so
+   test_path_is_file Makefile &&
+   test_path_is_file README &&
+   test_path_is_file src/part1.c &&
+   test_path_is_file src/part2.c &&
+   test_path_is_missing a.out &&
+   test_path_is_missing src/part3.c &&
+   test_path_is_file docs/manual.txt &&
+   test_path_is_file obj.o &&
+   test_path_is_file build/lib.so
 
 '
 
@@ -63,15 +63,15 @@ test_expect_success 'git clean src/' '
mkdir -p build docs &&
touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
git clean src/ &&
-   test -f Makefile &&
-   test -f README &&
-   test -f src/part1.c &&
-   test -f src/part2.c &&
-   test -f a.out &&
-   test ! -f src/part3.c &&
-   test -f docs/manual.txt &&
-   test -f obj.o &&
-   test -f build/lib.so
+   test_path_is_file Makefile &&
+   test_path_is_file README &&
+   test_path_is_file src/part1.c &&
+   test_path_is_file src/part2.c &&
+   test_path_is_file a.out &&
+   test_path_is_missing src/part3.c &&
+   test_path_is_file docs/manual.txt &&
+   test_path_is_file obj.o &&
+   test_path_is_file build/lib.so
 
 '
 
@@ -80,15 +80,15 @@ test_expect_success 'git clean src/ src/' '
mkdir -p build docs &&
touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
git clean src/ src/ &&
-   test -f Makefile &&
-   test -f README &&
-   test -f src/part1.c &&
-   test -f src/part2.c &&
-   test -f a.out &&
-   test ! -f src/part3.c &&
-   test -f docs/manual.txt &&
-   test -f obj.o &&
-   test -f build/lib.so
+   test_path_is_file Makefile &&
+   test_path_is_file README &&
+   test_path_is_file src/part1.c &&
+   test_path_is_file src/part2.c &&
+   test_path_is_file a.out &&
+   test_path_is_missing src/part3.c &&
+   test_path_is_file docs/manual.txt &&
+   test_path_is_file obj.o &&
+   test_path_is_file build/lib.so
 
 '
 
@@ -97,16 +97,16 @@ test_expect_success 'git clean with prefix' '
mkdir -p build docs src/test &&
touch a.out src/part3.c docs/manual.txt obj.o build/lib.so src/test/1.c 
&&
(cd src/ && git clean) &&
-   test -f Makefile &&
-   test -f README &&
-   test -f src/part1.c &&
-   test -f src/part2.c &&
-   test -f a.out &&
-   test ! -f src/part3.c &&
-   test -f src/test/1.c &&
-   test -f docs/manual.txt &&
-   test -f obj.o &&
-   test -f build/lib.so
+   test_path_is_file Makefile &&
+   test_path_is_file README &&
+   test_path_is_file src/part1.c &&
+   test_path_is_file src/part2.c &&
+   test_path_is_file a.out &&
+   test_path_is_missing src/part3.c &&
+   test_path_is_file src/test/1.c &&
+   test_path_is_file docs/manual.txt &&
+   test_path_is_file obj.o &&
+   test_path_is_file build/lib.so
 
 '
 
@@ -160,16 +160,16 @@ test_expect_success 'git clean -d with prefix and path' '
mkdir -p build docs src/feature &&
touch a.out src/part3.c src/feature/file.c docs/manual.txt obj.o 
build/lib.so &&
(cd src/ && git clean -d feature/) &&
-   test -f Makefile &&
-   test -f README &&
-   test -f src/part1.c &&
-   test -f src/part2.c &&
-   test -f a.out &&
-