[PATCH] t5000: use check_mtime()

[PATCH] t5000: use check_mtime()

[René Scharfe]

fd2da4b1ea (archive: add --mtime, 2023-02-18) added a helper function
for checking the file modification time of an extracted entry.  Use it
for the older mtime test as well to shorten the code and piggyback on
the archive extraction done to validate file contents.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 t/t5000-tar-tree.sh | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 918a2fc7c6..f0bd70dbd6 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -185,6 +185,7 @@ test_expect_success 'git archive' '
 '

 check_tar b
+check_mtime b a/a 1117231200

 test_expect_success 'git archive --mtime' '
 	git archive --mtime=2002-02-02T02:02:02-0200 HEAD >with_mtime.tar
@@ -257,14 +258,6 @@ test_expect_success 'git archive --remote with configured remote' '
 	test_cmp_bin b.tar b5-nick.tar
 '

-test_expect_success 'validate file modification time' '
-	mkdir extract &&
-	"$TAR" xf b.tar -C extract a/a &&
-	test-tool chmtime --get extract/a/a >b.mtime &&
-	echo "1117231200" >expected.mtime &&
-	test_cmp expected.mtime b.mtime
-'
-
 test_expect_success 'git get-tar-commit-id' '
 	git get-tar-commit-id <b.tar >actual &&
 	git rev-parse HEAD >expect &&
--
2.40.0

Re: [PATCH] t5000: use check_mtime()

[Andrei Rybak]

On 25/03/2023 13:16, René Scharfe wrote:
> fd2da4b1ea (archive: add --mtime, 2023-02-18) added a helper function
> for checking the file modification time of an extracted entry.  Use it
> for the older mtime test as well to shorten the code and piggyback on
> the archive extraction done to validate file contents.
> 
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>   t/t5000-tar-tree.sh | 9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
> index 918a2fc7c6..f0bd70dbd6 100755
> --- a/t/t5000-tar-tree.sh
> +++ b/t/t5000-tar-tree.sh
> @@ -185,6 +185,7 @@ test_expect_success 'git archive' '
>   '
> 
>   check_tar b
> +check_mtime b a/a 1117231200
> 
>   test_expect_success 'git archive --mtime' '
>   	git archive --mtime=2002-02-02T02:02:02-0200 HEAD >with_mtime.tar
> @@ -257,14 +258,6 @@ test_expect_success 'git archive --remote with configured remote' '
>   	test_cmp_bin b.tar b5-nick.tar
>   '
> 
> -test_expect_success 'validate file modification time' '
> -	mkdir extract &&
> -	"$TAR" xf b.tar -C extract a/a &&
> -	test-tool chmtime --get extract/a/a >b.mtime &&
> -	echo "1117231200" >expected.mtime &&
> -	test_cmp expected.mtime b.mtime
> -'
> -
>   test_expect_success 'git get-tar-commit-id' '
>   	git get-tar-commit-id <b.tar >actual &&
>   	git rev-parse HEAD >expect &&
> --
> 2.40.0

This patch looks good to me.

When reading check_mtime() I got confused by extra space in first parameter
to test_expect_success, but after running t5000 and reading check_tar, it'd
become obvious that the space is there to align together subtests related
to one invocation of git-archive:

     [...]
     ok 7 - remove ignored file
     ok 8 - git archive
     ok 9 -  extract tar archive
     ok 10 # skip  interpret pax headers (missing TAR_NEEDS_PAX_FALLBACK)
     ok 11 -  validate filenames
     ok 12 -  validate file contents
     ok 13 -  validate mtime of a/a
     ok 14 - git archive --mtime
     ok 15 -  extract tar archive
     ok 16 # skip  interpret pax headers (missing TAR_NEEDS_PAX_FALLBACK)
     ok 17 -  validate filenames
     ok 18 -  validate file contents
     ok 19 -  validate mtime of a/a
     ok 20 - git archive --prefix=prefix/
     [...]

