bot_update: call gclient sync after checking out a gerrit ref

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 df60837d5e4ae606a00cfc9199c66bf121ae8198..b8b3f0060b9d43d87a88e47faec705accd20852c 100755
--- a/recipe_modules/bot_update/resources/bot_update.py
+++ b/recipe_modules/bot_update/resources/bot_update.py
@@ -1338,8 +1338,8 @@ def ensure_checkout(solutions, revisions, first_sln, target_os, target_os_only,
   # 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.
@@ -1366,6 +1366,7 @@ def ensure_checkout(solutions, revisions, first_sln, target_os, target_os_only,
                          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: