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

On Mon, Dec 28, 2009 at 02:39:50PM +0100, Daniel Näslund wrote:
> On Mon, Dec 21, 2009 at 02:40:46PM +0000, Philip Martin wrote:
> > Daniel Näslund <daniel_at_longitudo.com> writes:
> >
> > > + /* First try to get the associated url and if there is none as is the case
> > > + * for exports then do use the user supplied values. */
> > > + err = svn_wc__node_get_url(&url, cb->ctx->wc_ctx,
> > > + abs_parent_dir, cb->pool, cb->pool);
> > > +
> > > + /* We want to check for just SVN_ERR_WC_PATH_NOT_FOUND here but since not
> > > + * all callers use absolute paths at the moment we get assertions about
> > > + * that, which we must catch. */
> > > + if (err)
> >
> > It would be better to check explicitly for PATH_NOT_FOUND and whatever
> > the error the assertion gives. Even better would be to fix the
> > callers so that the assertions don't happen. The problem with your
> > current code is that it's hiding problems, it means that we have to
> > remember this code and come back and fix it.
>
> I was wrong. It wasn't an assertion about abspaths. It was
> SVN_ERR_WC_NOT_WORKING_COPY that I for some reason did not get right.
> I've replaced the check for WC_PATH_NOT_FOUND with that one.
>
> I admit - I've considered a patch finished when there was no failing
> tests. But I'm trying to be better. My New Years resolution will be to
> not consider a solution finished until I've understood every part of it.
>
> make check passed.
>
> [[[
> Fix issue #3390, relative externals not updated during switch. As a side
> effect - allow externals hash diff functionality to be used with abspaths.
>
> * 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, philip
> ]]]
>
> Daniel

> Index: subversion/tests/cmdline/externals_tests.py
> ===================================================================
> --- subversion/tests/cmdline/externals_tests.py (revision 894131)
> +++ subversion/tests/cmdline/externals_tests.py (arbetskopia)
> @@ -1583,7 +1583,7 @@
> external_into_path_with_spaces,
> binary_file_externals,
> XFail(update_lose_file_external),
> - XFail(switch_relative_external),
> + switch_relative_external,
> export_sparse_wc_with_externals,
> relegate_external,
> wc_repos_file_externals,
> Index: subversion/libsvn_client/switch.c
> ===================================================================
> --- subversion/libsvn_client/switch.c (revision 894131)
> +++ subversion/libsvn_client/switch.c (arbetskopia)
> @@ -256,13 +256,15 @@
> PATH. When we call reporter->finish_report, the update_editor
> will be driven by svn_repos_dir_delta2.
>
> - We pass NULL for traversal_info because this is a switch, not an
> - update, and therefore we don't want to handle any externals
> - except the ones directly affected by the switch. */
> + We pass in an external_func for recording all externals. It
> + shouldn't be needed for a switch if it wasn't for the relative
> + externals of type '../path'. All of those must be resolved to
> + the new location. */
> err = svn_wc_crawl_revisions5(ctx->wc_ctx, local_abspath, reporter,
> report_baton, TRUE, depth, (! depth_is_sticky),
> (! server_supports_depth),
> - use_commit_times, NULL, NULL,
> + use_commit_times,
> + svn_client__external_info_gatherer, &efb,
> ctx->notify_func2, ctx->notify_baton2, pool);
>
> if (err)
> Index: subversion/libsvn_client/externals.c
> ===================================================================
> --- subversion/libsvn_client/externals.c (revision 894131)
> +++ subversion/libsvn_client/externals.c (arbetskopia)
> @@ -1088,6 +1088,9 @@
> int i;
> svn_wc_external_item2_t *item;
> const char *ambient_depth_w;
> + const char *url;
> + const char *abs_parent_dir;
> + svn_error_t *err;
> svn_depth_t ambient_depth;
>
> if (cb->is_export)
> @@ -1166,19 +1169,35 @@
> 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);
> +
> + /* If we're doing an 'svn export' the current dir will not be a
> + working copy. We can't get the parent_dir. */
> + if (err && err->apr_err == SVN_ERR_WC_NOT_WORKING_COPY)
> + {
> + /* 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);
> + }
> + else if (err)
> + svn_error_return(err);

         return svn_error_return(err);
         ^^^^^^
> + else
> + ib.parent_dir_url = url;

And maybe write it like this?

  if (err)
    {
      if (err->apr_arr == SVN_ERR_WC_NOT_WORKING_COPY)
        {
          ...
          svn_error_clear(err);
        }
      else
        return svn_error_return(err);
    }
  else
    ib.parent_dir_url = url;

Stefan

> +
> /* We must use a custom version of svn_hash_diff so that the diff
> entries are processed in the order they were originally specified
> in the svn:externals properties. */

-- 
printf("Eh???/n");