code review 1238044: Provide advice on using Mercurial Queues in conjunction...

Provide advice on using Mercurial Queues in conjunction with codreview

[pwil3058, 2010-05-21 01:00:13]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[r, 2010-05-21 18:29:24]

http://codereview.appspot.com/1238044/diff/2001/3001
File doc/contribute.html (right):

http://codereview.appspot.com/1238044/diff/2001/3001#newcode106
doc/contribute.html:106: The Mercurial Queues  (<strong>MQ</strong>) extension
provides a mechanism for
why all the <strong>?

http://codereview.appspot.com/1238044/diff/2001/3001#newcode110
doc/contribute.html:110: If you'r not using MQ these snippets can be safely
ignored.
spell check please.
you're

http://codereview.appspot.com/1238044/diff/2001/3001#newcode133
doc/contribute.html:133: Pulling, pushing, updating and committing while MQ
patches
the tone of this text is off.  also i  think the MQ stuff should simply be a
different section; maybe even a different page, if it's necessary at all. do mq
users really need to be handheld?

http://codereview.appspot.com/1238044/diff/2001/3001#newcode137
doc/contribute.html:137: <code>$GOROOT/.hg/hgrc</code> file in order to prevent
you accidently performing
accidentally

http://codereview.appspot.com/1238044/diff/2001/3001#newcode210
doc/contribute.html:210: <strong>[MQ:</strong> You should have all of the MQ
patches relavent to your
relevant

[rsc, 2010-05-21 19:37:39]

Thanks for writing all this down.
Perhaps it makes sense as a separate section
or even a separate page instead of being interlaced
with the rest of the page.  I think I'd lean toward
a separate page, linked from a rewrite of the
"Mercurial Power Users" line.

Russ

[pwil3058, 2010-05-21 23:46:56]

On 22/05/10 04:29, r@golang.org wrote:
>
> http://codereview.appspot.com/1238044/diff/2001/3001
> File doc/contribute.html (right):
>
> http://codereview.appspot.com/1238044/diff/2001/3001#newcode106
> doc/contribute.html:106: The Mercurial Queues (<strong>MQ</strong>)
> extension provides a mechanism for
> why all the <strong>?

Just trying to make it clear that the bits within the strong bits only 
apply if your using MQ.

>
> http://codereview.appspot.com/1238044/diff/2001/3001#newcode110
> doc/contribute.html:110: If you'r not using MQ these snippets can be
> safely ignored.
> spell check please.
> you're

Yes, I should have turned spell checking on in my editor.

>
> http://codereview.appspot.com/1238044/diff/2001/3001#newcode133
> doc/contribute.html:133: Pulling, pushing, updating and committing while
> MQ patches
> the tone of this text is off. also i think the MQ stuff should simply
> be a different section; maybe even a different page, if it's necessary
> at all. do mq users really need to be handheld?

Experienced ones, probably not.  Newcomers, probably yes.

I guess I was influenced by the (implied) request for instructions on 
how to use MQ with codereview in the existing document.  I interpreted 
this to mean the advice should be targeted at novices.

>
> http://codereview.appspot.com/1238044/diff/2001/3001#newcode137
> doc/contribute.html:137: <code>$GOROOT/.hg/hgrc</code> file in order to
> prevent you accidently performing
> accidentally

Yes, see above.

>
> http://codereview.appspot.com/1238044/diff/2001/3001#newcode210
> doc/contribute.html:210: <strong>[MQ:</strong> You should have all of
> the MQ patches relavent to your
> relevant
>
> http://codereview.appspot.com/1238044/show

Ditto.

More in reply to your other mail.

Peter

[pwil3058, 2010-05-22 00:06:11]

On 22/05/10 05:37, Russ Cox wrote:
> Thanks for writing all this down.
> Perhaps it makes sense as a separate section
> or even a separate page instead of being interlaced
> with the rest of the page.

OK.  I'm not wedded to my actual words or the way I interspersed them in 
the document.  The main thing is the meaning of the advice.

>  I think I'd lean toward
> a separate page, linked from a rewrite of the
> "Mercurial Power Users" line.

OK.  I'll take that tack but I'll target the advice a Mercurial novices 
as experienced users can probably figure it out for themselves (I did).

BTW I'm the author of the gwsmhg <http://gwsmhg.sourceforge.net/> PyGTK 
GUI wrapper for Mercurial and I'm looking at adding support for 
codereview to it (to make things easier for myself if nobody else).  My 
current thinking is to make this part of gwsmhg's MQ page and I've just 
modified that page to support the use of MQ guards as I figure they're a 
very useful mechanism when working on more than one change set in a 
single repository.  The next step will be codereview specific buttons 
and menus.

The main problem I'll face doing this is testing.  Is there a way I can 
set up a dummy code review site to test against?  I figure that I 
wouldn't be popular if I bombarded your site with test transactions as 
there would be a lot of them.

Peter

[rsc, 2010-05-22 00:34:58]

> The main problem I'll face doing this is testing.  Is there a way I can set
> up a dummy code review site to test against?  I figure that I wouldn't be
> popular if I bombarded your site with test transactions as there would be a
> lot of them.

How many is a lot?

codereview.appspot.com runs the code from http://code.google.com/p/rietveld.
If you download the App Engine SDK you can run a local copy.

Russ

[pwil3058, 2010-05-22 00:57:04]

On 22/05/10 10:34, Russ Cox wrote:
>> The main problem I'll face doing this is testing.  Is there a way I can set
>> up a dummy code review site to test against?  I figure that I wouldn't be
>> popular if I bombarded your site with test transactions as there would be a
>> lot of them.
>
> How many is a lot?

With GUIs I tend to get in a (code; test)* loop where each iteration can 
be quite short so a lot is a lot. :-)

>
> codereview.appspot.com runs the code from http://code.google.com/p/rietveld.
> If you download the App Engine SDK you can run a local copy.

I was hoping that it would be that easy.

Thanks
Peter

[pwil3058, 2010-05-26 01:38:15]

Hello rsc, r (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc1, 2010-06-01 21:17:49]

Looks pretty good; thanks for writing all this down
(and getting it working!).

http://codereview.appspot.com/1238044/diff/13001/14001
File doc/codereview_with_mq.html (right):

http://codereview.appspot.com/1238044/diff/13001/14001#newcode27
doc/codereview_with_mq.html:27: hgext.mq=
This can just be
mq = 

The hgext. prefix is deprecated now.

http://codereview.appspot.com/1238044/diff/13001/14001#newcode31
doc/codereview_with_mq.html:31: In addition, since pulling, pushing, updating
and committing while <code>mq</code> patches
Shorter:

Since pulling, pushing, updating and committing while <code>mq</code> patches
are applied can mangle your repository (or a remote one), add these lines to
prevent that case:

http://codereview.appspot.com/1238044/diff/13001/14001#newcode45
doc/codereview_with_mq.html:45: preupdate.mq-no-update = ! hg qtop > /dev/null
2>&1
Will this catch hg sync?
Should we edit hg sync to catch it instead?

http://codereview.appspot.com/1238044/diff/13001/14001#newcode80
doc/codereview_with_mq.html:80: for p in `hg qapplied`; do hg qguard $p +99999;
done
use more readable $(hg qapplied)

http://codereview.appspot.com/1238044/diff/13001/14001#newcode92
doc/codereview_with_mq.html:92: to the Go project</a> it is necessary to
synchronize your repository, using
s/ it is/, it is/
s/, using/ using/

http://codereview.appspot.com/1238044/diff/13001/14001#newcode94
doc/codereview_with_mq.html:94: As <code>hg sync</code> runs <code>hg pull
-u</code>
s/As/Because/
s/$/,/

http://codereview.appspot.com/1238044/diff/13001/14001#newcode95
doc/codereview_with_mq.html:95: it should not be run <code>hg sync</code> while
<code>mq</code> patches are
s/it should not be run/you should not run/

http://codereview.appspot.com/1238044/diff/13001/14001#newcode96
doc/codereview_with_mq.html:96: applied as it could lead to your repository
becoming corrupted so you should
applied.  Instead, pop all your patches...

http://codereview.appspot.com/1238044/diff/13001/14001#newcode102
doc/codereview_with_mq.html:102: Because the main Go repository is maintained as
a single line of reviewed changes and you
might be able to shorten the entire paragraph to

When reapplying the patches, you may need to resolve conflicts a described in
<a href...

http://codereview.appspot.com/1238044/diff/13001/14002
File doc/contribute.html (right):

http://codereview.appspot.com/1238044/diff/13001/14002#newcode106
doc/contribute.html:106: To allow Go contributors to take advantage of
I'd like to keep the power users caveat.  It's super easy to screw up with mq.

Mercurial power users: if you prefer to use the Mercurial Queues extension,
see ...

[pwil3058, 2010-06-02 00:13:20]

If these actions are OK let me know and I'll send a revised patch.

http://codereview.appspot.com/1238044/diff/13001/14001
File doc/codereview_with_mq.html (right):

http://codereview.appspot.com/1238044/diff/13001/14001#newcode27
doc/codereview_with_mq.html:27: hgext.mq=
On 2010/06/01 21:17:49, rsc1 wrote:
> This can just be
> mq = 
> 
> The hgext. prefix is deprecated now.
> 

Done.

http://codereview.appspot.com/1238044/diff/13001/14001#newcode31
doc/codereview_with_mq.html:31: In addition, since pulling, pushing, updating
and committing while <code>mq</code> patches
On 2010/06/01 21:17:49, rsc1 wrote:
> Shorter:
> 
> Since pulling, pushing, updating and committing while <code>mq</code> patches
> are applied can mangle your repository (or a remote one), add these lines to
> prevent that case:
> 
> 

Done.

http://codereview.appspot.com/1238044/diff/13001/14001#newcode45
doc/codereview_with_mq.html:45: preupdate.mq-no-update = ! hg qtop > /dev/null
2>&1
On 2010/06/01 21:17:49, rsc1 wrote:
> Will this catch hg sync?

The prechangegroup hook aborts "hg sync" according to my tests.

> Should we edit hg sync to catch it instead?
> 

Shouldn't be necessary.

http://codereview.appspot.com/1238044/diff/13001/14001#newcode80
doc/codereview_with_mq.html:80: for p in `hg qapplied`; do hg qguard $p +99999;
done
On 2010/06/01 21:17:49, rsc1 wrote:
> use more readable $(hg qapplied)
> 

Done.

http://codereview.appspot.com/1238044/diff/13001/14001#newcode92
doc/codereview_with_mq.html:92: to the Go project</a> it is necessary to
synchronize your repository, using
On 2010/06/01 21:17:49, rsc1 wrote:
> s/ it is/, it is/
> s/, using/ using/
> 
> 

Done.

http://codereview.appspot.com/1238044/diff/13001/14001#newcode94
doc/codereview_with_mq.html:94: As <code>hg sync</code> runs <code>hg pull
-u</code>
On 2010/06/01 21:17:49, rsc1 wrote:
> s/As/Because/
> s/$/,/
> 
> 

Done.

http://codereview.appspot.com/1238044/diff/13001/14001#newcode95
doc/codereview_with_mq.html:95: it should not be run <code>hg sync</code> while
<code>mq</code> patches are
On 2010/06/01 21:17:49, rsc1 wrote:
> s/it should not be run/you should not run/
> 

Done.

http://codereview.appspot.com/1238044/diff/13001/14001#newcode96
doc/codereview_with_mq.html:96: applied as it could lead to your repository
becoming corrupted so you should
On 2010/06/01 21:17:49, rsc1 wrote:
> applied.  Instead, pop all your patches...
> 

Done.

http://codereview.appspot.com/1238044/diff/13001/14001#newcode102
doc/codereview_with_mq.html:102: Because the main Go repository is maintained as
a single line of reviewed changes and you
On 2010/06/01 21:17:49, rsc1 wrote:
> might be able to shorten the entire paragraph to
> 
> When reapplying the patches, you may need to resolve conflicts a described in
> <a href...
> 

Done.

http://codereview.appspot.com/1238044/diff/13001/14002
File doc/contribute.html (right):

http://codereview.appspot.com/1238044/diff/13001/14002#newcode106
doc/contribute.html:106: To allow Go contributors to take advantage of
On 2010/06/01 21:17:49, rsc1 wrote:
> I'd like to keep the power users caveat.  It's super easy to screw up with mq.

But not that hard to fix :-) thanks to distributed nature of hg.

