Isolate git operations with a new repository

Shallow fetch operations permanently change the shallow configuration of
the repository they run in. Using a temporary worktree does not protect
the user's repository against such modifications. Switch to using a
new separate repository to provide appropriate protection.

Set up the alternates mechanism to have the new repository share storage
with the original. This cannot be done automatically by git-clone since
git-clone refuses to use a shallow repository as an alternate object
store (for good reason!). Manually create the new repository and provide
it with a copy of the original's shallow configuration to make this use
of alternates correct.

Author: howtonotwin

README.md

@@ -60,13 +60,11 @@ First, change to your local nixpkgs repository directory, i.e.:
 cd ~/git/nixpkgs
 ```
 
-If you've shallow cloned nixpkgs (`git clone --depth`), `nixpkgs-review` may be
-unable to perform merges due to missing merge base commits. Reclone nixpkgs
-without the `--depth` flag.
-
-Note that your local checkout git will not be affected by `nixpkgs-review`,
-since it will use [git-worktree](https://git-scm.com/docs/git-worktree) to
-perform fast checkouts.
+Note that `nixpkgs-review` will not leave any effects in your local Git
+repository (if it does, it's a bug!). It does its work in a temporary
+repository separate from your own. It uses Git's
+["alternates"](https://git-scm.com/docs/gitrepository-layout) mechanism to
+minimize transfers.
 
 Then run `nixpkgs-review` by providing the pull request number…
 
@@ -154,6 +152,18 @@ nix-shell> nixpkgs-review post-result
 nix-shell> nixpkgs-review comments
 ```
 
+## Caveats for shallow clones
+
+If you've shallow cloned nixpkgs (`git clone --depth`), `nixpkgs-review` may be
+unable to perform merges due to missing merge base commits. The simplest fix for
+this is, as you might imagine, recloning without the `--depth` flag. Even in
+cases where your shallow cuts do not interfere with merging, it is possible that
+the latest upstream commits provide a new path "around" your shallow boundary,
+in which case `git fetch` will (very, very slowly) more-or-less unshallow your
+repository. This is not a problem in `nixpkgs-review`, but a limitation of Git.
+It is possible to get around this by using a more intelligent fetching program
+to update your repository before running `nixpkgs-review`.
+
 ## Optional tools
 
 `nixpkgs-review` can integrate with several optional tools to enhance the user

nixpkgs_review/builddir.py

@@ -6,7 +6,6 @@
 from tempfile import TemporaryDirectory
 from typing import TYPE_CHECKING, Self
 
-from . import git
 from .overlay import Overlay
 from .utils import warn
 
@@ -72,10 +71,10 @@ def __init__(self, name: str) -> None:
 
         self.overlay = Overlay()
 
-        self.worktree_dir = self.path / "nixpkgs"
-        self.worktree_dir.mkdir()
+        self.clone_dir = self.path / "nixpkgs"
+        self.clone_dir.mkdir()
         self._nix_path_parts = [
-            f"nixpkgs={self.worktree_dir}",
+            f"nixpkgs={self.clone_dir}",
             f"nixpkgs-overlays={self.overlay.path}",
         ]
         self.nix_path = " ".join(self._nix_path_parts)
