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 <jrn@google.com>
Tested-by: LaMont Jones <lamontjones@google.com>

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,
+       The supplied value should be either a string, or a list of strings (to
-       or a list of strings (to store multiple values).
+       store multiple values), or None (to delete the key).
     """
     key = _key(name)
 

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'
 

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
+      self._RepairPreciousObjectsState(project, opt)
-      # 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')
       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:

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(
+@pytest.mark.parametrize('use_superproject, cli_args, result', [
-  '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))