Skip to content

Commit 905d1e3

Browse files
Fix prefix_sort ordering (#1469)
prefix_sort was using a sorting function that required, since we updated rust, a total ordering. In fact prefix_sort should reorder alphabetically, while preserving the order of filters that share a prefix as input or output directory. This commits replaces the function so that it does so, preventing crashes in cases where the ordering was revealed to not be total. Co-authored-by: Louis-Marie Givel <[email protected]>
1 parent f91135f commit 905d1e3

File tree

1 file changed

+72
-12
lines changed

1 file changed

+72
-12
lines changed

josh-core/src/filter/opt.rs

Lines changed: 72 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@
44
*/
55

66
use super::*;
7+
use std::cmp::Ordering;
8+
use std::collections::HashMap;
9+
use std::collections::VecDeque;
710

811
lazy_static! {
912
static ref OPTIMIZED: std::sync::Mutex<std::collections::HashMap<Filter, Filter>> =
@@ -197,21 +200,78 @@ fn last_chain(rest: Filter, filter: Filter) -> (Filter, Filter) {
197200
}
198201
}
199202

200-
fn prefix_sort(filters: &[Filter]) -> Vec<Filter> {
201-
let mut sorted = filters.to_owned();
202-
sorted.sort_by(|a, b| {
203-
let (src_a, src_b) = (src_path(*a), src_path(*b));
204-
if src_a.starts_with(&src_b) || src_b.starts_with(&src_a) {
205-
return std::cmp::Ordering::Equal;
206-
}
207-
let (dst_a, dst_b) = (dst_path(*a), dst_path(*b));
208-
if dst_a.starts_with(&dst_b) || dst_b.starts_with(&dst_a) {
209-
return std::cmp::Ordering::Equal;
203+
pub fn prefix_sort(filters: &[Filter]) -> Vec<Filter> {
204+
let n = filters.len();
205+
206+
// Step 1: Build graph of ordering constraints
207+
let mut graph: HashMap<usize, Vec<usize>> = HashMap::new();
208+
let mut indegree = vec![0; n];
209+
210+
for i in 0..n {
211+
for j in i + 1..n {
212+
let src_i = src_path(filters[i].clone());
213+
let dst_i = dst_path(filters[i].clone());
214+
let src_j = src_path(filters[j].clone());
215+
let dst_j = dst_path(filters[j].clone());
216+
217+
let constraint = if src_j.starts_with(&src_i) || src_i.starts_with(&src_j) {
218+
Some((i, j))
219+
} else if dst_j.starts_with(&dst_i) || dst_i.starts_with(&dst_j) {
220+
Some((i, j))
221+
} else {
222+
None
223+
};
224+
225+
if let Some((a, b)) = constraint {
226+
graph.entry(a).or_default().push(b);
227+
indegree[b] += 1;
228+
}
210229
}
230+
}
211231

212-
(&src_a, &dst_a).partial_cmp(&(&src_b, &dst_b)).unwrap()
232+
// Step 2: Sort indices alphabetically by (src, dst)
233+
let mut indices: Vec<usize> = (0..n).collect();
234+
indices.sort_by(|&i, &j| {
235+
let key_i = (src_path(filters[i].clone()), dst_path(filters[i].clone()));
236+
let key_j = (src_path(filters[j].clone()), dst_path(filters[j].clone()));
237+
238+
match key_i.0.cmp(&key_j.0) {
239+
Ordering::Equal => key_i.1.cmp(&key_j.1),
240+
other => other,
241+
}
213242
});
214-
sorted
243+
244+
// Step 3: Topological sort with alphabetical tie-break
245+
let mut result = Vec::new();
246+
let mut available: VecDeque<usize> = indices
247+
.iter()
248+
.copied()
249+
.filter(|&i| indegree[i] == 0)
250+
.collect();
251+
252+
while let Some(i) = available.pop_front() {
253+
result.push(i);
254+
if let Some(neighbors) = graph.get(&i) {
255+
for &j in neighbors {
256+
indegree[j] -= 1;
257+
if indegree[j] == 0 {
258+
// Insert j into available, keeping alphabetical order
259+
let pos = available.iter().position(|&x| {
260+
let key_j = (src_path(filters[j].clone()), dst_path(filters[j].clone()));
261+
let key_x = (src_path(filters[x].clone()), dst_path(filters[x].clone()));
262+
key_j < key_x
263+
});
264+
if let Some(p) = pos {
265+
available.insert(p, j);
266+
} else {
267+
available.push_back(j);
268+
}
269+
}
270+
}
271+
}
272+
}
273+
274+
result.into_iter().map(|i| filters[i].clone()).collect()
215275
}
216276

217277
fn common_pre(filters: &Vec<Filter>) -> Option<(Filter, Vec<Filter>)> {

0 commit comments

Comments
 (0)