Re: [PATCH] v3 Fix #3390 Relative externals not updated during switch

Hi!

The message from Stefan Sperling is copy-pasted from another thread that
refers to this one.

Comments inline, new log message at bottom.

On Thu, Dec 17, 2009 at 10:39:06PM +0100, Stefan Sperling wrote:
> 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);
> > + }

Fixed.

>
> 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?

I got assertions about some paths not beeing absolute. So until all
callers use absolute paths I catch all errors. I've added a note about
this in the code.

[[[
Fix issue #3390, relative externals not updated during switch.

* subversion/libsvn_client/switch.c
  (svn_client__switch_internal): Pass in an external_func to
    svn_wc_crawl_revision5().

* subversion/libsvn_client/externals.c
  (handle_externals_desc_change): Get the url for parent_dir with
    svn_wc__node_get_url(). Does not work for export where the
    parent_dir has no url. Then we resort to the old way of adding the
    parent_dir to the from_url.

* subversion/tests/cmdline/externals_tests.py
  (test_list): Remove XFail from switch_relative_external.

Patch by: Daniel Näslund <daniel{_AT_}longitudo.com>
Review by: stsp
]]]