launchpad-reviewers team mailing list archive

[Tom Wardill <tom.wardill@xxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lib/lp/code/browser/widgets/gitref.py b/lib/lp/code/browser/widgets/gitref.py
> index cfe8e40..2f4ce93 100644
> --- a/lib/lp/code/browser/widgets/gitref.py
> +++ b/lib/lp/code/browser/widgets/gitref.py
> @@ -104,21 +98,26 @@ class GitRepositoryPickerWidget(VocabularyPickerWidget):
>  class GitRefWidget(BrowserWidget, InputWidget):
>  
>      template = ViewPageTemplateFile("templates/gitref.pt")
> -    display_label = False
>      _widgets_set_up = False
>  
>      # If True, allow entering external repository URLs.
>      allow_external = False
>  
> +    # If True, only allow reference paths to be branches (refs/heads/*).
> +    require_branch = False

Currently, this only uses the GitRef version (and therefore require_branch = False). A GitBranch widget could exist in future (and would possibly make more sense on the edit Snap page)

> +
>      def setUpSubWidgets(self):
>          if self._widgets_set_up:
>              return
> +        path_vocabulary = "GitBranch" if self.require_branch else "GitRef"
>          fields = [
>              GitRepositoryField(
>                  __name__="repository", title=u"Git repository",
>                  required=False, vocabulary="GitRepository",
>                  allow_external=self.allow_external),
> -            TextLine(__name__="path", title=u"Git branch", required=False),
> +            Choice(
> +                __name__="path", title=u"Git branch", required=False,
> +                vocabulary=path_vocabulary),
>              ]
>          for field in fields:
>              setUpWidget(
> @@ -174,27 +178,31 @@ class GitRefWidget(BrowserWidget, InputWidget):
>                      "There is no Git repository named '%s' registered in "
>                      "Launchpad." % entered_name))
>          if self.path_widget.hasInput():
> -            path = self.path_widget.getInputValue()
> -        else:
> -            path = None
> -        if not path:
> -            if self.context.required:
> -                raise WidgetInputError(
> -                    self.name, self.label,
> -                    LaunchpadValidationError(
> -                        "Please enter a Git branch path."))
> +            # We've potentially just tried to change the repository that is
> +            # involved, or changing from a bzr branch to a git repo, so there
> +            # is no existing repository set up. We need to set this so we
> +            # can compare the ref against the 'new' repo.
> +            from zope.security.proxy import removeSecurityProxy

Not keen on this, unsure what the best way to avoid it is. Maybe an interface for the vocabulary?

> +            unsecure_vocab = removeSecurityProxy(self.path_widget.vocabulary)
> +            if IGitRepository.providedBy(repository):
> +                unsecure_vocab.setRepository(repository)
>              else:
> -                return
> -        if self.allow_external and not IGitRepository.providedBy(repository):
> -            ref = getUtility(IGitRefRemoteSet).new(repository, path)
> -        else:
> -            ref = repository.getRefByPath(path)
> -            if ref is None:
> +                unsecure_vocab.setRepositoryURL(repository)
> +            try:
> +                ref = self.path_widget.getInputValue()
> +            except ConversionError:
>                  raise WidgetInputError(
>                      self.name, self.label,
>                      LaunchpadValidationError(
>                          "The repository at %s does not contain a branch named "
> -                        "'%s'." % (repository.display_name, path)))
> +                        "'%s'." % (
> +                            repository.display_name,
> +                            self.path_widget._getFormInput())))
> +        if not ref and self.context.required:
> +            raise WidgetInputError(
> +                    self.name, self.label,
> +                    LaunchpadValidationError(
> +                        "Please enter a Git branch path."))
>          return ref
>  
>      def error(self):


-- 
https://code.launchpad.net/~twom/launchpad/+git/launchpad/+merge/392831
Your team Launchpad code reviewers is requested to review the proposed merge of ~twom/launchpad:git-branch-picker into launchpad:master.