{"thread":{"id":"59474","subject":"[PATCH] t5000: use check_mtime()","startedAt":"2023-03-25T12:16:42Z","lastAt":"2023-03-27T22:45:30Z","messageCount":4,"participants":["René Scharfe","Andrei Rybak","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"474168","messageId":"233edbbe-35c1-9b5b-7578-4c70c6d24449@web.de","threadId":"59474","inReplyTo":null,"subject":"[PATCH] t5000: use check_mtime()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-03-25T12:16:32Z","receivedAt":"2023-03-25T12:16:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"fd2da4b1ea (archive: add --mtime, 2023-02-18) added a helper function\nfor checking the file modification time of an extracted entry.  Use it\nfor the older mtime test as well to shorten the code and piggyback on\nthe archive extraction done to validate file contents.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/t5000-tar-tree.sh | 9 +--------\n 1 file changed, 1 insertion(+), 8 deletions(-)\n\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex 918a2fc7c6..f0bd70dbd6 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -185,6 +185,7 @@ test_expect_success 'git archive' '\n '\n\n check_tar b\n+check_mtime b a/a 1117231200\n\n test_expect_success 'git archive --mtime' '\n \tgit archive --mtime=2002-02-02T02:02:02-0200 HEAD >with_mtime.tar\n@@ -257,14 +258,6 @@ test_expect_success 'git archive --remote with configured remote' '\n \ttest_cmp_bin b.tar b5-nick.tar\n '\n\n-test_expect_success 'validate file modification time' '\n-\tmkdir extract &&\n-\t\"$TAR\" xf b.tar -C extract a/a &&\n-\ttest-tool chmtime --get extract/a/a >b.mtime &&\n-\techo \"1117231200\" >expected.mtime &&\n-\ttest_cmp expected.mtime b.mtime\n-'\n-\n test_expect_success 'git get-tar-commit-id' '\n \tgit get-tar-commit-id <b.tar >actual &&\n \tgit rev-parse HEAD >expect &&\n--\n2.40.0\n"},{"id":"474189","messageId":"5e53c8be-92b7-82e0-e204-a0cbfdffc529@gmail.com","threadId":"59474","inReplyTo":"233edbbe-35c1-9b5b-7578-4c70c6d24449@web.de","subject":"Re: [PATCH] t5000: use check_mtime()","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2023-03-26T12:53:06Z","receivedAt":"2023-03-26T12:53:13Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 25/03/2023 13:16, René Scharfe wrote:\n> fd2da4b1ea (archive: add --mtime, 2023-02-18) added a helper function\n> for checking the file modification time of an extracted entry.  Use it\n> for the older mtime test as well to shorten the code and piggyback on\n> the archive extraction done to validate file contents.\n> \n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>   t/t5000-tar-tree.sh | 9 +--------\n>   1 file changed, 1 insertion(+), 8 deletions(-)\n> \n> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\n> index 918a2fc7c6..f0bd70dbd6 100755\n> --- a/t/t5000-tar-tree.sh\n> +++ b/t/t5000-tar-tree.sh\n> @@ -185,6 +185,7 @@ test_expect_success 'git archive' '\n>   '\n> \n>   check_tar b\n> +check_mtime b a/a 1117231200\n> \n>   test_expect_success 'git archive --mtime' '\n>   \tgit archive --mtime=2002-02-02T02:02:02-0200 HEAD >with_mtime.tar\n> @@ -257,14 +258,6 @@ test_expect_success 'git archive --remote with configured remote' '\n>   \ttest_cmp_bin b.tar b5-nick.tar\n>   '\n> \n> -test_expect_success 'validate file modification time' '\n> -\tmkdir extract &&\n> -\t\"$TAR\" xf b.tar -C extract a/a &&\n> -\ttest-tool chmtime --get extract/a/a >b.mtime &&\n> -\techo \"1117231200\" >expected.mtime &&\n> -\ttest_cmp expected.mtime b.mtime\n> -'\n> -\n>   test_expect_success 'git get-tar-commit-id' '\n>   \tgit get-tar-commit-id <b.tar >actual &&\n>   \tgit rev-parse HEAD >expect &&\n> --\n> 2.40.0\n\nThis patch looks good to me.\n\nWhen reading check_mtime() I got confused by extra space in first parameter\nto test_expect_success, but after running t5000 and reading check_tar, it'd\nbecome obvious that the space is there to align together subtests related\nto one invocation of git-archive:\n\n     [...]\n     ok 7 - remove ignored file\n     ok 8 - git archive\n     ok 9 -  extract tar archive\n     ok 10 # skip  interpret pax headers (missing TAR_NEEDS_PAX_FALLBACK)\n     ok 11 -  validate filenames\n     ok 12 -  validate file contents\n     ok 13 -  validate mtime of a/a\n     ok 14 - git archive --mtime\n     ok 15 -  extract tar archive\n     ok 16 # skip  interpret pax headers (missing TAR_NEEDS_PAX_FALLBACK)\n     ok 17 -  validate filenames\n     ok 18 -  validate file contents\n     ok 19 -  validate mtime of a/a\n     ok 20 - git archive --prefix=prefix/\n     [...]\n\nThe only tangentially related nitpick is to the function check_mtime(),\nwhich doesn't follow the code style for Shell scripts -- a space is missing\nbefore parentheses.  Same for almost all the other helper functions in test\nfiles related to git-archive:\n\n     $ git grep -E '^[a-z_]+[(][)]' t/t500*\n     t/t5000-tar-tree.sh:get_pax_header() {\n     t/t5000-tar-tree.sh:check_tar() {\n     t/t5000-tar-tree.sh:check_added() {\n     t/t5000-tar-tree.sh:check_mtime() {\n     t/t5001-archive-attr.sh:test_expect_exists() {\n     t/t5001-archive-attr.sh:test_expect_missing() {\n     t/t5002-archive-attr-pattern.sh:test_expect_exists() {\n     t/t5002-archive-attr-pattern.sh:test_expect_missing() {\n     t/t5003-archive-zip.sh:check_zip() {\n     t/t5003-archive-zip.sh:check_added() {\n     t/t5004-archive-corner-cases.sh:make_dir() {\n     t/t5004-archive-corner-cases.sh:check_dir() {\n     t/t5004-archive-corner-cases.sh:build_tree() {\n\ncompare to:\n\n     $ git grep -E '^[a-z_]+ [(][)]' t/t500*\n     t/t5000-tar-tree.sh:tar_info () {\n     t/t5001-archive-attr.sh:extract_tar_to_dir () {\n\n\nQuote from Documentation/CodingGuidelines:\n\n     - We prefer a space between the function name and the parentheses,\n       and no space inside the parentheses. The opening \"{\" should also\n       be on the same line.\n    \n            (incorrect)\n            my_function(){\n                    ...\n    \n            (correct)\n            my_function () {\n                    ...\n\n"},{"id":"474251","messageId":"a47f8172-a8d0-abe8-09a8-f31b9e19b1a1@web.de","threadId":"59474","inReplyTo":"5e53c8be-92b7-82e0-e204-a0cbfdffc529@gmail.com","subject":"Re: [PATCH] t5000: use check_mtime()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-03-27T17:59:56Z","receivedAt":"2023-03-27T18:00:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 26.03.23 um 14:53 schrieb Andrei Rybak:\n>\n> The only tangentially related nitpick is to the function check_mtime(),\n> which doesn't follow the code style for Shell scripts -- a space is missing\n> before parentheses.  Same for almost all the other helper functions in test\n> files related to git-archive:\n>\n>     $ git grep -E '^[a-z_]+[(][)]' t/t500*\n>     t/t5000-tar-tree.sh:get_pax_header() {\n>     t/t5000-tar-tree.sh:check_tar() {\n>     t/t5000-tar-tree.sh:check_added() {\n>     t/t5000-tar-tree.sh:check_mtime() {\n>     t/t5001-archive-attr.sh:test_expect_exists() {\n>     t/t5001-archive-attr.sh:test_expect_missing() {\n>     t/t5002-archive-attr-pattern.sh:test_expect_exists() {\n>     t/t5002-archive-attr-pattern.sh:test_expect_missing() {\n>     t/t5003-archive-zip.sh:check_zip() {\n>     t/t5003-archive-zip.sh:check_added() {\n>     t/t5004-archive-corner-cases.sh:make_dir() {\n>     t/t5004-archive-corner-cases.sh:check_dir() {\n>     t/t5004-archive-corner-cases.sh:build_tree() {\n>\n> compare to:\n>\n>     $ git grep -E '^[a-z_]+ [(][)]' t/t500*\n>     t/t5000-tar-tree.sh:tar_info () {\n>     t/t5001-archive-attr.sh:extract_tar_to_dir () {\n>\n>\n> Quote from Documentation/CodingGuidelines:\n>\n>     - We prefer a space between the function name and the parentheses,\n>       and no space inside the parentheses. The opening \"{\" should also\n>       be on the same line.\n>               (incorrect)\n>            my_function(){\n>                    ...\n>               (correct)\n>            my_function () {\n>                    ...\n>\n\nCare to send a patch?\n\nThere's more by the way:\n\n   $ 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]}'\n   t/t0*.sh:16\n   t/t1*.sh:35\n   t/t2*.sh:3\n   t/te*.sh:2\n   t/t3*.sh:22\n   t/t4*.sh:23\n   t/t5*.sh:68\n   t/t6*.sh:11\n   t/t7*.sh:40\n   t/t8*.sh:1\n   t/t9*.sh:31\n   t/li*.sh:26\n\nPersonally I'd like to avoid wasting space and the second form looks odd\nto me, which turned me into a repeat offender out of negligence, I\nguess.  Anyway, since we have a documented rule (that came about by\nmajority vote of code lines IIUC) we should follow it.\n\nRené\n"},{"id":"474263","messageId":"xmqq5yaln795.fsf@gitster.g","threadId":"59474","inReplyTo":"a47f8172-a8d0-abe8-09a8-f31b9e19b1a1@web.de","subject":"Re: [PATCH] t5000: use check_mtime()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-27T22:45:26Z","receivedAt":"2023-03-27T22:45:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Care to send a patch?\n>\n> There's more by the way:\n> ...\n> Personally I'd like to avoid wasting space and the second form looks odd\n> to me, which turned me into a repeat offender out of negligence, I\n> guess.  Anyway, since we have a documented rule (that came about by\n> majority vote of code lines IIUC) we should follow it.\n\nI used to appreciate that I can grep for \"func (\" to find shell\nfunction definition while I can grep for \"func(\" for C, but that was\na long time ago.  \"git grep <pattern> --\" followed by \"\\*.c\" vs \"\\*.sh\"\nis what I use to limit the hits these days anyway ;-)\n"}]}