gclient_scm: add support for .git

gclient_scm: add support for .git


gclient_tests: add GitWrapper test cases


gclient_scm: make RunGit private and add url check


gclient_scm: add revert, run_hooks, and status


gclient_scm: add git implementations of cleanup, diff, and export


gclient_scm: implment GitSCM::update


gclient_scm: add a checks for unsupported scm and unimplemented method

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=27241

[msb, 2009-09-24 16:55:37]

Ignore this review for now. git-cl seems to have done the wrong thing
here. Not sure why drover is part of this code review.

Will fix and resend.

On Thu, Sep 24, 2009 at 9:53 AM,  <msb@chromium.org> wrote:
> Reviewers: Marc-Antoine Ruel,
>
> Description:
> gclient_scm: add support for .git
>
>
> gclient_tests: add GitWrapper test cases
>
>
> gclient_scm: make RunGit private and add url check
>
>
> gclient_scm: add revert, run_hooks, and status
>
>
> gclient_scm: add git implementations of cleanup, diff, and export
>
>
> gclient_scm: implment GitSCM::update
>
>
> gclient_scm: add a checks for unsupported scm and unimplemented method
>
> Please review this at http://codereview.chromium.org/235005
>
> Affected files:
> =A0A drover
> =A0drover.bat
> =A0M drover.py
> =A0M gclient_scm.py
> =A0M tests/gclient_test.py
> =A0M trychange.py
>
>
>

[M-A Ruel, 2009-09-24 17:44:25]

Mostly nits

http://codereview.chromium.org/235005/diff/14/15
File gclient_scm.py (right):

http://codereview.chromium.org/235005/diff/14/15#newcode40
Line 40: if url and (url[:4] == 'git:' or url[:4] == 'ssh:' or url[-4:] ==
'.git'):
Use startswith() and endswith() instead.

http://codereview.chromium.org/235005/diff/14/15#newcode90
Line 90: """ Wrapper for Git """
We never put spaces around.

http://codereview.chromium.org/235005/diff/14/15#newcode94
Line 94: checkout_path=os.path.join(self._root_dir, self.relpath)
 =

http://codereview.chromium.org/235005/diff/14/15#newcode141
Line 141: self._RunGit(['clone', url, checkout_path])
-q?

http://codereview.chromium.org/235005/diff/14/15#newcode160
Line 160: All reverted files will be appended to file_list, even if Subversion
subversion?

http://codereview.chromium.org/235005/diff/14/15#newcode181
Line 181: print self._RunGit(['status'], cwd=checkout_path)
I think you should do like revert and find the merge-base first. You really want
the diff against upstream here.

http://codereview.chromium.org/235005/diff/14/16
File tests/gclient_test.py (right):

http://codereview.chromium.org/235005/diff/14/16#newcode1318
Line 1318: class GitWrapperTestCase(GClientBaseTestCase):
nit: Err, all this should really go to gclient_scm_unittests.py

[msb, 2009-09-24 22:46:10]

Fixed. PTAL.

maruel@chromium.org (maruel@chromium.org) wrote:
> Mostly nits
>
>
> http://codereview.chromium.org/235005/diff/14/15
> File gclient_scm.py (right):
>
> http://codereview.chromium.org/235005/diff/14/15#newcode40
> Line 40: if url and (url[:4] == 'git:' or url[:4] == 'ssh:' or url[-4:]
> == '.git'):
> Use startswith() and endswith() instead.
>

Fixed.

> http://codereview.chromium.org/235005/diff/14/15#newcode90
> Line 90: """ Wrapper for Git """
> We never put spaces around.
>

Fixed.

> http://codereview.chromium.org/235005/diff/14/15#newcode94
> Line 94: checkout_path=os.path.join(self._root_dir, self.relpath)
>  =
>

Fixed.

> http://codereview.chromium.org/235005/diff/14/15#newcode141
> Line 141: self._RunGit(['clone', url, checkout_path])
> -q?
>

Good idea. Fixed.

> http://codereview.chromium.org/235005/diff/14/15#newcode160
> Line 160: All reverted files will be appended to file_list, even if
> Subversion
> subversion?
>

Fixed.

> http://codereview.chromium.org/235005/diff/14/15#newcode181
> Line 181: print self._RunGit(['status'], cwd=checkout_path)
> I think you should do like revert and find the merge-base first. You
> really want the diff against upstream here.
>

Fixed.

You're right. Good catch!

> http://codereview.chromium.org/235005/diff/14/16
> File tests/gclient_test.py (right):
>
> http://codereview.chromium.org/235005/diff/14/16#newcode1318
> Line 1318: class GitWrapperTestCase(GClientBaseTestCase):
> nit: Err, all this should really go to gclient_scm_unittests.py
>

Good idea. Fixed.

> http://codereview.chromium.org/235005

[M-A Ruel, 2009-09-25 01:12:33]

Mostly style nits.

I can say I learned a fair amount of new git commands. :)

lgtm

http://codereview.chromium.org/235005/diff/6001/6003
File gclient_scm.py (right):

http://codereview.chromium.org/235005/diff/6001/6003#newcode36
Line 36: 'svn' : SVNWrapper,
style nit: why SVNWrapper but GitWrapper? (caps)

http://codereview.chromium.org/235005/diff/6001/6003#newcode105
Line 105: cwd=checkout_path).strip()
alignment multiple places.

http://codereview.chromium.org/235005/diff/6001/6003#newcode112
Line 112: try:
I'd prefer:
if not os.path.exists(export_path):
  os.makedirs(export_path)

without muting the exception.

