Skip to content

Isolate git operations with a new repository#575

Open
howtonotwin wants to merge 2 commits intoMic92:mainfrom
howtonotwin:shallow-no-mutate
Open

Isolate git operations with a new repository#575
howtonotwin wants to merge 2 commits intoMic92:mainfrom
howtonotwin:shallow-no-mutate

Conversation

@howtonotwin
Copy link
Copy Markdown

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. Fixes #573.

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.

On the way I ended up doing a bit of refactoring re: the handling of nixpkgs-review wip.

This PR is a little rough around the edges, but it does seem to work and not bork my repository anymore.

Clarify that "the changes to be tested" can EITHER take the form of a
commit OR can be uncommitted changes by using one "changes" variable
where applicable instead of a pair of variables where exactly one is not
None.

Also introduce an enum to name the different forms of uncommitted
changes instead of using a bool.
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.
@SuperSandro2000
Copy link
Copy Markdown
Collaborator

Doesn't this make the experience for people with full clones worse?

I would suggest to use an extra local repository where those changes do not effect your main repo.

@howtonotwin
Copy link
Copy Markdown
Author

The stronger isolation means

  • New commits, fetches, and references from the review sandbox (the worktree that the the nixpkgs-review shell starts in) don't propagate to the original repository.
  • The review sandbox conversely doesn't inherit configuration or references from the original repository.

If these are problematic, then the git worktree method could be restored for full clones and this pseudo-git clone method restricted to only shallow clones. (The second point could be handled by copying configuration and references too, but that seems like a can of worms that we shouldn't open...)

Current nixpkgs-review more or less always corrupts a shallow clone (because the standard git tools have generally poor support for them anyway). The process of (efficiently) making a temporary repository which we can break requires low-level Git surgery and should be automated, which is exactly what this PR does.

@SuperSandro2000
Copy link
Copy Markdown
Collaborator

The review sandbox conversely doesn't inherit configuration or references from the original repository.

Which can be a problem and as you wrote further down we don't want to open a can of worms propagating those.

because the standard git tools have generally poor support for them anyway

Which is bad

requires low-level Git surgery and should be automated

Low level surgery could easily break on an update because it is low level things which we should avoid using if possible.


I have a hard time seeing that this outweighs the downsides. We make the logic even more complicated for something very little people use and testing it is also not easy. If you effectively want to contribute to nixpkgs you need a pretty full clone anyway as otherwise rebasing on staging or doing a backport is not possible. Also that every git command needs to be handled very carefully makes this very easy to create in the future with every change related to git commands which is not possible to be checked by everyone.

@Mic92
Copy link
Copy Markdown
Owner

Mic92 commented Dec 4, 2025

I kind of agree with @SuperSandro2000 here.

@howtonotwin
Copy link
Copy Markdown
Author

howtonotwin commented Dec 4, 2025

I mostly wrote this to have a nixpkgs-review that works for me (and to convince myself that it was at all possible to do what nixpkgs-review wants to do "correctly"). I don't mind if this PR doesn't get merged/is closed.

I do think that mainline (which already special cases shallow repositories) should at least warn/require a flag/document/do something to clarify that it is destructive (not just liable to error out) when run on a shallow clone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

git fetch messes up shallow clones

3 participants