New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ordering of ls -t, which was backwards #1057

Merged
merged 1 commit into from Jul 27, 2017

Conversation

Projects
None yet
3 participants
@ids1024
Member

ids1024 commented Jul 25, 2017

No description provided.

@ebfe

This comment has been minimized.

Member

ebfe commented Jul 26, 2017

Now -S is wrong?

@ids1024

This comment has been minimized.

Member

ids1024 commented Jul 26, 2017

Oops; yes, I hadn't noticed that this code handles multiple kinds of sort.

@ids1024

This comment has been minimized.

Member

ids1024 commented Jul 26, 2017

It should be correct now.

@@ -196,6 +196,7 @@ fn sort_entries(entries: &mut Vec<PathBuf>, options: &getopts::Matches) {
.unwrap_or(std::time::UNIX_EPOCH)
});
}
entries.reverse(); // Newest first

This comment has been minimized.

@Arcterus

Arcterus Jul 27, 2017

Member

Perhaps it makes more sense to change sort_by_key above into:

entries.sort_by(|a, b| {
    get_mod_time(b, options).cmp(get_mod_time(a, options)))
});

where get_mod_time() is defined as:

fn get_mod_time(path: &PathBuf, options: &getopts::Matches) -> time::SystemTime {
    get_metadata(path, options)
        .and_then(|md| md.modified())
        .unwrap_or(std::time::UNIX_EPOCH)
}

That way we don't have to reverse the vector. Of course, you would also have to define a similar get_create_time method or something to that effect for Unix.

This comment has been minimized.

@ids1024

ids1024 Jul 27, 2017

Member

Good point; it seems the standard library has a neat struct for reversing sorts, so I used that.

@@ -191,9 +192,10 @@ fn sort_entries(entries: &mut Vec<PathBuf>, options: &getopts::Matches) {
entries.sort_by_key(|k| get_metadata(k, options).map(|md| md.ctime()).unwrap_or(0));
} else {
entries.sort_by_key(|k| {
get_metadata(k, options)
// Newest first
Reverse(get_metadata(k, options)

This comment has been minimized.

@Arcterus

Arcterus Jul 27, 2017

Member

I didn't even know this was a thing lmao

@Arcterus Arcterus merged commit 4a5b76f into uutils:master Jul 27, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -191,9 +192,10 @@ fn sort_entries(entries: &mut Vec<PathBuf>, options: &getopts::Matches) {
entries.sort_by_key(|k| get_metadata(k, options).map(|md| md.ctime()).unwrap_or(0));

This comment has been minimized.

@Arcterus

Arcterus Jul 27, 2017

Member

Oh wait, I may have merged this too quickly. Does this need to be reversed as well?

This comment has been minimized.

@ids1024

ids1024 Jul 27, 2017

Member

Oh yes, I suppose it does.

Arcterus added a commit to Arcterus/coreutils that referenced this pull request Jul 27, 2017

cnd added a commit that referenced this pull request Jul 27, 2017

Merge pull request #1061 from Arcterus/ls-fix
ls: fix line that was forgotten in #1057
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment