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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 20, 2019, 17:55 UTC
Message-ID: <CAPig+cS6XPc9KZo3ytEHLFjMxEFqCk5OJMUjZyFBP0cA95u9Lw@mail.gmail.com>
In-Reply-To: <20191220153814.54899-1-emaste@FreeBSD.org>

On Fri, Dec 20, 2019 at 10:38 AM Ed Maste <emaste@freebsd.org> wrote:

Show 19 quoted lines

> 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?)