Another attempt: gclient: delete mismatching checkouts

gclient_scm.py

Index: gclient_scm.py
diff --git a/gclient_scm.py b/gclient_scm.py
index af48b5f7f33b5984667abe2e4887f9f224e59737..584b7224d621eff244f55c3803b0cb13240443dd 100644
--- a/gclient_scm.py
+++ b/gclient_scm.py
@@ -297,9 +297,7 @@ class GitWrapper(SCMWrapper):
       # hash is also a tag, only make a distinction at checkout
       rev_type = "hash"
 
-    if (not os.path.exists(self.checkout_path) or
-        (os.path.isdir(self.checkout_path) and
-         not os.path.exists(os.path.join(self.checkout_path, '.git')))):
+    if (not os.path.exists(self.checkout_path)):
       self._Clone(revision, url, options)
       self.UpdateSubmoduleConfig()
       if file_list is not None:
@@ -318,12 +316,29 @@ class GitWrapper(SCMWrapper):
       return self._Capture(['rev-parse', '--verify', 'HEAD'])
 
     if not os.path.exists(os.path.join(self.checkout_path, '.git')):
-      raise gclient_utils.Error('\n____ %s%s\n'
-                                '\tPath is not a git repo. No .git dir.\n'
-                                '\tTo resolve:\n'
-                                '\t\trm -rf %s\n'
-                                '\tAnd run gclient sync again\n'
-                                % (self.relpath, rev_str, self.relpath))
+      if options.force:
+        # Delete and re-sync.
+        print('_____ Conflicting directory found in %s. Removing.'
+              % self.checkout_path)
+        gclient_utils.rmtree(self.checkout_path)
+        self._Clone(revision, url, options)
+        self.UpdateSubmoduleConfig()
+        if file_list is not None:
+          files = self._Capture(['ls-files']).splitlines()
+          file_list.extend([os.path.join(self.checkout_path, f) for f in files])
+        if not verbose:
+          # Make the output a little prettier. It's nice to have some whitespace
+          # between projects when cloning.
+          print('')
+        return self._Capture(['rev-parse', '--verify', 'HEAD'])

[iannucci, 2014/03/05 22:04:07]

I really dislike this pattern in gclient (although it's very prevalent). Can't
this logic move into the 'if not exists' block above?

if not exists(path) or if not exists(path / .git):
  if not exists(path / .git):
    if force:
      rm_rf(path)
    else:
      raise Error()
  # clone fresh

It would remove the duplicated contents of this block, which is what I'm worried
about.

[borenet, 2014/03/06 14:27:19]

Done. It seems like this whole block is not needed, right?  The above verifies
that there's a .git directory and either clones or errors our, so we shouldn't
get to this point if there's no .git directory.

[borenet, 2014/03/06 18:04:49]

I'm a little concerned about adding this logic above the early return for
unmanaged checkouts.  This will cause an unexpected error if, for example,
.gclient has an unmanaged git checkout listed but there's actually an SVN
checkout sitting there.  That's a bizarre case, but it's roughly equivalent to
one we've seen before.

[iannucci, 2014/03/07 20:31:19]

Hm... Couldn't we just check the managed boolean above as well? Though maybe
it's ok because we're hiding behind --force?

I also think that an unmanaged gclient svn checkout is HIGHLY unlikely.

[borenet, 2014/03/07 21:50:12]

I think you're right, but I've been surprised before :)
At any rate, the early return itself is going to fail if there's an unmanaged,
non-git checkout here, due to the use of "git rev-parse --verify HEAD".  So I'm
no longer worried about it.

+      else:
+        raise gclient_utils.Error('\n____ %s%s\n'
+                                  '\tPath is not a git repo. No .git dir.\n'
+                                  '\tTo resolve:\n'
+                                  '\t\trm -rf %s\n'
+                                  '\tAnd run gclient sync again\n'
+                                  '\tOr run with --force\n'
+                                  % (self.relpath, rev_str, self.relpath))
 
     # See if the url has changed (the unittests use git://foo for the url, let
     # that through).
@@ -334,7 +349,7 @@ class GitWrapper(SCMWrapper):
     # Skip url auto-correction if remote.origin.gclient-auto-fix-url is set.
     # This allows devs to use experimental repos which have a different url
     # but whose branch(s) are the same as official repos.
-    if (current_url != url and
+    if (current_url.rstrip('/') != url.rstrip('/') and
         url != 'git://foo' and
         subprocess2.capture(
             ['git', 'config', 'remote.%s.gclient-auto-fix-url' % self.remote],
@@ -999,12 +1014,7 @@ class SVNWrapper(SCMWrapper):
     Raises:
       Error: if can't get URL for relative path.
     """
-    # Only update if git or hg is not controlling the directory.
-    git_path = os.path.join(self.checkout_path, '.git')
-    if os.path.exists(git_path):
-      print('________ found .git directory; skipping %s' % self.relpath)
-      return
-
+    # Only update if hg is not controlling the directory.
     hg_path = os.path.join(self.checkout_path, '.hg')
     if os.path.exists(hg_path):
       print('________ found .hg directory; skipping %s' % self.relpath)
@@ -1035,14 +1045,26 @@ class SVNWrapper(SCMWrapper):
       forced_revision = False
       rev_str = ''
 
-    # Get the existing scm url and the revision number of the current checkout.
     exists = os.path.exists(self.checkout_path)
     if exists and managed:
+      # Git is only okay if it's a git-svn checkout of the right repo.
+      if scm.GIT.IsGitSvn(self.checkout_path):
+        remote_url = scm.GIT.Capture(['config', '--local', '--get',
+                                      'svn-remote.svn.url'],
+                                     cwd=self.checkout_path).rstrip()
+        if remote_url.rstrip('/') == base_url.rstrip('/'):
+          print('\n_____ %s looks like a git-svn checkout. Skipping.'
+                % self.relpath)
+          return # TODO(borenet): Get the svn revision number?

[iannucci, 2014/03/07 20:31:19]

Can you try this scenario with --json-output?  It /should/ work, but I'm not
sure.

[borenet, 2014/03/07 21:50:12]

I tried this with a checkout of skia:
$ git svn clone -r 13710 https://skia.googlecode.com/svn/trunk
$ /hacked/gclient config https://skia.googlecode.com/svn/trunk --unmanaged
$ /hacked/gclient sync --output-json json_output.txt

(succeeds)

+
+    # Get the existing scm url and the revision number of the current checkout.
+    if exists and managed:
       try:
         from_info = scm.SVN.CaptureLocalInfo(
             [], os.path.join(self.checkout_path, '.'))
       except (gclient_utils.Error, subprocess2.CalledProcessError):
-        if options.reset and options.delete_unversioned_trees:
+        if (options.force or
+            (options.reset and options.delete_unversioned_trees)):
           print 'Removing troublesome path %s' % self.checkout_path
           gclient_utils.rmtree(self.checkout_path)
           exists = False
@@ -1172,7 +1194,7 @@ class SVNWrapper(SCMWrapper):
       revision = str(from_info_live['Revision'])
       rev_str = ' at %s' % revision
 
-    if from_info['URL'] != base_url:
+    if from_info['URL'].rstrip('/') != base_url.rstrip('/'):
       # The repository url changed, need to switch.
       try:
         to_info = scm.SVN.CaptureRemoteInfo(url)