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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
