{"thread":{"id":"52488","subject":"[PATCH] sparse-checkout: improve OS ls compatibility","startedAt":"2019-12-19T01:58:51Z","lastAt":"2019-12-20T19:41:18Z","messageCount":24,"participants":["Ed Maste","Derrick Stolee","Eric Wong","Junio C Hamano","Denton Liu","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"388493","messageId":"20191219015833.49314-1-emaste@FreeBSD.org","threadId":"52488","inReplyTo":null,"subject":"[PATCH] sparse-checkout: improve OS ls compatibility","fromName":"Ed Maste","fromEmail":"emaste@freebsd.org","sentAt":"2019-12-19T01:58:33Z","receivedAt":"2019-12-19T01:58:51Z","isPatch":true,"sender":{"key":"emaste@freebsd.org","avatar":"https://avatars.githubusercontent.com/u/1034582?v=4"},"body":"On FreeBSD, when executed by root ls enables the '-A' option:\n\n  -A  Include directory entries whose names begin with a dot (`.')\n      except for . and ...  Automatically set for the super-user unless\n      -I is specified.\n\nPipe ls's output to grep -v .git to remove the undesired entry.  Also\npass the -1 option to ensure one entry per line.\n\nSigned-off-by: Ed Maste <emaste@FreeBSD.org>\n---\nThere are several different ways this could be solved; this approach\nfelt cleanest to me, but there are at least two other reasonable\nalternatives:\n\n  * Add -a to the invocations and .git to the expected output\n\n  * Add LSFLAGS and set it to -I on BSDs, to turn off the special dot\n    behaviour\n\nI'll submit a new patch if a different approach is preferred.\n\n t/t1091-sparse-checkout-builtin.sh | 33 +++++++++++++++++-------------\n 1 file changed, 19 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex cee98a1c8a..3a3eafa653 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -4,6 +4,11 @@ test_description='sparse checkout builtin tests'\n \n . ./test-lib.sh\n \n+ls_no_git()\n+{\n+\tls -1 \"$1\" | grep -v .git\n+}\n+\n test_expect_success 'setup' '\n \tgit init repo &&\n \t(\n@@ -50,7 +55,7 @@ test_expect_success 'git sparse-checkout init' '\n \tEOF\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n \ttest_cmp_config -C repo true core.sparsecheckout &&\n-\tls repo >dir  &&\n+\tls_no_git repo >dir  &&\n \techo a >expect &&\n \ttest_cmp expect dir\n '\n@@ -73,7 +78,7 @@ test_expect_success 'init with existing sparse-checkout' '\n \t\t*folder*\n \tEOF\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n-\tls repo >dir  &&\n+\tls_no_git repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n@@ -90,7 +95,7 @@ test_expect_success 'clone --sparse' '\n \t\t!/*/\n \tEOF\n \ttest_cmp expect actual &&\n-\tls clone >dir &&\n+\tls_no_git clone >dir &&\n \techo a >expect &&\n \ttest_cmp expect dir\n '\n@@ -119,7 +124,7 @@ test_expect_success 'set sparse-checkout using builtin' '\n \tgit -C repo sparse-checkout list >actual &&\n \ttest_cmp expect actual &&\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n-\tls repo >dir  &&\n+\tls_no_git repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n@@ -139,7 +144,7 @@ test_expect_success 'set sparse-checkout using --stdin' '\n \tgit -C repo sparse-checkout list >actual &&\n \ttest_cmp expect actual &&\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n-\tls repo >dir  &&\n+\tls_no_git repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n@@ -154,7 +159,7 @@ test_expect_success 'cone mode: match patterns' '\n \tgit -C repo read-tree -mu HEAD 2>err &&\n \ttest_i18ngrep ! \"disabling cone patterns\" err &&\n \tgit -C repo reset --hard &&\n-\tls repo >dir  &&\n+\tls_no_git repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n@@ -177,7 +182,7 @@ test_expect_success 'sparse-checkout disable' '\n \ttest_path_is_file repo/.git/info/sparse-checkout &&\n \tgit -C repo config --list >config &&\n \ttest_must_fail git config core.sparseCheckout &&\n-\tls repo >dir &&\n+\tls_no_git repo >dir &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeep\n@@ -191,24 +196,24 @@ test_expect_success 'cone mode: init and set' '\n \tgit -C repo sparse-checkout init --cone &&\n \tgit -C repo config --list >config &&\n \ttest_i18ngrep \"core.sparsecheckoutcone=true\" config &&\n-\tls repo >dir  &&\n+\tls_no_git repo >dir  &&\n \techo a >expect &&\n \ttest_cmp expect dir &&\n \tgit -C repo sparse-checkout set deep/deeper1/deepest/ 2>err &&\n \ttest_must_be_empty err &&\n-\tls repo >dir  &&\n+\tls_no_git repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeep\n \tEOF\n \ttest_cmp expect dir &&\n-\tls repo/deep >dir  &&\n+\tls_no_git repo/deep >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeeper1\n \tEOF\n \ttest_cmp expect dir &&\n-\tls repo/deep/deeper1 >dir  &&\n+\tls_no_git repo/deep/deeper1 >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeepest\n@@ -234,7 +239,7 @@ test_expect_success 'cone mode: init and set' '\n \t\tfolder1\n \t\tfolder2\n \tEOF\n-\tls repo >dir &&\n+\tls_no_git repo >dir &&\n \ttest_cmp expect dir\n '\n \n@@ -256,7 +261,7 @@ test_expect_success 'revert to old sparse-checkout on bad update' '\n \ttest_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&\n \ttest_i18ngrep \"cannot set sparse-checkout patterns\" err &&\n \ttest_cmp repo/.git/info/sparse-checkout expect &&\n-\tls repo/deep >dir &&\n+\tls_no_git repo/deep >dir &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeeper1\n@@ -313,7 +318,7 @@ test_expect_success 'cone mode: set with core.ignoreCase=true' '\n \t\t/folder1/\n \tEOF\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n-\tls repo >dir &&\n+\tls_no_git repo >dir &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n-- \n2.24.0\n\n"},{"id":"388494","messageId":"46d9f9dd-b278-bade-af48-3a3bd2e4aa5e@gmail.com","threadId":"52488","inReplyTo":"20191219015833.49314-1-emaste@FreeBSD.org","subject":"Re: [PATCH] sparse-checkout: improve OS ls compatibility","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-12-19T02:07:13Z","receivedAt":"2019-12-19T02:07:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/18/2019 8:58 PM, Ed Maste wrote:\n\nThanks for the report!\n\nIt was a little unclear from the get-go what exactly the issue is.\n\n> On FreeBSD, when executed by root ls enables the '-A' option:\n> \n>   -A  Include directory entries whose names begin with a dot (`.')\n>       except for . and ...  Automatically set for the super-user unless\n>       -I is specified.\n\nIt appears that the \"ls\" commands in the sparse-checkout tests are\nreporting the \".git\" directory when executed on FreeBSD as root. Is this\nonly as root?\n\n> Pipe ls's output to grep -v .git to remove the undesired entry.  Also\n> pass the -1 option to ensure one entry per line.\n\nWhat if we instead ran \"ls -a\" and added .git to our expected output\n(when appropriate)? Would that be simpler (and reduce the process\ncount that this solution introduces).\n\nThanks,\n-Stolee\n \n> Signed-off-by: Ed Maste <emaste@FreeBSD.org>\n> ---\n> There are several different ways this could be solved; this approach\n> felt cleanest to me, but there are at least two other reasonable\n> alternatives:\n> \n>   * Add -a to the invocations and .git to the expected output\n> \n>   * Add LSFLAGS and set it to -I on BSDs, to turn off the special dot\n>     behaviour\n> \n> I'll submit a new patch if a different approach is preferred.\n> \n>  t/t1091-sparse-checkout-builtin.sh | 33 +++++++++++++++++-------------\n>  1 file changed, 19 insertions(+), 14 deletions(-)\n> \n> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n> index cee98a1c8a..3a3eafa653 100755\n> --- a/t/t1091-sparse-checkout-builtin.sh\n> +++ b/t/t1091-sparse-checkout-builtin.sh\n> @@ -4,6 +4,11 @@ test_description='sparse checkout builtin tests'\n>  \n>  . ./test-lib.sh\n>  \n> +ls_no_git()\n> +{\n> +\tls -1 \"$1\" | grep -v .git\n> +}\n> +\n>  test_expect_success 'setup' '\n>  \tgit init repo &&\n>  \t(\n> @@ -50,7 +55,7 @@ test_expect_success 'git sparse-checkout init' '\n>  \tEOF\n>  \ttest_cmp expect repo/.git/info/sparse-checkout &&\n>  \ttest_cmp_config -C repo true core.sparsecheckout &&\n> -\tls repo >dir  &&\n> +\tls_no_git repo >dir  &&\n>  \techo a >expect &&\n>  \ttest_cmp expect dir\n>  '\n> @@ -73,7 +78,7 @@ test_expect_success 'init with existing sparse-checkout' '\n>  \t\t*folder*\n>  \tEOF\n>  \ttest_cmp expect repo/.git/info/sparse-checkout &&\n> -\tls repo >dir  &&\n> +\tls_no_git repo >dir  &&\n>  \tcat >expect <<-EOF &&\n>  \t\ta\n>  \t\tfolder1\n> @@ -90,7 +95,7 @@ test_expect_success 'clone --sparse' '\n>  \t\t!/*/\n>  \tEOF\n>  \ttest_cmp expect actual &&\n> -\tls clone >dir &&\n> +\tls_no_git clone >dir &&\n>  \techo a >expect &&\n>  \ttest_cmp expect dir\n>  '\n> @@ -119,7 +124,7 @@ test_expect_success 'set sparse-checkout using builtin' '\n>  \tgit -C repo sparse-checkout list >actual &&\n>  \ttest_cmp expect actual &&\n>  \ttest_cmp expect repo/.git/info/sparse-checkout &&\n> -\tls repo >dir  &&\n> +\tls_no_git repo >dir  &&\n>  \tcat >expect <<-EOF &&\n>  \t\ta\n>  \t\tfolder1\n> @@ -139,7 +144,7 @@ test_expect_success 'set sparse-checkout using --stdin' '\n>  \tgit -C repo sparse-checkout list >actual &&\n>  \ttest_cmp expect actual &&\n>  \ttest_cmp expect repo/.git/info/sparse-checkout &&\n> -\tls repo >dir  &&\n> +\tls_no_git repo >dir  &&\n>  \tcat >expect <<-EOF &&\n>  \t\ta\n>  \t\tfolder1\n> @@ -154,7 +159,7 @@ test_expect_success 'cone mode: match patterns' '\n>  \tgit -C repo read-tree -mu HEAD 2>err &&\n>  \ttest_i18ngrep ! \"disabling cone patterns\" err &&\n>  \tgit -C repo reset --hard &&\n> -\tls repo >dir  &&\n> +\tls_no_git repo >dir  &&\n>  \tcat >expect <<-EOF &&\n>  \t\ta\n>  \t\tfolder1\n> @@ -177,7 +182,7 @@ test_expect_success 'sparse-checkout disable' '\n>  \ttest_path_is_file repo/.git/info/sparse-checkout &&\n>  \tgit -C repo config --list >config &&\n>  \ttest_must_fail git config core.sparseCheckout &&\n> -\tls repo >dir &&\n> +\tls_no_git repo >dir &&\n>  \tcat >expect <<-EOF &&\n>  \t\ta\n>  \t\tdeep\n> @@ -191,24 +196,24 @@ test_expect_success 'cone mode: init and set' '\n>  \tgit -C repo sparse-checkout init --cone &&\n>  \tgit -C repo config --list >config &&\n>  \ttest_i18ngrep \"core.sparsecheckoutcone=true\" config &&\n> -\tls repo >dir  &&\n> +\tls_no_git repo >dir  &&\n>  \techo a >expect &&\n>  \ttest_cmp expect dir &&\n>  \tgit -C repo sparse-checkout set deep/deeper1/deepest/ 2>err &&\n>  \ttest_must_be_empty err &&\n> -\tls repo >dir  &&\n> +\tls_no_git repo >dir  &&\n>  \tcat >expect <<-EOF &&\n>  \t\ta\n>  \t\tdeep\n>  \tEOF\n>  \ttest_cmp expect dir &&\n> -\tls repo/deep >dir  &&\n> +\tls_no_git repo/deep >dir  &&\n>  \tcat >expect <<-EOF &&\n>  \t\ta\n>  \t\tdeeper1\n>  \tEOF\n>  \ttest_cmp expect dir &&\n> -\tls repo/deep/deeper1 >dir  &&\n> +\tls_no_git repo/deep/deeper1 >dir  &&\n>  \tcat >expect <<-EOF &&\n>  \t\ta\n>  \t\tdeepest\n> @@ -234,7 +239,7 @@ test_expect_success 'cone mode: init and set' '\n>  \t\tfolder1\n>  \t\tfolder2\n>  \tEOF\n> -\tls repo >dir &&\n> +\tls_no_git repo >dir &&\n>  \ttest_cmp expect dir\n>  '\n>  \n> @@ -256,7 +261,7 @@ test_expect_success 'revert to old sparse-checkout on bad update' '\n>  \ttest_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&\n>  \ttest_i18ngrep \"cannot set sparse-checkout patterns\" err &&\n>  \ttest_cmp repo/.git/info/sparse-checkout expect &&\n> -\tls repo/deep >dir &&\n> +\tls_no_git repo/deep >dir &&\n>  \tcat >expect <<-EOF &&\n>  \t\ta\n>  \t\tdeeper1\n> @@ -313,7 +318,7 @@ test_expect_success 'cone mode: set with core.ignoreCase=true' '\n>  \t\t/folder1/\n>  \tEOF\n>  \ttest_cmp expect repo/.git/info/sparse-checkout &&\n> -\tls repo >dir &&\n> +\tls_no_git repo >dir &&\n>  \tcat >expect <<-EOF &&\n>  \t\ta\n>  \t\tfolder1\n> \n\n"},{"id":"388495","messageId":"CAPyFy2BROa9iMWBWf1hioYDaoEXPvyUNGHOZaZiD0TzVVhEtoA@mail.gmail.com","threadId":"52488","inReplyTo":"46d9f9dd-b278-bade-af48-3a3bd2e4aa5e@gmail.com","subject":"Re: [PATCH] sparse-checkout: improve OS ls compatibility","fromName":"Ed Maste","fromEmail":"emaste@freebsd.org","sentAt":"2019-12-19T02:18:13Z","receivedAt":"2019-12-19T02:18:26Z","isPatch":true,"sender":{"key":"emaste@freebsd.org","avatar":"https://avatars.githubusercontent.com/u/1034582?v=4"},"body":"On Wed, 18 Dec 2019 at 21:07, Derrick Stolee <stolee@gmail.com> wrote:\n>\n> On 12/18/2019 8:58 PM, Ed Maste wrote:\n>\n> Thanks for the report!\n>\n> It was a little unclear from the get-go what exactly the issue is.\n>\n> > On FreeBSD, when executed by root ls enables the '-A' option:\n> >\n> >   -A  Include directory entries whose names begin with a dot (`.')\n> >       except for . and ...  Automatically set for the super-user unless\n> >       -I is specified.\n>\n> It appears that the \"ls\" commands in the sparse-checkout tests are\n> reporting the \".git\" directory when executed on FreeBSD as root. Is this\n> only as root?\n\nYes, this is only as root - it seems Cirrus-CI invokes the build and\ntest scripts as root, which is why I had trouble reproducing it\nlocally.\n\n> > Pipe ls's output to grep -v .git to remove the undesired entry.  Also\n> > pass the -1 option to ensure one entry per line.\n>\n> What if we instead ran \"ls -a\" and added .git to our expected output\n> (when appropriate)? Would that be simpler (and reduce the process\n> count that this solution introduces).\n\nI originally tried that approach and thought it was a bit cumbersome,\nbut avoiding additional process invocations is a good argument. I'll\nsend a v2 with that change instead.\n"},{"id":"388496","messageId":"2653429f-46ac-c67d-cb08-8cc8695d77ae@gmail.com","threadId":"52488","inReplyTo":"CAPyFy2BROa9iMWBWf1hioYDaoEXPvyUNGHOZaZiD0TzVVhEtoA@mail.gmail.com","subject":"Re: [PATCH] sparse-checkout: improve OS ls compatibility","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-12-19T02:22:39Z","receivedAt":"2019-12-19T02:22:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/18/2019 9:18 PM, Ed Maste wrote:\n> On Wed, 18 Dec 2019 at 21:07, Derrick Stolee <stolee@gmail.com> wrote:\n>>\n>> On 12/18/2019 8:58 PM, Ed Maste wrote:\n>>\n>> Thanks for the report!\n>>\n>> It was a little unclear from the get-go what exactly the issue is.\n>>\n>>> On FreeBSD, when executed by root ls enables the '-A' option:\n>>>\n>>>   -A  Include directory entries whose names begin with a dot (`.')\n>>>       except for . and ...  Automatically set for the super-user unless\n>>>       -I is specified.\n>>\n>> It appears that the \"ls\" commands in the sparse-checkout tests are\n>> reporting the \".git\" directory when executed on FreeBSD as root. Is this\n>> only as root?\n> \n> Yes, this is only as root - it seems Cirrus-CI invokes the build and\n> test scripts as root, which is why I had trouble reproducing it\n> locally.\n> \n>>> Pipe ls's output to grep -v .git to remove the undesired entry.  Also\n>>> pass the -1 option to ensure one entry per line.\n>>\n>> What if we instead ran \"ls -a\" and added .git to our expected output\n>> (when appropriate)? Would that be simpler (and reduce the process\n>> count that this solution introduces).\n> \n> I originally tried that approach and thought it was a bit cumbersome,\n> but avoiding additional process invocations is a good argument. I'll\n> send a v2 with that change instead.\n\nI guess you are right that having \".\" and \"..\" appear is a bit silly.\nPerhaps your approach is cleaner, and the extra processes are not too\nmuch of a cost.\n\nLet's hold off on the v2 for a bit in case someone has a better idea.\n\nThanks,\n-Stolee\n"},{"id":"388498","messageId":"20191219024518.GA3411@dcvr","threadId":"52488","inReplyTo":"20191219015833.49314-1-emaste@FreeBSD.org","subject":"Re: [PATCH] sparse-checkout: improve OS ls compatibility","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-12-19T02:45:18Z","receivedAt":"2019-12-19T02:45:20Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Ed Maste <emaste@FreeBSD.org> wrote:\n> There are several different ways this could be solved; this approach\n> felt cleanest to me, but there are at least two other reasonable\n> alternatives:\n> \n>   * Add -a to the invocations and .git to the expected output\n> \n>   * Add LSFLAGS and set it to -I on BSDs, to turn off the special dot\n>     behaviour\n> \n> I'll submit a new patch if a different approach is preferred.\n\nRelying on \"ls\" itself seems a bit fragile.\nMy first choice is to write more tests in Perl5 :)\n\nBut using a shell for loop seems doable, here, since there\ndoesn't seem to be wonky characters.  I've done this in the past\nwhen I had to fix a system without \"ls\".\n\nThis goes on top of your patch:\n\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex 3a3eafa653..a431d05643 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -6,7 +6,7 @@ test_description='sparse checkout builtin tests'\n \n ls_no_git()\n {\n-\tls -1 \"$1\" | grep -v .git\n+\t( cd \"$1\" && for i in *; do echo \"$i\"; done )\n }\n \n test_expect_success 'setup' '\n\n"},{"id":"388513","messageId":"c4fef89a-2275-b4bc-b5c2-7bc647cd9bf6@gmail.com","threadId":"52488","inReplyTo":"20191219024518.GA3411@dcvr","subject":"Re: [PATCH] sparse-checkout: improve OS ls compatibility","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-12-19T13:56:44Z","receivedAt":"2019-12-19T13:56:47Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/18/2019 9:45 PM, Eric Wong wrote:\n> This goes on top of your patch:\n...\n> +\t( cd \"$1\" && for i in *; do echo \"$i\"; done )\n\nCould we drop the \"cd\" and \"echo\" processes with this line instead?\n\n\tfor i in \"$1\"/*; do printf \"$i\\n\"; done\n\nThanks,\n-Stolee\n"},{"id":"388524","messageId":"CAPyFy2AV2NG66LqBJr_Wb1_V5XhKnM+44m0H8FYa_3K4XupLow@mail.gmail.com","threadId":"52488","inReplyTo":"c4fef89a-2275-b4bc-b5c2-7bc647cd9bf6@gmail.com","subject":"Re: [PATCH] sparse-checkout: improve OS ls compatibility","fromName":"Ed Maste","fromEmail":"emaste@freebsd.org","sentAt":"2019-12-19T16:15:10Z","receivedAt":"2019-12-19T16:15:26Z","isPatch":true,"sender":{"key":"emaste@freebsd.org","avatar":"https://avatars.githubusercontent.com/u/1034582?v=4"},"body":"On Thu, 19 Dec 2019 at 08:56, Derrick Stolee <stolee@gmail.com> wrote:\n>\n> On 12/18/2019 9:45 PM, Eric Wong wrote:\n> > This goes on top of your patch:\n> ...\n> > +     ( cd \"$1\" && for i in *; do echo \"$i\"; done )\n>\n> Could we drop the \"cd\" and \"echo\" processes with this line instead?\n>\n>         for i in \"$1\"/*; do printf \"$i\\n\"; done\n\nThat would output repo/a, but we could do something like:\nfor i in \"$1\"/*; do echo \"${i#$1/}\"; done\n\necho's a builtin on any /bin/sh I'm aware of - do you have a /bin/sh\nwith builtin printf but not echo?\n"},{"id":"388525","messageId":"c68de4d3-c5b6-fc70-a233-9702e9552d94@gmail.com","threadId":"52488","inReplyTo":"CAPyFy2AV2NG66LqBJr_Wb1_V5XhKnM+44m0H8FYa_3K4XupLow@mail.gmail.com","subject":"Re: [PATCH] sparse-checkout: improve OS ls compatibility","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-12-19T16:34:31Z","receivedAt":"2019-12-19T16:34:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/19/2019 11:15 AM, Ed Maste wrote:\n> On Thu, 19 Dec 2019 at 08:56, Derrick Stolee <stolee@gmail.com> wrote:\n>>\n>> On 12/18/2019 9:45 PM, Eric Wong wrote:\n>>> This goes on top of your patch:\n>> ...\n>>> +     ( cd \"$1\" && for i in *; do echo \"$i\"; done )\n>>\n>> Could we drop the \"cd\" and \"echo\" processes with this line instead?\n>>\n>>         for i in \"$1\"/*; do printf \"$i\\n\"; done\n> \n> That would output repo/a, but we could do something like:\n> for i in \"$1\"/*; do echo \"${i#$1/}\"; done\n> \n> echo's a builtin on any /bin/sh I'm aware of - do you have a /bin/sh\n> with builtin printf but not echo?\n\nI guess I am misremembering the benefits of printf over echo. Carry\non with your approach.\n\nThanks,\n-Stolee\n\n"},{"id":"388555","messageId":"xmqqpngkb2ye.fsf@gitster-ct.c.googlers.com","threadId":"52488","inReplyTo":"20191219024518.GA3411@dcvr","subject":"Re: [PATCH] sparse-checkout: improve OS ls compatibility","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-19T18:11:21Z","receivedAt":"2019-12-19T18:11:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <e@80x24.org> writes:\n\n> But using a shell for loop seems doable, here, since there\n> doesn't seem to be wonky characters.  I've done this in the past\n> when I had to fix a system without \"ls\".\n>\n> This goes on top of your patch:\n>\n> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n> index 3a3eafa653..a431d05643 100755\n> --- a/t/t1091-sparse-checkout-builtin.sh\n> +++ b/t/t1091-sparse-checkout-builtin.sh\n> @@ -6,7 +6,7 @@ test_description='sparse checkout builtin tests'\n>  \n>  ls_no_git()\n>  {\n> -\tls -1 \"$1\" | grep -v .git\n> +\t( cd \"$1\" && for i in *; do echo \"$i\"; done )\n>  }\n\nHmph, my honest me is very tempted to say\n\n (1) don't run your tests as 'root', as that would break many tests\n     with prerequisite SANITY\n\n (2) fix your \"ls\" to behave\n\nbut if you want to list paths that match shell glob *, this would do\n\n\t(cd \"$1\" && printf \"%s\\n *)\n\nwithout any loop (other than the one printf gives us implicitly for\nfree), wouldn't it?\n\nNote that the helper function's name no longer reflects what it does\nwith such a change, so it needs to be renamed.  Together with style\nfix, perhaps\n\n\tls_no_dot () {\n\t\t(cd \"$1\" && printf \"%s\\n *)\n\t}\n\nis what we want, if somebody wants to keep using a broken /bin/ls?\n"},{"id":"388567","messageId":"CAPyFy2BubWbyq6tQmHYxquikn2+uHz+48VSfQ308BYiuE=SSWQ@mail.gmail.com","threadId":"52488","inReplyTo":"xmqqpngkb2ye.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] sparse-checkout: improve OS ls compatibility","fromName":"Ed Maste","fromEmail":"emaste@freebsd.org","sentAt":"2019-12-19T20:56:39Z","receivedAt":"2019-12-19T20:56:53Z","isPatch":true,"sender":{"key":"emaste@freebsd.org","avatar":"https://avatars.githubusercontent.com/u/1034582?v=4"},"body":"On Thu, 19 Dec 2019 at 13:11, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Hmph, my honest me is very tempted to say\n>\n>  (1) don't run your tests as 'root', as that would break many tests\n>      with prerequisite SANITY\n\nIt looks like this is just Cirrus-CI's default without explicit\nconfiguration or scripting to use an unprivileged user. Certainly\nrunning the build and test as root is generally not a good idea, but\nit is a purpose-built throwaway VM and so doesn't matter much. Anyhow,\nit certainly needs to be addressed to avoid skipping the SANITY tests.\n\n>  (2) fix your \"ls\" to behave\n\nWell, given that hidden dot files were ostensibly a bug and BSD ls has\nhad this behaviour for over 40 years it's far too late to change. I\ncan see the rationale for showing all files for root, even though I\ndislike the behaviour changing between privileged and unprivileged\nusers.\n\n> but if you want to list paths that match shell glob *, this would do\n>\n>         (cd \"$1\" && printf \"%s\\n *)\n>\n> without any loop (other than the one printf gives us implicitly for\n> free), wouldn't it?\n\nYes.\n\n> Note that the helper function's name no longer reflects what it does\n> with such a change, so it needs to be renamed.  Together with style\n> fix, perhaps\n>\n>         ls_no_dot () {\n>                 (cd \"$1\" && printf \"%s\\n *)\n>         }\n>\n> is what we want,\n\nI believe the tests should pass or be skipped when run as root, so I\nthink we should either require (something like) SANITY for these\ntests, or make the change above. I'm happy with either option; I'll\nsend a v2 based on the approach above for consideration.\n"},{"id":"388578","messageId":"20191219214516.69209-1-emaste@FreeBSD.org","threadId":"52488","inReplyTo":"20191219015833.49314-1-emaste@FreeBSD.org","subject":"[PATCH v2] sparse-checkout: improve OS ls compatibility","fromName":"Ed Maste","fromEmail":"emaste@freebsd.org","sentAt":"2019-12-19T21:45:16Z","receivedAt":"2019-12-19T21:45:29Z","isPatch":true,"sender":{"key":"emaste@freebsd.org","avatar":"https://avatars.githubusercontent.com/u/1034582?v=4"},"body":"On FreeBSD, when executed by root ls enables the '-A' option:\n\n  -A  Include directory entries whose names begin with a dot (`.')\n      except for . and ...  Automatically set for the super-user unless\n      -I is specified.\n\nAs a result the .git directory appeared in the output when run as root.\nSimulate no-dotfile ls behaviour using a shell glob.\n\nSigned-off-by: Ed Maste <emaste@FreeBSD.org>\nHelped-by: Eric Wong <e@80x24.org>\nHelped-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t1091-sparse-checkout-builtin.sh | 32 +++++++++++++++++-------------\n 1 file changed, 18 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex cee98a1c8a..7e8cac679e 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -4,6 +4,10 @@ test_description='sparse checkout builtin tests'\n \n . ./test-lib.sh\n \n+ls_no_dot() {\n+\t(cd \"$1\" && printf '%s\\n' *)\n+}\n+\n test_expect_success 'setup' '\n \tgit init repo &&\n \t(\n@@ -50,7 +54,7 @@ test_expect_success 'git sparse-checkout init' '\n \tEOF\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n \ttest_cmp_config -C repo true core.sparsecheckout &&\n-\tls repo >dir  &&\n+\tls_no_dot repo >dir  &&\n \techo a >expect &&\n \ttest_cmp expect dir\n '\n@@ -73,7 +77,7 @@ test_expect_success 'init with existing sparse-checkout' '\n \t\t*folder*\n \tEOF\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n-\tls repo >dir  &&\n+\tls_no_dot repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n@@ -90,7 +94,7 @@ test_expect_success 'clone --sparse' '\n \t\t!/*/\n \tEOF\n \ttest_cmp expect actual &&\n-\tls clone >dir &&\n+\tls_no_dot clone >dir &&\n \techo a >expect &&\n \ttest_cmp expect dir\n '\n@@ -119,7 +123,7 @@ test_expect_success 'set sparse-checkout using builtin' '\n \tgit -C repo sparse-checkout list >actual &&\n \ttest_cmp expect actual &&\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n-\tls repo >dir  &&\n+\tls_no_dot repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n@@ -139,7 +143,7 @@ test_expect_success 'set sparse-checkout using --stdin' '\n \tgit -C repo sparse-checkout list >actual &&\n \ttest_cmp expect actual &&\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n-\tls repo >dir  &&\n+\tls_no_dot repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n@@ -154,7 +158,7 @@ test_expect_success 'cone mode: match patterns' '\n \tgit -C repo read-tree -mu HEAD 2>err &&\n \ttest_i18ngrep ! \"disabling cone patterns\" err &&\n \tgit -C repo reset --hard &&\n-\tls repo >dir  &&\n+\tls_no_dot repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n@@ -177,7 +181,7 @@ test_expect_success 'sparse-checkout disable' '\n \ttest_path_is_file repo/.git/info/sparse-checkout &&\n \tgit -C repo config --list >config &&\n \ttest_must_fail git config core.sparseCheckout &&\n-\tls repo >dir &&\n+\tls_no_dot repo >dir &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeep\n@@ -191,24 +195,24 @@ test_expect_success 'cone mode: init and set' '\n \tgit -C repo sparse-checkout init --cone &&\n \tgit -C repo config --list >config &&\n \ttest_i18ngrep \"core.sparsecheckoutcone=true\" config &&\n-\tls repo >dir  &&\n+\tls_no_dot repo >dir  &&\n \techo a >expect &&\n \ttest_cmp expect dir &&\n \tgit -C repo sparse-checkout set deep/deeper1/deepest/ 2>err &&\n \ttest_must_be_empty err &&\n-\tls repo >dir  &&\n+\tls_no_dot repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeep\n \tEOF\n \ttest_cmp expect dir &&\n-\tls repo/deep >dir  &&\n+\tls_no_dot repo/deep >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeeper1\n \tEOF\n \ttest_cmp expect dir &&\n-\tls repo/deep/deeper1 >dir  &&\n+\tls_no_dot repo/deep/deeper1 >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeepest\n@@ -234,7 +238,7 @@ test_expect_success 'cone mode: init and set' '\n \t\tfolder1\n \t\tfolder2\n \tEOF\n-\tls repo >dir &&\n+\tls_no_dot repo >dir &&\n \ttest_cmp expect dir\n '\n \n@@ -256,7 +260,7 @@ test_expect_success 'revert to old sparse-checkout on bad update' '\n \ttest_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&\n \ttest_i18ngrep \"cannot set sparse-checkout patterns\" err &&\n \ttest_cmp repo/.git/info/sparse-checkout expect &&\n-\tls repo/deep >dir &&\n+\tls_no_dot repo/deep >dir &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeeper1\n@@ -313,7 +317,7 @@ test_expect_success 'cone mode: set with core.ignoreCase=true' '\n \t\t/folder1/\n \tEOF\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n-\tls repo >dir &&\n+\tls_no_dot repo >dir &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n-- \n2.24.0\n\n"},{"id":"388579","messageId":"xmqqtv5w9dqy.fsf@gitster-ct.c.googlers.com","threadId":"52488","inReplyTo":"CAPyFy2BubWbyq6tQmHYxquikn2+uHz+48VSfQ308BYiuE=SSWQ@mail.gmail.com","subject":"Re: [PATCH] sparse-checkout: improve OS ls compatibility","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-19T22:01:09Z","receivedAt":"2019-12-19T22:01:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ed Maste <emaste@freebsd.org> writes:\n\n>> Note that the helper function's name no longer reflects what it does\n>> with such a change, so it needs to be renamed.  Together with style\n>> fix, perhaps\n>>\n>>         ls_no_dot () {\n>>                 (cd \"$1\" && printf \"%s\\n *)\n>>         }\n>>\n>> is what we want,\n>\n> I believe the tests should pass or be skipped when run as root, so I\n> think we should either require (something like) SANITY for these\n> tests, or make the change above. I'm happy with either option; I'll\n> send a v2 based on the approach above for consideration.\n\nOK, after thinking about it a bit more, I think \"Your ls is broken\"\nwas completely missing the point.  What we want in the callers of\nthis helper is to list the contents of a directory, and \"ls\" is one\npossible (and easiest, if there were no \"oops, sometimes -A is enabled\n implementation by default\" complication) implementation.\n\nAnd \"ls_no_dot\" is a misnomer from that point of view.  We are not\neven using \"ls\", so perhaps we should just call it \"list_files\" or\nsomething?\n\nThanks.\n"},{"id":"388598","messageId":"20191219222748.GA63814@generichostname","threadId":"52488","inReplyTo":"20191219214516.69209-1-emaste@FreeBSD.org","subject":"Re: [PATCH v2] sparse-checkout: improve OS ls compatibility","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-19T22:27:48Z","receivedAt":"2019-12-19T22:26:13Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Ed,\n\nOn Thu, Dec 19, 2019 at 09:45:16PM +0000, Ed Maste wrote:\n> On FreeBSD, when executed by root ls enables the '-A' option:\n> \n>   -A  Include directory entries whose names begin with a dot (`.')\n>       except for . and ...  Automatically set for the super-user unless\n>       -I is specified.\n> \n> As a result the .git directory appeared in the output when run as root.\n> Simulate no-dotfile ls behaviour using a shell glob.\n> \n> Signed-off-by: Ed Maste <emaste@FreeBSD.org>\n> Helped-by: Eric Wong <e@80x24.org>\n> Helped-by: Junio C Hamano <gitster@pobox.com>\n\nSmall nit: the Helped-by trailers should come before your sign-off.\n\nTrailers should come in chronological order. Chronologically, they\nhelped you out with your patch and then, after that, you created your v2\nbased on their review and signed off on it.\n\nThanks,\n\nDenton\n"},{"id":"388613","messageId":"20191220153814.54899-1-emaste@FreeBSD.org","threadId":"52488","inReplyTo":"20191219015833.49314-1-emaste@FreeBSD.org","subject":"[PATCH v3] sparse-checkout: improve OS ls compatibility","fromName":"Ed Maste","fromEmail":"emaste@freebsd.org","sentAt":"2019-12-20T15:38:14Z","receivedAt":"2019-12-20T15:38:21Z","isPatch":true,"sender":{"key":"emaste@freebsd.org","avatar":"https://avatars.githubusercontent.com/u/1034582?v=4"},"body":"On FreeBSD, when executed by root ls enables the '-A' option:\n\n  -A  Include directory entries whose names begin with a dot (`.')\n      except for . and ...  Automatically set for the super-user unless\n      -I is specified.\n\nAs a result the .git directory appeared in the output when run as root.\nSimulate no-dotfile ls behaviour using a shell glob.\n\nHelped-by: Eric Wong <e@80x24.org>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Ed Maste <emaste@FreeBSD.org>\n---\nSince v2, rename the function list_files and add a comment explaining why\nit's not just using ls.  Note that this change is not necessary when running\nthe tests as an unprivileged user on FreeBSD, and the proposed FreeBSD CI\npatch has been updated to do so.  That said I still believe we should make\neither this change or add a prerequisite so this test does not run as root\non FreeBSD.\n\n t/t1091-sparse-checkout-builtin.sh | 35 ++++++++++++++++++------------\n 1 file changed, 21 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex cee98a1c8a..168702784d 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -4,6 +4,13 @@ test_description='sparse checkout builtin tests'\n \n . ./test-lib.sh\n \n+# List files in a directory, excluding hidden dot files (such as .git).\n+# This is similar to ls, but some ls implementations include dot files by\n+# default when run as root.\n+list_files() {\n+\t(cd \"$1\" && printf '%s\\n' *)\n+}\n+\n test_expect_success 'setup' '\n \tgit init repo &&\n \t(\n@@ -50,7 +57,7 @@ test_expect_success 'git sparse-checkout init' '\n \tEOF\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n \ttest_cmp_config -C repo true core.sparsecheckout &&\n-\tls repo >dir  &&\n+\tlist_files repo >dir  &&\n \techo a >expect &&\n \ttest_cmp expect dir\n '\n@@ -73,7 +80,7 @@ test_expect_success 'init with existing sparse-checkout' '\n \t\t*folder*\n \tEOF\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n-\tls repo >dir  &&\n+\tlist_files repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n@@ -90,7 +97,7 @@ test_expect_success 'clone --sparse' '\n \t\t!/*/\n \tEOF\n \ttest_cmp expect actual &&\n-\tls clone >dir &&\n+\tlist_files clone >dir &&\n \techo a >expect &&\n \ttest_cmp expect dir\n '\n@@ -119,7 +126,7 @@ test_expect_success 'set sparse-checkout using builtin' '\n \tgit -C repo sparse-checkout list >actual &&\n \ttest_cmp expect actual &&\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n-\tls repo >dir  &&\n+\tlist_files repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n@@ -139,7 +146,7 @@ test_expect_success 'set sparse-checkout using --stdin' '\n \tgit -C repo sparse-checkout list >actual &&\n \ttest_cmp expect actual &&\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n-\tls repo >dir  &&\n+\tlist_files repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n@@ -154,7 +161,7 @@ test_expect_success 'cone mode: match patterns' '\n \tgit -C repo read-tree -mu HEAD 2>err &&\n \ttest_i18ngrep ! \"disabling cone patterns\" err &&\n \tgit -C repo reset --hard &&\n-\tls repo >dir  &&\n+\tlist_files repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n@@ -177,7 +184,7 @@ test_expect_success 'sparse-checkout disable' '\n \ttest_path_is_file repo/.git/info/sparse-checkout &&\n \tgit -C repo config --list >config &&\n \ttest_must_fail git config core.sparseCheckout &&\n-\tls repo >dir &&\n+\tlist_files repo >dir &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeep\n@@ -191,24 +198,24 @@ test_expect_success 'cone mode: init and set' '\n \tgit -C repo sparse-checkout init --cone &&\n \tgit -C repo config --list >config &&\n \ttest_i18ngrep \"core.sparsecheckoutcone=true\" config &&\n-\tls repo >dir  &&\n+\tlist_files repo >dir  &&\n \techo a >expect &&\n \ttest_cmp expect dir &&\n \tgit -C repo sparse-checkout set deep/deeper1/deepest/ 2>err &&\n \ttest_must_be_empty err &&\n-\tls repo >dir  &&\n+\tlist_files repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeep\n \tEOF\n \ttest_cmp expect dir &&\n-\tls repo/deep >dir  &&\n+\tlist_files repo/deep >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeeper1\n \tEOF\n \ttest_cmp expect dir &&\n-\tls repo/deep/deeper1 >dir  &&\n+\tlist_files repo/deep/deeper1 >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeepest\n@@ -234,7 +241,7 @@ test_expect_success 'cone mode: init and set' '\n \t\tfolder1\n \t\tfolder2\n \tEOF\n-\tls repo >dir &&\n+\tlist_files repo >dir &&\n \ttest_cmp expect dir\n '\n \n@@ -256,7 +263,7 @@ test_expect_success 'revert to old sparse-checkout on bad update' '\n \ttest_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&\n \ttest_i18ngrep \"cannot set sparse-checkout patterns\" err &&\n \ttest_cmp repo/.git/info/sparse-checkout expect &&\n-\tls repo/deep >dir &&\n+\tlist_files repo/deep >dir &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeeper1\n@@ -313,7 +320,7 @@ test_expect_success 'cone mode: set with core.ignoreCase=true' '\n \t\t/folder1/\n \tEOF\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n-\tls repo >dir &&\n+\tlist_files repo >dir &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n-- \n2.24.0\n\n"},{"id":"388615","messageId":"9c3d10c3-76fb-9e9e-013b-b3f66b934dd6@gmail.com","threadId":"52488","inReplyTo":"20191220153814.54899-1-emaste@FreeBSD.org","subject":"Re: [PATCH v3] sparse-checkout: improve OS ls compatibility","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-12-20T16:05:22Z","receivedAt":"2019-12-20T16:05:25Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/20/2019 10:38 AM, Ed Maste wrote:\n> On FreeBSD, when executed by root ls enables the '-A' option:\n> \n>   -A  Include directory entries whose names begin with a dot (`.')\n>       except for . and ...  Automatically set for the super-user unless\n>       -I is specified.\n> \n> As a result the .git directory appeared in the output when run as root.\n> Simulate no-dotfile ls behaviour using a shell glob.\n\nThis patch looks good to me and seems to match where the\ndiscussion landed. Thanks for finding and fixing this!\n\n-Stolee\n\n"},{"id":"388634","messageId":"xmqqpngianlo.fsf@gitster-ct.c.googlers.com","threadId":"52488","inReplyTo":"9c3d10c3-76fb-9e9e-013b-b3f66b934dd6@gmail.com","subject":"Re: [PATCH v3] sparse-checkout: improve OS ls compatibility","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-20T17:55:15Z","receivedAt":"2019-12-20T17:55:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> On 12/20/2019 10:38 AM, Ed Maste wrote:\n>> On FreeBSD, when executed by root ls enables the '-A' option:\n>> \n>>   -A  Include directory entries whose names begin with a dot (`.')\n>>       except for . and ...  Automatically set for the super-user unless\n>>       -I is specified.\n>> \n>> As a result the .git directory appeared in the output when run as root.\n>> Simulate no-dotfile ls behaviour using a shell glob.\n>\n> This patch looks good to me and seems to match where the\n> discussion landed. Thanks for finding and fixing this!\n\nThanks, all.  Will queue.\n"},{"id":"388635","messageId":"CAPig+cS6XPc9KZo3ytEHLFjMxEFqCk5OJMUjZyFBP0cA95u9Lw@mail.gmail.com","threadId":"52488","inReplyTo":"20191220153814.54899-1-emaste@FreeBSD.org","subject":"Re: [PATCH v3] sparse-checkout: improve OS ls compatibility","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-20T17:55:34Z","receivedAt":"2019-12-20T17:55:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 20, 2019 at 10:38 AM Ed Maste <emaste@freebsd.org> wrote:\n> On FreeBSD, when executed by root ls enables the '-A' option:\n>\n>   -A  Include directory entries whose names begin with a dot (`.')\n>       except for . and ...  Automatically set for the super-user unless\n>       -I is specified.\n>\n> As a result the .git directory appeared in the output when run as root.\n> Simulate no-dotfile ls behaviour using a shell glob.\n>\n> Signed-off-by: Ed Maste <emaste@FreeBSD.org>\n> ---\n> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n> @@ -4,6 +4,13 @@ test_description='sparse checkout builtin tests'\n> +# List files in a directory, excluding hidden dot files (such as .git).\n> +# This is similar to ls, but some ls implementations include dot files by\n> +# default when run as root.\n> +list_files() {\n> +       (cd \"$1\" && printf '%s\\n' *)\n> +}\n\nNit: While this may indeed be a case for which an explanatory comment\nis justified, the comment itself is a bit lacking -- just enough to\nkeep it from being helpful for the next person who comes along to work\non this code. For instance, the too-abstract \"some ls implementations\"\ndoesn't provide enough information to point at a specific 'ls'\nimplementation if a person needs to do testing against one of these\nimplementations or wants to know if (say, several years from now) that\nanomalous behavior is still present. It would be helpful, therefore,\nto mention such an implementation by name:\n\n    ...some 'ls' implementations, such as on FreeBSD, include...\n\n(One can, of course, always argue that the commit message can be\nconsulted to learn about a particular 'ls' implementation, but then\nwhy have an in-code comment at all?)\n"},{"id":"388653","messageId":"CAPyFy2D25M2O1M_kaAYH_SzoJ2BW-K4470ybOSZPrQLMnHFB3Q@mail.gmail.com","threadId":"52488","inReplyTo":"CAPig+cS6XPc9KZo3ytEHLFjMxEFqCk5OJMUjZyFBP0cA95u9Lw@mail.gmail.com","subject":"Re: [PATCH v3] sparse-checkout: improve OS ls compatibility","fromName":"Ed Maste","fromEmail":"emaste@freebsd.org","sentAt":"2019-12-20T18:15:40Z","receivedAt":"2019-12-20T18:15:55Z","isPatch":true,"sender":{"key":"emaste@freebsd.org","avatar":"https://avatars.githubusercontent.com/u/1034582?v=4"},"body":"On Fri, 20 Dec 2019 at 12:55, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> It would be helpful, therefore,\n> to mention such an implementation by name:\n>\n>     ...some 'ls' implementations, such as on FreeBSD, include...\n>\n> (One can, of course, always argue that the commit message can be\n> consulted to learn about a particular 'ls' implementation, but then\n> why have an in-code comment at all?)\n\nIt could serve as a hint to check the commit history, but fair enough\n- I agree including the specific example is an improvement. I can\nresend if necessary but probably that change can just be folded in?\n"},{"id":"388654","messageId":"xmqqftheamea.fsf@gitster-ct.c.googlers.com","threadId":"52488","inReplyTo":"CAPig+cS6XPc9KZo3ytEHLFjMxEFqCk5OJMUjZyFBP0cA95u9Lw@mail.gmail.com","subject":"Re: [PATCH v3] sparse-checkout: improve OS ls compatibility","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-20T18:21:17Z","receivedAt":"2019-12-20T18:21:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> anomalous behavior is still present. It would be helpful, therefore,\n> to mention such an implementation by name:\n>\n>     ...some 'ls' implementations, such as on FreeBSD, include...\n>\n> (One can, of course, always argue that the commit message can be\n> consulted to learn about a particular 'ls' implementation, but then\n> why have an in-code comment at all?)\n\n\"This is similar to ls\" is not all that important, especially if we\nthen need to say how different from \"ls\" ours is.  The log message\nthat describes why we needed to move away from \"ls\" is a good place\nto say what aspect of \"ls\" was unsuitable.\n\nIf we _were_ to add an in-code comment, we may want to say something\nlike\n\n\t# Do not replace this with \"cd \"$1\" && ls\", as FreeBSD \"ls\"\n\t# enables \"-A\" when run by root without being told, and ends\n\t# up including \".git\" etc. in its output.\n\nto warn future developers against improving and/or cleaning up.\n\nNot that we encourage running our tests as root, though.  I am\nslightly worried that the above phrasing might be taken as such.\n"},{"id":"388656","messageId":"CAPig+cQ29dEbQgnJmGvODy9kGYq9TqKaJV5-mOPXbGFZ1HRWmw@mail.gmail.com","threadId":"52488","inReplyTo":"xmqqftheamea.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] sparse-checkout: improve OS ls compatibility","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-20T18:34:36Z","receivedAt":"2019-12-20T18:34:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 20, 2019 at 1:21 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> > anomalous behavior is still present. It would be helpful, therefore,\n> > to mention such an implementation by name:\n> >\n> >     ...some 'ls' implementations, such as on FreeBSD, include...\n> >\n> If we _were_ to add an in-code comment, we may want to say something\n> like\n>\n>         # Do not replace this with \"cd \"$1\" && ls\", as FreeBSD \"ls\"\n>         # enables \"-A\" when run by root without being told, and ends\n>         # up including \".git\" etc. in its output.\n>\n> to warn future developers against improving and/or cleaning up.\n\nI would find this comment more helpful than the existing one since it\nspells out the issue precisely. A minor tweak:\n\n    # Do not replace this with \"cd \"$1\" && ls\", as FreeBSD \"ls\"\n    # enables \"-A\" by default when run by root, and ends up\n    # including \".git\" etc. in its output.\n\n> Not that we encourage running our tests as root, though.  I am\n> slightly worried that the above phrasing might be taken as such.\n\nI'm not sure we really need to spell it out, but something like this\nmight allay that concern:\n\n    # Do not replace this with \"cd \"$1\" && ls\", as FreeBSD \"ls\"\n    # enables \"-A\" by default when run by root, and ends up\n    # including \".git\" etc. in its output. (Note, though, that\n    # running the test suite as root is generally not\n    # recommended.)\n"},{"id":"388657","messageId":"CAPyFy2AF+zcriUfZnpbXy+9r7hRpNBUe0agMuan-cE1ryqTipw@mail.gmail.com","threadId":"52488","inReplyTo":"xmqqftheamea.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] sparse-checkout: improve OS ls compatibility","fromName":"Ed Maste","fromEmail":"emaste@freebsd.org","sentAt":"2019-12-20T18:34:43Z","receivedAt":"2019-12-20T18:34:58Z","isPatch":true,"sender":{"key":"emaste@freebsd.org","avatar":"https://avatars.githubusercontent.com/u/1034582?v=4"},"body":"On Fri, 20 Dec 2019 at 13:21, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"This is similar to ls\" is not all that important, especially if we\n> then need to say how different from \"ls\" ours is.  The log message\n> that describes why we needed to move away from \"ls\" is a good place\n> to say what aspect of \"ls\" was unsuitable.\n\nOk, I'm also happy if it goes in with no comment; the reason I added\nit is I could foresee someone coming along in a few years, thinking\nthis is just a strange local implementation of ls, and changing it.\nBut, perhaps we can assume that any such person would check the\nhistory before doing so and the comment is not needed.\n\n> If we _were_ to add an in-code comment, we may want to say something\n> like\n>\n>         # Do not replace this with \"cd \"$1\" && ls\", as FreeBSD \"ls\"\n>         # enables \"-A\" when run by root without being told, and ends\n>         # up including \".git\" etc. in its output.\n>\n> to warn future developers against improving and/or cleaning up.\n\nIndeed, that is more direct, although it's not just FreeBSD ls; this\ncame from 4.2BSD and is probably common to most/all non-GNU ls\nimplementations. In particular, macOS behaves the same way. (Also, the\nreplacement would be even simpler, just \"ls $1\".)\n"},{"id":"388666","messageId":"xmqq7e2qajix.fsf@gitster-ct.c.googlers.com","threadId":"52488","inReplyTo":"CAPyFy2AF+zcriUfZnpbXy+9r7hRpNBUe0agMuan-cE1ryqTipw@mail.gmail.com","subject":"Re: [PATCH v3] sparse-checkout: improve OS ls compatibility","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-20T19:23:18Z","receivedAt":"2019-12-20T19:23:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ed Maste <emaste@freebsd.org> writes:\n\n> On Fri, 20 Dec 2019 at 13:21, Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> \"This is similar to ls\" is not all that important, especially if we\n>> then need to say how different from \"ls\" ours is.  The log message\n>> that describes why we needed to move away from \"ls\" is a good place\n>> to say what aspect of \"ls\" was unsuitable.\n>\n> Ok, I'm also happy if it goes in with no comment; the reason I added\n> it is I could foresee someone coming along in a few years, thinking\n> this is just a strange local implementation of ls, and changing it.\n> But, perhaps we can assume that any such person would check the\n> history before doing so and the comment is not needed.\n>\n>> If we _were_ to add an in-code comment, we may want to say something\n>> like\n>>\n>>         # Do not replace this with \"cd \"$1\" && ls\", as FreeBSD \"ls\"\n>>         # enables \"-A\" when run by root without being told, and ends\n>>         # up including \".git\" etc. in its output.\n>>\n>> to warn future developers against improving and/or cleaning up.\n>\n> Indeed, that is more direct, although it's not just FreeBSD ls; this\n> came from 4.2BSD and is probably common to most/all non-GNU ls\n> implementations. In particular, macOS behaves the same way. (Also, the\n> replacement would be even simpler, just \"ls $1\".)\n\nGood piece of info to include.  Final try for the day from me:\n\n    # Do not replace this with 'ls \"$1\"', as \"ls\" with BSD-lineage\n    # enables \"-A\" by default for root and ends up ...\n\n"},{"id":"388669","messageId":"CAPig+cR64QkBJ8ybMkMiX2CvrgajcGMyG41SMt4mA-VzHyae=A@mail.gmail.com","threadId":"52488","inReplyTo":"xmqq7e2qajix.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] sparse-checkout: improve OS ls compatibility","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-20T19:33:04Z","receivedAt":"2019-12-20T19:33:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 20, 2019 at 2:23 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Ed Maste <emaste@freebsd.org> writes:\n> > Ok, I'm also happy if it goes in with no comment; the reason I added\n> > it is I could foresee someone coming along in a few years, thinking\n> > this is just a strange local implementation of ls, and changing it.\n> > But, perhaps we can assume that any such person would check the\n> > history before doing so and the comment is not needed.\n\nThe in-code comment has sufficient value that I'd like to see it\nremain since your concern about someone coming along and wanting to\nreplace the function with \"ls\" is a genuine one, and because it saves\npeople the trouble of having to dig through history in the first\nplace. And, by \"people\", I mean that it may save reviewers too since\npatch submitters don't always dig through history or don't always\nexplain _why_ a change is a good idea or valid, which places the\nburden on reviewers instead.\n\n> > Indeed, that is more direct, although it's not just FreeBSD ls; this\n> > came from 4.2BSD and is probably common to most/all non-GNU ls\n> > implementations. In particular, macOS behaves the same way. (Also, the\n> > replacement would be even simpler, just \"ls $1\".)\n>\n> Good piece of info to include.  Final try for the day from me:\n>\n>     # Do not replace this with 'ls \"$1\"', as \"ls\" with BSD-lineage\n>     # enables \"-A\" by default for root and ends up ...\n\nThis looks good to me.\n"},{"id":"388671","messageId":"20191220194114.95509-1-emaste@FreeBSD.org","threadId":"52488","inReplyTo":"20191219015833.49314-1-emaste@FreeBSD.org","subject":"[PATCH v4] sparse-checkout: improve OS ls compatibility","fromName":"Ed Maste","fromEmail":"emaste@freebsd.org","sentAt":"2019-12-20T19:41:14Z","receivedAt":"2019-12-20T19:41:18Z","isPatch":true,"sender":{"key":"emaste@freebsd.org","avatar":"https://avatars.githubusercontent.com/u/1034582?v=4"},"body":"On FreeBSD, when executed by root ls enables the '-A' option:\n\n  -A  Include directory entries whose names begin with a dot (`.')\n      except for . and ...  Automatically set for the super-user unless\n      -I is specified.\n\nAs a result the .git directory appeared in the output when run as root.\nSimulate no-dotfile ls behaviour using a shell glob.\n\nHelped-by: Eric Wong <e@80x24.org>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Ed Maste <emaste@FreeBSD.org>\n---\nSince v3, adjust the comment to be an explicit caution against replacing\nwith ls.\n\n t/t1091-sparse-checkout-builtin.sh | 36 ++++++++++++++++++------------\n 1 file changed, 22 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex cee98a1c8a..6f7e2d0c9e 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -4,6 +4,14 @@ test_description='sparse checkout builtin tests'\n \n . ./test-lib.sh\n \n+list_files() {\n+\t# Do not replace this with 'ls \"$1\"', as \"ls\" with BSD-lineage\n+\t# enables \"-A\" by default for root and ends up including \".git\" and\n+\t# such in its output. (Note, though, that running the test suite as\n+\t# root is generally not recommended.)\n+\t(cd \"$1\" && printf '%s\\n' *)\n+}\n+\n test_expect_success 'setup' '\n \tgit init repo &&\n \t(\n@@ -50,7 +58,7 @@ test_expect_success 'git sparse-checkout init' '\n \tEOF\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n \ttest_cmp_config -C repo true core.sparsecheckout &&\n-\tls repo >dir  &&\n+\tlist_files repo >dir  &&\n \techo a >expect &&\n \ttest_cmp expect dir\n '\n@@ -73,7 +81,7 @@ test_expect_success 'init with existing sparse-checkout' '\n \t\t*folder*\n \tEOF\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n-\tls repo >dir  &&\n+\tlist_files repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n@@ -90,7 +98,7 @@ test_expect_success 'clone --sparse' '\n \t\t!/*/\n \tEOF\n \ttest_cmp expect actual &&\n-\tls clone >dir &&\n+\tlist_files clone >dir &&\n \techo a >expect &&\n \ttest_cmp expect dir\n '\n@@ -119,7 +127,7 @@ test_expect_success 'set sparse-checkout using builtin' '\n \tgit -C repo sparse-checkout list >actual &&\n \ttest_cmp expect actual &&\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n-\tls repo >dir  &&\n+\tlist_files repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n@@ -139,7 +147,7 @@ test_expect_success 'set sparse-checkout using --stdin' '\n \tgit -C repo sparse-checkout list >actual &&\n \ttest_cmp expect actual &&\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n-\tls repo >dir  &&\n+\tlist_files repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n@@ -154,7 +162,7 @@ test_expect_success 'cone mode: match patterns' '\n \tgit -C repo read-tree -mu HEAD 2>err &&\n \ttest_i18ngrep ! \"disabling cone patterns\" err &&\n \tgit -C repo reset --hard &&\n-\tls repo >dir  &&\n+\tlist_files repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n@@ -177,7 +185,7 @@ test_expect_success 'sparse-checkout disable' '\n \ttest_path_is_file repo/.git/info/sparse-checkout &&\n \tgit -C repo config --list >config &&\n \ttest_must_fail git config core.sparseCheckout &&\n-\tls repo >dir &&\n+\tlist_files repo >dir &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeep\n@@ -191,24 +199,24 @@ test_expect_success 'cone mode: init and set' '\n \tgit -C repo sparse-checkout init --cone &&\n \tgit -C repo config --list >config &&\n \ttest_i18ngrep \"core.sparsecheckoutcone=true\" config &&\n-\tls repo >dir  &&\n+\tlist_files repo >dir  &&\n \techo a >expect &&\n \ttest_cmp expect dir &&\n \tgit -C repo sparse-checkout set deep/deeper1/deepest/ 2>err &&\n \ttest_must_be_empty err &&\n-\tls repo >dir  &&\n+\tlist_files repo >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeep\n \tEOF\n \ttest_cmp expect dir &&\n-\tls repo/deep >dir  &&\n+\tlist_files repo/deep >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeeper1\n \tEOF\n \ttest_cmp expect dir &&\n-\tls repo/deep/deeper1 >dir  &&\n+\tlist_files repo/deep/deeper1 >dir  &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeepest\n@@ -234,7 +242,7 @@ test_expect_success 'cone mode: init and set' '\n \t\tfolder1\n \t\tfolder2\n \tEOF\n-\tls repo >dir &&\n+\tlist_files repo >dir &&\n \ttest_cmp expect dir\n '\n \n@@ -256,7 +264,7 @@ test_expect_success 'revert to old sparse-checkout on bad update' '\n \ttest_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&\n \ttest_i18ngrep \"cannot set sparse-checkout patterns\" err &&\n \ttest_cmp repo/.git/info/sparse-checkout expect &&\n-\tls repo/deep >dir &&\n+\tlist_files repo/deep >dir &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tdeeper1\n@@ -313,7 +321,7 @@ test_expect_success 'cone mode: set with core.ignoreCase=true' '\n \t\t/folder1/\n \tEOF\n \ttest_cmp expect repo/.git/info/sparse-checkout &&\n-\tls repo >dir &&\n+\tlist_files repo >dir &&\n \tcat >expect <<-EOF &&\n \t\ta\n \t\tfolder1\n-- \n2.24.0\n\n"}]}