Stefan Sperling <s...@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
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; }