On Thu, Dec 17, 2009 at 08:40:15PM +0100, Daniel Näslund wrote:
> Hi!
>
> NOTE: This patch depends on my patch for #3390 that has not been
> commited yet [1]. The external_func uses abspaths and that is fixed in
> the referred patch. I have two more patches that depend on [1].
>
> make check passes with [1] applied.
I'm travelling, and because I am being disorganised I don't have [1]
in my mailbox right now. So I cannot reply there. Sorry about messing
up the threading.
Reviewing [1]:
> @@ -1193,19 +1196,30 @@
> ib.pool = cb->pool;
> ib.iter_pool = svn_pool_create(cb->pool);
>
> - /* Get the URL of the parent directory by appending a portion of
> - parent_dir to from_url. from_url is the URL for to_path and
> - to_path is a substring of parent_dir, so append any characters in
> - parent_dir past strlen(to_path) to from_url (making sure to move
> - past a '/' in parent_dir, otherwise svn_path_url_add_component()
> - will error. */
> - len = strlen(cb->to_path);
> - if (ib.parent_dir[len] == '/')
> - ++len;
> - ib.parent_dir_url = svn_path_url_add_component2(cb->from_url,
> - ib.parent_dir + len,
> - cb->pool);
> + SVN_ERR(svn_dirent_get_absolute(&abs_parent_dir, ib.parent_dir,
> + cb->pool));
>
> + err = svn_wc__node_get_url(&url, cb->ctx->wc_ctx,
> + abs_parent_dir, cb->pool, cb->pool);
> + ib.parent_dir_url = url;
I'd rather put "ib.parent_dir_url = url;" into a final 'else' clause
of the following if statement.
> +
> + if (err)
> + {
> + /* Get the URL of the parent directory by appending a portion of
> + parent_dir to from_url. from_url is the URL for to_path and
> + to_path is a substring of parent_dir, so append any characters in
> + parent_dir past strlen(to_path) to from_url (making sure to move
> + past a '/' in parent_dir, otherwise svn_path_url_add_component()
> + will error. */
> + len = strlen(cb->to_path);
> + if (ib.parent_dir[len] == '/')
> + ++len;
> + ib.parent_dir_url = svn_path_url_add_component2(cb->from_url,
> + ib.parent_dir + len,
> + cb->pool);
> + svn_error_clear(err);
> + }
The above ignores any error returned from svn_wc__node_get_url().
Usually, we only ignore specific errors, and pass others back to the caller.
The idiom goes like this:
if (err)
{
if (err->apr_err == ...)
{
/* ignore this */
svn_error_clear(err);
}
else
SVN_ERR(err);
}
Any reason why you made this code ignore all errors?
Stefan
Received on 2009-12-17 22:39:58 CET