subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Julian Foad <julianf...@apache.org>
Subject Re: [PATCH] Re: [PATCH] A test for "Can't get entries" error
Date Wed, 21 Nov 2018 16:00:18 GMT
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

Mime
View raw message