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