Skip to content

Commit

Permalink
Avoid throwing an exception when sorting identical paths (#90)
Browse files Browse the repository at this point in the history
Fixes #44

The basic issue is that our loops `i` is being incremented and fed into `getSegment` before the out-of-bounds check. Thus if two paths are identical, it ends up overshooting the end of the sequence of segments, resulting in the error reported. The fix is to order the `i += 1` such that it takes place immediately before the out-of-bounds check, to ensure it is always in bounds when we pass it to `getSegment`

The `i == xSegCount` check at the bottom was also incorrect. Just because we've hit the end of the list doesn't mean the two paths are equal, as the point of the loop is to find the first non-equal string segment so we can compare them. Thus we should compare the two string segments every time: if they are equal and we haven't run out, we loop another time, otherwise we return the result of the comparison

Adds a test case that reproduces the failure on master, passes on this PR

Review by @lefou
  • Loading branch information
lihaoyi authored Dec 10, 2021
1 parent 460f8e8 commit b43111f
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 8 deletions.
16 changes: 11 additions & 5 deletions os/src/Path.scala
Original file line number Diff line number Diff line change
Expand Up @@ -408,15 +408,21 @@ object Path {
else{
var xSeg = ""
var ySeg = ""
var i = -1
var i = 0
var result: Integer = null
while ({
i += 1
xSeg = x.getSegment(i)
ySeg = y.getSegment(i)
i < xSegCount && xSeg == ySeg
i += 1
val compared = Ordering.String.compare(xSeg, ySeg)
if (i < xSegCount && compared == 0) true // continue
else {
result = compared
false
}
}) ()
if (i == xSegCount) 0
else Ordering.String.compare(xSeg, ySeg)

result
}
}
}
Expand Down
17 changes: 14 additions & 3 deletions os/test/src/PathTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -327,10 +327,21 @@ object PathTests extends TestSuite{
}
}
test("sorting"){
assert(
Seq(root/"c", root, root/"b", root/"a").sorted == Seq(root, root/"a", root/"b", root/"c"),
test - {
assert(
Seq(root/"c", root, root/"b", root/"a").sorted ==
Seq(root, root/"a", root/"b", root/"c")
)
}

test - assert(
Seq(up/"c", up/up/"c", rel/"b"/"c", rel/"a"/"c", rel/"a"/"d").sorted ==
Seq(rel/"a"/"c", rel/"a"/"d", rel/"b"/"c", up/"c", up/up/"c")
Seq(rel/"a"/"c", rel/"a"/"d", rel/"b"/"c", up/"c", up/up/"c")
)

test - assert(
Seq(os.root / "yo", os.root / "yo").sorted ==
Seq(os.root / "yo", os.root / "yo")
)
}
test("construction"){
Expand Down

0 comments on commit b43111f

Please sign in to comment.