[PATCH] sparse-checkout: improve OS ls compatibility

[PATCH] sparse-checkout: improve OS ls compatibility

[Ed Maste]

On FreeBSD, when executed by root ls enables the '-A' option:

  -A  Include directory entries whose names begin with a dot (`.')
      except for . and ...  Automatically set for the super-user unless
      -I is specified.

Pipe ls's output to grep -v .git to remove the undesired entry.  Also
pass the -1 option to ensure one entry per line.

Signed-off-by: Ed Maste <emaste@FreeBSD.org>
---
There are several different ways this could be solved; this approach
felt cleanest to me, but there are at least two other reasonable
alternatives:

  * Add -a to the invocations and .git to the expected output

  * Add LSFLAGS and set it to -I on BSDs, to turn off the special dot
    behaviour

I'll submit a new patch if a different approach is preferred.

 t/t1091-sparse-checkout-builtin.sh | 33 +++++++++++++++++-------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index cee98a1c8a..3a3eafa653 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -4,6 +4,11 @@ test_description='sparse checkout builtin tests'
 
 . ./test-lib.sh
 
+ls_no_git()
+{
+	ls -1 "$1" | grep -v .git
+}
+
 test_expect_success 'setup' '
 	git init repo &&
 	(
@@ -50,7 +55,7 @@ test_expect_success 'git sparse-checkout init' '
 	EOF
 	test_cmp expect repo/.git/info/sparse-checkout &&
 	test_cmp_config -C repo true core.sparsecheckout &&
-	ls repo >dir  &&
+	ls_no_git repo >dir  &&
 	echo a >expect &&
 	test_cmp expect dir
 '
@@ -73,7 +78,7 @@ test_expect_success 'init with existing sparse-checkout' '
 		*folder*
 	EOF
 	test_cmp expect repo/.git/info/sparse-checkout &&
-	ls repo >dir  &&
+	ls_no_git repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		folder1
@@ -90,7 +95,7 @@ test_expect_success 'clone --sparse' '
 		!/*/
 	EOF
 	test_cmp expect actual &&
-	ls clone >dir &&
+	ls_no_git clone >dir &&
 	echo a >expect &&
 	test_cmp expect dir
 '
@@ -119,7 +124,7 @@ test_expect_success 'set sparse-checkout using builtin' '
 	git -C repo sparse-checkout list >actual &&
 	test_cmp expect actual &&
 	test_cmp expect repo/.git/info/sparse-checkout &&
-	ls repo >dir  &&
+	ls_no_git repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		folder1
@@ -139,7 +144,7 @@ test_expect_success 'set sparse-checkout using --stdin' '
 	git -C repo sparse-checkout list >actual &&
 	test_cmp expect actual &&
 	test_cmp expect repo/.git/info/sparse-checkout &&
-	ls repo >dir  &&
+	ls_no_git repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		folder1
@@ -154,7 +159,7 @@ test_expect_success 'cone mode: match patterns' '
 	git -C repo read-tree -mu HEAD 2>err &&
 	test_i18ngrep ! "disabling cone patterns" err &&
 	git -C repo reset --hard &&
-	ls repo >dir  &&
+	ls_no_git repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		folder1
@@ -177,7 +182,7 @@ test_expect_success 'sparse-checkout disable' '
 	test_path_is_file repo/.git/info/sparse-checkout &&
 	git -C repo config --list >config &&
 	test_must_fail git config core.sparseCheckout &&
-	ls repo >dir &&
+	ls_no_git repo >dir &&
 	cat >expect <<-EOF &&
 		a
 		deep
@@ -191,24 +196,24 @@ test_expect_success 'cone mode: init and set' '
 	git -C repo sparse-checkout init --cone &&
 	git -C repo config --list >config &&
 	test_i18ngrep "core.sparsecheckoutcone=true" config &&
-	ls repo >dir  &&
+	ls_no_git repo >dir  &&
 	echo a >expect &&
 	test_cmp expect dir &&
 	git -C repo sparse-checkout set deep/deeper1/deepest/ 2>err &&
 	test_must_be_empty err &&
-	ls repo >dir  &&
+	ls_no_git repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		deep
 	EOF
 	test_cmp expect dir &&
-	ls repo/deep >dir  &&
+	ls_no_git repo/deep >dir  &&
 	cat >expect <<-EOF &&
 		a
 		deeper1
 	EOF
 	test_cmp expect dir &&
-	ls repo/deep/deeper1 >dir  &&
+	ls_no_git repo/deep/deeper1 >dir  &&
 	cat >expect <<-EOF &&
 		a
 		deepest
@@ -234,7 +239,7 @@ test_expect_success 'cone mode: init and set' '
 		folder1
 		folder2
 	EOF
-	ls repo >dir &&
+	ls_no_git repo >dir &&
 	test_cmp expect dir
 '
 
@@ -256,7 +261,7 @@ test_expect_success 'revert to old sparse-checkout on bad update' '
 	test_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&
 	test_i18ngrep "cannot set sparse-checkout patterns" err &&
 	test_cmp repo/.git/info/sparse-checkout expect &&
-	ls repo/deep >dir &&
+	ls_no_git repo/deep >dir &&
 	cat >expect <<-EOF &&
 		a
 		deeper1
@@ -313,7 +318,7 @@ test_expect_success 'cone mode: set with core.ignoreCase=true' '
 		/folder1/
 	EOF
 	test_cmp expect repo/.git/info/sparse-checkout &&
-	ls repo >dir &&
+	ls_no_git repo >dir &&
 	cat >expect <<-EOF &&
 		a
 		folder1
-- 
2.24.0

Re: [PATCH] sparse-checkout: improve OS ls compatibility

[Derrick Stolee]

On 12/18/2019 8:58 PM, Ed Maste wrote:

Thanks for the report!

It was a little unclear from the get-go what exactly the issue is.

> On FreeBSD, when executed by root ls enables the '-A' option:
> 
>   -A  Include directory entries whose names begin with a dot (`.')
>       except for . and ...  Automatically set for the super-user unless
>       -I is specified.

It appears that the "ls" commands in the sparse-checkout tests are
reporting the ".git" directory when executed on FreeBSD as root. Is this
only as root?

> Pipe ls's output to grep -v .git to remove the undesired entry.  Also
> pass the -1 option to ensure one entry per line.

What if we instead ran "ls -a" and added .git to our expected output
(when appropriate)? Would that be simpler (and reduce the process
count that this solution introduces).

Thanks,
-Stolee
 
> Signed-off-by: Ed Maste <emaste@FreeBSD.org>
> ---
> There are several different ways this could be solved; this approach
> felt cleanest to me, but there are at least two other reasonable
> alternatives:
> 
>   * Add -a to the invocations and .git to the expected output
> 
>   * Add LSFLAGS and set it to -I on BSDs, to turn off the special dot
>     behaviour
> 
> I'll submit a new patch if a different approach is preferred.
> 
>  t/t1091-sparse-checkout-builtin.sh | 33 +++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index cee98a1c8a..3a3eafa653 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -4,6 +4,11 @@ test_description='sparse checkout builtin tests'
>  
>  . ./test-lib.sh
>  
> +ls_no_git()
> +{
> +	ls -1 "$1" | grep -v .git
> +}
> +
>  test_expect_success 'setup' '
>  	git init repo &&
>  	(
> @@ -50,7 +55,7 @@ test_expect_success 'git sparse-checkout init' '
>  	EOF
>  	test_cmp expect repo/.git/info/sparse-checkout &&
>  	test_cmp_config -C repo true core.sparsecheckout &&
> -	ls repo >dir  &&
> +	ls_no_git repo >dir  &&
>  	echo a >expect &&
>  	test_cmp expect dir
>  '
> @@ -73,7 +78,7 @@ test_expect_success 'init with existing sparse-checkout' '
>  		*folder*
>  	EOF
>  	test_cmp expect repo/.git/info/sparse-checkout &&
> -	ls repo >dir  &&
> +	ls_no_git repo >dir  &&
>  	cat >expect <<-EOF &&
>  		a
>  		folder1
> @@ -90,7 +95,7 @@ test_expect_success 'clone --sparse' '
>  		!/*/
>  	EOF
>  	test_cmp expect actual &&
> -	ls clone >dir &&
> +	ls_no_git clone >dir &&
>  	echo a >expect &&
>  	test_cmp expect dir
>  '
> @@ -119,7 +124,7 @@ test_expect_success 'set sparse-checkout using builtin' '
>  	git -C repo sparse-checkout list >actual &&
>  	test_cmp expect actual &&
>  	test_cmp expect repo/.git/info/sparse-checkout &&
> -	ls repo >dir  &&
> +	ls_no_git repo >dir  &&
>  	cat >expect <<-EOF &&
>  		a
>  		folder1
> @@ -139,7 +144,7 @@ test_expect_success 'set sparse-checkout using --stdin' '
>  	git -C repo sparse-checkout list >actual &&
>  	test_cmp expect actual &&
>  	test_cmp expect repo/.git/info/sparse-checkout &&
> -	ls repo >dir  &&
> +	ls_no_git repo >dir  &&
>  	cat >expect <<-EOF &&
>  		a
>  		folder1
> @@ -154,7 +159,7 @@ test_expect_success 'cone mode: match patterns' '
>  	git -C repo read-tree -mu HEAD 2>err &&
>  	test_i18ngrep ! "disabling cone patterns" err &&
>  	git -C repo reset --hard &&
> -	ls repo >dir  &&
> +	ls_no_git repo >dir  &&
>  	cat >expect <<-EOF &&
>  		a
>  		folder1
> @@ -177,7 +182,7 @@ test_expect_success 'sparse-checkout disable' '
>  	test_path_is_file repo/.git/info/sparse-checkout &&
>  	git -C repo config --list >config &&
>  	test_must_fail git config core.sparseCheckout &&
> -	ls repo >dir &&
> +	ls_no_git repo >dir &&
>  	cat >expect <<-EOF &&
>  		a
>  		deep
> @@ -191,24 +196,24 @@ test_expect_success 'cone mode: init and set' '
>  	git -C repo sparse-checkout init --cone &&
>  	git -C repo config --list >config &&
>  	test_i18ngrep "core.sparsecheckoutcone=true" config &&
> -	ls repo >dir  &&
> +	ls_no_git repo >dir  &&
>  	echo a >expect &&
>  	test_cmp expect dir &&
>  	git -C repo sparse-checkout set deep/deeper1/deepest/ 2>err &&
>  	test_must_be_empty err &&
> -	ls repo >dir  &&
> +	ls_no_git repo >dir  &&
>  	cat >expect <<-EOF &&
>  		a
>  		deep
>  	EOF
>  	test_cmp expect dir &&
> -	ls repo/deep >dir  &&
> +	ls_no_git repo/deep >dir  &&
>  	cat >expect <<-EOF &&
>  		a
>  		deeper1
>  	EOF
>  	test_cmp expect dir &&
> -	ls repo/deep/deeper1 >dir  &&
> +	ls_no_git repo/deep/deeper1 >dir  &&
>  	cat >expect <<-EOF &&
>  		a
>  		deepest
> @@ -234,7 +239,7 @@ test_expect_success 'cone mode: init and set' '
>  		folder1
>  		folder2
>  	EOF
> -	ls repo >dir &&
> +	ls_no_git repo >dir &&
>  	test_cmp expect dir
>  '
>  
> @@ -256,7 +261,7 @@ test_expect_success 'revert to old sparse-checkout on bad update' '
>  	test_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&
>  	test_i18ngrep "cannot set sparse-checkout patterns" err &&
>  	test_cmp repo/.git/info/sparse-checkout expect &&
> -	ls repo/deep >dir &&
> +	ls_no_git repo/deep >dir &&
>  	cat >expect <<-EOF &&
>  		a
>  		deeper1
> @@ -313,7 +318,7 @@ test_expect_success 'cone mode: set with core.ignoreCase=true' '
>  		/folder1/
>  	EOF
>  	test_cmp expect repo/.git/info/sparse-checkout &&
> -	ls repo >dir &&
> +	ls_no_git repo >dir &&
>  	cat >expect <<-EOF &&
>  		a
>  		folder1
> 

Re: [PATCH] sparse-checkout: improve OS ls compatibility

[Ed Maste]

On Wed, 18 Dec 2019 at 21:07, Derrick Stolee <stolee@gmail.com> wrote:
>
> On 12/18/2019 8:58 PM, Ed Maste wrote:
>
> Thanks for the report!
>
> It was a little unclear from the get-go what exactly the issue is.
>
> > On FreeBSD, when executed by root ls enables the '-A' option:
> >
> >   -A  Include directory entries whose names begin with a dot (`.')
> >       except for . and ...  Automatically set for the super-user unless
> >       -I is specified.
>
> It appears that the "ls" commands in the sparse-checkout tests are
> reporting the ".git" directory when executed on FreeBSD as root. Is this
> only as root?

Yes, this is only as root - it seems Cirrus-CI invokes the build and
test scripts as root, which is why I had trouble reproducing it
locally.

> > Pipe ls's output to grep -v .git to remove the undesired entry.  Also
> > pass the -1 option to ensure one entry per line.
>
> What if we instead ran "ls -a" and added .git to our expected output
> (when appropriate)? Would that be simpler (and reduce the process
> count that this solution introduces).

I originally tried that approach and thought it was a bit cumbersome,
but avoiding additional process invocations is a good argument. I'll
send a v2 with that change instead.

Re: [PATCH] sparse-checkout: improve OS ls compatibility

[Derrick Stolee]

On 12/18/2019 9:18 PM, Ed Maste wrote:
> On Wed, 18 Dec 2019 at 21:07, Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 12/18/2019 8:58 PM, Ed Maste wrote:
>>
>> Thanks for the report!
>>
>> It was a little unclear from the get-go what exactly the issue is.
>>
>>> On FreeBSD, when executed by root ls enables the '-A' option:
>>>
>>>   -A  Include directory entries whose names begin with a dot (`.')
>>>       except for . and ...  Automatically set for the super-user unless
>>>       -I is specified.
>>
>> It appears that the "ls" commands in the sparse-checkout tests are
>> reporting the ".git" directory when executed on FreeBSD as root. Is this
>> only as root?
> 
> Yes, this is only as root - it seems Cirrus-CI invokes the build and
> test scripts as root, which is why I had trouble reproducing it
> locally.
> 
>>> Pipe ls's output to grep -v .git to remove the undesired entry.  Also
>>> pass the -1 option to ensure one entry per line.
>>
>> What if we instead ran "ls -a" and added .git to our expected output
>> (when appropriate)? Would that be simpler (and reduce the process
>> count that this solution introduces).
> 
> I originally tried that approach and thought it was a bit cumbersome,
> but avoiding additional process invocations is a good argument. I'll
> send a v2 with that change instead.

I guess you are right that having "." and ".." appear is a bit silly.
Perhaps your approach is cleaner, and the extra processes are not too
much of a cost.

Let's hold off on the v2 for a bit in case someone has a better idea.

Thanks,
-Stolee

Re: [PATCH] sparse-checkout: improve OS ls compatibility

[Eric Wong]

Ed Maste <emaste@FreeBSD.org> wrote:
> There are several different ways this could be solved; this approach
> felt cleanest to me, but there are at least two other reasonable
> alternatives:
> 
>   * Add -a to the invocations and .git to the expected output
> 
>   * Add LSFLAGS and set it to -I on BSDs, to turn off the special dot
>     behaviour
> 
> I'll submit a new patch if a different approach is preferred.

Relying on "ls" itself seems a bit fragile.
My first choice is to write more tests in Perl5 :)

But using a shell for loop seems doable, here, since there
doesn't seem to be wonky characters.  I've done this in the past
when I had to fix a system without "ls".

This goes on top of your patch:

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 3a3eafa653..a431d05643 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -6,7 +6,7 @@ test_description='sparse checkout builtin tests'
 
 ls_no_git()
 {
-	ls -1 "$1" | grep -v .git
+	( cd "$1" && for i in *; do echo "$i"; done )
 }
 
 test_expect_success 'setup' '

Re: [PATCH] sparse-checkout: improve OS ls compatibility

[Derrick Stolee]

On 12/18/2019 9:45 PM, Eric Wong wrote:
> This goes on top of your patch:
...
> +	( cd "$1" && for i in *; do echo "$i"; done )

Could we drop the "cd" and "echo" processes with this line instead?

	for i in "$1"/*; do printf "$i\n"; done

Thanks,
-Stolee

Re: [PATCH] sparse-checkout: improve OS ls compatibility

[Ed Maste]

On Thu, 19 Dec 2019 at 08:56, Derrick Stolee <stolee@gmail.com> wrote:
>
> On 12/18/2019 9:45 PM, Eric Wong wrote:
> > This goes on top of your patch:
> ...
> > +     ( cd "$1" && for i in *; do echo "$i"; done )
>
> Could we drop the "cd" and "echo" processes with this line instead?
>
>         for i in "$1"/*; do printf "$i\n"; done

That would output repo/a, but we could do something like:
for i in "$1"/*; do echo "${i#$1/}"; done

echo's a builtin on any /bin/sh I'm aware of - do you have a /bin/sh
with builtin printf but not echo?

Re: [PATCH] sparse-checkout: improve OS ls compatibility

[Derrick Stolee]

On 12/19/2019 11:15 AM, Ed Maste wrote:
> On Thu, 19 Dec 2019 at 08:56, Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 12/18/2019 9:45 PM, Eric Wong wrote:
>>> This goes on top of your patch:
>> ...
>>> +     ( cd "$1" && for i in *; do echo "$i"; done )
>>
>> Could we drop the "cd" and "echo" processes with this line instead?
>>
>>         for i in "$1"/*; do printf "$i\n"; done
> 
> That would output repo/a, but we could do something like:
> for i in "$1"/*; do echo "${i#$1/}"; done
> 
> echo's a builtin on any /bin/sh I'm aware of - do you have a /bin/sh
> with builtin printf but not echo?

I guess I am misremembering the benefits of printf over echo. Carry
on with your approach.

Thanks,
-Stolee

Re: [PATCH] sparse-checkout: improve OS ls compatibility

[Junio C Hamano]

Eric Wong <e@80x24.org> writes:

> But using a shell for loop seems doable, here, since there
> doesn't seem to be wonky characters.  I've done this in the past
> when I had to fix a system without "ls".
>
> This goes on top of your patch:
>
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index 3a3eafa653..a431d05643 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -6,7 +6,7 @@ test_description='sparse checkout builtin tests'
>  
>  ls_no_git()
>  {
> -	ls -1 "$1" | grep -v .git
> +	( cd "$1" && for i in *; do echo "$i"; done )
>  }

Hmph, my honest me is very tempted to say

 (1) don't run your tests as 'root', as that would break many tests
     with prerequisite SANITY

 (2) fix your "ls" to behave

but if you want to list paths that match shell glob *, this would do

	(cd "$1" && printf "%s\n *)

without any loop (other than the one printf gives us implicitly for
free), wouldn't it?

Note that the helper function's name no longer reflects what it does
with such a change, so it needs to be renamed.  Together with style
fix, perhaps

	ls_no_dot () {
		(cd "$1" && printf "%s\n *)
	}

is what we want, if somebody wants to keep using a broken /bin/ls?

Re: [PATCH] sparse-checkout: improve OS ls compatibility

[Ed Maste]

On Thu, 19 Dec 2019 at 13:11, Junio C Hamano <gitster@pobox.com> wrote:
>
> Hmph, my honest me is very tempted to say
>
>  (1) don't run your tests as 'root', as that would break many tests
>      with prerequisite SANITY

It looks like this is just Cirrus-CI's default without explicit
configuration or scripting to use an unprivileged user. Certainly
running the build and test as root is generally not a good idea, but
it is a purpose-built throwaway VM and so doesn't matter much. Anyhow,
it certainly needs to be addressed to avoid skipping the SANITY tests.

>  (2) fix your "ls" to behave

Well, given that hidden dot files were ostensibly a bug and BSD ls has
had this behaviour for over 40 years it's far too late to change. I
can see the rationale for showing all files for root, even though I
dislike the behaviour changing between privileged and unprivileged
users.

> but if you want to list paths that match shell glob *, this would do
>
>         (cd "$1" && printf "%s\n *)
>
> without any loop (other than the one printf gives us implicitly for
> free), wouldn't it?

Yes.

> Note that the helper function's name no longer reflects what it does
> with such a change, so it needs to be renamed.  Together with style
> fix, perhaps
>
>         ls_no_dot () {
>                 (cd "$1" && printf "%s\n *)
>         }
>
> is what we want,

I believe the tests should pass or be skipped when run as root, so I
think we should either require (something like) SANITY for these
tests, or make the change above. I'm happy with either option; I'll
send a v2 based on the approach above for consideration.

[PATCH v2] sparse-checkout: improve OS ls compatibility

[Ed Maste]

On FreeBSD, when executed by root ls enables the '-A' option:

  -A  Include directory entries whose names begin with a dot (`.')
      except for . and ...  Automatically set for the super-user unless
      -I is specified.

As a result the .git directory appeared in the output when run as root.
Simulate no-dotfile ls behaviour using a shell glob.

Signed-off-by: Ed Maste <emaste@FreeBSD.org>
Helped-by: Eric Wong <e@80x24.org>
Helped-by: Junio C Hamano <gitster@pobox.com>
---
 t/t1091-sparse-checkout-builtin.sh | 32 +++++++++++++++++-------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index cee98a1c8a..7e8cac679e 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -4,6 +4,10 @@ test_description='sparse checkout builtin tests'
 
 . ./test-lib.sh
 
+ls_no_dot() {
+	(cd "$1" && printf '%s\n' *)
+}
+
 test_expect_success 'setup' '
 	git init repo &&
 	(
@@ -50,7 +54,7 @@ test_expect_success 'git sparse-checkout init' '
 	EOF
 	test_cmp expect repo/.git/info/sparse-checkout &&
 	test_cmp_config -C repo true core.sparsecheckout &&
-	ls repo >dir  &&
+	ls_no_dot repo >dir  &&
 	echo a >expect &&
 	test_cmp expect dir
 '
@@ -73,7 +77,7 @@ test_expect_success 'init with existing sparse-checkout' '
 		*folder*
 	EOF
 	test_cmp expect repo/.git/info/sparse-checkout &&
-	ls repo >dir  &&
+	ls_no_dot repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		folder1
@@ -90,7 +94,7 @@ test_expect_success 'clone --sparse' '
 		!/*/
 	EOF
 	test_cmp expect actual &&
-	ls clone >dir &&
+	ls_no_dot clone >dir &&
 	echo a >expect &&
 	test_cmp expect dir
 '
@@ -119,7 +123,7 @@ test_expect_success 'set sparse-checkout using builtin' '
 	git -C repo sparse-checkout list >actual &&
 	test_cmp expect actual &&
 	test_cmp expect repo/.git/info/sparse-checkout &&
-	ls repo >dir  &&
+	ls_no_dot repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		folder1
@@ -139,7 +143,7 @@ test_expect_success 'set sparse-checkout using --stdin' '
 	git -C repo sparse-checkout list >actual &&
 	test_cmp expect actual &&
 	test_cmp expect repo/.git/info/sparse-checkout &&
-	ls repo >dir  &&
+	ls_no_dot repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		folder1
@@ -154,7 +158,7 @@ test_expect_success 'cone mode: match patterns' '
 	git -C repo read-tree -mu HEAD 2>err &&
 	test_i18ngrep ! "disabling cone patterns" err &&
 	git -C repo reset --hard &&
-	ls repo >dir  &&
+	ls_no_dot repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		folder1
@@ -177,7 +181,7 @@ test_expect_success 'sparse-checkout disable' '
 	test_path_is_file repo/.git/info/sparse-checkout &&
 	git -C repo config --list >config &&
 	test_must_fail git config core.sparseCheckout &&
-	ls repo >dir &&
+	ls_no_dot repo >dir &&
 	cat >expect <<-EOF &&
 		a
 		deep
@@ -191,24 +195,24 @@ test_expect_success 'cone mode: init and set' '
 	git -C repo sparse-checkout init --cone &&
 	git -C repo config --list >config &&
 	test_i18ngrep "core.sparsecheckoutcone=true" config &&
-	ls repo >dir  &&
+	ls_no_dot repo >dir  &&
 	echo a >expect &&
 	test_cmp expect dir &&
 	git -C repo sparse-checkout set deep/deeper1/deepest/ 2>err &&
 	test_must_be_empty err &&
-	ls repo >dir  &&
+	ls_no_dot repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		deep
 	EOF
 	test_cmp expect dir &&
-	ls repo/deep >dir  &&
+	ls_no_dot repo/deep >dir  &&
 	cat >expect <<-EOF &&
 		a
 		deeper1
 	EOF
 	test_cmp expect dir &&
-	ls repo/deep/deeper1 >dir  &&
+	ls_no_dot repo/deep/deeper1 >dir  &&
 	cat >expect <<-EOF &&
 		a
 		deepest
@@ -234,7 +238,7 @@ test_expect_success 'cone mode: init and set' '
 		folder1
 		folder2
 	EOF
-	ls repo >dir &&
+	ls_no_dot repo >dir &&
 	test_cmp expect dir
 '
 
@@ -256,7 +260,7 @@ test_expect_success 'revert to old sparse-checkout on bad update' '
 	test_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&
 	test_i18ngrep "cannot set sparse-checkout patterns" err &&
 	test_cmp repo/.git/info/sparse-checkout expect &&
-	ls repo/deep >dir &&
+	ls_no_dot repo/deep >dir &&
 	cat >expect <<-EOF &&
 		a
 		deeper1
@@ -313,7 +317,7 @@ test_expect_success 'cone mode: set with core.ignoreCase=true' '
 		/folder1/
 	EOF
 	test_cmp expect repo/.git/info/sparse-checkout &&
-	ls repo >dir &&
+	ls_no_dot repo >dir &&
 	cat >expect <<-EOF &&
 		a
 		folder1
-- 
2.24.0

Re: [PATCH] sparse-checkout: improve OS ls compatibility

[Junio C Hamano]

Ed Maste <emaste@freebsd.org> writes:

>> Note that the helper function's name no longer reflects what it does
>> with such a change, so it needs to be renamed.  Together with style
>> fix, perhaps
>>
>>         ls_no_dot () {
>>                 (cd "$1" && printf "%s\n *)
>>         }
>>
>> is what we want,
>
> I believe the tests should pass or be skipped when run as root, so I
> think we should either require (something like) SANITY for these
> tests, or make the change above. I'm happy with either option; I'll
> send a v2 based on the approach above for consideration.

OK, after thinking about it a bit more, I think "Your ls is broken"
was completely missing the point.  What we want in the callers of
this helper is to list the contents of a directory, and "ls" is one
possible (and easiest, if there were no "oops, sometimes -A is enabled
 implementation by default" complication) implementation.

And "ls_no_dot" is a misnomer from that point of view.  We are not
even using "ls", so perhaps we should just call it "list_files" or
something?

Thanks.

Re: [PATCH v2] sparse-checkout: improve OS ls compatibility

[Denton Liu]

Hi Ed,

On Thu, Dec 19, 2019 at 09:45:16PM +0000, Ed Maste wrote:
> On FreeBSD, when executed by root ls enables the '-A' option:
> 
>   -A  Include directory entries whose names begin with a dot (`.')
>       except for . and ...  Automatically set for the super-user unless
>       -I is specified.
> 
> As a result the .git directory appeared in the output when run as root.
> Simulate no-dotfile ls behaviour using a shell glob.
> 
> Signed-off-by: Ed Maste <emaste@FreeBSD.org>
> Helped-by: Eric Wong <e@80x24.org>
> Helped-by: Junio C Hamano <gitster@pobox.com>

Small nit: the Helped-by trailers should come before your sign-off.

Trailers should come in chronological order. Chronologically, they
helped you out with your patch and then, after that, you created your v2
based on their review and signed off on it.

Thanks,

Denton

[PATCH v3] sparse-checkout: improve OS ls compatibility

[Ed Maste]

On FreeBSD, when executed by root ls enables the '-A' option:

  -A  Include directory entries whose names begin with a dot (`.')
      except for . and ...  Automatically set for the super-user unless
      -I is specified.

As a result the .git directory appeared in the output when run as root.
Simulate no-dotfile ls behaviour using a shell glob.

Helped-by: Eric Wong <e@80x24.org>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ed Maste <emaste@FreeBSD.org>
---
Since v2, rename the function list_files and add a comment explaining why
it's not just using ls.  Note that this change is not necessary when running
the tests as an unprivileged user on FreeBSD, and the proposed FreeBSD CI
patch has been updated to do so.  That said I still believe we should make
either this change or add a prerequisite so this test does not run as root
on FreeBSD.

 t/t1091-sparse-checkout-builtin.sh | 35 ++++++++++++++++++------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index cee98a1c8a..168702784d 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -4,6 +4,13 @@ test_description='sparse checkout builtin tests'
 
 . ./test-lib.sh
 
+# List files in a directory, excluding hidden dot files (such as .git).
+# This is similar to ls, but some ls implementations include dot files by
+# default when run as root.
+list_files() {
+	(cd "$1" && printf '%s\n' *)
+}
+
 test_expect_success 'setup' '
 	git init repo &&
 	(
@@ -50,7 +57,7 @@ test_expect_success 'git sparse-checkout init' '
 	EOF
 	test_cmp expect repo/.git/info/sparse-checkout &&
 	test_cmp_config -C repo true core.sparsecheckout &&
-	ls repo >dir  &&
+	list_files repo >dir  &&
 	echo a >expect &&
 	test_cmp expect dir
 '
@@ -73,7 +80,7 @@ test_expect_success 'init with existing sparse-checkout' '
 		*folder*
 	EOF
 	test_cmp expect repo/.git/info/sparse-checkout &&
-	ls repo >dir  &&
+	list_files repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		folder1
@@ -90,7 +97,7 @@ test_expect_success 'clone --sparse' '
 		!/*/
 	EOF
 	test_cmp expect actual &&
-	ls clone >dir &&
+	list_files clone >dir &&
 	echo a >expect &&
 	test_cmp expect dir
 '
@@ -119,7 +126,7 @@ test_expect_success 'set sparse-checkout using builtin' '
 	git -C repo sparse-checkout list >actual &&
 	test_cmp expect actual &&
 	test_cmp expect repo/.git/info/sparse-checkout &&
-	ls repo >dir  &&
+	list_files repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		folder1
@@ -139,7 +146,7 @@ test_expect_success 'set sparse-checkout using --stdin' '
 	git -C repo sparse-checkout list >actual &&
 	test_cmp expect actual &&
 	test_cmp expect repo/.git/info/sparse-checkout &&
-	ls repo >dir  &&
+	list_files repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		folder1
@@ -154,7 +161,7 @@ test_expect_success 'cone mode: match patterns' '
 	git -C repo read-tree -mu HEAD 2>err &&
 	test_i18ngrep ! "disabling cone patterns" err &&
 	git -C repo reset --hard &&
-	ls repo >dir  &&
+	list_files repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		folder1
@@ -177,7 +184,7 @@ test_expect_success 'sparse-checkout disable' '
 	test_path_is_file repo/.git/info/sparse-checkout &&
 	git -C repo config --list >config &&
 	test_must_fail git config core.sparseCheckout &&
-	ls repo >dir &&
+	list_files repo >dir &&
 	cat >expect <<-EOF &&
 		a
 		deep
@@ -191,24 +198,24 @@ test_expect_success 'cone mode: init and set' '
 	git -C repo sparse-checkout init --cone &&
 	git -C repo config --list >config &&
 	test_i18ngrep "core.sparsecheckoutcone=true" config &&
-	ls repo >dir  &&
+	list_files repo >dir  &&
 	echo a >expect &&
 	test_cmp expect dir &&
 	git -C repo sparse-checkout set deep/deeper1/deepest/ 2>err &&
 	test_must_be_empty err &&
-	ls repo >dir  &&
+	list_files repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		deep
 	EOF
 	test_cmp expect dir &&
-	ls repo/deep >dir  &&
+	list_files repo/deep >dir  &&
 	cat >expect <<-EOF &&
 		a
 		deeper1
 	EOF
 	test_cmp expect dir &&
-	ls repo/deep/deeper1 >dir  &&
+	list_files repo/deep/deeper1 >dir  &&
 	cat >expect <<-EOF &&
 		a
 		deepest
@@ -234,7 +241,7 @@ test_expect_success 'cone mode: init and set' '
 		folder1
 		folder2
 	EOF
-	ls repo >dir &&
+	list_files repo >dir &&
 	test_cmp expect dir
 '
 
@@ -256,7 +263,7 @@ test_expect_success 'revert to old sparse-checkout on bad update' '
 	test_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&
 	test_i18ngrep "cannot set sparse-checkout patterns" err &&
 	test_cmp repo/.git/info/sparse-checkout expect &&
-	ls repo/deep >dir &&
+	list_files repo/deep >dir &&
 	cat >expect <<-EOF &&
 		a
 		deeper1
@@ -313,7 +320,7 @@ test_expect_success 'cone mode: set with core.ignoreCase=true' '
 		/folder1/
 	EOF
 	test_cmp expect repo/.git/info/sparse-checkout &&
-	ls repo >dir &&
+	list_files repo >dir &&
 	cat >expect <<-EOF &&
 		a
 		folder1
-- 
2.24.0

Re: [PATCH v3] sparse-checkout: improve OS ls compatibility

[Derrick Stolee]

On 12/20/2019 10:38 AM, Ed Maste wrote:
> On FreeBSD, when executed by root ls enables the '-A' option:
> 
>   -A  Include directory entries whose names begin with a dot (`.')
>       except for . and ...  Automatically set for the super-user unless
>       -I is specified.
> 
> As a result the .git directory appeared in the output when run as root.
> Simulate no-dotfile ls behaviour using a shell glob.

This patch looks good to me and seems to match where the
discussion landed. Thanks for finding and fixing this!

-Stolee

Re: [PATCH v3] sparse-checkout: improve OS ls compatibility

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> On 12/20/2019 10:38 AM, Ed Maste wrote:
>> On FreeBSD, when executed by root ls enables the '-A' option:
>> 
>>   -A  Include directory entries whose names begin with a dot (`.')
>>       except for . and ...  Automatically set for the super-user unless
>>       -I is specified.
>> 
>> As a result the .git directory appeared in the output when run as root.
>> Simulate no-dotfile ls behaviour using a shell glob.
>
> This patch looks good to me and seems to match where the
> discussion landed. Thanks for finding and fixing this!

Thanks, all.  Will queue.

Re: [PATCH v3] sparse-checkout: improve OS ls compatibility

[Eric Sunshine]

On Fri, Dec 20, 2019 at 10:38 AM Ed Maste <emaste@freebsd.org> wrote:
> On FreeBSD, when executed by root ls enables the '-A' option:
>
>   -A  Include directory entries whose names begin with a dot (`.')
>       except for . and ...  Automatically set for the super-user unless
>       -I is specified.
>
> As a result the .git directory appeared in the output when run as root.
> Simulate no-dotfile ls behaviour using a shell glob.
>
> Signed-off-by: Ed Maste <emaste@FreeBSD.org>
> ---
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> @@ -4,6 +4,13 @@ test_description='sparse checkout builtin tests'
> +# List files in a directory, excluding hidden dot files (such as .git).
> +# This is similar to ls, but some ls implementations include dot files by
> +# default when run as root.
> +list_files() {
> +       (cd "$1" && printf '%s\n' *)
> +}

Nit: While this may indeed be a case for which an explanatory comment
is justified, the comment itself is a bit lacking -- just enough to
keep it from being helpful for the next person who comes along to work
on this code. For instance, the too-abstract "some ls implementations"
doesn't provide enough information to point at a specific 'ls'
implementation if a person needs to do testing against one of these
implementations or wants to know if (say, several years from now) that
anomalous behavior is still present. It would be helpful, therefore,
to mention such an implementation by name:

    ...some 'ls' implementations, such as on FreeBSD, include...

(One can, of course, always argue that the commit message can be
consulted to learn about a particular 'ls' implementation, but then
why have an in-code comment at all?)

Re: [PATCH v3] sparse-checkout: improve OS ls compatibility

[Ed Maste]

On Fri, 20 Dec 2019 at 12:55, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> It would be helpful, therefore,
> to mention such an implementation by name:
>
>     ...some 'ls' implementations, such as on FreeBSD, include...
>
> (One can, of course, always argue that the commit message can be
> consulted to learn about a particular 'ls' implementation, but then
> why have an in-code comment at all?)

It could serve as a hint to check the commit history, but fair enough
- I agree including the specific example is an improvement. I can
resend if necessary but probably that change can just be folded in?

Re: [PATCH v3] sparse-checkout: improve OS ls compatibility

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> anomalous behavior is still present. It would be helpful, therefore,
> to mention such an implementation by name:
>
>     ...some 'ls' implementations, such as on FreeBSD, include...
>
> (One can, of course, always argue that the commit message can be
> consulted to learn about a particular 'ls' implementation, but then
> why have an in-code comment at all?)

"This is similar to ls" is not all that important, especially if we
then need to say how different from "ls" ours is.  The log message
that describes why we needed to move away from "ls" is a good place
to say what aspect of "ls" was unsuitable.

If we _were_ to add an in-code comment, we may want to say something
like

	# Do not replace this with "cd "$1" && ls", as FreeBSD "ls"
	# enables "-A" when run by root without being told, and ends
	# up including ".git" etc. in its output.

to warn future developers against improving and/or cleaning up.

Not that we encourage running our tests as root, though.  I am
slightly worried that the above phrasing might be taken as such.

Re: [PATCH v3] sparse-checkout: improve OS ls compatibility

[Eric Sunshine]

On Fri, Dec 20, 2019 at 1:21 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > anomalous behavior is still present. It would be helpful, therefore,
> > to mention such an implementation by name:
> >
> >     ...some 'ls' implementations, such as on FreeBSD, include...
> >
> If we _were_ to add an in-code comment, we may want to say something
> like
>
>         # Do not replace this with "cd "$1" && ls", as FreeBSD "ls"
>         # enables "-A" when run by root without being told, and ends
>         # up including ".git" etc. in its output.
>
> to warn future developers against improving and/or cleaning up.

I would find this comment more helpful than the existing one since it
spells out the issue precisely. A minor tweak:

    # Do not replace this with "cd "$1" && ls", as FreeBSD "ls"
    # enables "-A" by default when run by root, and ends up
    # including ".git" etc. in its output.

> Not that we encourage running our tests as root, though.  I am
> slightly worried that the above phrasing might be taken as such.

I'm not sure we really need to spell it out, but something like this
might allay that concern:

    # Do not replace this with "cd "$1" && ls", as FreeBSD "ls"
    # enables "-A" by default when run by root, and ends up
    # including ".git" etc. in its output. (Note, though, that
    # running the test suite as root is generally not
    # recommended.)

Re: [PATCH v3] sparse-checkout: improve OS ls compatibility

[Ed Maste]

On Fri, 20 Dec 2019 at 13:21, Junio C Hamano <gitster@pobox.com> wrote:
>
> "This is similar to ls" is not all that important, especially if we
> then need to say how different from "ls" ours is.  The log message
> that describes why we needed to move away from "ls" is a good place
> to say what aspect of "ls" was unsuitable.

Ok, I'm also happy if it goes in with no comment; the reason I added
it is I could foresee someone coming along in a few years, thinking
this is just a strange local implementation of ls, and changing it.
But, perhaps we can assume that any such person would check the
history before doing so and the comment is not needed.

> If we _were_ to add an in-code comment, we may want to say something
> like
>
>         # Do not replace this with "cd "$1" && ls", as FreeBSD "ls"
>         # enables "-A" when run by root without being told, and ends
>         # up including ".git" etc. in its output.
>
> to warn future developers against improving and/or cleaning up.

Indeed, that is more direct, although it's not just FreeBSD ls; this
came from 4.2BSD and is probably common to most/all non-GNU ls
implementations. In particular, macOS behaves the same way. (Also, the
replacement would be even simpler, just "ls $1".)

Re: [PATCH v3] sparse-checkout: improve OS ls compatibility

[Junio C Hamano]

Ed Maste <emaste@freebsd.org> writes:

> On Fri, 20 Dec 2019 at 13:21, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> "This is similar to ls" is not all that important, especially if we
>> then need to say how different from "ls" ours is.  The log message
>> that describes why we needed to move away from "ls" is a good place
>> to say what aspect of "ls" was unsuitable.
>
> Ok, I'm also happy if it goes in with no comment; the reason I added
> it is I could foresee someone coming along in a few years, thinking
> this is just a strange local implementation of ls, and changing it.
> But, perhaps we can assume that any such person would check the
> history before doing so and the comment is not needed.
>
>> If we _were_ to add an in-code comment, we may want to say something
>> like
>>
>>         # Do not replace this with "cd "$1" && ls", as FreeBSD "ls"
>>         # enables "-A" when run by root without being told, and ends
>>         # up including ".git" etc. in its output.
>>
>> to warn future developers against improving and/or cleaning up.
>
> Indeed, that is more direct, although it's not just FreeBSD ls; this
> came from 4.2BSD and is probably common to most/all non-GNU ls
> implementations. In particular, macOS behaves the same way. (Also, the
> replacement would be even simpler, just "ls $1".)

Good piece of info to include.  Final try for the day from me:

    # Do not replace this with 'ls "$1"', as "ls" with BSD-lineage
    # enables "-A" by default for root and ends up ...

Re: [PATCH v3] sparse-checkout: improve OS ls compatibility

[Eric Sunshine]

On Fri, Dec 20, 2019 at 2:23 PM Junio C Hamano <gitster@pobox.com> wrote:
> Ed Maste <emaste@freebsd.org> writes:
> > Ok, I'm also happy if it goes in with no comment; the reason I added
> > it is I could foresee someone coming along in a few years, thinking
> > this is just a strange local implementation of ls, and changing it.
> > But, perhaps we can assume that any such person would check the
> > history before doing so and the comment is not needed.

The in-code comment has sufficient value that I'd like to see it
remain since your concern about someone coming along and wanting to
replace the function with "ls" is a genuine one, and because it saves
people the trouble of having to dig through history in the first
place. And, by "people", I mean that it may save reviewers too since
patch submitters don't always dig through history or don't always
explain _why_ a change is a good idea or valid, which places the
burden on reviewers instead.

> > Indeed, that is more direct, although it's not just FreeBSD ls; this
> > came from 4.2BSD and is probably common to most/all non-GNU ls
> > implementations. In particular, macOS behaves the same way. (Also, the
> > replacement would be even simpler, just "ls $1".)
>
> Good piece of info to include.  Final try for the day from me:
>
>     # Do not replace this with 'ls "$1"', as "ls" with BSD-lineage
>     # enables "-A" by default for root and ends up ...

This looks good to me.

[PATCH v4] sparse-checkout: improve OS ls compatibility

[Ed Maste]

On FreeBSD, when executed by root ls enables the '-A' option:

  -A  Include directory entries whose names begin with a dot (`.')
      except for . and ...  Automatically set for the super-user unless
      -I is specified.

As a result the .git directory appeared in the output when run as root.
Simulate no-dotfile ls behaviour using a shell glob.

Helped-by: Eric Wong <e@80x24.org>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Ed Maste <emaste@FreeBSD.org>
---
Since v3, adjust the comment to be an explicit caution against replacing
with ls.

 t/t1091-sparse-checkout-builtin.sh | 36 ++++++++++++++++++------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index cee98a1c8a..6f7e2d0c9e 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -4,6 +4,14 @@ test_description='sparse checkout builtin tests'
 
 . ./test-lib.sh
 
+list_files() {
+	# Do not replace this with 'ls "$1"', as "ls" with BSD-lineage
+	# enables "-A" by default for root and ends up including ".git" and
+	# such in its output. (Note, though, that running the test suite as
+	# root is generally not recommended.)
+	(cd "$1" && printf '%s\n' *)
+}
+
 test_expect_success 'setup' '
 	git init repo &&
 	(
@@ -50,7 +58,7 @@ test_expect_success 'git sparse-checkout init' '
 	EOF
 	test_cmp expect repo/.git/info/sparse-checkout &&
 	test_cmp_config -C repo true core.sparsecheckout &&
-	ls repo >dir  &&
+	list_files repo >dir  &&
 	echo a >expect &&
 	test_cmp expect dir
 '
@@ -73,7 +81,7 @@ test_expect_success 'init with existing sparse-checkout' '
 		*folder*
 	EOF
 	test_cmp expect repo/.git/info/sparse-checkout &&
-	ls repo >dir  &&
+	list_files repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		folder1
@@ -90,7 +98,7 @@ test_expect_success 'clone --sparse' '
 		!/*/
 	EOF
 	test_cmp expect actual &&
-	ls clone >dir &&
+	list_files clone >dir &&
 	echo a >expect &&
 	test_cmp expect dir
 '
@@ -119,7 +127,7 @@ test_expect_success 'set sparse-checkout using builtin' '
 	git -C repo sparse-checkout list >actual &&
 	test_cmp expect actual &&
 	test_cmp expect repo/.git/info/sparse-checkout &&
-	ls repo >dir  &&
+	list_files repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		folder1
@@ -139,7 +147,7 @@ test_expect_success 'set sparse-checkout using --stdin' '
 	git -C repo sparse-checkout list >actual &&
 	test_cmp expect actual &&
 	test_cmp expect repo/.git/info/sparse-checkout &&
-	ls repo >dir  &&
+	list_files repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		folder1
@@ -154,7 +162,7 @@ test_expect_success 'cone mode: match patterns' '
 	git -C repo read-tree -mu HEAD 2>err &&
 	test_i18ngrep ! "disabling cone patterns" err &&
 	git -C repo reset --hard &&
-	ls repo >dir  &&
+	list_files repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		folder1
@@ -177,7 +185,7 @@ test_expect_success 'sparse-checkout disable' '
 	test_path_is_file repo/.git/info/sparse-checkout &&
 	git -C repo config --list >config &&
 	test_must_fail git config core.sparseCheckout &&
-	ls repo >dir &&
+	list_files repo >dir &&
 	cat >expect <<-EOF &&
 		a
 		deep
@@ -191,24 +199,24 @@ test_expect_success 'cone mode: init and set' '
 	git -C repo sparse-checkout init --cone &&
 	git -C repo config --list >config &&
 	test_i18ngrep "core.sparsecheckoutcone=true" config &&
-	ls repo >dir  &&
+	list_files repo >dir  &&
 	echo a >expect &&
 	test_cmp expect dir &&
 	git -C repo sparse-checkout set deep/deeper1/deepest/ 2>err &&
 	test_must_be_empty err &&
-	ls repo >dir  &&
+	list_files repo >dir  &&
 	cat >expect <<-EOF &&
 		a
 		deep
 	EOF
 	test_cmp expect dir &&
-	ls repo/deep >dir  &&
+	list_files repo/deep >dir  &&
 	cat >expect <<-EOF &&
 		a
 		deeper1
 	EOF
 	test_cmp expect dir &&
-	ls repo/deep/deeper1 >dir  &&
+	list_files repo/deep/deeper1 >dir  &&
 	cat >expect <<-EOF &&
 		a
 		deepest
@@ -234,7 +242,7 @@ test_expect_success 'cone mode: init and set' '
 		folder1
 		folder2
 	EOF
-	ls repo >dir &&
+	list_files repo >dir &&
 	test_cmp expect dir
 '
 
@@ -256,7 +264,7 @@ test_expect_success 'revert to old sparse-checkout on bad update' '
 	test_must_fail git -C repo sparse-checkout set deep/deeper1 2>err &&
 	test_i18ngrep "cannot set sparse-checkout patterns" err &&
 	test_cmp repo/.git/info/sparse-checkout expect &&
-	ls repo/deep >dir &&
+	list_files repo/deep >dir &&
 	cat >expect <<-EOF &&
 		a
 		deeper1
@@ -313,7 +321,7 @@ test_expect_success 'cone mode: set with core.ignoreCase=true' '
 		/folder1/
 	EOF
 	test_cmp expect repo/.git/info/sparse-checkout &&
-	ls repo >dir &&
+	list_files repo >dir &&
 	cat >expect <<-EOF &&
 		a
 		folder1
-- 
2.24.0