launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lib/lp/code/browser/branchmergeproposal.py b/lib/lp/code/browser/branchmergeproposal.py
> index eec3b00..e2eeaa3 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 git_ssh_url(self):

There's some risk of this being misunderstood in the future.  Maybe source_git_ssh_url (noting my comment below that the URL in question should be that of source_git_repository, not target_git_repository).

> +        """The git+ssh:// URL for the target repository,
> +        adjusted for this user."""
> +        base_url = urlsplit(self.context.target_git_repository.git_ssh_url)
> +        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/templates/branchmergeproposal-pagelet-summary.pt b/lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt
> index 16ba4a4..d2d5f84 100644
> --- a/lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt
> +++ b/lib/lp/code/templates/branchmergeproposal-pagelet-summary.pt
> @@ -121,9 +121,18 @@
>          <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="git-remote-add" tal:condition="python: view.user">
> +      <th>Merge guidelines:</th>
> +      <td>git remote add -f <span tal:content="string:${context/target_git_repository/owner/name} ${view/git_ssh_url}"/></td>
> +    </tr>
> +    <tr id="git-remote-update" tal:condition="python: view.user">
> +      <th></th>
> +      <td>git remote update <span tal:content="context/target_git_repository/owner/name"/></td>
> +    </tr>
> +    <tr id="git-merge-cmd" tal:condition="python: view.user">
> +      <th></th>
> +      <td>git merge <span tal:content="string:${context/target_git_repository/owner/name}/${context/merge_source/name}"/></td>
> +    </tr>

I'd put all the commands in a single <td>, also inside a <tt> or <code> tag or similar.

target_git_repository needs to be source_git_repository throughout here - the operation involved is fetching and merging the source into the target.

>      <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.