@@ -94,14 +93,6 @@ def __exit__(
         os.environ.clear()
         os.environ.update(self.environ)
 
-        with DisableKeyboardInterrupt():
-            if (self.worktree_dir / ".git").exists():
-                res = git.run(["worktree", "remove", "-f", str(self.worktree_dir)])
-                if res.returncode != 0:
-                    warn(
-                        f"Failed to remove worktree at {self.worktree_dir}. Please remove it manually. Git failed with: {res.returncode}"
-                    )
-
         self.overlay.cleanup()
 
         # Clean up the temporary directory if we created one

nixpkgs_review/review.py

@@ -23,7 +23,7 @@
 from .github import GithubClient, GitHubPullRequest
 from .nix import Attr, nix_build, nix_eval, nix_shell
 from .report import Report
-from .utils import System, current_system, die, info, sh, system_order_key, warn
+from .utils import System, current_system, die, info, system_order_key, warn
 
 if TYPE_CHECKING:
     import argparse
@@ -232,8 +232,8 @@ def _process_aliases_for_systems(self, system: str) -> set[str]:
             case _:
                 return {system}
 
-    def worktree_dir(self) -> str:
-        return str(self.builddir.worktree_dir)
+    def clone_dir(self) -> str:
+        return str(self.builddir.clone_dir)
 
     def _render_markdown(self, content: str, max_length: int = 1000) -> None:
         """Render markdown content using glow if available, otherwise plain text."""
@@ -329,17 +329,15 @@ def _display_pr_info(self, pr: GitHubPullRequest, pr_number: int) -> None:
         print(f"{'=' * 80}\n")
 
     def git_merge(self, commit: str) -> None:
-        res = git.run(
-            ["merge", "--no-commit", "--no-ff", commit], cwd=self.worktree_dir()
-        )
+        res = git.run(["merge", "--no-commit", "--no-ff", commit], cwd=self.clone_dir())
         if res.returncode != 0:
-            msg = f"Failed to merge {commit} into {self.worktree_dir()}. git merge failed with exit code {res.returncode}"
+            msg = f"Failed to merge {commit} into {self.clone_dir()}. git merge failed with exit code {res.returncode}"
             raise NixpkgsReviewError(msg)
 
     def git_checkout(self, commit: str) -> None:
-        res = git.run(["checkout", commit], cwd=self.worktree_dir())
+        res = git.run(["checkout", commit], cwd=self.clone_dir())
         if res.returncode != 0:
-            msg = f"Failed to checkout {commit} in {self.worktree_dir()}. git checkout failed with exit code {res.returncode}"
+            msg = f"Failed to checkout {commit} in {self.clone_dir()}. git checkout failed with exit code {res.returncode}"
             raise NixpkgsReviewError(msg)
 
     def apply_uncommitted(self, changes: UncommittedChanges) -> None:
@@ -362,10 +360,10 @@ def apply_uncommitted(self, changes: UncommittedChanges) -> None:
             sys.exit(0)
 
         info("Applying `nixpkgs` diff...")
-        result = git.run(["apply"], cwd=self.worktree_dir(), stdin=diff)
+        result = git.run(["apply"], cwd=self.clone_dir(), stdin=diff)
 
         if result.returncode != 0:
-            die(f"Failed to apply diff in {self.worktree_dir()}")
+            die(f"Failed to apply diff in {self.clone_dir()}")
 
     def build_changes(
         self,
@@ -380,8 +378,11 @@ def build_changes(
         `changes` can be a string naming a commit, or it can be one of the
         `UncommittedChanges` variants. `merge_commit`, if given, is used
         instead of repeating the merge of `changes` into `base_commit`.
+
+        The sandbox repository is expected to be set up and able to see both
+        commits.
         """
-        self.git_worktree(base_commit)
+        self.git_checkout(base_commit)
         changed_attrs: dict[System, set[str]] = {}
 
         if self.only_packages:
@@ -448,10 +449,42 @@ def build_changes(
 
         return self.build(changed_attrs, self.build_args)
 
-    def git_worktree(self, commit: str) -> None:
-        res = git.run(["worktree", "add", self.worktree_dir(), commit])
+    def git_clone(
+        self, repo: str, ref: str, *, shallow_depth: int | None = None
+    ) -> None:
+        # moral equivalent of git clone --revision=<new_head> --reference=. [--depth=...] <repo> <dir>
+        # but it will work even with a shallow repository as reference
+        # we need the isolation of a separate repository because --depth "irreversibly" mutates .git/shallow
+        res = git.run(["init"], self.clone_dir())
+        if res.returncode != 0:
+            msg = f"Failed to create sandbox repository for {ref} in {self.clone_dir()}. git init failed with exit code {res.returncode}"
+            raise NixpkgsReviewError(msg)
+
+        # really, we should be locking this file until the clone is disposed of
+        shallow_original = resolve_git_dir() / "shallow"
+        if shallow_original.exists():
+            shallow_target = Path(self.clone_dir()) / ".git" / "shallow"
+            shutil.copyfile(shallow_original, shallow_target)
+
+        alternates_path = (
+            Path(self.clone_dir()) / ".git" / "objects" / "info" / "alternates"
+        )
+        object_store_path = resolve_git_dir().absolute() / "objects"
+        with alternates_path.open("a") as alternates:
+            alternates.write(f"{object_store_path}\n")
+
+        fetch_args = ["fetch", repo, f"+{ref}:refs/heads/nixpkgs-review"]
+        if shallow_depth:
+            fetch_args.append(f"--depth={shallow_depth}")
+        # we should warn if shallow_depth is not set and the original repository is shallow (and there are commits to fetch)
+        # in this case git fetch (really, upload-pack) acts stupidly and has a very high chance of both unshallowing the repository
+        # AND taking forever to compute how to do it
+        # (but it WILL work and we WILL have enough history to do any merges we need later, which is better than if we set shallow_depth incorrectly)
+        # if the user wants something more efficient, they can kill us and do it themselves
+
+        res = git.run(fetch_args, self.clone_dir())
         if res.returncode != 0:
-            msg = f"Failed to add worktree for {commit} in {self.worktree_dir()}. git worktree failed with exit code {res.returncode}"
+            msg = f"Failed to fetch {ref} into sandbox repository {self.clone_dir()}. git fetch failed with exit code {res.returncode}"
             raise NixpkgsReviewError(msg)
 
     def build(
@@ -526,24 +559,26 @@ def build_pr(self, pr_number: int) -> dict[System, list[Attr]]:
         packages_per_system = (
             self._fetch_packages_from_github_eval(pr) if self._use_github_eval else None
         )
-
-        [merge_rev] = fetch_refs(self.remote, pr["merge_commit_sha"], shallow_depth=2)
-        base_rev = git.verify_commit_hash(f"{merge_rev}^1")
-        head_rev = git.verify_commit_hash(f"{merge_rev}^2")
-
         if self.only_packages:
             packages_per_system = {
                 system: set(self.only_packages) for system in self.systems
             }
 
+        merge_rev = pr["merge_commit_sha"]
+        base_rev = pr["base"]["sha"]
+        head_rev = pr["head"]["sha"]
+
         if packages_per_system is None:
+            self.git_clone(self.remote, merge_rev, shallow_depth=2)
             return self.build_changes(base_rev, head_rev, merge_rev)
 
         match self.checkout:
             case CheckoutOption.MERGE:
-                self.git_worktree(merge_rev)
+                checkout_rev = merge_rev
             case CheckoutOption.COMMIT:
-                self.git_worktree(head_rev)
+                checkout_rev = head_rev
+        self.git_clone(self.remote, checkout_rev, shallow_depth=1)
+        self.git_checkout(checkout_rev)
 
         for system in list(packages_per_system.keys()):
             if system not in self.systems:
@@ -622,10 +657,10 @@ def review_local(
         print_result: bool = False,
         approve_pr: bool = False,
     ) -> None:
-        branch_rev = fetch_refs(self.remote, branch)[0]
+        self.git_clone(self.remote, branch)
         self.start_review(
             changes if isinstance(changes, str) else None,
-            self.build_changes(branch_rev, changes),
+            self.build_changes("nixpkgs-review", changes),
             path,
             print_result=print_result,
             approve_pr=approve_pr,
@@ -904,49 +939,6 @@ def resolve_git_dir() -> Path:
             raise NixpkgsReviewError(msg)
 
 
-def fetch_refs(repo: str, *refs: str, shallow_depth: int = 1) -> list[str]:
-    shallow = subprocess.run(
-        ["git", "rev-parse", "--is-shallow-repository"],
-        text=True,
-        stdout=subprocess.PIPE,
-        check=False,
-    )
-    if shallow.returncode != 0:
-        msg = f"Failed to detect if {repo} is shallow repository"
-        raise NixpkgsReviewError(msg)
-
-    fetch_cmd = [
-        "git",
-        "-c",
-        "fetch.prune=false",
-        "fetch",
-        "--no-tags",
-        "--force",
-        repo,
-    ]
-    if shallow.stdout.strip() == "true":
-        fetch_cmd.append(f"--depth={shallow_depth}")
-    for i, ref in enumerate(refs):
-        fetch_cmd.append(f"{ref}:refs/nixpkgs-review/{i}")
-    dotgit = resolve_git_dir()
-    with locked_open(dotgit / "nixpkgs-review", "w"):
-        res = sh(fetch_cmd)
-        if res.returncode != 0:
-            msg = f"Failed to fetch {refs} from {repo}. git fetch failed with exit code {res.returncode}"
-            raise NixpkgsReviewError(msg)
-        shas = []
-        for i, ref in enumerate(refs):
-            rev_parse_cmd = ["git", "rev-parse", "--verify", f"refs/nixpkgs-review/{i}"]
-            out = subprocess.run(
-                rev_parse_cmd, text=True, stdout=subprocess.PIPE, check=False
-            )
-            if out.returncode != 0:
-                msg = f"Failed to fetch {ref} from {repo} with command: {''.join(rev_parse_cmd)}"
-                raise NixpkgsReviewError(msg)
-            shas.append(out.stdout.strip())
-        return shas
-
-
 def differences(
     old: list[Package], new: list[Package]
 ) -> tuple[list[Package], list[Package]]: