code review 4172060: codereview: fix clpatch with empty diffs

codereview: fix clpatch with empty diffs

Avoid passing the placeholder diff to hgpatch, so that
clpatch-ing an empty diff grabs the metadata and warns
about it being empty, rather than failing with a
hard-to-debug problem ("mkdir: no such file or dir",
no metadata, etc).

[niemeyer, 2011-02-21 03:25:13]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[adg, 2011-02-21 23:04:40]

R=rsc

[rsc, 2011-02-22 20:13:21]

http://codereview.appspot.com/4172060/diff/3001/lib/codereview/codereview.py
File lib/codereview/codereview.py (right):

http://codereview.appspot.com/4172060/diff/3001/lib/codereview/codereview.py#...
lib/codereview/codereview.py:1139: if patch != emptydiff:
while you're here, move the if err != "" up a few lines so
it is right after the call to DownloadCL.
then

if patch == emptydiff:
    return 'codereview issue %s has no diff' % clname

seems like it should suffice

[niemeyer, 2011-02-23 04:24:35]

> lib/codereview/codereview.py:1139: if patch != emptydiff:
> while you're here, move the if err != "" up a few lines so
> it is right after the call to DownloadCL.

Done.

> if patch == emptydiff:
>     return 'codereview issue %s has no diff' % clname
> 
> seems like it should suffice

Doing just this will prevent the metadata from being downloaded.
That makes it hard to use "change -d" or continue a partially
failed attempt which went far enough as to create the metadata
on the server.

That said, I realize that just fixing the empty diff won't be
enough either, so I've added the logic you requested, but also
added a new --mine flag to handle owned CLs better so that both
the reviewer case and the continuation case are handled properly.

Please take a look and see what you think.

[rsc, 2011-02-23 05:08:45]

I'd prefer not to go down this road.

hg clpatch is not for moving your own CLs from one machine to another.
I know that's how people sometimes use it, but it's just not for that.
Doing that would be a separate, more involved change, and I don't
believe that it is the right model.  clpatch is for developers to patch in
a CL in order to test or submit it.  There's no point in patching in no diff.

It makes more sense to use a separate tool to move CLs between
machines.  For some possibilities, see the message below from December.

Russ



---------- Forwarded message ----------
From: Russ Cox <rsc@golang.org>
Date: Tue, Dec 14, 2010 at 09:39
Subject: Re: [go-nuts] moving CL's around
To: at <arctanofyourface@gmail.com>
Cc: golang-nuts@googlegroups.com

>     I have a few CL's that I'm working on and want to move them from one
> computer to the next (i.e. work <-> home).  Is there something built into
> the system to allow this, or is the best way to just tar and copy?

Tar and copy (or rsync or unison or ...) is the best way.
upload + clpatch is okay but it's easy to get out of sync.

The actual metadata is in the .hg/codereview directory.
At minimum you need to copy the changed files from
the repository itself and the associated
.hg/codereview/cl.NNNNNN file.

If you really want to get complicated, I use a script
I call clpush that lets me turn individual CLs on and off.
The usage is to say "clpush" to get a list of known CLs
and then "clpush -NNNNNN" to turn off a CL or
"clpush +NNNNNN" to bring it back.
I think it's a little too fiddly to be part of the standard
Mercurial extension, and writing it in shell was faster
than Python anyway, but if you are interested I posted
a copy at https://gist.github.com/740498.
Using clpush you could "clpush -NNNNNN" and
then copy .hg/codereview/cl????.NNNNNN to the
other computer and there "clpush +NNNNNN".

Russ

[niemeyer, 2011-02-23 12:24:21]

> I'd prefer not to go down this road.
> 
> hg clpatch is not for moving your own CLs from one machine to another.
> I know that's how people sometimes use it, but it's just not for that.

The problem I'm trying to solve is not moving CLs around, but to
continue a CL which lives no where else but at codereview.  Do you
think this would be acceptable, or should I move back to just
fixing the empty diff bug?

> It makes more sense to use a separate tool to move CLs between
> machines.  For some possibilities, see the message below from December.

I was just pondering about your approach the other day.  I really miss
some modern features of source control management while shaping up patches
for inclusion. I have been playing with managing the Go tree within git
with this purpose:

    http://github.com/niemeyer/golang

I've been evolving the "sync" tool (in the tree) so that good part of the
process is automated.  It's not ready for general consumption yet, though.

[rsc, 2011-02-23 15:53:28]

>> hg clpatch is not for moving your own CLs from one machine to another.
>> I know that's how people sometimes use it, but it's just not for that.
>
> The problem I'm trying to solve is not moving CLs around, but to
> continue a CL which lives no where else but at codereview.

That's a move too.
Please go back to just fixing the empty diff bug.

If you want to add support for moving CLs between
machines I think it should be separate commands,
not clpatch.  Clpatch is specialized for patching
in order to try something or submit it.

Russ

[niemeyer, 2011-02-23 16:24:54]

Sounds good.  Back to just fixing clpatch.

PTAL

Out of curiosity, here is the change history for this CL:

https://github.com/niemeyer/golang/commits/4172060-clpatch-no-files

[rsc, 2011-02-23 16:33:02]

LGTM

[rsc, 2011-02-23 16:48:48]

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

codereview: fix clpatch with empty diffs

Avoid passing the placeholder diff to hgpatch, so that
clpatch-ing an empty diff grabs the metadata and warns
about it being empty, rather than failing with a
hard-to-debug problem ("mkdir: no such file or dir",
no metadata, etc).

R=rsc
CC=golang-dev
http://codereview.appspot.com/4172060

Committer: Russ Cox <rsc@golang.org>