Bazaar Support For upload.py

Support upload.py for Bazaar, this is based upon the work done a while back
under issue http://codereview.appspot.com/2891

[kevin.kubasik]

Just adding some notes as a ping to someone who might review this.

http://codereview.appspot.com/14053/diff/1001/1002
File static/upload.py (right):

http://codereview.appspot.com/14053/diff/1001/1002#newcode582
Line 582: 
Added logic to handle bzr's diff return code of 1.

http://codereview.appspot.com/14053/diff/1001/1002#newcode1142
Line 1142: class BazaarVCS(VersionControlSystem):
The Bzr implementation.

[Andi Albrecht]

I think we'll need someone more familiar with bzr to review this patch.

My knowledge of Bazaar isn't that good (anymore). I've stopped using it
while working on the initial version of this patch (curiously enough).
I could have a look at the integration with upload.py but I wonder if I'm
the right one... ;-)

[kevin.kubasik]

Well, it appears that no one else really knows either... Is the upload.py
integration at least correct/not totally offensive? The bzr stuff is pretty
straightforward. (I'll try and get someone from the bzr dev team to get in here
and review as well)

[Andi Albrecht]

Not actually a review, just one remark...

BTW, I think it would be good to have a more regular bzr user in the reviewers
list, as you've suggested.

http://codereview.appspot.com/14053/diff/1001/1002
File static/upload.py (right):

http://codereview.appspot.com/14053/diff/1001/1002#newcode579
Line 579: print_output=False,check_returncode=True):
Consider RunShellWithReturnCode(). I think check_returncode isn't needed
anymore.

[bialix]

Couple of notes.

http://codereview.appspot.com/14053/diff/1001/1002
File static/upload.py (right):

http://codereview.appspot.com/14053/diff/1001/1002#newcode1145
Line 1145: def GenerateDiff(self, args):
You really need to consider of using bzrlib directly instead of running bzr
commands.

http://codereview.appspot.com/14053/diff/1001/1002#newcode1301
Line 1301: elif os.path.isdir(".bzr"):
Bazaar has root command similar to Mercurial. I think you should copy the
Mercurial approach to detect bzr, instead of isdir() call.

If this function supposed to be run from the subdirectory of bzr branch you
won't find ".bzr" control directory.

[bialix]

Comment about GetUnknownFiles

http://codereview.appspot.com/14053/diff/1001/1002
File static/upload.py (right):

http://codereview.appspot.com/14053/diff/1001/1002#newcode1171
Line 1171: """Return a list of files unknown to the VCS."""
You can use command: bzr ls --unknown --verbose
and get all unknown files without manual parsing status output.

[bialix]

Withdraw suggestion about bzrlib.

http://codereview.appspot.com/14053/diff/1001/1002
File static/upload.py (right):

http://codereview.appspot.com/14053/diff/1001/1002#newcode1145
Line 1145: def GenerateDiff(self, args):
On 2009/03/07 22:19:06, bialix wrote:
> You really need to consider of using bzrlib directly instead of running bzr
> commands.

OK, as pointed by Guido and Andi, using bzrlib is bad idea. Don't look at this
comment then.

[kevin.kubasik]

While this is overall a solid sentiment, bzr has several plugins that are
commonly used (like quilt) which allow patch stacking. These plugins override
bzr stat (so that it returns the expected diffs etc.) but don't often override
other commands. My feeling is that bzr stat is what users are going to run
before upload.py to check, so we should base our added files on that. 

Just my $0.02,
Kevin Kubasik

On 2009/03/08 12:03:33, bialix wrote:
> Comment about GetUnknownFiles
> 
> http://codereview.appspot.com/14053/diff/1001/1002
> File static/upload.py (right):
> 
> http://codereview.appspot.com/14053/diff/1001/1002#newcode1171
> Line 1171: """Return a list of files unknown to the VCS."""
> You can use command: bzr ls --unknown --verbose
> and get all unknown files without manual parsing status output.

[bialix]

Note about bzr detection: there is posssible interaction with svn/git branches
via bzr-svn/bzr-git plugins.

http://codereview.appspot.com/14053/diff/5004/6003
File static/upload.py (right):

http://codereview.appspot.com/14053/diff/5004/6003#newcode1296
Line 1296: try:
I think you need either use command ["bzr", "--no-plugins", "root"] OR move bzr
detection down to the end of this function, at least it should be after
hg/svn/git. Because if user has bzr-svn or bzr-git plugin installed, then bzr
can open foreign branches and provide support for them.

Other changes looks good for me.

[techtonik]

The patch doesn't apply to the latest version. Could you, please, update it to
the latest version and probably open a new issue, so that we can start review
from scratch?