Julian Foad wrote on Wed, 21 Nov 2018 16:00 +0000:
> 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.
The comment is somewhat out of date, as it refers to a local variable ID
that doesn't exist. I share your concern, however: I wondered if after
taking the 'shortcut' the loop was entered with some invariant not
holding and if patching the svn_fs_fs__dag_open() call was simply
covering up that underlying issue; however, in the end I think the
issue is real.
It _does_ look odd that open_path_allow_null is checked in two places,
though. I suppose the second check could be removed and deferred to the
next iteration. At that point we might also be able to find a way to
reproduce the original error even in a codepath that doesn't take the
'shortcut', in order to increase our confidence in the patch.
While we're at this function, does anyone understand why directory is
accessed without checking whether directory is not NUL? There is
a comment there, but it doesn't enlighten me. (However, I haven't run
'blame' on that comment yet.) Even if it's correct, is there any reason
not to add an SVN_ERR_ASSERT(directory) there?
> 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.
Have you read the docstring of the open_path_allow_null enumerator?
Received on 2018-11-22 01:51:49 CET