Skip to content

Commit b43111f

Browse files
authored
Avoid throwing an exception when sorting identical paths (#90)
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
1 parent 460f8e8 commit b43111f

File tree

2 files changed

+25
-8
lines changed

2 files changed

+25
-8
lines changed

os/src/Path.scala

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -408,15 +408,21 @@ object Path {
408408
else{
409409
var xSeg = ""
410410
var ySeg = ""
411-
var i = -1
411+
var i = 0
412+
var result: Integer = null
412413
while ({
413-
i += 1
414414
xSeg = x.getSegment(i)
415415
ySeg = y.getSegment(i)
416-
i < xSegCount && xSeg == ySeg
416+
i += 1
417+
val compared = Ordering.String.compare(xSeg, ySeg)
418+
if (i < xSegCount && compared == 0) true // continue
419+
else {
420+
result = compared
421+
false
422+
}
417423
}) ()
418-
if (i == xSegCount) 0
419-
else Ordering.String.compare(xSeg, ySeg)
424+
425+
result
420426
}
421427
}
422428
}

os/test/src/PathTests.scala

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -327,10 +327,21 @@ object PathTests extends TestSuite{
327327
}
328328
}
329329
test("sorting"){
330-
assert(
331-
Seq(root/"c", root, root/"b", root/"a").sorted == Seq(root, root/"a", root/"b", root/"c"),
330+
test - {
331+
assert(
332+
Seq(root/"c", root, root/"b", root/"a").sorted ==
333+
Seq(root, root/"a", root/"b", root/"c")
334+
)
335+
}
336+
337+
test - assert(
332338
Seq(up/"c", up/up/"c", rel/"b"/"c", rel/"a"/"c", rel/"a"/"d").sorted ==
333-
Seq(rel/"a"/"c", rel/"a"/"d", rel/"b"/"c", up/"c", up/up/"c")
339+
Seq(rel/"a"/"c", rel/"a"/"d", rel/"b"/"c", up/"c", up/up/"c")
340+
)
341+
342+
test - assert(
343+
Seq(os.root / "yo", os.root / "yo").sorted ==
344+
Seq(os.root / "yo", os.root / "yo")
334345
)
335346
}
336347
test("construction"){

0 commit comments

Comments
 (0)