launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lib/lp/code/browser/tests/test_gitref.py b/lib/lp/code/browser/tests/test_gitref.py
> index a55268a..4da620d 100644
> --- a/lib/lp/code/browser/tests/test_gitref.py
> +++ b/lib/lp/code/browser/tests/test_gitref.py
> @@ -160,10 +160,10 @@ class TestGitRefView(BrowserTestCase):
>  
>      def test_push_directions_logged_in_cannot_push_individual(self):
>          repo = self.factory.makeGitRepository()
> -        [ref] = self.factory.makeGitRefs(repository=repo,
> -                                         paths=["refs/heads/branch"])
> +        ref1, ref2 = self.factory.makeGitRefs(repository=repo,
> +            paths=["refs/heads/master", "refs/heads/branch"])
>          login_person(self.user)
> -        view = create_initialized_view(ref, "+index", principal=self.user)
> +        view = create_initialized_view(ref2, "+index", principal=self.user)

The changes to this test seem unnecessary.

>          git_push_url_text_match = soupmatchers.HTMLContains(
>              soupmatchers.Tag(
>                  'Push url text', 'dt',
> diff --git a/lib/lp/code/templates/git-macros.pt b/lib/lp/code/templates/git-macros.pt
> index 9baaf4d..7e5a46d 100644
> --- a/lib/lp/code/templates/git-macros.pt
> +++ b/lib/lp/code/templates/git-macros.pt
> @@ -67,6 +67,13 @@
>              <em>BRANCHNAME</em>
>              </tt>
>            </dd>
> +          <dt>To merge this branch:</dt>
> +          <dd>
> +            <tt id="merge-checkout-cmd" class="command" tal:content="string:git checkout ${view/default_branch}" />
> +          </dd>

As discussed in our 1:1 today, I think this (and the similar bit below) should have a "git remote add" command, and probably shouldn't have a "git checkout" because we don't have a good idea of the ancestor branch here.

You could also avoid the duplication by moving this into a separate <dl> tag, rather than the one with push instructions where it doesn't entirely make sense anyway.  Perhaps consider moving this up to within the <dl id="clone-url"> tag, since it arguably fits a little better there?

> +          <dd>
> +            <tt id="merge-cmd" class="command" tal:content="string:git merge ${view/current_branch}" />

Drop the current_branch property (which is somewhat inaccurate, since a ref's path might well be something like "refs/heads/feature/some-name" and in that case current_branch would return "feature" which is wrong), and just use ${context/name} instead.

> +          </dd>
>          </dl>
>          <dt tal:condition="python: getattr(view, 'allow_fork', False)">
>            <a tal:attributes="href view/fork_url" class="sprite add">


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