| Message ID | 20191219015833.49314-1-emaste@FreeBSD.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sparse-checkout: improve OS ls compatibility | expand |
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 >
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.
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
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' '
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
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?
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
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?
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.
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.
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
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(-)