> 
> Mercurial power users: if you prefer to use the Mercurial Queues extension,
> see ...
> 

Done.

[rsc, 2010-06-02 02:19:32]

I can't see if they're OK unless you send a revised patch.  ;-)

On Tue, Jun 1, 2010 at 17:13,  <pwil3058@gmail.com> wrote:
> If these actions are OK let me know and I'll send a revised patch.
>
>
> http://codereview.appspot.com/1238044/diff/13001/14001
> File doc/codereview_with_mq.html (right):
>
> http://codereview.appspot.com/1238044/diff/13001/14001#newcode27
> doc/codereview_with_mq.html:27: hgext.mq=
> On 2010/06/01 21:17:49, rsc1 wrote:
>>
>> This can just be
>> mq =
>
>> The hgext. prefix is deprecated now.
>
>
> Done.
>
> http://codereview.appspot.com/1238044/diff/13001/14001#newcode31
> doc/codereview_with_mq.html:31: In addition, since pulling, pushing,
> updating and committing while <code>mq</code> patches
> On 2010/06/01 21:17:49, rsc1 wrote:
>>
>> Shorter:
>
>> Since pulling, pushing, updating and committing while <code>mq</code>
>
> patches
>>
>> are applied can mangle your repository (or a remote one), add these
>
> lines to
>>
>> prevent that case:
>
>
>
> Done.
>
> http://codereview.appspot.com/1238044/diff/13001/14001#newcode45
> doc/codereview_with_mq.html:45: preupdate.mq-no-update = ! hg qtop >
> /dev/null 2>&1
> On 2010/06/01 21:17:49, rsc1 wrote:
>>
>> Will this catch hg sync?
>
> The prechangegroup hook aborts "hg sync" according to my tests.
>
>> Should we edit hg sync to catch it instead?
>
>
> Shouldn't be necessary.
>
> http://codereview.appspot.com/1238044/diff/13001/14001#newcode80
> doc/codereview_with_mq.html:80: for p in `hg qapplied`; do hg qguard $p
> +99999; done
> On 2010/06/01 21:17:49, rsc1 wrote:
>>
>> use more readable $(hg qapplied)
>
>
> Done.
>
> http://codereview.appspot.com/1238044/diff/13001/14001#newcode92
> doc/codereview_with_mq.html:92: to the Go project</a> it is necessary to
> synchronize your repository, using
> On 2010/06/01 21:17:49, rsc1 wrote:
>>
>> s/ it is/, it is/
>> s/, using/ using/
>
>
>
> Done.
>
> http://codereview.appspot.com/1238044/diff/13001/14001#newcode94
> doc/codereview_with_mq.html:94: As <code>hg sync</code> runs <code>hg
> pull -u</code>
> On 2010/06/01 21:17:49, rsc1 wrote:
>>
>> s/As/Because/
>> s/$/,/
>
>
>
> Done.
>
> http://codereview.appspot.com/1238044/diff/13001/14001#newcode95
> doc/codereview_with_mq.html:95: it should not be run <code>hg
> sync</code> while <code>mq</code> patches are
> On 2010/06/01 21:17:49, rsc1 wrote:
>>
>> s/it should not be run/you should not run/
>
>
> Done.
>
> http://codereview.appspot.com/1238044/diff/13001/14001#newcode96
> doc/codereview_with_mq.html:96: applied as it could lead to your
> repository becoming corrupted so you should
> On 2010/06/01 21:17:49, rsc1 wrote:
>>
>> applied.  Instead, pop all your patches...
>
>
> Done.
>
> http://codereview.appspot.com/1238044/diff/13001/14001#newcode102
> doc/codereview_with_mq.html:102: Because the main Go repository is
> maintained as a single line of reviewed changes and you
> On 2010/06/01 21:17:49, rsc1 wrote:
>>
>> might be able to shorten the entire paragraph to
>
>> When reapplying the patches, you may need to resolve conflicts a
>
> described in
>>
>> <a href...
>
>
> Done.
>
> http://codereview.appspot.com/1238044/diff/13001/14002
> File doc/contribute.html (right):
>
> http://codereview.appspot.com/1238044/diff/13001/14002#newcode106
> doc/contribute.html:106: To allow Go contributors to take advantage of
> On 2010/06/01 21:17:49, rsc1 wrote:
>>
>> I'd like to keep the power users caveat.  It's super easy to screw up
>
> with mq.
>
> But not that hard to fix :-) thanks to distributed nature of hg.
>
>
>> Mercurial power users: if you prefer to use the Mercurial Queues
>
> extension,
>>
>> see ...
>
>
> Done.
>
> http://codereview.appspot.com/1238044/show
>

