[PATCH] Issue #2100: --revision ARG1:ARG2 handling improvement

[Ryan Kersh <rp...@cs.uoregon.edu>]

Log message included below and patch attached.

This patch didn't break any of the make check tests.  Should I add a 
test to subversion/tests/libsvn_subr/opt-test.c to put 
svn_opt_parse_revision through the paces as well?

Ryan

[[[
Implement Issue #2100: --revision ARG1:ARG2 handling improvement.

* subversion/include/svn_opt.h
   (svn_opt_parse_revision): Fixed up doc string comment so that it
     reflects the enhancements to --revision ARG1:ARG2 handling.

* subversion/libsvn_subr/opt.c
   (parse_one_rev): Added 'else if' to handle ':' character.
   (svn_opt_parse_revision): Added 'if' statement to handle special
     case where arg is ':'.  Also, reworked some code so that right_rev
     will get parsed correctly by parse_one_rev (now that it has
     changed).
]]]

Re: [PATCH] Make revision range ends optional (was Re: [PATCH] Issue #2100: --revision ARG1:ARG2 handling improvement)

[Julian Foad <ju...@btopenworld.com>]

Ryan Kersh wrote:
> I did some black-box testing, and AFAICT, the only commands that accept 
> ARG1:ARG2 revision arguments are blame, diff, log, and merge.  Of those, 
> only log has a non 0:HEAD default.  The merge command requires the -r 
> option and has no default.

Thank you for investigating.  Those are indeed the only commands that take a 
range.  However, the present defaults (when "-r" is not given at all) are more 
varied than you imply:

   blame
     1:BASE for a local target
     1:HEAD for a URL (close enough to 0:HEAD for this argument)

   diff
     BASE:WC for a local target
     (start is required; end defaults to HEAD) for a URL

   log
     BASE:0 for a local target
     HEAD:0 for a URL

   merge
     (required)

Therefore the commands for which revision-range defaults of 0:HEAD would work 
well are "blame" on a URL and "diff" on a URL.  For the other commands and 
targets, it would be confusing or inappropriate.

> It just seems natural to me that if the revision range ends become 
> optional that we would use 0:HEAD, ascending from left to right, as the 
> default.  If "svn log" defaults to BASE:1 and "svn log -r:" is 0:HEAD, I 
> think it would be okay, because the user would have to be pretty 
> sophisticated to use the -r option in the first place, and would know 
> what they were doing.  I can see where you're coming from, but for me 
> the handiness of it outweighs the possible surprises it might cause. You 
> are probably a better judge of this... do you still think the 0:HEAD 
> default is bad?

The way this feature is presently described in issue 2100 and implemented in 
your patch is not right.

To do this properly, "svn" would need a way of distinguishing between "-r X" 
and "-r X:" so that the former could be treated as a single revision, and the 
latter as a revision range with an unspecified end, whose end is to be 
determined by the specific subcommand, not by the generic argument parser.

I am still not sure that the ability to omit range ends is useful enough to 
justify its existence, but if you would like to implement it in such a way that 
the individual subcommands can interpret it as they see fit, I would reconsider it.

I'm sorry that the issue you chose to work on turns out to be undesigned and 
mostly unreviewed.  (The person who filed the issue appears never to have 
contacted the mailing lists, which is against our policy.)  Please feel free to 
pick a different one unless you are particularly keen on this one.

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Make revision range ends optional

[Ryan Kersh <rp...@cs.uoregon.edu>]

Ryan Kersh wrote:
>> A documentation patch to go with this would be really nice.
> 
> 
> Do you mean the doxygen comment in svn_opt.h.  If so, it's already done. 
>  Otherwise, I'd be happy to provide a documentation patch for anything 
> else if you point me in the right direction.

Oh... I think you're asking for an additional patch for:
http://svn.red-bean.com/svnbook/trunk/src/en/book/ch03.xml
and
http://svn.red-bean.com/svnbook/trunk/src/en/book/ch09.xml

I'll start working on that.  Let me know if I'm off base here.

--Ryan


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

[PATCH] Make revision range ends optional (was Re: [PATCH] Issue #2100: --revision ARG1:ARG2 handling improvement)

[Ryan Kersh <rp...@cs.uoregon.edu>]

Julian Foad wrote:
> I don't recall any discussion of this issue on the mailing list; was 
> there?  My first thoughts are along the lines of: isn't this horribly 
> inconsistent for some commands where the default is not 0:HEAD, e.g. 
> "svn log" which defaults to BASE:1 ?

I wanted to become more acquainted with the code-base so I selected one 
of the bite-sized tasks that looked interesting to me.  I assumed that 
some discussion of this issue had already taken place because it was in 
the issue tracker.  Sorry about that.

More on your second question below...

> Maybe I'm wrong.  I can certainly see that it would be handy in some 
> situations.  List the uses of it in different Subversion commands, 
> comparing to the present defaults, and convince me that there are no 
> ugly contradictions or surprises, only goodness.

I did some black-box testing, and AFAICT, the only commands that accept 
ARG1:ARG2 revision arguments are blame, diff, log, and merge.  Of those, 
only log has a non 0:HEAD default.  The merge command requires the -r 
option and has no default.

It just seems natural to me that if the revision range ends become 
optional that we would use 0:HEAD, ascending from left to right, as the 
default.  If "svn log" defaults to BASE:1 and "svn log -r:" is 0:HEAD, I 
think it would be okay, because the user would have to be pretty 
sophisticated to use the -r option in the first place, and would know 
what they were doing.  I can see where you're coming from, but for me 
the handiness of it outweighs the possible surprises it might cause. 
You are probably a better judge of this... do you still think the 0:HEAD 
default is bad?

>>  Should I add a test to subversion/tests/libsvn_subr/opt-test.c to put 
>> svn_opt_parse_revision through the paces as well?
> 
> 
> That would be very nice, though not essential.

I've included a new test in opt-test.c in the revised patch (attached).

> The description of this issue and this email thread and this patch could 
> be more specific.  Maybe "Make revision range ends optional"?

I like your description.  I've updated the email thread and log message 
accordingly.  I don't think I have the permissions to change the issue 
description.

> A documentation patch to go with this would be really nice.

Do you mean the doxygen comment in svn_opt.h.  If so, it's already done. 
  Otherwise, I'd be happy to provide a documentation patch for anything 
else if you point me in the right direction.

Here is the revised log message:

[[[
Implement Issue #2100: Make revision range ends optional.

* subversion/include/svn_opt.h
   (svn_opt_parse_revision): Fixed up doc string comment so that it
     reflects the enhancements to --revision ARG1:ARG2 handling.

* subversion/libsvn_subr/opt.c
   (parse_one_rev): Added 'else if' to handle ':' character.
   (svn_opt_parse_revision): Added 'if' statement to handle special
     case where arg is ':'.  Also, reworked some code so that right_rev
     will get parsed correctly by parse_one_rev (now that it has
     changed).

* subversion/tests/libsvn_subr/opt-test.c
   (test_parse_revision): New test.
   (svn_test_descriptor_t): Added test_parse_revision to list of tests.
]]]

Re: [PATCH] Issue #2100: --revision ARG1:ARG2 handling improvement

[Julian Foad <ju...@btopenworld.com>]

Ryan Kersh wrote:
> Log message included below and patch attached.

Thanks for volunteering this work, but ...

I don't recall any discussion of this issue on the mailing list; was there?  My 
first thoughts are along the lines of: isn't this horribly inconsistent for 
some commands where the default is not 0:HEAD, e.g. "svn log" which defaults to 
BASE:1 ?

Maybe I'm wrong.  I can certainly see that it would be handy in some 
situations.  List the uses of it in different Subversion commands, comparing to 
the present defaults, and convince me that there are no ugly contradictions or 
surprises, only goodness.


> This patch didn't break any of the make check tests.

Good.  It wouldn't be acceptable if it did.

>  Should I add a 
> test to subversion/tests/libsvn_subr/opt-test.c to put 
> svn_opt_parse_revision through the paces as well?

That would be very nice, though not essential.

> [[[
> Implement Issue #2100: --revision ARG1:ARG2 handling improvement.

The description of this issue and this email thread and this patch could be 
more specific.  Maybe "Make revision range ends optional"?

A documentation patch to go with this would be really nice.

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org