You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by rh...@apache.org on 2015/09/29 14:16:04 UTC
svn commit: r1705843 - in /subversion/trunk/subversion:
libsvn_client/externals.c tests/cmdline/externals_tests.py
Author: rhuijben
Date: Tue Sep 29 12:16:04 2015
New Revision: 1705843
URL: http://svn.apache.org/viewvc?rev=1705843&view=rev
Log:
Remove registration of external in the EXTERNALS table when we find a
versioned node that takes its place during update.
* subversion/libsvn_client/externals.c
(switch_dir_external): Check node for wcroot before trying to handle as wc.
* subversion/tests/cmdline/externals_tests.py
(external_externally_removed): New test.
(test_list): Add external_externally_removed.
Modified:
subversion/trunk/subversion/libsvn_client/externals.c
subversion/trunk/subversion/tests/cmdline/externals_tests.py
Modified: subversion/trunk/subversion/libsvn_client/externals.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/externals.c?rev=1705843&r1=1705842&r2=1705843&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/externals.c (original)
+++ subversion/trunk/subversion/libsvn_client/externals.c Tue Sep 29 12:16:04 2015
@@ -231,6 +231,40 @@ switch_dir_external(const char *local_ab
if (node_url)
{
+ svn_boolean_t is_wcroot;
+
+ SVN_ERR(svn_wc__is_wcroot(&is_wcroot, ctx->wc_ctx, local_abspath,
+ pool));
+
+ if (! is_wcroot)
+ {
+ /* This can't be a directory external! */
+
+ err = svn_wc__external_remove(ctx->wc_ctx, defining_abspath,
+ local_abspath,
+ TRUE /* declaration_only */,
+ ctx->cancel_func, ctx->cancel_baton,
+ pool);
+
+ if (err && err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND)
+ {
+ /* New external... No problem that we can't remove it */
+ svn_error_clear(err);
+ err = NULL;
+ }
+
+ return svn_error_createf(SVN_ERR_WC_PATH_UNEXPECTED_STATUS, err,
+ _("The external '%s' defined in %s at '%s' "
+ "cannot be checked out because '%s' is "
+ "already a versioned path."),
+ url_from_externals_definition,
+ SVN_PROP_EXTERNALS,
+ svn_dirent_local_style(defining_abspath,
+ pool),
+ svn_dirent_local_style(local_abspath,
+ pool));
+ }
+
/* If we have what appears to be a version controlled
subdir, and its top-level URL matches that of our
externals definition, perform an update. */
Modified: subversion/trunk/subversion/tests/cmdline/externals_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/externals_tests.py?rev=1705843&r1=1705842&r2=1705843&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/externals_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/externals_tests.py Tue Sep 29 12:16:04 2015
@@ -4302,6 +4302,57 @@ def file_external_recorded_info(sbox):
}]
svntest.actions.run_and_verify_info(expected_infos, sbox.ospath('i'))
+def external_externally_removed(sbox):
+ "external externally removed"
+
+ sbox.build(read_only = True)
+
+ sbox.simple_propset('svn:externals', '^/A/B B', '')
+
+ # Try fetching the external with a versioned obstruction
+ sbox.simple_mkdir('B')
+ expected_err = ".*W155035: The external.*B' is already a versioned path"
+ svntest.actions.run_and_verify_svn(None, expected_err,
+ 'up', sbox.wc_dir)
+ sbox.simple_rm('B')
+
+
+ os.makedirs(sbox.ospath('B'))
+ expected_err2 = "svn: warning: W155007:.*B'"
+ svntest.actions.run_and_verify_svn(None, expected_err2,
+ 'up', sbox.wc_dir)
+ os.rmdir(sbox.ospath('B'))
+
+ # Fetch the external
+ sbox.simple_update()
+
+ svntest.main.safe_rmtree(sbox.ospath('B'))
+ sbox.simple_update() # Fetched again
+ if not os.path.isdir(sbox.ospath('B')):
+ raise svntest.Failure("B not recreated")
+
+ svntest.main.safe_rmtree(sbox.ospath('B'))
+ sbox.simple_propdel('svn:externals', '')
+
+ expected_output = [
+ "Updating '%s':\n" % sbox.wc_dir,
+ "Removed external '%s'\n" % sbox.ospath('B'),
+ "Updated to revision 1.\n"
+ ]
+ svntest.actions.run_and_verify_svn(expected_output, [],
+ 'up', sbox.wc_dir)
+
+
+ sbox.simple_propset('svn:externals', '^/A/B B', '')
+ sbox.simple_update()
+ svntest.main.safe_rmtree(sbox.ospath('B'))
+ sbox.simple_mkdir('B')
+
+ svntest.actions.run_and_verify_svn(None, expected_err,
+ 'up', sbox.wc_dir)
+
+ sbox.simple_propdel('svn:externals', '')
+ sbox.simple_update() # Should succeed
########################################################################
# Run the tests
@@ -4377,6 +4428,7 @@ test_list = [ None,
nested_notification,
file_external_to_normal_file,
file_external_recorded_info,
+ external_externally_removed,
]
if __name__ == '__main__':
RE: svn commit: r1705843 - in /subversion/trunk/subversion: libsvn_client/externals.c tests/cmdline/externals_tests.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
> Sent: maandag 26 oktober 2015 09:05
> To: dev@subversion.apache.org; Bert Huijben <be...@qqmail.nl>
> Subject: Re: svn commit: r1705843 - in /subversion/trunk/subversion:
> libsvn_client/externals.c tests/cmdline/externals_tests.py
>
> On 29 September 2015 at 15:16, <rh...@apache.org> wrote:
> > Author: rhuijben
> > Date: Tue Sep 29 12:16:04 2015
> > New Revision: 1705843
> >
> > URL: http://svn.apache.org/viewvc?rev=1705843&view=rev
> > Log:
> > Remove registration of external in the EXTERNALS table when we find a
> > versioned node that takes its place during update.
> >
>
> >
> ==========================================================
> ====================
> > --- subversion/trunk/subversion/libsvn_client/externals.c (original)
> > +++ subversion/trunk/subversion/libsvn_client/externals.c Tue Sep 29
> 12:16:04 2015
> > @@ -231,6 +231,40 @@ switch_dir_external(const char *local_ab
> >
> > if (node_url)
> > {
> > + svn_boolean_t is_wcroot;
> > +
> > + SVN_ERR(svn_wc__is_wcroot(&is_wcroot, ctx->wc_ctx,
> local_abspath,
> > + pool));
> > +
> > + if (! is_wcroot)
> > + {
> > + /* This can't be a directory external! */
> > +
> > + err = svn_wc__external_remove(ctx->wc_ctx, defining_abspath,
> > + local_abspath,
> > + TRUE /* declaration_only */,
> > + ctx->cancel_func, ctx->cancel_baton,
> > + pool);
> > +
> > + if (err && err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND)
> > + {
> > + /* New external... No problem that we can't remove it */
> > + svn_error_clear(err);
> > + err = NULL;
> > + }
> > +
> > + return
> svn_error_createf(SVN_ERR_WC_PATH_UNEXPECTED_STATUS, err,
> Hi Bert,
>
> I'm not sure that passing error from svn_wc__external_remove() to
> error chain is the good idea. Why do not return it in the same way
> like we handle error from svn_wc__is_wcroot()?
That first call can typically never error (as the location is at least below the primary working copy and the result already in the hashtable in wc_db), which makes the error handling mostly a no-op.
In the second case the current code guarantees that the actual result is always the same error... a pattern we use in many cases. The secondary error is not that interesting, while the unexpected status us.
The external_remove code only tries to remove the bad information to help you not see this error again, as you would have before this patch. If it fails nothing is lost. (But I don't like to just clear error chains).
Most likely users will never see the internal error anyway, as some outside code filters it to the top level error code only in the notification handling. (Processing errors in externals are usually not fatal)
Bert
Re: svn commit: r1705843 - in /subversion/trunk/subversion:
libsvn_client/externals.c tests/cmdline/externals_tests.py
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 26 October 2015 at 13:00, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
>> Sent: maandag 26 oktober 2015 09:05
>> To: dev@subversion.apache.org; Bert Huijben <be...@qqmail.nl>
>> Subject: Re: svn commit: r1705843 - in /subversion/trunk/subversion:
>> libsvn_client/externals.c tests/cmdline/externals_tests.py
>
>
>> ]]]
>> See attached patch.
>>
>> Otherwise error message could be confusing, in some cases. For example
>> if svn_wc__external_remove() return SVN_ERR_CANCELED error message
>> will be something like:
>> [[[
>> The external '%s' defined in %s at '%s' cannot be checked out because
>> '%s' is already a versioned path.
>> The operation was interrupted
>> ]]]
>
> I didn't really answer your mail:
>
> + 0.5 on the patch.
>
> I think either way would work. If you like to see it changed before the backport, feel free to add my +1 on your patch.
>
I've committed my patch in r1710558 and going to add to backport now.
--
Ivan Zhakov
RE: svn commit: r1705843 - in /subversion/trunk/subversion: libsvn_client/externals.c tests/cmdline/externals_tests.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Ivan Zhakov [mailto:ivan@visualsvn.com]
> Sent: maandag 26 oktober 2015 09:05
> To: dev@subversion.apache.org; Bert Huijben <be...@qqmail.nl>
> Subject: Re: svn commit: r1705843 - in /subversion/trunk/subversion:
> libsvn_client/externals.c tests/cmdline/externals_tests.py
> ]]]
> See attached patch.
>
> Otherwise error message could be confusing, in some cases. For example
> if svn_wc__external_remove() return SVN_ERR_CANCELED error message
> will be something like:
> [[[
> The external '%s' defined in %s at '%s' cannot be checked out because
> '%s' is already a versioned path.
> The operation was interrupted
> ]]]
I didn't really answer your mail:
+ 0.5 on the patch.
I think either way would work. If you like to see it changed before the backport, feel free to add my +1 on your patch.
Bert
Re: svn commit: r1705843 - in /subversion/trunk/subversion:
libsvn_client/externals.c tests/cmdline/externals_tests.py
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 29 September 2015 at 15:16, <rh...@apache.org> wrote:
> Author: rhuijben
> Date: Tue Sep 29 12:16:04 2015
> New Revision: 1705843
>
> URL: http://svn.apache.org/viewvc?rev=1705843&view=rev
> Log:
> Remove registration of external in the EXTERNALS table when we find a
> versioned node that takes its place during update.
>
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/externals.c (original)
> +++ subversion/trunk/subversion/libsvn_client/externals.c Tue Sep 29 12:16:04 2015
> @@ -231,6 +231,40 @@ switch_dir_external(const char *local_ab
>
> if (node_url)
> {
> + svn_boolean_t is_wcroot;
> +
> + SVN_ERR(svn_wc__is_wcroot(&is_wcroot, ctx->wc_ctx, local_abspath,
> + pool));
> +
> + if (! is_wcroot)
> + {
> + /* This can't be a directory external! */
> +
> + err = svn_wc__external_remove(ctx->wc_ctx, defining_abspath,
> + local_abspath,
> + TRUE /* declaration_only */,
> + ctx->cancel_func, ctx->cancel_baton,
> + pool);
> +
> + if (err && err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND)
> + {
> + /* New external... No problem that we can't remove it */
> + svn_error_clear(err);
> + err = NULL;
> + }
> +
> + return svn_error_createf(SVN_ERR_WC_PATH_UNEXPECTED_STATUS, err,
Hi Bert,
I'm not sure that passing error from svn_wc__external_remove() to
error chain is the good idea. Why do not return it in the same way
like we handle error from svn_wc__is_wcroot()?
I mean code like this:
[[
if (err && err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND)
{
/* New external... No problem that we can't remove it */
svn_error_clear(err);
}
else if (err)
return svn_error_trace(err);
return svn_error_createf(SVN_ERR_WC_PATH_UNEXPECTED_STATUS, NULL,
...
]]]
See attached patch.
Otherwise error message could be confusing, in some cases. For example
if svn_wc__external_remove() return SVN_ERR_CANCELED error message
will be something like:
[[[
The external '%s' defined in %s at '%s' cannot be checked out because
'%s' is already a versioned path.
The operation was interrupted
]]]
--
Ivan Zhakov