The only tangentially related nitpick is to the function check_mtime(),
which doesn't follow the code style for Shell scripts -- a space is missing
before parentheses.  Same for almost all the other helper functions in test
files related to git-archive:

     $ git grep -E '^[a-z_]+[(][)]' t/t500*
     t/t5000-tar-tree.sh:get_pax_header() {
     t/t5000-tar-tree.sh:check_tar() {
     t/t5000-tar-tree.sh:check_added() {
     t/t5000-tar-tree.sh:check_mtime() {
     t/t5001-archive-attr.sh:test_expect_exists() {
     t/t5001-archive-attr.sh:test_expect_missing() {
     t/t5002-archive-attr-pattern.sh:test_expect_exists() {
     t/t5002-archive-attr-pattern.sh:test_expect_missing() {
     t/t5003-archive-zip.sh:check_zip() {
     t/t5003-archive-zip.sh:check_added() {
     t/t5004-archive-corner-cases.sh:make_dir() {
     t/t5004-archive-corner-cases.sh:check_dir() {
     t/t5004-archive-corner-cases.sh:build_tree() {

compare to:

     $ git grep -E '^[a-z_]+ [(][)]' t/t500*
     t/t5000-tar-tree.sh:tar_info () {
     t/t5001-archive-attr.sh:extract_tar_to_dir () {


Quote from Documentation/CodingGuidelines:

     - We prefer a space between the function name and the parentheses,
       and no space inside the parentheses. The opening "{" should also
       be on the same line.
    
            (incorrect)
            my_function(){
                    ...
    
            (correct)
            my_function () {
                    ...

Re: [PATCH] t5000: use check_mtime()

[René Scharfe]

Am 26.03.23 um 14:53 schrieb Andrei Rybak:
>
> The only tangentially related nitpick is to the function check_mtime(),
> which doesn't follow the code style for Shell scripts -- a space is missing
> before parentheses.  Same for almost all the other helper functions in test
> files related to git-archive:
>
>     $ git grep -E '^[a-z_]+[(][)]' t/t500*
>     t/t5000-tar-tree.sh:get_pax_header() {
>     t/t5000-tar-tree.sh:check_tar() {
>     t/t5000-tar-tree.sh:check_added() {
>     t/t5000-tar-tree.sh:check_mtime() {
>     t/t5001-archive-attr.sh:test_expect_exists() {
>     t/t5001-archive-attr.sh:test_expect_missing() {
>     t/t5002-archive-attr-pattern.sh:test_expect_exists() {
>     t/t5002-archive-attr-pattern.sh:test_expect_missing() {
>     t/t5003-archive-zip.sh:check_zip() {
>     t/t5003-archive-zip.sh:check_added() {
>     t/t5004-archive-corner-cases.sh:make_dir() {
>     t/t5004-archive-corner-cases.sh:check_dir() {
>     t/t5004-archive-corner-cases.sh:build_tree() {
>
> compare to:
>
>     $ git grep -E '^[a-z_]+ [(][)]' t/t500*
>     t/t5000-tar-tree.sh:tar_info () {
>     t/t5001-archive-attr.sh:extract_tar_to_dir () {
>
>
> Quote from Documentation/CodingGuidelines:
>
>     - We prefer a space between the function name and the parentheses,
>       and no space inside the parentheses. The opening "{" should also
>       be on the same line.
>               (incorrect)
>            my_function(){
>                    ...
>               (correct)
>            my_function () {
>                    ...
>

Care to send a patch?

There's more by the way:

   $ git grep -c '^[a-zA-Z0-9_]*()' t/*.sh | awk -v FS=: '{c[substr($1,1,4)]+=$2} END {for (x in c) print x "*.sh:" c[x]}'
   t/t0*.sh:16
   t/t1*.sh:35
   t/t2*.sh:3
   t/te*.sh:2
   t/t3*.sh:22
   t/t4*.sh:23
   t/t5*.sh:68
   t/t6*.sh:11
   t/t7*.sh:40
   t/t8*.sh:1
   t/t9*.sh:31
   t/li*.sh:26

Personally I'd like to avoid wasting space and the second form looks odd
to me, which turned me into a repeat offender out of negligence, I
guess.  Anyway, since we have a documented rule (that came about by
majority vote of code lines IIUC) we should follow it.

René

Re: [PATCH] t5000: use check_mtime()

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Care to send a patch?
>
> There's more by the way:
> ...
> Personally I'd like to avoid wasting space and the second form looks odd
> to me, which turned me into a repeat offender out of negligence, I
> guess.  Anyway, since we have a documented rule (that came about by
> majority vote of code lines IIUC) we should follow it.

I used to appreciate that I can grep for "func (" to find shell
function definition while I can grep for "func(" for C, but that was
a long time ago.  "git grep <pattern> --" followed by "\*.c" vs "\*.sh"
is what I use to limit the hits these days anyway ;-)