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/02/10 10:42:03 UTC
[PATCH] svn command - ls - Multiple targets
This patch is a followup of the following thread. All tests pass with
this patch.
http://svn.haxx.se/dev/archive-2011-01/0210.shtml
Log
[[[
Make 'svn ls' continue processing targets after printing warning if one
or more of the targets is a non-existent URL or wc-entry. Also return a
non-zero error code and print an error message at the end in those
situations.
* subversion/svn/list-cmd.c
(svn_cl__list): If one of the targets is a non-existent URL or
wc-entry, don't bail out. Just warn and move on to the next
target. Also return a non-zero error code and print an error message
at the end in those situations.
* subversion/tests/cmdline/basic_tests.py
(ls_non_existent_wc_target, ls_non_existent_url_target,
ls_multiple_wc_targets, ls_multiple_url_targets): New tests.
(test_list): Add reference to new tests.
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Re: [PATCH] svn command - ls - Multiple targets
Posted by Noorul Islam K M <no...@collab.net>.
Stefan Sperling <st...@elego.de> writes:
> On Mon, Feb 14, 2011 at 07:55:17PM +0530, Noorul Islam K M wrote:
>
>> Incorporated all review comments. Please find attached latest patch.
>
> Thanks! Committed in r1070510 (with similar log message tweak as
> I did for the 'add' diff).
>
> Also, I did not commit the change for svn info, because I wasn't sure
> whether you included it accidentally:
>
>> Index: subversion/svn/info-cmd.c
>> ===================================================================
>> --- subversion/svn/info-cmd.c (revision 1070486)
>> +++ subversion/svn/info-cmd.c (working copy)
>> @@ -588,7 +588,9 @@
>> SVN_ERR(svn_cl__xml_print_footer("info", pool));
>>
>> if (saw_a_problem)
>> - return svn_error_create(SVN_ERR_BASE, NULL, NULL);
>> + return svn_error_create(
>> + SVN_ERR_ILLEGAL_TARGET, NULL,
>> + _("Could not list all targets because some targets don't exist"));
>> else
>> return SVN_NO_ERROR;
>> }
>
> Can you resend this change as a separate patch, also renaming the
> saw_a_problem variable for consistency with 'ls' and 'add'?
My bad. Sorry about the noise. Sure, I will send a separate patch for
info and cat. Thank you!
Thanks and Regards
Noorul
Re: [PATCH] svn command - ls - Multiple targets
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Feb 14, 2011 at 07:55:17PM +0530, Noorul Islam K M wrote:
> Incorporated all review comments. Please find attached latest patch.
Thanks! Committed in r1070510 (with similar log message tweak as
I did for the 'add' diff).
Also, I did not commit the change for svn info, because I wasn't sure
whether you included it accidentally:
> Index: subversion/svn/info-cmd.c
> ===================================================================
> --- subversion/svn/info-cmd.c (revision 1070486)
> +++ subversion/svn/info-cmd.c (working copy)
> @@ -588,7 +588,9 @@
> SVN_ERR(svn_cl__xml_print_footer("info", pool));
>
> if (saw_a_problem)
> - return svn_error_create(SVN_ERR_BASE, NULL, NULL);
> + return svn_error_create(
> + SVN_ERR_ILLEGAL_TARGET, NULL,
> + _("Could not list all targets because some targets don't exist"));
> else
> return SVN_NO_ERROR;
> }
Can you resend this change as a separate patch, also renaming the
saw_a_problem variable for consistency with 'ls' and 'add'?
Re: [PATCH] svn command - ls - Multiple targets
Posted by Noorul Islam K M <no...@collab.net>.
Stefan Sperling <st...@elego.de> writes:
> On Mon, Feb 14, 2011 at 07:32:51PM +0530, Noorul Islam K M wrote:
>
>> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>>
>> > Stefan Sperling wrote on Mon, Feb 14, 2011 at 13:25:13 +0100:
>> >
>> >> On Mon, Feb 14, 2011 at 01:18:58PM +0100, Stefan Sperling wrote:
>> >> > > @@ -301,5 +318,8 @@
>> >> > > if (opt_state->xml && ! opt_state->incremental)
>> >> > > SVN_ERR(svn_cl__xml_print_footer("lists", pool));
>> >> > >
>> >> > > - return SVN_NO_ERROR;
>> >> > > + if (saw_a_problem)
>> >> > > + return svn_error_create(SVN_ERR_BASE, NULL, NULL);
>> >> > > + else
>> >> > > + return SVN_NO_ERROR;
>> >> > > }
>> >>
>> >> Oh, and I'm not sure if SVN_ERR_BASE and no message is the right
>> >> thing to do here. Maybe this should be an error code such as
>> >> SVN_ERR_ILLEGAL_TARGET and a message like _("Could not list all
>> >> targets because some targets don't exist")?
>> >
>> > Is it easily possible to give the name of the non-existent target here?
>> >
>> > (i.e., "it's good for error messages to contain an %s")
>>
>> In a particular scenario there will be more than one targets which might
>> fail. In those cases a warning is printed. For the user to know that
>> something went wrong we are printing "svn: E200000: A problem occurred;
>> see other errors for details".
>
> I would prefer this message to be more explicit. We aren't printing
> other errors as far as I can tell. We are printing warnings, not errors.
> So the message saying "see other errors for details" might be confusing.
Incorporated all review comments. Please find attached latest patch.
Thanks and Regards
Noorul
Re: [PATCH] svn command - ls - Multiple targets
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Feb 14, 2011 at 07:32:51PM +0530, Noorul Islam K M wrote:
> Daniel Shahaf <d....@daniel.shahaf.name> writes:
>
> > Stefan Sperling wrote on Mon, Feb 14, 2011 at 13:25:13 +0100:
> >
> >> On Mon, Feb 14, 2011 at 01:18:58PM +0100, Stefan Sperling wrote:
> >> > > @@ -301,5 +318,8 @@
> >> > > if (opt_state->xml && ! opt_state->incremental)
> >> > > SVN_ERR(svn_cl__xml_print_footer("lists", pool));
> >> > >
> >> > > - return SVN_NO_ERROR;
> >> > > + if (saw_a_problem)
> >> > > + return svn_error_create(SVN_ERR_BASE, NULL, NULL);
> >> > > + else
> >> > > + return SVN_NO_ERROR;
> >> > > }
> >>
> >> Oh, and I'm not sure if SVN_ERR_BASE and no message is the right
> >> thing to do here. Maybe this should be an error code such as
> >> SVN_ERR_ILLEGAL_TARGET and a message like _("Could not list all
> >> targets because some targets don't exist")?
> >
> > Is it easily possible to give the name of the non-existent target here?
> >
> > (i.e., "it's good for error messages to contain an %s")
>
> In a particular scenario there will be more than one targets which might
> fail. In those cases a warning is printed. For the user to know that
> something went wrong we are printing "svn: E200000: A problem occurred;
> see other errors for details".
I would prefer this message to be more explicit. We aren't printing
other errors as far as I can tell. We are printing warnings, not errors.
So the message saying "see other errors for details" might be confusing.
Re: [PATCH] svn command - ls - Multiple targets
Posted by Noorul Islam K M <no...@collab.net>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> Stefan Sperling wrote on Mon, Feb 14, 2011 at 13:25:13 +0100:
>
>> On Mon, Feb 14, 2011 at 01:18:58PM +0100, Stefan Sperling wrote:
>> > > @@ -301,5 +318,8 @@
>> > > if (opt_state->xml && ! opt_state->incremental)
>> > > SVN_ERR(svn_cl__xml_print_footer("lists", pool));
>> > >
>> > > - return SVN_NO_ERROR;
>> > > + if (saw_a_problem)
>> > > + return svn_error_create(SVN_ERR_BASE, NULL, NULL);
>> > > + else
>> > > + return SVN_NO_ERROR;
>> > > }
>>
>> Oh, and I'm not sure if SVN_ERR_BASE and no message is the right
>> thing to do here. Maybe this should be an error code such as
>> SVN_ERR_ILLEGAL_TARGET and a message like _("Could not list all
>> targets because some targets don't exist")?
>
> Is it easily possible to give the name of the non-existent target here?
>
> (i.e., "it's good for error messages to contain an %s")
In a particular scenario there will be more than one targets which might
fail. In those cases a warning is printed. For the user to know that
something went wrong we are printing "svn: E200000: A problem occurred;
see other errors for details".
Thanks and Regards
Noorul
Re: [PATCH] svn command - ls - Multiple targets
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Mon, Feb 14, 2011 at 13:25:13 +0100:
> On Mon, Feb 14, 2011 at 01:18:58PM +0100, Stefan Sperling wrote:
> > > @@ -301,5 +318,8 @@
> > > if (opt_state->xml && ! opt_state->incremental)
> > > SVN_ERR(svn_cl__xml_print_footer("lists", pool));
> > >
> > > - return SVN_NO_ERROR;
> > > + if (saw_a_problem)
> > > + return svn_error_create(SVN_ERR_BASE, NULL, NULL);
> > > + else
> > > + return SVN_NO_ERROR;
> > > }
>
> Oh, and I'm not sure if SVN_ERR_BASE and no message is the right
> thing to do here. Maybe this should be an error code such as
> SVN_ERR_ILLEGAL_TARGET and a message like _("Could not list all
> targets because some targets don't exist")?
Is it easily possible to give the name of the non-existent target here?
(i.e., "it's good for error messages to contain an %s")
Re: [PATCH] svn command - ls - Multiple targets
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Feb 14, 2011 at 01:18:58PM +0100, Stefan Sperling wrote:
> > @@ -301,5 +318,8 @@
> > if (opt_state->xml && ! opt_state->incremental)
> > SVN_ERR(svn_cl__xml_print_footer("lists", pool));
> >
> > - return SVN_NO_ERROR;
> > + if (saw_a_problem)
> > + return svn_error_create(SVN_ERR_BASE, NULL, NULL);
> > + else
> > + return SVN_NO_ERROR;
> > }
Oh, and I'm not sure if SVN_ERR_BASE and no message is the right
thing to do here. Maybe this should be an error code such as
SVN_ERR_ILLEGAL_TARGET and a message like _("Could not list all
targets because some targets don't exist")?
Re: [PATCH] svn command - ls - Multiple targets
Posted by Noorul Islam K M <no...@collab.net>.
Philip Martin <ph...@wandisco.com> writes:
> Stefan Sperling <st...@elego.de> writes:
>
>>> + alpha = os.path.join(wc_dir, 'A/B/E/alpha')
>>> + beta = os.path.join(wc_dir, 'A/B/E/beta')
>>
>> I think you need this here instead:
>> alpha = os.path.join(wc_dir, 'A', 'B', 'E', alpha')
>> beta = os.path.join(wc_dir, 'A', 'B', 'E', 'beta')
>>
>> Otherwise you'll get something like this on Windows:
>> C:\foo\bar\wc_dir\A/B/E/alpha
>> C:\foo\bar\wc_dir\A/B/E/beta
>
> Or:
>
> sbox.ospath('A/B/E/alpha')
>
Incorporated.
>>> + exit_code, output, error = svntest.main.run_svn(1, 'ls', alpha,
>>> + non_existent_path, beta)
>>> +
>>> + # Verify error
>>> + if not expected_err_re.match("".join(error)):
>>> + raise svntest.Failure('Cat failed: expected error "%s", but received '
>>> + '"%s"' % (expected_err, "".join(error)))
>
> It should be possible to pass the regex into run_and_verify_svn.
I tried but different combinations. At last I ended up with this.
Thanks and Regards
Noorul
Re: [PATCH] svn command - ls - Multiple targets
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Sperling <st...@elego.de> writes:
>> + alpha = os.path.join(wc_dir, 'A/B/E/alpha')
>> + beta = os.path.join(wc_dir, 'A/B/E/beta')
>
> I think you need this here instead:
> alpha = os.path.join(wc_dir, 'A', 'B', 'E', alpha')
> beta = os.path.join(wc_dir, 'A', 'B', 'E', 'beta')
>
> Otherwise you'll get something like this on Windows:
> C:\foo\bar\wc_dir\A/B/E/alpha
> C:\foo\bar\wc_dir\A/B/E/beta
Or:
sbox.ospath('A/B/E/alpha')
>> + exit_code, output, error = svntest.main.run_svn(1, 'ls', alpha,
>> + non_existent_path, beta)
>> +
>> + # Verify error
>> + if not expected_err_re.match("".join(error)):
>> + raise svntest.Failure('Cat failed: expected error "%s", but received '
>> + '"%s"' % (expected_err, "".join(error)))
It should be possible to pass the regex into run_and_verify_svn.
--
Philip
Re: [PATCH] svn command - ls - Multiple targets
Posted by Stefan Sperling <st...@elego.de>.
Hi Noorul,
comments inline:
On Fri, Feb 11, 2011 at 12:49:04PM +0530, Noorul Islam K M wrote:
> ===================================================================
> --- subversion/tests/cmdline/basic_tests.py (revision 1068757)
> +++ subversion/tests/cmdline/basic_tests.py (working copy)
> @@ -2690,6 +2690,85 @@
> [], 'ls',
> url)
>
> +def ls_non_existent_wc_target(sbox):
> + "ls a non-existent wc target"
> +
> + sbox.build(read_only = True)
> + wc_dir = sbox.wc_dir
> +
> + non_existent_path = os.path.join(wc_dir, 'non-existent')
> +
> + expected_err = "svn: warning: W155010: The node '" + \
> + re.escape(os.path.abspath(non_existent_path)) + "' was not found"
> +
> + svntest.actions.run_and_verify_svn2(None, None, expected_err,
> + 1, 'ls', non_existent_path)
> +
> +def ls_non_existent_url_target(sbox):
> + "ls a non-existent url target"
> +
> + sbox.build(read_only = True, create_wc = False)
> +
> + non_existent_url = sbox.repo_url + '/non-existent'
> + expected_err = "svn: warning: W160013: .*"
> +
> + svntest.actions.run_and_verify_svn2(None, None, expected_err,
> + 1, 'ls', non_existent_url)
> +
> +def ls_multiple_wc_targets(sbox):
> + "ls multiple wc targets"
> +
> + sbox.build(read_only = True)
> + wc_dir = sbox.wc_dir
> +
> + alpha = os.path.join(wc_dir, 'A/B/E/alpha')
> + beta = os.path.join(wc_dir, 'A/B/E/beta')
I think you need this here instead:
alpha = os.path.join(wc_dir, 'A', 'B', 'E', alpha')
beta = os.path.join(wc_dir, 'A', 'B', 'E', 'beta')
Otherwise you'll get something like this on Windows:
C:\foo\bar\wc_dir\A/B/E/alpha
C:\foo\bar\wc_dir\A/B/E/beta
> + non_existent_path = os.path.join(wc_dir, 'non-existent')
> +
> + # All targets are existing
> + svntest.actions.run_and_verify_svn2(None, None, [],
> + 0, 'ls', alpha, beta)
> +
> + # One non-existing target
> + expected_err = "svn: warning: W155010: The node '" + \
> + re.escape(os.path.abspath(non_existent_path)) + "' was not found.\n" + \
> + ".*\nsvn: E200000: A problem occurred; see other errors for details\n"
> + expected_err_re = re.compile(expected_err)
> +
> + exit_code, output, error = svntest.main.run_svn(1, 'ls', alpha,
> + non_existent_path, beta)
> +
> + # Verify error
> + if not expected_err_re.match("".join(error)):
> + raise svntest.Failure('Cat failed: expected error "%s", but received '
> + '"%s"' % (expected_err, "".join(error)))
> +
> +def ls_multiple_url_targets(sbox):
> + "ls multiple url targets"
> +
> + sbox.build(read_only = True, create_wc = False)
> +
> + alpha = sbox.repo_url + '/A/B/E/alpha'
> + beta = sbox.repo_url + '/A/B/E/beta'
> + non_existent_url = sbox.repo_url + '/non-existent'
> +
> + # All targets are existing
> + svntest.actions.run_and_verify_svn2(None, None, [],
> + 0, 'ls', alpha, beta)
> +
> + # One non-existing target
> + expected_err = "svn: warning: W160013: .*\n" + \
> + ".*\nsvn: E200000: A problem occurred; see other errors for details\n"
> + expected_err_re = re.compile(expected_err)
> +
> + exit_code, output, error = svntest.main.run_svn(1, 'ls', alpha,
> + non_existent_url, beta)
> +
> + # Verify error
> + if not expected_err_re.match("".join(error)):
> + raise svntest.Failure('Cat failed: expected error "%s", but received "%s"' % \
> + (expected_err, "".join(error)))
> +
> ########################################################################
> # Run the tests
>
> @@ -2751,6 +2830,10 @@
> basic_relocate,
> delete_urls_with_spaces,
> ls_url_special_characters,
> + ls_non_existent_wc_target,
> + ls_non_existent_url_target,
> + ls_multiple_wc_targets,
> + ls_multiple_url_targets,
4 tests! Nice :)
> ]
>
> if __name__ == '__main__':
> Index: subversion/svn/list-cmd.c
> ===================================================================
> --- subversion/svn/list-cmd.c (revision 1068757)
> +++ subversion/svn/list-cmd.c (working copy)
> @@ -215,6 +215,8 @@
> apr_pool_t *subpool = svn_pool_create(pool);
> apr_uint32_t dirent_fields;
> struct print_baton pb;
> + svn_boolean_t saw_a_problem = FALSE;
Maybe give this variable a more specific name, like 'seen_nonexistent_target'?
> + svn_error_t *err;
>
> SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
> opt_state->targets,
> @@ -280,14 +282,29 @@
> SVN_ERR(svn_cl__error_checked_fputs(sb->data, stdout));
> }
>
> - SVN_ERR(svn_client_list2(truepath, &peg_revision,
> - &(opt_state->start_revision),
> - opt_state->depth,
> - dirent_fields,
> - (opt_state->xml || opt_state->verbose),
> - opt_state->xml ? print_dirent_xml : print_dirent,
> - &pb, ctx, subpool));
> + err = svn_client_list2(truepath, &peg_revision,
> + &(opt_state->start_revision),
> + opt_state->depth,
> + dirent_fields,
> + (opt_state->xml || opt_state->verbose),
> + opt_state->xml ? print_dirent_xml : print_dirent,
> + &pb, ctx, subpool);
>
> + if (err)
> + {
> + /* If one of the targets is a non-existent URL or wc-entry,
> + don't bail out. Just warn and move on to the next target. */
> + if (err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND ||
> + err->apr_err == SVN_ERR_FS_NOT_FOUND)
> + svn_handle_warning2(stderr, err, "svn: ");
> + else
> + return svn_error_return(err);
> +
> + svn_error_clear(err);
> + err = NULL;
> + saw_a_problem = TRUE;
> + }
> +
> if (opt_state->xml)
> {
> svn_stringbuf_t *sb = svn_stringbuf_create("", pool);
> @@ -301,5 +318,8 @@
> if (opt_state->xml && ! opt_state->incremental)
> SVN_ERR(svn_cl__xml_print_footer("lists", pool));
>
> - return SVN_NO_ERROR;
> + if (saw_a_problem)
> + return svn_error_create(SVN_ERR_BASE, NULL, NULL);
> + else
> + return SVN_NO_ERROR;
> }
Re: [PATCH] svn command - ls - Multiple targets
Posted by Noorul Islam K M <no...@collab.net>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
> Noorul Islam K M wrote on Thu, Feb 10, 2011 at 16:50:39 +0530:
>
>> Index: subversion/tests/cmdline/basic_tests.py
>> +def ls_multiple_url_targets(sbox):
>> + "ls multiple url targets"
>> +
>> + sbox.build()
>> +
>> + alpha = sbox.repo_url + '/A/B/E/alpha'
>> + beta = sbox.repo_url + '/A/B/E/beta'
>> + non_existent_url = sbox.repo_url + '/non-existent'
>
> Can you please pass create_wc and/or read_only to sbox.build() when
> appropriate? That way the test will not create its own repos+wc and the
> tests will be slightly faster.
Thank you for providing this information. Everyday I am learning
something new. Please find attached updated patch with your review
comments incorporated.
Thanks and Regards
Noorul
Re: [PATCH] svn command - ls - Multiple targets
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Noorul Islam K M wrote on Thu, Feb 10, 2011 at 16:50:39 +0530:
> Index: subversion/tests/cmdline/basic_tests.py
> +def ls_multiple_url_targets(sbox):
> + "ls multiple url targets"
> +
> + sbox.build()
> +
> + alpha = sbox.repo_url + '/A/B/E/alpha'
> + beta = sbox.repo_url + '/A/B/E/beta'
> + non_existent_url = sbox.repo_url + '/non-existent'
Can you please pass create_wc and/or read_only to sbox.build() when
appropriate? That way the test will not create its own repos+wc and the
tests will be slightly faster.
Re: [PATCH] svn command - ls - Multiple targets
Posted by Noorul Islam K M <no...@collab.net>.
Noorul Islam K M <no...@collab.net> writes:
> Noorul Islam K M <no...@collab.net> writes:
>
>> This patch is a followup of the following thread. All tests pass with
>> this patch.
>>
>> http://svn.haxx.se/dev/archive-2011-01/0210.shtml
>>
>> Log
>> [[[
>>
>> Make 'svn ls' continue processing targets after printing warning if one
>> or more of the targets is a non-existent URL or wc-entry. Also return a
>> non-zero error code and print an error message at the end in those
>> situations.
>>
>> * subversion/svn/list-cmd.c
>> (svn_cl__list): If one of the targets is a non-existent URL or
>> wc-entry, don't bail out. Just warn and move on to the next
>> target. Also return a non-zero error code and print an error message
>> at the end in those situations.
>>
>> * subversion/tests/cmdline/basic_tests.py
>> (ls_non_existent_wc_target, ls_non_existent_url_target,
>> ls_multiple_wc_targets, ls_multiple_url_targets): New tests.
>> (test_list): Add reference to new tests.
>>
>> Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
>> ]]]
>>
>> Index: subversion/tests/cmdline/basic_tests.py
>> ===================================================================
>> --- subversion/tests/cmdline/basic_tests.py (revision 1069209)
>> +++ subversion/tests/cmdline/basic_tests.py (working copy)
>> @@ -2690,6 +2690,84 @@
>> [], 'ls',
>> url)
>>
>> +def ls_non_existent_wc_target(sbox):
>> + "ls a non-existent wc target"
>> +
>> + sbox.build()
>> + wc_dir = sbox.wc_dir
>> +
>> + non_existent_path = os.path.join(wc_dir, 'non-existent')
>> + expected_err = "svn: warning: W155010: The node '" + \
>> + os.path.abspath(non_existent_path) + "' was not found"
>> +
>> + svntest.actions.run_and_verify_svn2(None, None, expected_err,
>> + 1, 'ls', non_existent_path)
>> +
>> +def ls_non_existent_url_target(sbox):
>> + "ls a non-existent url target"
>> +
>> + sbox.build()
>> +
>> + non_existent_url = sbox.repo_url + '/non-existent'
>> + expected_err = "svn: warning: W160013: .*"
>> +
>> + svntest.actions.run_and_verify_svn2(None, None, expected_err,
>> + 1, 'ls', non_existent_url)
>> +
>> +def ls_multiple_wc_targets(sbox):
>> + "ls multiple wc targets"
>> +
>> + sbox.build()
>> + wc_dir = sbox.wc_dir
>> +
>> + alpha = os.path.join(wc_dir, 'A/B/E/alpha')
>> + beta = os.path.join(wc_dir, 'A/B/E/beta')
>> + non_existent_path = os.path.join(wc_dir, 'non-existent')
>> +
>> + # All targets are existing
>> + svntest.actions.run_and_verify_svn2(None, None, [],
>> + 0, 'ls', alpha, beta)
>> +
>> + # One non-existing target
>> + expected_err = "svn: warning: W155010: The node '" + \
>> + os.path.abspath(non_existent_path) + "' was not found.\n" + \
>> + ".*\nsvn: E200000: A problem occurred; see other errors for details\n"
>> + expected_err_re = re.compile(expected_err)
>> +
>> + exit_code, output, error = svntest.main.run_svn(1, 'ls', alpha,
>> + non_existent_path, beta)
>> +
>> + # Verify error
>> + if not expected_err_re.match("".join(error)):
>> + raise svntest.Failure('Cat failed: expected error "%s", but received '
>> + '"%s"' % (expected_err, "".join(error)))
>> +
>> +def ls_multiple_url_targets(sbox):
>> + "ls multiple url targets"
>> +
>> + sbox.build()
>> +
>> + alpha = sbox.repo_url + '/A/B/E/alpha'
>> + beta = sbox.repo_url + '/A/B/E/beta'
>> + non_existent_url = sbox.repo_url + '/non-existent'
>> +
>> + # All targets are existing
>> + svntest.actions.run_and_verify_svn2(None, None, [],
>> + 0, 'ls', alpha, beta)
>> +
>> + # One non-existing target
>> + expected_err = "svn: warning: W160013: .*\n" + \
>> + ".*\nsvn: E200000: A problem occurred; see other errors for details\n"
>> + expected_err_re = re.compile(expected_err)
>> +
>> + exit_code, output, error = svntest.main.run_svn(1, 'ls', alpha,
>> + non_existent_url, beta)
>> +
>> + # Verify error
>> + if not expected_err_re.match("".join(error)):
>> + raise svntest.Failure('Cat failed: expected error "%s", but received "%s"' % \
>> + (expected_err, "".join(error)))
>> +
>> ########################################################################
>> # Run the tests
>>
>> @@ -2751,6 +2829,10 @@
>> basic_relocate,
>> delete_urls_with_spaces,
>> ls_url_special_characters,
>> + ls_non_existent_wc_target,
>> + ls_non_existent_url_target,
>> + ls_multiple_wc_targets,
>> + ls_multiple_url_targets,
>> ]
>>
>> if __name__ == '__main__':
>> Index: subversion/svn/list-cmd.c
>> ===================================================================
>> --- subversion/svn/list-cmd.c (revision 1069209)
>> +++ subversion/svn/list-cmd.c (working copy)
>> @@ -215,6 +215,8 @@
>> apr_pool_t *subpool = svn_pool_create(pool);
>> apr_uint32_t dirent_fields;
>> struct print_baton pb;
>> + svn_boolean_t saw_a_problem = FALSE;
>> + svn_error_t *err;
>>
>> SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
>> opt_state->targets,
>> @@ -280,14 +282,29 @@
>> SVN_ERR(svn_cl__error_checked_fputs(sb->data, stdout));
>> }
>>
>> - SVN_ERR(svn_client_list2(truepath, &peg_revision,
>> - &(opt_state->start_revision),
>> - opt_state->depth,
>> - dirent_fields,
>> - (opt_state->xml || opt_state->verbose),
>> - opt_state->xml ? print_dirent_xml : print_dirent,
>> - &pb, ctx, subpool));
>> + err = svn_client_list2(truepath, &peg_revision,
>> + &(opt_state->start_revision),
>> + opt_state->depth,
>> + dirent_fields,
>> + (opt_state->xml || opt_state->verbose),
>> + opt_state->xml ? print_dirent_xml : print_dirent,
>> + &pb, ctx, subpool);
>>
>> + if (err)
>> + {
>> + /* If one of the targets is a non-existent URL or wc-entry,
>> + don't bail out. Just warn and move on to the next target. */
>> + if (err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND ||
>> + err->apr_err == SVN_ERR_FS_NOT_FOUND)
>> + svn_handle_warning2(stderr, err, "svn: ");
>> + else
>> + return svn_error_return(err);
>> +
>> + svn_error_clear(err);
>> + err = NULL;
>> + saw_a_problem = TRUE;
>> + }
>> +
>> if (opt_state->xml)
>> {
>> svn_stringbuf_t *sb = svn_stringbuf_create("", pool);
>> @@ -301,5 +318,8 @@
>> if (opt_state->xml && ! opt_state->incremental)
>> SVN_ERR(svn_cl__xml_print_footer("lists", pool));
>>
>> - return SVN_NO_ERROR;
>> + if (saw_a_problem)
>> + return svn_error_create(SVN_ERR_BASE, NULL, NULL);
>> + else
>> + return SVN_NO_ERROR;
>> }
>
> A slightly modified patch is attached. This one escapes windows path
> separator '\' in regex so that test would pass on windows build.
>
> Thanks and Regards
> Noorul
>
> Index: subversion/tests/cmdline/basic_tests.py
> ===================================================================
> --- subversion/tests/cmdline/basic_tests.py (revision 1069209)
> +++ subversion/tests/cmdline/basic_tests.py (working copy)
> @@ -25,7 +25,7 @@
> ######################################################################
>
> # General modules
> -import shutil, stat, re, os
> +import shutil, stat, re, os, sys
>
> # Our testing module
> import svntest
> @@ -2690,6 +2690,91 @@
> [], 'ls',
> url)
>
> +def ls_non_existent_wc_target(sbox):
> + "ls a non-existent wc target"
> +
> + sbox.build()
> + wc_dir = sbox.wc_dir
> +
> + non_existent_path = os.path.join(wc_dir, 'non-existent')
> +
> + if sys.platform == 'win32':
> + non_existent_path = non_existent_path.replace("\\", "\\\\")
> +
> + expected_err = "svn: warning: W155010: The node '" + \
> + os.path.abspath(non_existent_path) + "' was not found"
> +
> + svntest.actions.run_and_verify_svn2(None, None, expected_err,
> + 1, 'ls', non_existent_path)
> +
> +def ls_non_existent_url_target(sbox):
> + "ls a non-existent url target"
> +
> + sbox.build()
> +
> + non_existent_url = sbox.repo_url + '/non-existent'
> + expected_err = "svn: warning: W160013: .*"
> +
> + svntest.actions.run_and_verify_svn2(None, None, expected_err,
> + 1, 'ls', non_existent_url)
> +
> +def ls_multiple_wc_targets(sbox):
> + "ls multiple wc targets"
> +
> + sbox.build()
> + wc_dir = sbox.wc_dir
> +
> + alpha = os.path.join(wc_dir, 'A/B/E/alpha')
> + beta = os.path.join(wc_dir, 'A/B/E/beta')
> + non_existent_path = os.path.join(wc_dir, 'non-existent')
> +
> + if sys.platform == 'win32':
> + non_existent_path = non_existent_path.replace("\\", "\\\\")
> +
> + # All targets are existing
> + svntest.actions.run_and_verify_svn2(None, None, [],
> + 0, 'ls', alpha, beta)
> +
> + # One non-existing target
> + expected_err = "svn: warning: W155010: The node '" + \
> + os.path.abspath(non_existent_path) + "' was not found.\n" + \
> + ".*\nsvn: E200000: A problem occurred; see other errors for details\n"
> + expected_err_re = re.compile(expected_err)
> +
> + exit_code, output, error = svntest.main.run_svn(1, 'ls', alpha,
> + non_existent_path, beta)
> +
> + # Verify error
> + if not expected_err_re.match("".join(error)):
> + raise svntest.Failure('Cat failed: expected error "%s", but received '
> + '"%s"' % (expected_err, "".join(error)))
> +
> +def ls_multiple_url_targets(sbox):
> + "ls multiple url targets"
> +
> + sbox.build()
> +
> + alpha = sbox.repo_url + '/A/B/E/alpha'
> + beta = sbox.repo_url + '/A/B/E/beta'
> + non_existent_url = sbox.repo_url + '/non-existent'
> +
> + # All targets are existing
> + svntest.actions.run_and_verify_svn2(None, None, [],
> + 0, 'ls', alpha, beta)
> +
> + # One non-existing target
> + expected_err = "svn: warning: W160013: .*\n" + \
> + ".*\nsvn: E200000: A problem occurred; see other errors for details\n"
> + expected_err_re = re.compile(expected_err)
> +
> + exit_code, output, error = svntest.main.run_svn(1, 'ls', alpha,
> + non_existent_url, beta)
> +
> + # Verify error
> + if not expected_err_re.match("".join(error)):
> + raise svntest.Failure('Cat failed: expected error "%s", but received "%s"' % \
> + (expected_err, "".join(error)))
> +
> ########################################################################
> # Run the tests
>
> @@ -2751,6 +2836,10 @@
> basic_relocate,
> delete_urls_with_spaces,
> ls_url_special_characters,
> + ls_non_existent_wc_target,
> + ls_non_existent_url_target,
> + ls_multiple_wc_targets,
> + ls_multiple_url_targets,
> ]
>
> if __name__ == '__main__':
> Index: subversion/svn/list-cmd.c
> ===================================================================
> --- subversion/svn/list-cmd.c (revision 1069209)
> +++ subversion/svn/list-cmd.c (working copy)
> @@ -215,6 +215,8 @@
> apr_pool_t *subpool = svn_pool_create(pool);
> apr_uint32_t dirent_fields;
> struct print_baton pb;
> + svn_boolean_t saw_a_problem = FALSE;
> + svn_error_t *err;
>
> SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
> opt_state->targets,
> @@ -280,14 +282,29 @@
> SVN_ERR(svn_cl__error_checked_fputs(sb->data, stdout));
> }
>
> - SVN_ERR(svn_client_list2(truepath, &peg_revision,
> - &(opt_state->start_revision),
> - opt_state->depth,
> - dirent_fields,
> - (opt_state->xml || opt_state->verbose),
> - opt_state->xml ? print_dirent_xml : print_dirent,
> - &pb, ctx, subpool));
> + err = svn_client_list2(truepath, &peg_revision,
> + &(opt_state->start_revision),
> + opt_state->depth,
> + dirent_fields,
> + (opt_state->xml || opt_state->verbose),
> + opt_state->xml ? print_dirent_xml : print_dirent,
> + &pb, ctx, subpool);
>
> + if (err)
> + {
> + /* If one of the targets is a non-existent URL or wc-entry,
> + don't bail out. Just warn and move on to the next target. */
> + if (err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND ||
> + err->apr_err == SVN_ERR_FS_NOT_FOUND)
> + svn_handle_warning2(stderr, err, "svn: ");
> + else
> + return svn_error_return(err);
> +
> + svn_error_clear(err);
> + err = NULL;
> + saw_a_problem = TRUE;
> + }
> +
> if (opt_state->xml)
> {
> svn_stringbuf_t *sb = svn_stringbuf_create("", pool);
> @@ -301,5 +318,8 @@
> if (opt_state->xml && ! opt_state->incremental)
> SVN_ERR(svn_cl__xml_print_footer("lists", pool));
>
> - return SVN_NO_ERROR;
> + if (saw_a_problem)
> + return svn_error_create(SVN_ERR_BASE, NULL, NULL);
> + else
> + return SVN_NO_ERROR;
> }
I got this hint from one of the recent commits. So even better way of
escaping '\'. Attached is the modified patch.
Thanks and Regards
Noorul
Re: [PATCH] svn command - ls - Multiple targets
Posted by Noorul Islam K M <no...@collab.net>.
Noorul Islam K M <no...@collab.net> writes:
> This patch is a followup of the following thread. All tests pass with
> this patch.
>
> http://svn.haxx.se/dev/archive-2011-01/0210.shtml
>
> Log
> [[[
>
> Make 'svn ls' continue processing targets after printing warning if one
> or more of the targets is a non-existent URL or wc-entry. Also return a
> non-zero error code and print an error message at the end in those
> situations.
>
> * subversion/svn/list-cmd.c
> (svn_cl__list): If one of the targets is a non-existent URL or
> wc-entry, don't bail out. Just warn and move on to the next
> target. Also return a non-zero error code and print an error message
> at the end in those situations.
>
> * subversion/tests/cmdline/basic_tests.py
> (ls_non_existent_wc_target, ls_non_existent_url_target,
> ls_multiple_wc_targets, ls_multiple_url_targets): New tests.
> (test_list): Add reference to new tests.
>
> Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
> ]]]
>
> Index: subversion/tests/cmdline/basic_tests.py
> ===================================================================
> --- subversion/tests/cmdline/basic_tests.py (revision 1069209)
> +++ subversion/tests/cmdline/basic_tests.py (working copy)
> @@ -2690,6 +2690,84 @@
> [], 'ls',
> url)
>
> +def ls_non_existent_wc_target(sbox):
> + "ls a non-existent wc target"
> +
> + sbox.build()
> + wc_dir = sbox.wc_dir
> +
> + non_existent_path = os.path.join(wc_dir, 'non-existent')
> + expected_err = "svn: warning: W155010: The node '" + \
> + os.path.abspath(non_existent_path) + "' was not found"
> +
> + svntest.actions.run_and_verify_svn2(None, None, expected_err,
> + 1, 'ls', non_existent_path)
> +
> +def ls_non_existent_url_target(sbox):
> + "ls a non-existent url target"
> +
> + sbox.build()
> +
> + non_existent_url = sbox.repo_url + '/non-existent'
> + expected_err = "svn: warning: W160013: .*"
> +
> + svntest.actions.run_and_verify_svn2(None, None, expected_err,
> + 1, 'ls', non_existent_url)
> +
> +def ls_multiple_wc_targets(sbox):
> + "ls multiple wc targets"
> +
> + sbox.build()
> + wc_dir = sbox.wc_dir
> +
> + alpha = os.path.join(wc_dir, 'A/B/E/alpha')
> + beta = os.path.join(wc_dir, 'A/B/E/beta')
> + non_existent_path = os.path.join(wc_dir, 'non-existent')
> +
> + # All targets are existing
> + svntest.actions.run_and_verify_svn2(None, None, [],
> + 0, 'ls', alpha, beta)
> +
> + # One non-existing target
> + expected_err = "svn: warning: W155010: The node '" + \
> + os.path.abspath(non_existent_path) + "' was not found.\n" + \
> + ".*\nsvn: E200000: A problem occurred; see other errors for details\n"
> + expected_err_re = re.compile(expected_err)
> +
> + exit_code, output, error = svntest.main.run_svn(1, 'ls', alpha,
> + non_existent_path, beta)
> +
> + # Verify error
> + if not expected_err_re.match("".join(error)):
> + raise svntest.Failure('Cat failed: expected error "%s", but received '
> + '"%s"' % (expected_err, "".join(error)))
> +
> +def ls_multiple_url_targets(sbox):
> + "ls multiple url targets"
> +
> + sbox.build()
> +
> + alpha = sbox.repo_url + '/A/B/E/alpha'
> + beta = sbox.repo_url + '/A/B/E/beta'
> + non_existent_url = sbox.repo_url + '/non-existent'
> +
> + # All targets are existing
> + svntest.actions.run_and_verify_svn2(None, None, [],
> + 0, 'ls', alpha, beta)
> +
> + # One non-existing target
> + expected_err = "svn: warning: W160013: .*\n" + \
> + ".*\nsvn: E200000: A problem occurred; see other errors for details\n"
> + expected_err_re = re.compile(expected_err)
> +
> + exit_code, output, error = svntest.main.run_svn(1, 'ls', alpha,
> + non_existent_url, beta)
> +
> + # Verify error
> + if not expected_err_re.match("".join(error)):
> + raise svntest.Failure('Cat failed: expected error "%s", but received "%s"' % \
> + (expected_err, "".join(error)))
> +
> ########################################################################
> # Run the tests
>
> @@ -2751,6 +2829,10 @@
> basic_relocate,
> delete_urls_with_spaces,
> ls_url_special_characters,
> + ls_non_existent_wc_target,
> + ls_non_existent_url_target,
> + ls_multiple_wc_targets,
> + ls_multiple_url_targets,
> ]
>
> if __name__ == '__main__':
> Index: subversion/svn/list-cmd.c
> ===================================================================
> --- subversion/svn/list-cmd.c (revision 1069209)
> +++ subversion/svn/list-cmd.c (working copy)
> @@ -215,6 +215,8 @@
> apr_pool_t *subpool = svn_pool_create(pool);
> apr_uint32_t dirent_fields;
> struct print_baton pb;
> + svn_boolean_t saw_a_problem = FALSE;
> + svn_error_t *err;
>
> SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
> opt_state->targets,
> @@ -280,14 +282,29 @@
> SVN_ERR(svn_cl__error_checked_fputs(sb->data, stdout));
> }
>
> - SVN_ERR(svn_client_list2(truepath, &peg_revision,
> - &(opt_state->start_revision),
> - opt_state->depth,
> - dirent_fields,
> - (opt_state->xml || opt_state->verbose),
> - opt_state->xml ? print_dirent_xml : print_dirent,
> - &pb, ctx, subpool));
> + err = svn_client_list2(truepath, &peg_revision,
> + &(opt_state->start_revision),
> + opt_state->depth,
> + dirent_fields,
> + (opt_state->xml || opt_state->verbose),
> + opt_state->xml ? print_dirent_xml : print_dirent,
> + &pb, ctx, subpool);
>
> + if (err)
> + {
> + /* If one of the targets is a non-existent URL or wc-entry,
> + don't bail out. Just warn and move on to the next target. */
> + if (err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND ||
> + err->apr_err == SVN_ERR_FS_NOT_FOUND)
> + svn_handle_warning2(stderr, err, "svn: ");
> + else
> + return svn_error_return(err);
> +
> + svn_error_clear(err);
> + err = NULL;
> + saw_a_problem = TRUE;
> + }
> +
> if (opt_state->xml)
> {
> svn_stringbuf_t *sb = svn_stringbuf_create("", pool);
> @@ -301,5 +318,8 @@
> if (opt_state->xml && ! opt_state->incremental)
> SVN_ERR(svn_cl__xml_print_footer("lists", pool));
>
> - return SVN_NO_ERROR;
> + if (saw_a_problem)
> + return svn_error_create(SVN_ERR_BASE, NULL, NULL);
> + else
> + return SVN_NO_ERROR;
> }
A slightly modified patch is attached. This one escapes windows path
separator '\' in regex so that test would pass on windows build.
Thanks and Regards
Noorul