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 2010/11/03 11:02:25 UTC
[PATCH] Fix for issue 3620 - revert command
Stefan Sperling <st...@elego.de> writes:
>
> Noorul, if you're looking for more tasks, you could take a look at
> issue #3620 which is about a similar problem:
> http://subversion.tigris.org/issues/show_bug.cgi?id=3620
> I'll happily review and commit patches for issue #3620.
>
> Thanks,
> Stefan
Here is the patch for revert command. I will look into other commands as
and when time permits.
Log
[[[
Make 'svn revert' verify that none of its targets are URLs.
* subversion/libsvn_client/revert.c,
subversion/svn/revert-cmd.c
(svn_client_revert2, svn_cl__revert): Raise an error if any targets
look like URLs.
* subversion/tests/cmdline/input_validation_tests.py
(invalid_revert_targets, test_list): New test.
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Thanks and Regards
Noorul
Re: Fix for issue 3620 - revert command
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> On Wed, 2010-11-03 at 14:46 +0100, Stefan Sperling wrote:
>
>> On Wed, Nov 03, 2010 at 03:32:25PM +0530, Noorul Islam K M wrote:
>> >
>> > Stefan Sperling <st...@elego.de> writes:
>> > >
>> > > Noorul, if you're looking for more tasks, you could take a look at
>> > > issue #3620 which is about a similar problem:
>> > > http://subversion.tigris.org/issues/show_bug.cgi?id=3620
>> > > I'll happily review and commit patches for issue #3620.
>> > >
>> > > Thanks,
>> > > Stefan
>> >
>> > Here is the patch for revert command. I will look into other commands as
>> > and when time permits.
>> >
>> > Log
>> >
>> > [[[
>> > Make 'svn revert' verify that none of its targets are URLs.
>> >
>> > * subversion/libsvn_client/revert.c,
>> > subversion/svn/revert-cmd.c
>> > (svn_client_revert2, svn_cl__revert): Raise an error if any targets
>> > look like URLs.
>> >
>> > * subversion/tests/cmdline/input_validation_tests.py
>> > (invalid_revert_targets, test_list): New test.
>> >
>> > Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
>> > ]]]
>> >
>> > Thanks and Regards
>> > Noorul
>> >
>>
>> > Index: subversion/tests/cmdline/input_validation_tests.py
>> > ===================================================================
>> > --- subversion/tests/cmdline/input_validation_tests.py (revision 1030340)
>> > +++ subversion/tests/cmdline/input_validation_tests.py (working copy)
>> > @@ -173,6 +173,12 @@
>> > run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'upgrade',
>> > target, target)
>> >
>> > +def invalid_revert_targets(sbox):
>> > + "non-working copy paths for 'revert'"
>> > + sbox.build(read_only=True)
>> > + for target in _invalid_wc_path_targets:
>> > + run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'revert',
>> > + target)
>> >
>> > ########################################################################
>> > # Run the tests
>> > @@ -192,6 +198,7 @@
>> > invalid_log_targets,
>> > invalid_merge_args,
>> > invalid_wcpath_upgrade,
>> > + invalid_revert_targets,
>> > ]
>> >
>> > if __name__ == '__main__':
>> > Index: subversion/svn/revert-cmd.c
>> > ===================================================================
>> > --- subversion/svn/revert-cmd.c (revision 1030340)
>> > +++ subversion/svn/revert-cmd.c (working copy)
>> > @@ -27,6 +27,7 @@
>> >
>> > /*** Includes. ***/
>> >
>> > +#include "svn_path.h"
>> > #include "svn_client.h"
>> > #include "svn_error_codes.h"
>> > #include "svn_error.h"
>> > @@ -47,6 +48,7 @@
>> > svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
>> > apr_array_header_t *targets = NULL;
>> > svn_error_t *err;
>> > + int i;
>> >
>> > SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
>> > opt_state->targets,
>> > @@ -63,6 +65,19 @@
>> >
>> > SVN_ERR(svn_cl__eat_peg_revisions(&targets, targets, scratch_pool));
>> >
>> > + /* Don't even attempt to modify the working copy if any of the
>> > + * targets look like URLs. URLs are invalid input. */
>> > + for (i = 0; i < targets->nelts; i++)
>> > + {
>> > + const char *target = APR_ARRAY_IDX(targets, i, const char *);
>> > +
>> > + if (svn_path_is_url(target))
>> > + return svn_error_return(svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,
>> > + NULL,
>> > + _("'%s' is not a local path"),
>> > + target));
>> > + }
>> > +
>> > err = svn_client_revert2(targets, opt_state->depth,
>> > opt_state->changelists, ctx, scratch_pool);
>> >
>> > Index: subversion/libsvn_client/revert.c
>> > ===================================================================
>> > --- subversion/libsvn_client/revert.c (revision 1030340)
>> > +++ subversion/libsvn_client/revert.c (working copy)
>> > @@ -27,6 +27,7 @@
>> >
>> > /*** Includes. ***/
>> >
>> > +#include "svn_path.h"
>> > #include "svn_wc.h"
>> > #include "svn_client.h"
>> > #include "svn_dirent_uri.h"
>> > @@ -37,6 +38,7 @@
>> > #include "client.h"
>> > #include "private/svn_wc_private.h"
>> >
>> > +#include "svn_private_config.h"
>> >
>> >
>> > /*** Code. ***/
>> > @@ -121,6 +123,19 @@
>> > svn_boolean_t use_commit_times;
>> > struct revert_with_write_lock_baton baton;
>> >
>> > + /* Don't even attempt to modify the working copy if any of the
>> > + * targets look like URLs. URLs are invalid input. */
>> > + for (i = 0; i < paths->nelts; i++)
>> > + {
>> > + const char *path = APR_ARRAY_IDX(paths, i, const char *);
>> > +
>> > + if (svn_path_is_url(path))
>> > + return svn_error_return(svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,
>>
>> The SVN_ERR_CL_* codes are for use by the command line client, I think.
>> So I've used SVN_ERR_ILLEGAL_TARGET for errors thrown from libsvn_client.
>> But I can fix the error code locally, you don't need to send another patch.
>
> -0 on duplicating this check inside libsvn_client.
>
Did you mean -1?
Re: [PATCH] Fix for issue 3620 - revert command
Posted by Julian Foad <ju...@wandisco.com>.
On Wed, 2010-11-03 at 16:25 +0100, Stefan Sperling wrote:
> On Wed, Nov 03, 2010 at 04:22:57PM +0100, Stefan Sperling wrote:
> > On Wed, Nov 03, 2010 at 03:04:25PM +0000, Julian Foad wrote:
> > > -0 on duplicating this check inside libsvn_client.
> >
> > The rationale is that our libraries should reject invalid input
> > to prevent third party clients which pass invalid input from crashing.
> > Clients passing a canonicalised path instead of a URL are not violating
> > API rules at the syntactic level so they shouldn't just crash.
> >
> > And there's no reason to have our own client pass invalid input either.
>
> Oh, and the SVN_ERR_CL_ARG_PARSING_ERROR error code causes our
> client to print an informative line such as "Try svn help".
> So there's added value over the SVN_ERR_ILLEGAL_TARGET code that
> our libraries are throwing.
>
> I think the libraries shouldn't throw SVN_ERR_CL_* errors.
Yeah, OK. I guess I'm too minimalist.
- Julian
Re: [PATCH] Fix for issue 3620 - revert command
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Nov 03, 2010 at 04:22:57PM +0100, Stefan Sperling wrote:
> On Wed, Nov 03, 2010 at 03:04:25PM +0000, Julian Foad wrote:
> > -0 on duplicating this check inside libsvn_client.
>
> The rationale is that our libraries should reject invalid input
> to prevent third party clients which pass invalid input from crashing.
> Clients passing a canonicalised path instead of a URL are not violating
> API rules at the syntactic level so they shouldn't just crash.
>
> And there's no reason to have our own client pass invalid input either.
Oh, and the SVN_ERR_CL_ARG_PARSING_ERROR error code causes our
client to print an informative line such as "Try svn help".
So there's added value over the SVN_ERR_ILLEGAL_TARGET code that
our libraries are throwing.
I think the libraries shouldn't throw SVN_ERR_CL_* errors.
Stefar
Re: [PATCH] Fix for issue 3620 - revert command
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Nov 03, 2010 at 03:04:25PM +0000, Julian Foad wrote:
> -0 on duplicating this check inside libsvn_client.
The rationale is that our libraries should reject invalid input
to prevent third party clients which pass invalid input from crashing.
Clients passing a canonicalised path instead of a URL are not violating
API rules at the syntactic level so they shouldn't just crash.
And there's no reason to have our own client pass invalid input either.
Stefan
Re: Fix for issue 3620 - revert command
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> On Wed, 2010-11-03 at 14:46 +0100, Stefan Sperling wrote:
>
>> On Wed, Nov 03, 2010 at 03:32:25PM +0530, Noorul Islam K M wrote:
>> >
>> > Stefan Sperling <st...@elego.de> writes:
>> > >
>> > > Noorul, if you're looking for more tasks, you could take a look at
>> > > issue #3620 which is about a similar problem:
>> > > http://subversion.tigris.org/issues/show_bug.cgi?id=3620
>> > > I'll happily review and commit patches for issue #3620.
>> > >
>> > > Thanks,
>> > > Stefan
>> >
>> > Here is the patch for revert command. I will look into other commands as
>> > and when time permits.
>> >
>> > Log
>> >
>> > [[[
>> > Make 'svn revert' verify that none of its targets are URLs.
>> >
>> > * subversion/libsvn_client/revert.c,
>> > subversion/svn/revert-cmd.c
>> > (svn_client_revert2, svn_cl__revert): Raise an error if any targets
>> > look like URLs.
>> >
>> > * subversion/tests/cmdline/input_validation_tests.py
>> > (invalid_revert_targets, test_list): New test.
>> >
>> > Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
>> > ]]]
>> >
>> > Thanks and Regards
>> > Noorul
>> >
>>
>> > Index: subversion/tests/cmdline/input_validation_tests.py
>> > ===================================================================
>> > --- subversion/tests/cmdline/input_validation_tests.py (revision 1030340)
>> > +++ subversion/tests/cmdline/input_validation_tests.py (working copy)
>> > @@ -173,6 +173,12 @@
>> > run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'upgrade',
>> > target, target)
>> >
>> > +def invalid_revert_targets(sbox):
>> > + "non-working copy paths for 'revert'"
>> > + sbox.build(read_only=True)
>> > + for target in _invalid_wc_path_targets:
>> > + run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'revert',
>> > + target)
>> >
>> > ########################################################################
>> > # Run the tests
>> > @@ -192,6 +198,7 @@
>> > invalid_log_targets,
>> > invalid_merge_args,
>> > invalid_wcpath_upgrade,
>> > + invalid_revert_targets,
>> > ]
>> >
>> > if __name__ == '__main__':
>> > Index: subversion/svn/revert-cmd.c
>> > ===================================================================
>> > --- subversion/svn/revert-cmd.c (revision 1030340)
>> > +++ subversion/svn/revert-cmd.c (working copy)
>> > @@ -27,6 +27,7 @@
>> >
>> > /*** Includes. ***/
>> >
>> > +#include "svn_path.h"
>> > #include "svn_client.h"
>> > #include "svn_error_codes.h"
>> > #include "svn_error.h"
>> > @@ -47,6 +48,7 @@
>> > svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
>> > apr_array_header_t *targets = NULL;
>> > svn_error_t *err;
>> > + int i;
>> >
>> > SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
>> > opt_state->targets,
>> > @@ -63,6 +65,19 @@
>> >
>> > SVN_ERR(svn_cl__eat_peg_revisions(&targets, targets, scratch_pool));
>> >
>> > + /* Don't even attempt to modify the working copy if any of the
>> > + * targets look like URLs. URLs are invalid input. */
>> > + for (i = 0; i < targets->nelts; i++)
>> > + {
>> > + const char *target = APR_ARRAY_IDX(targets, i, const char *);
>> > +
>> > + if (svn_path_is_url(target))
>> > + return svn_error_return(svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,
>> > + NULL,
>> > + _("'%s' is not a local path"),
>> > + target));
>> > + }
>> > +
>> > err = svn_client_revert2(targets, opt_state->depth,
>> > opt_state->changelists, ctx, scratch_pool);
>> >
>> > Index: subversion/libsvn_client/revert.c
>> > ===================================================================
>> > --- subversion/libsvn_client/revert.c (revision 1030340)
>> > +++ subversion/libsvn_client/revert.c (working copy)
>> > @@ -27,6 +27,7 @@
>> >
>> > /*** Includes. ***/
>> >
>> > +#include "svn_path.h"
>> > #include "svn_wc.h"
>> > #include "svn_client.h"
>> > #include "svn_dirent_uri.h"
>> > @@ -37,6 +38,7 @@
>> > #include "client.h"
>> > #include "private/svn_wc_private.h"
>> >
>> > +#include "svn_private_config.h"
>> >
>> >
>> > /*** Code. ***/
>> > @@ -121,6 +123,19 @@
>> > svn_boolean_t use_commit_times;
>> > struct revert_with_write_lock_baton baton;
>> >
>> > + /* Don't even attempt to modify the working copy if any of the
>> > + * targets look like URLs. URLs are invalid input. */
>> > + for (i = 0; i < paths->nelts; i++)
>> > + {
>> > + const char *path = APR_ARRAY_IDX(paths, i, const char *);
>> > +
>> > + if (svn_path_is_url(path))
>> > + return svn_error_return(svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,
>>
>> The SVN_ERR_CL_* codes are for use by the command line client, I think.
>> So I've used SVN_ERR_ILLEGAL_TARGET for errors thrown from libsvn_client.
>> But I can fix the error code locally, you don't need to send another patch.
>
> -0 on duplicating this check inside libsvn_client.
>
Did you mean -1?
>From svn log -c 965551
Input validation is done both right beneath the client API layer and
within the CLI client. This makes sure that our CLI client behaves well,
i.e. it won't ask the client library to perform operations it knows might
fail due to invalid input. The checks within the client library help third-
party clients which don't perform proper input validation even though they
should.
Thanks and Regards
Noorul
Re: [PATCH] Fix for issue 3620 - revert command
Posted by Julian Foad <ju...@wandisco.com>.
On Wed, 2010-11-03 at 14:46 +0100, Stefan Sperling wrote:
> On Wed, Nov 03, 2010 at 03:32:25PM +0530, Noorul Islam K M wrote:
> >
> > Stefan Sperling <st...@elego.de> writes:
> > >
> > > Noorul, if you're looking for more tasks, you could take a look at
> > > issue #3620 which is about a similar problem:
> > > http://subversion.tigris.org/issues/show_bug.cgi?id=3620
> > > I'll happily review and commit patches for issue #3620.
> > >
> > > Thanks,
> > > Stefan
> >
> > Here is the patch for revert command. I will look into other commands as
> > and when time permits.
> >
> > Log
> >
> > [[[
> > Make 'svn revert' verify that none of its targets are URLs.
> >
> > * subversion/libsvn_client/revert.c,
> > subversion/svn/revert-cmd.c
> > (svn_client_revert2, svn_cl__revert): Raise an error if any targets
> > look like URLs.
> >
> > * subversion/tests/cmdline/input_validation_tests.py
> > (invalid_revert_targets, test_list): New test.
> >
> > Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
> > ]]]
> >
> > Thanks and Regards
> > Noorul
> >
>
> > Index: subversion/tests/cmdline/input_validation_tests.py
> > ===================================================================
> > --- subversion/tests/cmdline/input_validation_tests.py (revision 1030340)
> > +++ subversion/tests/cmdline/input_validation_tests.py (working copy)
> > @@ -173,6 +173,12 @@
> > run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'upgrade',
> > target, target)
> >
> > +def invalid_revert_targets(sbox):
> > + "non-working copy paths for 'revert'"
> > + sbox.build(read_only=True)
> > + for target in _invalid_wc_path_targets:
> > + run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'revert',
> > + target)
> >
> > ########################################################################
> > # Run the tests
> > @@ -192,6 +198,7 @@
> > invalid_log_targets,
> > invalid_merge_args,
> > invalid_wcpath_upgrade,
> > + invalid_revert_targets,
> > ]
> >
> > if __name__ == '__main__':
> > Index: subversion/svn/revert-cmd.c
> > ===================================================================
> > --- subversion/svn/revert-cmd.c (revision 1030340)
> > +++ subversion/svn/revert-cmd.c (working copy)
> > @@ -27,6 +27,7 @@
> >
> > /*** Includes. ***/
> >
> > +#include "svn_path.h"
> > #include "svn_client.h"
> > #include "svn_error_codes.h"
> > #include "svn_error.h"
> > @@ -47,6 +48,7 @@
> > svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
> > apr_array_header_t *targets = NULL;
> > svn_error_t *err;
> > + int i;
> >
> > SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
> > opt_state->targets,
> > @@ -63,6 +65,19 @@
> >
> > SVN_ERR(svn_cl__eat_peg_revisions(&targets, targets, scratch_pool));
> >
> > + /* Don't even attempt to modify the working copy if any of the
> > + * targets look like URLs. URLs are invalid input. */
> > + for (i = 0; i < targets->nelts; i++)
> > + {
> > + const char *target = APR_ARRAY_IDX(targets, i, const char *);
> > +
> > + if (svn_path_is_url(target))
> > + return svn_error_return(svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,
> > + NULL,
> > + _("'%s' is not a local path"),
> > + target));
> > + }
> > +
> > err = svn_client_revert2(targets, opt_state->depth,
> > opt_state->changelists, ctx, scratch_pool);
> >
> > Index: subversion/libsvn_client/revert.c
> > ===================================================================
> > --- subversion/libsvn_client/revert.c (revision 1030340)
> > +++ subversion/libsvn_client/revert.c (working copy)
> > @@ -27,6 +27,7 @@
> >
> > /*** Includes. ***/
> >
> > +#include "svn_path.h"
> > #include "svn_wc.h"
> > #include "svn_client.h"
> > #include "svn_dirent_uri.h"
> > @@ -37,6 +38,7 @@
> > #include "client.h"
> > #include "private/svn_wc_private.h"
> >
> > +#include "svn_private_config.h"
> >
> >
> > /*** Code. ***/
> > @@ -121,6 +123,19 @@
> > svn_boolean_t use_commit_times;
> > struct revert_with_write_lock_baton baton;
> >
> > + /* Don't even attempt to modify the working copy if any of the
> > + * targets look like URLs. URLs are invalid input. */
> > + for (i = 0; i < paths->nelts; i++)
> > + {
> > + const char *path = APR_ARRAY_IDX(paths, i, const char *);
> > +
> > + if (svn_path_is_url(path))
> > + return svn_error_return(svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,
>
> The SVN_ERR_CL_* codes are for use by the command line client, I think.
> So I've used SVN_ERR_ILLEGAL_TARGET for errors thrown from libsvn_client.
> But I can fix the error code locally, you don't need to send another patch.
-0 on duplicating this check inside libsvn_client.
- Julian
> Looks great otherwise. I will test and quite likely commit this later today.
>
> Thanks,
> Stefan
>
> > + NULL,
> > + _("'%s' is not a local path"),
> > + path));
> > + }
> > +
> > cfg = ctx->config ? apr_hash_get(ctx->config, SVN_CONFIG_CATEGORY_CONFIG,
> > APR_HASH_KEY_STRING) : NULL;
> >
>
Re: [PATCH] Fix for issue 3620 - revert command
Posted by Julian Foad <ju...@wandisco.com>.
On Wed, 2010-11-03 at 14:46 +0100, Stefan Sperling wrote:
> On Wed, Nov 03, 2010 at 03:32:25PM +0530, Noorul Islam K M wrote:
> >
> > Stefan Sperling <st...@elego.de> writes:
> > >
> > > Noorul, if you're looking for more tasks, you could take a look at
> > > issue #3620 which is about a similar problem:
> > > http://subversion.tigris.org/issues/show_bug.cgi?id=3620
> > > I'll happily review and commit patches for issue #3620.
> > >
> > > Thanks,
> > > Stefan
> >
> > Here is the patch for revert command. I will look into other commands as
> > and when time permits.
> >
> > Log
> >
> > [[[
> > Make 'svn revert' verify that none of its targets are URLs.
> >
> > * subversion/libsvn_client/revert.c,
> > subversion/svn/revert-cmd.c
> > (svn_client_revert2, svn_cl__revert): Raise an error if any targets
> > look like URLs.
> >
> > * subversion/tests/cmdline/input_validation_tests.py
> > (invalid_revert_targets, test_list): New test.
> >
> > Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
> > ]]]
> >
> > Thanks and Regards
> > Noorul
> >
>
> > Index: subversion/tests/cmdline/input_validation_tests.py
> > ===================================================================
> > --- subversion/tests/cmdline/input_validation_tests.py (revision 1030340)
> > +++ subversion/tests/cmdline/input_validation_tests.py (working copy)
> > @@ -173,6 +173,12 @@
> > run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'upgrade',
> > target, target)
> >
> > +def invalid_revert_targets(sbox):
> > + "non-working copy paths for 'revert'"
> > + sbox.build(read_only=True)
> > + for target in _invalid_wc_path_targets:
> > + run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'revert',
> > + target)
> >
> > ########################################################################
> > # Run the tests
> > @@ -192,6 +198,7 @@
> > invalid_log_targets,
> > invalid_merge_args,
> > invalid_wcpath_upgrade,
> > + invalid_revert_targets,
> > ]
> >
> > if __name__ == '__main__':
> > Index: subversion/svn/revert-cmd.c
> > ===================================================================
> > --- subversion/svn/revert-cmd.c (revision 1030340)
> > +++ subversion/svn/revert-cmd.c (working copy)
> > @@ -27,6 +27,7 @@
> >
> > /*** Includes. ***/
> >
> > +#include "svn_path.h"
> > #include "svn_client.h"
> > #include "svn_error_codes.h"
> > #include "svn_error.h"
> > @@ -47,6 +48,7 @@
> > svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
> > apr_array_header_t *targets = NULL;
> > svn_error_t *err;
> > + int i;
> >
> > SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
> > opt_state->targets,
> > @@ -63,6 +65,19 @@
> >
> > SVN_ERR(svn_cl__eat_peg_revisions(&targets, targets, scratch_pool));
> >
> > + /* Don't even attempt to modify the working copy if any of the
> > + * targets look like URLs. URLs are invalid input. */
> > + for (i = 0; i < targets->nelts; i++)
> > + {
> > + const char *target = APR_ARRAY_IDX(targets, i, const char *);
> > +
> > + if (svn_path_is_url(target))
> > + return svn_error_return(svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,
> > + NULL,
> > + _("'%s' is not a local path"),
> > + target));
> > + }
> > +
> > err = svn_client_revert2(targets, opt_state->depth,
> > opt_state->changelists, ctx, scratch_pool);
> >
> > Index: subversion/libsvn_client/revert.c
> > ===================================================================
> > --- subversion/libsvn_client/revert.c (revision 1030340)
> > +++ subversion/libsvn_client/revert.c (working copy)
> > @@ -27,6 +27,7 @@
> >
> > /*** Includes. ***/
> >
> > +#include "svn_path.h"
> > #include "svn_wc.h"
> > #include "svn_client.h"
> > #include "svn_dirent_uri.h"
> > @@ -37,6 +38,7 @@
> > #include "client.h"
> > #include "private/svn_wc_private.h"
> >
> > +#include "svn_private_config.h"
> >
> >
> > /*** Code. ***/
> > @@ -121,6 +123,19 @@
> > svn_boolean_t use_commit_times;
> > struct revert_with_write_lock_baton baton;
> >
> > + /* Don't even attempt to modify the working copy if any of the
> > + * targets look like URLs. URLs are invalid input. */
> > + for (i = 0; i < paths->nelts; i++)
> > + {
> > + const char *path = APR_ARRAY_IDX(paths, i, const char *);
> > +
> > + if (svn_path_is_url(path))
> > + return svn_error_return(svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,
>
> The SVN_ERR_CL_* codes are for use by the command line client, I think.
> So I've used SVN_ERR_ILLEGAL_TARGET for errors thrown from libsvn_client.
> But I can fix the error code locally, you don't need to send another patch.
-0 on duplicating this check inside libsvn_client.
- Julian
> Looks great otherwise. I will test and quite likely commit this later today.
>
> Thanks,
> Stefan
>
> > + NULL,
> > + _("'%s' is not a local path"),
> > + path));
> > + }
> > +
> > cfg = ctx->config ? apr_hash_get(ctx->config, SVN_CONFIG_CATEGORY_CONFIG,
> > APR_HASH_KEY_STRING) : NULL;
> >
>
Re: [PATCH] Fix for issue 3620 - revert command
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Nov 03, 2010 at 03:32:25PM +0530, Noorul Islam K M wrote:
>
> Stefan Sperling <st...@elego.de> writes:
> >
> > Noorul, if you're looking for more tasks, you could take a look at
> > issue #3620 which is about a similar problem:
> > http://subversion.tigris.org/issues/show_bug.cgi?id=3620
> > I'll happily review and commit patches for issue #3620.
> >
> > Thanks,
> > Stefan
>
> Here is the patch for revert command. I will look into other commands as
> and when time permits.
>
> Log
>
> [[[
> Make 'svn revert' verify that none of its targets are URLs.
>
> * subversion/libsvn_client/revert.c,
> subversion/svn/revert-cmd.c
> (svn_client_revert2, svn_cl__revert): Raise an error if any targets
> look like URLs.
>
> * subversion/tests/cmdline/input_validation_tests.py
> (invalid_revert_targets, test_list): New test.
>
> Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
> ]]]
>
> Thanks and Regards
> Noorul
>
> Index: subversion/tests/cmdline/input_validation_tests.py
> ===================================================================
> --- subversion/tests/cmdline/input_validation_tests.py (revision 1030340)
> +++ subversion/tests/cmdline/input_validation_tests.py (working copy)
> @@ -173,6 +173,12 @@
> run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'upgrade',
> target, target)
>
> +def invalid_revert_targets(sbox):
> + "non-working copy paths for 'revert'"
> + sbox.build(read_only=True)
> + for target in _invalid_wc_path_targets:
> + run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'revert',
> + target)
>
> ########################################################################
> # Run the tests
> @@ -192,6 +198,7 @@
> invalid_log_targets,
> invalid_merge_args,
> invalid_wcpath_upgrade,
> + invalid_revert_targets,
> ]
>
> if __name__ == '__main__':
> Index: subversion/svn/revert-cmd.c
> ===================================================================
> --- subversion/svn/revert-cmd.c (revision 1030340)
> +++ subversion/svn/revert-cmd.c (working copy)
> @@ -27,6 +27,7 @@
>
> /*** Includes. ***/
>
> +#include "svn_path.h"
> #include "svn_client.h"
> #include "svn_error_codes.h"
> #include "svn_error.h"
> @@ -47,6 +48,7 @@
> svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
> apr_array_header_t *targets = NULL;
> svn_error_t *err;
> + int i;
>
> SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
> opt_state->targets,
> @@ -63,6 +65,19 @@
>
> SVN_ERR(svn_cl__eat_peg_revisions(&targets, targets, scratch_pool));
>
> + /* Don't even attempt to modify the working copy if any of the
> + * targets look like URLs. URLs are invalid input. */
> + for (i = 0; i < targets->nelts; i++)
> + {
> + const char *target = APR_ARRAY_IDX(targets, i, const char *);
> +
> + if (svn_path_is_url(target))
> + return svn_error_return(svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,
> + NULL,
> + _("'%s' is not a local path"),
> + target));
> + }
> +
> err = svn_client_revert2(targets, opt_state->depth,
> opt_state->changelists, ctx, scratch_pool);
>
> Index: subversion/libsvn_client/revert.c
> ===================================================================
> --- subversion/libsvn_client/revert.c (revision 1030340)
> +++ subversion/libsvn_client/revert.c (working copy)
> @@ -27,6 +27,7 @@
>
> /*** Includes. ***/
>
> +#include "svn_path.h"
> #include "svn_wc.h"
> #include "svn_client.h"
> #include "svn_dirent_uri.h"
> @@ -37,6 +38,7 @@
> #include "client.h"
> #include "private/svn_wc_private.h"
>
> +#include "svn_private_config.h"
>
>
> /*** Code. ***/
> @@ -121,6 +123,19 @@
> svn_boolean_t use_commit_times;
> struct revert_with_write_lock_baton baton;
>
> + /* Don't even attempt to modify the working copy if any of the
> + * targets look like URLs. URLs are invalid input. */
> + for (i = 0; i < paths->nelts; i++)
> + {
> + const char *path = APR_ARRAY_IDX(paths, i, const char *);
> +
> + if (svn_path_is_url(path))
> + return svn_error_return(svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,
The SVN_ERR_CL_* codes are for use by the command line client, I think.
So I've used SVN_ERR_ILLEGAL_TARGET for errors thrown from libsvn_client.
But I can fix the error code locally, you don't need to send another patch.
Looks great otherwise. I will test and quite likely commit this later today.
Thanks,
Stefan
> + NULL,
> + _("'%s' is not a local path"),
> + path));
> + }
> +
> cfg = ctx->config ? apr_hash_get(ctx->config, SVN_CONFIG_CATEGORY_CONFIG,
> APR_HASH_KEY_STRING) : NULL;
>
Re: [PATCH] Fix for issue 3620 - revert command
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Nov 03, 2010 at 03:32:25PM +0530, Noorul Islam K M wrote:
> Here is the patch for revert command. I will look into other commands as
> and when time permits.
>
> Log
>
> [[[
> Make 'svn revert' verify that none of its targets are URLs.
>
> * subversion/libsvn_client/revert.c,
> subversion/svn/revert-cmd.c
> (svn_client_revert2, svn_cl__revert): Raise an error if any targets
> look like URLs.
>
> * subversion/tests/cmdline/input_validation_tests.py
> (invalid_revert_targets, test_list): New test.
>
> Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
> ]]]
r1030598, thank you!
Re: [PATCH] Fix for issue 3620 - revert command
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Nov 03, 2010 at 03:32:25PM +0530, Noorul Islam K M wrote:
>
> Stefan Sperling <st...@elego.de> writes:
> >
> > Noorul, if you're looking for more tasks, you could take a look at
> > issue #3620 which is about a similar problem:
> > http://subversion.tigris.org/issues/show_bug.cgi?id=3620
> > I'll happily review and commit patches for issue #3620.
> >
> > Thanks,
> > Stefan
>
> Here is the patch for revert command. I will look into other commands as
> and when time permits.
>
> Log
>
> [[[
> Make 'svn revert' verify that none of its targets are URLs.
>
> * subversion/libsvn_client/revert.c,
> subversion/svn/revert-cmd.c
> (svn_client_revert2, svn_cl__revert): Raise an error if any targets
> look like URLs.
>
> * subversion/tests/cmdline/input_validation_tests.py
> (invalid_revert_targets, test_list): New test.
>
> Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
> ]]]
>
> Thanks and Regards
> Noorul
>
> Index: subversion/tests/cmdline/input_validation_tests.py
> ===================================================================
> --- subversion/tests/cmdline/input_validation_tests.py (revision 1030340)
> +++ subversion/tests/cmdline/input_validation_tests.py (working copy)
> @@ -173,6 +173,12 @@
> run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'upgrade',
> target, target)
>
> +def invalid_revert_targets(sbox):
> + "non-working copy paths for 'revert'"
> + sbox.build(read_only=True)
> + for target in _invalid_wc_path_targets:
> + run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'revert',
> + target)
>
> ########################################################################
> # Run the tests
> @@ -192,6 +198,7 @@
> invalid_log_targets,
> invalid_merge_args,
> invalid_wcpath_upgrade,
> + invalid_revert_targets,
> ]
>
> if __name__ == '__main__':
> Index: subversion/svn/revert-cmd.c
> ===================================================================
> --- subversion/svn/revert-cmd.c (revision 1030340)
> +++ subversion/svn/revert-cmd.c (working copy)
> @@ -27,6 +27,7 @@
>
> /*** Includes. ***/
>
> +#include "svn_path.h"
> #include "svn_client.h"
> #include "svn_error_codes.h"
> #include "svn_error.h"
> @@ -47,6 +48,7 @@
> svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
> apr_array_header_t *targets = NULL;
> svn_error_t *err;
> + int i;
>
> SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
> opt_state->targets,
> @@ -63,6 +65,19 @@
>
> SVN_ERR(svn_cl__eat_peg_revisions(&targets, targets, scratch_pool));
>
> + /* Don't even attempt to modify the working copy if any of the
> + * targets look like URLs. URLs are invalid input. */
> + for (i = 0; i < targets->nelts; i++)
> + {
> + const char *target = APR_ARRAY_IDX(targets, i, const char *);
> +
> + if (svn_path_is_url(target))
> + return svn_error_return(svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,
> + NULL,
> + _("'%s' is not a local path"),
> + target));
> + }
> +
> err = svn_client_revert2(targets, opt_state->depth,
> opt_state->changelists, ctx, scratch_pool);
>
> Index: subversion/libsvn_client/revert.c
> ===================================================================
> --- subversion/libsvn_client/revert.c (revision 1030340)
> +++ subversion/libsvn_client/revert.c (working copy)
> @@ -27,6 +27,7 @@
>
> /*** Includes. ***/
>
> +#include "svn_path.h"
> #include "svn_wc.h"
> #include "svn_client.h"
> #include "svn_dirent_uri.h"
> @@ -37,6 +38,7 @@
> #include "client.h"
> #include "private/svn_wc_private.h"
>
> +#include "svn_private_config.h"
>
>
> /*** Code. ***/
> @@ -121,6 +123,19 @@
> svn_boolean_t use_commit_times;
> struct revert_with_write_lock_baton baton;
>
> + /* Don't even attempt to modify the working copy if any of the
> + * targets look like URLs. URLs are invalid input. */
> + for (i = 0; i < paths->nelts; i++)
> + {
> + const char *path = APR_ARRAY_IDX(paths, i, const char *);
> +
> + if (svn_path_is_url(path))
> + return svn_error_return(svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,
The SVN_ERR_CL_* codes are for use by the command line client, I think.
So I've used SVN_ERR_ILLEGAL_TARGET for errors thrown from libsvn_client.
But I can fix the error code locally, you don't need to send another patch.
Looks great otherwise. I will test and quite likely commit this later today.
Thanks,
Stefan
> + NULL,
> + _("'%s' is not a local path"),
> + path));
> + }
> +
> cfg = ctx->config ? apr_hash_get(ctx->config, SVN_CONFIG_CATEGORY_CONFIG,
> APR_HASH_KEY_STRING) : NULL;
>