1039522 - Include command to pull review request head

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Mark Côté [:mcote]]

A really useful feature would be a URL in the review request (squashed or otherwise) that would allow you to check out that "branch" (head, I guess).  This would make it simple to test the code under review and would bring this system into closer parity with GitHub.

A separate field would probably be best, but in the interest of getting things done, I'm totally okay with the link being in the Description field.

Comment 1

[Mark Côté [:mcote]]

The commits view being introduced in bug 1034171 is probably a good place for these links.

Comment 2

[Mark Côté [:mcote]]

Attachment: Review for review ID: bz://1039522/mcote

/r/96 - Bug 1039522 - Include pull command in description of squashed request.

Comment 3

[Gregory Szorc [:gps]]

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py
(Diff revision 1)
> +        ['', 'hg pull -r %s reviews' % commits['individual'][-1]['id']])

We don't know for certain that the user has aliased *reviews* as the remote name. But this is easy enough to implement.

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py
(Diff revision 1)
> +        squashed_description[-1] += 'these commits:'

Do none of these changes to squash_description cause a test failure? I'm almost certain they would.

Comment 4

[Mark Côté [:mcote]]

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py
(Diff revision 1)
> +        squashed_description[-1] += 'these commits:'

> Do none of these changes to squash_description cause a test failure? I'm almost certain they would.
Whoops, of course.  Will fix them up.

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py
(Diff revision 1)
> +        ['', 'hg pull -r %s reviews' % commits['individual'][-1]['id']])

> We don't know for certain that the user has aliased *reviews* as the remote name. But this is easy enough to implement.
smacleod suggested we just standardize on a path name and implement it in the client later, so that's what I'm doing. :)

Comment 5

[Gregory Szorc [:gps]]

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py
(Diff revision 1)
> +        ['', 'hg pull -r %s reviews' % commits['individual'][-1]['id']])

> We don't know for certain that the user has aliased *reviews* as the remote name. But this is easy enough to implement.
Can we make it *review* instead of *reviews*? One less character to type :) And, *hg push review* kinda sounds more like a sentence: *Mercurial, push this for review.*

Comment 6

[Mark Côté [:mcote]]

Comment on attachment 8464229 [details]
Review for review ID: bz://1039522/mcote

/r/96 - Bug 1039522 - Include pull command in description of squashed request.
/r/97 - Bug 1039522 - Update tests, change repo nick from 'reviews' to 'review'.

Comment 7

[Steven MacLeod [:smacleod]]

Looks good to me, after fixing the one issue. Please fold the two commits together before landing, they should really be one.

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py
(Diff revision 2)
> +        ['', 'hg pull -r %s review' % commits['individual'][-1]['id']])

nit: can we please make this:
            ['', 'hg pull review -r %s' % commits['individual'][-1]['id']])
            
Having "review" before the -r argument makes it flow really nicely in english :D

Along with the magic gps wants to add where the "review" path is auto detected and added, I wonder if in the future it'd be worth adding extra magic so that you could specify "-r <reviewid>" instead of the sha1... so it would be something like "hg pull review -r bz://1234/mcote" to pull down the commits. Could be a nice to have down the road.

Comment 8

[Mark Côté [:mcote]]

Agreed, would be nice.  File a bug? :)

Comment 9

[Mark Côté [:mcote]]

Pushed: http://hg.mozilla.org/hgcustom/version-control-tools/rev/79aea0531ff0

Then I realized I forgot to update the tests for the little change, so fixed those: http://hg.mozilla.org/hgcustom/version-control-tools/rev/d94741da4f19 :)

Comment 10

[Gregory Szorc [:gps]]

Regarding magic with -r identifiers, Mercurial does this via protocol handlers. e.g. bz://. One of the reasons I wanted to use proto:// handlers for the identifiers was to facilitate this kind of magic in the future :)

We could probably introduce rb://<number> to magically map RB review requests into known commit IDs. Good follow-up.