project: don't use dest-branch or tags as SHA-1 upstream fallback https://gerrit-review.googlesource.com/614762 resolves a fallback upstream for a SHA-1-pinned project that has no explicit upstream, so --current-branch can narrow the fetch to one branch instead of fetching all heads. It drew that fallback from dest-branch and from the manifest <default> upstream, dest-branch, and revision. dest-branch and tags are not valid fallback candidates: * dest-branch is the review destination for upload, not a fetch source. * Tags are meant to be immutable snapshots of a single tagged commit, and fetching it cannot retrieve an arbitrary pinned SHA-1. Restrict the fallback to branch heads resolved from the manifest upstream and revision defaults, and drop dest-branch entirely. Bug: 541240657 Bug: 544041102 Change-Id: I89ae86891ee166f4c340553ac5a9068efcc18461 Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/622541 Tested-by: Gavin Mak <gavinmak@google.com> Commit-Queue: Gavin Mak <gavinmak@google.com> Reviewed-by: Brian Gan <brgan@google.com>
diff --git a/project.py b/project.py index 1c3d8bf..a73c7d4 100644 --- a/project.py +++ b/project.py
@@ -2936,19 +2936,16 @@ return True def _GetUpstreamFallback(self) -> Optional[str]: - """Resolve a fallback upstream ref when revisionExpr is a SHA-1.""" + """Resolve a fallback upstream branch when revisionExpr is a SHA-1. + + Returns manifest default upstream or revision if it names a branch, + or None to fall back to fetching all heads. + """ default = self.manifest.default - candidates = [self.dest_branch] - if default: - candidates.extend( - ( - default.upstreamExpr, - default.destBranchExpr, - default.revisionExpr, - ) - ) - for cand in candidates: - if cand and not IsId(cand): + if not default: + return None + for cand in (default.upstreamExpr, default.revisionExpr): + if cand and not IsId(cand) and not cand.startswith(R_TAGS): return cand return None
diff --git a/tests/test_project.py b/tests/test_project.py index 7d77ed8..e716b1a 100644 --- a/tests/test_project.py +++ b/tests/test_project.py
@@ -1612,14 +1612,16 @@ self.assertTrue(res) mock_git_cmd.assert_not_called() - def test_remote_fetch_sha1_upstream_fallback(self) -> None: - """Test _RemoteFetch resolves upstream fallback for SHA-1 revisions.""" + def test_remote_fetch_sha1_dest_branch_not_fetched(self) -> None: + """Test _RemoteFetch ignores dest-branch for SHA-1 revisions.""" sha = "4f8a3c0000000000000000000000000000000000" with utils_for_test.TempGitTree() as tempdir: proj = self._get_project(tempdir, revisionExpr=sha) - proj._CheckForImmutableRevision.side_effect = [False, True] + proj._CheckForImmutableRevision.return_value = False proj.upstream = None proj.dest_branch = "my-dest-branch" + proj.manifest.default.upstreamExpr = None + proj.manifest.default.revisionExpr = None mock_remote = mock.MagicMock() mock_remote.name = "origin" @@ -1643,14 +1645,12 @@ self.assertTrue(res) mock_git_cmd.assert_called_once() cmd_args = mock_git_cmd.call_args[0][1] - self.assertIn( + self.assertIn("+refs/heads/*:refs/remotes/origin/*", cmd_args) + self.assertNotIn( "+refs/heads/my-dest-branch:" "refs/remotes/origin/my-dest-branch", cmd_args, ) - self.assertNotIn( - "+refs/heads/*:refs/remotes/origin/*", cmd_args - ) def test_remote_fetch_sha1_manifest_default_fallback(self) -> None: """Test _RemoteFetch upstream fallback from manifest defaults.""" @@ -1693,21 +1693,23 @@ "+refs/heads/*:refs/remotes/origin/*", cmd_args ) - def test_remote_fetch_sha1_tag_fallback(self) -> None: - """Test _RemoteFetch resolves upstream fallback to tag correctly.""" + def test_remote_fetch_sha1_tag_not_fetched(self) -> None: + """Test _RemoteFetch ignores tag defaults for SHA-1 revisions.""" sha = "4f8a3c0000000000000000000000000000000000" with utils_for_test.TempGitTree() as tempdir: proj = self._get_project(tempdir, revisionExpr=sha) - proj._CheckForImmutableRevision.side_effect = [False, True] + proj._CheckForImmutableRevision.return_value = False proj.upstream = None - proj.dest_branch = "refs/tags/v1.0" + proj.dest_branch = None + proj.manifest.default.upstreamExpr = None + proj.manifest.default.revisionExpr = "refs/tags/v1.0" mock_remote = mock.MagicMock() mock_remote.name = "origin" def _to_local(r: str) -> str: - if r.startswith("refs/tags/"): - return "refs/tags/" + r[10:] + if r.startswith("refs/heads/"): + return "refs/remotes/origin/" + r[11:] return r mock_remote.ToLocal.side_effect = _to_local @@ -1724,11 +1726,9 @@ self.assertTrue(res) mock_git_cmd.assert_called_once() cmd_args = mock_git_cmd.call_args[0][1] - self.assertIn("tag", cmd_args) - self.assertIn("v1.0", cmd_args) - self.assertNotIn( - "+refs/heads/*:refs/remotes/origin/*", cmd_args - ) + self.assertIn("+refs/heads/*:refs/remotes/origin/*", cmd_args) + self.assertNotIn("tag", cmd_args) + self.assertNotIn("v1.0", cmd_args) def test_remote_fetch_sha1_metaproject_without_manifest_xml(self) -> None: """Test MetaProject _RemoteFetch with SHA-1 fetches all branches.""" @@ -1785,6 +1785,105 @@ proj.manifest.default = None self.assertIsNone(proj._GetUpstreamFallback()) + def test_get_upstream_fallback_ignores_dest_branch(self) -> None: + """dest_branch alone yields no fallback upstream.""" + with utils_for_test.TempGitTree() as tempdir: + proj = self._get_project( + tempdir, + revisionExpr="0123456789abcdef0123456789abcdef01234567", + ) + proj.dest_branch = "main" + proj.manifest.default.upstreamExpr = None + proj.manifest.default.revisionExpr = None + self.assertIsNone(proj._GetUpstreamFallback()) + + def test_get_upstream_fallback_ignores_tag(self) -> None: + """A tag default is not returned as a fallback upstream.""" + with utils_for_test.TempGitTree() as tempdir: + proj = self._get_project( + tempdir, + revisionExpr="0123456789abcdef0123456789abcdef01234567", + ) + proj.dest_branch = None + proj.manifest.default.upstreamExpr = None + proj.manifest.default.revisionExpr = "refs/tags/v1.0" + self.assertIsNone(proj._GetUpstreamFallback()) + + def test_get_upstream_fallback_uses_revision_branch(self) -> None: + """A branch revision default is a valid fallback upstream.""" + with utils_for_test.TempGitTree() as tempdir: + proj = self._get_project( + tempdir, + revisionExpr="0123456789abcdef0123456789abcdef01234567", + ) + proj.dest_branch = "dest-should-be-ignored" + proj.manifest.default.upstreamExpr = None + proj.manifest.default.revisionExpr = "main" + self.assertEqual(proj._GetUpstreamFallback(), "main") + + def test_remote_fetch_sha1_missing_upstream_falls_back_to_all_heads( + self, + ) -> None: + """A missing upstream remote branch falls back to fetching all heads.""" + sha = "4f8a3c0000000000000000000000000000000000" + with utils_for_test.TempGitTree() as tempdir: + proj = self._get_project(tempdir, revisionExpr=sha) + proj._CheckForImmutableRevision = mock.MagicMock(return_value=False) + proj.upstream = None + proj.dest_branch = None + proj.manifest.default.upstreamExpr = None + proj.manifest.default.revisionExpr = "master" + + mock_remote = mock.MagicMock() + mock_remote.name = "origin" + + def _to_local(r: str) -> str: + if r.startswith("refs/heads/"): + return "refs/remotes/origin/" + r[11:] + return r + + mock_remote.ToLocal.side_effect = _to_local + mock_remote.PreConnectFetch.return_value = True + proj.GetRemote = mock.MagicMock(return_value=mock_remote) + + fetched_specs: List[List[str]] = [] + + def _fake_git_command( + _proj: Optional[project.Project], + cmd: List[str], + **kwargs, + ) -> mock.MagicMock: + fetched_specs.append(cmd) + inst = mock.MagicMock() + if "+refs/heads/master:refs/remotes/origin/master" in cmd: + inst.Wait.return_value = 128 + inst.stdout = ( + "fatal: couldn't find remote ref refs/heads/master\n" + ) + else: + inst.Wait.return_value = 0 + inst.stdout = "" + return inst + + with mock.patch( + "project.GitCommand", side_effect=_fake_git_command + ): + res = proj._RemoteFetch(current_branch_only=True) + + self.assertTrue(res) + self.assertTrue( + any( + "+refs/heads/master:refs/remotes/origin/master" in c + for c in fetched_specs + ) + ) + self.assertTrue( + any( + "+refs/heads/*:refs/remotes/origin/*" in c + for c in fetched_specs + ) + ) + class GetEnvVarsTests(unittest.TestCase): """Tests for GetEnvVars project environment variable generation."""