bot_update: call gclient sync after checking out a gerrit ref

recipe_modules/bot_update/resources/bot_update.py

 #!/usr/bin/env python
 # Copyright 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 # TODO(hinoka): Use logging.
 
 import cStringIO
 import codecs
 import collections
@@ skipping 1320 matching lines @@
     # Run deps2git if there is a DEPS change after the last .DEPS.git commit.
     for solution in solutions:
       ensure_deps2git(solution, shallow, git_cache_dir)
 
   # Ensure our build/ directory is set up with the correct .gclient file.
   gclient_configure(solutions, target_os, target_os_only, git_cache_dir)
 
   # Let gclient do the DEPS syncing.
   # The branch-head refspec is a special case because its possible Chrome
   # src, which contains the branch-head refspecs, is DEPSed in.
-  gclient_output = gclient_sync(buildspec or BRANCH_HEADS_REFSPEC in refs,
-                                shallow)
+  with_branch_heads = (buildspec or BRANCH_HEADS_REFSPEC in refs)
+  gclient_output = gclient_sync(with_branch_heads, shallow)
 
   # Now that gclient_sync has finished, we should revert any .DEPS.git so that
   # presubmit doesn't complain about it being modified.
   if (not buildspec and
       git('ls-files', '.DEPS.git', cwd=first_sln).strip()):
     git('checkout', 'HEAD', '--', '.DEPS.git', cwd=first_sln)
 
   if buildspec and runhooks:
     # Run gclient runhooks if we're on an official builder.
     # TODO(hinoka): Remove this when the official builders run their own
     #               runhooks step.
     gclient_runhooks(gyp_env)
 
   # Finally, ensure that all DEPS are pinned to the correct revision.
   dir_names = [sln['name'] for sln in solutions]
   ensure_deps_revisions(gclient_output.get('solutions', {}),
                         dir_names, revisions)
   # Apply the rest of the patch here (sans DEPS)
   if patches:
     apply_svn_patch(patch_root, patches, blacklist=already_patched)
   elif issue:
     apply_rietveld_issue(issue, patchset, patch_root, rietveld_server,
                          revision_mapping, git_ref, apply_issue_email_file,
                          apply_issue_key_file, blacklist=already_patched)

[tandrii(chromium), 2016/04/20 11:41:19]

so, is this normally calling gclient sync automatically?
i frankly don't understand why the difference between gerrit and rietveld :(

[Paweł Hajdan Jr., 2016/04/20 11:48:52]

My understanding is that it isn't.

However, there's additional patching logic on original lines 1314-1328 that is
not being performed for gerrit.
With gerrit, we're not really patching anything. We're checking out the ref
(which is more like overwriting than patching). That means a gerrit ref can only
apply to one repo in my understanding. This is different with unified text
diffs, which might patch multiple repos, and are applied on top of existing
sources.

My reasoning is, eventually we'd only support gerrit refs (possible a set of
refs for each repo), which should simplify this code.

Does that make sense?

   elif gerrit_ref:
     apply_gerrit_ref(gerrit_repo, gerrit_ref, patch_root, gerrit_reset)
+    gclient_sync(with_branch_heads, shallow)

[Yoshisato Yanagisawa, 2016/04/21 02:47:33]

For our use case, this might revert the changes by git fetch.  I think second
gclient sync should sync to FETCH_HEAD not branch head, and because of that
reason, apply_gerrit_ref might be the good place to run second gclient sync.  I
suggest to add ii. step below to apply_gerrit_ref.
i. git fetch to apply patch
ii. if git diff --name-only has DEPS, then gclient sync to FETCH_HEAD
iii. git reset --soft if without gerrit_reset.

FYI, my workaround implementation is like:
1. bot_update with no_gerrit_reset to make got_revision have FETCH_HEAD
revision.
2. gclient sync to got_revision property.
https://goto.google.com/gmviv
I am afraid that this change might also break my workaround because got_revision
might be overridden by HEAD.

[Paweł Hajdan Jr., 2016/04/21 11:36:29]

gclient sync shouldn't touch main solution when it's unmanaged, and I was
testing with goma where it was unmanaged

Does that make sense to you? Are we good to land?

If not, what specific changes do you suggest? We can always make a specific test
if you want, and in the worst case revert the CL.

[Yoshisato Yanagisawa, 2016/04/22 01:05:20]

Yes, I seems to misunderstand the behavior.  I thought --revision option is also
applied to client revision to "gclient sync", but it seems not.

I also followed the same step this + Ryan's suggestion bot_update do (i.e. run
gclient sync only after apply_gerrit_ref), and confirmed it works.

 
   # Reset the deps_file point in the solutions so that hooks get run properly.
   for sln in solutions:
     sln['deps_file'] = sln.get('deps_file', 'DEPS').replace('.DEPS.git', 'DEPS')
   gclient_configure(solutions, target_os, target_os_only, git_cache_dir)
 
   return gclient_output
 
 
 def parse_revisions(revisions, root):
@@ skipping 376 matching lines @@
   except Exception:
     # Unexpected failure.
     emit_flag(options.flag_file)
     raise
   else:
     emit_flag(options.flag_file)
 
 
 if __name__ == '__main__':
   sys.exit(main())