Completing implementation of GitCheckout in depot_tools

Completing implementation of GitCheckout in depot_tools.

Tested with:
* Unit tests have been added and they work.
* I also tested end-to-end using a skiabot-test repository in https://skia.googlesource.com/ (it is hidden). CLs I tested with are:
** https://codereview.chromium.org/22797006/ : Add whitespace in file1
** https://codereview.chromium.org/22815013/ : Remove whitespace from file1
** https://codereview.chromium.org/22867025/ : Add new files in directories
** https://codereview.chromium.org/22901018/ : Edit file in directory and delete file in directory
** https://codereview.chromium.org/22918014/ : Add, Delete and Modify 3 files
** https://codereview.chromium.org/23360004/ : Add new files in new directories


Note:
* When committing GitCheckout uses the --author to specify the original author. The author flag takes in 'Firstname Lastname <email_addr>' but we do not know the Firstname and LastName of the original author, which is why the code here parses out the username from the email address and uses it.
  Eg: For email address xyz@example.com it passes in --author 'xyz <xyz@example.com>'
* An example of the changes required in a project to use GitCheckout instead of SvnCheckout is in https://codereview.chromium.org/22859063/


Created to fix the following feature requests-
https://code.google.com/p/chromium/issues/detail?id=261619 : Update the Chrome commit queue to push to src.git.
https://code.google.com/p/skia/issues/detail?id=1593 : Add Git support to the Commit Queue.

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

[rmistry, 2013-08-23 13:59:19]

Hi Chase,

Since Robbie is OOO is there anybody else who can take a look at this till he is
back.

[M-A Ruel, 2013-08-23 17:05:10]

A few comments while waiting for Robert to be back.

https://codereview.chromium.org/22794015/diff/12001/checkout.py
File checkout.py (right):

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode553
checkout.py:553: class GitCheckoutBase(CheckoutBase):
The reason for this base class was for a git-svn support class. Since this
GitSvn support class was remove, there's no point in keeping two classes so I'd
recommend to rename it to GitCheckout and merge all the code.

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode578
checkout.py:578: assert self.git_url
This assert should be in the constructor.

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode590
checkout.py:590: try:
Add a comment:
# Look if the commit hash already exist. If so, we can skip a 'git fetch' call.

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode601
checkout.py:601: self._check_call_git(['pull', '--quiet'])
Why not keep the explicit remote?

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode771
checkout.py:771: return rev
Also, it should switch back to the master branch and remove the working branch.

[M-A Ruel, 2013-08-23 17:08:04]

https://codereview.chromium.org/22794015/diff/12001/checkout.py
File checkout.py (right):

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode705
checkout.py:705: self._check_call_git(['commit', '--amend', '-m',
commit_message])
You also want '--author', user. You don't need any specific server side support.

Also, we'll need .gitconfig to have a proper 'commit-bot@chromium.org' setup so
it is correctly specified as the committer.

[rmistry, 2013-08-26 13:08:17]

https://codereview.chromium.org/22794015/diff/12001/checkout.py
File checkout.py (right):

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode553
checkout.py:553: class GitCheckoutBase(CheckoutBase):
On 2013/08/23 17:05:10, M-A Ruel wrote:
> The reason for this base class was for a git-svn support class. Since this
> GitSvn support class was remove, there's no point in keeping two classes so
I'd
> recommend to rename it to GitCheckout and merge all the code.

Makes sense, Done.

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode578
checkout.py:578: assert self.git_url
On 2013/08/23 17:05:10, M-A Ruel wrote:
> This assert should be in the constructor.

Done.

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode590
checkout.py:590: try:
On 2013/08/23 17:05:10, M-A Ruel wrote:
> Add a comment:
> # Look if the commit hash already exist. If so, we can skip a 'git fetch'
call.

Done.

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode601
checkout.py:601: self._check_call_git(['pull', '--quiet'])
On 2013/08/23 17:05:10, M-A Ruel wrote:
> Why not keep the explicit remote?

There is difference in behavior here between these two commands:
* git pull
* git pull origin master

