Closed Bug 1098265 Opened 7 years ago Closed 6 years ago

MozReview documents needs details on how to pull patches for review

Categories

(MozReview Graveyard :: General, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: standard8, Assigned: dminor)

Details

Attachments

(1 file)

The MozReview documents (http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview.html) really could do with some information on how to do a pull and a review, and get back to your original state.

On the bugs, you just get:

====
Pull down this commit:

hg pull review -r 9fe37f5753287a3d72cdbe06af68a65ffefab0b1
====

but its not entirely clear/documented anywhere any easy ways of doing that in such a way that you get updated to that revision, and that it doesn't affect hg outgoing, or supply some other way of getting back to the original state easily.
Priority: -- → P1
Assignee: nobody → dminor
I've added some sections on basic patch manipulation. Please let me know if there are any other topics you'd like to see discussed. Thanks!
Attachment #8616833 - Flags: feedback?(standard8)
Attachment #8616833 - Flags: feedback?(rjesup)
Comment on attachment 8616833 [details] [diff] [review]
New section on how to work with patches

Review of attachment 8616833 [details] [diff] [review]:
-----------------------------------------------------------------

::: docs/mozreview/reviewboard.rst
@@ +207,5 @@
> +   to go.
> +
> +   To pull the commits down use the url provided in the review
> +   description, but avoiding doing an update at the same time, which will force 
> +   a merge. For instance, if the revision is foo::

'foo':: may be clearer

@@ +238,5 @@
> +
> +   If you're using queues, you can pop it from your queue and then delete it::
> +
> +   $ hg qpop 
> +   $ hg qdelete foo.diff

Is it always named foo.diff (where foo is the revision)?
Attachment #8616833 - Flags: feedback?(rjesup) → feedback+
Comment on attachment 8616833 [details] [diff] [review]
New section on how to work with patches

Review of attachment 8616833 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me as well. Thanks!
Attachment #8616833 - Flags: feedback+
Comment on attachment 8616833 [details] [diff] [review]
New section on how to work with patches

Review of attachment 8616833 [details] [diff] [review]:
-----------------------------------------------------------------

::: docs/mozreview/reviewboard.rst
@@ +200,4 @@
>     to offer little value over just going to Review Board and looking at
>     the original comments there.)
>  
> +Working With "Patches"

There should be === or --- under here to create a new section.

@@ +207,5 @@
> +   to go.
> +
> +   To pull the commits down use the url provided in the review
> +   description, but avoiding doing an update at the same time, which will force 
> +   a merge. For instance, if the revision is foo::

``foo`` will do monospace/<code> magic is almost certainly what you want for inline identifiers.

Also, `hg pull` + `hg update` will *not* do a merge unless you have uncommitted local changes, in which case the pull is irrelevant to a merge occurring.

@@ +219,5 @@
> +
> +   Or import it into a mercurial queue::
> +
> +   $ hg qimport -r foo
> +   $ hg qapplied

We should strive for no new MQ references in Mercurial documentation, as MQ is an unloved feature and only traps people into sub-optimal workflows.

@@ +230,5 @@
> +
> +   $ hg export qtip > foo.patch
> +
> +   To remove the patch once you've finished, assuming you're using bookmarks,
> +   you can strip it from the repository and then delete the bookmark::

I'm not a fan of these instructions. Extra heads are harmless. You should never need to strip your repo. Learn how to use multiple heads. https://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/bookmarks.html#understanding-head-based-development
Thanks for your feedback, I've made the fixes you suggested.

> 
> We should strive for no new MQ references in Mercurial documentation, as MQ
> is an unloved feature and only traps people into sub-optimal workflows.
> 
I disagree, I've added some words to indicate that using bookmarks is the preferred workflow, but I think it's important to provide both sets of instructions. Conversations I've had have indicated that people are more likely to try bookmarks if there is a side-by-side view of how to do things with queues and with bookmarks. This can be done in blog posts, but it is much more discoverable in the docs.
Attachment #8616833 - Flags: feedback?(standard8)
Rather than delay further I've pushed the updates to [1]. We can always revise later.

[1] https://hg.mozilla.org/hgcustom/version-control-tools/rev/364e671fcbe3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.