Julian Foad wrote on Mon, May 19, 2014 at 12:15:00 +0100:
> Daniel Shahaf wrote:
>
> > URL: http://svn.apache.org/r1592724
> > Log:
> > Follow-up to r3265 (r843339): In 'svn propget -r', error out on
> > non-existing properties.
>
> This makes 'propget --revprop' inconsistent with 'propget [versioned
> prop]' which still prints nothing. Did you intend to change both
> forms, for consistency with 'svnlook'?
>
> Previously, 'svn propget' would print nothing for a nonexistent prop
> (either versioned or revprop), while 'svnlook propget' would issue an
> error for a non-existent prop (either versioned or revprop), which was
> a bit more consistent than what we now have.
I've updated issue #4505 with answers to the concerns raised in the thread:
http://subversion.tigris.org/issues/show_bug.cgi?id=4505#desc2
To summarize, the answers are: effect on XML mode is the same as on non-XML
mode; there is no regression in -R mode because r1592724 only changed --revprop
mode, which ignores -R; and there are no other inconsistencies beyond 'svn
propget nodeprop' not erroring.
At this point we have two options: either revert the change (so 'svn
propget' doesn't error either in revprop mode or in nodeprop mode; this
is the 1.8 behaviour) or make 'svn propget nodeprop' (without -R) error
out too.
I'm attaching a patch implementing the latter.
Thoughts?
Daniel
---
Personally, I lean towards applying the patch. Like any change, it may break
users who depend on the current semantics, but it's also likely to convert
silent failure modes to noisy failures for users who didn't realize 'propget
svn:lgmsg' (sic) doesn't error. For scripted users, the current behaviour
means non-existent properties are indistinguishable from empty-valued ones
(unless 'proplist' is used to query existence).
Index: subversion/svn/propget-cmd.c
===================================================================
--- subversion/svn/propget-cmd.c (revision 1598027)
+++ subversion/svn/propget-cmd.c (working copy)
@@ -320,6 +320,7 @@ svn_cl__propget(apr_getopt_t *os,
const char *pname, *pname_utf8;
apr_array_header_t *args, *targets;
svn_stream_t *out;
+ svn_boolean_t warned = FALSE;
if (opt_state->verbose && (opt_state->revprop || opt_state->strict
|| opt_state->xml))
@@ -480,6 +481,21 @@ svn_cl__propget(apr_getopt_t *os,
omit_newline = opt_state->strict;
like_proplist = opt_state->verbose && !opt_state->strict;
+ /* If there are no properties, and exactly one node was queried,
+ then warn. */
+ if (opt_state->depth == svn_depth_empty
+ && !opt_state->show_inherited_props
+ && apr_hash_count(props) == 0)
+ {
+ svn_error_t *err;
+ err = svn_error_createf(SVN_ERR_PROPERTY_NOT_FOUND, NULL,
+ _("Property '%s' not found on '%s'"),
+ pname_utf8, target);
+ svn_handle_warning2(stderr, err, "svn: ");
+ svn_error_clear(err);
+ warned = TRUE;
+ }
+
if (opt_state->xml)
SVN_ERR(print_properties_xml(
pname_utf8, props,
@@ -500,5 +516,8 @@ svn_cl__propget(apr_getopt_t *os,
svn_pool_destroy(subpool);
}
+ if (warned)
+ return svn_error_create(SVN_ERR_BASE, NULL, NULL);
+
return SVN_NO_ERROR;
}
Index: subversion/tests/cmdline/copy_tests.py
===================================================================
--- subversion/tests/cmdline/copy_tests.py (revision 1598027)
+++ subversion/tests/cmdline/copy_tests.py (working copy)
@@ -1039,8 +1039,8 @@ def repos_to_wc(sbox):
})
svntest.actions.run_and_verify_status(wc_dir, expected_output)
- # Validate the merge info of the copy destination (we expect none)
- svntest.actions.run_and_verify_svn(None, [], [],
+ # Validate the mergeinfo of the copy destination (we expect none)
+ svntest.actions.run_and_verify_svn(None, [], '.*W200017: Property.*not found',
'propget', SVN_PROP_MERGEINFO,
os.path.join(D_dir, 'B'))
@@ -2737,8 +2737,8 @@ def copy_added_paths_to_URL(sbox):
'cp', '-m', '',
upsilon_path, upsilon_copy_URL)
- # Validate the merge info of the copy destination (we expect none).
- svntest.actions.run_and_verify_svn(None, [], [],
+ # Validate the mergeinfo of the copy destination (we expect none).
+ svntest.actions.run_and_verify_svn(None, [], '.*W200017: Property.*not found',
'propget',
SVN_PROP_MERGEINFO, upsilon_copy_URL)
@@ -3428,7 +3428,7 @@ def copy_peg_rev_url(sbox):
sigma_url + '@', '-m', 'rev 3')
# Validate the copy destination's mergeinfo (we expect none).
- svntest.actions.run_and_verify_svn(None, [], [],
+ svntest.actions.run_and_verify_svn(None, [], '.*W200017: Property.*not found',
'propget', SVN_PROP_MERGEINFO, sigma_url)
# Update to HEAD and verify disk contents
Index: subversion/tests/cmdline/externals_tests.py
===================================================================
--- subversion/tests/cmdline/externals_tests.py (revision 1598027)
+++ subversion/tests/cmdline/externals_tests.py (working copy)
@@ -3029,9 +3029,9 @@ def duplicate_targets(sbox):
'svn:externals', '^/A/B/E ././barf\n^/A/D/G .//barf', wc_dir)
# svn pg svn:externals .
- expected_stdout = []
+ expected_stderr = '.*W200017: Property.*not found'
- actions.run_and_verify_svn2('OUTPUT', expected_stdout, [], 0, 'pg',
+ actions.run_and_verify_svn2('OUTPUT', [], expected_stderr, 1, 'pg',
'svn:externals', wc_dir)
# svn ps svn:externals "^/A/B/E ok" .
Index: subversion/tests/cmdline/merge_reintegrate_tests.py
===================================================================
--- subversion/tests/cmdline/merge_reintegrate_tests.py (revision 1598027)
+++ subversion/tests/cmdline/merge_reintegrate_tests.py (working copy)
@@ -2528,7 +2528,9 @@ def no_source_subtree_mergeinfo(sbox):
svntest.main.run_svn(None, 'update', wc_dir)
# Verify that merge results in no subtree mergeinfo
- svntest.actions.run_and_verify_svn(None, [], [], 'propget', 'svn:mergeinfo',
+ expected_stderr = '.*W200017: Property.*not found'
+ svntest.actions.run_and_verify_svn(None, [], expected_stderr,
+ 'propget', 'svn:mergeinfo',
sbox.repo_url + '/A/B2/E')
# Merge trunk to branch-2
@@ -2537,7 +2539,8 @@ def no_source_subtree_mergeinfo(sbox):
svntest.main.run_svn(None, 'update', wc_dir)
# Verify that there is still no subtree mergeinfo
- svntest.actions.run_and_verify_svn(None, [], [], 'propget', 'svn:mergeinfo',
+ svntest.actions.run_and_verify_svn(None, [], expected_stderr,
+ 'propget', 'svn:mergeinfo',
sbox.repo_url + '/A/B2/E')
# Reintegrate branch-2 to trunk, this fails in 1.6.x from 1.6.13.
Index: subversion/tests/cmdline/merge_tests.py
===================================================================
--- subversion/tests/cmdline/merge_tests.py (revision 1598027)
+++ subversion/tests/cmdline/merge_tests.py (working copy)
@@ -355,7 +355,7 @@ def textual_merges_galore(sbox):
svntest.tree.detect_conflict_files, list(tau_conflict_support_files))
- svntest.actions.run_and_verify_svn(None, [], [],
+ svntest.actions.run_and_verify_svn(None, [], '.*W200017: Property.*not found',
'propget', SVN_PROP_MERGEINFO,
os.path.join(other_wc,
"A", "D", "G", "rho"))
@@ -4894,7 +4894,7 @@ def mergeinfo_elision(sbox):
svntest.actions.run_and_verify_status(beta_COPY_path, expected_status)
# Once again A_COPY/B/E/beta has no mergeinfo.
- svntest.actions.run_and_verify_svn(None, [], [],
+ svntest.actions.run_and_verify_svn(None, [], '.*W200017: Property.*not found',
'propget', SVN_PROP_MERGEINFO,
beta_COPY_path)
@@ -5977,7 +5977,8 @@ def empty_mergeinfo(sbox):
A_COPY_path)
svntest.actions.run_and_verify_status(wc_dir, wc_status)
# Check that A_COPY's mergeinfo is gone.
- svntest.actions.run_and_verify_svn(None, [], [], 'pg', 'svn:mergeinfo',
+ svntest.actions.run_and_verify_svn(None, [], '.*W200017: Property.*not found',
+ 'pg', 'svn:mergeinfo',
A_COPY_path)
#----------------------------------------------------------------------
Index: subversion/tests/cmdline/prop_tests.py
===================================================================
--- subversion/tests/cmdline/prop_tests.py (revision 1598027)
+++ subversion/tests/cmdline/prop_tests.py (working copy)
@@ -1601,12 +1601,13 @@ def props_over_time(sbox):
pget_expected = expected
if pget_expected:
pget_expected = [ pget_expected + "\n" ]
+ expected_err = [] if expected else '.*W200017: Property.*not found.*'
if op_rev != 0:
- svntest.actions.run_and_verify_svn(None, pget_expected, [],
+ svntest.actions.run_and_verify_svn(None, pget_expected, expected_err,
'propget', 'revision', peg_path,
'-r', str(op_rev))
else:
- svntest.actions.run_and_verify_svn(None, pget_expected, [],
+ svntest.actions.run_and_verify_svn(None, pget_expected, expected_err,
'propget', 'revision', peg_path)
### Test 'svn proplist -v'
Index: subversion/tests/cmdline/resolve_tests.py
===================================================================
--- subversion/tests/cmdline/resolve_tests.py (revision 1598028)
+++ subversion/tests/cmdline/resolve_tests.py (working copy)
@@ -203,10 +203,16 @@ def prop_conflict_resolution(sbox):
'--accept=postpone', wc_dir)
svntest.actions.run_and_verify_resolve([iota_path, mu_path], '-R',
'--accept', resolve_accept, wc_dir)
+ if resolved_deleted_prop_val_output:
+ expected_deleted_stderr = []
+ else:
+ expected_deleted_stderr = '.*W200017: Property.*not found'
+
svntest.actions.run_and_verify_svn(
'svn revolve -R --accept=' + resolve_accept + ' of prop conflict '
'not resolved as expected;',
- resolved_deleted_prop_val_output, [], 'pg', 'propname', iota_path)
+ resolved_deleted_prop_val_output, expected_deleted_stderr,
+ 'pg', 'propname', iota_path)
svntest.actions.run_and_verify_svn(
'svn revolve -R --accept=' + resolve_accept + ' of prop conflict '
'not resolved as expected;',
Index: subversion/tests/cmdline/schedule_tests.py
===================================================================
--- subversion/tests/cmdline/schedule_tests.py (revision 1598027)
+++ subversion/tests/cmdline/schedule_tests.py (working copy)
@@ -184,15 +184,17 @@ def add_executable(sbox):
file_ospath = sbox.ospath(fileName)
if executable:
expected_out = ["*\n"]
+ expected_err = []
else:
expected_out = []
+ expected_err = '.*W200017: Property.*not found'
# create an empty file
open(file_ospath, "w")
os.chmod(file_ospath, perm)
sbox.simple_add(fileName)
- svntest.actions.run_and_verify_svn(None, expected_out, [],
+ svntest.actions.run_and_verify_svn(None, expected_out, expected_err,
'propget', "svn:executable", file_ospath)
test_cases = [
Index: subversion/tests/cmdline/svnmucc_tests.py
===================================================================
--- subversion/tests/cmdline/svnmucc_tests.py (revision 1598027)
+++ subversion/tests/cmdline/svnmucc_tests.py (working copy)
@@ -353,7 +353,8 @@ def propset_root_internal(sbox, target):
'-m', 'log msg',
'propdel', 'foo',
target)
- svntest.actions.run_and_verify_svn(None, [], [],
+ svntest.actions.run_and_verify_svn(None, [],
+ '.*W200017: Property.*not found',
'propget', '--strict', 'foo',
target)
Follow-up to r1592724: make 'svn propget nodeprop' error out on non-existing
properties, too.
* subversion/svn/propget-cmd.c
(svn_cl__propget): As above. Issue a warning for each explicit target
and error out at the end if any warnings have been issued.
* subversion/tests/cmdline/copy_tests.py
(repos_to_wc, copy_added_paths_to_URL, copy_peg_rev_url):
* subversion/tests/cmdline/externals_tests.py
(duplicate_targets):
* subversion/tests/cmdline/merge_reintegrate_tests.py
(no_source_subtree_mergeinfo):
* subversion/tests/cmdline/merge.py
(textual_merges_galore, mergeinfo_elision, empty_mergeinfo):
* subversion/tests/cmdline/prop_tests.py
(props_over_time):
* subversion/tests/cmdline/resolve_tests.py
(prop_conflict_resolution):
* subversion/tests/cmdline/schedule_tests.py
(add_executable):
* subversion/tests/cmdline/svnmucc_tests.py
(propset_root_internal):
Adjust failure mode expectations.
