Re: Old --sort patch revisited

[Michael A Stevens]

On Thu, 3 May 2007, Eric Blake wrote:

> Of the files in your patch, only the changes to ls.c are needed; the files
> in man/ and po/ are generated files (either by help2man or by the
> translation project), so patching them directly is not required and only
> makes it harder to see the real patch.  Patches should also include a
> ChangeLog entry that summarize the changes, and be generated against the
> development head (CVS or git).

I was stabbing at the .po files by makeing distclean but the .po files 
were still in doc/. After doing a ./configure to rebuild doc/Makefile I 
made clean and then rebuilt the docs. I left them in the patch because 
they were there on a distclean, perhaps I didn't note the proper clean 
target. Is there another target besides distclean that also cleans the 
built docs? I'll take note about the Changelog and CVS for anything in 
the future.

> >      sort_time,                 /* -t */
> > +    sort_ctime,                        /* -c */
> > +    sort_atime,                        /* -u */
>
> Why is this necessary?  It looks like you are trying to support:
>
> ls --sort=ctime

Exactly.

> but isn't that already possible with:
>
> ls --sort=time --time=ctime

It works, but after having read the old docs before reading the code 
--sort=atime seems like the better way to do this as a long option. 
Reading how the code structures time as an index modifier makes me see 
your point though.

> Or was your intent to make it possible to sort by ctime but still display
> atime, as in:
>
> ls -l --sort=ctime --time=atime

This was not my intent, but perhaps there is a use for that. The other 
part that seems a little ugly is where the number of sort functions is 
verified. I just removed 2 for the two bogus sort options I added but it 
still doesn't seem to flow right with how the other enums are structured. 
Perhaps someone has a suggestion on how to better implement this change.

> although that seems somewhat confusing.
>
> If this patch is accepted, you would also need to update coreutils.texi
> and NEWS, as well as the testsuite.

If its actually a worthwhile change to the functionality I'll make sure to 
do that.


Mike