|
|
|
Created:
16 years, 11 months ago by kevin.kubasik Modified:
14 years, 2 months ago Base URL:
http://rietveld.googlecode.com/svn/trunk/ Visibility:
Public. |
Support upload.py for Bazaar, this is based upon the work done a while back under issue http://codereview.appspot.com/2891
Patch Set 1 #Patch Set 2 : Updated to work against latest upload.py #
Total comments: 7
Patch Set 3 : Updated RunShell to not use depreciated params #Patch Set 4 : Utilized bzr root to detect when in subdirectory #
Total comments: 1
Total messages: 10
|
kevin.kubasik
Just adding some notes as a ping to someone who might review this. http://codereview.appspot.com/14053/diff/1001/1002 File ...
|
16 years, 11 months ago (2009年02月09日 17:54:25 UTC) #1 |
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.
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... ;-)
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)
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.
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.
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.
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.
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.
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.
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?