Re: RBTools 0.5.4 questions
We're using the SVN property set at the root of the repository for the same reasons as Alexey. I can also confirm that this approach works perfectly fine even when I have only a subdirectory checked out. On Wednesday, January 15, 2014 12:38:21 AM UTC-5, Christian Hammond wrote: -- Christian Hammond - chi...@chipx86.com javascript: Review Board - http://www.reviewboard.org Beanbag, Inc. - http://www.beanbaginc.com On Tue, Jan 14, 2014 at 7:47 PM, Alexey Neyman alexey...@gmail.comjavascript: wrote: Hi Chris, Thanks for a detailed response. One question on UUID - why can't it be cached in RB's database at the time of repository setup, instead of being queried from Subversion servers for each 'rbt post' as you described? On .reviewboardrc, this approach would work if the checked-out module is known. In our case, for example, we have at least: /trunk/src /trunk/docs (and a few other directories) - If a developer works purely on updating the user documentation, for example, he would just check out /trunk/docs. Which means for 'rbt post' to work, there needs to be /trunk/docs/.reviewboardrc. - Similarly, if a change does not involve anything but sources, he'd only check out /trunk/src - ergo, there must be /trunk/src/.reviewboardrc. - If a change involves both the source code modifications and user doc changes, he'd check out /trunk - so, adding /trunk/.reviewboardrc as well... I think you see where this is getting :) Putting a separate .reviewboardrc into each subdirectory is hardly maintainable, especially if we need to change it at some point. Now, imagine if we migrate the RB to a different server - in addition to updating multiple .reviewboardrc's for /trunk, we'd also need to update the .reviewboardrc in each branch - since many branches are still actively maintained. This is why we're using a property set at the top of the repository. I implore to add a repository name property to avoid the above maintenance nightmare. That makes sense, and is a valid use case for UUID matching. Caching is possible. It didn't used to be when the UUID code was written, but it's worth investigating now. I'll be honest, I don't have a lot of time to do that right now with trying to get RB 2.0 out the door, but a patch wouldn't be too difficult. I'd accept one for caching the UUID if someone wanted to write it. The problem would be if you changed the repository out from underneath Review Board, changing UUIDs. You'd need some way to reset. Maybe not worth optimizing for, but should be considered. Unfortunately, I don't believe the property isn't any more a solution than .reviewboardrc. For the current reviewboard:url property, we're just walking up the local directory stack anyway, meaning a property on / won't work if checking out from some subdirectory. You'd still need a property in every one of those directories, which is only slightly different than a .reviewboardrc in every one of those directories. (The latter at least allows for consistency with the rest of the codebase and other revision control systems.) If we were to fix that to check the property from the root of the remote server (provided / isn't checked out), then there's not much difference between that and grabbing the .reviewboardrc from the root of the remote server. I'll admit I may be missing something, as it's been a while since I've dealt with Subversion or our Subversion code on any real level. Christian -- Get the Review Board Power Pack at http://www.reviewboard.org/powerpack/ --- Sign up for Review Board hosting at RBCommons: https://rbcommons.com/ --- Happy user? Let us know at http://www.reviewboard.org/users/ --- You received this message because you are subscribed to the Google Groups reviewboard group. To unsubscribe from this group and stop receiving emails from it, send an email to reviewboard+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
RBTools 0.5.4 questions
Hi,
I've just upgraded RBTools to 0.5.4, and have a couple of questions:
1. When I tried to invoke post-review (yes, I still have to use that - see
the second question as to why) on a working copy which had some files
renamed, it printed the following message:
One or more files in your changeset has history scheduled with commit.
Please try again with '--svn-show-copies-as-adds=y/n'
However, post-review does not support this option:
$ post-review --svn-show-copies-as-adds=n
The post-review tool is deprecated in favor of therbt suite of
commands. post-review will go away in RBTools 0.6.x.
Usage: post-review [-pond] [-r review_id] [changenum]
post-review: error: no such option: --svn-show-copies-as-adds
2. As to using post-review instead of 'rbt post': in our set up,
post-review works but 'rbt post' doesn't. The reason is that our SVN
repository is configured in RB with file:// protocol, while developers
access it using svn:// protocol. The reasons for this configuration is to
reduce the load on the server (since RB is running on the same machine, why
should it go over the network when it can access repository directly). This
also allows RB to see the repository without a dedicated RB user.
With this set-up, post-review was able to match svn://server/project URL
in the working copy to file:///project in the RB configuration. 'rbt
post' is apparently not so smart when creating a new review request:
DEBUG:root:Error data: {u'stat': u'fail', u'repository':
u'svn://server/project', u'err': {u'msg': u'The repository path specified
is not in the list of known repositories', u'code': 206}}
Strangely, though, updating an existing review request 'rbt post -r XXX'
works.
Can the post-review's repository matching code be ported to 'rbt post'? Or
alternatively, can RB be enhanced so that a repository could be matched by
multiple paths?
BTW, Subversion repositories have a UUID - perhaps, RB could match based on
UUID rather than on comparing the paths?
Regards,
Alexey.
--
Get the Review Board Power Pack at http://www.reviewboard.org/powerpack/
---
Sign up for Review Board hosting at RBCommons: https://rbcommons.com/
---
Happy user? Let us know at http://www.reviewboard.org/users/
---
You received this message because you are subscribed to the Google Groups
reviewboard group.
To unsubscribe from this group and stop receiving emails from it, send an email
to reviewboard+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
Re: RBTools 0.5.4 questions
Alexey,
Responses inline
On Tue, Jan 14, 2014 at 6:14 PM, Alexey Neyman alexey.ney...@gmail.comwrote:
1. When I tried to invoke post-review (yes, I still have to use that - see
the second question as to why) on a working copy which had some files
renamed, it printed the following message:
One or more files in your changeset has history scheduled with commit.
Please try again with '--svn-show-copies-as-adds=y/n'
However, post-review does not support this option:
$ post-review --svn-show-copies-as-adds=n
The post-review tool is deprecated in favor of therbt suite of
commands. post-review will go away in RBTools 0.6.x.
Usage: post-review [-pond] [-r review_id] [changenum]
post-review: error: no such option: --svn-show-copies-as-adds
This is an oversight. A third-party patch introduced the
--svn-show-copies-as-adds in the backend, but didn't include the option in
post-review. We'll get a patch in to fix it for 0.5.5, and I've attached
that patch here if you'd like to try it out.
2. As to using post-review instead of 'rbt post': in our set up,
post-review works but 'rbt post' doesn't. The reason is that our SVN
repository is configured in RB with file:// protocol, while developers
access it using svn:// protocol. The reasons for this configuration is to
reduce the load on the server (since RB is running on the same machine, why
should it go over the network when it can access repository directly). This
also allows RB to see the repository without a dedicated RB user.
We're going to be improving the way that rbt post (and post-review) match
SVN repositories on the server. 0.5.5 will include this change:
https://reviews.reviewboard.org/r/5248/ , which will let you pass in the
repository name through the --repository-url flag (or associated REPOSITORY
config key). For the 0.6 release, we'll be separating this out into a
--repository setting.
-David
--
Get the Review Board Power Pack at http://www.reviewboard.org/powerpack/
---
Sign up for Review Board hosting at RBCommons: https://rbcommons.com/
---
Happy user? Let us know at http://www.reviewboard.org/users/
---
You received this message because you are subscribed to the Google Groups
reviewboard group.
To unsubscribe from this group and stop receiving emails from it, send an email
to reviewboard+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
diff --git a/rbtools/postreview.py b/rbtools/postreview.py
index b75ab80..9a281d9 100755
--- a/rbtools/postreview.py
+++ b/rbtools/postreview.py
@@ -1145,6 +1145,12 @@ def parse_options(args):
default=get_config_value(configs, 'P4_PASSWD'),
help=the Perforce password or ticket of the user
in the P4USER environment variable)
+parser.add_option('--svn-show-copies-as-adds',
+ dest='svn_show_copies_as_adds',
+ metavar='y/n',
+ default=None,
+ help=don't show copied or moved files with their
+ source)
parser.add_option('--svn-changelist', dest='svn_changelist', default=None,
help='generate the diff for review based on a local SVN '
'changelist')
Re: RBTools 0.5.4 questions
Hi Alexey, On Tue, Jan 14, 2014 at 7:00 PM, Alexey Neyman alexey.ney...@gmail.comwrote: Hi David, On Tuesday, January 14, 2014 6:27:32 PM UTC-8, David Trowbridge wrote: Alexey, Responses inline On Tue, Jan 14, 2014 at 6:14 PM, Alexey Neyman alexey...@gmail.comwrote: 1. When I tried to invoke post-review (yes, I still have to use that - see the second question as to why) on a working copy which had some files renamed, it printed the following message: One or more files in your changeset has history scheduled with commit. Please try again with '--svn-show-copies-as-adds=y/n' However, post-review does not support this option: $ post-review --svn-show-copies-as-adds=n The post-review tool is deprecated in favor of therbt suite of commands. post-review will go away in RBTools 0.6.x. Usage: post-review [-pond] [-r review_id] [changenum] post-review: error: no such option: --svn-show-copies-as-adds This is an oversight. A third-party patch introduced the --svn-show-copies-as-adds in the backend, but didn't include the option in post-review. We'll get a patch in to fix it for 0.5.5, and I've attached that patch here if you'd like to try it out. Thanks, it works. We just rolled out 0.5.5, which should have this plus a Subversion repository fix. 2. As to using post-review instead of 'rbt post': in our set up, post-review works but 'rbt post' doesn't. The reason is that our SVN repository is configured in RB with file:// protocol, while developers access it using svn:// protocol. The reasons for this configuration is to reduce the load on the server (since RB is running on the same machine, why should it go over the network when it can access repository directly). This also allows RB to see the repository without a dedicated RB user. We're going to be improving the way that rbt post (and post-review) match SVN repositories on the server. 0.5.5 will include this change: https://reviews.reviewboard.org/r/5248/ , which will let you pass in the repository name through the --repository-url flag (or associated REPOSITORY config key). For the 0.6 release, we'll be separating this out into a --repository setting. Does this mean that every post-review invocation would have to be performed with this --repository-url option? Not very convenient, IMO. I think it would be much easier if RB - compared SVN repository UUIDs - these are guaranteed to be the same, regardless of how the repository is accessed OR - allowed to store this repository URL in the repository itself, same as it does with the URL to the ReviewBoard (this way, rbt can obtain the property from by running 'svn propget') OR - allowed to configure repositories with also-can-be-accessed-as list of URLs We're trying to get people to standardize on a .reviewboardrc file in the repository containing the configuration. That's been the recommended setup for years. It's easier to support and is guaranteed to work better and faster. The support for storing in properties is, frankly, something I wish I didn't add back in the day. Maybe I could be convinced to add an equivalent property for the repository name, but I'm on the fence about that. Most of the code for UUIDs is still there, but it looks like it just didn't get ported to rbt post. We can probably add that, but I have to warn you that UUIDs are always going to be significantly slower than having a .reviewboardrc file with a repository name. Review Board has to, with every post, scan the UUIDs of all the repositories it knows about until it finds, and this doesn't scale well. For large installs, this can even timeout the request, particularly when Subversion servers are being slow to respond. An Also-can-be-accesed-list was considered at one point, but honestly, we're unlikely to ever do it. It goes down a path that's detrimental. A list isn't enough for these sort of matches. Some repositories require a username in the URL, so we'd have to support pattern matching. That means we'd then have to fetch all repositories in the database and pattern match across them all, which won't be efficient enough for large installs that have hundreds of repos. By including a .reviewboardrc and setting REPOSITORY = name (which should now work as of RBTools 0.5.5), you'll get the fastest post speed and better future compatibility. Christian -- Get the Review Board Power Pack at http://www.reviewboard.org/powerpack/ --- Sign up for Review Board hosting at RBCommons: https://rbcommons.com/ --- Happy user? Let us know at http://www.reviewboard.org/users/ --- You received this message because you are subscribed to the Google Groups reviewboard group. To unsubscribe from this group and stop receiving emails from it, send an email to reviewboard+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
Re: RBTools 0.5.4 questions
-- Christian Hammond - chip...@chipx86.com Review Board - http://www.reviewboard.org Beanbag, Inc. - http://www.beanbaginc.com On Tue, Jan 14, 2014 at 7:47 PM, Alexey Neyman alexey.ney...@gmail.comwrote: Hi Chris, Thanks for a detailed response. One question on UUID - why can't it be cached in RB's database at the time of repository setup, instead of being queried from Subversion servers for each 'rbt post' as you described? On .reviewboardrc, this approach would work if the checked-out module is known. In our case, for example, we have at least: /trunk/src /trunk/docs (and a few other directories) - If a developer works purely on updating the user documentation, for example, he would just check out /trunk/docs. Which means for 'rbt post' to work, there needs to be /trunk/docs/.reviewboardrc. - Similarly, if a change does not involve anything but sources, he'd only check out /trunk/src - ergo, there must be /trunk/src/.reviewboardrc. - If a change involves both the source code modifications and user doc changes, he'd check out /trunk - so, adding /trunk/.reviewboardrc as well... I think you see where this is getting :) Putting a separate .reviewboardrc into each subdirectory is hardly maintainable, especially if we need to change it at some point. Now, imagine if we migrate the RB to a different server - in addition to updating multiple .reviewboardrc's for /trunk, we'd also need to update the .reviewboardrc in each branch - since many branches are still actively maintained. This is why we're using a property set at the top of the repository. I implore to add a repository name property to avoid the above maintenance nightmare. That makes sense, and is a valid use case for UUID matching. Caching is possible. It didn't used to be when the UUID code was written, but it's worth investigating now. I'll be honest, I don't have a lot of time to do that right now with trying to get RB 2.0 out the door, but a patch wouldn't be too difficult. I'd accept one for caching the UUID if someone wanted to write it. The problem would be if you changed the repository out from underneath Review Board, changing UUIDs. You'd need some way to reset. Maybe not worth optimizing for, but should be considered. Unfortunately, I don't believe the property isn't any more a solution than .reviewboardrc. For the current reviewboard:url property, we're just walking up the local directory stack anyway, meaning a property on / won't work if checking out from some subdirectory. You'd still need a property in every one of those directories, which is only slightly different than a .reviewboardrc in every one of those directories. (The latter at least allows for consistency with the rest of the codebase and other revision control systems.) If we were to fix that to check the property from the root of the remote server (provided / isn't checked out), then there's not much difference between that and grabbing the .reviewboardrc from the root of the remote server. I'll admit I may be missing something, as it's been a while since I've dealt with Subversion or our Subversion code on any real level. Christian -- Get the Review Board Power Pack at http://www.reviewboard.org/powerpack/ --- Sign up for Review Board hosting at RBCommons: https://rbcommons.com/ --- Happy user? Let us know at http://www.reviewboard.org/users/ --- You received this message because you are subscribed to the Google Groups reviewboard group. To unsubscribe from this group and stop receiving emails from it, send an email to reviewboard+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/groups/opt_out.
