launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Diff comments:

> diff --git a/lib/lp/code/browser/branchmergeproposal.py b/lib/lp/code/browser/branchmergeproposal.py
> index eec3b00..6e5ee15 100644
> --- a/lib/lp/code/browser/branchmergeproposal.py
> +++ b/lib/lp/code/browser/branchmergeproposal.py
> @@ -645,6 +649,15 @@ class BranchMergeProposalView(LaunchpadFormView, UnmergedRevisionsMixin,
>          """Location of page for commenting on this proposal."""
>          return canonical_url(self.context, view_name='+comment')
>  
> +    @property
> +    def source_git_ssh_url(self):
> +        """The git+ssh:// URL for the source repository,
> +        adjusted for this user."""
> +        base_url = urlsplit(self.context.target_git_repository.git_ssh_url)

The property name is "source_*", but it is using the "target" repository to make the URL. Did you intend to use self.context.source_git_repository here?

> +        url = list(base_url)
> +        url[1] = "{}@{}".format(self.user.name, base_url.hostname)
> +        return urlunsplit(url)
> +
>      @cachedproperty
>      def conversation(self):
>          """Return a conversation that is to be rendered."""
> diff --git a/lib/lp/code/browser/tests/test_branchmergeproposal.py b/lib/lp/code/browser/tests/test_branchmergeproposal.py
> index f2ec4ec..fedfb18 100644
> --- a/lib/lp/code/browser/tests/test_branchmergeproposal.py
> +++ b/lib/lp/code/browser/tests/test_branchmergeproposal.py
> @@ -1754,6 +1756,10 @@ class TestBranchMergeProposalBrowserView(BrowserTestCase):
>  
>      layer = DatabaseFunctionalLayer
>  
> +    def setUp(self):
> +        super(TestBranchMergeProposalBrowserView, self).setUp()
> +        self.hosting_fixture = self.useFixture(GitHostingFixture())

It seems that the tests are not using self.hosting_fixture. It is possible that it was necessary while creating the MPs for the test scenarios (in which case, it's totally fine to add this here). But if it was not necessary, maybe we can drop this setUp method.

> +
>      def test_prerequisite_bzr(self):
>          # A prerequisite branch is rendered in the Bazaar case.
>          branch = self.factory.makeProductBranch()
> diff --git a/lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt b/lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt
> index 16ba4a4..c0c7883 100644
> --- a/lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt
> +++ b/lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt
> @@ -121,9 +121,17 @@
>          <div tal:replace="structure context/@@++diff-stats" />
>        </td>
>      </tr>
> -    <tal:comment condition="nothing">
> -      <!-- XXX cjwatson 2015-04-18: Add merge instructions for Git. -->
> -    </tal:comment>
> +    <tr id="summary-row-merge-instruction-git" tal:condition="python: view.user and context.source_git_repository">
> +        <th>Merge guidelines:</th>

The indentation here is mixing 2 and 4 spaces.

> +        <td>
> +          <tt id="remote-add" class="command" tal:content="string:git remote add -f ${context/source_git_repository/owner/name} ${view/source_git_ssh_url}" />
> +          <br />
> +          <tt id="remote-update" class="command" tal:content="string:git remote update ${context/source_git_repository/owner/name}" />
> +          <br />
> +          <tt id="merge-cmd" class="command" tal:content="string:git merge ${context/source_git_repository/owner/name}/${context/merge_source/name}" />

I wonder if we should be explicit here and include an instruction to run "git checkout ${context/target_git_ref/path}" or something similar *before* the "git merge" command.

Otherwise, one could copy and paste the commands directly to the console, and end up merging the proposed branch in the wrong target.

> +        </td>
> +    </tr>
> +
>      <tr id="summary-row-merge-instruction"
>          tal:condition="context/source_branch">
>        <th>To merge this branch:</th>


-- 
https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/393312
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilasc/launchpad:git-merge-instructions-mp into launchpad:master.