8000 uucore: leading zeros are ignored in version compare by shinhs0506 · Pull Request #5013 · uutils/coreutils · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

uucore: leading zeros are ignored in version compare #5013

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 9, 2023
Merged

uucore: leading zeros are ignored in version compare #5013

merged 3 commits into from
Jul 9, 2023

Conversation

shinhs0506
Copy link
Contributor

should fix tests/misc/sort-version.sh

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/misc/sort-version is no longer failing!

@tertsdiepraam
Copy link
Member

Have you seen the previous discussion on this? In particular this thread: #4325

@shinhs0506
Copy link
Contributor Author

@tertsdiepraam Ah, i totally missed that. thanks for pointing it out.

I was using this as a reference for the non-stable sorting. And i guess stable sorting just preserves the relative order in case when the values are equal?

@tertsdiepraam
Copy link
Member

Indeed. I'm not sure how intentional that is on GNU's side. We could start a discussion there too, because it is (in my opinion) a bit strange.

@shinhs0506
Copy link
Contributor Author

they do specify --stable here. Would make sense if they wanted something deterministic.

@tertsdiepraam
Copy link
Member
tertsdiepraam commented Jun 27, 2023 •
edited
Loading

Interesting, I hadn't seen that sentence. That makes sense if it's about the last resort comparison. We should do the same then!

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/misc/sort-version is no longer failing!

@shinhs0506
Copy link
Contributor Author

@tertsdiepraam should be ready for review

@tertsdiepraam
Copy link
Member

This does not fix the issue for both stable and unstable does it? I would imagine that version_cmp needs to stake a stable flag that defines the behaviour. Or do you think it should he fixed in some other way?

@shinhs0506
Copy link
Contributor Author

In my opinion, the comparison function should return Cmp::Ordering::Equals if they are equal. And the Caller should handle the equal case as it is done here

And i believe this pr fixes both stable and unstable case

@tertsdiepraam
Copy link
Member
tertsdiepraam commented Jul 2, 2023 •
edited
Loading

Returning equal does make sense. How does this solve both cases, though?

Edit: Let me clarify. sort does the right thing now, but ls doesn't do the last resort comparison, so it would be broken now right?

@shinhs0506
Copy link
Contributor Author

I can't find anything on how ls breaks ties. Nothing will necessarily be 'broken' because it would still sort correctly in a non-deterministic way which makes sense because ls doesn't do last resort comparison as you said.

If we wanted to provide our own rules, and make ls behave similar to sort, that would be a reasonable approach too.

@tertsdiepraam
Copy link
Member

Yeah not really broken, but it would be something we might need to fix. That could just mean that we need to open an issue, but we do need to know. Could you do some testing with ls with GNU and uutils before and after this patch?

@shinhs0506
Copy link
Contributor Author

I did some manual testing and looks like GNU's ls also behaves like sort when breaking ties.

@shinhs0506 shinhs0506 mentioned this pull request Jul 4, 2023
Merged
@github-actions
Copy link
github-actions bot commented Jul 5, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/misc/sort-version is no longer failing!

@sylvestre
Copy link
Contributor

Thanks :)
Still an improvement over the previous state :)

@sylvestre sylvestre merged commit 78fd0ef into uutils:main Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shinhs0506 @tertsdiepraam @sylvestre
0