[pwil3058, 2010-06-02 04:24:59]

Hello rsc, r (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2010-06-02 04:31:55]

minor comments only. it's pretty good, if a bit repetitive, which is not a bad
thing.

http://codereview.appspot.com/1238044/diff/22001/23001
File doc/codereview_with_mq.html (right):

http://codereview.appspot.com/1238044/diff/22001/23001#newcode21
doc/codereview_with_mq.html:21: To enable the <code>mq</code> edit either
<code>$HOME/.hgrc</code> (to enable it
"enable the mq edit" doesn't sound like english.  can you say this in a
non-jargony way?  maybe "enable Mercurial Queues"?

http://codereview.appspot.com/1238044/diff/22001/23001#newcode32
doc/codereview_with_mq.html:32: are applied can mangle your repository (or a
remote one), add these lines to
drop the parens. also s/mangle/damage/

http://codereview.appspot.com/1238044/diff/22001/23001#newcode43
doc/codereview_with_mq.html:43: preupdate.mq-no-update = ! hg qtop > /dev/null
2>&1
why isn't there a line for committing?

http://codereview.appspot.com/1238044/diff/22001/23001#newcode49
doc/codereview_with_mq.html:49: The entire checked-out tree is writeable and you
can use <code>mq</code>,
s/writeable/writable/

http://codereview.appspot.com/1238044/diff/22001/23001#newcode52
doc/codereview_with_mq.html:52: of <a
href="http://hgbook.red-bean.com/read/">Mercurial: The Definitive Guide</a>,
the first time it's fine to use the full title. second and subsequent, a
linkless "The Guide" will do.

[pwil3058, 2010-06-02 05:33:32]

Hello rsc, r (cc: golang-dev@googlegroups.com),

Please take another look.

[pwil3058, 2010-06-02 05:37:32]

http://codereview.appspot.com/1238044/diff/22001/23001
File doc/codereview_with_mq.html (right):

http://codereview.appspot.com/1238044/diff/22001/23001#newcode21
doc/codereview_with_mq.html:21: To enable the <code>mq</code> edit either
<code>$HOME/.hgrc</code> (to enable it
On 2010/06/02 04:31:56, r wrote:
> "enable the mq edit" doesn't sound like english.  can you say this in a
> non-jargony way?  maybe "enable Mercurial Queues"?

How about I just remove the "the" in front of mq?  That's what's making it into
gibberish.

http://codereview.appspot.com/1238044/diff/22001/23001#newcode32
doc/codereview_with_mq.html:32: are applied can mangle your repository (or a
remote one), add these lines to
On 2010/06/02 04:31:56, r wrote:
> drop the parens. also s/mangle/damage/

Done.

http://codereview.appspot.com/1238044/diff/22001/23001#newcode43
doc/codereview_with_mq.html:43: preupdate.mq-no-update = ! hg qtop > /dev/null
2>&1
On 2010/06/02 04:31:56, r wrote:
> why isn't there a line for committing?

Good question.  It shouldn't be necessary as codereview disables commit.

I don't have one in my normal hgrc whcih probably means commit hooks get in the
way of qrefresh and qput which are commits under another name.

I never have a problem because my GUI disables all the "commit" buttons whenever
MQ patches are applied and I always use the GUI for hg stuff (because I'm too
lazy to type).

http://codereview.appspot.com/1238044/diff/22001/23001#newcode49
doc/codereview_with_mq.html:49: The entire checked-out tree is writeable and you
can use <code>mq</code>,
On 2010/06/02 04:31:56, r wrote:
> s/writeable/writable/

Done.

http://codereview.appspot.com/1238044/diff/22001/23001#newcode52
doc/codereview_with_mq.html:52: of <a
href="http://hgbook.red-bean.com/read/">Mercurial: The Definitive Guide</a>,
On 2010/06/02 04:31:56, r wrote:
> the first time it's fine to use the full title. second and subsequent, a
> linkless "The Guide" will do.

Done.

[rsc1, 2010-06-03 00:04:09]

LGTM

[rsc, 2010-06-03 00:11:55]

*** Submitted as http://code.google.com/p/go/source/detail?r=433cbad2f237 ***

doc: codereview + Mercurial Queues

R=rsc, r
CC=golang-dev
http://codereview.appspot.com/1238044

Committer: Russ Cox <rsc@golang.org>