It took me a while to figure out why the assert in line 674, in the original
checkout.py, was failing when changes were made to the repo outside the CQ. It
is because of this line in the manpage for git-pull:

"""
A parameter <ref> without a colon is equivalent to <ref>: when pulling/fetching,
so it merges <ref> into the current branch without storing the remote branch
anywhere locally
"""

This means that the equivalent command to 'git pull' for the CQ git local master
is 'git pull origin master:refs/remotes/origin/master' and not 'git pull origin
master'.

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode705
checkout.py:705: self._check_call_git(['commit', '--amend', '-m',
commit_message])
On 2013/08/23 17:08:04, M-A Ruel wrote:
> You also want '--author', user. You don't need any specific server side
support.
> 
> Also, we'll need .gitconfig to have a proper 'commit-bot@chromium.org' setup
so
> it is correctly specified as the committer.

I do not completely understand this. If the CQ is running as
commit-bot@chromium.org and .gitconfig contains name and email of commit-bot.
Then we want --author to be the original creator of the CL (xyz@chromium.org) ?
I tried this locally and I get-
fatal: No existing author found with 'xyz@chromium.org'.

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode771
checkout.py:771: return rev
On 2013/08/23 17:05:10, M-A Ruel wrote:
> Also, it should switch back to the master branch and remove the working
branch.

That is done in prepare step before the patches are applied.
I think it is better to do it at the start rather than at the end incase there
is an exception / crash that causes it to not cleanup at the end.

[M-A Ruel, 2013-08-26 14:01:11]

https://codereview.chromium.org/22794015/diff/12001/checkout.py
File checkout.py (right):

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode601
checkout.py:601: self._check_call_git(['pull', '--quiet'])
On 2013/08/26 13:08:17, rmistry wrote:
> On 2013/08/23 17:05:10, M-A Ruel wrote:
> > Why not keep the explicit remote?
> 
> There is difference in behavior here between these two commands:
> * git pull
> * git pull origin master
> 
> It took me a while to figure out why the assert in line 674, in the original
> checkout.py, was failing when changes were made to the repo outside the CQ. It
> is because of this line in the manpage for git-pull:
> 
> """
> A parameter <ref> without a colon is equivalent to <ref>: when
pulling/fetching,
> so it merges <ref> into the current branch without storing the remote branch
> anywhere locally
> """
> 
> This means that the equivalent command to 'git pull' for the CQ git local
master
> is 'git pull origin master:refs/remotes/origin/master' and not 'git pull
origin
> master'.

Wouha, thanks, I had never realized that.

Then I'd like this to be put in a comment and having the whole fetch line
provided, i.e.
git pull origin master:refs/remotes/origin/master

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode705
checkout.py:705: self._check_call_git(['commit', '--amend', '-m',
commit_message])
On 2013/08/26 13:08:17, rmistry wrote:
> On 2013/08/23 17:08:04, M-A Ruel wrote:
> > You also want '--author', user. You don't need any specific server side
> support.
> > 
> > Also, we'll need .gitconfig to have a proper 'commit-bot@chromium.org' setup
> so
> > it is correctly specified as the committer.
> 
> I do not completely understand this. If the CQ is running as
> mailto:commit-bot@chromium.org and .gitconfig contains name and email of
commit-bot.
> Then we want --author to be the original creator of the CL
(mailto:xyz@chromium.org) ?
> I tried this locally and I get-
> fatal: No existing author found with 'xyz@chromium.org'.

What tool output this error, git? It's interesting. Git makes a difference
between who authored a CL and who committed it. I'd really like the CQ to take
advantage of this information to inser the fact the CQ did commit the CL on
behalf of the author. As a matter of fact, it's doing this in the subversion
case but by using an ugly svn revision property. In git, it's natively supported
but I've never tried it personally.

About the actual implementations, here's an optional idea:
One thing that could be necessary is to change apply() to accept the CL
description and author, since it's needed earlier in git case (vs svn). Then it
wouldn't be necessary to use a git comment hack, which is somewhat annoying.
That said, this means the callers (e.g. the commit queue and apply_issue.py)
need to be updated, which is a tad more involved :/ You could use
description=None, author=None in apply() so make the conversion easier, then
this data could be saved as an instance member so the next commit() call knows
the commit_message and user to use. It's only necessary for SvnCheckout.

Or you can do both the CQ and depot_tools CLs right away and have them committed
simultaneously and have the CQs be restarted right away, to not have to support
two APIs.

I don't mind personally, Robbie may have an opinion.

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode771
checkout.py:771: return rev
On 2013/08/26 13:08:17, rmistry wrote:
> On 2013/08/23 17:05:10, M-A Ruel wrote:
> > Also, it should switch back to the master branch and remove the working
> branch.
> 
> That is done in prepare step before the patches are applied.
> I think it is better to do it at the start rather than at the end incase there
> is an exception / crash that causes it to not cleanup at the end.

What I'm saying is that it should be done at the 2 places. It is so
apply_issue.py does the right thing too.

[rmistry, 2013-08-26 15:16:16]

https://codereview.chromium.org/22794015/diff/12001/checkout.py
File checkout.py (right):

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode601
checkout.py:601: self._check_call_git(['pull', '--quiet'])
On 2013/08/26 14:01:11, M-A Ruel wrote:
> On 2013/08/26 13:08:17, rmistry wrote:
> > On 2013/08/23 17:05:10, M-A Ruel wrote:
> > > Why not keep the explicit remote?
> > 
> > There is difference in behavior here between these two commands:
> > * git pull
> > * git pull origin master
> > 
> > It took me a while to figure out why the assert in line 674, in the original
> > checkout.py, was failing when changes were made to the repo outside the CQ.
It
> > is because of this line in the manpage for git-pull:
> > 
> > """
> > A parameter <ref> without a colon is equivalent to <ref>: when
> pulling/fetching,
> > so it merges <ref> into the current branch without storing the remote branch
> > anywhere locally
> > """
> > 
> > This means that the equivalent command to 'git pull' for the CQ git local
> master
> > is 'git pull origin master:refs/remotes/origin/master' and not 'git pull
> origin
> > master'.
> 
> Wouha, thanks, I had never realized that.
> 
> Then I'd like this to be put in a comment and having the whole fetch line
> provided, i.e.
> git pull origin master:refs/remotes/origin/master

Done.

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode705
checkout.py:705: self._check_call_git(['commit', '--amend', '-m',
commit_message])
On 2013/08/26 14:01:11, M-A Ruel wrote:
> On 2013/08/26 13:08:17, rmistry wrote:
> > On 2013/08/23 17:08:04, M-A Ruel wrote:
> > > You also want '--author', user. You don't need any specific server side
> > support.
> > > 
> > > Also, we'll need .gitconfig to have a proper 'commit-bot@chromium.org'
setup
> > so
> > > it is correctly specified as the committer.
> > 
> > I do not completely understand this. If the CQ is running as
> > mailto:commit-bot@chromium.org and .gitconfig contains name and email of
> commit-bot.
> > Then we want --author to be the original creator of the CL
> (mailto:xyz@chromium.org) ?
> > I tried this locally and I get-
> > fatal: No existing author found with 'xyz@chromium.org'.
> 
> What tool output this error, git? It's interesting. Git makes a difference
> between who authored a CL and who committed it. I'd really like the CQ to take
> advantage of this information to inser the fact the CQ did commit the CL on
> behalf of the author. As a matter of fact, it's doing this in the subversion
> case but by using an ugly svn revision property. In git, it's natively
supported
> but I've never tried it personally.

I see what I was doing wrong. I was doing:
--author "test@example.com"

this causes git to use it as a search token to search through previous commits,
looking for other commits by that author.
What worked is:
--author "firstname lastname test@example.com"

I do not think the CQ knows the first and last name of the original author,
maybe I should parse the email address and use that as the name instead. In this
case it would be "test test@example.com".

WDYT?

> 
> About the actual implementations, here's an optional idea:
> One thing that could be necessary is to change apply() to accept the CL
> description and author, since it's needed earlier in git case (vs svn). Then
it
> wouldn't be necessary to use a git comment hack, which is somewhat annoying.
> That said, this means the callers (e.g. the commit queue and apply_issue.py)
> need to be updated, which is a tad more involved :/ You could use
> description=None, author=None in apply() so make the conversion easier, then
> this data could be saved as an instance member so the next commit() call knows
> the commit_message and user to use. It's only necessary for SvnCheckout.
> 
> Or you can do both the CQ and depot_tools CLs right away and have them
committed
> simultaneously and have the CQs be restarted right away, to not have to
support
> two APIs.
> 
> I don't mind personally, Robbie may have an opinion.

I don't think its too bad to use the --amend here during commit time. I will let
you and Robbie decide, though that part can probably be done in a separate CL
after this one.

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode771
checkout.py:771: return rev
On 2013/08/26 14:01:11, M-A Ruel wrote:
> On 2013/08/26 13:08:17, rmistry wrote:
> > On 2013/08/23 17:05:10, M-A Ruel wrote:
> > > Also, it should switch back to the master branch and remove the working
> > branch.
> > 
> > That is done in prepare step before the patches are applied.
> > I think it is better to do it at the start rather than at the end incase
there
> > is an exception / crash that causes it to not cleanup at the end.
> 
> What I'm saying is that it should be done at the 2 places. It is so
> apply_issue.py does the right thing too.

I guess it does not hurt to have it in both places. Done.

[cmp_google, 2013-08-26 18:11:32]

+agable, xusydoc, ilevy, szager, I'm guessing at least two of them should be
able to give you a review

[rmistry, 2013-08-27 12:44:48]

https://codereview.chromium.org/22794015/diff/12001/checkout.py
File checkout.py (right):

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode705
checkout.py:705: self._check_call_git(['commit', '--amend', '-m',
commit_message])
On 2013/08/26 15:16:17, rmistry wrote:
> On 2013/08/26 14:01:11, M-A Ruel wrote:
> > On 2013/08/26 13:08:17, rmistry wrote:
> > > On 2013/08/23 17:08:04, M-A Ruel wrote:
> > > > You also want '--author', user. You don't need any specific server side
> > > support.
> > > > 
> > > > Also, we'll need .gitconfig to have a proper 'commit-bot@chromium.org'
> setup
> > > so
> > > > it is correctly specified as the committer.
> > > 
> > > I do not completely understand this. If the CQ is running as
> > > mailto:commit-bot@chromium.org and .gitconfig contains name and email of
> > commit-bot.
> > > Then we want --author to be the original creator of the CL
> > (mailto:xyz@chromium.org) ?
> > > I tried this locally and I get-
> > > fatal: No existing author found with 'xyz@chromium.org'.
> > 
> > What tool output this error, git? It's interesting. Git makes a difference
> > between who authored a CL and who committed it. I'd really like the CQ to
take
> > advantage of this information to inser the fact the CQ did commit the CL on
> > behalf of the author. As a matter of fact, it's doing this in the subversion
> > case but by using an ugly svn revision property. In git, it's natively
> supported
> > but I've never tried it personally.
> 
> I see what I was doing wrong. I was doing:
> --author "test@example.com"
> 
> this causes git to use it as a search token to search through previous
commits,
> looking for other commits by that author.
> What worked is:
> --author "firstname lastname test@example.com"
> 
> I do not think the CQ knows the first and last name of the original author,
> maybe I should parse the email address and use that as the name instead. In
this
> case it would be "test test@example.com".
> 
> WDYT?
> 
> > 
> > About the actual implementations, here's an optional idea:
> > One thing that could be necessary is to change apply() to accept the CL
> > description and author, since it's needed earlier in git case (vs svn). Then
> it
> > wouldn't be necessary to use a git comment hack, which is somewhat annoying.
> > That said, this means the callers (e.g. the commit queue and apply_issue.py)
> > need to be updated, which is a tad more involved :/ You could use
> > description=None, author=None in apply() so make the conversion easier, then
> > this data could be saved as an instance member so the next commit() call
knows
> > the commit_message and user to use. It's only necessary for SvnCheckout.
> > 
> > Or you can do both the CQ and depot_tools CLs right away and have them
> committed
> > simultaneously and have the CQs be restarted right away, to not have to
> support
> > two APIs.
> > 
> > I don't mind personally, Robbie may have an opinion.
> 
> I don't think its too bad to use the --amend here during commit time. I will
let
> you and Robbie decide, though that part can probably be done in a separate CL
> after this one.