http://codereview.chromium.org/235005/diff/6001/6004
File tests/gclient_scm_test.py (right):

http://codereview.chromium.org/235005/diff/6001/6004#newcode100
Line 100: self.root_dir, files_list)
alignment at a few places.

http://codereview.chromium.org/235005/diff/6001/6004#newcode164
Line 164: gclient_scm.RunSVNAndGetFileList(['update', '--revision', 'BASE'],
base_path,
80 cols

http://codereview.chromium.org/235005/diff/6001/6004#newcode178
Line 178: []).AndReturn(None)
.

http://codereview.chromium.org/235005/diff/6001/6004#newcode231
Line 231: self.root_dir, files_list)
.

http://codereview.chromium.org/235005/diff/6001/6004#newcode267
Line 267: '.', True).AndReturn(xml_text)
.

http://codereview.chromium.org/235005/diff/6001/6004#newcode273
Line 273: 'Copied From URL':
'http://src.chromium.org/svn/trunk/src/chrome/app/DEPS',
.

http://codereview.chromium.org/235005/diff/6001/6004#newcode308
Line 308: '.', True).AndReturn(xml_text)
.

http://codereview.chromium.org/235005/diff/6001/6004#newcode426
Line 426: relpath=self.relpath)
.

http://codereview.chromium.org/235005/diff/6001/6004#newcode437
Line 437: relpath=self.relpath)
.

http://codereview.chromium.org/235005/diff/6001/6004#newcode450
Line 450: relpath=self.relpath)
.

http://codereview.chromium.org/235005/diff/6001/6004#newcode469
Line 469: relpath=self.relpath)
.

http://codereview.chromium.org/235005/diff/6001/6004#newcode484
Line 484: relpath=self.relpath)
.

http://codereview.chromium.org/235005/diff/6001/6004#newcode497
Line 497: relpath=self.relpath)
.

http://codereview.chromium.org/235005/diff/6001/6004#newcode525
Line 525: relpath=self.relpath)
.

[msb, 2009-09-25 17:46:07]

Fixed. PTAL.

Also:

* made self.checkout_path a SCMWrapper member
* using CreateSCM instead of GitWrapper for SCM creation
* default cwd to self.checkout_path for _RunGit

maruel@chromium.org (maruel@chromium.org) wrote:
> Mostly style nits.
>
> I can say I learned a fair amount of new git commands. :)
>
> lgtm
>
>
> http://codereview.chromium.org/235005/diff/6001/6003
> File gclient_scm.py (right):
>
> http://codereview.chromium.org/235005/diff/6001/6003#newcode36
> Line 36: 'svn' : SVNWrapper,
> style nit: why SVNWrapper but GitWrapper? (caps)
>

There was already a IsUsingGit method so I continued with that convention.

It also makes it easier for the eyes to split out words:

GetGitRev

versus:

GetGITRev

But I can change if you prefer.

> http://codereview.chromium.org/235005/diff/6001/6003#newcode105
> Line 105: cwd=checkout_path).strip()
> alignment multiple places.
>

Fixed.

> http://codereview.chromium.org/235005/diff/6001/6003#newcode112
> Line 112: try:
> I'd prefer:
> if not os.path.exists(export_path):
>   os.makedirs(export_path)
>
> without muting the exception.
>

Fixed.

> http://codereview.chromium.org/235005/diff/6001/6004
> File tests/gclient_scm_test.py (right):
>
> http://codereview.chromium.org/235005/diff/6001/6004#newcode100
> Line 100: self.root_dir, files_list)
> alignment at a few places.
>

Fixed.

> http://codereview.chromium.org/235005/diff/6001/6004#newcode164
> Line 164: gclient_scm.RunSVNAndGetFileList(['update', '--revision',
> 'BASE'], base_path,
> 80 cols
>

Fixed all.

> http://codereview.chromium.org/235005/diff/6001/6004#newcode178
> Line 178: []).AndReturn(None)
> .
>
> http://codereview.chromium.org/235005/diff/6001/6004#newcode231
> Line 231: self.root_dir, files_list)
> .
>
> http://codereview.chromium.org/235005/diff/6001/6004#newcode267
> Line 267: '.', True).AndReturn(xml_text)
> .
>
> http://codereview.chromium.org/235005/diff/6001/6004#newcode273
> Line 273: 'Copied From URL':
> 'http://src.chromium.org/svn/trunk/src/chrome/app/DEPS',
> .
>
> http://codereview.chromium.org/235005/diff/6001/6004#newcode308
> Line 308: '.', True).AndReturn(xml_text)
> .
>
> http://codereview.chromium.org/235005/diff/6001/6004#newcode426
> Line 426: relpath=self.relpath)
> .
>
> http://codereview.chromium.org/235005/diff/6001/6004#newcode437
> Line 437: relpath=self.relpath)
> .
>
> http://codereview.chromium.org/235005/diff/6001/6004#newcode450
> Line 450: relpath=self.relpath)
> .
>
> http://codereview.chromium.org/235005/diff/6001/6004#newcode469
> Line 469: relpath=self.relpath)
> .
>
> http://codereview.chromium.org/235005/diff/6001/6004#newcode484
> Line 484: relpath=self.relpath)
> .
>
> http://codereview.chromium.org/235005/diff/6001/6004#newcode497
> Line 497: relpath=self.relpath)
> .
>
> http://codereview.chromium.org/235005/diff/6001/6004#newcode525
> Line 525: relpath=self.relpath)
> .
>
> http://codereview.chromium.org/235005

[M-A Ruel, 2009-09-25 17:52:36]

lgtm