{"thread":{"id":"53032","subject":"[PATCH 0/6] fix test failure with busybox","startedAt":"2020-03-19T14:00:43Z","lastAt":"2020-03-22T06:05:23Z","messageCount":35,"participants":["Đoàn Trần Công Danh","Jeff King","Eric Sunshine","Junio C Hamano","Danh Doan"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"393449","messageId":"cover.1584625896.git.congdanhqx@gmail.com","threadId":"53032","inReplyTo":null,"subject":"[PATCH 0/6] fix test failure with busybox","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-03-19T14:00:01Z","receivedAt":"2020-03-19T14:00:43Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"Despite some non-compiance from busybox's sh(1), grep(1), diff(1),\nAlpine Linux is still a popular choice for container these days.\n\nFix false-positive failure in testsuite when run in Alpine Linux.\n\nt5703.{4,5,6,7} is still failing.\nDespite git pack-objects behaves correctly,\nupload-pack.c works incorrectly on this platform.\n\nI haven't dig deeper, yet.\n\nĐoàn Trần Công Danh (6):\n t4061: use POSIX compliance regex(7)\n test-lib-functions: test_cmp: eval $GIT_TEST_CMP\n t5003: skip conversion test if unzip -a is unavailable\n t5616: use rev-parse instead to get HEAD's object_id\n t7063: use POSIX find(1) syntax\n t4124: fix test for non-compliance diff\n\n t/t4061-diff-indent.sh | 2 +-\n t/t4124-apply-ws-rule.sh | 6 ++++++\n t/t5003-archive-zip.sh | 20 ++++++++++++++------\n t/t5616-partial-clone.sh | 2 +-\n t/t7063-status-untracked-cache.sh | 2 +-\n t/test-lib-functions.sh | 2 +-\n 6 files changed, 24 insertions(+), 10 deletions(-)\n\n-- \n2.26.0.rc2.234.g969ad452ca\n\n"},{"id":"393450","messageId":"c45d6383173d8d3e73cdcdd6e993d3259d519a68.1584625896.git.congdanhqx@gmail.com","threadId":"53032","inReplyTo":"cover.1584625896.git.congdanhqx@gmail.com","subject":"[PATCH 1/6] t4061: use POSIX compliance regex(7)","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-03-19T14:00:02Z","receivedAt":"2020-03-19T14:00:47Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"BRE interprets `+` literally, and\n`\\+` is undefined for POSIX BRE, from:\nhttps://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap09.html#tag_09_03_02\n\n> The interpretation of an ordinary character preceded\n> by an unescaped ( '\\\\' ) is undefined, except for:\n> - The characters ')', '(', '{', and '}'\n> - The digits 1 to 9 inclusive\n> - A character inside a bracket expression\n\nThis test is failing with busybox sed, the default sed of Alpine Linux\n\nFix it by using literal `+` instead.\n\nSigned-off-by: Đoàn Trần Công Danh \n---\n t/t4061-diff-indent.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh\nindex 2affd7a100..0f7a6d97a8 100755\n--- a/t/t4061-diff-indent.sh\n+++ b/t/t4061-diff-indent.sh\n@@ -17,7 +17,7 @@ compare_diff () {\n # Compare blame output using the expectation for a diff as reference.\n # Only look for the lines coming from non-boundary commits.\n compare_blame () {\n-\tsed -n -e \"1,4d\" -e \"s/^\\+//p\" <\"$1\" >.tmp-1\n+\tsed -n -e \"1,4d\" -e \"s/^+//p\" <\"$1\" >.tmp-1\n \tsed -ne \"s/^[^^][^)]*) *//p\" <\"$2\" >.tmp-2\n \ttest_cmp .tmp-1 .tmp-2 && rm -f .tmp-1 .tmp-2\n }\n-- \n2.26.0.rc2.234.g969ad452ca\n\n"},{"id":"393451","messageId":"7e184d97df8c673b0edfb6223c82385579777b19.1584625896.git.congdanhqx@gmail.com","threadId":"53032","inReplyTo":"cover.1584625896.git.congdanhqx@gmail.com","subject":"[PATCH 2/6] test-lib-functions: test_cmp: eval $GIT_TEST_CMP","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-03-19T14:00:03Z","receivedAt":"2020-03-19T14:00:50Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"Shell recognises first non-assignment token as command name.\nThus, ` cd t/perf; ./p0000-perf-lib-sanity.sh -d -i -v` reports:\n\n> test_cmp:1: command not found: diff -u\n\nUsing `eval` to unquote $GIT_TEST_CMP as same as precedence in `git_editor`.\n\nSigned-off-by: Đoàn Trần Công Danh \n---\n t/test-lib-functions.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 352c213d52..ab0e47ae17 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -905,7 +905,7 @@ test_expect_code () {\n # - not all diff versions understand \"-u\"\n \n test_cmp() {\n-\t$GIT_TEST_CMP \"$@\"\n+\teval \"$GIT_TEST_CMP\" '\"$@\"'\n }\n \n # Check that the given config key has the expected value.\n-- \n2.26.0.rc2.234.g969ad452ca\n\n"},{"id":"393452","messageId":"27c7813c0fe81455b675611b9e11fee6ee2a79e9.1584625896.git.congdanhqx@gmail.com","threadId":"53032","inReplyTo":"cover.1584625896.git.congdanhqx@gmail.com","subject":"[PATCH 3/6] t5003: skip conversion test if unzip -a is unavailable","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-03-19T14:00:04Z","receivedAt":"2020-03-19T14:00:53Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"Alpine Linux's default unzip(1) doesn't support `-a`.\n\nSkip those tests on that platform.\n\nSigned-off-by: Đoàn Trần Công Danh \n---\n t/t5003-archive-zip.sh | 20 ++++++++++++++------\n 1 file changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\nindex 106eddbd85..78fb4bf323 100755\n--- a/t/t5003-archive-zip.sh\n+++ b/t/t5003-archive-zip.sh\n@@ -15,6 +15,14 @@ test_lazy_prereq UNZIP_SYMLINKS '\n \t)\n '\n \n+test_lazy_prereq UNZIP_CONVERT '\n+\t(\n+\t\tmkdir unzip-convert &&\n+\t\tcd unzip-convert &&\n+\t\t\"$GIT_UNZIP\" -a \"$TEST_DIRECTORY\"/t5003/infozip-symlinks.zip\n+\t)\n+'\n+\n check_zip() {\n \tzipfile=$1.zip\n \tlistfile=$1.lst\n@@ -39,33 +47,33 @@ check_zip() {\n \textracted=${dir_with_prefix}a\n \toriginal=a\n \n-\ttest_expect_success UNZIP \" extract ZIP archive with EOL conversion\" '\n+\ttest_expect_success UNZIP_CONVERT \" extract ZIP archive with EOL conversion\" '\n \t\t(mkdir $dir && cd $dir && \"$GIT_UNZIP\" -a ../$zipfile)\n \t'\n \n-\ttest_expect_success UNZIP \" validate that text files are converted\" \"\n+\ttest_expect_success UNZIP_CONVERT \" validate that text files are converted\" \"\n \t\ttest_cmp_bin $extracted/text.cr $extracted/text.crlf &&\n \t\ttest_cmp_bin $extracted/text.cr $extracted/text.lf\n \t\"\n \n-\ttest_expect_success UNZIP \" validate that binary files are unchanged\" \"\n+\ttest_expect_success UNZIP_CONVERT \" validate that binary files are unchanged\" \"\n \t\ttest_cmp_bin $original/binary.cr $extracted/binary.cr &&\n \t\ttest_cmp_bin $original/binary.crlf $extracted/binary.crlf &&\n \t\ttest_cmp_bin $original/binary.lf $extracted/binary.lf\n \t\"\n \n-\ttest_expect_success UNZIP \" validate that diff files are converted\" \"\n+\ttest_expect_success UNZIP_CONVERT \" validate that diff files are converted\" \"\n \t\ttest_cmp_bin $extracted/diff.cr $extracted/diff.crlf &&\n \t\ttest_cmp_bin $extracted/diff.cr $extracted/diff.lf\n \t\"\n \n-\ttest_expect_success UNZIP \" validate that -diff files are unchanged\" \"\n+\ttest_expect_success UNZIP_CONVERT \" validate that -diff files are unchanged\" \"\n \t\ttest_cmp_bin $original/nodiff.cr $extracted/nodiff.cr &&\n \t\ttest_cmp_bin $original/nodiff.crlf $extracted/nodiff.crlf &&\n \t\ttest_cmp_bin $original/nodiff.lf $extracted/nodiff.lf\n \t\"\n \n-\ttest_expect_success UNZIP \" validate that custom diff is unchanged \" \"\n+\ttest_expect_success UNZIP_CONVERT \" validate that custom diff is unchanged \" \"\n \t\ttest_cmp_bin $original/custom.cr $extracted/custom.cr &&\n \t\ttest_cmp_bin $original/custom.crlf $extracted/custom.crlf &&\n \t\ttest_cmp_bin $original/custom.lf $extracted/custom.lf\n-- \n2.26.0.rc2.234.g969ad452ca\n\n"},{"id":"393453","messageId":"b9beadcc510301ba385f1b01b653024c8f369b49.1584625896.git.congdanhqx@gmail.com","threadId":"53032","inReplyTo":"cover.1584625896.git.congdanhqx@gmail.com","subject":"[PATCH 4/6] t5616: use rev-parse instead to get HEAD's object_id","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-03-19T14:00:05Z","receivedAt":"2020-03-19T14:00:54Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"Only HEAD's object_id is necessary, rev-list is an overkill.\n\nDespite POSIX requires grep(1) treat single pattern with \nas multiple patterns.\nbusybox's grep(1) (as of v1.31.1) haven't implemented it yet.\n\nUse rev-parse to simplify the test and avoid busybox unimplemented\nfeatures.\n\nSigned-off-by: Đoàn Trần Công Danh \n---\n t/t5616-partial-clone.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 77bb91e976..135187c5b5 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -49,7 +49,7 @@ test_expect_success 'do partial clone 1' '\n test_expect_success 'verify that .promisor file contains refs fetched' '\n \tls pc1/.git/objects/pack/pack-*.promisor >promisorlist &&\n \ttest_line_count = 1 promisorlist &&\n-\tgit -C srv.bare rev-list HEAD >headhash &&\n+\tgit -C srv.bare rev-parse HEAD >headhash &&\n \tgrep \"$(cat headhash) HEAD\" $(cat promisorlist) &&\n \tgrep \"$(cat headhash) refs/heads/master\" $(cat promisorlist)\n '\n-- \n2.26.0.rc2.234.g969ad452ca\n\n"},{"id":"393454","messageId":"215801b02aceeed1e0f6313679c567a914ad5dd8.1584625896.git.congdanhqx@gmail.com","threadId":"53032","inReplyTo":"cover.1584625896.git.congdanhqx@gmail.com","subject":"[PATCH 5/6] t7063: use POSIX find(1) syntax","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-03-19T14:00:06Z","receivedAt":"2020-03-19T14:00:57Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"Since commit 6b7728db81, (t7063: work around FreeBSD's lazy mtime\nupdate feature, 2016-08-03), we started to use ls as a trick to update\ndirectory's mtime.\n\nHowever, `-ls` flag isn't required by POSIX's find(1), and\nbusybox(1) doesn't implement it.\n\nUse an equivalence `-exec ls -dils {} +` instead.\n\nSigned-off-by: Đoàn Trần Công Danh \n---\n t/t7063-status-untracked-cache.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex 190ae149cf..c2731d445a 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -18,7 +18,7 @@ GIT_FORCE_UNTRACKED_CACHE=true\n export GIT_FORCE_UNTRACKED_CACHE\n \n sync_mtime () {\n-\tfind . -type d -ls >/dev/null\n+\tfind . -type d -exec ls -dils {} + >/dev/null\n }\n \n avoid_racy() {\n-- \n2.26.0.rc2.234.g969ad452ca\n\n"},{"id":"393455","messageId":"285c6830c5182cb602d4fe559525083f69a158e9.1584625896.git.congdanhqx@gmail.com","threadId":"53032","inReplyTo":"cover.1584625896.git.congdanhqx@gmail.com","subject":"[PATCH 6/6] t4124: fix test for non-compliance diff","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-03-19T14:00:07Z","receivedAt":"2020-03-19T14:00:59Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"POSIX's diff(1) requires output in normal diff format.\nHowever, busybox's diff's output is written in unified format.\n\nPOSIX requires no option for normal-diff format.\n\nA hint in test-lib-functions::test_cmp said `diff -u` isn't available\neverywhere.\n\nWorkaround this problem by assuming `diff(1)` output is unified\nif we couldn't make anything from normal-diff format.\n\nSigned-off-by: Đoàn Trần Công Danh \n---\n t/t4124-apply-ws-rule.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\nindex 971a5a7512..2a54ce96b5 100755\n--- a/t/t4124-apply-ws-rule.sh\n+++ b/t/t4124-apply-ws-rule.sh\n@@ -52,6 +52,12 @@ test_fix () {\n \n \t# find touched lines\n \t$DIFF file target | sed -n -e \"s/^> //p\" >fixed\n+\tif ! test -s fixed; then\n+\t\t$DIFF file target |\n+\t\tgrep '^+' |\n+\t\tgrep -v '^+++' |\n+\t\tsed -e \"s/+//\" >fixed\n+\tfi\n \n \t# the changed lines are all expected to change\n \tfixed_cnt=$(wc -l Despite some non-compiance from busybox's sh(1), grep(1), diff(1),\n> Alpine Linux is still a popular choice for container these days.\n> \n> Fix false-positive failure in testsuite when run in Alpine Linux.\n> \n> t5703.{4,5,6,7} is still failing.\n> Despite git pack-objects behaves correctly,\n> upload-pack.c works incorrectly on this platform.\n> \n> I haven't dig deeper, yet.\n\nI was able to reproduce the problems on Debian (with busybox installed)\nwith:\n\n mkdir /tmp/bb\n (cd /tmp/bb\n bb=$(which busybox)\n for i in $($bb --list); do\n ln -s $bb $i\n done)\n PATH=/tmp/bb:$PATH make test TEST_SHELL_PATH=/tmp/bb/sh\n\nThe issue in t5703 is the sed call in get_actual_commits(). It's trying\nto cut off the early (text) part of the file, and pass through the\nbinary goo of the packfile. Presumably busybox's sed isn't binary-clean.\n\nOur usual strategy here would be to switch to perl, which is more\npredictable about binary bytes.\n\nWe're also feeding this into a test-tool helper. It's possible that\nhelper could be made smart enough to replace both this sed invocation\nand the one in get_actual_refs().\n\n-Peff\n"},{"id":"393459","messageId":"20200319155307.GB3513282@coredump.intra.peff.net","threadId":"53032","inReplyTo":"c45d6383173d8d3e73cdcdd6e993d3259d519a68.1584625896.git.congdanhqx@gmail.com","subject":"Re: [PATCH 1/6] t4061: use POSIX compliance regex(7)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-19T15:53:07Z","receivedAt":"2020-03-19T15:53:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2020 at 09:00:02PM +0700, Đoàn Trần Công Danh wrote:\n\n> BRE interprets `+` literally, and\n> `\\+` is undefined for POSIX BRE, from:\n> https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap09.html#tag_09_03_02\n> \n> > The interpretation of an ordinary character preceded\n> > by an unescaped ( '\\\\' ) is undefined, except for:\n> > - The characters ')', '(', '{', and '}'\n> > - The digits 1 to 9 inclusive\n> > - A character inside a bracket expression\n> \n> This test is failing with busybox sed, the default sed of Alpine Linux\n> \n> Fix it by using literal `+` instead.\n\nThis makes sense, I think. It could hurt a sed which is expected ERE and\nneeds the \"+\" escaped, but I think such a sed would be wrong (and I\nimagine would break things elsewhere).\n\n-Peff\n"},{"id":"393460","messageId":"CAPig+cRzHtGu2KfjoO4DaPWzDrJOhEQGkgo2Fm7egUPM9m7Vyw@mail.gmail.com","threadId":"53032","inReplyTo":"20200319155307.GB3513282@coredump.intra.peff.net","subject":"Re: [PATCH 1/6] t4061: use POSIX compliance regex(7)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-03-19T16:01:28Z","receivedAt":"2020-03-19T16:01:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 19, 2020 at 11:53 AM Jeff King wrote:\n> On Thu, Mar 19, 2020 at 09:00:02PM +0700, Đoàn Trần Công Danh wrote:\n> > Fix it by using literal `+` instead.\n>\n> This makes sense, I think. It could hurt a sed which is expected ERE and\n> needs the \"+\" escaped, but I think such a sed would be wrong (and I\n> imagine would break things elsewhere).\n\nI had the same thought and considered suggesting a character class:\n\n sed -n -e \"1,4d\" -e \"s/^[+]//p\" <\"$1\" >.tmp-1\n\nto make it painfully obvious that \"+\" is not special in the\nexpression. But then I thought better of it -- for the same reason as\nyou (to wit: such a 'sed' would be wrong) -- and decided against\nsaying anything.\n"},{"id":"393461","messageId":"20200319160211.GC3513282@coredump.intra.peff.net","threadId":"53032","inReplyTo":"7e184d97df8c673b0edfb6223c82385579777b19.1584625896.git.congdanhqx@gmail.com","subject":"Re: [PATCH 2/6] test-lib-functions: test_cmp: eval $GIT_TEST_CMP","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-19T16:02:11Z","receivedAt":"2020-03-19T16:02:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2020 at 09:00:03PM +0700, Đoàn Trần Công Danh wrote:\n\n> Shell recognises first non-assignment token as command name.\n> Thus, ` cd t/perf; ./p0000-perf-lib-sanity.sh -d -i -v` reports:\n> \n> > test_cmp:1: command not found: diff -u\n> \n> Using `eval` to unquote $GIT_TEST_CMP as same as precedence in `git_editor`.\n\nAdding an \"eval\" here will subtly change the behavior for other shells.\nRight now they'd just do whitespace splitting (which presumably busybox\nis not), but with this we'd expand metachars, etc.\n\nI suspect that's fine (and maybe even preferable, because if you really\ndo have a space in the path you can actually escape it). So I don't mind\nthis change.\n\nI do worry that this whitespace splitting behavior could bite us in\nother scripts. Curiously, my version of busybox (1.30.1) doesn't seem to\nhave any problem with it, though.\n\n-Peff\n"},{"id":"393462","messageId":"20200319160310.GD3513282@coredump.intra.peff.net","threadId":"53032","inReplyTo":"27c7813c0fe81455b675611b9e11fee6ee2a79e9.1584625896.git.congdanhqx@gmail.com","subject":"Re: [PATCH 3/6] t5003: skip conversion test if unzip -a is unavailable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-19T16:03:10Z","receivedAt":"2020-03-19T16:03:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2020 at 09:00:04PM +0700, Đoàn Trần Công Danh wrote:\n\n> Alpine Linux's default unzip(1) doesn't support `-a`.\n> \n> Skip those tests on that platform.\n\nMakes sense. One minor nit:\n\n> +test_lazy_prereq UNZIP_CONVERT '\n> +\t(\n> +\t\tmkdir unzip-convert &&\n> +\t\tcd unzip-convert &&\n> +\t\t\"$GIT_UNZIP\" -a \"$TEST_DIRECTORY\"/t5003/infozip-symlinks.zip\n> +\t)\n> +'\n\nLazy prereqs are already evaluated in a throw-away directory, so you can\ndrop the subshell and mkdir/cd.\n\n-Peff\n"},{"id":"393464","messageId":"20200319160707.GE3513282@coredump.intra.peff.net","threadId":"53032","inReplyTo":"b9beadcc510301ba385f1b01b653024c8f369b49.1584625896.git.congdanhqx@gmail.com","subject":"Re: [PATCH 4/6] t5616: use rev-parse instead to get HEAD's object_id","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-19T16:07:07Z","receivedAt":"2020-03-19T16:07:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2020 at 09:00:05PM +0700, Đoàn Trần Công Danh wrote:\n\n> Only HEAD's object_id is necessary, rev-list is an overkill.\n> \n> Despite POSIX requires grep(1) treat single pattern with \n> as multiple patterns.\n> busybox's grep(1) (as of v1.31.1) haven't implemented it yet.\n> \n> Use rev-parse to simplify the test and avoid busybox unimplemented\n> features.\n\nThat makes sense. It would also future-proof us against the test\nchanging the graph such that HEAD actually has ancestors.\n\n> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n> index 77bb91e976..135187c5b5 100755\n> --- a/t/t5616-partial-clone.sh\n> +++ b/t/t5616-partial-clone.sh\n> @@ -49,7 +49,7 @@ test_expect_success 'do partial clone 1' '\n> test_expect_success 'verify that .promisor file contains refs fetched' '\n> \tls pc1/.git/objects/pack/pack-*.promisor >promisorlist &&\n> \ttest_line_count = 1 promisorlist &&\n> -\tgit -C srv.bare rev-list HEAD >headhash &&\n> +\tgit -C srv.bare rev-parse HEAD >headhash &&\n\nMaybe worth using \"rev-parse --verify\" which would double check that we\nproduced a useful hash (it seems like an unlikely failure mode, but it's\neasy enough to cover).\n\n-Peff\n"},{"id":"393465","messageId":"20200319161200.GF3513282@coredump.intra.peff.net","threadId":"53032","inReplyTo":"215801b02aceeed1e0f6313679c567a914ad5dd8.1584625896.git.congdanhqx@gmail.com","subject":"Re: [PATCH 5/6] t7063: use POSIX find(1) syntax","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-19T16:12:00Z","receivedAt":"2020-03-19T16:12:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2020 at 09:00:06PM +0700, Đoàn Trần Công Danh wrote:\n\n> Since commit 6b7728db81, (t7063: work around FreeBSD's lazy mtime\n> update feature, 2016-08-03), we started to use ls as a trick to update\n> directory's mtime.\n> \n> However, `-ls` flag isn't required by POSIX's find(1), and\n> busybox(1) doesn't implement it.\n> \n> Use an equivalence `-exec ls -dils {} +` instead.\n\nMakes sense. I wonder if we need all of \"-dils\", but it's not clear to\nme which syscalls actually trigger the FreeBSD lazy-update behavior. I\nguess probably it's stat()ing the directory, so \"ls -ld\" would be\nsufficient (and that's implied by the examples in 6b7728db81).\n\nBut I doubt the extra options would create a portability problem, so I\nthink it's fine either way.\n\n-Peff\n"},{"id":"393466","messageId":"CAPig+cSZvhb4H54QZ=m+A+WF5vg6h014cn5JxiB5N4Om-m2tbw@mail.gmail.com","threadId":"53032","inReplyTo":"20200319160211.GC3513282@coredump.intra.peff.net","subject":"Re: [PATCH 2/6] test-lib-functions: test_cmp: eval $GIT_TEST_CMP","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-03-19T16:14:09Z","receivedAt":"2020-03-19T16:14:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 19, 2020 at 12:02 PM Jeff King wrote:\n> On Thu, Mar 19, 2020 at 09:00:03PM +0700, Đoàn Trần Công Danh wrote:\n> > > test_cmp:1: command not found: diff -u\n> >\n> > Using `eval` to unquote $GIT_TEST_CMP as same as precedence in `git_editor`.\n>\n> I do worry that this whitespace splitting behavior could bite us in\n> other scripts. Curiously, my version of busybox (1.30.1) doesn't seem to\n> have any problem with it, though.\n\nI had the same reaction upon reading the patch. It's not just other\nscripts in which the problem might manifest, but a change to the value\nof some variable which is used as a command invocation. Providing a\n\"fix\" for this one particular case may help get past test a failure on\nAlpine under busybox, but it is not a good general solution. (Some\nsort of helper function which smooths away differences like this --\nsay git_indirect_cmd() or something -- which can be used wherever a\n$VARIABLE is invoked as a command might be a better approach, but I\nhaven't really thought it through.)\n"},{"id":"393468","messageId":"20200319163334.GG3513282@coredump.intra.peff.net","threadId":"53032","inReplyTo":"285c6830c5182cb602d4fe559525083f69a158e9.1584625896.git.congdanhqx@gmail.com","subject":"Re: [PATCH 6/6] t4124: fix test for non-compliance diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-19T16:33:34Z","receivedAt":"2020-03-19T16:33:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2020 at 09:00:07PM +0700, Đoàn Trần Công Danh wrote:\n\n> POSIX's diff(1) requires output in normal diff format.\n> However, busybox's diff's output is written in unified format.\n\nThat's a pretty big difference. I'm surprised this only produces one\nproblem in the test scripts. ;)\n\n> POSIX requires no option for normal-diff format.\n> \n> A hint in test-lib-functions::test_cmp said `diff -u` isn't available\n> everywhere.\n> \n> Workaround this problem by assuming `diff(1)` output is unified\n> if we couldn't make anything from normal-diff format.\n\nI wonder if we could use \"git diff\" here. We have to be careful about\ncircular reasoning in our tests (i.e., making sure we're not verifying\noutput with the same code that we're testing), but I think here we're\nchecking how \"apply --whitespace=fix\" works.\n\nBut if this is the only spot, then adjusting to handle unified or normal\ndiff isn't too bad.\n\n> diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\n> index 971a5a7512..2a54ce96b5 100755\n> --- a/t/t4124-apply-ws-rule.sh\n> +++ b/t/t4124-apply-ws-rule.sh\n> @@ -52,6 +52,12 @@ test_fix () {\n> \n> \t# find touched lines\n> \t$DIFF file target | sed -n -e \"s/^> //p\" >fixed\n> +\tif ! test -s fixed; then\n> +\t\t$DIFF file target |\n> +\t\tgrep '^+' |\n> +\t\tgrep -v '^+++' |\n> +\t\tsed -e \"s/+//\" >fixed\n> +\tfi\n\nI think those greps could be lumped into sed like:\n\n sed -ne \"s/^+[^+]//p\"\n\n(at the cost of missing blank lines, but I think that's OK for our\npurposes here; it could be fixed with an ERE).\n\nCould we then make a single invocation that covers both diff formats? We\ncan further observe that the only thing we do with the \"fixed\" file is\ncount the lines, so we can leave the markers. Which means we could ditch\nsed entirely and use grep. Something like:\n\ndiff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\nindex 971a5a7512..15cb0c81b7 100755\n--- a/t/t4124-apply-ws-rule.sh\n+++ b/t/t4124-apply-ws-rule.sh\n@@ -50,8 +50,9 @@ test_fix () {\n \t# fix should not barf\n \tapply_patch --whitespace=fix || return 1\n \n-\t# find touched lines\n-\t$DIFF file target | sed -n -e \"s/^> //p\" >fixed\n+\t# find touched lines; handle either normal or unified\n+\t# diff, as system diff may generate either\n+\t$DIFF file target | grep '^[>+][^+]' >fixed\n \n \t# the changed lines are all expected to change\n \tfixed_cnt=$(wc -l Despite some non-compiance from busybox's sh(1), grep(1), diff(1),\n> Alpine Linux is still a popular choice for container these days.\n> \n> Fix false-positive failure in testsuite when run in Alpine Linux.\n\nThanks, these all looked like sensible directions. I left a few small\ncomments that I think are worth addressing.\n\n-Peff\n"},{"id":"393491","messageId":"xmqqwo7gc8aq.fsf@gitster.c.googlers.com","threadId":"53032","inReplyTo":"CAPig+cRzHtGu2KfjoO4DaPWzDrJOhEQGkgo2Fm7egUPM9m7Vyw@mail.gmail.com","subject":"Re: [PATCH 1/6] t4061: use POSIX compliance regex(7)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-19T22:02:37Z","receivedAt":"2020-03-19T22:02:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Thu, Mar 19, 2020 at 11:53 AM Jeff King wrote:\n>> On Thu, Mar 19, 2020 at 09:00:02PM +0700, Đoàn Trần Công Danh wrote:\n>> > Fix it by using literal `+` instead.\n>>\n>> This makes sense, I think. It could hurt a sed which is expected ERE and\n>> needs the \"+\" escaped, but I think such a sed would be wrong (and I\n>> imagine would break things elsewhere).\n>\n> I had the same thought and considered suggesting a character class:\n>\n> sed -n -e \"1,4d\" -e \"s/^[+]//p\" <\"$1\" >.tmp-1\n>\n> to make it painfully obvious that \"+\" is not special in the\n> expression. But then I thought better of it -- for the same reason as\n> you (to wit: such a 'sed' would be wrong) -- and decided against\n> saying anything.\n\nI have only one thing that needs fixing, which is s/compliance/compliant/;\non the title. Other than that, it looks good.\n\nHaving said that, I would have done the [+] thing if I were doing\nthis patch myself. As long as we see no \"wrong\" sed that is broken\nby this change, I am OK with it, though.\n\nThanks.\n"},{"id":"393492","messageId":"xmqqsgi4c7o6.fsf@gitster.c.googlers.com","threadId":"53032","inReplyTo":"20200319161200.GF3513282@coredump.intra.peff.net","subject":"Re: [PATCH 5/6] t7063: use POSIX find(1) syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-19T22:16:09Z","receivedAt":"2020-03-19T22:16:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Mar 19, 2020 at 09:00:06PM +0700, Đoàn Trần Công Danh wrote:\n>\n>> Since commit 6b7728db81, (t7063: work around FreeBSD's lazy mtime\n>> update feature, 2016-08-03), we started to use ls as a trick to update\n>> directory's mtime.\n>> \n>> However, `-ls` flag isn't required by POSIX's find(1), and\n>> busybox(1) doesn't implement it.\n>> \n>> Use an equivalence `-exec ls -dils {} +` instead.\n>\n> Makes sense. I wonder if we need all of \"-dils\", but it's not clear to\n> me which syscalls actually trigger the FreeBSD lazy-update behavior. I\n> guess probably it's stat()ing the directory, so \"ls -ld\" would be\n> sufficient (and that's implied by the examples in 6b7728db81).\n>\n> But I doubt the extra options would create a portability problem, so I\n> think it's fine either way.\n\nThanks. I too wondered if -dils is really needed (POSIX of course\nhave all of them, but we have to deal with non-POSIX systems, too,\nand I am not sure how things like \"-i\" works there).\n\ns/equivalence/equivalent/; perhaps?\n"},{"id":"393493","messageId":"xmqqlfnwc5p0.fsf@gitster.c.googlers.com","threadId":"53032","inReplyTo":"20200319163334.GG3513282@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] t4124: fix test for non-compliance diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-19T22:58:51Z","receivedAt":"2020-03-19T22:59:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Mar 19, 2020 at 09:00:07PM +0700, Đoàn Trần Công Danh wrote:\n>\n>> POSIX's diff(1) requires output in normal diff format.\n>> However, busybox's diff's output is written in unified format.\n>\n> That's a pretty big difference. I'm surprised this only produces one\n> problem in the test scripts. ;)\n>\n>> POSIX requires no option for normal-diff format.\n>> \n>> A hint in test-lib-functions::test_cmp said `diff -u` isn't available\n>> everywhere.\n>> \n>> Workaround this problem by assuming `diff(1)` output is unified\n>> if we couldn't make anything from normal-diff format.\n\nI do not mind working it around, but I am a bit disturbed by an\nuneven attitude towards POSIX noncompliance this series has. If\nwe were willing to break other people's \"sed\" that does not do BRE\ncorrectly, instead of using '[+]' trick to accomodate them while\nmaking sure that an implementation that does not use nonstandard\nextension and does only BRE, we should just similarly be writing\nsuch an implementation of noncompliant diff off as broken, yet we\nbend backwards over to make sure we can work with them here. \n\nIOW, I do not have trouble changing the test so that it works with\nnoncompliant \"diff\". But then in the same series, I would prefer to\nsee the existing test keeps working with a possibly noncompliant\n\"sed\" implementation that has been working well with the tests.\n\n> But if this is the only spot, then adjusting to handle unified or normal\n> diff isn't too bad.\n\nYup.\n\n> Could we then make a single invocation that covers both diff formats? We\n> can further observe that the only thing we do with the \"fixed\" file is\n> count the lines, so we can leave the markers. Which means we could ditch\n> sed entirely and use grep. Something like:\n>\n> diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\n> index 971a5a7512..15cb0c81b7 100755\n> --- a/t/t4124-apply-ws-rule.sh\n> +++ b/t/t4124-apply-ws-rule.sh\n> @@ -50,8 +50,9 @@ test_fix () {\n> \t# fix should not barf\n> \tapply_patch --whitespace=fix || return 1\n> \n> -\t# find touched lines\n> -\t$DIFF file target | sed -n -e \"s/^> //p\" >fixed\n> +\t# find touched lines; handle either normal or unified\n> +\t# diff, as system diff may generate either\n> +\t$DIFF file target | grep '^[>+][^+]' >fixed\n> \n> \t# the changed lines are all expected to change\n> \tfixed_cnt=$(wc -l \n> seems to work for with both busybox diff and GNU diff.\n\nSounds OK to me.\n"},{"id":"393502","messageId":"20200320003715.GA1858@danh.dev","threadId":"53032","inReplyTo":"20200319155136.GA3513282@coredump.intra.peff.net","subject":"Re: [PATCH 0/6] fix test failure with busybox","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-03-20T00:37:15Z","receivedAt":"2020-03-20T00:37:22Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-03-19 11:51:36-0400, Jeff King wrote:\n> On Thu, Mar 19, 2020 at 09:00:01PM +0700, Đoàn Trần Công Danh wrote:\n> \n> > Despite some non-compiance from busybox's sh(1), grep(1), diff(1),\n> > Alpine Linux is still a popular choice for container these days.\n> > \n> > Fix false-positive failure in testsuite when run in Alpine Linux.\n> > \n> > t5703.{4,5,6,7} is still failing.\n> > Despite git pack-objects behaves correctly,\n> > upload-pack.c works incorrectly on this platform.\n> > \n> > I haven't dig deeper, yet.\n> \n> I was able to reproduce the problems on Debian (with busybox installed)\n> with:\n> \n> mkdir /tmp/bb\n> (cd /tmp/bb\n> bb=$(which busybox)\n> for i in $($bb --list); do\n> ln -s $bb $i\n> done)\n> PATH=/tmp/bb:$PATH make test TEST_SHELL_PATH=/tmp/bb/sh\n> \n> The issue in t5703 is the sed call in get_actual_commits(). It's trying\n> to cut off the early (text) part of the file, and pass through the\n> binary goo of the packfile. Presumably busybox's sed isn't binary-clean.\n\nI've checked, busybox's sed think every input is text file,\nand in POSIX, every line in every text file must be terminated by\n.\n\nThus, busybox's sed append a after `0000` marker, render the\npack file invalid.\n\n> Our usual strategy here would be to switch to perl, which is more\n> predictable about binary bytes.\n\nPerl works fine here.\n\n> We're also feeding this into a test-tool helper. It's possible that\n> helper could be made smart enough to replace both this sed invocation\n> and the one in get_actual_refs().\n\nI looked into this direction, I guess you meant something like this:\n\n-------------8<--------------\ndiff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c\nindex 282d536384..1d62143dbc 100644\n--- a/t/helper/test-pkt-line.c\n+++ b/t/helper/test-pkt-line.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"test-tool.h\"\n #include \"pkt-line.h\"\n+#include \"strbuf.h\"\n \n static void pack_line(const char *line)\n {\n@@ -53,6 +54,13 @@ static void unpack(void)\n static void unpack_sideband(void)\n {\n \tstruct packet_reader reader;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\twhile (strbuf_getline(&buf, stdin) == 0)\n+\t\tif (strstr(buf.buf, \"packfile\"))\n+\t\t break;\n+\tstrbuf_release(&buf);\n+\n \tpacket_reader_init(&reader, 0, NULL, 0,\n \t\t\t PACKET_READ_GENTLE_ON_EOF |\n \t\t\t PACKET_READ_CHOMP_NEWLINE);\ndiff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh\nindex 7fba3063bf..a34460f7d8 100755\n--- a/t/t5703-upload-pack-ref-in-want.sh\n+++ b/t/t5703-upload-pack-ref-in-want.sh\n@@ -13,10 +13,7 @@ get_actual_refs () {\n }\n \n get_actual_commits () {\n-\tsed -n -e '/packfile/,/0000/{\n-\t\t/packfile/d\n-\t\tp\n-\t\t}' o.pack &&\n+\ttest-tool pkt-line unpack-sideband o.pack &&\n \tgit index-pack o.pack &&\n \tgit verify-pack -v o.idx >objs &&\n \tgrep commit objs | cut -d\" \" -f1 | sort >actual_commits\n----------------------->8-------------------\n\nBut, this doesn't work. as `packet_reader_read` reads directly from fd 0.\n\nI think perl should be good for now.\n\n-- \nDanh\n"},{"id":"393503","messageId":"20200320003921.GB1858@danh.dev","threadId":"53032","inReplyTo":"20200319160310.GD3513282@coredump.intra.peff.net","subject":"Re: [PATCH 3/6] t5003: skip conversion test if unzip -a is unavailable","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-03-20T00:39:21Z","receivedAt":"2020-03-20T00:39:27Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-03-19 12:03:10-0400, Jeff King wrote:\n> On Thu, Mar 19, 2020 at 09:00:04PM +0700, Đoàn Trần Công Danh wrote:\n> \n> > Alpine Linux's default unzip(1) doesn't support `-a`.\n> > \n> > Skip those tests on that platform.\n> \n> Makes sense. One minor nit:\n> \n> > +test_lazy_prereq UNZIP_CONVERT '\n> > +\t(\n> > +\t\tmkdir unzip-convert &&\n> > +\t\tcd unzip-convert &&\n> > +\t\t\"$GIT_UNZIP\" -a \"$TEST_DIRECTORY\"/t5003/infozip-symlinks.zip\n> > +\t)\n> > +'\n> \n> Lazy prereqs are already evaluated in a throw-away directory, so you can\n> drop the subshell and mkdir/cd.\n\nI was trying to keep it consistent with UNZIP_SYMLINKS above it.\nMaybe, it's worth to clean it, too.\n\n-- \nDanh\n"},{"id":"393504","messageId":"20200320005751.GC1858@danh.dev","threadId":"53032","inReplyTo":"20200319160707.GE3513282@coredump.intra.peff.net","subject":"Re: [PATCH 4/6] t5616: use rev-parse instead to get HEAD's object_id","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-03-20T00:57:51Z","receivedAt":"2020-03-20T00:57:58Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-03-19 12:07:07-0400, Jeff King wrote:\n> On Thu, Mar 19, 2020 at 09:00:05PM +0700, Đoàn Trần Công Danh wrote:\n> \n> > Only HEAD's object_id is necessary, rev-list is an overkill.\n> > \n> > Despite POSIX requires grep(1) treat single pattern with \n> > as multiple patterns.\n> > busybox's grep(1) (as of v1.31.1) haven't implemented it yet.\n> > \n> > Use rev-parse to simplify the test and avoid busybox unimplemented\n> > features.\n> \n> That makes sense. It would also future-proof us against the test\n> changing the graph such that HEAD actually has ancestors.\n> \n> > diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n> > index 77bb91e976..135187c5b5 100755\n> > --- a/t/t5616-partial-clone.sh\n> > +++ b/t/t5616-partial-clone.sh\n> > @@ -49,7 +49,7 @@ test_expect_success 'do partial clone 1' '\n> > test_expect_success 'verify that .promisor file contains refs fetched' '\n> > \tls pc1/.git/objects/pack/pack-*.promisor >promisorlist &&\n> > \ttest_line_count = 1 promisorlist &&\n> > -\tgit -C srv.bare rev-list HEAD >headhash &&\n> > +\tgit -C srv.bare rev-parse HEAD >headhash &&\n> \n> Maybe worth using \"rev-parse --verify\" which would double check that we\n> produced a useful hash (it seems like an unlikely failure mode, but it's\n> easy enough to cover).\n\nYeah, I also thought it wasn't expected to be a failure in this case.\nUsing `--verify` doesn't cost anything in this case, though.\nI will add it in the reroll.\n\n-- \nDanh\n"},{"id":"393506","messageId":"20200320012956.GD1858@danh.dev","threadId":"53032","inReplyTo":"20200319160211.GC3513282@coredump.intra.peff.net","subject":"Re: [PATCH 2/6] test-lib-functions: test_cmp: eval $GIT_TEST_CMP","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-03-20T01:29:56Z","receivedAt":"2020-03-20T01:30:02Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-03-19 12:02:11-0400, Jeff King wrote:\n> On Thu, Mar 19, 2020 at 09:00:03PM +0700, Đoàn Trần Công Danh wrote:\n> \n> > Shell recognises first non-assignment token as command name.\n> > Thus, ` cd t/perf; ./p0000-perf-lib-sanity.sh -d -i -v` reports:\n> > \n> > > test_cmp:1: command not found: diff -u\n> > \n> > Using `eval` to unquote $GIT_TEST_CMP as same as precedence in `git_editor`.\n> \n> Adding an \"eval\" here will subtly change the behavior for other shells.\n> Right now they'd just do whitespace splitting (which presumably busybox\n> is not), but with this we'd expand metachars, etc.\n> \n> I suspect that's fine (and maybe even preferable, because if you really\n> do have a space in the path you can actually escape it). So I don't mind\n> this change.\n> \n> I do worry that this whitespace splitting behavior could bite us in\n> other scripts. Curiously, my version of busybox (1.30.1) doesn't seem to\n> have any problem with it, though.\n\nAh, this patch wasn't meant for busybox.\nIt run into failure with both bash and dash in VoidLinux, and bash in\nArchLinux, I couldn't start p0000 in Ubuntu docker image due to\nmissing /usr/bin/time (even if I have /usr/bin/time installed).\n\n-- \nDanh\n"},{"id":"393507","messageId":"20200320013526.GE1858@danh.dev","threadId":"53032","inReplyTo":"xmqqwo7gc8aq.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/6] t4061: use POSIX compliance regex(7)","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-03-20T01:35:26Z","receivedAt":"2020-03-20T01:35:31Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-03-19 15:02:37-0700, Junio C Hamano wrote:\n> Eric Sunshine writes:\n> \n> > On Thu, Mar 19, 2020 at 11:53 AM Jeff King wrote:\n> >> On Thu, Mar 19, 2020 at 09:00:02PM +0700, Đoàn Trần Công Danh wrote:\n> >> > Fix it by using literal `+` instead.\n> >>\n> >> This makes sense, I think. It could hurt a sed which is expected ERE and\n> >> needs the \"+\" escaped, but I think such a sed would be wrong (and I\n> >> imagine would break things elsewhere).\n> >\n> > I had the same thought and considered suggesting a character class:\n> >\n> > sed -n -e \"1,4d\" -e \"s/^[+]//p\" <\"$1\" >.tmp-1\n> >\n> > to make it painfully obvious that \"+\" is not special in the\n> > expression. But then I thought better of it -- for the same reason as\n> > you (to wit: such a 'sed' would be wrong) -- and decided against\n> > saying anything.\n> \n> I have only one thing that needs fixing, which is s/compliance/compliant/;\n> on the title. Other than that, it looks good.\n> \n> Having said that, I would have done the [+] thing if I were doing\n> this patch myself. As long as we see no \"wrong\" sed that is broken\n> by this change, I am OK with it, though.\n\nWell, `[+]` thing was my first version for this change,\nbut I change in to this version afterward.\n\nHowever, your comment in a later patch:\n\n> IOW, I do not have trouble changing the test so that it works with\n> noncompliant \"diff\". But then in the same series, I would prefer to\n> see the existing test keeps working with a possibly noncompliant\n> \"sed\" implementation that has been working well with the tests.\n\nchanged my mind.\n\nI will use `[+]` here in the reroll.\n\n-- \nDanh\n"},{"id":"393508","messageId":"20200320014142.GF1858@danh.dev","threadId":"53032","inReplyTo":"xmqqsgi4c7o6.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 5/6] t7063: use POSIX find(1) syntax","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-03-20T01:41:42Z","receivedAt":"2020-03-20T01:41:50Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-03-19 15:16:09-0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > On Thu, Mar 19, 2020 at 09:00:06PM +0700, Đoàn Trần Công Danh wrote:\n> >\n> >> Since commit 6b7728db81, (t7063: work around FreeBSD's lazy mtime\n> >> update feature, 2016-08-03), we started to use ls as a trick to update\n> >> directory's mtime.\n> >> \n> >> However, `-ls` flag isn't required by POSIX's find(1), and\n> >> busybox(1) doesn't implement it.\n> >> \n> >> Use an equivalence `-exec ls -dils {} +` instead.\n> >\n> > Makes sense. I wonder if we need all of \"-dils\", but it's not clear to\n\nFrom the original commit message, I think whichever flags that call\nstat(2) would be do it. It's `-d` (to check is_directory), and `-i`\nfor inode number.\n\nThis make make wonder, will it be enough to just use:\n\n\tfind . -type d >/dev/null\n\n> > me which syscalls actually trigger the FreeBSD lazy-update behavior. I\n> > guess probably it's stat()ing the directory, so \"ls -ld\" would be\n> > sufficient (and that's implied by the examples in 6b7728db81).\n> >\n> > But I doubt the extra options would create a portability problem, so I\n> > think it's fine either way.\n> \n> Thanks. I too wondered if -dils is really needed (POSIX of course\n> have all of them, but we have to deal with non-POSIX systems, too,\n> and I am not sure how things like \"-i\" works there).\n\nI think \"-i\" asks for stat(2) to get inode number,\nwhich will ask FreeBSD sync mtime.\n> \n> s/equivalence/equivalent/; perhaps?\n\nWill do, I've never correctly used -ence and -ent pairs of words.\n\n-- \nDanh\n"},{"id":"393509","messageId":"20200320015223.GG1858@danh.dev","threadId":"53032","inReplyTo":"20200319163334.GG3513282@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] t4124: fix test for non-compliance diff","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-03-20T01:52:23Z","receivedAt":"2020-03-20T01:52:30Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-03-19 12:33:34-0400, Jeff King wrote:\n> On Thu, Mar 19, 2020 at 09:00:07PM +0700, Đoàn Trần Công Danh wrote:\n> \n> > POSIX's diff(1) requires output in normal diff format.\n> > However, busybox's diff's output is written in unified format.\n> \n> That's a pretty big difference. I'm surprised this only produces one\n> problem in the test scripts. ;)\n> \n> > POSIX requires no option for normal-diff format.\n> > \n> > A hint in test-lib-functions::test_cmp said `diff -u` isn't available\n> > everywhere.\n> > \n> > Workaround this problem by assuming `diff(1)` output is unified\n> > if we couldn't make anything from normal-diff format.\n> \n> I wonder if we could use \"git diff\" here. We have to be careful about\n> circular reasoning in our tests (i.e., making sure we're not verifying\n> output with the same code that we're testing), but I think here we're\n> checking how \"apply --whitespace=fix\" works.\n> \n> But if this is the only spot, then adjusting to handle unified or normal\n> diff isn't too bad.\n> \n> > diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\n> > index 971a5a7512..2a54ce96b5 100755\n> > --- a/t/t4124-apply-ws-rule.sh\n> > +++ b/t/t4124-apply-ws-rule.sh\n> > @@ -52,6 +52,12 @@ test_fix () {\n> > \n> > \t# find touched lines\n> > \t$DIFF file target | sed -n -e \"s/^> //p\" >fixed\n> > +\tif ! test -s fixed; then\n> > +\t\t$DIFF file target |\n> > +\t\tgrep '^+' |\n> > +\t\tgrep -v '^+++' |\n> > +\t\tsed -e \"s/+//\" >fixed\n> > +\tfi\n> \n> I think those greps could be lumped into sed like:\n> \n> sed -ne \"s/^+[^+]//p\"\n> \n> (at the cost of missing blank lines, but I think that's OK for our\n> purposes here; it could be fixed with an ERE).\n> \n> Could we then make a single invocation that covers both diff formats? We\n> can further observe that the only thing we do with the \"fixed\" file is\n> count the lines, so we can leave the markers. Which means we could ditch\n> sed entirely and use grep. Something like:\n> \n> diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\n> index 971a5a7512..15cb0c81b7 100755\n> --- a/t/t4124-apply-ws-rule.sh\n> +++ b/t/t4124-apply-ws-rule.sh\n> @@ -50,8 +50,9 @@ test_fix () {\n> \t# fix should not barf\n> \tapply_patch --whitespace=fix || return 1\n> \n> -\t# find touched lines\n> -\t$DIFF file target | sed -n -e \"s/^> //p\" >fixed\n> +\t# find touched lines; handle either normal or unified\n> +\t# diff, as system diff may generate either\n> +\t$DIFF file target | grep '^[>+][^+]' >fixed\n> \n> \t# the changed lines are all expected to change\n> \tfixed_cnt=$(wc -l \n> seems to work for with both busybox diff and GNU diff.\n\n3 lines after this one:\n\n\t?*) expect_cnt=$(grep \"[$1]\" +]\".\n\nConsidering the complicated of:\n\n\ttest_expect_success \"rule=$rule\" '\n\t\tgit config core.whitespace \"$rule\" &&\n\t\ttest_fix \"$tt$ts$ti$th\"\n\t'\n\nI think it's better to use sed here.\n\n-- \nDanh\n"},{"id":"393510","messageId":"20200320022002.GA13597@danh.dev","threadId":"53032","inReplyTo":"20200320014142.GF1858@danh.dev","subject":"Re: [PATCH 5/6] t7063: use POSIX find(1) syntax","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-03-20T02:20:02Z","receivedAt":"2020-03-20T02:20:10Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-03-20 08:41:42+0700, Danh Doan wrote:\n> On 2020-03-19 15:16:09-0700, Junio C Hamano wrote:\n> > Jeff King writes:\n> > \n> > > On Thu, Mar 19, 2020 at 09:00:06PM +0700, Đoàn Trần Công Danh wrote:\n> > >\n> > >> Since commit 6b7728db81, (t7063: work around FreeBSD's lazy mtime\n> > >> update feature, 2016-08-03), we started to use ls as a trick to update\n> > >> directory's mtime.\n> > >> \n> > >> However, `-ls` flag isn't required by POSIX's find(1), and\n> > >> busybox(1) doesn't implement it.\n> > >> \n> > >> Use an equivalence `-exec ls -dils {} +` instead.\n> > >\n> > > Makes sense. I wonder if we need all of \"-dils\", but it's not clear to\n> \n> From the original commit message, I think whichever flags that call\n> stat(2) would be do it. It's `-d` (to check is_directory), and `-i`\n> for inode number.\n> \n> This make make wonder, will it be enough to just use:\n> \n> \tfind . -type d >/dev/null\n\nFrom the conversation in: xmqqmvktakvt.fsf@gitster.mtv.corp.google.com\nI think\n\n\tfind . -type d\n\nwould trigger enough lstat(2), and `-ls` was added for separated stat(2).\n\nI guess either `-exec ls -d` or `-exec ls -i` will be enough.\n\n> \n> > > me which syscalls actually trigger the FreeBSD lazy-update behavior. I\n> > > guess probably it's stat()ing the directory, so \"ls -ld\" would be\n> > > sufficient (and that's implied by the examples in 6b7728db81).\n> > >\n> > > But I doubt the extra options would create a portability problem, so I\n> > > think it's fine either way.\n> > \n> > Thanks. I too wondered if -dils is really needed (POSIX of course\n> > have all of them, but we have to deal with non-POSIX systems, too,\n> > and I am not sure how things like \"-i\" works there).\n> \n> I think \"-i\" asks for stat(2) to get inode number,\n> which will ask FreeBSD sync mtime.\n> > \n> > s/equivalence/equivalent/; perhaps?\n> \n> Will do, I've never correctly used -ence and -ent pairs of words.\n\n-- \nDanh\n"},{"id":"393512","messageId":"20200320052055.GA499858@coredump.intra.peff.net","threadId":"53032","inReplyTo":"xmqqlfnwc5p0.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 6/6] t4124: fix test for non-compliance diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-20T05:20:55Z","receivedAt":"2020-03-20T05:20:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2020 at 03:58:51PM -0700, Junio C Hamano wrote:\n\n> >> Workaround this problem by assuming `diff(1)` output is unified\n> >> if we couldn't make anything from normal-diff format.\n> \n> I do not mind working it around, but I am a bit disturbed by an\n> uneven attitude towards POSIX noncompliance this series has. If\n> we were willing to break other people's \"sed\" that does not do BRE\n> correctly, instead of using '[+]' trick to accomodate them while\n> making sure that an implementation that does not use nonstandard\n> extension and does only BRE, we should just similarly be writing\n> such an implementation of noncompliant diff off as broken, yet we\n> bend backwards over to make sure we can work with them here.\n> \n> IOW, I do not have trouble changing the test so that it works with\n> noncompliant \"diff\". But then in the same series, I would prefer to\n> see the existing test keeps working with a possibly noncompliant\n> \"sed\" implementation that has been working well with the tests.\n\nI don't think it's inconsistent. Real-world experience trumps standards.\nWe _know_ that there is a real-world diff that generates only unified\ndiffs, and it is not too hard to work around it. So we should do so.\n\nA sed that uses ERE and requires backslash-escaping pluses is\ntheoretical at this point. POSIX forbids it, and I would guess that\nworking around it would be more than just the \"[+]\" we found, because\nother patterns probably need it, too. But we won't know until we find\none to test on.\n\nSo I'm not entirely against \"[+]\" as a defensive measure. But I have a\nslight preference to avoid it until we know it's needed, not because\nit's hard to do once, but because I don't want to grow too many\ndefensive superstitions if we don't know they're warranted.\n\n-Peff\n"},{"id":"393513","messageId":"20200320052314.GB499858@coredump.intra.peff.net","threadId":"53032","inReplyTo":"20200320015223.GG1858@danh.dev","subject":"Re: [PATCH 6/6] t4124: fix test for non-compliance diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-20T05:23:14Z","receivedAt":"2020-03-20T05:23:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 20, 2020 at 08:52:23AM +0700, Danh Doan wrote:\n\n> > diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\n> > index 971a5a7512..15cb0c81b7 100755\n> > --- a/t/t4124-apply-ws-rule.sh\n> > +++ b/t/t4124-apply-ws-rule.sh\n> > @@ -50,8 +50,9 @@ test_fix () {\n> > \t# fix should not barf\n> > \tapply_patch --whitespace=fix || return 1\n> > \n> > -\t# find touched lines\n> > -\t$DIFF file target | sed -n -e \"s/^> //p\" >fixed\n> > +\t# find touched lines; handle either normal or unified\n> > +\t# diff, as system diff may generate either\n> > +\t$DIFF file target | grep '^[>+][^+]' >fixed\n> > \n> > \t# the changed lines are all expected to change\n> > \tfixed_cnt=$(wc -l > \n> > seems to work for with both busybox diff and GNU diff.\n> \n> 3 lines after this one:\n> \n> \t?*) expect_cnt=$(grep \"[$1]\" \n> As of now, we could simply replace sed with grep entirely,\n> because ! \"$1\" ~\"[>+]\".\n> \n> Considering the complicated of:\n> \n> \ttest_expect_success \"rule=$rule\" '\n> \t\tgit config core.whitespace \"$rule\" &&\n> \t\ttest_fix \"$tt$ts$ti$th\"\n> \t'\n> \n> I think it's better to use sed here.\n\nFair enough. I think it's OK now, but I agree that it puts a pretty\nsubtle assumption into the test_fix function (and that's far removed\nfrom what actual tests will call it with, so it's easy to miss). Using\nsed should be more maintainable.\n\n-Peff\n"},{"id":"393514","messageId":"20200320053052.GC499858@coredump.intra.peff.net","threadId":"53032","inReplyTo":"20200320003715.GA1858@danh.dev","subject":"Re: [PATCH 0/6] fix test failure with busybox","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-20T05:30:52Z","receivedAt":"2020-03-20T05:30:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 20, 2020 at 07:37:15AM +0700, Danh Doan wrote:\n\n> > The issue in t5703 is the sed call in get_actual_commits(). It's trying\n> > to cut off the early (text) part of the file, and pass through the\n> > binary goo of the packfile. Presumably busybox's sed isn't binary-clean.\n> \n> I've checked, busybox's sed think every input is text file,\n> and in POSIX, every line in every text file must be terminated by\n> .\n> \n> Thus, busybox's sed append a after `0000` marker, render the\n> pack file invalid.\n\nAh, thanks for tracking it down.\n\n> > We're also feeding this into a test-tool helper. It's possible that\n> > helper could be made smart enough to replace both this sed invocation\n> > and the one in get_actual_refs().\n> \n> I looked into this direction, I guess you meant something like this:\n> [...]\n> @@ -53,6 +54,13 @@ static void unpack(void)\n> static void unpack_sideband(void)\n> {\n> \tstruct packet_reader reader;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\twhile (strbuf_getline(&buf, stdin) == 0)\n> +\t\tif (strstr(buf.buf, \"packfile\"))\n> +\t\t break;\n> +\tstrbuf_release(&buf);\n> +\n> \tpacket_reader_init(&reader, 0, NULL, 0,\n> \t\t\t PACKET_READ_GENTLE_ON_EOF |\n> \t\t\t PACKET_READ_CHOMP_NEWLINE);\n> [...]\n> But, this doesn't work. as `packet_reader_read` reads directly from fd 0.\n\nYou'd want to decode the packets first first, and then skip until after\nyou see the packet with \"packfile\" in it. Or to be more generic, just\nwait until you see packets with actual sideband data. Something like the\npatch below works (though I'm also fine with just switching to perl).\n\ndiff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c\nindex 282d536384..12ca698e17 100644\n--- a/t/helper/test-pkt-line.c\n+++ b/t/helper/test-pkt-line.c\n@@ -67,7 +67,7 @@ static void unpack_sideband(void)\n \t\tcase PACKET_READ_NORMAL:\n \t\t\tband = reader.line[0] & 0xff;\n \t\t\tif (band < 1 || band > 2)\n-\t\t\t\tdie(\"unexpected side band %d\", band);\n+\t\t\t\tcontinue; /* skip non-sideband packets */\n \t\t\tfd = band;\n \n \t\t\twrite_or_die(fd, reader.line + 1, reader.pktlen - 1);\ndiff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh\nindex 7fba3063bf..a34460f7d8 100755\n--- a/t/t5703-upload-pack-ref-in-want.sh\n+++ b/t/t5703-upload-pack-ref-in-want.sh\n@@ -13,10 +13,7 @@ get_actual_refs () {\n }\n \n get_actual_commits () {\n-\tsed -n -e '/packfile/,/0000/{\n-\t\t/packfile/d\n-\t\tp\n-\t\t}' o.pack &&\n+\ttest-tool pkt-line unpack-sideband o.pack &&\n \tgit index-pack o.pack &&\n \tgit verify-pack -v o.idx >objs &&\n \tgrep commit objs | cut -d\" \" -f1 | sort >actual_commits\n"},{"id":"393515","messageId":"20200320053214.GD499858@coredump.intra.peff.net","threadId":"53032","inReplyTo":"20200320003921.GB1858@danh.dev","subject":"Re: [PATCH 3/6] t5003: skip conversion test if unzip -a is unavailable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-20T05:32:14Z","receivedAt":"2020-03-20T05:32:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 20, 2020 at 07:39:21AM +0700, Danh Doan wrote:\n\n> > > +test_lazy_prereq UNZIP_CONVERT '\n> > > +\t(\n> > > +\t\tmkdir unzip-convert &&\n> > > +\t\tcd unzip-convert &&\n> > > +\t\t\"$GIT_UNZIP\" -a \"$TEST_DIRECTORY\"/t5003/infozip-symlinks.zip\n> > > +\t)\n> > > +'\n> > \n> > Lazy prereqs are already evaluated in a throw-away directory, so you can\n> > drop the subshell and mkdir/cd.\n> \n> I was trying to keep it consistent with UNZIP_SYMLINKS above it.\n> Maybe, it's worth to clean it, too.\n\nAh, sorry, I should have looked at more context. I think it would be\nworth cleaning up the existing one.\n\n-Peff\n"},{"id":"393516","messageId":"20200320053730.GE499858@coredump.intra.peff.net","threadId":"53032","inReplyTo":"20200320014142.GF1858@danh.dev","subject":"Re: [PATCH 5/6] t7063: use POSIX find(1) syntax","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-20T05:37:30Z","receivedAt":"2020-03-20T05:37:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 20, 2020 at 08:41:42AM +0700, Danh Doan wrote:\n\n> On 2020-03-19 15:16:09-0700, Junio C Hamano wrote:\n> > Jeff King writes:\n> > \n> > > On Thu, Mar 19, 2020 at 09:00:06PM +0700, Đoàn Trần Công Danh wrote:\n> > >\n> > >> Since commit 6b7728db81, (t7063: work around FreeBSD's lazy mtime\n> > >> update feature, 2016-08-03), we started to use ls as a trick to update\n> > >> directory's mtime.\n> > >> \n> > >> However, `-ls` flag isn't required by POSIX's find(1), and\n> > >> busybox(1) doesn't implement it.\n> > >> \n> > >> Use an equivalence `-exec ls -dils {} +` instead.\n> > >\n> > > Makes sense. I wonder if we need all of \"-dils\", but it's not clear to\n> \n> From the original commit message, I think whichever flags that call\n> stat(2) would be do it. It's `-d` (to check is_directory), and `-i`\n> for inode number.\n> \n> This make make wonder, will it be enough to just use:\n> \n> \tfind . -type d >/dev/null\n\nPerhaps we can get a friendly FreeBSD developer (cc'd) to run a quick\ntest for us.\n\nEd, the question is whether:\n\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex 190ae149cf..6791c6b95a 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -18,7 +18,7 @@ GIT_FORCE_UNTRACKED_CACHE=true\n export GIT_FORCE_UNTRACKED_CACHE\n \n sync_mtime () {\n-\tfind . -type d -ls >/dev/null\n+\tfind . -type d >/dev/null\n }\n \n avoid_racy() {\n\nlets t7063 consistently pass on FreeBSD.\n\n-Peff\n"},{"id":"393670","messageId":"20200322003703.GA11801@danh.dev","threadId":"53032","inReplyTo":"20200320053730.GE499858@coredump.intra.peff.net","subject":"Re: [PATCH 5/6] t7063: use POSIX find(1) syntax","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-03-22T00:37:03Z","receivedAt":"2020-03-22T00:37:09Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-03-20 01:37:30-0400, Jeff King wrote:\n> On Fri, Mar 20, 2020 at 08:41:42AM +0700, Danh Doan wrote:\n> \n> > On 2020-03-19 15:16:09-0700, Junio C Hamano wrote:\n> > > Jeff King writes:\n> > > \n> > > > On Thu, Mar 19, 2020 at 09:00:06PM +0700, Đoàn Trần Công Danh wrote:\n> > > >\n> > > >> Since commit 6b7728db81, (t7063: work around FreeBSD's lazy mtime\n> > > >> update feature, 2016-08-03), we started to use ls as a trick to update\n> > > >> directory's mtime.\n> > > >> \n> > > >> However, `-ls` flag isn't required by POSIX's find(1), and\n> > > >> busybox(1) doesn't implement it.\n> > > >> \n> > > >> Use an equivalence `-exec ls -dils {} +` instead.\n> > > >\n> > > > Makes sense. I wonder if we need all of \"-dils\", but it's not clear to\n> > \n> > From the original commit message, I think whichever flags that call\n> > stat(2) would be do it. It's `-d` (to check is_directory), and `-i`\n> > for inode number.\n> > \n> > This make make wonder, will it be enough to just use:\n> > \n> > \tfind . -type d >/dev/null\n> \n> Perhaps we can get a friendly FreeBSD developer (cc'd) to run a quick\n> test for us.\n> \n> Ed, the question is whether:\n> \n> diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\n> index 190ae149cf..6791c6b95a 100755\n> --- a/t/t7063-status-untracked-cache.sh\n> +++ b/t/t7063-status-untracked-cache.sh\n> @@ -18,7 +18,7 @@ GIT_FORCE_UNTRACKED_CACHE=true\n> export GIT_FORCE_UNTRACKED_CACHE\n> \n> sync_mtime () {\n> -\tfind . -type d -ls >/dev/null\n> +\tfind . -type d >/dev/null\n> }\n> \n> avoid_racy() {\n> \n> lets t7063 consistently pass on FreeBSD.\n\nI've tried myself with a FreeBSD VM which stays on top of an HDD,\nt7063 consistently pass for 1000 run.\n\nI guess it should be fine\n\nI'll resend with this while waiting for Ed's response.\n\n-- \nDanh\n"},{"id":"393689","messageId":"20200322060521.GD578498@coredump.intra.peff.net","threadId":"53032","inReplyTo":"20200322003703.GA11801@danh.dev","subject":"Re: [PATCH 5/6] t7063: use POSIX find(1) syntax","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-22T06:05:21Z","receivedAt":"2020-03-22T06:05:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 22, 2020 at 07:37:03AM +0700, Danh Doan wrote:\n\n> > sync_mtime () {\n> > -\tfind . -type d -ls >/dev/null\n> > +\tfind . -type d >/dev/null\n> > }\n> > \n> > avoid_racy() {\n> > \n> > lets t7063 consistently pass on FreeBSD.\n> \n> I've tried myself with a FreeBSD VM which stays on top of an HDD,\n> t7063 consistently pass for 1000 run.\n> \n> I guess it should be fine\n> \n> I'll resend with this while waiting for Ed's response.\n\nThanks, that's good enough for me. I just didn't want us committing\nsomething without _somebody_ trying it on FreeBSD (and I was too lazy to\ninstall a VM myself).\n\n-Peff\n"}]}