From fa8d939c8f6a3d25d9a203f28b16a71d891dcc1c Mon Sep 17 00:00:00 2001 From: LaMont Jones Date: Wed, 2 Nov 2022 22:01:29 +0000 Subject: [PATCH] sync: clear preciousObjects when set in error. If this is a project that is not using object sharing (there is only one copy of the remote project) then clear preciousObjects. To override this for a project, run: git config --replace-all repo.preservePreciousObjects true Change-Id: If3ea061c631c5ecd44ead84f68576012e2c7405c Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/350235 Reviewed-by: Jonathan Nieder Tested-by: LaMont Jones --- git_config.py | 4 +- project.py | 2 +- subcmds/sync.py | 96 +++++++++++++++++++++++++++++--------- tests/test_subcmds_sync.py | 55 +++++++++++++++++++--- 4 files changed, 126 insertions(+), 31 deletions(-) diff --git a/git_config.py b/git_config.py index 98cade32..94378e9a 100644 --- a/git_config.py +++ b/git_config.py @@ -219,8 +219,8 @@ class GitConfig(object): """Set the value(s) for a key. Only this configuration file is modified. - The supplied value should be either a string, - or a list of strings (to store multiple values). + The supplied value should be either a string, or a list of strings (to + store multiple values), or None (to delete the key). """ key = _key(name) diff --git a/project.py b/project.py index b975b72a..9d7476b4 100644 --- a/project.py +++ b/project.py @@ -59,7 +59,7 @@ MAXIMUM_RETRY_SLEEP_SEC = 3600.0 # +-10% random jitter is added to each Fetches retry sleep duration. RETRY_JITTER_PERCENT = 0.1 -# Whether to use alternates. +# Whether to use alternates. Switching back and forth is *NOT* supported. # TODO(vapier): Remove knob once behavior is verified. _ALTERNATES = os.environ.get('REPO_USE_ALTERNATES') == '1' diff --git a/subcmds/sync.py b/subcmds/sync.py index 082b254f..83c9ad36 100644 --- a/subcmds/sync.py +++ b/subcmds/sync.py @@ -755,33 +755,87 @@ later is required to fix a server side protocol bug. shutil.copy(os.path.join(pack_dir, fname), bak_fname + '.tmp') shutil.move(bak_fname + '.tmp', bak_fname) + @staticmethod + def _GetPreciousObjectsState(project: Project, opt): + """Get the preciousObjects state for the project. + + Args: + project (Project): the project to examine, and possibly correct. + opt (optparse.Values): options given to sync. + + Returns: + Expected state of extensions.preciousObjects: + False: Should be disabled. (not present) + True: Should be enabled. + """ + if project.use_git_worktrees: + return False + projects = project.manifest.GetProjectsWithName(project.name, + all_manifests=True) + if len(projects) == 1: + return False + relpath = project.RelPath(local=opt.this_manifest_only) + if len(projects) > 1: + # Objects are potentially shared with another project. + # See the logic in Project.Sync_NetworkHalf regarding UseAlternates. + # - When False, shared projects share (via symlink) + # .repo/project-objects/{PROJECT_NAME}.git as the one-and-only objects + # directory. All objects are precious, since there is no project with a + # complete set of refs. + # - When True, shared projects share (via info/alternates) + # .repo/project-objects/{PROJECT_NAME}.git as an alternate object store, + # which is written only on the first clone of the project, and is not + # written subsequently. (When Sync_NetworkHalf sees that it exists, it + # makes sure that the alternates file points there, and uses a + # project-local .git/objects directory for all syncs going forward. + # We do not support switching between the options. The environment + # variable is present for testing and migration only. + return not project.UseAlternates + print(f'\r{relpath}: project not found in manifest.', file=sys.stderr) + return False + + def _RepairPreciousObjectsState(self, project: Project, opt): + """Correct the preciousObjects state for the project. + + Args: + project (Project): the project to examine, and possibly correct. + opt (optparse.Values): options given to sync. + """ + expected = self._GetPreciousObjectsState(project, opt) + actual = project.config.GetBoolean('extensions.preciousObjects') or False + relpath = project.RelPath(local = opt.this_manifest_only) + + if (expected != actual and + not project.config.GetBoolean('repo.preservePreciousObjects')): + # If this is unexpected, log it and repair. + Trace(f'{relpath} expected preciousObjects={expected}, got {actual}') + if expected: + if not opt.quiet: + print('\r%s: Shared project %s found, disabling pruning.' % + (relpath, project.name)) + if git_require((2, 7, 0)): + project.EnableRepositoryExtension('preciousObjects') + else: + # This isn't perfect, but it's the best we can do with old git. + print('\r%s: WARNING: shared projects are unreliable when using ' + 'old versions of git; please upgrade to git-2.7.0+.' + % (relpath,), + file=sys.stderr) + project.config.SetString('gc.pruneExpire', 'never') + else: + if not opt.quiet: + print(f'\r{relpath}: not shared, disabling pruning.') + project.config.SetString('extensions.preciousObjects', None) + project.config.SetString('gc.pruneExpire', None) + def _GCProjects(self, projects, opt, err_event): pm = Progress('Garbage collecting', len(projects), delay=False, quiet=opt.quiet) pm.update(inc=0, msg='prescan') tidy_dirs = {} for project in projects: - # Make sure pruning never kicks in with shared projects that do not use - # alternates to avoid corruption. - if (not project.use_git_worktrees and - len(project.manifest.GetProjectsWithName(project.name, all_manifests=True)) > 1): - if project.UseAlternates: - # Undo logic set by previous versions of repo. - project.config.SetString('extensions.preciousObjects', None) - project.config.SetString('gc.pruneExpire', None) - else: - if not opt.quiet: - print('\r%s: Shared project %s found, disabling pruning.' % - (project.relpath, project.name)) - if git_require((2, 7, 0)): - project.EnableRepositoryExtension('preciousObjects') - else: - # This isn't perfect, but it's the best we can do with old git. - print('\r%s: WARNING: shared projects are unreliable when using old ' - 'versions of git; please upgrade to git-2.7.0+.' - % (project.relpath,), - file=sys.stderr) - project.config.SetString('gc.pruneExpire', 'never') + self._RepairPreciousObjectsState(project, opt) + project.config.SetString('gc.autoDetach', 'false') # Only call git gc once per objdir, but call pack-refs for the remainder. if project.objdir not in tidy_dirs: diff --git a/tests/test_subcmds_sync.py b/tests/test_subcmds_sync.py index aad713f2..13f3f873 100644 --- a/tests/test_subcmds_sync.py +++ b/tests/test_subcmds_sync.py @@ -11,9 +11,9 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. - """Unittests for the subcmds/sync.py module.""" +import unittest from unittest import mock import pytest @@ -21,17 +21,14 @@ import pytest from subcmds import sync -@pytest.mark.parametrize( - 'use_superproject, cli_args, result', - [ +@pytest.mark.parametrize('use_superproject, cli_args, result', [ (True, ['--current-branch'], True), (True, ['--no-current-branch'], True), (True, [], True), (False, ['--current-branch'], True), (False, ['--no-current-branch'], False), (False, [], None), - ] -) +]) def test_get_current_branch_only(use_superproject, cli_args, result): """Test Sync._GetCurrentBranchOnly logic. @@ -41,5 +38,49 @@ def test_get_current_branch_only(use_superproject, cli_args, result): cmd = sync.Sync() opts, _ = cmd.OptionParser.parse_args(cli_args) - with mock.patch('git_superproject.UseSuperproject', return_value=use_superproject): + with mock.patch('git_superproject.UseSuperproject', + return_value=use_superproject): assert cmd._GetCurrentBranchOnly(opts, cmd.manifest) == result + + +class GetPreciousObjectsState(unittest.TestCase): + """Tests for _GetPreciousObjectsState.""" + + def setUp(self): + """Common setup.""" + self.cmd = sync.Sync() + self.project = p = mock.MagicMock(use_git_worktrees=False, + UseAlternates=False) + p.manifest.GetProjectsWithName.return_value = [p] + + self.opt = mock.Mock(spec_set=['this_manifest_only']) + self.opt.this_manifest_only = False + + def test_worktrees(self): + """False for worktrees.""" + self.project.use_git_worktrees = True + self.assertFalse(self.cmd._GetPreciousObjectsState(self.project, self.opt)) + + def test_not_shared(self): + """Singleton project.""" + self.assertFalse(self.cmd._GetPreciousObjectsState(self.project, self.opt)) + + def test_shared(self): + """Shared project.""" + self.project.manifest.GetProjectsWithName.return_value = [ + self.project, self.project + ] + self.assertTrue(self.cmd._GetPreciousObjectsState(self.project, self.opt)) + + def test_shared_with_alternates(self): + """Shared project, with alternates.""" + self.project.manifest.GetProjectsWithName.return_value = [ + self.project, self.project + ] + self.project.UseAlternates = True + self.assertFalse(self.cmd._GetPreciousObjectsState(self.project, self.opt)) + + def test_not_found(self): + """Project not found in manifest.""" + self.project.manifest.GetProjectsWithName.return_value = [] + self.assertFalse(self.cmd._GetPreciousObjectsState(self.project, self.opt))