Test for both EOS conditions before considering periods, so as to make shorter strings compare "less".
- Group Reviewers
- Maniphest Tasks
- T72878: Outliner: Alphabetical sorting sorts shorter names last.
Thanks for this! While reviewing this I smelled a bug, but also found that it's very hard to keep track of all the (corner) cases and how the function will handle them.
So I decided it's best to add some unit tests for this (rB525b0e0ccb08). These confirmed my suspicion: Case sensitive comparison failed with these changes here.
This issue would be easy to fix I think. I also wanted to do some slight adjustments to your code based on personal taste.
So committed my own fix which is based on this, see rBf52d9a878de9.
Thanks again, saved me some time!
We usually use XXX for issues that require bigger refactoring. For me this is a little annoyance, but not much of a real issue. So while it's good to hint as this fact, let's not make it look scarier than it is ;)
This misses case sensitive comparison. E.g. inputting "A", and "a" would return 0, but should return -1.
This is a case-insensitive compare. Or that's what the name of this function, and the comment before it, state. That's why I removed the strcmp in the first place.
I mean, the presence of that strcmp gave me pause as well, but then the function is commented as case-insensitive and relies on tolower; leaving it like this (with strcmp) is, in fact, a bug, as it would only do case-sensitive comparison of full string, but will happily early out with case-insensitive comparison if the numbers compare not equal, or if there's a '.'.