[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Old --sort patch revisited

From: Michael A Stevens
Subject: Re: Old --sort patch revisited
Date: Thu, 3 May 2007 23:44:33 -0400 (EDT)

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


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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]