Went ahead and added functionality for --author @ PatchSet 12.

[rmistry, 2013-08-30 13:33:49]

Friendly ping for Robbie.

[M-A Ruel, 2013-09-02 20:35:09]

https://codereview.chromium.org/22794015/diff/43001/checkout.py
File checkout.py (right):

https://codereview.chromium.org/22794015/diff/43001/checkout.py#newcode355
checkout.py:355: unused_revert=False):
Keep revert=False for compatibility.

Why not:
assert not revert, 'Implement revert=True'

https://codereview.chromium.org/22794015/diff/43001/checkout.py#newcode622
checkout.py:622: '%s:refs/remotes/%s/%s' % (self.remote_branch, self.remote,
I think it'd be more readable if you made it a named variable, so the cmd list
fits a single line.

https://codereview.chromium.org/22794015/diff/43001/checkout.py#newcode678
checkout.py:678: if revert:
You mean "reversed"? Is that really needed?

[Isaac (away), 2013-09-02 23:38:57]

https://codereview.chromium.org/22794015/diff/12001/checkout.py
File checkout.py (right):

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode601
checkout.py:601: self._check_call_git(['pull', '--quiet'])
On 2013/08/26 15:16:17, rmistry wrote:
> On 2013/08/26 14:01:11, M-A Ruel wrote:
> > On 2013/08/26 13:08:17, rmistry wrote:
> > > On 2013/08/23 17:05:10, M-A Ruel wrote:
> > > > Why not keep the explicit remote?
> > > 
> > > There is difference in behavior here between these two commands:
> > > * git pull
> > > * git pull origin master
> > > 
> > > It took me a while to figure out why the assert in line 674, in the
original
> > > checkout.py, was failing when changes were made to the repo outside the
CQ.
> It
> > > is because of this line in the manpage for git-pull:
> > > 
> > > """
> > > A parameter <ref> without a colon is equivalent to <ref>: when
> > pulling/fetching,
> > > so it merges <ref> into the current branch without storing the remote
branch
> > > anywhere locally
> > > """
> > > 
> > > This means that the equivalent command to 'git pull' for the CQ git local
> > master
> > > is 'git pull origin master:refs/remotes/origin/master' and not 'git pull
> > origin
> > > master'.
> > 
> > Wouha, thanks, I had never realized that.
> > 
> > Then I'd like this to be put in a comment and having the whole fetch line
> > provided, i.e.
> > git pull origin master:refs/remotes/origin/master
> 
> Done.

Why not "git fetch origin"?  git pull is roughly a combo of fetch and merge.

[rmistry, 2013-09-03 12:40:35]

https://codereview.chromium.org/22794015/diff/12001/checkout.py
File checkout.py (right):

https://codereview.chromium.org/22794015/diff/12001/checkout.py#newcode601
checkout.py:601: self._check_call_git(['pull', '--quiet'])
On 2013/09/02 23:38:58, Isaac wrote:
> On 2013/08/26 15:16:17, rmistry wrote:
> > On 2013/08/26 14:01:11, M-A Ruel wrote:
> > > On 2013/08/26 13:08:17, rmistry wrote:
> > > > On 2013/08/23 17:05:10, M-A Ruel wrote:
> > > > > Why not keep the explicit remote?
> > > > 
> > > > There is difference in behavior here between these two commands:
> > > > * git pull
> > > > * git pull origin master
> > > > 
> > > > It took me a while to figure out why the assert in line 674, in the
> original
> > > > checkout.py, was failing when changes were made to the repo outside the
> CQ.
> > It
> > > > is because of this line in the manpage for git-pull:
> > > > 
> > > > """
> > > > A parameter <ref> without a colon is equivalent to <ref>: when
> > > pulling/fetching,
> > > > so it merges <ref> into the current branch without storing the remote
> branch
> > > > anywhere locally
> > > > """
> > > > 
> > > > This means that the equivalent command to 'git pull' for the CQ git
local
> > > master
> > > > is 'git pull origin master:refs/remotes/origin/master' and not 'git pull
> > > origin
> > > > master'.
> > > 
> > > Wouha, thanks, I had never realized that.
> > > 
> > > Then I'd like this to be put in a comment and having the whole fetch line
> > > provided, i.e.
> > > git pull origin master:refs/remotes/origin/master
> > 
> > Done.
> 
> Why not "git fetch origin"?  git pull is roughly a combo of fetch and merge.

The documentation on pull and fetch are a little complicated, I know the git
pull here works, there may be a corresponding fetch call (maybe followed by a
rebase or merge call) that works as well. I prefer leaving the git pull here
especially since we do have some comments explaining what it is doing.

https://codereview.chromium.org/22794015/diff/43001/checkout.py
File checkout.py (right):

https://codereview.chromium.org/22794015/diff/43001/checkout.py#newcode355
checkout.py:355: unused_revert=False):
On 2013/09/02 20:35:09, M-A Ruel wrote:
> Keep revert=False for compatibility.
> 
> Why not:
> assert not revert, 'Implement revert=True'

