Added --repository-url flag to post-review for generating diff outside a working copy

Review Request #609 — Created Oct. 24, 2008 and submitted


Review Board SVN (deprecated)


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:
./ -n ../style/
./ -n --repository-url=file:///usr/local/svn/ --revision-range=4807:4808
./ -n --repository-url=file:///usr/local/svn/ (fails: no --revision-range parameter supplied)

  1. It would be nice to be able to guess the repository type based on the URL and then fall back to requiring the type.
    I'm not thrilled with getting rid of the ability to do per-SCMClient options, but it seems we're not using that functionality anymore anyways, so it's alright.
    1. I removed the repository-type option, and edited the get_repository_info method of SVNClient so that it checks whether repository_url is set to a valid SVN repository.  If support for the repository-url option ever gets added to GitClient, there won't be an obvious way to specify whether svn or git-svn should be used for creating the diff (assuming both are installed).  Would that be a problem?
    2. We could ignore it in the case of git-svn. I think it's fine. We'll fall off that bridge when we come to it.
  2. /trunk/reviewboard/contrib/tools/post-review (Diff revision 2)
    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.
    1. You're right, that can be a lot more condensed.  I've changed it to add the repository url if supplied, and in either case use all the rest of the same logic.
  2. I just noticed that this line relies on the repository_info object, which isn't available in this method.  My working copy has additional changes that do supply repository_info as a parameter; I'll upload another diff soon.
  1. This is shaping up nicely. A couple small things left and I'd say it's ready.
  2. 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.
  3. /trunk/reviewboard/contrib/tools/post-review (Diff revision 5)
    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.
    1. I agree that it's not great to show irrelevant options, but I can't think of any way around it when theoretically the repository type is not determined until after a parameter is evaluated.  If PerforceClient eventually supports --repository-url, wouldn't that option need to be exposed all the time?  If you can think of a way around that, I'd be happy to work on it (though I'd love to get this code accepted first).
    2. I can't really think of a way off-hand. I'm fine with it going in like this.
  4. Should use parens for multi-line if statements, and also the alignment is currently off.
    Also, space before PerforceClient.
    1. Done, assuming you meant a pair of parens enclosing the entire if statement.
Review request changed
Change Summary:
Added default param to SVN do_diff, and fixed parens/indentation of options.p4_client conditional.
  1. Looks great. Thanks for your hard work on this.
    Committed as r1635.