Added --repository-url flag to post-review for generating diff outside a working copy
Review Request #609 — Created Oct. 24, 2008 and submitted
In review 588 (commit r1544) I added support for running post-review with Subversion outside of a working copy. One downside to that patch was that it required a global setting for REPOSITORY_URL because the SCM Client type was determined before the command line options were read. It also forced the use of Subversion if REPOSITORY_URL were set. This patch moves the option reading code above the SCM Client construction and removes the global REPOSITORY_URL. It adds two flags: --repository-url (URL to a repository) --repository-type (SCM name, e.g. svn, p4, perforce, etc.) If the repository url is supplied, the repository-type and a revision-range must also be supplied. The type is then used to explicitly create an SCM Client. Otherwise, the previously-available for loop is still used to pick an SCM Client based on the results of the repository_info constructor. This change requires that all command line parameters be registered regardless of available SCM client. I have updated the help text of several to make clear that they are only available with certain SCM clients, and also added checks after the options are read that incompatible options have not been set. repository-url is still only used with Subversion, but it should be easier to add code for other SCM types if that's desirable. Is this a good direction for post-review? ---- 10/29/08: In diff 2, I have removed the --repository-type option.
Fairly little. I'll do more testing if there's interest in this change. I have tested it on Redhat with Subversion 1.5.2 with the following commands: ./postreview.py -n ../style/ ./postreview.py -n --repository-url=file:///usr/local/svn/ --revision-range=4807:4808 ./postreview.py -n --repository-url=file:///usr/local/svn/ (fails: no --revision-range parameter supplied)
-
-
Seems we're doing a lot of duplication of code here. It'd be nice to condense this. In particular, have a params list that defaults to [], but has options.repository_url in it, and have the existing execute call append that to the command. Then re-use the existing repository URL search. I'd rather not duplicate any logic in here.
NO
- Change Summary:
-
Whoops, added space back into regex for finding SVN repository root.
- Diff:
-
Revision 4 (+85 -83)
NO
- Change Summary:
-
This diff supplies repository_info to the diff_between_revisions method so that it can set the url to the repository path (fixing a bug in the previous diffs). This diff also sends the (non-option) command line arguments to diff_between_revisions, just as they are sent to diff(). diff_between_revisions will use the first option, if it exists, as a base path so that a review request that spans multiple revisions can be restricted by path/branch. For example: post-review --repository-url=file:///usr/local/svn --revision-range=5140:5143 branches/test will diff only the files in branches/test, even if some files in trunk were changed in rev 5141.
- Diff:
-
Revision 5 (+113 -94)
-
This is shaping up nicely. A couple small things left and I'd say it's ready.
-
I would just default repository_info to None and not pass None in to the do_diff call above. This makes it slightly more self-documenting that this is an optional parameter.
-
I'm actually a little concerned with exposing this. It doesn't make sense for a lot of repository types. It's okay for this change but I think we need to figure out if there's anything we can do to make this more context-aware in the new setup.
-
Should use parens for multi-line if statements, and also the alignment is currently off. Also, space before PerforceClient.
NO
- Change Summary:
-
Added default param to SVN do_diff, and fixed parens/indentation of options.p4_client conditional.
- Diff:
-
Revision 6 (+112 -93)