Daniel Shahaf wrote: > Daniel Shahaf wrote on Tue, Nov 20, 2018 at 09:28:19 +0000: > > The test XPASSes with SVN_X_DOES_NOT_MARK_THE_SPOT=1 (see notes/knobs), > > so it's something to do with the caches. > > So, looking at subversion/libsvn_fs_fs/tree.c: [...] > In words: svn_fs_fs__dag_open() is asked to find the child "iota" of > r2:/B/mu, and instead of setting 'child' to NULL as the code expects, it > returns an error which percolates all the way to the client. > > The reason SVN_X_DOES_NOT_MARK_THE_SPOT fixes it is that it bypasses the > optimization earlier in the function. That optimization causes the the > very first iteration of the loop is to process "/B/mu". With caches > disabled, the first iteration of the loop processes "/" and the second > iteration processes "/B" and exits early, here: > > 1144 /* The path isn't finished yet; we'd better be in a > directory. */ > 1145 if (svn_fs_fs__dag_node_kind(child) != svn_node_dir) > 1146 { > 1147 const char *msg; > 1148 > 1149 /* Since this is not a directory and we are looking > for some > 1150 sub-path, that sub-path will not exist. That will > be o.k., > 1151 if we are just here to check for the path's > existence. */ > 1152 if (flags & open_path_allow_null) > 1153 { > 1154 parent_path = NULL; > 1155 break; > 1156 } > > So, we just need to make the svn_fs_fs__dag_open() call set child=NULL > so it can fall back to the existing logic for handling FLAGS: > > [[[ > Index: subversion/libsvn_fs_fs/tree.c > =================================================================== > --- subversion/libsvn_fs_fs/tree.c (revision 1845259) > +++ subversion/libsvn_fs_fs/tree.c (working copy) > @@ -1083,8 +1083,10 @@ > pool)); > if (cached_node) > child = cached_node; > - else > - SVN_ERR(svn_fs_fs__dag_open(&child, here, entry, pool, > iterpool)); > + else if (svn_fs_fs__dag_node_kind(here) == svn_node_dir) > + SVN_ERR(svn_fs_fs__dag_open(&child, here, entry, pool, > iterpool)); > + else > + child = NULL; > > /* "file not found" requires special handling. */ > if (child == NULL) > ]]] > > Makes sense?
The top-of-loop comment says: "/* Whenever we are at the top of this loop: - HERE is our current directory, ..." As HERE is apparently NOT a directory in this case, I wonder if the comment simply should say something like "current path (usually a directory)", or whether anything else is amiss too. In reviewing the code I was unable to keep track of all the nuances of what happens (and should happen) in all the edge cases. Especially when 'flags & open_path_allow_null' is true and the requested path is a child of a non-directory like the "/B/mu/iota" in this case: that combination doesn't seem to be well documented, which makes me wonder what the callers expect it to do. -- - Julian