Teach bot_update to remove partially deleted git repos.

recipe_modules/bot_update/resources/bot_update.py

Index: recipe_modules/bot_update/resources/bot_update.py
diff --git a/recipe_modules/bot_update/resources/bot_update.py b/recipe_modules/bot_update/resources/bot_update.py
index f4a307b25fbef2b9c4016c320cc98cc0d63df010..21d34a1bdff52f774e12ffc065fb78d47ebeb68b 100755
--- a/recipe_modules/bot_update/resources/bot_update.py
+++ b/recipe_modules/bot_update/resources/bot_update.py
@@ -7,7 +7,6 @@
 
 import cStringIO
 import codecs
-import collections
 import copy
 import ctypes
 import json
@@ -16,7 +15,6 @@ import os
 import pprint
 import random
 import re
-import socket
 import subprocess
 import sys
 import tempfile
@@ -299,7 +297,9 @@ def remove(target):
   dead_folder = path.join(BUILDER_DIR, 'build.dead')
   if not path.exists(dead_folder):
     os.makedirs(dead_folder)
-  os.rename(target, path.join(dead_folder, uuid.uuid4().hex))
+  dest = path.join(dead_folder, uuid.uuid4().hex)
+  print 'Removing %s => %s' % (target, dest)

[dnj, 2016/09/15 21:44:34]

nit: I think this is a bit confusing, since this isn't actually removing things
here, and removals don't normally include two paths.

Maybe "Marking for removal: %s => %s" ?

[Vadim Sh., 2016/09/15 21:56:26]

Done.

+  os.rename(target, dest)
 
 
 def ensure_no_checkout(dir_names):
@@ -458,6 +458,11 @@ def force_revision(folder_name, revision):
     git('checkout', '--force', ref, cwd=folder_name)
 
 
+def is_broken_repo_dir(repo_dir):
+  # Treat absence of 'config' as a signal of a partially deleted repo.
+  return not path.exists(os.path.join(repo_dir, '.git', 'config'))

[dnj, 2016/09/15 21:44:34]

This is a good check. WDYT about adding something like:

... or $("git -C <repo_dir> status").return_code != 0

This will actually let Git try and run in each directory, maybe detecting other
types of corruption.

[tandrii(chromium), 2016/09/15 21:49:43]

+1, but could be separate CL.

[Vadim Sh., 2016/09/15 21:56:26]

I'm afraid running 'git', it can complicate life, e.g. it can discover parent
'.git' (I've seen it doing that with some broken checkouts). And --git-dir is
magic.

I really don't like modifying this script beyond absolute minimum. Not unless I
can reliably verify my modification work, without breaking all bots in the
process. And I can't. So I'll be modifying it in absolute minimum way.

[dnj, 2016/09/15 22:03:12]

I think we could ensure this doesn't happen with:
git_dir = 
git(
    '--git-dir', os.path.join(repo_dir, '.git'),
    '--work-tree', repo_dir,
    'status')
Fair enough. It'd be nice if we had some way to actually canary this :/

+
+
 def git_checkout(solutions, revisions, shallow, refs, git_cache_dir):
   build_dir = os.getcwd()
   # Before we do anything, break all git_cache locks.
@@ -496,6 +501,12 @@ def git_checkout(solutions, revisions, shallow, refs, git_cache_dir):
           'clone', '--no-checkout', '--local', '--shared', mirror_dir, sln_dir)
 
       try:
+        # If repo deletion was aborted midway, it may have left .git in broken
+        # state.
+        if path.exists(sln_dir) and is_broken_repo_dir(sln_dir):
+          print 'Git repo %s appears to be broken, removing it' % sln_dir
+          remove(sln_dir)
+
         if not path.isdir(sln_dir):
           git(*clone_cmd)
         else:
@@ -1044,7 +1055,7 @@ def main():
   # Print a helpful message to tell developers whats going on with this step.
   print_debug_info()
 
-  # Parse, munipulate, and print the gclient solutions.
+  # Parse, manipulate, and print the gclient solutions.
   specs = {}
   exec(options.specs, specs)
   orig_solutions = specs.get('solutions', [])