Skip to content

Fix path sort comparator to be children-first and add test - #9730

Merged
ebarboni merged 1 commit into
apache:deliveryfrom
mbien:path-component-comparator
Oct 21, 2026
Merged

ebarboni merged 1 commit into
apache:deliveryfrom
mbien:path-component-comparator

Conversation

@mbien

@mbien mbien commented Oct 18, 2026 •

Copy link
Copy Markdown
Member

the path sorting indentation requires child paths to follow right after their parent. But this isn't the case when simply sorting by String.

I actually had that feeling that this wouldn't work while looking at the PR #9602 (comment), but I couldn't come up with an example at that time. PR was also merged without full review.

added test

note:

Moved @RandomlyFails annotation from test class to test methods, otherwise the new testcase would be running.

fixes #9727

targets delivery

the path sorting indentation requires child paths to follow right
after their parent. But this isn't the case when simply sorting by
String.

added test

note: 

Moved RandomlyFails annotation from test class to test methods,
otherwise the testcase would not run.
@mbien mbien added Editor ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Oct 18, 2026
@mbien mbien linked an issue Oct 18, 2026 that may be closed by this pull request
@mbien

mbien commented Oct 18, 2026

Copy link
Copy Markdown
Member Author

usually we would revert changes if they don't work as expected. but this is probably small enough to fix during RC. ("famous last words")

@mbien mbien added this to the NB32 milestone Oct 18, 2026

@matthiasblaesing matthiasblaesing left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only eyeballed this, but the approach looks sane to me.

@neilcsmith-net neilcsmith-net left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly eyeballed - looks sane. Thanks! Perils of merging things too quickly.

I was curious if sorting with Path was any better, but also seems to sort just by text value without consideration of depth.

Post-release might be worth considering if the comparators should be in FileUtil so they can be used anywhere else that might need this.

@mbien

mbien commented Oct 19, 2026 •

Copy link
Copy Markdown
Member Author

using Path's comparator directly has the same effect with the potential to be worse: since if Paths are on different filesystem impls the behavior is undefined. (and undefined sorting can throw exceptions in the sort alg - e.g like back when the installer crashed on windows due to weird shell folder File instances)

what I initially tried though is to create path anyway, just to iterate through the components -> cleaner code. However this breaks on windows since normalized / separated String piped into Path.get() would fail. So this might be the best option atm.

@ebarboni
ebarboni merged commit f7d3553 into apache:delivery Oct 21, 2026
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) Editor Project UI View

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Project path indentaiton via String sorted paths showes some issues

4 participants