| Message ID | 233edbbe-35c1-9b5b-7578-4c70c6d24449@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t5000: use check_mtime() | expand |
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 () { ...
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é
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 ;-)
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 &&
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(-) -- 2.40.0