You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ne...@apache.org on 2012/11/15 17:57:06 UTC
svn commit: r1409883 - in /subversion/trunk/subversion:
libsvn_wc/externals.c tests/cmdline/externals_tests.py
Author: neels
Date: Thu Nov 15 16:57:05 2012
New Revision: 1409883
URL: http://svn.apache.org/viewvc?rev=1409883&view=rev
Log:
Implement issue #4227: Externals: Multiple definitions for the same path;
limitation: only check for duplicates within the same svn:externals prop.
* subversion/libsvn_wc/externals.c
(svn_wc_parse_externals_description3):
Return an error when DESC contains the same WC target twice (or more). For
that purpose, always compose the list of items, regardless of whether it
will be returned or not. Side effect: if EXTERNALS_P is nonzero, and if
parsing of DESC returns an error,
* subversion/tests/cmdline/externals_tests.py
(duplicate_targets): New test for above.
Modified:
subversion/trunk/subversion/libsvn_wc/externals.c
subversion/trunk/subversion/tests/cmdline/externals_tests.py
Modified: subversion/trunk/subversion/libsvn_wc/externals.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/externals.c?rev=1409883&r1=1409882&r2=1409883&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/externals.c (original)
+++ subversion/trunk/subversion/libsvn_wc/externals.c Thu Nov 15 16:57:05 2012
@@ -164,15 +164,16 @@ svn_wc_parse_externals_description3(apr_
apr_pool_t *pool)
{
int i;
+ int j;
apr_array_header_t *externals = NULL;
apr_array_header_t *lines = svn_cstring_split(desc, "\n\r", TRUE, pool);
const char *parent_directory_display = svn_path_is_url(parent_directory) ?
parent_directory : svn_dirent_local_style(parent_directory, pool);
/* If an error occurs halfway through parsing, *externals_p should stay
- * untouched. So, store the list in a local var first. */
- if (externals_p)
- externals = apr_array_make(pool, 1, sizeof(svn_wc_external_item2_t *));
+ * untouched. So, store the list in a local var first. Since we are checking
+ * for duplicate targets, always compose the list regardless. */
+ externals = apr_array_make(pool, 1, sizeof(svn_wc_external_item2_t *));
for (i = 0; i < lines->nelts; i++)
{
@@ -330,8 +331,23 @@ svn_wc_parse_externals_description3(apr_
item->url = svn_dirent_canonicalize(item->url, pool);
}
- if (externals)
- APR_ARRAY_PUSH(externals, svn_wc_external_item2_t *) = item;
+ /* Has the same WC target path already been mentioned in this prop? */
+ for (j = 0; j < externals->nelts; j++)
+ {
+ svn_wc_external_item2_t *other_item =
+ APR_ARRAY_IDX(externals, j, svn_wc_external_item2_t *);
+
+ if (strcmp(item->target_dir, other_item->target_dir) == 0)
+ return svn_error_createf
+ (SVN_ERR_CLIENT_INVALID_EXTERNALS_DESCRIPTION, NULL,
+ _("Invalid %s property on '%s': "
+ "target '%s' appears more than once"),
+ SVN_PROP_EXTERNALS,
+ parent_directory_display,
+ item->target_dir);
+ }
+
+ APR_ARRAY_PUSH(externals, svn_wc_external_item2_t *) = item;
}
if (externals_p)
Modified: subversion/trunk/subversion/tests/cmdline/externals_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/externals_tests.py?rev=1409883&r1=1409882&r2=1409883&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/externals_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/externals_tests.py Thu Nov 15 16:57:05 2012
@@ -2878,6 +2878,68 @@ def url_to_wc_copy_of_externals(sbox):
"OUTPUT", expected_stdout, [], 0, 'copy', repo_url + '/A/C',
sbox.ospath('External-WC-to-URL-Copy'))
+@Issue(4227)
+def duplicate_targets(sbox):
+ "local path appears twice in one svn:external prop"
+
+ if False:
+ svntest.factory.make(sbox, r"""
+ svn ps svn:externals "^/A/B/E barf\n^/A/B/E barf" .
+ svn ps svn:externals "^/A/B/E barf\n^/A/D/G barf" .
+ svn ps svn:externals "^/A/B/E barf/.\n^/A/D/G ./barf" .
+ svn ps svn:externals "^/A/B/E ././barf\n^/A/D/G .//barf" .
+ svn pl -v .
+ svn ps svn:externals "^/A/B/E ok" .
+ svn pl -v .
+ """)
+
+ sbox.build()
+ wc_dir = sbox.wc_dir
+ abs_wc_dir = os.path.abspath(sbox.wc_dir)
+
+ expected_stderr = verify.RegexOutput(
+ ".*Invalid svn:externals property on '" + abs_wc_dir +
+ "': target 'barf' appears more than once\n",
+ match_all=False)
+
+ # svn ps svn:externals "^/A/B/E barf\n^/A/B/E barf" .
+ actions.run_and_verify_svn2('OUTPUT', [], expected_stderr, 1, 'ps',
+ 'svn:externals', '^/A/B/E barf\n^/A/B/E barf', wc_dir)
+
+ # svn ps svn:externals "^/A/B/E barf\n^/A/D/G barf" .
+ actions.run_and_verify_svn2('OUTPUT', [], expected_stderr, 1, 'ps',
+ 'svn:externals', '^/A/B/E barf\n^/A/D/G barf', wc_dir)
+
+ # svn ps svn:externals "^/A/B/E barf/.\n^/A/D/G ./barf" .
+ actions.run_and_verify_svn2('OUTPUT', [], expected_stderr, 1, 'ps',
+ 'svn:externals', '^/A/B/E barf/.\n^/A/D/G ./barf', wc_dir)
+
+ # svn ps svn:externals "^/A/B/E ././barf\n^/A/D/G .//barf" .
+ actions.run_and_verify_svn2('OUTPUT', [], expected_stderr, 1, 'ps',
+ 'svn:externals', '^/A/B/E ././barf\n^/A/D/G .//barf', wc_dir)
+
+ # svn pl -v .
+ actions.run_and_verify_svn2('OUTPUT', [], [], 0, 'pl', '-v',
+ wc_dir)
+
+ # svn ps svn:externals "^/A/B/E ok" .
+ expected_stdout = ["property 'svn:externals' set on '" + wc_dir + "'\n"]
+
+ actions.run_and_verify_svn2('OUTPUT', expected_stdout, [], 0, 'ps',
+ 'svn:externals', '^/A/B/E ok', wc_dir)
+
+ # svn pl -v .
+ expected_stdout = [
+ "Properties on '" + wc_dir + "':\n",
+ ' svn:externals\n',
+ ' ^/A/B/E ok\n',
+ ' \n',
+ ]
+
+ actions.run_and_verify_svn2('OUTPUT', expected_stdout, [], 0, 'pl', '-v',
+ wc_dir)
+
+
########################################################################
# Run the tests
@@ -2925,6 +2987,7 @@ test_list = [ None,
remap_file_external_with_prop_del,
dir_external_with_dash_r_only,
url_to_wc_copy_of_externals,
+ duplicate_targets,
]
if __name__ == '__main__':
Re: svn commit: r1409883 - in /subversion/trunk/subversion:
libsvn_wc/externals.c tests/cmdline/externals_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
neels@apache.org wrote on Thu, Nov 15, 2012 at 16:57:06 -0000:
> Author: neels
> Date: Thu Nov 15 16:57:05 2012
> New Revision: 1409883
>
> URL: http://svn.apache.org/viewvc?rev=1409883&view=rev
> Log:
> Implement issue #4227: Externals: Multiple definitions for the same path;
> limitation: only check for duplicates within the same svn:externals prop.
>
> @@ -164,15 +164,16 @@ svn_wc_parse_externals_description3(apr_
> apr_pool_t *pool)
> {
> int i;
> + int j;
> apr_array_header_t *externals = NULL;
> apr_array_header_t *lines = svn_cstring_split(desc, "\n\r", TRUE, pool);
> const char *parent_directory_display = svn_path_is_url(parent_directory) ?
> parent_directory : svn_dirent_local_style(parent_directory, pool);
>
> /* If an error occurs halfway through parsing, *externals_p should stay
> - * untouched. So, store the list in a local var first. */
> - if (externals_p)
> - externals = apr_array_make(pool, 1, sizeof(svn_wc_external_item2_t *));
> + * untouched. So, store the list in a local var first. Since we are checking
> + * for duplicate targets, always compose the list regardless. */
> + externals = apr_array_make(pool, 1, sizeof(svn_wc_external_item2_t *));
>
> for (i = 0; i < lines->nelts; i++)
> {
> @@ -330,8 +331,23 @@ svn_wc_parse_externals_description3(apr_
> item->url = svn_dirent_canonicalize(item->url, pool);
> }
>
> - if (externals)
> - APR_ARRAY_PUSH(externals, svn_wc_external_item2_t *) = item;
> + /* Has the same WC target path already been mentioned in this prop? */
> + for (j = 0; j < externals->nelts; j++)
> + {
This looks like an O(n²) algorithm, maybe implement something more
efficient?
e.g., you could construct a hash with all ->target_dir's as keys, and
then check whether (apr_hash_count() == externals->nelts).
> + svn_wc_external_item2_t *other_item =
> + APR_ARRAY_IDX(externals, j, svn_wc_external_item2_t *);
> +
> + if (strcmp(item->target_dir, other_item->target_dir) == 0)
> + return svn_error_createf
> + (SVN_ERR_CLIENT_INVALID_EXTERNALS_DESCRIPTION, NULL,
> + _("Invalid %s property on '%s': "
> + "target '%s' appears more than once"),
> + SVN_PROP_EXTERNALS,
> + parent_directory_display,
> + item->target_dir);
> + }
> +
> + APR_ARRAY_PUSH(externals, svn_wc_external_item2_t *) = item;
> }
>
> if (externals_p)
>
Re: svn commit: r1409883 - in /subversion/trunk/subversion:
libsvn_wc/externals.c tests/cmdline/externals_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
neels@apache.org wrote on Thu, Nov 15, 2012 at 16:57:06 -0000:
> Author: neels
> Date: Thu Nov 15 16:57:05 2012
> New Revision: 1409883
>
> URL: http://svn.apache.org/viewvc?rev=1409883&view=rev
> Log:
> Implement issue #4227: Externals: Multiple definitions for the same path;
> limitation: only check for duplicates within the same svn:externals prop.
>
> @@ -164,15 +164,16 @@ svn_wc_parse_externals_description3(apr_
> apr_pool_t *pool)
> {
> int i;
> + int j;
> apr_array_header_t *externals = NULL;
> apr_array_header_t *lines = svn_cstring_split(desc, "\n\r", TRUE, pool);
> const char *parent_directory_display = svn_path_is_url(parent_directory) ?
> parent_directory : svn_dirent_local_style(parent_directory, pool);
>
> /* If an error occurs halfway through parsing, *externals_p should stay
> - * untouched. So, store the list in a local var first. */
> - if (externals_p)
> - externals = apr_array_make(pool, 1, sizeof(svn_wc_external_item2_t *));
> + * untouched. So, store the list in a local var first. Since we are checking
> + * for duplicate targets, always compose the list regardless. */
> + externals = apr_array_make(pool, 1, sizeof(svn_wc_external_item2_t *));
>
> for (i = 0; i < lines->nelts; i++)
> {
> @@ -330,8 +331,23 @@ svn_wc_parse_externals_description3(apr_
> item->url = svn_dirent_canonicalize(item->url, pool);
> }
>
> - if (externals)
> - APR_ARRAY_PUSH(externals, svn_wc_external_item2_t *) = item;
> + /* Has the same WC target path already been mentioned in this prop? */
> + for (j = 0; j < externals->nelts; j++)
> + {
This looks like an O(n²) algorithm, maybe implement something more
efficient?
e.g., you could construct a hash with all ->target_dir's as keys, and
then check whether (apr_hash_count() == externals->nelts).
> + svn_wc_external_item2_t *other_item =
> + APR_ARRAY_IDX(externals, j, svn_wc_external_item2_t *);
> +
> + if (strcmp(item->target_dir, other_item->target_dir) == 0)
> + return svn_error_createf
> + (SVN_ERR_CLIENT_INVALID_EXTERNALS_DESCRIPTION, NULL,
> + _("Invalid %s property on '%s': "
> + "target '%s' appears more than once"),
> + SVN_PROP_EXTERNALS,
> + parent_directory_display,
> + item->target_dir);
> + }
> +
> + APR_ARRAY_PUSH(externals, svn_wc_external_item2_t *) = item;
> }
>
> if (externals_p)
>