From 720e010ba1b2fb75f5007b8b2bb139da91bce3a9 Mon Sep 17 00:00:00 2001 From: Edward Lesmes Date: Tue, 21 Jul 2020 15:54:18 +0000 Subject: [PATCH] Revert "bot_update: Refactor branch to detach and checkout." This reverts commit b99e61c58a40e77c7b84a2ab0d8caad2d3122659. Reason for revert: Doesn't work for refs/branch-heads/* Original change's description: > bot_update: Refactor branch to detach and checkout. > > Refactor _git_checkout to use information from > get_target_branch_and_revision to know what branch to checkout after > cloning. > > Bug: 1104182 > Change-Id: Ib3ba57ca0b6803f172b85121c2a4b123f17bfb8c > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2303291 > Reviewed-by: Josip Sokcevic > Commit-Queue: Edward Lesmes TBR=ehmaldonado@chromium.org,infra-scoped@luci-project-accounts.iam.gserviceaccount.com,sokcevic@google.com Change-Id: If51423df99fd9c164f8e42e0220ee2d9bc2a39f6 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 1104182 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2310293 Reviewed-by: Edward Lesmes Commit-Queue: Edward Lesmes --- .../bot_update/resources/bot_update.py | 80 +++++++++++-------- 1 file changed, 48 insertions(+), 32 deletions(-) diff --git a/recipes/recipe_modules/bot_update/resources/bot_update.py b/recipes/recipe_modules/bot_update/resources/bot_update.py index 7517a10bc..4e9c103c4 100755 --- a/recipes/recipe_modules/bot_update/resources/bot_update.py +++ b/recipes/recipe_modules/bot_update/resources/bot_update.py @@ -588,30 +588,54 @@ def get_total_disk_space(): return (total, free) -def get_target_branch_and_revision(solution_name, git_url, revisions): - solution_name = solution_name.strip('/') - configured = revisions.get(solution_name) or revisions.get(git_url) - - if configured is None or COMMIT_HASH_RE.match(configured): - # TODO(crbug.com/1104182): Get the default branch instead of assuming - # 'master'. - branch = 'origin/master' - revision = configured or 'HEAD' - return branch, revision - elif ':' in configured: - branch, revision = configured.split(':', 1) +def _get_target_branch_and_revision(solution_name, git_url, revisions): + normalized_name = solution_name.strip('/') + if normalized_name in revisions: + configured = revisions[normalized_name] + elif git_url in revisions: + configured = revisions[git_url] else: - branch = configured - revision = 'HEAD' + return 'master', 'HEAD' + + parts = configured.split(':', 1) + if len(parts) == 2: + # Support for "branch:revision" syntax. + return parts + if COMMIT_HASH_RE.match(configured): + return 'master', configured + return configured, 'HEAD' + + +def get_target_pin(solution_name, git_url, revisions): + """Returns revision to be checked out if it is pinned, else None.""" + _, revision = _get_target_branch_and_revision( + solution_name, git_url, revisions) + if COMMIT_HASH_RE.match(revision): + return revision + return None - if branch.startswith('refs/heads/'): - branch = 'refs/remotes/origin/' + branch[len('refs/heads/'):] - elif branch.startswith('refs/branch-heads/'): - branch = 'refs/remotes/branch-heads/' + branch[len('refs/branch-heads/'):] - elif not branch.startswith(('refs/', 'origin/')): - branch = 'origin/' + branch - return branch, revision +def force_solution_revision(solution_name, git_url, revisions, cwd): + branch, revision = _get_target_branch_and_revision( + solution_name, git_url, revisions) + if revision and revision.upper() != 'HEAD': + treeish = revision + else: + # TODO(machenbach): This won't work with branch-heads, as Gerrit's + # destination branch would be e.g. refs/branch-heads/123. But here + # we need to pass refs/remotes/branch-heads/123 to check out. + # This will also not work if somebody passes a local refspec like + # refs/heads/master. It needs to translate to refs/remotes/origin/master + # first. See also https://crbug.com/740456 . + if branch.startswith(('refs/', 'origin/')): + treeish = branch + else: + treeish = 'origin/' + branch + + # Note that -- argument is necessary to ensure that git treats `treeish` + # argument as revision or ref, and not as a file/directory which happens to + # have the exact same name. + git('checkout', '--force', treeish, '--', cwd=cwd) def _has_in_git_cache(revision_sha1, refs, git_cache_dir, url): @@ -696,10 +720,8 @@ def _git_checkout(sln, sln_dir, revisions, refs, no_fetch_tags, git_cache_dir, 'GIT_TRACE_PERFORMANCE': 'true', } - branch, revision = get_target_branch_and_revision(name, url, revisions) - pin = revision if COMMIT_HASH_RE.match(revision) else None - # Step 1: populate/refresh cache, if necessary. + pin = get_target_pin(name, url, revisions) if not pin: # Refresh only once. git(*populate_cmd, env=env) @@ -753,10 +775,7 @@ def _git_checkout(sln, sln_dir, revisions, refs, no_fetch_tags, git_cache_dir, # to master in contrast with the non-clone case, which results in a # detached HEAD. This prevents fetching the default branch so detach the # HEAD after cloning. - # Note that the '--' argument is needed to ensure that git treats - # 'pin or branch' as revision or ref, and not as file/directory which - # happens to have the exact same name. - git('checkout', pin or branch, '--detach', '--', cwd=sln_dir) + git('checkout', 'HEAD', '--detach', cwd=sln_dir) _git_disable_gc(sln_dir) else: _git_disable_gc(sln_dir) @@ -773,10 +792,7 @@ def _git_checkout(sln, sln_dir, revisions, refs, no_fetch_tags, git_cache_dir, if sys.platform.startswith('win'): _maybe_break_locks(sln_dir, tries=3) - # Note that the '--' argument is needed to ensure that git treats - # 'pin or branch' as revision or ref, and not as file/directory which - # happens to have the exact same name. - git('checkout', '--force', pin or branch, '--', cwd=sln_dir) + force_solution_revision(name, url, revisions, sln_dir) git('clean', '-dff', cwd=sln_dir) return except SubprocessFailed as e: