From faf1032816546d298b584b1f35af2d87aed390cd Mon Sep 17 00:00:00 2001 From: Christian Schilling Date: Tue, 16 Dec 2025 19:23:17 +0100 Subject: [PATCH] Refactor chain to use a vec It was discovered during profiling that filter optimisation spends lots of time rearranging chains. Using a vec will make operations like finding the last element in a chain much faster. --- josh-core/src/build.rs | 2 +- josh-core/src/filter/mod.rs | 113 ++++++++----- josh-core/src/filter/op.rs | 2 +- josh-core/src/filter/opt.rs | 291 ++++++++++++++++++++++++-------- josh-core/src/filter/persist.rs | 39 ++--- josh-core/src/flang/mod.rs | 69 +++++--- josh-core/src/flang/parse.rs | 17 +- tests/filter/file.t | 2 +- tests/filter/filter_id.t | 106 ++++++------ tests/filter/scope_filter.t | 110 ++++++------ tests/proxy/clone_with_meta.t | 2 +- tests/proxy/workspace_errors.t | 4 +- 12 files changed, 475 insertions(+), 282 deletions(-) diff --git a/josh-core/src/build.rs b/josh-core/src/build.rs index 36ec0c8cb..390501dbb 100644 --- a/josh-core/src/build.rs +++ b/josh-core/src/build.rs @@ -44,7 +44,7 @@ pub fn squash(ids: Option<&[(git2::Oid, Filter)]>) -> Filter { /// Create a filter that is the result of feeding the output of `first` into `second` pub fn chain(first: Filter, second: Filter) -> Filter { - opt::optimize(to_filter(Op::Chain(first, second))) + opt::optimize(to_filter(Op::Chain(vec![first, second]))) } /// Create a filter that is the result of overlaying the output of `first` onto `second` diff --git a/josh-core/src/filter/mod.rs b/josh-core/src/filter/mod.rs index 85e04f0d0..edf7de231 100644 --- a/josh-core/src/filter/mod.rs +++ b/josh-core/src/filter/mod.rs @@ -222,9 +222,11 @@ fn lazy_refs2(op: &Op) -> Vec { }) } Op::Exclude(filter) | Op::Pin(filter) => lazy_refs(*filter), - Op::Chain(a, b) => { - let mut av = lazy_refs(*a); - av.append(&mut lazy_refs(*b)); + Op::Chain(filters) => { + let mut av = vec![]; + for filter in filters { + av.append(&mut lazy_refs(*filter)); + } av } Op::Subtract(a, b) => { @@ -282,7 +284,7 @@ fn resolve_refs2(refs: &std::collections::HashMap, op: &Op) - } Op::Exclude(filter) => Op::Exclude(resolve_refs(refs, *filter)), Op::Pin(filter) => Op::Pin(resolve_refs(refs, *filter)), - Op::Chain(a, b) => Op::Chain(resolve_refs(refs, *a), resolve_refs(refs, *b)), + Op::Chain(filters) => Op::Chain(filters.iter().map(|f| resolve_refs(refs, *f)).collect()), Op::Subtract(a, b) => Op::Subtract(resolve_refs(refs, *a), resolve_refs(refs, *b)), Op::Rev(filters) => { let lr = filters @@ -344,7 +346,9 @@ fn src_path2(op: &Op) -> std::path::PathBuf { normalize_path(&match op { Op::Subdir(path) => path.to_owned(), Op::File(_, source_path) => source_path.to_owned(), - Op::Chain(a, b) => src_path(*a).join(src_path(*b)), + Op::Chain(filters) => filters + .iter() + .fold(std::path::PathBuf::new(), |acc, f| acc.join(src_path(*f))), _ => std::path::PathBuf::new(), }) } @@ -357,7 +361,10 @@ fn dst_path2(op: &Op) -> std::path::PathBuf { normalize_path(&match op { Op::Prefix(path) => path.to_owned(), Op::File(dest_path, _) => dest_path.to_owned(), - Op::Chain(a, b) => dst_path(*b).join(dst_path(*a)), + Op::Chain(filters) => filters + .iter() + .rev() + .fold(std::path::PathBuf::new(), |acc, f| acc.join(dst_path(*f))), _ => std::path::PathBuf::new(), }) } @@ -497,15 +504,22 @@ fn apply_to_commit2( Op::Nop => return Ok(Some(commit.id())), Op::Empty => return Ok(Some(git2::Oid::zero())), - Op::Chain(a, b) => { - let r = some_or!(apply_to_commit2(&to_op(*a), commit, transaction)?, { - return Ok(None); - }); - return if let Ok(r) = repo.find_commit(r) { - apply_to_commit2(&to_op(*b), &r, transaction) - } else { - Ok(Some(git2::Oid::zero())) - }; + Op::Chain(filters) => { + let mut current_oid = commit.id(); + for filter in filters { + if current_oid == git2::Oid::zero() { + break; + } + let current_commit = repo.find_commit(current_oid)?; + let r = some_or!( + apply_to_commit2(&to_op(*filter), ¤t_commit, transaction)?, + { + return Ok(None); + } + ); + current_oid = r; + } + return Ok(Some(current_oid)); } Op::Squash(None) => { return Some(history::rewrite_commit( @@ -521,6 +535,7 @@ fn apply_to_commit2( if let Some(oid) = transaction.get(filter, commit.id()) { return Ok(Some(oid)); } + // Continue to process the filter if not cached } }; @@ -637,9 +652,11 @@ fn apply_to_commit2( } Op::Squash(Some(ids)) => { if let Some(sq) = ids.get(&LazyRef::Resolved(commit.id())) { - let oid = if let Some(oid) = - apply_to_commit2(&Op::Chain(filter::squash(None), *sq), commit, transaction)? - { + let oid = if let Some(oid) = apply_to_commit2( + &Op::Chain(vec![filter::squash(None), *sq]), + commit, + transaction, + )? { oid } else { return Ok(None); @@ -866,7 +883,7 @@ fn apply_to_commit2( for (root, _link_file) in v { let embeding = some_or!( apply_to_commit2( - &Op::Chain(message("{@}"), file(root.join(".josh-link.toml"))), + &Op::Chain(vec![message("{@}"), file(root.join(".josh-link.toml"))]), &commit, transaction )?, @@ -1424,8 +1441,12 @@ fn apply2<'a>( Ok(x.with_tree(tree::compose(transaction, filtered)?)) } - Op::Chain(a, b) => { - return apply(transaction, *b, apply(transaction, *a, x.clone())?); + Op::Chain(filters) => { + let mut result = x; + for filter in filters { + result = apply(transaction, *filter, result)?; + } + return Ok(result); } Op::Hook(_) => Err(josh_error("not applicable to tree")), @@ -1488,24 +1509,35 @@ pub fn unapply<'a>( return Ok(ws); } - if let Op::Chain(a, b) = to_op(filter) { - // If filter "a" is invertable, use "invert(invert(a))" version of it, otherwise use as is - let a_normalized = if let Ok(a_inverted) = invert(a) { - invert(a_inverted)? + if let Op::Chain(filters) = to_op(filter) { + if filters.is_empty() { + return Ok(tree); + } + if filters.len() == 1 { + return unapply(transaction, filters[0], tree, parent_tree); + } + // Split into first and rest, unapply recursively + let (first, rest) = filters.split_first().unwrap(); + let rest_chain = to_filter(Op::Chain(rest.to_vec())); + + // Compute filtered_parent_tree for the first filter + let first_normalized = if let Ok(first_inverted) = invert(*first) { + invert(first_inverted)? } else { - a + *first }; let filtered_parent_tree = apply( transaction, - a_normalized, + first_normalized, Rewrite::from_tree(parent_tree.clone()), )? .into_tree(); + // Recursively unapply: first unapply the rest, then unapply first return unapply( transaction, - a, - unapply(transaction, b, tree, filtered_parent_tree)?, + *first, + unapply(transaction, rest_chain, tree, filtered_parent_tree)?, parent_tree, ); } @@ -1721,7 +1753,7 @@ fn is_ancestor_of( pub fn is_linear(filter: Filter) -> bool { match to_op(filter) { Op::Linear => true, - Op::Chain(a, b) => is_linear(a) || is_linear(b), + Op::Chain(filters) => filters.iter().any(|f| is_linear(*f)), _ => false, } } @@ -1735,7 +1767,9 @@ where let f = f.into_iter().map(|f| legalize_pin(f, c)).collect(); to_filter(Op::Compose(f)) } - Op::Chain(a, b) => to_filter(Op::Chain(legalize_pin(a, c), legalize_pin(b, c))), + Op::Chain(filters) => to_filter(Op::Chain( + filters.iter().map(|f| legalize_pin(*f, c)).collect(), + )), Op::Subtract(a, b) => to_filter(Op::Subtract(legalize_pin(a, c), legalize_pin(b, c))), Op::Exclude(f) => to_filter(Op::Exclude(legalize_pin(f, c))), Op::Pin(f) => c(f), @@ -1761,14 +1795,15 @@ fn legalize_stored(t: &cache::Transaction, f: Filter, tree: &git2::Tree) -> Josh .collect::>>()?; to_filter(Op::Compose(f)) } - Op::Chain(a, b) => { - let first = legalize_stored(t, a, tree)?; - let second = legalize_stored( - t, - b, - &apply(t, first, Rewrite::from_tree(tree.clone()))?.tree, - )?; - to_filter(Op::Chain(first, second)) + Op::Chain(filters) => { + let mut result = vec![]; + let mut current_tree = tree.clone(); + for filter in filters { + let legalized = legalize_stored(t, filter, ¤t_tree)?; + current_tree = apply(t, legalized, Rewrite::from_tree(current_tree.clone()))?.tree; + result.push(legalized); + } + to_filter(Op::Chain(result)) } Op::Subtract(a, b) => to_filter(Op::Subtract( legalize_stored(t, a, tree)?, diff --git a/josh-core/src/filter/op.rs b/josh-core/src/filter/op.rs index 7d0ebf389..52b7beda9 100644 --- a/josh-core/src/filter/op.rs +++ b/josh-core/src/filter/op.rs @@ -83,7 +83,7 @@ pub enum Op { Unapply(LazyRef, Filter), Compose(Vec), - Chain(Filter, Filter), + Chain(Vec), Subtract(Filter, Filter), Exclude(Filter), Pin(Filter), diff --git a/josh-core/src/filter/opt.rs b/josh-core/src/filter/opt.rs index be1fba62c..4ffe4d75b 100644 --- a/josh-core/src/filter/opt.rs +++ b/josh-core/src/filter/opt.rs @@ -66,18 +66,79 @@ pub fn simplify(filter: Filter) -> Filter { } Op::Compose(out.drain(..).map(simplify).collect()) } - Op::Chain(a, b) => match (to_op(a), to_op(b)) { - (a, Op::Chain(x, y)) => Op::Chain(to_filter(Op::Chain(to_filter(a), x)), y), - (Op::Prefix(x), Op::Prefix(y)) => Op::Prefix(y.join(x)), - (Op::Subdir(x), Op::Subdir(y)) => Op::Subdir(x.join(y)), - (Op::Chain(x, y), b) => match (to_op(x), to_op(y), b.clone()) { - (x, Op::Prefix(p1), Op::Prefix(p2)) => { - Op::Chain(simplify(to_filter(x)), to_filter(Op::Prefix(p2.join(p1)))) + Op::Chain(filters) => { + // Flatten nested chains + let mut flattened = vec![]; + for filter in filters { + if let Op::Chain(nested) = to_op(filter) { + flattened.extend(nested); + } else { + flattened.push(filter); } - _ => Op::Chain(simplify(a), simplify(to_filter(b))), - }, - (a, b) => Op::Chain(simplify(to_filter(a)), simplify(to_filter(b))), - }, + } + // Simplify each filter + let simplified: Vec<_> = flattened.iter().map(|f| simplify(*f)).collect(); + // Try to combine adjacent Prefix/Subdir operations + let mut result = vec![]; + let mut i = 0; + while i < simplified.len() { + if i + 1 < simplified.len() { + match (to_op(simplified[i]), to_op(simplified[i + 1])) { + (Op::Prefix(x), Op::Prefix(y)) => { + result.push(to_filter(Op::Prefix(y.join(x)))); + i += 2; + continue; + } + (Op::Subdir(x), Op::Subdir(y)) => { + result.push(to_filter(Op::Subdir(x.join(y)))); + i += 2; + continue; + } + (Op::Prefix(a), Op::Subdir(b)) if a != b => { + // If subdir path starts with prefix path, strip the prefix + if let Ok(stripped) = b.strip_prefix(&a) { + if stripped == std::path::Path::new("") { + // They cancel out completely + i += 2; + continue; + } else { + result.push(to_filter(Op::Subdir(stripped.to_owned()))); + i += 2; + continue; + } + } + } + (Op::Chain(x_filters), Op::Prefix(p2)) => { + if !x_filters.is_empty() { + if let Some((last_idx, Op::Prefix(p1))) = + x_filters.iter().enumerate().rev().find_map(|(idx, f)| { + if let Op::Prefix(p) = to_op(*f) { + Some((idx, Op::Prefix(p))) + } else { + None + } + }) + { + let mut new_x = x_filters.clone(); + new_x[last_idx] = to_filter(Op::Prefix(p2.join(p1))); + result.push(to_filter(Op::Chain(new_x))); + i += 2; + continue; + } + } + } + _ => {} + } + } + result.push(simplified[i]); + i += 1; + } + if result.len() == 1 { + to_op(result[0]) + } else { + Op::Chain(result) + } + } Op::Subtract(a, b) => { let (a, b) = (to_op(a), to_op(b)); Op::Subtract(simplify(to_filter(a)), simplify(to_filter(b))) @@ -117,23 +178,31 @@ pub fn flatten(filter: Filter) -> Filter { } Op::Compose(out.drain(..).map(flatten).collect()) } - Op::Chain(af, bf) => match (to_op(af), to_op(bf)) { - (_, Op::Compose(filters)) => { - let mut out = vec![]; - for f in filters { - out.push(to_filter(Op::Chain(af, f))); + Op::Chain(filters) => { + // Flatten nested chains first + let mut flattened = vec![]; + for filter in filters { + if let Op::Chain(nested) = to_op(filter) { + flattened.extend(nested); + } else { + flattened.push(filter); } - Op::Compose(out) } - (Op::Compose(filters), _) => { - let mut out = vec![]; - for f in filters { - out.push(to_filter(Op::Chain(f, bf))); + // Check if any filter is a Compose and distribute + for (i, filter) in flattened.iter().enumerate() { + if let Op::Compose(compose_filters) = to_op(*filter) { + // Distribute: create a Compose where each element is the chain with one compose element + let mut result = vec![]; + for compose_filter in compose_filters { + let mut new_chain = flattened.clone(); + new_chain[i] = compose_filter; + result.push(to_filter(Op::Chain(new_chain))); + } + return to_filter(Op::Compose(result)); } - Op::Compose(out) } - _ => Op::Chain(flatten(af), flatten(bf)), - }, + Op::Chain(flattened.iter().map(|f| flatten(*f)).collect()) + } Op::Subtract(a, b) => { let (a, b) = (to_op(a), to_op(b)); Op::Subtract(flatten(to_filter(a)), flatten(to_filter(b))) @@ -158,12 +227,14 @@ fn group(filters: &Vec) -> Vec> { continue; } - if let Op::Chain(a, _) = to_op(*f) { - if let Op::Chain(x, _) = to_op(res[res.len() - 1][0]) { - if a == x { - let n = res.len(); - res[n - 1].push(*f); - continue; + if let Op::Chain(filters) = to_op(*f) { + if !filters.is_empty() { + if let Op::Chain(other_filters) = to_op(res[res.len() - 1][0]) { + if !other_filters.is_empty() && filters[0] == other_filters[0] { + let n = res.len(); + res[n - 1].push(*f); + continue; + } } } } @@ -198,7 +269,15 @@ fn group(filters: &Vec) -> Vec> { fn last_chain(rest: Filter, filter: Filter) -> (Filter, Filter) { match to_op(filter) { - Op::Chain(a, b) => last_chain(to_filter(Op::Chain(rest, a)), b), + Op::Chain(filters) => { + if filters.is_empty() { + (rest, filter) + } else { + let mut new_rest = vec![rest]; + new_rest.extend(filters[..filters.len() - 1].iter().copied()); + last_chain(to_filter(Op::Chain(new_rest)), filters[filters.len() - 1]) + } + } _ => (rest, filter), } } @@ -343,12 +422,21 @@ fn common_pre(filters: &Vec) -> Option<(Filter, Vec)> { let mut rest = vec![]; let mut c: Option = None; for f in filters { - if let Op::Chain(a, b) = to_op(*f) { - rest.push(b); - if c.is_none() { - c = Some(a); - } - if c != Some(a) { + if let Op::Chain(chain_filters) = to_op(*f) { + if !chain_filters.is_empty() { + let first = chain_filters[0]; + let rest_chain = if chain_filters.len() > 1 { + to_filter(Op::Chain(chain_filters[1..].to_vec())) + } else { + to_filter(Op::Nop) + }; + rest.push(rest_chain); + if c.is_none() { + c = Some(first); + } else if c != Some(first) { + return None; + } + } else { return None; } } else { @@ -440,11 +528,10 @@ fn step(filter: Filter) -> Filter { let result = to_filter(match to_op(filter) { Op::Subdir(path) => { if path.components().count() > 1 { - let mut components = path.components(); - let a = components.next().unwrap(); Op::Chain( - to_filter(Op::Subdir(std::path::PathBuf::from(&a))), - to_filter(Op::Subdir(components.as_path().to_owned())), + path.components() + .map(|x| to_filter(Op::Subdir(std::path::PathBuf::from(&x)))) + .collect(), ) } else { Op::Subdir(path) @@ -452,11 +539,11 @@ fn step(filter: Filter) -> Filter { } Op::Prefix(path) => { if path.components().count() > 1 { - let mut components = path.components(); - let a = components.next().unwrap(); Op::Chain( - to_filter(Op::Prefix(components.as_path().to_owned())), - to_filter(Op::Prefix(std::path::PathBuf::from(&a))), + path.components() + .rev() + .map(|x| to_filter(Op::Prefix(std::path::PathBuf::from(&x)))) + .collect(), ) } else { Op::Prefix(path) @@ -470,9 +557,9 @@ fn step(filter: Filter) -> Filter { filters.retain(|x| *x != to_filter(Op::Empty)); let mut grouped = group(&filters); if let Some((common, rest)) = common_pre(&filters) { - Op::Chain(common, to_filter(Op::Compose(rest))) + Op::Chain(vec![common, to_filter(Op::Compose(rest))]) } else if let Some((common, rest)) = common_post(&filters) { - Op::Chain(to_filter(Op::Compose(rest)), common) + Op::Chain(vec![to_filter(Op::Compose(rest)), common]) } else if grouped.len() != 1 && grouped.len() != filters.len() { Op::Compose( grouped @@ -485,27 +572,61 @@ fn step(filter: Filter) -> Filter { Op::Compose(filters.drain(..).map(step).collect()) } } - Op::Chain(a, b) => match (to_op(a), to_op(b)) { - (Op::Chain(x, y), b) => Op::Chain(x, to_filter(Op::Chain(y, to_filter(b)))), - (Op::Prefix(a), Op::Subdir(b)) if a == b => Op::Nop, - (Op::Prefix(a), Op::Subdir(b)) - if a != b && a.components().count() == b.components().count() => - { - Op::Empty - } - (Op::Prefix(a), Op::Subdir(b)) if a != b => { - if let Ok(stripped) = a.strip_prefix(&b) { - Op::Prefix(stripped.to_owned()) + Op::Chain(filters) => { + // Flatten nested chains + let mut flattened = vec![]; + for filter in &filters { + if let Op::Chain(nested) = to_op(*filter) { + flattened.extend(nested); } else { - to_op(filter) + flattened.push(*filter); } } - (Op::Nop, b) => b, - (a, Op::Nop) => a, - (Op::Empty, _) => Op::Empty, - (_, Op::Empty) => Op::Empty, - (a, b) => Op::Chain(step(to_filter(a)), step(to_filter(b))), - }, + + // If any filter is Op::Empty the whole chain results in Op::Empty + if filters.contains(&to_filter(Op::Empty)) { + return to_filter(Op::Empty); + } + + // Remove Nop filters + let nop_filter = to_filter(Op::Nop); + flattened.retain(|f| *f != nop_filter); + if flattened.is_empty() { + return to_filter(Op::Nop); + } + + // Optimize adjacent Prefix/Subdir pairs + let mut result = vec![]; + let mut i = 0; + while i < flattened.len() { + if i + 1 < flattened.len() { + match (to_op(flattened[i]), to_op(flattened[i + 1])) { + (Op::Prefix(a), Op::Subdir(b)) if a == b => { + // Skip both, they cancel out + i += 2; + continue; + } + (Op::Prefix(a), Op::Subdir(b)) + if a != b && a.components().count() == b.components().count() => + { + // :prefix=a:/b will always result in an empty tree since the + // output of :prefix=a does not have a subtree "b" + return to_filter(Op::Empty); + } + _ => {} + } + } + result.push(step(flattened[i])); + i += 1; + } + if result.is_empty() { + Op::Nop + } else if result.len() == 1 { + to_op(result[0]) + } else { + Op::Chain(result) + } + } Op::Exclude(b) if b == to_filter(Op::Nop) => Op::Empty, Op::Exclude(b) | Op::Pin(b) if b == to_filter(Op::Empty) => Op::Nop, Op::Exclude(b) => Op::Exclude(step(b)), @@ -516,23 +637,37 @@ fn step(filter: Filter) -> Filter { (Op::Message(..), Op::Message(..)) => Op::Empty, (_, Op::Nop) => Op::Empty, (a, Op::Empty) => a, - (Op::Chain(a, b), Op::Chain(c, d)) if a == c => { - Op::Chain(a, to_filter(Op::Subtract(b, d))) + (Op::Chain(a_filters), Op::Chain(b_filters)) + if !a_filters.is_empty() + && !b_filters.is_empty() + && a_filters[0] == b_filters[0] => + { + let mut new_a = a_filters.clone(); + let mut new_b = b_filters.clone(); + let common = new_a.remove(0); + new_b.remove(0); + Op::Chain(vec![ + common, + to_filter(Op::Subtract( + to_filter(Op::Chain(new_a)), + to_filter(Op::Chain(new_b)), + )), + ]) } (_, b) if prefix_of(b.clone()) != to_filter(Op::Nop) => { Op::Subtract(af, last_chain(to_filter(Op::Nop), to_filter(b)).0) } - (a, _) if prefix_of(a.clone()) != to_filter(Op::Nop) => Op::Chain( + (a, _) if prefix_of(a.clone()) != to_filter(Op::Nop) => Op::Chain(vec![ to_filter(Op::Subtract( last_chain(to_filter(Op::Nop), to_filter(a.clone())).0, bf, )), prefix_of(a), - ), + ]), (_, b) if is_prefix(b.clone()) => Op::Subtract(af, to_filter(Op::Nop)), _ if common_post(&vec![af, bf]).is_some() => { let (cp, rest) = common_post(&vec![af, bf]).unwrap(); - Op::Chain(to_filter(Op::Subtract(rest[0], rest[1])), cp) + Op::Chain(vec![to_filter(Op::Subtract(rest[0], rest[1])), cp]) } (Op::Compose(mut av), _) if av.contains(&bf) => { av.retain(|x| *x != bf); @@ -591,7 +726,14 @@ pub fn invert(filter: Filter) -> JoshResult { rs_tracing::trace_scoped!("invert", "spec": spec(filter)); let result = to_filter(match to_op(filter) { - Op::Chain(a, b) => Op::Chain(invert(b)?, invert(a)?), + Op::Chain(filters) => { + let inverted: Vec<_> = filters + .iter() + .rev() + .map(|f| invert(*f)) + .collect::>()?; + Op::Chain(inverted) + } Op::Compose(filters) => Op::Compose( filters .into_iter() @@ -616,10 +758,11 @@ mod tests { fn regression_chain_prefix_subdir() { // When prefix is chained with subdir, and the subdir is deeper than // the prefix, the errornous code optimized this to just Prefix("a") - let filter = to_filter(Op::Chain( + let filter = to_filter(Op::Chain(vec![ to_filter(Op::Prefix(std::path::PathBuf::from("a"))), to_filter(Op::Subdir(std::path::PathBuf::from("a/b"))), - )); - assert_eq!(filter, optimize(filter)); + ])); + let expected = to_filter(Op::Subdir(std::path::PathBuf::from("b"))); + assert_eq!(expected, optimize(filter)); } } diff --git a/josh-core/src/filter/persist.rs b/josh-core/src/filter/persist.rs index 545d3fe45..91fb55aa1 100644 --- a/josh-core/src/filter/persist.rs +++ b/josh-core/src/filter/persist.rs @@ -21,6 +21,10 @@ pub(crate) fn to_op(filter: Filter) -> Op { .clone() } +pub(crate) fn to_ops(filters: &[Filter]) -> Vec { + return filters.iter().map(|x| to_op(*x)).collect(); +} + fn push_blob_entries( entries: &mut Vec, items: impl IntoIterator, gix_hash::ObjectId)>, @@ -196,8 +200,8 @@ impl InMemoryBuilder { let params_tree = self.build_filter_params(&[*a, *b])?; push_tree_entries(&mut entries, [("subtract", params_tree)]); } - Op::Chain(a, b) => { - let params_tree = self.build_filter_params(&[*a, *b])?; + Op::Chain(filters) => { + let params_tree = self.build_filter_params(filters)?; push_tree_entries(&mut entries, [("chain", params_tree)]); } Op::Exclude(b) => { @@ -713,24 +717,21 @@ fn from_tree2(repo: &git2::Repository, tree_oid: git2::Oid) -> JoshResult { } "chain" => { let chain_tree = repo.find_tree(entry.id())?; - if chain_tree.len() == 2 { - let a_tree = repo.find_tree( - chain_tree - .get_name("0") - .ok_or_else(|| josh_error("chain: missing 0"))? - .id(), - )?; - let b_tree = repo.find_tree( - chain_tree - .get_name("1") - .ok_or_else(|| josh_error("chain: missing 1"))? - .id(), - )?; - let a = from_tree2(repo, a_tree.id())?; - let b = from_tree2(repo, b_tree.id())?; - Ok(Op::Chain(to_filter(a), to_filter(b))) + if chain_tree.len() >= 1 { + let mut filters = vec![]; + for i in 0..chain_tree.len() { + let filter_tree = repo.find_tree( + chain_tree + .get_name(&i.to_string()) + .ok_or_else(|| josh_error(&format!("chain: missing {}", i)))? + .id(), + )?; + let filter = from_tree2(repo, filter_tree.id())?; + filters.push(to_filter(filter)); + } + Ok(Op::Chain(filters)) } else { - Err(josh_error("chain: expected 2 entries")) + Err(josh_error("chain: expected at least 1 entry")) } } "exclude" => { diff --git a/josh-core/src/flang/mod.rs b/josh-core/src/flang/mod.rs index e6044fe18..1b8f3e095 100644 --- a/josh-core/src/flang/mod.rs +++ b/josh-core/src/flang/mod.rs @@ -2,7 +2,9 @@ pub mod parse; use crate::filter::op::Op; use crate::filter::opt; +use crate::filter::persist::to_filter; use crate::filter::persist::to_op; +use crate::filter::persist::to_ops; use crate::filter::sequence_number; use crate::filter::{self, Filter}; @@ -53,23 +55,38 @@ fn pretty2(op: &Op, indent: usize, compose: bool) -> String { Op::Compose(filters) => ff(&filters, "pin", indent), b => format!(":pin[{}]", pretty2(&b, indent, false)), }, - Op::Chain(a, b) => match (to_op(*a), to_op(*b)) { - (Op::Subdir(p1), Op::Prefix(p2)) if p1 == p2 => { - format!("::{}/", parse::quote_if(&p1.to_string_lossy())) + Op::Chain(filters) => { + if filters.is_empty() { + return ":/".to_string(); } - (a, Op::Prefix(p)) if compose => { - format!( - "{} = {}", - parse::quote_if(&p.to_string_lossy()), - pretty2(&a, indent, false) - ) + if filters.len() == 1 { + return pretty2(&to_op(filters[0]), indent, compose); } - (a, b) => format!( - "{}{}", - pretty2(&a, indent, false), - pretty2(&b, indent, false) - ), - }, + // Check for special case: Subdir + Prefix that cancel + match &to_ops(&filters)[..] { + [Op::Subdir(p1), Op::Prefix(p2)] if p1 == p2 => { + return format!("::{}/", parse::quote_if(&p1.to_string_lossy())); + } + [a @ .., Op::Prefix(p)] if compose => { + return format!( + "{} = {}", + parse::quote_if(&p.to_string_lossy()), + pretty2( + &Op::Chain(a.iter().map(|x| to_filter(x.clone())).collect()), + indent, + false + ) + ); + } + _ => {} + } + // General case: concatenate all filters + filters + .iter() + .map(|f| pretty2(&to_op(*f), indent, false)) + .collect::>() + .join("") + } Op::RegexReplace(replacements) => { let v = replacements .iter() @@ -159,12 +176,24 @@ pub(crate) fn spec2(op: &Op) -> String { format!(":replace({})", v.join(",")) } - Op::Chain(a, b) => match (to_op(*a), to_op(*b)) { - (Op::Subdir(p1), Op::Prefix(p2)) if p1 == p2 => { - format!("::{}/", parse::quote_if(&p1.to_string_lossy())) + Op::Chain(filters) => { + if filters.is_empty() { + return ":/".to_string(); } - (a, b) => format!("{}{}", spec2(&a), spec2(&b)), - }, + if filters.len() == 2 { + match (to_op(filters[0]), to_op(filters[1])) { + (Op::Subdir(p1), Op::Prefix(p2)) if p1 == p2 => { + return format!("::{}/", parse::quote_if(&p1.to_string_lossy())); + } + _ => {} + } + } + filters + .iter() + .map(|f| spec2(&to_op(*f))) + .collect::>() + .join("") + } Op::Nop => ":/".to_string(), Op::Empty => ":empty".to_string(), diff --git a/josh-core/src/flang/parse.rs b/josh-core/src/flang/parse.rs index 71a403a65..2158af88b 100644 --- a/josh-core/src/flang/parse.rs +++ b/josh-core/src/flang/parse.rs @@ -114,10 +114,10 @@ fn parse_item(pair: pest::iterators::Pair) -> JoshResult { if arg.ends_with('/') { let arg = arg.trim_end_matches('/'); - Ok(Op::Chain( + Ok(Op::Chain(vec![ to_filter(Op::Subdir(std::path::PathBuf::from(arg))), to_filter(make_op(&["prefix", arg])?), - )) + ])) } else if arg.contains('*') { // Pattern case - error if combined with = (destination=source syntax) if second_arg.is_some() { @@ -197,10 +197,10 @@ fn parse_item(pair: pest::iterators::Pair) -> JoshResult { if v.len() == 2 { let oid = LazyRef::parse(v[0])?; let filter = parse(v[1])?; - Ok(Op::Chain( + Ok(Op::Chain(vec![ filter, filter::to_filter(Op::HistoryConcat(oid, filter)), - )) + ])) } else { Err(josh_error("wrong argument count for :from")) } @@ -261,7 +261,7 @@ fn parse_item(pair: pest::iterators::Pair) -> JoshResult { let y = to_filter(Op::Compose(y_filters)); let inverted_x = invert(x)?; - Ok(Op::Chain(x, to_filter(Op::Chain(y, inverted_x)))) + Ok(Op::Chain(vec![x, y, inverted_x])) } _ => Err(josh_error("parse_item: no match")), } @@ -380,7 +380,12 @@ pub fn parse(filter_spec: &str) -> JoshResult { for pair in r.into_inner() { let v = parse_item(pair)?; chain = Some(if let Some(c) = chain { - Op::Chain(to_filter(c), to_filter(v)) + if let Op::Chain(mut filters) = c { + filters.push(to_filter(v)); + Op::Chain(filters) + } else { + Op::Chain(vec![to_filter(c), to_filter(v)]) + } } else { v }); diff --git a/tests/filter/file.t b/tests/filter/file.t index 7fd287318..87505ef60 100644 --- a/tests/filter/file.t +++ b/tests/filter/file.t @@ -57,7 +57,7 @@ |-- josh | `-- 24 | `-- 0 - | `-- b3521f07f4c095a5b2142766269603c039a39271 + | `-- fc16bb70cbbc24982dac74e19c853a8fc91a2aed `-- tags 6 directories, 2 files diff --git a/tests/filter/filter_id.t b/tests/filter/filter_id.t index e90fd955b..6e3ee15ea 100644 --- a/tests/filter/filter_id.t +++ b/tests/filter/filter_id.t @@ -76,29 +76,25 @@ | |-- 0 | | `-- subdir | | `-- 0 - | `-- 1 - | `-- chain - | |-- 0 - | | `-- subdir - | | `-- 0 - | `-- 1 - | `-- prefix - | `-- 0 + | |-- 1 + | | `-- subdir + | | `-- 0 + | `-- 2 + | `-- prefix + | `-- 0 `-- 1 `-- chain |-- 0 | `-- subdir | `-- 0 - `-- 1 - `-- chain - |-- 0 - | `-- subdir - | `-- 0 - `-- 1 - `-- prefix - `-- 0 + |-- 1 + | `-- subdir + | `-- 0 + `-- 2 + `-- prefix + `-- 0 - 26 directories, 7 files + 22 directories, 7 files $ git diff ${EMPTY_TREE}..${FILTER_HASH} diff --git a/chain/0/subdir/0 b/chain/0/subdir/0 new file mode 100644 @@ -116,19 +112,19 @@ @@ -0,0 +1 @@ +b \ No newline at end of file - diff --git a/chain/1/compose/0/chain/1/chain/0/subdir/0 b/chain/1/compose/0/chain/1/chain/0/subdir/0 + diff --git a/chain/1/compose/0/chain/1/subdir/0 b/chain/1/compose/0/chain/1/subdir/0 new file mode 100644 index 0000000..c59d9b6 --- /dev/null - +++ b/chain/1/compose/0/chain/1/chain/0/subdir/0 + +++ b/chain/1/compose/0/chain/1/subdir/0 @@ -0,0 +1 @@ +d \ No newline at end of file - diff --git a/chain/1/compose/0/chain/1/chain/1/prefix/0 b/chain/1/compose/0/chain/1/chain/1/prefix/0 + diff --git a/chain/1/compose/0/chain/2/prefix/0 b/chain/1/compose/0/chain/2/prefix/0 new file mode 100644 index 0000000..c1b0730 --- /dev/null - +++ b/chain/1/compose/0/chain/1/chain/1/prefix/0 + +++ b/chain/1/compose/0/chain/2/prefix/0 @@ -0,0 +1 @@ +x \ No newline at end of file @@ -140,19 +136,19 @@ @@ -0,0 +1 @@ +c \ No newline at end of file - diff --git a/chain/1/compose/1/chain/1/chain/0/subdir/0 b/chain/1/compose/1/chain/1/chain/0/subdir/0 + diff --git a/chain/1/compose/1/chain/1/subdir/0 b/chain/1/compose/1/chain/1/subdir/0 new file mode 100644 index 0000000..c59d9b6 --- /dev/null - +++ b/chain/1/compose/1/chain/1/chain/0/subdir/0 + +++ b/chain/1/compose/1/chain/1/subdir/0 @@ -0,0 +1 @@ +d \ No newline at end of file - diff --git a/chain/1/compose/1/chain/1/chain/1/prefix/0 b/chain/1/compose/1/chain/1/chain/1/prefix/0 + diff --git a/chain/1/compose/1/chain/2/prefix/0 b/chain/1/compose/1/chain/2/prefix/0 new file mode 100644 index 0000000..e25f181 --- /dev/null - +++ b/chain/1/compose/1/chain/1/chain/1/prefix/0 + +++ b/chain/1/compose/1/chain/2/prefix/0 @@ -0,0 +1 @@ +y \ No newline at end of file @@ -545,31 +541,23 @@ Test ::a/b/c/ (nested directory path with trailing slash) |-- 0 | `-- subdir | `-- 0 - `-- 1 - `-- chain - |-- 0 - | `-- subdir - | `-- 0 - `-- 1 - `-- chain - |-- 0 - | `-- subdir - | `-- 0 - `-- 1 - `-- chain - |-- 0 - | `-- prefix - | `-- 0 - `-- 1 - `-- chain - |-- 0 - | `-- prefix - | `-- 0 - `-- 1 - `-- prefix - `-- 0 + |-- 1 + | `-- subdir + | `-- 0 + |-- 2 + | `-- subdir + | `-- 0 + |-- 3 + | `-- prefix + | `-- 0 + |-- 4 + | `-- prefix + | `-- 0 + `-- 5 + `-- prefix + `-- 0 - 22 directories, 6 files + 14 directories, 6 files $ git diff 4b825dc642cb6eb9a060e54bf8d69288fbee4904..${FILTER_HASH} diff --git a/chain/0/subdir/0 b/chain/0/subdir/0 new file mode 100644 @@ -579,43 +567,43 @@ Test ::a/b/c/ (nested directory path with trailing slash) @@ -0,0 +1 @@ +a \ No newline at end of file - diff --git a/chain/1/chain/0/subdir/0 b/chain/1/chain/0/subdir/0 + diff --git a/chain/1/subdir/0 b/chain/1/subdir/0 new file mode 100644 index 0000000..63d8dbd --- /dev/null - +++ b/chain/1/chain/0/subdir/0 + +++ b/chain/1/subdir/0 @@ -0,0 +1 @@ +b \ No newline at end of file - diff --git a/chain/1/chain/1/chain/0/subdir/0 b/chain/1/chain/1/chain/0/subdir/0 + diff --git a/chain/2/subdir/0 b/chain/2/subdir/0 new file mode 100644 index 0000000..3410062 --- /dev/null - +++ b/chain/1/chain/1/chain/0/subdir/0 + +++ b/chain/2/subdir/0 @@ -0,0 +1 @@ +c \ No newline at end of file - diff --git a/chain/1/chain/1/chain/1/chain/0/prefix/0 b/chain/1/chain/1/chain/1/chain/0/prefix/0 + diff --git a/chain/3/prefix/0 b/chain/3/prefix/0 new file mode 100644 index 0000000..3410062 --- /dev/null - +++ b/chain/1/chain/1/chain/1/chain/0/prefix/0 + +++ b/chain/3/prefix/0 @@ -0,0 +1 @@ +c \ No newline at end of file - diff --git a/chain/1/chain/1/chain/1/chain/1/chain/0/prefix/0 b/chain/1/chain/1/chain/1/chain/1/chain/0/prefix/0 + diff --git a/chain/4/prefix/0 b/chain/4/prefix/0 new file mode 100644 index 0000000..63d8dbd --- /dev/null - +++ b/chain/1/chain/1/chain/1/chain/1/chain/0/prefix/0 + +++ b/chain/4/prefix/0 @@ -0,0 +1 @@ +b \ No newline at end of file - diff --git a/chain/1/chain/1/chain/1/chain/1/chain/1/prefix/0 b/chain/1/chain/1/chain/1/chain/1/chain/1/prefix/0 + diff --git a/chain/5/prefix/0 b/chain/5/prefix/0 new file mode 100644 index 0000000..2e65efe --- /dev/null - +++ b/chain/1/chain/1/chain/1/chain/1/chain/1/prefix/0 + +++ b/chain/5/prefix/0 @@ -0,0 +1 @@ +a \ No newline at end of file diff --git a/tests/filter/scope_filter.t b/tests/filter/scope_filter.t index 15f2f71a5..d48070981 100644 --- a/tests/filter/scope_filter.t +++ b/tests/filter/scope_filter.t @@ -24,16 +24,14 @@ Test basic scope filter syntax :[Y] |-- 0 | `-- subdir | `-- 0 - `-- 1 - `-- chain - |-- 0 - | `-- subdir - | `-- 0 - `-- 1 - `-- prefix - `-- 0 + |-- 1 + | `-- subdir + | `-- 0 + `-- 2 + `-- prefix + `-- 0 - 10 directories, 3 files + 8 directories, 3 files $ cat sub1/file1 cat: sub1/file1: No such file or directory [1] @@ -52,26 +50,24 @@ Test scope filter with multiple filters in compose |-- 0 | `-- subdir | `-- 0 - `-- 1 - `-- chain - |-- 0 - | `-- compose - | |-- 0 - | | `-- subdir - | | `-- 0 - | `-- 1 - | `-- chain - | |-- 0 - | | `-- subdir - | | `-- 0 - | `-- 1 - | `-- subdir - | `-- 0 - `-- 1 - `-- prefix - `-- 0 + |-- 1 + | `-- compose + | |-- 0 + | | `-- subdir + | | `-- 0 + | `-- 1 + | `-- chain + | |-- 0 + | | `-- subdir + | | `-- 0 + | `-- 1 + | `-- subdir + | `-- 0 + `-- 2 + `-- prefix + `-- 0 - 18 directories, 5 files + 16 directories, 5 files Test scope filter with prefix filter $ FILTER_HASH=$(josh-filter -i ':<:prefix=sub1>[:prefix=file1]') @@ -95,19 +91,17 @@ Test scope filter with subdir and exclude |-- 0 | `-- subdir | `-- 0 - `-- 1 - `-- chain - |-- 0 - | `-- exclude - | `-- 0 - | `-- file - | |-- 0 - | `-- 1 - `-- 1 - `-- prefix - `-- 0 + |-- 1 + | `-- exclude + | `-- 0 + | `-- file + | |-- 0 + | `-- 1 + `-- 2 + `-- prefix + `-- 0 - 12 directories, 4 files + 10 directories, 4 files Test scope filter verifies it expands to chain(X, chain(Y, invert(X))) $ FILTER_HASH=$(josh-filter -i ':<:/sub1>[:/file1]') @@ -129,24 +123,22 @@ Test scope filter with nested filters |-- 0 | `-- subdir | `-- 0 - `-- 1 - `-- chain - |-- 0 - | `-- compose - | |-- 0 - | | `-- subdir - | | `-- 0 - | `-- 1 - | `-- chain - | |-- 0 - | | `-- subdir - | | `-- 0 - | `-- 1 - | `-- subdir - | `-- 0 - `-- 1 - `-- prefix - `-- 0 + |-- 1 + | `-- compose + | |-- 0 + | | `-- subdir + | | `-- 0 + | `-- 1 + | `-- chain + | |-- 0 + | | `-- subdir + | | `-- 0 + | `-- 1 + | `-- subdir + | `-- 0 + `-- 2 + `-- prefix + `-- 0 - 18 directories, 5 files + 16 directories, 5 files diff --git a/tests/proxy/clone_with_meta.t b/tests/proxy/clone_with_meta.t index ed5ffca17..d7ef8550c 100644 --- a/tests/proxy/clone_with_meta.t +++ b/tests/proxy/clone_with_meta.t @@ -122,10 +122,10 @@ "::with_prefix.git/", ] "real_repo.git" = [ + ":/sub1", "::sub1/", "::sub2/", ":prefix=my_prefix", - ":prefix=my_prefix:/my_prefix:/sub1", ] . |-- josh diff --git a/tests/proxy/workspace_errors.t b/tests/proxy/workspace_errors.t index 922b73c8b..e77a49d9a 100644 --- a/tests/proxy/workspace_errors.t +++ b/tests/proxy/workspace_errors.t @@ -141,7 +141,7 @@ No match for filters remote: upstream: response body: remote: remote: To http://localhost:8001/real_repo.git - remote: 5119a73..25943be JOSH_PUSH -> master + remote: 5119a73..25c566d JOSH_PUSH -> master remote: warnings: remote: No match for "::abc" remote: No match for "a/b = :/b/c/*" @@ -150,7 +150,7 @@ No match for filters remote: No match for "test = ::test" remote: No match for "test/test = :/test:/" remote: No match for "::test/test/" - remote: REWRITE(064643c5fdf5295695d383a511e4335ea3262fce -> 9cbc5874da793480ee59207ca72d9f0523b8b127) + remote: REWRITE(064643c5fdf5295695d383a511e4335ea3262fce -> e972e5448a9595045a092fb5f7d82dc6797b4d23) To http://localhost:8002/real_repo.git:workspace=ws.git 66a8b5e..064643c HEAD -> master