Yes will do in a separate CL, see the comment below.

https://codereview.chromium.org/22794015/diff/43001/checkout.py#newcode622
checkout.py:622: '%s:refs/remotes/%s/%s' % (self.remote_branch, self.remote,
On 2013/09/02 20:35:09, M-A Ruel wrote:
> I think it'd be more readable if you made it a named variable, so the cmd list
> fits a single line.

Done.

https://codereview.chromium.org/22794015/diff/43001/checkout.py#newcode678
checkout.py:678: if revert:
On 2013/09/02 20:35:09, M-A Ruel wrote:
> You mean "reversed"? Is that really needed?

Context behind the revert parameters:
I am also working on adding one-click revert functionality to depot_tools,
rietveld and CQ. I figured I would go ahead and add revert/reversed
functionality to this CL, but it should really be in a separate CL. I reverted
the reverts.

When I do send out that CL (probably after this one is submitted) I will
incorporate your comments there.

[M-A Ruel, 2013-09-03 13:10:15]

looks good, a few optional nits (except the encode one which I'm pretty sure
it'll throw).

https://codereview.chromium.org/22794015/diff/54001/checkout.py
File checkout.py (right):

https://codereview.chromium.org/22794015/diff/54001/checkout.py#newcode578
checkout.py:578: logging.info('Checking out %s in %s' %
logging.info('Checking out %s in %s', self.project_name, self.project_path)

https://codereview.chromium.org/22794015/diff/54001/checkout.py#newcode611
checkout.py:611: """Sync the remote branch."""
Syncs

https://codereview.chromium.org/22794015/diff/54001/checkout.py#newcode624
checkout.py:624: def _get_revision(self):
maybe _get_head_commit_hash(self):
?
since 'revision' has no real meaning in git.

https://codereview.chromium.org/22794015/diff/54001/checkout.py#newcode720
checkout.py:720: 
Probably worth verifying that working_branch is the current branch.
Also please consider the scenarios where any of the commands below throws. In
practice I think the follow up prepare() will clean the mess up but just be sure
to have thought about it. Otherwise use try: <code> / finally: self.prepare();
but I don't think it'll be necessary.

https://codereview.chromium.org/22794015/diff/54001/checkout.py#newcode721
checkout.py:721: commit_cmd = ['commit', '--amend', '-m', commit_message]
You want commit_message.encode('utf-8') otherwise it'll throw on the first
non-ascii character. You can test this with 'é' if you want.

https://codereview.chromium.org/22794015/diff/54001/checkout.py#newcode726
checkout.py:726: commit_cmd.extend(['--author', '%s <%s>' % (name, user)])
Maybe later: try to detect if "user" is in git-compatible "Name <email>" format.

https://codereview.chromium.org/22794015/diff/54001/tests/checkout_test.py
File tests/checkout_test.py (right):

https://codereview.chromium.org/22794015/diff/54001/tests/checkout_test.py#ne...
tests/checkout_test.py:52: #
http://stackoverflow.com/questions/2816369/git-push-error-remote-rejected-mas...
# http://stackoverflow.com/questions/2816369/git-push-error-remote
will work just fine.

[rmistry, 2013-09-03 15:46:28]

https://codereview.chromium.org/22794015/diff/54001/checkout.py
File checkout.py (right):

https://codereview.chromium.org/22794015/diff/54001/checkout.py#newcode578
checkout.py:578: logging.info('Checking out %s in %s' %
On 2013/09/03 13:10:16, M-A Ruel wrote:
> logging.info('Checking out %s in %s', self.project_name, self.project_path)

Done.

https://codereview.chromium.org/22794015/diff/54001/checkout.py#newcode611
checkout.py:611: """Sync the remote branch."""
On 2013/09/03 13:10:16, M-A Ruel wrote:
> Syncs

Done.

https://codereview.chromium.org/22794015/diff/54001/checkout.py#newcode624
checkout.py:624: def _get_revision(self):
On 2013/09/03 13:10:16, M-A Ruel wrote:
> maybe _get_head_commit_hash(self):
> ?
> since 'revision' has no real meaning in git.

Makes sense. Done.

https://codereview.chromium.org/22794015/diff/54001/checkout.py#newcode720
checkout.py:720: 
On 2013/09/03 13:10:16, M-A Ruel wrote:
> Probably worth verifying that working_branch is the current branch.
> Also please consider the scenarios where any of the commands below throws. In
> practice I think the follow up prepare() will clean the mess up but just be
sure
> to have thought about it. Otherwise use try: <code> / finally: self.prepare();
> but I don't think it'll be necessary.

Added an assert to verify that working_branch is the current branch.
In my experiments it does look like prepare() cleans messes up because it forces
master branch to be at the latest commit hash and deletes the working_branch.

https://codereview.chromium.org/22794015/diff/54001/checkout.py#newcode721
checkout.py:721: commit_cmd = ['commit', '--amend', '-m', commit_message]
On 2013/09/03 13:10:16, M-A Ruel wrote:
> You want commit_message.encode('utf-8') otherwise it'll throw on the first
> non-ascii character. You can test this with 'é' if you want.

I added 'é' to the commit message in this CL:
https://codereview.chromium.org/22797006/

It looks like it committed with no errors in the test git repo with the commit
message:
cac2319  Modifying file1. testing utf-8 with é by Ravi Mistry - 11 minutes ago
master

Did I test it wrong?

https://codereview.chromium.org/22794015/diff/54001/checkout.py#newcode726
checkout.py:726: commit_cmd.extend(['--author', '%s <%s>' % (name, user)])
On 2013/09/03 13:10:16, M-A Ruel wrote:
> Maybe later: try to detect if "user" is in git-compatible "Name <email>"
format.

Added a TODO here.

https://codereview.chromium.org/22794015/diff/54001/tests/checkout_test.py
File tests/checkout_test.py (right):

https://codereview.chromium.org/22794015/diff/54001/tests/checkout_test.py#ne...
tests/checkout_test.py:52: #
http://stackoverflow.com/questions/2816369/git-push-error-remote-rejected-mas...
On 2013/09/03 13:10:16, M-A Ruel wrote:
> # http://stackoverflow.com/questions/2816369/git-push-error-remote
> will work just fine.

Cool, thanks.

[rmistry, 2013-09-04 14:34:35]

Any other comments from anybody else?

I am waiting on this CL for a few other CLs I have ready.

[M-A Ruel, 2013-09-04 14:45:06]

lgtm

[rmistry, 2013-09-04 17:10:10]

Thank you for the detailed review M-A.

I will submit tomorrow morning if there are no other
comments/questions/objections today.

[iannucci, 2013-09-04 18:06:40]

Sorry for totally slacking on this review :(. This lgtm.

[commit-bot: I haz the power, 2013-09-05 11:57:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmistry@google.com/22794015/68001

[commit-bot: I haz the power, 2013-09-05 11:59:41]

Change committed as 221392