You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by da...@apache.org on 2014/05/06 13:48:06 UTC
svn commit: r1592724 - in /subversion/trunk/subversion: include/svn_client.h
svn/propget-cmd.c tests/cmdline/prop_tests.py tests/cmdline/svnadmin_tests.py
Author: danielsh
Date: Tue May 6 11:48:05 2014
New Revision: 1592724
URL: http://svn.apache.org/r1592724
Log:
Follow-up to r3265 (r843339): In 'svn propget -r', error out on non-existing properties.
* subversion/svn/propget-cmd.c
(svn_cl__propget): Handle the (propval == NULL) case.
* subversion/include/svn_client.h
(svn_client_revprop_get): Document API behaviour on non-existing properties.
* subversion/tests/cmdline/svnadmin_tests.py
(empty_date): Use exit code to verify lack of property.
* subversion/tests/cmdline/prop_tests.py
(revprop_change): Same. 'propget' was actually already failing (with
the expected E200017), but the test was ignoring the exit code.
(I think the before-this-change code can never fail, since
re.match('cha-ching', 'cha-ching\n') would return False.)
Modified:
subversion/trunk/subversion/include/svn_client.h
subversion/trunk/subversion/svn/propget-cmd.c
subversion/trunk/subversion/tests/cmdline/prop_tests.py
subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py
Modified: subversion/trunk/subversion/include/svn_client.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_client.h?rev=1592724&r1=1592723&r2=1592724&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_client.h (original)
+++ subversion/trunk/subversion/include/svn_client.h Tue May 6 11:48:05 2014
@@ -5242,6 +5242,8 @@ svn_client_propget(apr_hash_t **props,
* in @a ctx for authentication, and @a pool for all memory allocation.
* Return the actual rev queried in @a *set_rev.
*
+ * If @a propname does not exist on @a revision, set @a *propval to @c NULL.
+ *
* Note that unlike its cousin svn_client_propget(), this routine
* doesn't affect the working copy at all; it's a pure network
* operation that queries an *unversioned* property attached to a
Modified: subversion/trunk/subversion/svn/propget-cmd.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/propget-cmd.c?rev=1592724&r1=1592723&r2=1592724&view=diff
==============================================================================
--- subversion/trunk/subversion/svn/propget-cmd.c (original)
+++ subversion/trunk/subversion/svn/propget-cmd.c Tue May 6 11:48:05 2014
@@ -365,7 +365,14 @@ svn_cl__propget(apr_getopt_t *os,
URL, &(opt_state->start_revision),
&rev, ctx, pool));
- if (propval != NULL)
+ if (propval == NULL)
+ {
+ return svn_error_createf(SVN_ERR_PROPERTY_NOT_FOUND, NULL,
+ _("Property '%s' not found on "
+ "revision %ld"),
+ pname_utf8, opt_state->start_revision);
+ }
+ else
{
if (opt_state->xml)
{
Modified: subversion/trunk/subversion/tests/cmdline/prop_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/prop_tests.py?rev=1592724&r1=1592723&r2=1592724&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/prop_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/prop_tests.py Tue May 6 11:48:05 2014
@@ -847,14 +847,10 @@ def revprop_change(sbox):
'propdel', '--revprop', '-r', '0',
'cash-sound', sbox.wc_dir)
- actual_exit, actual_stdout, actual_stderr = svntest.main.run_svn(
- None, 'pg', '--revprop', '-r', '0', 'cash-sound', sbox.wc_dir)
-
# The property should have been deleted.
- regex = 'cha-ching'
- for line in actual_stdout:
- if re.match(regex, line):
- raise svntest.Failure
+ svntest.actions.run_and_verify_svn(None, None,
+ '.*(E195011|E200017).*cash-sound.*',
+ 'propget', '--revprop', '-r', '0', 'cash-sound', sbox.wc_dir)
#----------------------------------------------------------------------
Modified: subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py?rev=1592724&r1=1592723&r2=1592724&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/svnadmin_tests.py Tue May 6 11:48:05 2014
@@ -474,8 +474,8 @@ def empty_date(sbox):
'--ignore-uuid')
# Verify that the revision still lacks the svn:date property.
- svntest.actions.run_and_verify_svn(None, [], [], "propget",
- "--revprop", "-r1", "svn:date",
+ svntest.actions.run_and_verify_svn(None, [], '.*(E195011|E200017).*svn:date',
+ "propget", "--revprop", "-r1", "svn:date",
sbox.wc_dir)
#----------------------------------------------------------------------
Re: r1592724: In 'svn propget --revprop', error out on non-existing properties.
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Shahaf wrote:
> Julian Foad wrote on Mon, May 19, 2014 at 12:15:00 +0100:
>> Hi Daniel. Can I ask: Why?
>
> Because I assumed it would error, realized it didn't, and digging into
> the history revealed that it was probably an unintentional omission.
OK. I recall we made a similar change -- to 'propdel' I think -- a couple of releases ago, maybe 1.7.
>> This makes 'propget --revprop' inconsistent with 'propget [versioned
>> prop]' which still prints nothing. Did you intend to change both
>> forms, for consistency with 'svnlook'?
>
> I hadn't checked 'svn propget versioned_prop'. It makes
> sense to me that it should error out on non-existing properties.
Me too, but ...
> Would that be a reasonable course to take? Or does anyone prefer to
> retain the pre-r1592724 behaviour?
While I prefer your version as a better UI design, I think this particular change is one that might be better reverted, as the benefit of leaving the UI as it was might outweigh the benefit of changing it. I have in mind that some third parties do treat our CLI as an API, whatever we think of the rights and wrongs of that. For example, NetBeans IDE version 7 can use Subversion 1.8 *only* through the command-line client CLI [1].
The difficulty is I can't make an informed evaluation of the pros and cons by considering this change alone. The user experience may improve incrementally with each tweak we make [2], but if the first incompatible change breaks NetBeans's Subversion support (for example), the impact doesn't get much worse if we make lots more incompatible changes after that. Making one such change alone might therefore be a net loss, whereas making that same change as part of a larger set of changes might be a good thing.
Either we should batch up the proposed UI changes and consider as a whole whether they're worth making for the next release, or we should err on the side of caution and avoid making any change where the potential benefit is small and the potential for harm through incompatibility is significant. I might email separately about the idea of batching them up.
- Julian
[1] For other version combinations it can interface through JavaHL or SvnKit.
[2] At least for new users and adaptable users; ignoring habituation.
Re: svn commit: r1592724 - in /subversion/trunk/subversion:
include/svn_client.h svn/propget-cmd.c tests/cmdline/prop_tests.py
tests/cmdline/svnadmin_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
I've filed http://subversion.tigris.org/issues/show_bug.cgi?id=4505 to
track this.
Julian Foad wrote on Mon, May 19, 2014 at 17:53:19 +0100:
> Branko Čibej wrote:
> > On 19.05.2014 13:30, Daniel Shahaf wrote:
> >> Because I assumed it would error, realized it didn't, and digging into
> >> the history revealed that it was probably an unintentional omission.
> [...]
> >> I hadn't checked 'svn propget versioned_prop'. It makes sense to me that it
> >> should error out on non-existing properties. Would that be a reasonable course
> >> to take? Or does anyone prefer to retain the pre-r1592724 behaviour?
> >
> > Didn't we spend time making sure that recursive operations do /not/
> > error out in such cases?
>
> That sounds possible.
>
> Another thought: what is the impact on the XML output mode? That's a more important API, and I haven't reviewed the impact at all.
>
> - Julian
Re: svn commit: r1592724 - in /subversion/trunk/subversion: include/svn_client.h svn/propget-cmd.c tests/cmdline/prop_tests.py tests/cmdline/svnadmin_tests.py
Posted by Julian Foad <ju...@btopenworld.com>.
Branko Čibej wrote:
> On 19.05.2014 13:30, Daniel Shahaf wrote:
>> Because I assumed it would error, realized it didn't, and digging into
>> the history revealed that it was probably an unintentional omission.
[...]
>> I hadn't checked 'svn propget versioned_prop'. It makes sense to me that it
>> should error out on non-existing properties. Would that be a reasonable course
>> to take? Or does anyone prefer to retain the pre-r1592724 behaviour?
>
> Didn't we spend time making sure that recursive operations do /not/
> error out in such cases?
That sounds possible.
Another thought: what is the impact on the XML output mode? That's a more important API, and I haven't reviewed the impact at all.
- Julian
Re: svn commit: r1592724 - in /subversion/trunk/subversion: include/svn_client.h
svn/propget-cmd.c tests/cmdline/prop_tests.py tests/cmdline/svnadmin_tests.py
Posted by Branko Čibej <br...@wandisco.com>.
On 19.05.2014 13:30, Daniel Shahaf wrote:
> 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.
>> Hi Daniel. Can I ask: Why?
>>
> Because I assumed it would error, realized it didn't, and digging into
> the history revealed that it was probably an unintentional omission.
>
>> This makes 'propget --revprop' inconsistent with 'propget [versioned
>> prop]' which still prints nothing. Did you intend to change both
>> forms, for consistency with 'svnlook'?
>>
> I hadn't checked 'svn propget versioned_prop'. It makes
> sense to me that it should error out on non-existing properties.
>
> Would that be a reasonable course to take? Or does anyone prefer to
> retain the pre-r1592724 behaviour?
Didn't we spend time making sure that recursive operations do /not/
error out in such cases?
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1592724 - in /subversion/trunk/subversion:
include/svn_client.h svn/propget-cmd.c tests/cmdline/prop_tests.py
tests/cmdline/svnadmin_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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.
>
> Hi Daniel. Can I ask: Why?
>
Because I assumed it would error, realized it didn't, and digging into
the history revealed that it was probably an unintentional omission.
> This makes 'propget --revprop' inconsistent with 'propget [versioned
> prop]' which still prints nothing. Did you intend to change both
> forms, for consistency with 'svnlook'?
>
I hadn't checked 'svn propget versioned_prop'. It makes
sense to me that it should error out on non-existing properties.
Would that be a reasonable course to take? Or does anyone prefer to
retain the pre-r1592724 behaviour?
Thanks,
Daniel
> 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.
>
> - Julian
>
> >
> > * subversion/svn/propget-cmd.c
> > (svn_cl__propget): Handle the (propval == NULL) case.
> >
> > * subversion/include/svn_client.h
> > (svn_client_revprop_get): Document API behaviour on non-existing properties.
> >
> > * subversion/tests/cmdline/svnadmin_tests.py
> > (empty_date): Use exit code to verify lack of property.
> >
> > * subversion/tests/cmdline/prop_tests.py
> > (revprop_change): Same. 'propget' was actually already failing (with
> > the expected E200017), but the test was ignoring the exit code.
> > (I think the before-this-change code can never fail, since
> > re.match('cha-ching', 'cha-ching\n') would return False.)
Re: svn commit: r1592724 - in /subversion/trunk/subversion:
include/svn_client.h svn/propget-cmd.c tests/cmdline/prop_tests.py
tests/cmdline/svnadmin_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Wed, May 28, 2014 at 14:50:37 +0000:
> 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.
Committed as r1600724.
Re: svn commit: r1592724 - in /subversion/trunk/subversion:
include/svn_client.h svn/propget-cmd.c tests/cmdline/prop_tests.py
tests/cmdline/svnadmin_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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).
Re: svn commit: r1592724 - in /subversion/trunk/subversion: include/svn_client.h svn/propget-cmd.c tests/cmdline/prop_tests.py tests/cmdline/svnadmin_tests.py
Posted by Julian Foad <ju...@btopenworld.com>.
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.
Hi Daniel. Can I ask: Why?
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.
- Julian
>
> * subversion/svn/propget-cmd.c
> (svn_cl__propget): Handle the (propval == NULL) case.
>
> * subversion/include/svn_client.h
> (svn_client_revprop_get): Document API behaviour on non-existing properties.
>
> * subversion/tests/cmdline/svnadmin_tests.py
> (empty_date): Use exit code to verify lack of property.
>
> * subversion/tests/cmdline/prop_tests.py
> (revprop_change): Same. 'propget' was actually already failing (with
> the expected E200017), but the test was ignoring the exit code.
> (I think the before-this-change code can never fail, since
> re.match('cha-ching', 'cha-ching\n') would return False.)