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[1] is accessed without checking whether directory[0] 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[0]) 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? Cheers, Daniel
