Bazaar support

Adds support for Bazaar to upload.py.

* Some parts are very similar to Evan's patch.
* Fetching of base files is not supported (push only).
* No changes on the server side.
* patched upload.py was used to upload this patch ;-)

[GvR, 2008-08-12 16:49:23]

I'm not a bzr user (can you spot a trend? :-) -- could you find someone else to
review and test?

[ondrej.certik, 2008-08-12 17:20:22]

I tested it and it works fine for me. It'd be a nice feature to add a support to
upload several revisions at once, currently it merges them into one patch, but
it should upload each patch within one issue to rietveld.

Otherwise +1 from me.

http://codereview.appspot.com/2891/diff/4/21
File static/upload.py (right):

http://codereview.appspot.com/2891/diff/4/21#newcode830
Line 830: "from the top-level directory of your branch."))
I would change this to print "make sure to call upload.py from the top-level
directory" and that's it. Because we are going to add git and mercurial support
soon.

[Evan Martin, 2008-08-12 17:54:42]

This looks good in general.  My main concern is the way you handle the bzr
command-line flags.  I think it'd be better for only the bzr class to need to
know about any bzr flags -- perhaps there's a way to pass unknown flags down to
the VCS class?

http://codereview.appspot.com/2891/diff/4/21
File static/upload.py (right):

http://codereview.appspot.com/2891/diff/4/21#newcode744
Line 744: ErrorExit("--bzr_rev is not allowed for Subversion based projects.")
This sort of pattern will mean each VCS will need to test for all other VCS's
flags.

http://codereview.appspot.com/2891/diff/4/21#newcode763
Line 763: # changes found) then it is catched here.
Really, the exit status is 1 -- see the documentation for os.wait().  I think it
would make sense for RunShell to return the "real" exit code rather than this
bit-shifted value, and to test for 1 here.

http://codereview.appspot.com/2891/diff/4/21#newcode772
Line 772: lines[i] = "Index: %s\n%s" % (line.split("'")[1], "="*67)
For what it's worth, I believe the line of equals signs isn't required by
Rietveld.

[ondrej.certik, 2008-08-12 17:59:48]

On 2008/08/12 17:54:42, Evan Martin wrote:
> This looks good in general.  My main concern is the way you handle the bzr
> command-line flags.  I think it'd be better for only the bzr class to need to
> know about any bzr flags -- perhaps there's a way to pass unknown flags down
to
> the VCS class?
> 


I added support for Mercurial here:

http://codereview.appspot.com/2878

which depends on this patch. So you can see what needs to be done to make it
work.

[Andi Albrecht, 2008-08-19 08:01:02]

No new patch set, just replies to your comments while keeping it in sync with
the current trunk (I think we should do the git patch first...)

http://codereview.appspot.com/2891/diff/4/21
File static/upload.py (right):

http://codereview.appspot.com/2891/diff/4/21#newcode744
Line 744: ErrorExit("--bzr_rev is not allowed for Subversion based projects.")
On 2008/08/12 17:54:42, Evan Martin wrote:
> This sort of pattern will mean each VCS will need to test for all other VCS's
> flags.
> 

Option is now removed. I think I've "bzr cat -r submit: filename" was what I'm
looking for...

http://codereview.appspot.com/2891/diff/4/21#newcode763
Line 763: # changes found) then it is catched here.
On 2008/08/12 17:54:42, Evan Martin wrote:
> Really, the exit status is 1 -- see the documentation for os.wait().  I think
it
> would make sense for RunShell to return the "real" exit code rather than this
> bit-shifted value, and to test for 1 here.

When I use subprocess.Popen() and run "bzr diff" on an unmodified file, the
return code is 0. For modified files it's 1...

So bzr and svn behave different in that case. The current behavior (for SVN) is
to silently ignore unmodfied files if they are given at the command line (e.g.
'upload.py modified_file.txt unmodified_file.txt' --> unmodified_file.txt is
simply ignored).

http://codereview.appspot.com/2891/diff/4/21#newcode772
Line 772: lines[i] = "Index: %s\n%s" % (line.split("'")[1], "="*67)
On 2008/08/12 17:54:42, Evan Martin wrote:
> For what it's worth, I believe the line of equals signs isn't required by
> Rietveld.

Done.

http://codereview.appspot.com/2891/diff/4/21#newcode830
Line 830: "from the top-level directory of your branch."))
On 2008/08/12 17:20:22, ondrej.certik wrote:
> I would change this to print "make sure to call upload.py from the top-level
> directory" and that's it. Because we are going to add git and mercurial
support
> soon.

Already implemented in r252.

[Andi Albrecht, 2008-08-21 05:02:36]

Updated the patch per comments.

Evan, I've added a '--revision' command line option that should be used to
specify the revision to diff against (in other words, the tree-ish for git and
the revspec for Bazaar). The command line options are passed to the
VersionControlSystem constructor and I've modified GitVCS to handle this option.
Can you verify that it still works? Sorry for the change in the command line
use, but I need this option for other bzr commands too and I have to check if a
revision is specified for each of them to build the correct command line args
for bzr...

[Andi Albrecht, 2008-08-23 05:55:13]

Has someone time for a review? ;-)

[Antoine Pitrou, 2008-09-05 20:31:54]

(putting myself in cc)

[GvR, 2008-09-16 17:37:38]

On 2008/09/05 20:31:54, Antoine Pitrou wrote:
> (putting myself in cc)

Still waiting for reviews...

[Andi Albrecht, 2009-01-08 05:15:14]

On 2008/09/16 17:37:38, GvR wrote:
> On 2008/09/05 20:31:54, Antoine Pitrou wrote:
> > (putting myself in cc)
> 
> Still waiting for reviews...

No one seems to be interested in bzr support (and, to be honest, I've switched
to git a few months ago too)... ,-)

I'm closing this issue now due some lack of interest - if this comes up again,
I'll keep the current sources here:
http://github.com/andialbrecht/rietveld/tree/bzr_support