Stefan Sperling <stsp_at_elego.de> writes:
> On Mon, Feb 14, 2011 at 07:32:51PM +0530, Noorul Islam K M wrote:
>
>> Daniel Shahaf <d.s_at_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
Index: subversion/tests/cmdline/basic_tests.py
===================================================================
--- subversion/tests/cmdline/basic_tests.py (revision 1070486)
+++ subversion/tests/cmdline/basic_tests.py (working copy)
@@ -2690,6 +2690,87 @@
[], '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 = sbox.ospath('A/B/E/alpha')
+ beta = sbox.ospath('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: E200009: Could not list all targets because some targets " + \
+ "don't exist\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: E200009: Could not list all targets because some targets " + \
+ "don't exist\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 +2832,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 1070486)
+++ 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 seen_nonexistent_target = 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;
+ seen_nonexistent_target = TRUE;
+ }
+
if (opt_state->xml)
{
svn_stringbuf_t *sb = svn_stringbuf_create("", pool);
@@ -301,5 +318,10 @@
if (opt_state->xml && ! opt_state->incremental)
SVN_ERR(svn_cl__xml_print_footer("lists", pool));
- return SVN_NO_ERROR;
+ if (seen_nonexistent_target)
+ 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;
}
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;
}
Received on 2011-02-14 15:26:12 CET