Re: [PATCH] ls: add --sort=width (-W) option to sort by filename width

[Pádraig Brady]

On 11/04/2021 17:16, Jim Meyering wrote:
> On Sun, Apr 11, 2021 at 9:10 AM Jim Meyering <jim@meyering.net> wrote:
> > On Sat, Apr 10, 2021 at 8:22 PM Pádraig Brady <P@draigbrady.com> wrote:
> > > On 09/04/2021 23:51, Pádraig Brady wrote:
> > > > On 09/04/2021 13:02, Carl Edquist wrote:
> > > > > Dear Coreutils Maintainers,
> > > > >
> > > > > I'd like to introduce my favorite 'ls' option, '-W', which I have been
> > > > > enjoying using regularly over the last few years.
> > > > >
> > > > > The concept is just to sort filenames by their printed widths.
> > > > >
> > > > >
> > > > > (If this sounds odd, I invite you hear it out, try and see for yourself!)
> > > > >
> > > > >
> > > > > I am including a patch with my implementation and accompanying tests - as
> > > > > well as some sample output.  And I'll happily field any requests for
> > > > > improvements.
> > > >
> > > > I quite like this. It seems useful.
> > > > Also doing outside of ls is quite awkward,
> > > > especially considering multi column output.
> > > >
> > > > I would avoid the -W short option, as that would clash with ls from FreeBSD for 
> > > > example.
> > > > It's probably best to not provide a short option for this at all.
> > >
> > > Playing around with this a bit more,
> > > I really like how much more concise it makes output in general.
> > >
> > > I've attached two patches which I'll apply tomorrow at some stage.
> > >
> > > The first adjusts your patch to:
> > >     Remove -W short option.
> > >     Fix crash on systems with statx().
> > >     s/filename/file name/.
> > >     Expand in texinfo that --sort=width is most useful with -C (the default)
> > >
> > > The second is a performance improvement,
> > > as we're now calling quote_name_width a lot more.
> >
> > Nice. I like this new option, too! Who would have thought :-)
> >
> > Two minor stylistic suggestions:
> >
> > move this declaration of "i" down into the loop where it's used, to
> > save two lines:
> >
> > +static void
> > +update_current_files_cache (void)
> > +{
> > +  size_t i;
> > +
> > +  for (i = 0; i < cwd_n_used; i++)
> >
> > And please use "char const *" (not const char *) in the added code of
> > ls.c. Admittedly this is a really small nit, especially since the
> > existing code in that file is not yet self-consistent. But at least
> > the preferred spelling outnumbers the others by about 3 to 1.
>
> Oops. I see that you've pushed already.

I'd already adjusted that function a little before the push,
to pull the invariant outside of the for loop.

> I'll adjust if you don't beat me to it again. Thanks!

The changes didn't introduce any new instances
of that const declaration style (apart from the new forward declaration),
but you're right we should be consistent here.
This seems like the perfect thing for a syntax check,
so I added a simple one as follows:

+# Prefer the const declaration form, with const following the type
+sc_prohibit-const-char:
+       @prohibit='const char \*'                               \
+       in_vc_files='\.[ch]$$'                                  \
+       halt='Use char const *, not const char *'               \
+         $(_sc_search_regexp)

I adjusted all files to pass and pushed.

cheers,
Pádraig