Completing implementation of GitCheckout in depot_tools

checkout.py

Index: checkout.py
===================================================================
--- checkout.py	(revision 218449)
+++ checkout.py	(working copy)
@@ -552,14 +552,17 @@
 
 class GitCheckoutBase(CheckoutBase):

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

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.

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

Makes sense, Done.

   """Base class for git checkout. Not to be used as-is."""
-  def __init__(self, root_dir, project_name, remote_branch,
+  def __init__(self, root_dir, project_name, remote_branch, git_url,
       post_processors=None):
     super(GitCheckoutBase, self).__init__(
         root_dir, project_name, post_processors)
-    # There is no reason to not hardcode it.
-    self.remote = 'origin'
+    self.git_url = git_url
     self.remote_branch = remote_branch
+    # The working branch where patches will be applied. It will track the
+    # remote branch.
     self.working_branch = 'working_branch'
+    # There is no reason to not hardcode origin.
+    self.remote = 'origin'
 
   def prepare(self, revision):
     """Resets the git repository in a clean state.
@@ -567,8 +570,22 @@
     Checks it out if not present and deletes the working branch.
     """
     assert self.remote_branch
-    assert os.path.isdir(self.project_path)
-    self._check_call_git(['reset', '--hard', '--quiet'])
+
+    if not os.path.isdir(self.project_path):
+      # Clone the repo if the directory is not present.
+      logging.info('Checking out %s in %s' %
+          (self.project_name, self.project_path))
+      assert self.git_url

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

This assert should be in the constructor.

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

Done.

+      self._check_call_git(
+          ['clone', self.git_url, '-b', self.remote_branch, self.project_path],
+          cwd=None, timeout=FETCH_TIMEOUT)
+    else:
+      # Throw away all uncommitted changes in the existing checkout.
+      self._check_call_git(['checkout', self.remote_branch])
+      self._check_call_git(
+          ['reset', '--hard', '--quiet',
+           '%s/%s' % (self.remote, self.remote_branch)])
+
     if revision:
       try:

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

Add a comment:
# Look if the commit hash already exist. If so, we can skip a 'git fetch' call.

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

Done.

         revision = self._check_output_git(['rev-parse', revision])
@@ -581,12 +598,17 @@
       branches, active = self._branches()
       if active != 'master':
         self._check_call_git(['checkout', '--force', '--quiet', 'master'])
-      self._check_call_git(['pull', self.remote, self.remote_branch, '--quiet'])
+      self._check_call_git(['pull', '--quiet'])

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

Why not keep the explicit remote?

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

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'.

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

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

[rmistry, 2013/08/26 15:16:17]

Done.

[Isaac (away), 2013/09/02 23:38:58]

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

[rmistry, 2013/09/03 12:40:36]

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.

       if self.working_branch in branches:
         self._call_git(['branch', '-D', self.working_branch])
+    return self._get_revision()
 
+  def _get_revision(self):
+    """Gets the current revision from the local branch."""
+    return self._check_output_git(['rev-parse', 'HEAD']).strip()
+
   def apply_patch(self, patches, post_processors=None, verbose=False):
-    """Applies a patch on 'working_branch' and switch to it.
+    """Applies a patch on 'working_branch' and switches to it.
 
     Also commits the changes on the local branch.
 
@@ -597,8 +619,9 @@
     # trying again?
     if self.remote_branch:
       self._check_call_git(
-          ['checkout', '-b', self.working_branch,
-            '%s/%s' % (self.remote, self.remote_branch), '--quiet'])
+          ['checkout', '-b', self.working_branch, '-t', self.remote_branch,
+           '--quiet'])
+
     for index, p in enumerate(patches):
       stdout = []
       try:
@@ -667,20 +690,20 @@
     if verbose:
       cmd.append('--verbose')
     self._check_call_git(cmd)
-    # TODO(maruel): Weirdly enough they don't match, need to investigate.
-    #found_files = self._check_output_git(
-    #    ['diff', 'master', '--name-only']).splitlines(False)
-    #assert sorted(patches.filenames) == sorted(found_files), (
-    #    sorted(out), sorted(found_files))
+    found_files = self._check_output_git(
+        ['diff', '%s/%s' % (self.remote, self.remote_branch),
+         '--name-only']).splitlines(False)
+    assert sorted(patches.filenames) == sorted(found_files), (
+        sorted(patches.filenames), sorted(found_files))
 
   def commit(self, commit_message, user):
-    """Updates the commit message.
+    """Commits and Updates the commit message.
 
     Subclass needs to dcommit or push.
     """
     assert isinstance(commit_message, unicode)
     self._check_call_git(['commit', '--amend', '-m', commit_message])

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

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]

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'.

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

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.

[rmistry, 2013/08/26 15:16:17]

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?
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.

[rmistry, 2013/08/27 12:44:49]

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

-    return self._check_output_git(['rev-parse', 'HEAD']).strip()
+    return self._get_revision()
 
   def _check_call_git(self, args, **kwargs):
     kwargs.setdefault('cwd', self.project_path)
@@ -733,11 +756,21 @@
 class GitCheckout(GitCheckoutBase):
   """Git checkout implementation."""
   def _fetch_remote(self):
-    # git fetch is always verbose even with -q -q so redirect its output.
+    # git fetch is always verbose even with -q, so redirect its output.
     self._check_output_git(['fetch', self.remote, self.remote_branch],
                            timeout=FETCH_TIMEOUT)
 
+  def commit(self, commit_message, user):
+    """Updates the commit message and pushes."""
+    # Update the commit message and 'git commit' by calling the superclass.
+    rev = super(GitCheckout, self).commit(commit_message, user)
+    # Push to the remote repository.
+    self._check_call_git(
+        ['push', 'origin', '%s:%s' % (self.working_branch, self.remote_branch),
+         '--force', '--quiet'])
+    return rev

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

Also, it should switch back to the master branch and remove the working branch.

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

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]

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:17]

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

 
+
 class ReadOnlyCheckout(object):
   """Converts a checkout into a read-only one."""
   def __init__(self, checkout, post_processors=None):