From: Andrei Rybak <rybak.a.v@gmail.com>
Date: Sun, 26 Mar 2023 12:53:06 GMT
Subject: Re: [PATCH] t5000: use check_mtime()
Message-ID: <5e53c8be-92b7-82e0-e204-a0cbfdffc529@gmail.com>
In-Reply-To: <233edbbe-35c1-9b5b-7578-4c70c6d24449@web.de>

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 () {
                    ...