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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 20, 2019, 19:33 UTC
Message-ID: <CAPig+cR64QkBJ8ybMkMiX2CvrgajcGMyG41SMt4mA-VzHyae=A@mail.gmail.com>
In-Reply-To: <xmqq7e2qajix.fsf@gitster-ct.c.googlers.com>

On Fri, Dec 20, 2019 at 2:23 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

> 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.

Show 9 quoted lines

> > 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.