Fix inconsistent trees from getindex and merge. #40
Conversation
Codecov Report
@@ Coverage Diff @@
## master #40 +/- ##
==========================================
+ Coverage 84.22% 87.05% +2.82%
==========================================
Files 6 6
Lines 317 363 +46
==========================================
+ Hits 267 316 +49
+ Misses 50 47 -3
Continue to review full report at Codecov.
|
|
@shashi Bump? In case you have more pressing matters on your table or just don't want to context switch: I can work around the issue from the outside by just adding |
| @@ -73,7 +73,7 @@ function _regex_map(yes, no, t::FileTree, regex::Regex, toplevel=true) | |||
| if isempty(cs) | |||
| return no(t) | |||
| else | |||
| return FileTree(t; children=cs) | |||
| return FileTree(t; children=cs) |> setparent | |||
There was a problem hiding this comment.
Setparent here may copy sub trees more times than necessary. I’d suggest moving this to the place which calls _regex_map or _glob_map
There was a problem hiding this comment.
I guess you mean the getindex functions? Sure thing! Will commit once tests finish.
|
Sorry about the delay here. This got drowned out in the other notifications. Thanks for pinging again! This looks good to me, except for that comment I left. |
|
Thanks for checking! Do you have an opinion on this:
Is there some reason one should be able to create trees where nodes are not the parent of their children? |
Fixes #39
Testing revealed that the root issue is that
getindexfor both globs and regexps returns a tree where the children of the root does not have the root as parent. Same thing also happens aftermerge.Not sure if this is intentional, but it seems to me that a tree like this is inconsistent and should be considered invalid. Another option could be to add a call to
setparentin the convenience “copy” constructor forFileTreeif one wants to be really strict about not allowing such trees.It turned out that
mvandcp(or in factregex_rewrite_treeandattach) relied on this behavior to leave the root out of all processing. I first tried to changeattachto not add the root node (i.e last line changed tomerge(t, t1; combine=combine)and assume thatpathargument included the path to the root.This did however cause problems when root node is a subdirectory as it would change the tree to use the root of the root node as, uhm, root (i.e if root node has path a/b then the returned tree would have
aas root andbas its child). This also had the adverse consequence that the public API formvandcpwould break as regexes where now matched against the whole path including the root node. Both these problems where caught by themvtests in taxi.jl.Instead I resorted to just truncating the canonical path from behind in
regex_rewrite_tree. Not sure if this is the most elegant way to do it.