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