Hi Tim,

Thanks for plugging your way through this!

A few tiny comments below.

I look forward to when we can collapse the CVS stuff into a URL
somehow or other as well, but this is a good start :-)

Cheers,
mwh

> === modified file 'lib/lp/code/browser/codeimport.py'
> --- lib/lp/code/browser/codeimport.py	2009-12-08 02:32:03 +0000
> +++ lib/lp/code/browser/codeimport.py	2010-01-11 23:55:03 +0000
> @@ -15,7 +15,6 @@
>      'CodeImportView',
>      ]
>
> -from cgi import escape
>
>  from BeautifulSoup import BeautifulSoup
>  from zope.app.form import CustomWidgetFactory
> @@ -24,16 +23,16 @@
>  from zope.component import getUtility
>  from zope.formlib import form
>  from zope.interface import Interface
> -from zope.schema import Choice, TextLine
> +from zope.schema import Choice
>
>  from canonical.cachedproperty import cachedproperty
>  from canonical.launchpad import _
> +from canonical.launchpad.fields import URIField
>  from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
>  from lp.code.enums import (
>      BranchSubscriptionDiffSize, BranchSubscriptionNotificationLevel,
>      CodeImportReviewStatus, CodeReviewNotificationLevel,
>      RevisionControlSystems)
> -from lp.code.interfaces.branch import branch_name_validator
>  from lp.code.interfaces.branchnamespace import (
>      get_branch_namespace, IBranchNamespacePolicy)
>  from lp.code.interfaces.codeimport import (
> @@ -128,8 +127,7 @@
>
>      custom_widget('cvs_root', StrippedTextWidget, displayWidth=50)
>      custom_widget('cvs_module', StrippedTextWidget, displayWidth=20)
> -    custom_widget('svn_branch_url', URIWidget, displayWidth=50)
> -    custom_widget('git_repo_url', URIWidget, displayWidth=50)
> +    custom_widget('url', URIWidget, displayWidth=50)
>
>      @cachedproperty
>      def _super_user(self):
> @@ -173,56 +171,73 @@
>                      canonical_url(code_import.branch),
>                      code_import.branch.unique_name))
>
> -    def _validateSVN(self, svn_branch_url, existing_import=None):
> -        """If the user has specified a subversion url, we need
> -        to make sure that there isn't already an import with
> -        that url."""
> -        if svn_branch_url is None:
> +    def _validateURL(self, url, existing_import=None, field_name='url'):
> +        """If the user has specified a url, we need to make sure that there
> +        isn't already an import with that url."""
> +        if url is None:
>              self.setSecondaryFieldError(
> -                'svn_branch_url', 'Enter the URL of a Subversion branch.')
> +                field_name, 'Enter the URL of a foreign VCS branch.')
>          else:
> -            code_import = getUtility(ICodeImportSet).getBySVNDetails(
> -                svn_branch_url)
> +            code_import = getUtility(ICodeImportSet).getByURL(url)
>              if (code_import is not None and
>                  code_import != existing_import):
>                  self.setFieldError(
> -                    'svn_branch_url',
> +                    field_name,
>                      structured("""
> -                    This Subversion branch URL is already specified for
> +                    This foreign branch URL is already specified for
>                      the imported branch <a href="%s">%s</a>.""",
>                      canonical_url(code_import.branch),
>                      code_import.branch.unique_name))
>
> -    def _validateGit(self, git_repo_url, existing_import=None):
> -        """If the user has specified a git repo url, we need
> -        to make sure that there isn't already an import with
> -        that url."""
> -        if git_repo_url is None:
> -            self.setSecondaryFieldError(
> -                'git_repo_url', 'Enter the URL of a Git repo.')
> -        else:
> -            code_import = getUtility(ICodeImportSet).getByGitDetails(
> -                git_repo_url)
> -            if (code_import is not None and
> -                code_import != existing_import):
> -                self.setFieldError(
> -                    'git_repo_url',
> -                    structured("""
> -                    This Git repository URL is already specified for
> -                    the imported branch <a href="%s">%s</a>.""",
> -                    escape(canonical_url(code_import.branch)),
> -                    escape(code_import.branch.unique_name)))
> +
> +
> +class NewCodeImportForm(Interface):
> +    """The fields presented on the form for editing a code import."""
> +
> +    use_template(
> +        ICodeImport,
> +        ['product', 'rcs_type', 'cvs_root', 'cvs_module'])
> +
> +    svn_branch_url = URIField(
> +        title=_("Branch URL"), required=False,
> +        description=_(
> +            "The URL of a Subversion branch, starting with svn:// or"
> +            " http(s)://. Only trunk branches are imported."),
> +        allowed_schemes=["http", "https", "svn"],
> +        allow_userinfo=False,
> +        allow_port=True,
> +        allow_query=False,
> +        allow_fragment=False,
> +        trailing_slash=False)
> +
> +    git_repo_url = URIField(
> +        title=_("Repo URL"), required=False,
> +        description=_(
> +            "The URL of the git repository.  The MASTER branch will be "
> +            "imported."),
> +        allowed_schemes=["git"],
> +        allow_userinfo=False, # Only anonymous access is supported.
> +        allow_port=True,
> +        allow_query=False,    # Query makes no sense in Subversion.

It's not your fault at all but "Query makes no sense in Subversion."
makes no sense here.

> +        allow_fragment=False,
> +        trailing_slash=False)
> +
> +    branch_name = copy_field(
> +        IBranch['name'],
> +        __name__='branch_name',
> +        title=_('Branch Name'),
> +        description=_(
> +            "This will be used in the branch URL to identify the "
> +            "imported branch.  Examples: main, trunk."),
> +        )
>
>
>  class CodeImportNewView(CodeImportBaseView):
>      """The view to request a new code import."""
>
> +    schema = NewCodeImportForm
>      for_input = True
>      label = 'Request a code import'
> -    field_names = [
> -        'product', 'rcs_type', 'svn_branch_url', 'cvs_root', 'cvs_module',
> -        'git_repo_url',
> -        ]
>
>      custom_widget('rcs_type', LaunchpadRadioWidget)
>
> @@ -236,19 +251,6 @@
>          """Cancel should take the user back to the root site."""
>          return '/'
>
> -    def setUpFields(self):
> -        CodeImportBaseView.setUpFields(self)
> -        # Add in the field for the branch name.
> -        name_field = form.Fields(
> -            TextLine(
> -                __name__='branch_name',
> -                title=_('Branch Name'), required=True, description=_(
> -                    "This will be used in the branch URL to identify the "
> -                    "imported branch.  Examples: main, trunk."),
> -                constraint=branch_name_validator),
> -            render_context=self.render_context)
> -        self.form_fields = self.form_fields + name_field
> -
>      def setUpWidgets(self):
>          CodeImportBaseView.setUpWidgets(self)
>
> @@ -268,18 +270,31 @@
>          self.rcs_type_git = str(git_button)
>          self.rcs_type_emptymarker = str(empty_marker)
>
> +    def _getImportLocation(self, data):
> +        """Return the import location based on type."""
> +        rcs_type = data['rcs_type']
> +        if rcs_type == RevisionControlSystems.CVS:
> +            return data.get('cvs_root'), data.get('cvs_module'), None
> +        elif rcs_type == RevisionControlSystems.BZR_SVN:
> +            return None, None, data.get('svn_branch_url')
> +        elif rcs_type == RevisionControlSystems.GIT:
> +            return None, None, data.get('git_repo_url')
> +        else:
> +            raise AssertionError(
> +                'Unexpected revision control type %r.' % rcs_type)
> +
>      def _create_import(self, data, status):
>          """Create the code import."""
> +        cvs_root, cvs_module, url = self._getImportLocation(data)
>          return getUtility(ICodeImportSet).new(
>              registrant=self.user,
>              product=data['product'],
>              branch_name=data['branch_name'],
>              rcs_type=data['rcs_type'],
> -            svn_branch_url=data['svn_branch_url'],
> -            cvs_root=data['cvs_root'],
> -            cvs_module=data['cvs_module'],
> -            review_status=status,
> -            git_repo_url=data['git_repo_url'])
> +            url=url,
> +            cvs_root=cvs_root,
> +            cvs_module=cvs_module,
> +            review_status=status)
>
>      def _setBranchExists(self, existing_branch):
>          """Set a field error indicating that the branch already exists."""
> @@ -357,19 +372,13 @@
>          # Make sure fields for unselected revision control systems
>          # are blanked out:
>          if rcs_type == RevisionControlSystems.CVS:
> -            data['svn_branch_url'] = None
> -            data['git_repo_url'] = None
>              self._validateCVS(data.get('cvs_root'), data.get('cvs_module'))
>          elif rcs_type == RevisionControlSystems.BZR_SVN:
> -            data['cvs_root'] = None
> -            data['cvs_module'] = None
> -            data['git_repo_url'] = None
> -            self._validateSVN(data.get('svn_branch_url'))
> +            self._validateURL(
> +                data.get('svn_branch_url'), field_name='svn_branch_url')
>          elif rcs_type == RevisionControlSystems.GIT:
> -            data['cvs_root'] = None
> -            data['cvs_module'] = None
> -            data['svn_branch_url'] = None
> -            self._validateGit(data.get('git_repo_url'))
> +            self._validateURL(
> +                data.get('git_repo_url'), field_name='git_repo_url')
>          else:
>              raise AssertionError(
>                  'Unexpected revision control type %r.' % rcs_type)
> @@ -380,7 +389,7 @@
>
>      use_template(
>          ICodeImport,
> -        ['svn_branch_url', 'cvs_root', 'cvs_module', 'git_repo_url'])
> +        ['url', 'cvs_root', 'cvs_module'])
>      whiteboard = copy_field(IBranch['whiteboard'])
>
>
> @@ -457,15 +466,12 @@
>          # If the import is a Subversion import, then omit the CVS
>          # fields, and vice versa.
>          if self.code_import.rcs_type == RevisionControlSystems.CVS:
> -            self.form_fields = self.form_fields.omit(
> -                'svn_branch_url', 'git_repo_url')
> +            self.form_fields = self.form_fields.omit('url')
>          elif self.code_import.rcs_type in (RevisionControlSystems.SVN,
> -                                           RevisionControlSystems.BZR_SVN):
> -            self.form_fields = self.form_fields.omit(
> -                'cvs_root', 'cvs_module', 'git_repo_url')
> -        elif self.code_import.rcs_type == RevisionControlSystems.GIT:
> -            self.form_fields = self.form_fields.omit(
> -                'cvs_root', 'cvs_module', 'svn_branch_url')
> +                                           RevisionControlSystems.BZR_SVN,
> +                                           RevisionControlSystems.GIT):
> +            self.form_fields = self.form_fields.omit(
> +                'cvs_root', 'cvs_module')
>          else:
>              raise AssertionError('Unknown rcs_type for code import.')
>
> @@ -496,12 +502,9 @@
>                  data.get('cvs_root'), data.get('cvs_module'),
>                  self.code_import)
>          elif self.code_import.rcs_type in (RevisionControlSystems.SVN,
> -                                           RevisionControlSystems.BZR_SVN):
> -            self._validateSVN(
> -                data.get('svn_branch_url'), self.code_import)
> -        elif self.code_import.rcs_type == RevisionControlSystems.GIT:
> -            self._validateGit(
> -                data.get('git_repo_url'), self.code_import)
> +                                           RevisionControlSystems.BZR_SVN,
> +                                           RevisionControlSystems.GIT):
> +            self._validateURL(data.get('url'), self.code_import)
>          else:
>              raise AssertionError('Unknown rcs_type for code import.')
>

