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
Received on 2018-11-21 17:00:26 CET