Re: post-review uses incorrect svn diff option with single rev
Hi Christian,
Many thanks. I've posted the bug (2256) but while testing a solution,
I notice other strange behaviour with svn diff and single revision
range, but this time with --repository-url option. I don't know the
use case so not sure if it is a bug or not:
$ post-review --repository-url=http://svn/repo --revision-range=157778
--debug
>>> RBTools 0.3.3
>>> Home =
>>> svn info http://svn/repo
>>> diff --version
>>> repository info: Path: http://svn/repo, Base path: /, Supports changesets:
>>> False
>>> HTTP GETting api/
>>> HTTP GETting http:///reviews/api/info/
>>> Using the new web API
>>> svn diff --diff-cmd=diff http://svn/repo/@157778 http://svn/repo/@HEAD
which returns diff between 157778 and HEAD of the repository. A clear
decision is made in the code for this:
if options.repository_url:
revisions = revision_range.split(':')
if len(revisions) < 1:
return None
elif len(revisions) == 1:
revisions.append('HEAD')
but I don't know why it does this. I do not understand how fetching a
diff between a rev and HEAD could be useful for code review.
In any case, it is not consistent ("If you only need to post a single
revision, you can type: $ post-review --revision-range=REVISION"), and
it could/should probably be explicit if required, e.g. --revision-
range=157778:HEAD
I suggest the executed command for a single --revision-range option
with --repository-url should be:
$ post-review --repository-url=http://svn/repo --revision-range=REV --
debug
...
>>> svn diff --diff-cmd=diff http://svn/repo/@REV-1 http://svn/repo/@REV
or
>>> svn diff --diff-cmd=diff -c REV http://svn/repo
If you wish me to raise a bug and suggest a solution for this also, I
will do so.
The rest of the svn diff implementation seems fine to me.
--
Want to help the Review Board project? Donate today at
http://www.reviewboard.org/donate/
Happy user? Let us know at http://www.reviewboard.org/users/
-~--~~~~--~~--~--~---
To unsubscribe from this group, send email to
reviewboard+unsubscr...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/reviewboard?hl=en
Re: post-review uses incorrect svn diff option with single rev
Hi Seamus,
Sorry for the quarantine. We get a lot of spam attempts.
Mind filing a bug and posting a review request for this change on
http://reviews.reviewboard.org?
Christian
On Friday, August 19, 2011, Seamus Linden wrote:
> My first post seems to be in quarantine, so can't respond to it. Anyway,
here is a simple code change to fix the problem I reported, so there's no
real need to post either message. This seems to work fine.
>
> /usr/local/lib/python2.6/dist-packages/RBTools-0.3.3-py2.6.egg/rbtools$
diff postreview.py.old postreview.py
> 1712c1712,1718
> < return (self.do_diff(["svn", "diff", "--diff-cmd=diff",
"-r",
> ---
>> revisions = revision_range.split(':')
>> if len(revisions) == 1:
>> return (self.do_diff(["svn", "diff", "--diff-cmd=diff",
"-c",
>> revision_range],
>> repository_info), None)
>> else:
>> return (self.do_diff(["svn", "diff", "--diff-cmd=diff",
"-r",
>
>
> On Thu, Aug 18, 2011 at 5:31 PM, shay wrote:
>>
>> Trialling this software, using v1.6 RC2.
>>
>> Doing a "post-commit" review using post-review with SVN repository, I
>> notice the wrong svn diff option is used when specifying a single SVN
>> revision (rather than a range of revisions).
>>
>> The documentation states "If you only need to post a single revision,
>> you can type:
>> $ post-review --revision-range=REVISION"
>> but this doesn't work properly.
>>
>> Debug output:
>>
>> $ post-review --revision-range=157778 --debug
>> >>> RBTools 0.3.3
>> >>> Home = /home/
>> >>> svn info
>> >>> diff --version
>> >>> repository info: Path: http:///repo, Base path: //trunk,
Supports changesets: False
>> >>> HTTP GETting api/
>> >>> HTTP GETting http:///reviews/api/info/
>> >>> Using the new web API
>> >>> svn diff --diff-cmd=diff -r 157778
>> ^CTraceback (most recent call last):
>>
>> KeyboardInterrupt
>>
>> The -r option with a single arg is equivalent to 0:157778, and begins
>> fetching EVERY change made on the current path, up to rev 157778 in
>> this example. In my case, this was thousands of revs...
>>
>> The svn diff command should be:
>> svn diff --diff-cmd=diff -c 157778
>>
>> i.e. it should use -c option.
>>
>> The -r option works when a range is specified with 2 args, so post-
>> review could convert a single arg to a range using 2 args as in svn
>> diff -r (rev-1):rev
>>
>> so the following would do the same job as -c (and might be easier to
>> code):
>> svn diff --diff-cmd=diff -r 15:157778
>>
>> Specifying a revision range with 2 args works correctly, e.g.:
>>
>> $ post-review --revision-range=15:157778 --debug
>> >>> RBTools 0.3.3
>> >>> Home = /home/
>> >>> svn info
>> >>> diff --version
>> >>> repository info: Path: http:///repo, Base path: //trunk,
Supports changesets: False
>> >>> HTTP GETting api/
>> >>> HTTP GETting http:///reviews/api/info/
>> >>> Using the new web API
>> >>> svn diff --diff-cmd=diff -r 15:157778
>> ^CTraceback (most recent call last):
>>
>> KeyboardInterrupt
>>
>
> --
> Want to help the Review Board project? Donate today at
http://www.reviewboard.org/donate/
> Happy user? Let us know at http://www.reviewboard.org/users/
> -~--~~~~--~~--~--~---
> To unsubscribe from this group, send email to
reviewboard+unsubscr...@googlegroups.com <
reviewboard%2bunsubscr...@googlegroups.com>
> For more options, visit this group at
http://groups.google.com/group/reviewboard?hl=en
--
--
Christian Hammond - chip...@chipx86.com
Review Board - http://www.reviewboard.org
VMware, Inc. - http://www.vmware.com
--
Want to help the Review Board project? Donate today at
http://www.reviewboard.org/donate/
Happy user? Let us know at http://www.reviewboard.org/users/
-~--~~~~--~~--~--~---
To unsubscribe from this group, send email to
reviewboard+unsubscr...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/reviewboard?hl=en
Re: post-review uses incorrect svn diff option with single rev
My first post seems to be in quarantine, so can't respond to it. Anyway,
here is a simple code change to fix the problem I reported, so there's no
real need to post either message. This seems to work fine.
/usr/local/lib/python2.6/dist-packages/RBTools-0.3.3-py2.6.egg/rbtools$ diff
postreview.py.old postreview.py
1712c1712,1718
< return (self.do_diff(["svn", "diff", "--diff-cmd=diff", "-r",
---
> revisions = revision_range.split(':')
> if len(revisions) == 1:
> return (self.do_diff(["svn", "diff", "--diff-cmd=diff",
"-c",
> revision_range],
> repository_info), None)
> else:
> return (self.do_diff(["svn", "diff", "--diff-cmd=diff",
"-r",
On Thu, Aug 18, 2011 at 5:31 PM, shay wrote:
> Trialling this software, using v1.6 RC2.
>
> Doing a "post-commit" review using post-review with SVN repository, I
> notice the wrong svn diff option is used when specifying a single SVN
> revision (rather than a range of revisions).
>
> The documentation states "If you only need to post a single revision,
> you can type:
> $ post-review --revision-range=REVISION"
> but this doesn't work properly.
>
> Debug output:
>
> $ post-review --revision-range=157778 --debug
> >>> RBTools 0.3.3
> >>> Home = /home/
> >>> svn info
> >>> diff --version
> >>> repository info: Path: http:///repo, Base path: //trunk,
> Supports changesets: False
> >>> HTTP GETting api/
> >>> HTTP GETting http:///reviews/api/info/
> >>> Using the new web API
> >>> svn diff --diff-cmd=diff -r 157778
> ^CTraceback (most recent call last):
>
> KeyboardInterrupt
>
> The -r option with a single arg is equivalent to 0:157778, and begins
> fetching EVERY change made on the current path, up to rev 157778 in
> this example. In my case, this was thousands of revs...
>
> The svn diff command should be:
> svn diff --diff-cmd=diff -c 157778
>
> i.e. it should use -c option.
>
> The -r option works when a range is specified with 2 args, so post-
> review could convert a single arg to a range using 2 args as in svn
> diff -r (rev-1):rev
>
> so the following would do the same job as -c (and might be easier to
> code):
> svn diff --diff-cmd=diff -r 15:157778
>
> Specifying a revision range with 2 args works correctly, e.g.:
>
> $ post-review --revision-range=15:157778 --debug
> >>> RBTools 0.3.3
> >>> Home = /home/
> >>> svn info
> >>> diff --version
> >>> repository info: Path: http:///repo, Base path: //trunk,
> Supports changesets: False
> >>> HTTP GETting api/
> >>> HTTP GETting http:///reviews/api/info/
> >>> Using the new web API
> >>> svn diff --diff-cmd=diff -r 15:157778
> ^CTraceback (most recent call last):
>
> KeyboardInterrupt
>
>
--
Want to help the Review Board project? Donate today at
http://www.reviewboard.org/donate/
Happy user? Let us know at http://www.reviewboard.org/users/
-~--~~~~--~~--~--~---
To unsubscribe from this group, send email to
reviewboard+unsubscr...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/reviewboard?hl=en
