Julian Foad wrote on Fri, 23 Nov 2018 13:56 +0000:
> Daniel Shahaf wrote:
> > Julian Foad wrote on Wed, 21 Nov 2018 16:00 +0000:
> > > 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?
>
> Maybe I was thinking of open_path_last_optional. "If FLAGS &
> open_path_last_optional is ... non-zero, require all the parent
> directories to exist as normal ..." when in this case the parent is not
> a directory.
Ah, yes, I see. The docstring has a lacuna.
By code inspection, all three callsites that pass
open_path_last_optional expect the following postcondition:
.
(*parent_path_p)->node != NULL || svn_fs_fs__dag_node_kind((*parent_path_p)->parent->node) == svn_node_dir
.
which agrees with open_path() of r2:/B/mu/iota returning
SVN_ERR_FS_NOT_DIRECTORY.
That said, that postcondition doesn't seem to be a good API: the two
possible outcomes are very different from each other, to the point that
every single caller branches on them and handles them differently. (The
function does not "do one thing well".) If a caller of open_path(open_path_last_optional)
forgets to check whether parent->node is NULL or not, we'll probably
have a bug (hopefully nothing worse than a segfault or an error from the
DAG layer). I don't see why we couldn't remove open_path_allow_null
entirely and have callsites that pass it just call open_path(..., dirname(path), ...)
and do the existence check on the child's basename explicitly. The case
that 'path' exists is an error anyway.
All that said, the callers' code does appear to be correct, in a
quick skim.
Cheers,
Daniel
Received on 2018-11-23 16:56:33 CET