> === modified file 'lib/lp/code/doc/codeimport-event.txt'
> --- lib/lp/code/doc/codeimport-event.txt	2009-07-01 13:16:44 +0000
> +++ lib/lp/code/doc/codeimport-event.txt	2010-01-11 23:55:03 +0000
> @@ -108,9 +108,8 @@
>  First we create a Subversion import.
>
>      >>> from lp.code.enums import RevisionControlSystems
> -    >>> svn_url = 'svn://svn.example.com/trunk'
> -    >>> svn_import = new_code_import('svn-trunk',
> -    ...     rcs_type=RevisionControlSystems.SVN, svn_branch_url=svn_url)

You can delete new_code_import entirely now.

> +    >>> svn_import = factory.makeCodeImport(
> +    ...     svn_branch_url='svn://svn.example.com/trunk')
>
>  CodeImportSet.newCreate creates an event from the new CodeImport object
>  and the person that created it. Here, the creator is the nopriv user.
> @@ -131,11 +130,11 @@
>
>      >>> print_items(svn_create_event)
>      CODE_IMPORT <muted>
> -    OWNER u'52'
> +    OWNER ...
>      REVIEW_STATUS u'NEW'
>      ASSIGNEE None
>      UPDATE_INTERVAL None
> -    SVN_BRANCH_URL u'svn://svn.example.com/trunk'
> +    URL u'svn://svn.example.com/trunk'
>
>  The database IDs of the CodeImport is also recorded. It is useful to
>  collate events associated with deleted CodeImport objects.
> @@ -149,14 +148,13 @@
>  import source. For a CVS import, CVS details are recorded instead of the
>  Subversion URL.
>
> -    >>> cvs_import = new_code_import('cvs-main',
> -    ...     rcs_type=RevisionControlSystems.CVS,
> +    >>> cvs_import = factory.makeCodeImport(
>      ...     cvs_root=':pserver:<email address hidden>:/cvsroot',
>      ...     cvs_module='hello')
>      >>> cvs_create_event = event_set.newCreate(cvs_import, nopriv)
>      >>> print_items(cvs_create_event)
>      CODE_IMPORT <muted>
> -    OWNER u'52'
> +    OWNER ...
>      REVIEW_STATUS u'NEW'
>      ASSIGNEE None
>      UPDATE_INTERVAL None
> @@ -165,17 +163,17 @@
>
>  And for a Git import, the git details are recorded.
>
> -    >>> git_import = new_code_import('git-main',
> -    ...     rcs_type=RevisionControlSystems.GIT,
> +    >>> git_import = factory.makeCodeImport(
>      ...     git_repo_url='git://git.example.org/main.git')
>      >>> git_create_event = event_set.newCreate(git_import, nopriv)
>      >>> print_items(git_create_event)
>      CODE_IMPORT <muted>
> -    OWNER u'52'
> -    REVIEW_STATUS u'NEW'
> +    OWNER ...
> +    REVIEW_STATUS u'REVIEWED'
>      ASSIGNEE None
>      UPDATE_INTERVAL None
> -    GIT_REPO_URL u'git://git.example.org/main.git'
> +    URL u'git://git.example.org/main.git'
> +
>
>  == MODIFY ==
>
> @@ -204,7 +202,8 @@
>  Then changes can be applied.
>
>      >>> from lp.code.enums import CodeImportReviewStatus
> -    >>> svn_import.review_status = CodeImportReviewStatus.REVIEWED
> +    >>> removeSecurityProxy(svn_import).review_status = (
> +    ...     CodeImportReviewStatus.REVIEWED)
>
>  After applying changes, the newModify method can create an event that
>  details the changes that have been applied.
> @@ -229,12 +228,12 @@
>
>      >>> print_items(modify_event)
>      CODE_IMPORT <muted>
> -    OWNER u'52'
> +    OWNER ...
>      REVIEW_STATUS u'REVIEWED'
>      OLD_REVIEW_STATUS u'NEW'
>      ASSIGNEE None
>      UPDATE_INTERVAL None
> -    SVN_BRANCH_URL u'svn://svn.example.com/trunk'
> +    URL u'svn://svn.example.com/trunk'
>
>  If no change of interest is found, no event is created.
>
> @@ -245,55 +244,6 @@
>      >>> len(list(event_set.getAll())) == old_event_set_len
>      True
>
> -In most events, only the source details for the selected version control
> -system are recorded. If the rcs_type changes, we record old and new
> -values for all changed attributes to explicitly represent transitions to
> -None and from None.
> -
> -    >>> token = event_set.beginModify(cvs_import)
> -    >>> cvs_import.rcs_type = RevisionControlSystems.SVN
> -    >>> cvs_import.svn_branch_url = u'svn://svn.example.com/from-cvs'
> -    >>> cvs_import.cvs_root = None
> -    >>> cvs_import.cvs_module = None
> -    >>> modify_event = event_set.newModify(cvs_import, nopriv, token)
> -    >>> print_items(modify_event)
> -    CODE_IMPORT <muted>
> -    OWNER u'52'
> -    REVIEW_STATUS u'NEW'
> -    ASSIGNEE None
> -    UPDATE_INTERVAL None
> -    CVS_ROOT None
> -    CVS_MODULE None
> -    OLD_CVS_ROOT u':pserver:<email address hidden>:/cvsroot'
> -    OLD_CVS_MODULE u'hello'
> -    SVN_BRANCH_URL u'svn://svn.example.com/from-cvs'
> -    OLD_SVN_BRANCH_URL None
> -
> -Aside from source details changes, MODIFY events can record changes to
> -the owner, the review_status, the assignee, and the update_interval of a
> -code import.
> -
> -    >>> from datetime import timedelta
> -    >>> sample_owner = getUtility(IPersonSet).getByName('name12')
> -    >>> sample_assignee = getUtility(IPersonSet).getByName('ddaa')
> -    >>> token = event_set.beginModify(svn_import)
> -    >>> svn_import.owner = sample_owner
> -    >>> svn_import.review_status = CodeImportReviewStatus.SUSPENDED
> -    >>> svn_import.assignee = sample_assignee
> -    >>> svn_import.update_interval = timedelta(hours=1)
> -    >>> modify_event = event_set.newModify(svn_import, nopriv, token)
> -    >>> print_items(modify_event)
> -    CODE_IMPORT <muted>
> -    OWNER u'12'
> -    OLD_OWNER u'52'
> -    REVIEW_STATUS u'SUSPENDED'
> -    OLD_REVIEW_STATUS u'REVIEWED'
> -    ASSIGNEE u'23'
> -    OLD_ASSIGNEE None
> -    UPDATE_INTERVAL u'1:00:00'
> -    OLD_UPDATE_INTERVAL None
> -    SVN_BRANCH_URL u'svn://svn.example.com/trunk'
> -
>
>  === REQUEST ===
>

> === modified file 'lib/lp/codehosting/codeimport/worker.py'
> --- lib/lp/codehosting/codeimport/worker.py	2009-12-22 23:50:27 +0000
> +++ lib/lp/codehosting/codeimport/worker.py	2010-01-11 23:55:03 +0000
> @@ -107,80 +107,62 @@
>      :ivar branch_id: The id of the branch associated to this code import, used
>          for locating the existing import and the foreign tree.
>      :ivar rcstype: 'svn' or 'cvs' as appropriate.
> -    :ivar svn_branch_url: The branch URL if rcstype in ['svn', 'bzr-svn'],
> -        None otherwise.
> +    :ivar url: The branch URL if rcstype in ['svn', 'bzr-svn',
> +        'git'], None otherwise.
>      :ivar cvs_root: The $CVSROOT if rcstype == 'cvs', None otherwise.
>      :ivar cvs_module: The CVS module if rcstype == 'cvs', None otherwise.
> -    :ivar git_repo_url: The URL of the git repo, if rcstype == 'git', None,
> -        otherwise.
>      """
>
> -    def __init__(self, branch_id, rcstype, svn_branch_url=None, cvs_root=None,
> -                 cvs_module=None, git_repo_url=None):
> +    def __init__(self, branch_id, rcstype, url=None, cvs_root=None,
> +                 cvs_module=None):
>          self.branch_id = branch_id
>          self.rcstype = rcstype
> -        self.svn_branch_url = svn_branch_url
> +        self.url = url
>          self.cvs_root = cvs_root
>          self.cvs_module = cvs_module
> -        self.git_repo_url = git_repo_url
>
>      @classmethod
>      def fromArguments(cls, arguments):
>          """Convert command line-style arguments to an instance."""
>          branch_id = int(arguments.pop(0))
>          rcstype = arguments.pop(0)
> -        if rcstype in ['svn', 'bzr-svn']:
> -            [svn_branch_url] = arguments
> -            cvs_root = cvs_module = git_repo_url = None
> +        if rcstype in ['svn', 'bzr-svn', 'git']:
> +            [url] = arguments
> +            cvs_root = cvs_module = None
>          elif rcstype == 'cvs':
> -            svn_branch_url = git_repo_url = None
> +            url = None
>              [cvs_root, cvs_module] = arguments
> -        elif rcstype == 'git':
> -            cvs_root = cvs_module = svn_branch_url = None
> -            [git_repo_url] = arguments
>          else:
>              raise AssertionError("Unknown rcstype %r." % rcstype)
> -        return cls(
> -            branch_id, rcstype, svn_branch_url, cvs_root, cvs_module,
> -            git_repo_url)
> +        return cls(branch_id, rcstype, url, cvs_root, cvs_module)
>
>      @classmethod
>      def fromCodeImport(cls, code_import):
>          """Convert a `CodeImport` to an instance."""
> +        branch_id = code_import.branch.id
>          if code_import.rcs_type == RevisionControlSystems.SVN:
> -            rcstype = 'svn'
> -            svn_branch_url = str(code_import.svn_branch_url)
> -            cvs_root = cvs_module = git_repo_url = None
> +            return cls(branch_id, 'svn', str(code_import.url))
>          elif code_import.rcs_type == RevisionControlSystems.BZR_SVN:
> -            rcstype = 'bzr-svn'
> -            svn_branch_url = str(code_import.svn_branch_url)
> -            cvs_root = cvs_module = git_repo_url = None
> +            return cls(branch_id, 'bzr-svn', str(code_import.url))
>          elif code_import.rcs_type == RevisionControlSystems.CVS:
> -            rcstype = 'cvs'
> -            svn_branch_url = git_repo_url = None
> -            cvs_root = str(code_import.cvs_root)
> -            cvs_module = str(code_import.cvs_module)
> +            return cls(
> +                branch_id, 'cvs',
> +                cvs_root=str(code_import.cvs_root),
> +                cvs_module=str(code_import.cvs_module))
>          elif code_import.rcs_type == RevisionControlSystems.GIT:
> -            rcstype = 'git'
> -            svn_branch_url = cvs_root = cvs_module = None
> -            git_repo_url = str(code_import.git_repo_url)
> +            return cls(branch_id, 'git', str(code_import.url))
>          else:
>              raise AssertionError("Unknown rcstype %r." % code_import.rcs_type)
> -        return cls(
> -            code_import.branch.id, rcstype, svn_branch_url,
> -            cvs_root, cvs_module, git_repo_url)
>
>      def asArguments(self):
>          """Return a list of arguments suitable for passing to a child process.
>          """
>          result = [str(self.branch_id), self.rcstype]
> -        if self.rcstype in ['svn', 'bzr-svn']:
> -            result.append(self.svn_branch_url)
> +        if self.rcstype in ['svn', 'bzr-svn', 'git']:
> +            result.append(self.url)
>          elif self.rcstype == 'cvs':
>              result.append(self.cvs_root)
>              result.append(self.cvs_module)
> -        elif self.rcstype == 'git':
> -            result.append(self.git_repo_url)
>          else:
>              raise AssertionError("Unknown rcstype %r." % self.rcstype)
>          return result
> @@ -294,7 +276,7 @@
>          source_details = self.import_data_store.source_details
>          if source_details.rcstype == 'svn':
>              return SubversionWorkingTree(
> -                source_details.svn_branch_url, str(target_path))
> +                source_details.url, str(target_path))
>          elif source_details.rcstype == 'cvs':
>              return CVSWorkingTree(
>                  source_details.cvs_root, source_details.cvs_module,
> @@ -497,7 +479,7 @@
>      @property
>      def pull_url(self):
>          """Return the URL that should be pulled from."""
> -        raise NotImplementedError
> +        return self.source_details.url

As mentioned in person, pull_url isn't needed at all now.

>      @property
>      def format_classes(self):
> @@ -535,11 +517,6 @@
>      """
>
>      @property
> -    def pull_url(self):
> -        """See `PullingImportWorker.pull_url`."""
> -        return self.source_details.git_repo_url
> -
> -    @property
>      def format_classes(self):
>          """See `PullingImportWorker.opening_format`."""
>          # We only return LocalGitBzrDirFormat for tests.
> @@ -575,11 +552,6 @@
>      """An import worker for importing Subversion via bzr-svn."""
>
>      @property
> -    def pull_url(self):
> -        """See `PullingImportWorker.pull_url`."""
> -        return self.source_details.svn_branch_url
> -
> -    @property
>      def format_classes(self):
>          """See `PullingImportWorker.opening_format`."""
>          from bzrlib.plugins.svn.format import SvnRemoteFormat