{"thread":{"id":"49039","subject":"[PATCH 0/1] t/test-lib: make `test_dir_is_empty` more robust","startedAt":"2018-08-05T02:21:03Z","lastAt":"2018-09-12T19:50:24Z","messageCount":23,"participants":["William Chargin","Jonathan Nieder","Eric Sunshine","Junio C Hamano","Johannes Sixt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"354517","messageId":"20180805022002.28907-1-wchargin@gmail.com","threadId":"49039","inReplyTo":null,"subject":"[PATCH 0/1] t/test-lib: make `test_dir_is_empty` more robust","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-08-05T02:20:01Z","receivedAt":"2018-08-05T02:21:03Z","isPatch":true,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"While the `test_dir_is_empty` function appears correct in most normal\nuse cases, it can fail when filenames contain newlines. I originally\nwrote this patch for the standalone Sharness library, but that library\nadvises that such patches be sent to the Git mailing list first.\n\nWilliam Chargin (1):\n  t/test-lib: make `test_dir_is_empty` more robust\n\n t/t0000-basic.sh        | 29 +++++++++++++++++++++++++++++\n t/test-lib-functions.sh |  2 +-\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\n-- \n2.18.0.548.g79b975644\n\n"},{"id":"354518","messageId":"20180805022002.28907-2-wchargin@gmail.com","threadId":"49039","inReplyTo":"20180805022002.28907-1-wchargin@gmail.com","subject":"[PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-08-05T02:20:02Z","receivedAt":"2018-08-05T02:21:03Z","isPatch":true,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"The previous behavior could incorrectly pass given a directory with a\nfilename containing a newline. This patch changes the implementation to\nuse `ls -A`, which is specified by POSIX. The output should be empty\nexactly if the directory is empty.\n\nThe newly added unit test fails before this change and passes after it.\n\nSigned-off-by: William Chargin <wchargin@gmail.com>\n---\n t/t0000-basic.sh        | 29 +++++++++++++++++++++++++++++\n t/test-lib-functions.sh |  2 +-\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 34859fe4a..3885b26f9 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -821,6 +821,35 @@ test_expect_success 'tests clean up even on failures' \"\n \tEOF\n \"\n \n+test_expect_success 'test_dir_is_empty behaves even in pathological cases' \"\n+\trun_sub_test_lib_test \\\n+\t\tdir-empty 'behavior of test_dir_is_empty' <<-\\\\EOF &&\n+\ttest_expect_success 'should pass with actually empty directory' '\n+\t\tmkdir empty_dir &&\n+\t\ttest_dir_is_empty empty_dir\n+\t'\n+\ttest_expect_success 'should fail with a normal filename' '\n+\t\tmkdir nonempty_dir &&\n+\t\ttouch nonempty_dir/some_file &&\n+\t\ttest_must_fail test_dir_is_empty nonempty_dir\n+\t'\n+\ttest_expect_success 'should fail with dot-newline-dot filename' '\n+\t\tmkdir pathological_dir &&\n+\t\tprintf \\\"pathological_dir/.\\\\\\\\n.\\\\\\\\0\\\" | xargs -0 touch &&\n+\t\ttest_must_fail test_dir_is_empty pathological_dir\n+\t'\n+\ttest_done\n+\tEOF\n+\tcheck_sub_test_lib_test dir-empty <<-\\\\EOF\n+\t> ok 1 - should pass with actually empty directory\n+\t> ok 2 - should fail with a normal filename\n+\t> ok 3 - should fail with dot-newline-dot filename\n+\t> # passed all 3 test(s)\n+\t> 1..3\n+\tEOF\n+\"\n+\n+\n ################################################################\n # Basics of the basics\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 2b2181dca..f7ff28ef6 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -568,7 +568,7 @@ test_path_is_dir () {\n # Check if the directory exists and is empty as expected, barf otherwise.\n test_dir_is_empty () {\n \ttest_path_is_dir \"$1\" &&\n-\tif test -n \"$(ls -a1 \"$1\" | egrep -v '^\\.\\.?$')\"\n+\tif test \"$(ls -A1 \"$1\" | wc -c)\" != 0\n \tthen\n \t\techo \"Directory '$1' is not empty, it contains:\"\n \t\tls -la \"$1\"\n-- \n2.18.0.548.g79b975644\n\n"},{"id":"354523","messageId":"20180805033629.GH258270@aiede.svl.corp.google.com","threadId":"49039","inReplyTo":"20180805022002.28907-1-wchargin@gmail.com","subject":"Re: [PATCH 0/1] t/test-lib: make `test_dir_is_empty` more robust","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-05T03:36:29Z","receivedAt":"2018-08-05T03:36:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nWilliam Chargin wrote:\n\n> While the `test_dir_is_empty` function appears correct in most normal\n> use cases, it can fail when filenames contain newlines.\n\nThis information belongs in the commit message, since it's useful\ncontext for understanding the motivation behind the patch when\nencountering it with e.g. \"git log\".  That's part of why I recommend\nnever sending a separate cover-letter email for a single-patch series.\n\nSee [1] from Documentation/SubmittingPatches for more on this subject.\n\n>                                                         I originally\n> wrote this patch for the standalone Sharness library, but that library\n> advises that such patches be sent to the Git mailing list first.\n\nThanks for writing it!  Continuing the note about administrivia, this\nkind of cover letter material that you want to not be part of the\ncommit message can go below the three-dashes delimiter when you send a\npatch.  There's more about this at [2].\n\nThanks again,\nJonathan\n\n[1] https://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html#describe-changes\n[2] https://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html#send-patches\n"},{"id":"354524","messageId":"CAFW+GMCT_Ptu59vBLTvFXNn6nX5pBgyToCyYKe8J7JZnV2Bv9Q@mail.gmail.com","threadId":"49039","inReplyTo":"20180805033629.GH258270@aiede.svl.corp.google.com","subject":"Re: [PATCH 0/1] t/test-lib: make `test_dir_is_empty` more robust","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-08-05T04:19:19Z","receivedAt":"2018-08-05T04:19:42Z","isPatch":true,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"Hi Jonathan,\n\n> This information belongs in the commit message\n\nAgreed: I included it at the start of the commit message, though I\nsuppose that the wording in the cover letter is clearer, so I've amended\nthe commit to use that wording instead.\n\n>                         Continuing the note about administrivia, this\n> kind of cover letter material that you want to not be part of the\n> commit message can go below the three-dashes delimiter when you send a\n> patch.\n\nPerfect; this is what I was missing. Thanks. Let me try again. :-)\n"},{"id":"354525","messageId":"20180805041956.GI258270@aiede.svl.corp.google.com","threadId":"49039","inReplyTo":"20180805022002.28907-2-wchargin@gmail.com","subject":"Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-05T04:19:56Z","receivedAt":"2018-08-05T04:20:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nWilliam Chargin wrote:\n\n> Subject: t/test-lib: make `test_dir_is_empty` more robust\n\nThis subject line will appear out of context in \"git shortlog\" output,\nso it's useful to pack in enough information to briefly summarize what\nthe change does.  How about something like\n\n\ttest_dir_is_empty: avoid being confused by $'.\\n.' file\n\nor\n\n\ttest_dir_is_empty: simplify by using \"ls --almost-all\"\n\n?\n\n[...]\n> +\ttest_expect_success 'should fail with dot-newline-dot filename' '\n> +\t\tmkdir pathological_dir &&\n> +\t\tprintf \\\"pathological_dir/.\\\\\\\\n.\\\\\\\\0\\\" | xargs -0 touch &&\n\nI don't think \"xargs -0\" is present on all supported platforms.  I'd\nbe tempted to use\n\n\t\t>pathological_dir/$'.\\n.'\n\nbut $'' is too recent of a shell feature to count on (e.g., dash doesn't\nsupport it).  See t/t3600-rm.sh for an example of a portable way to do\nthis.\n\nNot all filesystems support newlines in filenames.  I think we'd want\nto factor out the FUNNYNAMES prereq from there into a test_lazy_prereq\nso this test can be skipped on such filenames.\n\n[...]\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -568,7 +568,7 @@ test_path_is_dir () {\n>  # Check if the directory exists and is empty as expected, barf otherwise.\n>  test_dir_is_empty () {\n>  \ttest_path_is_dir \"$1\" &&\n> -\tif test -n \"$(ls -a1 \"$1\" | egrep -v '^\\.\\.?$')\"\n> +\tif test \"$(ls -A1 \"$1\" | wc -c)\" != 0\n\nAnother portability gotcha: wc output includes a space on Mac so this\ntest would always return true there.  How about\n\n\tif test -n \"$(ls -A1 \"$1\")\"\n\n?\n\n\"ls -A\" was added in POSIX.1-2017.  Its rationale section explains\n\n\tEarlier versions of this standard did not describe the BSD -A\n\toption (like -a, but dot and dot-dot are not written out). It\n\thas been added due to widespread implementation.\n\nThat's very recent, but the widespread implementation it mentions is\nless so.  This would be our first use of \"ls -A\", so I'd be interested\nto hear from people on more obscure platforms.  It does seem to be\nwidespread.\n\nCan you say a little more about where you ran into this?  Did you\ndiscover it by code inspection?\n\nI do think the resulting implementation using -A is simpler.  Thanks\nfor writing it.\n\nThanks and hope that helps,\nJonathan\n"},{"id":"354526","messageId":"20180805042031.20447-1-wchargin@gmail.com","threadId":"49039","inReplyTo":"20180805033629.GH258270@aiede.svl.corp.google.com","subject":"[PATCH v2] t/test-lib: make `test_dir_is_empty` more robust","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-08-05T04:20:31Z","receivedAt":"2018-08-05T04:20:39Z","isPatch":true,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"While the `test_dir_is_empty` function appears correct in most normal\nuse cases, it can fail when filenames contain newlines. This patch\nchanges the implementation to use `ls -A`, which is specified by POSIX.\nThe output should be empty exactly if the directory is empty.\n\nThe newly added unit test fails before this change and passes after it.\n\nSigned-off-by: William Chargin <wchargin@gmail.com>\n---\n\nI originally wrote this patch for the standalone Sharness library, but\nthat library advises that such patches be sent to the Git mailing list\nfirst.\n\n t/t0000-basic.sh        | 29 +++++++++++++++++++++++++++++\n t/test-lib-functions.sh |  2 +-\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 34859fe4a..3885b26f9 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -821,6 +821,35 @@ test_expect_success 'tests clean up even on failures' \"\n \tEOF\n \"\n \n+test_expect_success 'test_dir_is_empty behaves even in pathological cases' \"\n+\trun_sub_test_lib_test \\\n+\t\tdir-empty 'behavior of test_dir_is_empty' <<-\\\\EOF &&\n+\ttest_expect_success 'should pass with actually empty directory' '\n+\t\tmkdir empty_dir &&\n+\t\ttest_dir_is_empty empty_dir\n+\t'\n+\ttest_expect_success 'should fail with a normal filename' '\n+\t\tmkdir nonempty_dir &&\n+\t\ttouch nonempty_dir/some_file &&\n+\t\ttest_must_fail test_dir_is_empty nonempty_dir\n+\t'\n+\ttest_expect_success 'should fail with dot-newline-dot filename' '\n+\t\tmkdir pathological_dir &&\n+\t\tprintf \\\"pathological_dir/.\\\\\\\\n.\\\\\\\\0\\\" | xargs -0 touch &&\n+\t\ttest_must_fail test_dir_is_empty pathological_dir\n+\t'\n+\ttest_done\n+\tEOF\n+\tcheck_sub_test_lib_test dir-empty <<-\\\\EOF\n+\t> ok 1 - should pass with actually empty directory\n+\t> ok 2 - should fail with a normal filename\n+\t> ok 3 - should fail with dot-newline-dot filename\n+\t> # passed all 3 test(s)\n+\t> 1..3\n+\tEOF\n+\"\n+\n+\n ################################################################\n # Basics of the basics\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 2b2181dca..f7ff28ef6 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -568,7 +568,7 @@ test_path_is_dir () {\n # Check if the directory exists and is empty as expected, barf otherwise.\n test_dir_is_empty () {\n \ttest_path_is_dir \"$1\" &&\n-\tif test -n \"$(ls -a1 \"$1\" | egrep -v '^\\.\\.?$')\"\n+\tif test \"$(ls -A1 \"$1\" | wc -c)\" != 0\n \tthen\n \t\techo \"Directory '$1' is not empty, it contains:\"\n \t\tls -la \"$1\"\n-- \n2.18.0.548.geb6c14151\n\n"},{"id":"354528","messageId":"CAPig+cQSryjWrFZML_g0fR5oq6dzVaWPXm_4R-Jg_ay1R2DzbQ@mail.gmail.com","threadId":"49039","inReplyTo":"20180805041956.GI258270@aiede.svl.corp.google.com","subject":"Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-05T05:23:05Z","receivedAt":"2018-08-05T05:37:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 5, 2018 at 12:20 AM Jonathan Nieder <jrnieder@gmail.com> wrote:\n> William Chargin wrote:\n> >  test_dir_is_empty () {\n> >       test_path_is_dir \"$1\" &&\n> > -     if test -n \"$(ls -a1 \"$1\" | egrep -v '^\\.\\.?$')\"\n> > +     if test \"$(ls -A1 \"$1\" | wc -c)\" != 0\n>\n> Another portability gotcha: wc output includes a space on Mac so this\n> test would always return true there.  How about\n>\n>         if test -n \"$(ls -A1 \"$1\")\"\n>\n> \"ls -A\" was added in POSIX.1-2017. [...]\n> That's very recent, but the widespread implementation it mentions is\n> less so.  This would be our first use of \"ls -A\", so I'd be interested\n> to hear from people on more obscure platforms.  It does seem to be\n> widespread.\n\nA simpler approach, without the portability concerns of -A, would be\nto remove the \".\" and \"..\" lines from the top of the listing:\n\n    ls -f1 \"$1\" | sed '1,2d'\n\nIf we're worried about -f not being sufficiently portable, then an\neven simpler approach would be to check whether the output of 'ls -a1'\nhas more lines than the two expected (\".\" and \"..\"):\n\n    test $(ls -a1 \"$1\" | wc -l) -gt 2\n\nI think I favor this final implementation over the others.\n"},{"id":"354529","messageId":"CAFW+GMBrRUQ88XZzLMVAiEGDjLDwLaW6Qi1mSGrXYHoSu8iJag@mail.gmail.com","threadId":"49039","inReplyTo":"20180805041956.GI258270@aiede.svl.corp.google.com","subject":"Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-08-05T05:24:05Z","receivedAt":"2018-08-05T05:37:00Z","isPatch":true,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"Hi,\n\n> This subject line will appear out of context in \"git shortlog\" output,\n> so it's useful to pack in enough information to briefly summarize what\n> the change does.\n\nI'm happy to do so. I think that \"simplify\" is misleading, because this\nis a bug fix, not a refactoring. I like your first suggestion, though it\nexceeds the 50-character soft limit. What do you think of:\n\n        test_dir_is_empty: find files with newline in name\n\n?\n\n> I don't think \"xargs -0\" is present on all supported platforms\n\nWow---I'm shocked that it's not POSIX, but you're right. (That makes\n`xargs` so much less useful!)\n\nt/t3600-rm.sh seems to just literally embed the newline into the\nargument to `touch`. I can do that. (I intentionally avoided $'' for the\nreason that you mention.)\n\n> Not all filesystems support newlines in filenames.  I think we'd want\n> to factor out the FUNNYNAMES prereq from there into a test_lazy_prereq\n> so this test can be skipped on such filenames.\n\nThis makes sense. Would you like me to send in a separate patch to add\nthis `test_lazy_prereq` to `t/test-lib.sh`, fixing up existing uses (of\nwhich there are several), and then rebase this patch on top of it?\n\n> Another portability gotcha: wc output includes a space on Mac so this\n> test would always return true there.\n\nThat's good to know. I can use `test -n \"$(ls -A1 \"$1\")\"` as you\nsuggest, although...\n\n> \"ls -A\" was added in POSIX.1-2017. [...]\n> That's very recent, but the widespread implementation it mentions is\n> less so.  This would be our first use of \"ls -A\", so I'd be interested\n> to hear from people on more obscure platforms.  It does seem to be\n> widespread.\n\n...if you prefer, a variant of `test \"$(ls -a1 | wc -l)\" -eq 2` should\nsatisfy all the crtieria that you mention above (POSIX since forever,\nshould work on Mac). The assumption is that a file with a newline\ncharacter may take up more than one line, but every file must take up\n_at least_ one line, and we get two lines from `.` and `..`. If this\nassumption is false, then I will have learned yet another new thing!\n\n> Can you say a little more about where you ran into this?  Did you\n> discover it by code inspection?\n\nSure. Yes. I have a build script that accepts a target output directory,\nand rejects if the directory is nonempty. I used `ls -A | wc -l` to\nimplement this check. When testing the script with Sharness, I ran\nacross `test_dir_is_empty`. I was curious about the implementation,\nhaving recently implemented the same test myself. The `egrep` in the\nimplementation stood out to me as suspicious, and so it was easy to come\nup with an explicit counterexample.\n\n> I do think the resulting implementation using -A is simpler.  Thanks\n> for writing it.\n\nYou're welcome. Thank you for the detailed and helpful review.\n\nBest,\nWC\n"},{"id":"354530","messageId":"xmqqftzts63d.fsf@gitster-ct.c.googlers.com","threadId":"49039","inReplyTo":"20180805041956.GI258270@aiede.svl.corp.google.com","subject":"Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-05T06:03:18Z","receivedAt":"2018-08-05T06:03:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> but $'' is too recent of a shell feature to count on (e.g., dash doesn't\n> support it).  See t/t3600-rm.sh for an example of a portable way to do\n\nIs that \"too recent\"?  I thought it was bashism, not even in POSIX,\nbut I may be mistaken.\n\nQuite honestly, our tests are still run inside a sort-of controlled\nenvironment, so if it _requires_ use of things we have avoided so\nfar, like \"ls -A\" and \"xargs -0\", in order to be resistant to\nfunnyly-named files like dot-LF-dot, I would say it is not worth\nworrying about them--instead we can simply refrain from using such a\npathological name, can't we?\n\n\"ls -A\" may be in POSIX, but our attitude generally is to avoid\nsaying things like \"it is in POSIX so it's your platform's fault\nthat it is not yet supported\".  We instead say \"even it may be in\nPOSIX, in real life many people don't have it, so let's avoid it\".\nAnd \"xargs -0\" I do not think is.\n"},{"id":"354535","messageId":"20180805062316.GB44140@aiede.svl.corp.google.com","threadId":"49039","inReplyTo":"xmqqftzts63d.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-05T06:23:16Z","receivedAt":"2018-08-05T06:34:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> but $'' is too recent of a shell feature to count on (e.g., dash doesn't\n>> support it).  See t/t3600-rm.sh for an example of a portable way to do\n>\n> Is that \"too recent\"?  I thought it was bashism, not even in POSIX,\n> but I may be mistaken.\n\nYou're right.  I got a little ahead of myself: it's not part of POSIX\nyet but is likely to be so once the details get ironed out:\nhttp://austingroupbugs.net/view.php?id=249\n\n> Quite honestly, our tests are still run inside a sort-of controlled\n> environment, so if it _requires_ use of things we have avoided so\n> far, like \"ls -A\" and \"xargs -0\", in order to be resistant to\n> funnyly-named files like dot-LF-dot, I would say it is not worth\n> worrying about them--instead we can simply refrain from using such a\n> pathological name, can't we?\n\nThe \"xargs -0\" is a bit of a red herring.  That construct is\ndefinitely not needed for the test it was used in.\n\nFor \"ls -A\", I agree with you that the benefit is not very high, so\nthe cost would have to be pretty low for this to be worth it.  But\ngiven the lineage of \"ls -A\", I feel there's a chance that it's\nwidespread enough that it would meet that bar.\n\n> \"ls -A\" may be in POSIX, but our attitude generally is to avoid\n> saying things like \"it is in POSIX so it's your platform's fault\n> that it is not yet supported\".  We instead say \"even it may be in\n> POSIX, in real life many people don't have it, so let's avoid it\".\n> And \"xargs -0\" I do not think is.\n\nIndeed.\n\nThanks,\nJonathan\n"},{"id":"354536","messageId":"20180805063448.GC44140@aiede.svl.corp.google.com","threadId":"49039","inReplyTo":"CAFW+GMBrRUQ88XZzLMVAiEGDjLDwLaW6Qi1mSGrXYHoSu8iJag@mail.gmail.com","subject":"Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-05T06:34:48Z","receivedAt":"2018-08-05T06:34:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"William Chargin wrote:\n> Jonathan Nieder wrote:\n\n>> This subject line will appear out of context in \"git shortlog\" output,\n>> so it's useful to pack in enough information to briefly summarize what\n>> the change does.\n>\n> I'm happy to do so. I think that \"simplify\" is misleading, because this\n> is a bug fix, not a refactoring. I like your first suggestion, though it\n> exceeds the 50-character soft limit. What do you think of:\n>\n>         test_dir_is_empty: find files with newline in name\n\nThanks.  I think we can ignore the 50-character soft limit.  It's\noften too low and expressivity is more important.  So if you like my\nfirst suggestion, then I'd say start with that and tweak to your taste\nfrom there.\n\n[...]\n>> I don't think \"xargs -0\" is present on all supported platforms\n>\n> Wow---I'm shocked that it's not POSIX, but you're right. (That makes\n> `xargs` so much less useful!)\n\nTo be clear, as Junio mentioned, POSIX is useful as a guide to what\n*might* be portable, but what we care about is what is portable to the\nplatforms used in practice.\n\n[...]\n>> Not all filesystems support newlines in filenames.  I think we'd want\n>> to factor out the FUNNYNAMES prereq from there into a test_lazy_prereq\n>> so this test can be skipped on such filenames.\n>\n> This makes sense. Would you like me to send in a separate patch to add\n> this `test_lazy_prereq` to `t/test-lib.sh`, fixing up existing uses (of\n> which there are several), and then rebase this patch on top of it?\n\nThanks for the offer!  Yes, that would be nice: such a patch would be\nnice as a cleanup in its own right, too.\n\nSuch a patch would be helpful at any time.  For rebasing this patch,\nmy advice would be to hold off until the discussion has settled down a\nbit.  If we're lucky, people in other time zones might have an idea\nbeyond the ones we've come up with.\n\n[...]\n>> \"ls -A\" was added in POSIX.1-2017. [...]\n>> That's very recent, but the widespread implementation it mentions is\n>> less so.  This would be our first use of \"ls -A\", so I'd be interested\n>> to hear from people on more obscure platforms.  It does seem to be\n>> widespread.\n>\n> ...if you prefer, a variant of `test \"$(ls -a1 | wc -l)\" -eq 2` should\n> satisfy all the crtieria that you mention above (POSIX since forever,\n> should work on Mac). The assumption is that a file with a newline\n> character may take up more than one line, but every file must take up\n> _at least_ one line, and we get two lines from `.` and `..`. If this\n> assumption is false, then I will have learned yet another new thing!\n\nAs a piece of trivia, '.' and '..' are allowed not to exist.  So this test\ncan have false negatives and false positives.\n\nFilesystems omitting them are quite rare so this might work reasonably\nwell in practice, though.\n\nThanks,\nJonathan\n"},{"id":"354539","messageId":"ae85fef4-19f6-39a8-e680-3d08e74ecd5e@kdbg.org","threadId":"49039","inReplyTo":"20180805042031.20447-1-wchargin@gmail.com","subject":"Re: [PATCH v2] t/test-lib: make `test_dir_is_empty` more robust","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-08-05T08:34:40Z","receivedAt":"2018-08-05T08:34:46Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 05.08.2018 um 06:20 schrieb William Chargin:\n> While the `test_dir_is_empty` function appears correct in most normal\n> use cases, it can fail when filenames contain newlines. This patch\n> changes the implementation to use `ls -A`, which is specified by POSIX.\n> The output should be empty exactly if the directory is empty.\n> \n> The newly added unit test fails before this change and passes after it.\n> \n> Signed-off-by: William Chargin <wchargin@gmail.com>\n> ---\n> \n> I originally wrote this patch for the standalone Sharness library, but\n> that library advises that such patches be sent to the Git mailing list\n> first.\n> \n>   t/t0000-basic.sh        | 29 +++++++++++++++++++++++++++++\n>   t/test-lib-functions.sh |  2 +-\n>   2 files changed, 30 insertions(+), 1 deletion(-)\n> \n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> index 34859fe4a..3885b26f9 100755\n> --- a/t/t0000-basic.sh\n> +++ b/t/t0000-basic.sh\n> @@ -821,6 +821,35 @@ test_expect_success 'tests clean up even on failures' \"\n>   \tEOF\n>   \"\n>   \n> +test_expect_success 'test_dir_is_empty behaves even in pathological cases' \"\n> +\trun_sub_test_lib_test \\\n> +\t\tdir-empty 'behavior of test_dir_is_empty' <<-\\\\EOF &&\n> +\ttest_expect_success 'should pass with actually empty directory' '\n> +\t\tmkdir empty_dir &&\n> +\t\ttest_dir_is_empty empty_dir\n> +\t'\n> +\ttest_expect_success 'should fail with a normal filename' '\n> +\t\tmkdir nonempty_dir &&\n> +\t\ttouch nonempty_dir/some_file &&\n> +\t\ttest_must_fail test_dir_is_empty nonempty_dir\n> +\t'\n> +\ttest_expect_success 'should fail with dot-newline-dot filename' '\n> +\t\tmkdir pathological_dir &&\n> +\t\tprintf \\\"pathological_dir/.\\\\\\\\n.\\\\\\\\0\\\" | xargs -0 touch &&\n> +\t\ttest_must_fail test_dir_is_empty pathological_dir\n> +\t'\n> +\ttest_done\n> +\tEOF\n> +\tcheck_sub_test_lib_test dir-empty <<-\\\\EOF\n> +\t> ok 1 - should pass with actually empty directory\n> +\t> ok 2 - should fail with a normal filename\n> +\t> ok 3 - should fail with dot-newline-dot filename\n> +\t> # passed all 3 test(s)\n> +\t> 1..3\n> +\tEOF\n> +\"\n> +\n> +\n>   ################################################################\n>   # Basics of the basics\n>   \n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 2b2181dca..f7ff28ef6 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -568,7 +568,7 @@ test_path_is_dir () {\n>   # Check if the directory exists and is empty as expected, barf otherwise.\n>   test_dir_is_empty () {\n>   \ttest_path_is_dir \"$1\" &&\n> -\tif test -n \"$(ls -a1 \"$1\" | egrep -v '^\\.\\.?$')\"\n> +\tif test \"$(ls -A1 \"$1\" | wc -c)\" != 0\n>   \tthen\n>   \t\techo \"Directory '$1' is not empty, it contains:\"\n>   \t\tls -la \"$1\"\n> \n\nDid you accidentally resend the same patch in this v2? I can't spot the \ndifference to v1.\n\n-- Hannes\n"},{"id":"354579","messageId":"20180805205231.GA14688@sigill.intra.peff.net","threadId":"49039","inReplyTo":"CAPig+cQSryjWrFZML_g0fR5oq6dzVaWPXm_4R-Jg_ay1R2DzbQ@mail.gmail.com","subject":"Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-05T20:52:31Z","receivedAt":"2018-08-05T20:52:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 05, 2018 at 01:23:05AM -0400, Eric Sunshine wrote:\n\n> A simpler approach, without the portability concerns of -A, would be\n> to remove the \".\" and \"..\" lines from the top of the listing:\n> \n>     ls -f1 \"$1\" | sed '1,2d'\n> \n> If we're worried about -f not being sufficiently portable, then an\n> even simpler approach would be to check whether the output of 'ls -a1'\n> has more lines than the two expected (\".\" and \"..\"):\n> \n>     test $(ls -a1 \"$1\" | wc -l) -gt 2\n> \n> I think I favor this final implementation over the others.\n\nPerhaps even simpler:\n\n  test \"$1\" = \"$(find \"$1\")\"\n\nThat will recurse any subdirectories, possibly wasting time, but since\nthe point is that we expect it to be empty, that's probably OK.\n\nLike Junio said elsewhere, I am not sure if it is even worth caring too\nmuch about pathological cases in the test suite, where we control all of\nthe paths. But using `find` does shave off one process invocation, too. :)\n\n-Peff\n"},{"id":"354599","messageId":"20180806130216.GA29966@sigill.intra.peff.net","threadId":"49039","inReplyTo":"20180805205231.GA14688@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-06T13:02:16Z","receivedAt":"2018-08-06T13:02:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 05, 2018 at 04:52:31PM -0400, Jeff King wrote:\n\n> On Sun, Aug 05, 2018 at 01:23:05AM -0400, Eric Sunshine wrote:\n> \n> > A simpler approach, without the portability concerns of -A, would be\n> > to remove the \".\" and \"..\" lines from the top of the listing:\n> > \n> >     ls -f1 \"$1\" | sed '1,2d'\n> > \n> > If we're worried about -f not being sufficiently portable, then an\n> > even simpler approach would be to check whether the output of 'ls -a1'\n> > has more lines than the two expected (\".\" and \"..\"):\n> > \n> >     test $(ls -a1 \"$1\" | wc -l) -gt 2\n> > \n> > I think I favor this final implementation over the others.\n> \n> Perhaps even simpler:\n> \n>   test \"$1\" = \"$(find \"$1\")\"\n\nActually, I guess it needs to add \"-print\", since IIRC that is not the\ndefault on some old versions of \"find\".\n\n-Peff\n"},{"id":"354661","messageId":"CAPig+cQK7JSosa0hNhgw7xoSui2f0m6yfRLWytsg_Zow3bN5bg@mail.gmail.com","threadId":"49039","inReplyTo":"20180806130216.GA29966@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-06T17:52:41Z","receivedAt":"2018-08-06T17:52:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 6, 2018 at 9:02 AM Jeff King <peff@peff.net> wrote:\n> On Sun, Aug 05, 2018 at 04:52:31PM -0400, Jeff King wrote:\n> > Perhaps even simpler:\n> >\n> >   test \"$1\" = \"$(find \"$1\")\"\n>\n> Actually, I guess it needs to add \"-print\", since IIRC that is not the\n> default on some old versions of \"find\".\n\nYou recall correctly.\n\nI tend to avoid 'find' in scripts when I don't need its recursive\nbehavior due to *extremely* poor performance on Windows (at least that\nwas the case years ago), especially when there are a lot of files in\nthe directory being listed. However, for this usage, we expect the\ndirectory to be empty, so perhaps performance isn't an issue.\n"},{"id":"355277","messageId":"CAFW+GMCfMVLPH4909W-x8HHVn8-8oX=Dk=0D7sz=cWdh0xKOPw@mail.gmail.com","threadId":"49039","inReplyTo":"CAPig+cQK7JSosa0hNhgw7xoSui2f0m6yfRLWytsg_Zow3bN5bg@mail.gmail.com","subject":"Re: [PATCH 1/1] t/test-lib: make `test_dir_is_empty` more robust","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-08-12T04:06:53Z","receivedAt":"2018-08-12T04:07:16Z","isPatch":true,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"> That will recurse any subdirectories, possibly wasting time, but since\n> the point is that we expect it to be empty, that's probably OK.\n\nOne caveat involves invocations of `test_must_fail test_dir_is_empty`,\nwherein we _don't_ actually expect the directory to be empty. It looks\nlike there might not be any such invocations in Git, though, for what\nit's worth.\n\nThe solution that counts the lines of `ls -a` seems like it should be\nuniversally portable, both in POSIX and in practice, and it doesn't\ncomplicate the code at all. (I'd say that it's slightly simpler than the\n`egrep` approach in the status quo.) I've adjusted the patch to use\nEric's version, which I like a bit more than mine; I'll send out the\nrevised commit presently.\n\nIt's true that within Git we can simply try to avoid creating\npathological file names. But Sharness is also exposed as a standalone\nlibrary, and it seems clear that the public function therein should be\ncorrect in all cases. Assuming that the new implementation is\nsufficiently inoffensive, is there any harm in being perhaps a bit more\nrobust than is practically needed, and also maintaining consistency with\nSharness?\n\nThanks for all the suggestions and feedback so far!\n\nBest,\nWC\n"},{"id":"355278","messageId":"20180812040620.15298-1-wchargin@gmail.com","threadId":"49039","inReplyTo":"CAPig+cQK7JSosa0hNhgw7xoSui2f0m6yfRLWytsg_Zow3bN5bg@mail.gmail.com","subject":"[PATCH v3] test_dir_is_empty: properly detect files with newline in name","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-08-12T04:06:20Z","receivedAt":"2018-08-12T04:07:55Z","isPatch":true,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"While the `test_dir_is_empty` function appears correct in most normal\nuse cases, it can fail when filenames contain newlines. This patch\nchanges the implementation to check that the output of `ls -a` has at\nmost two lines (for `.` and `..`), which should be better behaved.\n\nThe newly added unit test fails before this change and passes after it.\n\nSigned-off-by: William Chargin <wchargin@gmail.com>\n---\nThis patch depends on \"t: factor out FUNNYNAMES as shared lazy prereq\"\n(2018-08-06), available from `wc/make-funnynames-shared-lazy-prereq`.\nThe code will work on master, but the test will not run due to a missing\nprereq.\n\nI originally wrote this patch for the standalone Sharness library, but\nthat library advises that such patches be sent to the Git mailing list\nfirst.\n\n t/t0000-basic.sh        | 31 +++++++++++++++++++++++++++++++\n t/test-lib-functions.sh |  2 +-\n 2 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 34859fe4a..60134d7ab 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -821,6 +821,37 @@ test_expect_success 'tests clean up even on failures' \"\n \tEOF\n \"\n \n+test_expect_success FUNNYNAMES \\\n+\t'test_dir_is_empty behaves even in pathological cases' \"\n+\trun_sub_test_lib_test \\\n+\t\tdir-empty 'behavior of test_dir_is_empty' <<-\\\\EOF &&\n+\ttest_expect_success 'should pass with actually empty directory' '\n+\t\tmkdir empty_dir &&\n+\t\ttest_dir_is_empty empty_dir\n+\t'\n+\ttest_expect_success 'should fail with a normal filename' '\n+\t\tmkdir nonempty_dir &&\n+\t\ttouch nonempty_dir/some_file &&\n+\t\ttest_must_fail test_dir_is_empty nonempty_dir\n+\t'\n+\ttest_expect_success 'should fail with dot-newline-dot filename' '\n+\t\tmkdir pathological_dir &&\n+\t\ttouch \\\"pathological_dir/.\n+\t.\\\" &&\n+\t\ttest_must_fail test_dir_is_empty pathological_dir\n+\t'\n+\ttest_done\n+\tEOF\n+\tcheck_sub_test_lib_test dir-empty <<-\\\\EOF\n+\t> ok 1 - should pass with actually empty directory\n+\t> ok 2 - should fail with a normal filename\n+\t> ok 3 - should fail with dot-newline-dot filename\n+\t> # passed all 3 test(s)\n+\t> 1..3\n+\tEOF\n+\"\n+\n+\n ################################################################\n # Basics of the basics\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 2b2181dca..f0241992b 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -568,7 +568,7 @@ test_path_is_dir () {\n # Check if the directory exists and is empty as expected, barf otherwise.\n test_dir_is_empty () {\n \ttest_path_is_dir \"$1\" &&\n-\tif test -n \"$(ls -a1 \"$1\" | egrep -v '^\\.\\.?$')\"\n+\tif test \"$(ls -a1 \"$1\" | wc -l)\" -gt 2\n \tthen\n \t\techo \"Directory '$1' is not empty, it contains:\"\n \t\tls -la \"$1\"\n-- \n2.18.0.548.g101af7bd4\n\n"},{"id":"355288","messageId":"CAPig+cTca5YsXeXYbEEuq8Y_GraUYWwzdKO7r+PR9=JRTR_xFA@mail.gmail.com","threadId":"49039","inReplyTo":"20180812040620.15298-1-wchargin@gmail.com","subject":"Re: [PATCH v3] test_dir_is_empty: properly detect files with newline in name","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-12T06:17:52Z","receivedAt":"2018-08-12T06:18:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 12, 2018 at 12:07 AM William Chargin <wchargin@gmail.com> wrote:\n> While the `test_dir_is_empty` function appears correct in most normal\n> use cases, it can fail when filenames contain newlines. This patch\n> changes the implementation to check that the output of `ls -a` has at\n> most two lines (for `.` and `..`), which should be better behaved.\n>\n> The newly added unit test fails before this change and passes after it.\n>\n> Signed-off-by: William Chargin <wchargin@gmail.com>\n> ---\n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> @@ -821,6 +821,37 @@ test_expect_success 'tests clean up even on failures' \"\n> +test_expect_success FUNNYNAMES \\\n> +       'test_dir_is_empty behaves even in pathological cases' \"\n> +       test_expect_success 'should fail with a normal filename' '\n> +               mkdir nonempty_dir &&\n> +               touch nonempty_dir/some_file &&\n\nWe usually avoid \"touch\" unless the timestamp of the file is\nsignificant. In this case, it isn't, so it would be more idiomatic to\nsay simply:\n\n    >nonempty_dir/some_file &&\n\n> +               test_must_fail test_dir_is_empty nonempty_dir\n\nThis is an abuse of test_must_fail() which is intended strictly for\ntesting 'git' invocations which might fail for reasons other than the\nexpected one (for instance, git might crash). Instead, you should just\nuse '!', like this:\n\n    ! test_dir_is_empty nonempty_dir\n\n> +       test_expect_success 'should fail with dot-newline-dot filename' '\n> +               mkdir pathological_dir &&\n> +               touch \\\"pathological_dir/.\n> +       .\\\" &&\n> +               test_must_fail test_dir_is_empty pathological_dir\n\nSame comments as above.\n"},{"id":"355289","messageId":"CAFW+GMDu-FVFXZeB=fV5a+HyGXea-UoMpZS90T9b2G_=xd3iLw@mail.gmail.com","threadId":"49039","inReplyTo":"CAPig+cTca5YsXeXYbEEuq8Y_GraUYWwzdKO7r+PR9=JRTR_xFA@mail.gmail.com","subject":"Re: [PATCH v3] test_dir_is_empty: properly detect files with newline in name","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-08-12T06:32:37Z","receivedAt":"2018-08-12T06:33:21Z","isPatch":true,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"Thanks for the review.\n\n> We usually avoid \"touch\" unless the timestamp of the file is\n> significant.\n\nMakes sense. Will change as you suggest.\n\n> This is an abuse of test_must_fail() which is intended strictly for\n> testing 'git' invocations which might fail for reasons other than the\n> expected one (for instance, git might crash).\n\nInteresting. I didn't infer this from the docs on `test_must_fail` in\n`t/test-lib-functions.sh`. Sharness, which is supposed to be independent\nof Git, explicitly says to use `test_must_fail` instead of `!`.\n(Admittedly, the implementations are different, but only slightly:\nwithin Git, a Valgrind error 126 is a failure, not success.)\n\nI also see other uses of `test_must_fail` throughout the codebase: e.g.,\nwith `kill`, `test`, `test_cmp`, `run_sub_test_lib_test`, etc. as the\ntarget command. Are these invocations in error?\n\n(I'm nevertheless happy to change this as you suggest.)\n\nI'll make these changes locally and hold on to the patch, waiting for\nothers' input.\n\nBest,\nWC\n"},{"id":"355290","messageId":"CAPig+cQSg-t6KGj3s0LJi+FU7LSQMNTmSHhMJJH=PgMUU9GWOA@mail.gmail.com","threadId":"49039","inReplyTo":"CAFW+GMDu-FVFXZeB=fV5a+HyGXea-UoMpZS90T9b2G_=xd3iLw@mail.gmail.com","subject":"Re: [PATCH v3] test_dir_is_empty: properly detect files with newline in name","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-12T06:44:30Z","receivedAt":"2018-08-12T06:44:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 12, 2018 at 2:33 AM William Chargin <wchargin@gmail.com> wrote:\n> > This is an abuse of test_must_fail() which is intended strictly for\n> > testing 'git' invocations which might fail for reasons other than the\n> > expected one (for instance, git might crash).\n>\n> Interesting. I didn't infer this from the docs on `test_must_fail` in\n> `t/test-lib-functions.sh`. Sharness, which is supposed to be independent\n> of Git, explicitly says to use `test_must_fail` instead of `!`.\n\nThis sort of knowledge is, perhaps unfortunately, spread around in too\nmany places. In this case, it's mentioned in t/README. The relevant\nexcerpt:\n\n    Don't use '! git cmd' when you want to make sure the git command\n    exits with failure in a controlled way by calling \"die()\".\n    Instead, use 'test_must_fail git cmd'.  This will signal a failure\n    if git dies in an unexpected way (e.g. segfault).\n\n    On the other hand, don't use test_must_fail for running regular\n    platform commands; just use '! cmd'.  We are not in the business\n    of verifying that the world given to us sanely works.\n\nIt probably wouldn't hurt to update the comment block above\ntest_must_fail() in t/test-functions-lib.sh.\n\n> I also see other uses of `test_must_fail` throughout the codebase: e.g.,\n> with `kill`, `test`, `test_cmp`, `run_sub_test_lib_test`, etc. as the\n> target command. Are these invocations in error?\n\nLooks that way, even run_sub_test_lib_test().\n"},{"id":"357991","messageId":"20180912183534.7486-1-wchargin@gmail.com","threadId":"49039","inReplyTo":"CAPig+cQSg-t6KGj3s0LJi+FU7LSQMNTmSHhMJJH=PgMUU9GWOA@mail.gmail.com","subject":"[PATCH v4] test_dir_is_empty: fix edge cases with newlines and hyphens","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-09-12T18:35:34Z","receivedAt":"2018-09-12T18:36:21Z","isPatch":true,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"While the `test_dir_is_empty` function appears correct in most normal\nuse cases, it can improperly pass if a directory contains a filename\nwith a newline, and can improperly fail if an empty directory looks like\nan argument to `ls`. This patch changes the implementation to check that\nthe output of `ls -a` has at most two lines (for `.` and `..`), which\nshould be better behaved, and adds the `--` delimiter before the\ndirectory name when invoking `ls`.\n\nThe newly added unit test fails before this change and passes after it.\n\nSigned-off-by: William Chargin <wchargin@gmail.com>\n---\nThis patch depends on \"t: factor out FUNNYNAMES as shared lazy prereq\"\n(2018-08-06), which is now in master.\n\nI originally wrote this patch for the standalone Sharness library, but\nthat library advises that such patches be sent to the Git mailing list\nfirst.\n\nTested on GNU/Linux (Mint 18.2) and macOS (10.13).\n\n t/t0000-basic.sh        | 43 +++++++++++++++++++++++++++++++++++++++++\n t/test-lib-functions.sh |  2 +-\n 2 files changed, 44 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 850f651e4e..a5c57c6aa5 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -821,6 +821,49 @@ test_expect_success 'tests clean up even on failures' \"\n \tEOF\n \"\n \n+test_expect_success FUNNYNAMES \\\n+\t'test_dir_is_empty behaves even in pathological cases' \"\n+\trun_sub_test_lib_test \\\n+\t\tdir-empty 'behavior of test_dir_is_empty' <<-\\\\EOF &&\n+\ttest_expect_success 'should pass with actually empty directory' '\n+\t\tmkdir empty_dir &&\n+\t\ttest_dir_is_empty empty_dir\n+\t'\n+\ttest_expect_success 'should fail with a normal filename' '\n+\t\tmkdir nonempty_dir &&\n+\t\t>nonempty_dir/some_file &&\n+\t\t! test_dir_is_empty nonempty_dir\n+\t'\n+\ttest_expect_success 'should fail with dot-newline-dot filename' '\n+\t\tmkdir pathological_dir &&\n+\t\t>\\\"pathological_dir/.\n+\t.\\\" &&\n+\t\t! test_dir_is_empty pathological_dir\n+\t'\n+\ttest_expect_success 'should pass with an empty directory \\\"-l\\\"' '\n+\t\tmkdir -- -l &&\n+\t\ttest_dir_is_empty -l &&\n+\t\trmdir -- -l\n+\t'\n+\ttest_expect_success 'should pass with an empty directory \\\"--wat\\\"' '\n+\t\tmkdir -- --wat &&\n+\t\ttest_dir_is_empty --wat &&\n+\t\trmdir -- --wat\n+\t'\n+\ttest_done\n+\tEOF\n+\tcheck_sub_test_lib_test dir-empty <<-\\\\EOF\n+\t> ok 1 - should pass with actually empty directory\n+\t> ok 2 - should fail with a normal filename\n+\t> ok 3 - should fail with dot-newline-dot filename\n+\t> ok 4 - should pass with an empty directory \\\"-l\\\"\n+\t> ok 5 - should pass with an empty directory \\\"--wat\\\"\n+\t> # passed all 5 test(s)\n+\t> 1..5\n+\tEOF\n+\"\n+\n+\n ################################################################\n # Basics of the basics\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 4207af4077..3df6b8027f 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -576,7 +576,7 @@ test_path_exists () {\n # Check if the directory exists and is empty as expected, barf otherwise.\n test_dir_is_empty () {\n \ttest_path_is_dir \"$1\" &&\n-\tif test -n \"$(ls -a1 \"$1\" | egrep -v '^\\.\\.?$')\"\n+\tif test \"$(ls -a1 -- \"$1\" | wc -l)\" -gt 2\n \tthen\n \t\techo \"Directory '$1' is not empty, it contains:\"\n \t\tls -la \"$1\"\n-- \n2.18.0.549.gd66323a05\n\n"},{"id":"357993","messageId":"20180912183755.7875-1-wchargin@gmail.com","threadId":"49039","inReplyTo":"CAPig+cQSg-t6KGj3s0LJi+FU7LSQMNTmSHhMJJH=PgMUU9GWOA@mail.gmail.com","subject":"[PATCH v4] test_dir_is_empty: fix edge cases with newlines and hyphens","fromName":"William Chargin","fromEmail":"wchargin@gmail.com","sentAt":"2018-09-12T18:37:55Z","receivedAt":"2018-09-12T18:38:03Z","isPatch":true,"sender":{"key":"wchargin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/4317806?v=4"},"body":"While the `test_dir_is_empty` function appears correct in most normal\nuse cases, it can improperly pass if a directory contains a filename\nwith a newline, and can improperly fail if an empty directory looks like\nan argument to `ls`. This patch changes the implementation to check that\nthe output of `ls -a` has at most two lines (for `.` and `..`), which\nshould be better behaved, and adds the `--` delimiter before the\ndirectory name when invoking `ls`.\n\nThe newly added unit test fails before this change and passes after it.\n\nSigned-off-by: William Chargin <wchargin@gmail.com>\n---\nThis patch depends on \"t: factor out FUNNYNAMES as shared lazy prereq\"\n(2018-08-06), which is now in master.\n\nI originally wrote this patch for the standalone Sharness library, but\nthat library advises that such patches be sent to the Git mailing list\nfirst.\n\nTested on GNU/Linux (Mint 18.2) and macOS (10.13).\n\n t/t0000-basic.sh        | 43 +++++++++++++++++++++++++++++++++++++++++\n t/test-lib-functions.sh |  2 +-\n 2 files changed, 44 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 850f651e4e..a5c57c6aa5 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -821,6 +821,49 @@ test_expect_success 'tests clean up even on failures' \"\n \tEOF\n \"\n \n+test_expect_success FUNNYNAMES \\\n+\t'test_dir_is_empty behaves even in pathological cases' \"\n+\trun_sub_test_lib_test \\\n+\t\tdir-empty 'behavior of test_dir_is_empty' <<-\\\\EOF &&\n+\ttest_expect_success 'should pass with actually empty directory' '\n+\t\tmkdir empty_dir &&\n+\t\ttest_dir_is_empty empty_dir\n+\t'\n+\ttest_expect_success 'should fail with a normal filename' '\n+\t\tmkdir nonempty_dir &&\n+\t\t>nonempty_dir/some_file &&\n+\t\t! test_dir_is_empty nonempty_dir\n+\t'\n+\ttest_expect_success 'should fail with dot-newline-dot filename' '\n+\t\tmkdir pathological_dir &&\n+\t\t>\\\"pathological_dir/.\n+\t.\\\" &&\n+\t\t! test_dir_is_empty pathological_dir\n+\t'\n+\ttest_expect_success 'should pass with an empty directory \\\"-l\\\"' '\n+\t\tmkdir -- -l &&\n+\t\ttest_dir_is_empty -l &&\n+\t\trmdir -- -l\n+\t'\n+\ttest_expect_success 'should pass with an empty directory \\\"--wat\\\"' '\n+\t\tmkdir -- --wat &&\n+\t\ttest_dir_is_empty --wat &&\n+\t\trmdir -- --wat\n+\t'\n+\ttest_done\n+\tEOF\n+\tcheck_sub_test_lib_test dir-empty <<-\\\\EOF\n+\t> ok 1 - should pass with actually empty directory\n+\t> ok 2 - should fail with a normal filename\n+\t> ok 3 - should fail with dot-newline-dot filename\n+\t> ok 4 - should pass with an empty directory \\\"-l\\\"\n+\t> ok 5 - should pass with an empty directory \\\"--wat\\\"\n+\t> # passed all 5 test(s)\n+\t> 1..5\n+\tEOF\n+\"\n+\n+\n ################################################################\n # Basics of the basics\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 4207af4077..3df6b8027f 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -576,7 +576,7 @@ test_path_exists () {\n # Check if the directory exists and is empty as expected, barf otherwise.\n test_dir_is_empty () {\n \ttest_path_is_dir \"$1\" &&\n-\tif test -n \"$(ls -a1 \"$1\" | egrep -v '^\\.\\.?$')\"\n+\tif test \"$(ls -a1 -- \"$1\" | wc -l)\" -gt 2\n \tthen\n \t\techo \"Directory '$1' is not empty, it contains:\"\n \t\tls -la \"$1\"\n-- \n2.18.0.549.gd66323a05\n\n"},{"id":"358003","messageId":"xmqq7ejqlclg.fsf@gitster-ct.c.googlers.com","threadId":"49039","inReplyTo":"20180912183534.7486-1-wchargin@gmail.com","subject":"Re: [PATCH v4] test_dir_is_empty: fix edge cases with newlines and hyphens","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-12T19:50:19Z","receivedAt":"2018-09-12T19:50:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"William Chargin <wchargin@gmail.com> writes:\n\n> While the `test_dir_is_empty` function appears correct in most normal\n> use cases, it can improperly pass if a directory contains a filename\n> with a newline, and can improperly fail if an empty directory looks like\n> an argument to `ls`. This patch changes the implementation to check that\n> the output of `ls -a` has at most two lines (for `.` and `..`), which\n> should be better behaved, and adds the `--` delimiter before the\n> directory name when invoking `ls`.\n\nAFIAK dot and dot-dot are allowed not to exist; \"at most two\" is not\na good test.\n\nQuite honestly, our tests are still run inside a sort-of controlled\nenvironment, so if it _requires_ use of things we have avoided\ndepending on, like \"ls -A\" and \"xargs -0\", or the fact that most\nfilesystems always have \".\" and \"..\" even in an empty directory, in\norder to be resistant to funnily-named files like dot-LF-dot, I\nwould say it is not worth worrying about these funny names--instead\nwe can simply refrain from using such a pathological name, can't we?\n\nIn other words, is there a real-world need in the context of our\ntest suite for this change?\n\nAlso, I find that its support for directories whose names begin with\na dash red-herring.  All the test scripts in our test suite knows that\nthey can prefix \"./\" to avoid problems, i.e.\n\n\ttest_dir_is_empty ./--wat\n\nSo it appears that the only problematic case is when we create a\ndirectory, create a file or a directory whose name is dot-LF-dot and\nnothing else, and then do something that ought to cause that file to\ndisappear, and make sure that the directory is empty, e.g.\n\n\tmkdir empty &&\n\techo foo >\"empty/$dotLFdot\" &&\n\tgit add \"empty/$dotLFdot\" &&\n\tgit reset --hard &&\n\ttest_dir_is_empty empty\n\nWe do want to make sure funny names can be added with \"git add\" and\n\"git reset --hard\" to HEAD that lacked those paths with funny names\nto remove them correctly.  But the funny names used in such a test\ndo not have to be $dotLFdot; you can use \"${dotLFdot}X\" instead in\nthe above and can ensure whatever the original test wanted to\nensure.\n\nSo...\n\n\n\n"}]}