You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2013/02/04 19:59:08 UTC
Re: r1442088 - Improve DAG node / path lookup performance in FSFS
Hi Stefan.
> URL: http://svn.apache.org/viewvc?rev=1442088&view=rev
> Log:
> Improve DAG node / path lookup performance in FSFS. Since this is a hot
> path in many fs_repo-based functions (like fs_verify), 20 .. 30% time
> saved here can easily become significant.
>
> The idea here is that get_dag() already has knowledge about the PATH
> parameter and does not actually use all the result data returned by
> open_path(). So, define a number of flags that allow a caller to tell
> open_path() where it may take short-cuts based on the context.
>
> * subversion/libsvn_fs_fs/tree.c
> (dag_node_cache_set): add comment to document a rationale
> (enum open_path_flags_t): declare further flags
> (open_path): support the new flags to short-circuit portions of code
> (get_dag,
> fs_closest_copy,
> get_mergeinfo_for_path_internal): provide flags to open_path()
> + /* When this flag is set, don't bother to lookup the DAG node in
> + our caches because we already tried this. Ignoring this flag
> + has no functional impact. */
> + open_path_uncached = 2,
> +
> + /* Assume that the path parameter is already in canonical form. */
> + open_path_is_canonical = 4,
I don't like this 'is_canonical' flag. In the rest of Subversion, nearly all APIs assume paths are canonical already, and for a good reason: re-canonicalizing at a lower level tends to be expensive because it happens more times and with less knowledge of whether it's needed. I think it would be better (including faster) to simply require that the path is canonical, and make sure the callers honour that.
I see that many of the functions in this file currently appear to be accepting and dealing with non-canonical paths, but I think the way to go is to change that.
> + /* The caller does not care about the parent node chain but only
> + the final DAG node. */
> + open_path_node_only = 8
> } open_path_flags_t;
>
>
> @@ -871,6 +885,10 @@ typedef enum open_path_flags_t {
> callers that create new nodes --- we find the parent directory for
> them, and tell them whether the entry exists already.
>
> + The remaining bits in FLAGS are hints that allow this function
> + to take shortcuts based on knowledge that the caller provides,
> + such as the fact that PATH is already in canonical form.
Maybe give a different example.
> NOTE: Public interfaces which only *read* from the filesystem
> should not call this function directly, but should instead use
> get_dag().
> @@ -884,23 +902,47 @@ open_path(parent_path_t **parent_path_p,
> apr_pool_t *pool)
> {
> svn_fs_t *fs = root->fs;
> - dag_node_t *here; /* The directory we're currently looking at. */
> - parent_path_t *parent_path; /* The path from HERE up to the root. */
> + dag_node_t *here = NULL; /* The directory we're currently looking at. */
> + parent_path_t *parent_path; /* The path from HERE up to the root. */
> const char *rest; /* The portion of PATH we haven't traversed yet. */
> - const char *canon_path = svn_fs__is_canonical_abspath(path)
> +
> + /* ensure a canonical path representation */
> + const char *canon_path = ( (flags & open_path_is_canonical)
> + || svn_fs__is_canonical_abspath(path))
> ? path
> : svn_fs__canonicalize_abspath(path, pool);
Here you seem also to have decided that when canonicalizing, it's more efficient to test with svn_fs__is_canonical_abspath() before calling svn_fs__canonicalize_abspath(). Are you sure that is true? If it is true here (and I see you already have the same pattern in one of the callers), then perhaps it is true in general, so why not do it inside svn_fs__canonicalize_abspath() so all callers benefit?
> @@ -1142,7 +1195,9 @@ get_dag(dag_node_t **dag_node_p,
> {
> /* Call open_path with no flags, as we want this to return an
> * error if the node for which we are searching doesn't exist. */
The comment was falsified by your change and needs updating. (There are similar comments on open_path() calls elsewhere in the file, which probably should be updated in the same way even though those calls are still passing no flags.)
> - SVN_ERR(open_path(&parent_path, root, path, 0, NULL, pool));
> + SVN_ERR(open_path(&parent_path, root, path,
> + open_path_uncached | open_path_is_canonical
> + | open_path_node_only, NULL, pool));
(This is the only significant place where the 'is_canonical' flag is used.)
> node = parent_path->node;
>
> /* No need to cache our find -- open_path() will do that for us. */
> @@ -3162,7 +3217,7 @@ static svn_error_t *fs_closest_copy(svn_
> if (kind == svn_node_none)
> return SVN_NO_ERROR;
> SVN_ERR(open_path(©_dst_parent_path, copy_dst_root, path,
> - 0, NULL, pool));
> + open_path_node_only, NULL, pool));
> copy_dst_node = copy_dst_parent_path->node;
> if (! svn_fs_fs__id_check_related(svn_fs_fs__dag_get_id(copy_dst_node),
>
> svn_fs_fs__dag_get_id(parent_path->node)))
> @@ -3775,7 +3830,8 @@ get_mergeinfo_for_path_internal(svn_merg
>
> path = svn_fs__canonicalize_abspath(path, scratch_pool);
>
> - SVN_ERR(open_path(&parent_path, rev_root, path, 0, NULL, scratch_pool));
> + SVN_ERR(open_path(&parent_path, rev_root, path, open_path_is_canonical,
> + NULL, scratch_pool));
Here you've left a plain 'canonicalize' in the caller, even though you appear to think that the version inside open_path() is more efficient. (The result of this 'canonicalize' here is *only* passed to open_path().)
> if (inherit == svn_mergeinfo_nearest_ancestor && !
> parent_path->parent)
> return SVN_NO_ERROR;
- Julian
Re: r1442088 - Improve DAG node / path lookup performance in FSFS
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Feb 22, 2013 at 5:43 PM, Julian Foad <ju...@btopenworld.com>wrote:
> Bert Huijben wrote:
> > [Julian]
> > I don't like this 'is_canonical' flag. In the rest of Subversion, nearly
> > all APIs assume paths are canonical already [...]
> > I see that many of the functions in this file currently appear to be
> > accepting and dealing with non-canonical paths, but I think the way to
> go is
> > to change that.
> > <<
> >
> > [Stefan]
> > Turned out not to be too much work to fix all callers.
> > <<
> >
> > I still see that we canonicalize the paths on +- every api. (About 20
> calls
> > in tree.c and dag.c)
>
We need to do that on API level but not internally.
Julian's suggestion to push the canonicalization
(canonization?) to the FS loader is properly the
right thing to do and will simplify BDB code as well.
> > Eventually this api should move to something like an
> > assert(is_canonical(path,…)) on all functions and just assume that
> callers
> > do the right thing, like we do everywhere else
> >
> > Canonicalizing is everywhere is expensive and error prone.
>
> Yes. I've just been looking at this, and I'd like to expand on the "error
> prone" aspect.
>
> I don't know if we've ever fully specified what is a valid FS path, but I
> noticed that svn_fs__canonicalize_abspath() doesn't treat a "." path
> segment as special, whereas all our other path APIs treat a "." segment as
> non-canonical.
>
> That means that if anything passes a path with a "." segment into the FS
> layer API, this canonicalization will not remove it and then some other
> function will throw an assertion failure or produce the wrong result, for
> some definition of 'wrong'. Indeed I tried inserting a '.' in a path in a
> call to svn_fs_node_id() in fs-base-test.c, and an assertion faliure was
> thrown.
>
Hm. "." segments should never occur on the FS level.
My guess is that the main reason for all the c14n was
some caller beings unclean when it comes to adding
'/' to the beginning and the end may maybe producing
"//" artifacts back when paths got glued together by hand.
Also svn_fs__canonicalize_abspath() accepts NULL but no code seems to
> require that -- at least the test suite still passes using FSFS when I
> remove that special case (and remove a test case explicitly testing that
> function with NULL).
>
NULL might also be an artifact from "way back when".
> PROPOSAL
>
> 1. Agree and document the spec for an FS layer path. Something like:
> sequence of non-null UTF-8 characters, leading '/', no trailing '/' unless
> the empty path, no '//', no '.' segments, no '..' segments. (There's no
> need for the FS layer to treat '.' and '..' as special, but since the
> higher layers do it's just easier if we choose the same rules and so can
> use the same path functions.)
>
+1 on specifying what the API wants.
> 2. Delete svn_fs__canonicalize_abspath(); use svn_fspath__canonicalize()
> instead; optimize svn_fspath__canonicalize(), and consider whether we want
> a variant of that function that doesn't always allocate in the new pool.
>
FYI, svn_fspath__canonicalize() does not handle ".." segments
(not too important here but raised an eyebrow when I read the code).
3. Move the canonicalization calls out of the vtable implementation
> functions, into the FS vtable wrapper functions. Consider whether it needs
> to canonicalize all non-canonical paths. May be better to throw an error
> here for some cases -- especially for a '..' segment and perhaps also '.'.
>
It seems that there are FSFS-internal and maybe BDB-internal
callers of those functions that don't pass paths in canonical form.
I can't think of a reliable way to find all direct and indirect producers
of paths in FSFS, so we break some rarely used functionality.
Therefore, I suggest postponing the change to 1.9.
> API entry points
> > that must assume non canonical paths should be deprecated, with the
> > deprecated function handling the canonicalization. (Like
> > svn_path_url_add_component which had some assumptions built in).
>
> 4. Consider whether the FS API still needs to canonicalize its input paths
> in this way. I guess at the moment, for backward compatibility, it does,
> but we could rev the FS API and change that.
>
I think we should rev the API in 1.9 and eliminate internal
c14n from FSFS and BDB code. I can see that as being
another aspect in the general refactoring of FSFS code.
> > All our apis assume paths are canonical and I currently see code
> (get_dag())
> > that tries something and if it fails canonicalizes the path and tries
> again.
>
Looks weird but it speeds up typical usage of our FS API
layer: If we hit the cache, the path has been in canonical
form - otherwise, we would not have found the entry keyed
by this path. DAG lookup is really computationally expensive
these days compared to other parts.
> We should really get away from this kind of patterns wherever we can.
> > (Security boundaries might be an exception)
>
> Agreed.
>
No disagreement here.
-- Stefan^2.
--
Certified & Supported Apache Subversion Downloads:
*
http://www.wandisco.com/subversion/download
*
Re: r1442088 - Improve DAG node / path lookup performance in FSFS
Posted by Julian Foad <ju...@btopenworld.com>.
Bert Huijben wrote:
> [Julian]
> I don't like this 'is_canonical' flag. In the rest of Subversion, nearly
> all APIs assume paths are canonical already [...]
> I see that many of the functions in this file currently appear to be
> accepting and dealing with non-canonical paths, but I think the way to go is
> to change that.
> <<
>
> [Stefan]
> Turned out not to be too much work to fix all callers.
> <<
>
> I still see that we canonicalize the paths on +- every api. (About 20 calls
> in tree.c and dag.c)
>
> Eventually this api should move to something like an
> assert(is_canonical(path,…)) on all functions and just assume that callers
> do the right thing, like we do everywhere else
>
> Canonicalizing is everywhere is expensive and error prone.
Yes. I've just been looking at this, and I'd like to expand on the "error prone" aspect.
I don't know if we've ever fully specified what is a valid FS path, but I noticed that svn_fs__canonicalize_abspath() doesn't treat a "." path segment as special, whereas all our other path APIs treat a "." segment as non-canonical.
That means that if anything passes a path with a "." segment into the FS layer API, this canonicalization will not remove it and then some other function will throw an assertion failure or produce the wrong result, for some definition of 'wrong'. Indeed I tried inserting a '.' in a path in a call to svn_fs_node_id() in fs-base-test.c, and an assertion faliure was thrown.
Also svn_fs__canonicalize_abspath() accepts NULL but no code seems to require that -- at least the test suite still passes using FSFS when I remove that special case (and remove a test case explicitly testing that function with NULL).
PROPOSAL
1. Agree and document the spec for an FS layer path. Something like: sequence of non-null UTF-8 characters, leading '/', no trailing '/' unless the empty path, no '//', no '.' segments, no '..' segments. (There's no need for the FS layer to treat '.' and '..' as special, but since the higher layers do it's just easier if we choose the same rules and so can use the same path functions.)
2. Delete svn_fs__canonicalize_abspath(); use svn_fspath__canonicalize() instead; optimize svn_fspath__canonicalize(), and consider whether we want a variant of that function that doesn't always allocate in the new pool.
3. Move the canonicalization calls out of the vtable implementation functions, into the FS vtable wrapper functions. Consider whether it needs to canonicalize all non-canonical paths. May be better to throw an error here for some cases -- especially for a '..' segment and perhaps also '.'.
> API entry points
> that must assume non canonical paths should be deprecated, with the
> deprecated function handling the canonicalization. (Like
> svn_path_url_add_component which had some assumptions built in).
4. Consider whether the FS API still needs to canonicalize its input paths in this way. I guess at the moment, for backward compatibility, it does, but we could rev the FS API and change that.
> All our apis assume paths are canonical and I currently see code (get_dag())
> that tries something and if it fails canonicalizes the path and tries again.
>
> We should really get away from this kind of patterns wherever we can.
> (Security boundaries might be an exception)
Agreed.
- Julian
RE: r1442088 - Improve DAG node / path lookup performance in FSFS
Posted by Bert Huijben <be...@qqmail.nl>.
Replying on some part:
(Can we please use plain text. Im often guilty myself too, but this thread
is getting too far and I cant tell the difference between who wrote what)
[Julian]
I don't like this 'is_canonical' flag. In the rest of Subversion, nearly
all APIs assume paths are canonical already, and for a good reason:
re-canonicalizing at a lower level tends to be expensive because it happens
more times and with less knowledge of whether it's needed. I think it would
be better (including faster) to simply require that the path is canonical,
and make sure the callers honour that.
I see that many of the functions in this file currently appear to be
accepting and dealing with non-canonical paths, but I think the way to go is
to change that.
<<
[Stefan]
Turned out not to be too much work to fix all callers.
<<
I still see that we canonicalize the paths on +- every api. (About 20 calls
in tree.c and dag.c)
Eventually this api should move to something like an
assert(is_canonical(path,
)) on all functions and just assume that callers
do the right thing, like we do everywhere else
Canonicalizing is everywhere is expensive and error prone. API entry points
that must assume non canonical paths should be deprecated, with the
deprecated function handling the canonicalization. (Like
svn_path_url_add_component which had some assumptions built in).
All our apis assume paths are canonical and I currently see code (get_dag())
that tries something and if it fails canonicalizes the path and tries again.
We should really get away from this kind of patterns wherever we can.
(Security boundaries might be an exception)
Bert
From: Stefan Fuhrmann [mailto:stefan.fuhrmann@wandisco.com]
Sent: donderdag 21 februari 2013 22:56
To: Julian Foad
Cc: dev@subversion.apache.org
Subject: Re: r1442088 - Improve DAG node / path lookup performance in FSFS
On Mon, Feb 4, 2013 at 7:59 PM, Julian Foad <ju...@btopenworld.com>
wrote:
Hi Stefan.
> URL: http://svn.apache.org/viewvc?rev=1442088&view=rev
> Log:
> Improve DAG node / path lookup performance in FSFS. Since this is a hot
> path in many fs_repo-based functions (like fs_verify), 20 .. 30% time
> saved here can easily become significant.
>
> The idea here is that get_dag() already has knowledge about the PATH
> parameter and does not actually use all the result data returned by
> open_path(). So, define a number of flags that allow a caller to tell
> open_path() where it may take short-cuts based on the context.
>
> * subversion/libsvn_fs_fs/tree.c
> (dag_node_cache_set): add comment to document a rationale
> (enum open_path_flags_t): declare further flags
> (open_path): support the new flags to short-circuit portions of code
> (get_dag,
> fs_closest_copy,
> get_mergeinfo_for_path_internal): provide flags to open_path()
> + /* When this flag is set, don't bother to lookup the DAG node in
> + our caches because we already tried this. Ignoring this flag
> + has no functional impact. */
> + open_path_uncached = 2,
> +
> + /* Assume that the path parameter is already in canonical form. */
> + open_path_is_canonical = 4,
I don't like this 'is_canonical' flag. In the rest of Subversion, nearly
all APIs assume paths are canonical already, and for a good reason:
re-canonicalizing at a lower level tends to be expensive because it happens
more times and with less knowledge of whether it's needed. I think it would
be better (including faster) to simply require that the path is canonical,
and make sure the callers honour that.
I see that many of the functions in this file currently appear to be
accepting and dealing with non-canonical paths, but I think the way to go is
to change that.
Turned out not to be too much work to fix all callers.
> + /* The caller does not care about the parent node chain but only
> + the final DAG node. */
> + open_path_node_only = 8
> } open_path_flags_t;
>
>
> @@ -871,6 +885,10 @@ typedef enum open_path_flags_t {
> callers that create new nodes --- we find the parent directory for
> them, and tell them whether the entry exists already.
>
> + The remaining bits in FLAGS are hints that allow this function
> + to take shortcuts based on knowledge that the caller provides,
> + such as the fact that PATH is already in canonical form.
Maybe give a different example.
Done.
> NOTE: Public interfaces which only *read* from the filesystem
> should not call this function directly, but should instead use
> get_dag().
> @@ -884,23 +902,47 @@ open_path(parent_path_t **parent_path_p,
> apr_pool_t *pool)
> {
> svn_fs_t *fs = root->fs;
> - dag_node_t *here; /* The directory we're currently looking at. */
> - parent_path_t *parent_path; /* The path from HERE up to the root. */
> + dag_node_t *here = NULL; /* The directory we're currently looking at.
*/
> + parent_path_t *parent_path; /* The path from HERE up to the root. */
> const char *rest; /* The portion of PATH we haven't traversed yet. */
> - const char *canon_path = svn_fs__is_canonical_abspath(path)
> +
> + /* ensure a canonical path representation */
> + const char *canon_path = ( (flags & open_path_is_canonical)
> + || svn_fs__is_canonical_abspath(path))
> ? path
> : svn_fs__canonicalize_abspath(path, pool);
Here you seem also to have decided that when canonicalizing, it's more
efficient to test with svn_fs__is_canonical_abspath() before calling
svn_fs__canonicalize_abspath(). Are you sure that is true? If it is true
here (and I see you already have the same pattern in one of the callers),
then perhaps it is true in general, so why not do it inside
svn_fs__canonicalize_abspath() so all callers benefit?
It's now all inside svn_fs__canonicalize_abspath, which has now
subtly changed semantics. But all existing callers should be fine
with that.
> @@ -1142,7 +1195,9 @@ get_dag(dag_node_t **dag_node_p,
> {
> /* Call open_path with no flags, as we want this to return an
> * error if the node for which we are searching doesn't exist.
*/
The comment was falsified by your change and needs updating. (There are
similar comments on open_path() calls elsewhere in the file, which probably
should be updated in the same way even though those calls are still passing
no flags.)
> - SVN_ERR(open_path(&parent_path, root, path, 0, NULL, pool));
> + SVN_ERR(open_path(&parent_path, root, path,
> + open_path_uncached | open_path_is_canonical
> + | open_path_node_only, NULL, pool));
(This is the only significant place where the 'is_canonical' flag is used.)
> node = parent_path->node;
>
> /* No need to cache our find -- open_path() will do that for
us. */
> @@ -3162,7 +3217,7 @@ static svn_error_t *fs_closest_copy(svn_
> if (kind == svn_node_none)
> return SVN_NO_ERROR;
> SVN_ERR(open_path(©_dst_parent_path, copy_dst_root, path,
> - 0, NULL, pool));
> + open_path_node_only, NULL, pool));
> copy_dst_node = copy_dst_parent_path->node;
> if (! svn_fs_fs__id_check_related(svn_fs_fs__dag_get_id(copy_dst_node),
>
> svn_fs_fs__dag_get_id(parent_path->node)))
> @@ -3775,7 +3830,8 @@ get_mergeinfo_for_path_internal(svn_merg
>
> path = svn_fs__canonicalize_abspath(path, scratch_pool);
>
> - SVN_ERR(open_path(&parent_path, rev_root, path, 0, NULL,
scratch_pool));
> + SVN_ERR(open_path(&parent_path, rev_root, path, open_path_is_canonical,
> + NULL, scratch_pool));
Here you've left a plain 'canonicalize' in the caller, even though you
appear to think that the version inside open_path() is more efficient. (The
result of this 'canonicalize' here is *only* passed to open_path().)
> if (inherit == svn_mergeinfo_nearest_ancestor && !
> parent_path->parent)
> return SVN_NO_ERROR;
- Julian
Changes committed in r1448810 with a follow-up in r1448820.
Thanks for the review!
-- Stefan^2.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
Re: r1442088 - Improve DAG node / path lookup performance in FSFS
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Feb 4, 2013 at 7:59 PM, Julian Foad <ju...@btopenworld.com>wrote:
> Hi Stefan.
>
> > URL: http://svn.apache.org/viewvc?rev=1442088&view=rev
> > Log:
> > Improve DAG node / path lookup performance in FSFS. Since this is a hot
> > path in many fs_repo-based functions (like fs_verify), 20 .. 30% time
> > saved here can easily become significant.
> >
> > The idea here is that get_dag() already has knowledge about the PATH
> > parameter and does not actually use all the result data returned by
> > open_path(). So, define a number of flags that allow a caller to tell
> > open_path() where it may take short-cuts based on the context.
> >
> > * subversion/libsvn_fs_fs/tree.c
> > (dag_node_cache_set): add comment to document a rationale
> > (enum open_path_flags_t): declare further flags
> > (open_path): support the new flags to short-circuit portions of code
> > (get_dag,
> > fs_closest_copy,
> > get_mergeinfo_for_path_internal): provide flags to open_path()
>
>
> > + /* When this flag is set, don't bother to lookup the DAG node in
> > + our caches because we already tried this. Ignoring this flag
> > + has no functional impact. */
> > + open_path_uncached = 2,
> > +
> > + /* Assume that the path parameter is already in canonical form. */
> > + open_path_is_canonical = 4,
>
> I don't like this 'is_canonical' flag. In the rest of Subversion, nearly
> all APIs assume paths are canonical already, and for a good reason:
> re-canonicalizing at a lower level tends to be expensive because it happens
> more times and with less knowledge of whether it's needed. I think it
> would be better (including faster) to simply require that the path is
> canonical, and make sure the callers honour that.
>
> I see that many of the functions in this file currently appear to be
> accepting and dealing with non-canonical paths, but I think the way to go
> is to change that.
>
Turned out not to be too much work to fix all callers.
> + /* The caller does not care about the parent node chain but only
> > + the final DAG node. */
> > + open_path_node_only = 8
> > } open_path_flags_t;
> >
> >
> > @@ -871,6 +885,10 @@ typedef enum open_path_flags_t {
> > callers that create new nodes --- we find the parent directory for
> > them, and tell them whether the entry exists already.
> >
> > + The remaining bits in FLAGS are hints that allow this function
> > + to take shortcuts based on knowledge that the caller provides,
> > + such as the fact that PATH is already in canonical form.
>
> Maybe give a different example.
>
Done.
> > NOTE: Public interfaces which only *read* from the filesystem
> > should not call this function directly, but should instead use
> > get_dag().
> > @@ -884,23 +902,47 @@ open_path(parent_path_t **parent_path_p,
> > apr_pool_t *pool)
> > {
> > svn_fs_t *fs = root->fs;
> > - dag_node_t *here; /* The directory we're currently looking at. */
> > - parent_path_t *parent_path; /* The path from HERE up to the root. */
> > + dag_node_t *here = NULL; /* The directory we're currently looking
> at. */
> > + parent_path_t *parent_path; /* The path from HERE up to the root. */
> > const char *rest; /* The portion of PATH we haven't traversed yet. */
> > - const char *canon_path = svn_fs__is_canonical_abspath(path)
> > +
> > + /* ensure a canonical path representation */
> > + const char *canon_path = ( (flags & open_path_is_canonical)
> > + || svn_fs__is_canonical_abspath(path))
> > ? path
> > : svn_fs__canonicalize_abspath(path, pool);
>
> Here you seem also to have decided that when canonicalizing, it's more
> efficient to test with svn_fs__is_canonical_abspath() before calling
> svn_fs__canonicalize_abspath(). Are you sure that is true? If it is true
> here (and I see you already have the same pattern in one of the callers),
> then perhaps it is true in general, so why not do it inside
> svn_fs__canonicalize_abspath() so all callers benefit?
>
It's now all inside svn_fs__canonicalize_abspath, which has now
subtly changed semantics. But all existing callers should be fine
with that.
> @@ -1142,7 +1195,9 @@ get_dag(dag_node_t **dag_node_p,
> > {
> > /* Call open_path with no flags, as we want this to return an
> > * error if the node for which we are searching doesn't
> exist. */
>
> The comment was falsified by your change and needs updating. (There are
> similar comments on open_path() calls elsewhere in the file, which probably
> should be updated in the same way even though those calls are still passing
> no flags.)
>
> > - SVN_ERR(open_path(&parent_path, root, path, 0, NULL, pool));
> > + SVN_ERR(open_path(&parent_path, root, path,
> > + open_path_uncached | open_path_is_canonical
> > + | open_path_node_only, NULL, pool));
>
>
> (This is the only significant place where the 'is_canonical' flag is used.)
>
> > node = parent_path->node;
> >
> > /* No need to cache our find -- open_path() will do that for
> us. */
> > @@ -3162,7 +3217,7 @@ static svn_error_t *fs_closest_copy(svn_
> > if (kind == svn_node_none)
> > return SVN_NO_ERROR;
> > SVN_ERR(open_path(©_dst_parent_path, copy_dst_root, path,
> > - 0, NULL, pool));
> > + open_path_node_only, NULL, pool));
> > copy_dst_node = copy_dst_parent_path->node;
> > if (!
> svn_fs_fs__id_check_related(svn_fs_fs__dag_get_id(copy_dst_node),
> >
> > svn_fs_fs__dag_get_id(parent_path->node)))
> > @@ -3775,7 +3830,8 @@ get_mergeinfo_for_path_internal(svn_merg
> >
> > path = svn_fs__canonicalize_abspath(path, scratch_pool);
> >
> > - SVN_ERR(open_path(&parent_path, rev_root, path, 0, NULL,
> scratch_pool));
> > + SVN_ERR(open_path(&parent_path, rev_root, path,
> open_path_is_canonical,
> > + NULL, scratch_pool));
>
>
> Here you've left a plain 'canonicalize' in the caller, even though you
> appear to think that the version inside open_path() is more efficient.
> (The result of this 'canonicalize' here is *only* passed to open_path().)
>
>
> > if (inherit == svn_mergeinfo_nearest_ancestor && !
> > parent_path->parent)
> > return SVN_NO_ERROR;
>
>
> - Julian
>
>
Changes committed in r1448810 with a follow-up in r1448820.
Thanks for the review!
-- Stefan^2.
--
Certified & Supported Apache Subversion Downloads:
*
http://www.wandisco.com/subversion/download
*