You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Noorul Islam K M <no...@collab.net> on 2011/07/14 07:14:54 UTC
Re: svn commit: r1145972 - in /subversion/trunk/subversion: libsvn_wc/wc-queries.sql libsvn_wc/wc_db.c svn/changelist-cmd.c tests/cmdline/changelist_tests.py tests/cmdline/tree_conflict_tests.py
rhuijben@apache.org writes:
> Author: rhuijben
> Date: Wed Jul 13 12:28:17 2011
> New Revision: 1145972
>
> URL: http://svn.apache.org/viewvc?rev=1145972&view=rev
> Log:
> Make the 'populate target tree' code in wc_db.c return an error when it
> receives an nonexisting node as (root-)target.
> (This also resolves some remaining parts of issue #3779)
>
> * subversion/libsvn_wc/wc-queries.sql
> (STMT_INSERT_TARGET_DEPTH_INFINITY,
> STMT_INSERT_TARGET_WITH_CHANGELIST_DEPTH_INFINITY): Avoid the LIKE operator
> when we can use an index to obtain the same result.
>
> * subversion/libsvn_wc/wc_db.c
> (populate_targets_tree): Don't pass/generate the like argument. Obtain the
> number of affected rows and check if the node actually exists if the number
> of affected rows is 0.
>
> * subversion/svn/changelist-cmd.c
> (svn_cl__changelist): Return an error when an error occurred, just like
> the other svn commands that handle SVN_WC_PATH_NOT_FOUND.
>
> * subversion/tests/cmdline/changelist_tests.py
> (add_remove_non_existent_target,
> add_remove_unversioned_target): New tests. Based on a patch by Noorul
> Islam K M, but tweaked for the different error handling.
> (test_list): Add new tests.
>
> * subversion/tests/cmdline/tree_conflict_tests.py
> (actual_only_node_behaviour): Update expected result and remove review marker
> as we now produce a warning.
>
> Found by: danielsh
> Noorul Islam K M <noorul{_AT_}collab.net>
>
> Modified:
> subversion/trunk/subversion/libsvn_wc/wc-queries.sql
> subversion/trunk/subversion/libsvn_wc/wc_db.c
> subversion/trunk/subversion/svn/changelist-cmd.c
> subversion/trunk/subversion/tests/cmdline/changelist_tests.py
> subversion/trunk/subversion/tests/cmdline/tree_conflict_tests.py
>
> Modified: subversion/trunk/subversion/libsvn_wc/wc-queries.sql
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc-queries.sql?rev=1145972&r1=1145971&r2=1145972&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/wc-queries.sql (original)
> +++ subversion/trunk/subversion/libsvn_wc/wc-queries.sql Wed Jul 13 12:28:17 2011
> @@ -473,7 +473,10 @@ WHERE wc_id = ?1 AND (parent_relpath = ?
> INSERT INTO targets_list(wc_id, local_relpath, parent_relpath, kind)
> SELECT wc_id, local_relpath, parent_relpath, kind
> FROM nodes_current
> -WHERE wc_id = ?1 AND (local_relpath = ?2 OR local_relpath LIKE ?3 ESCAPE '#')
> +WHERE wc_id = ?1
> + AND (?2 = ''
> + OR local_relpath = ?2
> + OR (local_relpath > ?2 || '/' AND local_relpath < ?2 || '0'))
>
> -- STMT_INSERT_TARGET_WITH_CHANGELIST
> INSERT INTO targets_list(wc_id, local_relpath, parent_relpath, kind)
> @@ -503,8 +506,11 @@ INSERT INTO targets_list(wc_id, local_re
> SELECT N.wc_id, N.local_relpath, N.parent_relpath, N.kind
> FROM actual_node AS A JOIN nodes_current AS N
> ON A.wc_id = N.wc_id AND A.local_relpath = N.local_relpath
> - WHERE N.wc_id = ?1 AND A.changelist = ?3
> - AND (N.local_relpath = ?2 OR N.local_relpath LIKE ?4 ESCAPE '#')
> + WHERE N.wc_id = ?1
> + AND (?2 = ''
> + OR N.local_relpath = ?2
> + OR (N.local_relpath > ?2 || '/' AND N.local_relpath < ?2 || '0'))
> + AND A.changelist = ?3
>
> -- STMT_SELECT_TARGETS
> SELECT local_relpath, parent_relpath from targets_list
>
> Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.c?rev=1145972&r1=1145971&r2=1145972&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/wc_db.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/wc_db.c Wed Jul 13 12:28:17 2011
> @@ -4741,14 +4741,7 @@ populate_targets_tree(svn_wc__db_wcroot_
> apr_pool_t *scratch_pool)
> {
> svn_sqlite__stmt_t *stmt;
> - const char *like_arg;
> -
> - if (depth == svn_depth_infinity)
> - {
> - /* Calculate a value we're going to need later. */
> - like_arg = construct_like_arg(local_relpath, scratch_pool);
> - }
> -
> + int affected_rows = 0;
> SVN_ERR(svn_sqlite__exec_statements(wcroot->sdb,
> STMT_CREATE_TARGETS_LIST));
>
> @@ -4786,15 +4779,16 @@ populate_targets_tree(svn_wc__db_wcroot_
>
> for (i = 0; i < changelist_filter->nelts; i++)
> {
> + int sub_affected;
> const char *changelist = APR_ARRAY_IDX(changelist_filter, i,
> const char *);
>
> SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb, stmt_idx));
> SVN_ERR(svn_sqlite__bindf(stmt, "iss", wcroot->wc_id,
> local_relpath, changelist));
> - if (depth == svn_depth_infinity)
> - SVN_ERR(svn_sqlite__bind_text(stmt, 4, like_arg));
> - SVN_ERR(svn_sqlite__step_done(stmt));
> + SVN_ERR(svn_sqlite__update(&sub_affected, stmt));
> +
> + affected_rows += sub_affected;
> }
> }
> else /* No changelist filtering */
> @@ -4827,9 +4821,21 @@ populate_targets_tree(svn_wc__db_wcroot_
>
> SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb, stmt_idx));
> SVN_ERR(svn_sqlite__bindf(stmt, "is", wcroot->wc_id, local_relpath));
> - if (depth == svn_depth_infinity)
> - SVN_ERR(svn_sqlite__bind_text(stmt, 3, like_arg));
> - SVN_ERR(svn_sqlite__step_done(stmt));
> + SVN_ERR(svn_sqlite__update(&affected_rows, stmt));
> + }
> +
> + /* Does the target exist? */
> + if (affected_rows == 0)
> + {
> + svn_boolean_t exists;
> + SVN_ERR(does_node_exist(&exists, wcroot, local_relpath));
> +
> + if (!exists)
> + return svn_error_createf(SVN_ERR_WC_PATH_NOT_FOUND, NULL,
> + _("The node '%s' was not found."),
> + path_for_error_message(wcroot,
> + local_relpath,
> + scratch_pool));
> }
>
> return SVN_NO_ERROR;
>
> Modified: subversion/trunk/subversion/svn/changelist-cmd.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/changelist-cmd.c?rev=1145972&r1=1145971&r2=1145972&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/svn/changelist-cmd.c (original)
> +++ subversion/trunk/subversion/svn/changelist-cmd.c Wed Jul 13 12:28:17 2011
> @@ -45,6 +45,7 @@ svn_cl__changelist(apr_getopt_t *os,
> svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
> apr_array_header_t *targets;
> svn_depth_t depth = opt_state->depth;
> + svn_boolean_t success = TRUE;
>
> /* If we're not removing changelists, then our first argument should
> be the name of a changelist. */
> @@ -98,24 +99,31 @@ svn_cl__changelist(apr_getopt_t *os,
>
> if (changelist_name)
> {
> - return svn_cl__try
> - (svn_client_add_to_changelist(targets, changelist_name,
> + SVN_ERR(svn_cl__try(
> + svn_client_add_to_changelist(targets, changelist_name,
> depth, opt_state->changelists,
> ctx, pool),
> - NULL, opt_state->quiet,
> + &success, opt_state->quiet,
> SVN_ERR_UNVERSIONED_RESOURCE,
> SVN_ERR_WC_PATH_NOT_FOUND,
> - SVN_NO_ERROR);
> + SVN_NO_ERROR));
> }
> else
> {
> - return svn_cl__try
> - (svn_client_remove_from_changelists(targets, depth,
> + SVN_ERR(svn_cl__try(
> + svn_client_remove_from_changelists(targets, depth,
> opt_state->changelists,
> ctx, pool),
> - NULL, opt_state->quiet,
> + &success, opt_state->quiet,
> SVN_ERR_UNVERSIONED_RESOURCE,
> SVN_ERR_WC_PATH_NOT_FOUND,
> - SVN_NO_ERROR);
> + SVN_NO_ERROR));
> }
> +
> + if (!success)
> + return svn_error_create(SVN_ERR_ILLEGAL_TARGET, NULL,
> + _("Could not display info for all targets because "
> + "some targets don't exist"));
I think this should be something like this.
Could not add all targets to changelist because some targets don't exist
I will add test cases for handling multiple targets.
> + else
> + return SVN_NO_ERROR;
> }
>
> Modified: subversion/trunk/subversion/tests/cmdline/changelist_tests.py
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/changelist_tests.py?rev=1145972&r1=1145971&r2=1145972&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/tests/cmdline/changelist_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/changelist_tests.py Wed Jul 13 12:28:17 2011
> @@ -1132,6 +1132,43 @@ def revert_deleted_in_changelist(sbox):
> 'revert', '-R', sbox.ospath('A'))
> svntest.actions.run_and_verify_info(expected_infos, sbox.ospath('A/mu'))
>
> +def add_remove_non_existent_target(sbox):
> + "add and remove non-existent target to changelist"
> +
> + sbox.build(read_only = True)
> + wc_dir = sbox.wc_dir
> + bogus_path = os.path.join(wc_dir, 'A', 'bogus')
> +
> + expected_err = "svn: warning: W155010: The node '" + \
> + re.escape(os.path.abspath(bogus_path)) + \
> + "' was not found"
This is actually different from what 1.6 displays. Is this fine?
Thanks and Regards
Noorul
RE: svn commit: r1145972 - in /subversion/trunk/subversion: libsvn_wc/wc-queries.sql libsvn_wc/wc_db.c svn/changelist-cmd.c tests/cmdline/changelist_tests.py tests/cmdline/tree_conflict_tests.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Noorul Islam K M [mailto:noorul@collab.net]
> Sent: donderdag 14 juli 2011 7:15
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1145972 - in /subversion/trunk/subversion:
> libsvn_wc/wc-queries.sql libsvn_wc/wc_db.c svn/changelist-cmd.c
> tests/cmdline/changelist_tests.py tests/cmdline/tree_conflict_tests.py
> > +
> > + if (!success)
> > + return svn_error_create(SVN_ERR_ILLEGAL_TARGET, NULL,
> > + _("Could not display info for all targets
because "
> > + "some targets don't exist"));
>
> I think this should be something like this.
>
> Could not add all targets to changelist because some targets don't exist
>
> I will add test cases for handling multiple targets.
I just copied the (already translated) message that most other commands use.
Technically the current text is correct, but I agree that it could be
improved.
>
> > + else
> > + return SVN_NO_ERROR;
> > }
> >
> > Modified: subversion/trunk/subversion/tests/cmdline/changelist_tests.py
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/c
> hangelist_tests.py?rev=1145972&r1=1145971&r2=1145972&view=diff
> >
> ==========================================================
> ====================
> > --- subversion/trunk/subversion/tests/cmdline/changelist_tests.py
> (original)
> > +++ subversion/trunk/subversion/tests/cmdline/changelist_tests.py Wed
> Jul 13 12:28:17 2011
> > @@ -1132,6 +1132,43 @@ def revert_deleted_in_changelist(sbox):
> > 'revert', '-R', sbox.ospath('A'))
> > svntest.actions.run_and_verify_info(expected_infos,
> sbox.ospath('A/mu'))
> >
> > +def add_remove_non_existent_target(sbox):
> > + "add and remove non-existent target to changelist"
> > +
> > + sbox.build(read_only = True)
> > + wc_dir = sbox.wc_dir
> > + bogus_path = os.path.join(wc_dir, 'A', 'bogus')
> > +
> > + expected_err = "svn: warning: W155010: The node '" + \
> > + re.escape(os.path.abspath(bogus_path)) + \
> > + "' was not found"
>
> This is actually different from what 1.6 displays. Is this fine?
- *All* errors and warnings from 'svn' are different from what 1.6 displays
as we added the error number for 1.7.
- Almost every error that talks about entries is replaced with something
that talks about nodes.
- And many errors now display absolute paths.
And I don't think we ever promised that the errors would be identical.
With this patch I just made sure that we return an error so scripts can rely
on just checking the exitcode instead of having to parse 'some warning'
Bert
[PATCH] Re: svn commit: r1145972 - in /subversion/trunk/subversion: libsvn_wc/wc-queries.sql libsvn_wc/wc_db.c svn/changelist-cmd.c tests/cmdline/changelist_tests.py tests/cmdline/tree_conflict_tests.py
Posted by Noorul Islam K M <no...@collab.net>.
Noorul Islam K M <no...@collab.net> writes:
> rhuijben@apache.org writes:
>
>> if (changelist_name)
>> {
>> - return svn_cl__try
>> - (svn_client_add_to_changelist(targets, changelist_name,
>> + SVN_ERR(svn_cl__try(
>> + svn_client_add_to_changelist(targets, changelist_name,
>> depth, opt_state->changelists,
>> ctx, pool),
>> - NULL, opt_state->quiet,
>> + &success, opt_state->quiet,
>> SVN_ERR_UNVERSIONED_RESOURCE,
>> SVN_ERR_WC_PATH_NOT_FOUND,
>> - SVN_NO_ERROR);
>> + SVN_NO_ERROR));
>> }
>> else
>> {
>> - return svn_cl__try
>> - (svn_client_remove_from_changelists(targets, depth,
>> + SVN_ERR(svn_cl__try(
>> + svn_client_remove_from_changelists(targets, depth,
>> opt_state->changelists,
>> ctx, pool),
>> - NULL, opt_state->quiet,
>> + &success, opt_state->quiet,
>> SVN_ERR_UNVERSIONED_RESOURCE,
>> SVN_ERR_WC_PATH_NOT_FOUND,
>> - SVN_NO_ERROR);
>> + SVN_NO_ERROR));
>> }
>> +
>> + if (!success)
>> + return svn_error_create(SVN_ERR_ILLEGAL_TARGET, NULL,
>> + _("Could not display info for all targets because "
>> + "some targets don't exist"));
>
> I think this should be something like this.
>
> Could not add all targets to changelist because some targets don't exist
>
> I will add test cases for handling multiple targets.
>
Log
[[[
* subversion/svn/changelist-cmd.c
(svn_cl__changelist): Tweak error message. Also display different
error message for add and remove.
* subversion/tests/cmdline/changelist_tests.py
(mix_existing_and_non_existent_target): New test, verify that svn cl
errors out in desirable way if mixture for existing and non-existent
targets are passed.
(test_list): Add new test.
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Thanks and Regards
Noorul