`git fetch` messes up shallow clones

[howtonotwin]

I've got a shallow clone of nixpkgs together with a script to let me "git fetch" to it properly. Long story short, its master tracks NixOS/nixpkgs's master, its depth is neither an integer nor fixed, and when I make a branch to do something master will usually end up advancing by the time I'm ready to run nixpkgs-review

      o <---- master (advances with a cronjob)
     /|\
    / | \          o <- my-topic-branch
   / /|  \/-------/
  / | |\  \
 /  | | |  \
~~~~~~~~~~~~~ <- commits listed in .git/shallow (widened over time by cronjob)

nixpkgs-review puts my workflow in peril in a couple of ways:

• It does git fetch --depth=<n> if it sees the repository is shallow. While it does take care to use its own namespace of refs, .git/shallow is not namespaced. The --depth=<n> beheads my history, disconnecting master from my-topic-branch. This causes the dreaded "refusing to merge unrelated histories" error, even if my master was already up to date (that is, if a fetch was not necessary) and even if my repository otherwise contained enough history to merge master and my-topic-branch.

  That is, nixpkgs-review turns my repository into the following and then dies about it

        o <---- master (advances with a cronjob)
        ~ <- new shallow boundary due to --depth=<n>
                     
            o <- my-topic-branch
           / \
          /   \
         ~~~~~~~ <- partial old contents of .git/shallow
  

  Not only does nixpkgs-review fail, but a large chunk of the repository has become unreachable. I can manually fix it by deleting the offending new commits from .git/shallow, but in the mean time a git gc could theoretically remove some of the unreachable commits and prune the now-extraneous entries from .git/shallow and make the situation permanent.

  TL;DR: git fetch --depth=<n> is logically a destructive operation. nixpkgs-review shouldn't be doing it.

• It does git fetch at all. git fetch (at least, as of git-2.50.1) simply doesn't handle shallow repositories very well. Even if nixpkgs-review were patched not to pass --depth=<n> to git fetch, if NixOS/nixpkgs has advanced beyond my master and nixpkgs-review does a git fetch of the new tip commit, then that git fetch may pull in a line of commits that goes around my shallow boundary and thus start pulling down the whole history of nixpkgs.

        o <---- NixOS/nixpkgs's master
       /|\
      / | \
     /  o <----- my master
    /  /|\  \
   /  / | \  \
  |  ~~~~~~ <- my .git/shallow
  |
  v to rest of nixpkgs history (huge)
    (to add insult to injury git fetch is incredibly slow at negotiating which commits it wants in this case)
  

  This is much less of a problem; were this to happen I could just interrupt the fetch and run my own script to update my repository properly before rerunning nixpkgs-review.

All that said, I think nixpkgs-review should consider doing some or all of the following:

• Provide an option to override the shallow repository detection and make sure --depth is never passed to git fetch.
  • Actually, I can't think of a situation where anyone would benefit from nixpkgs-review passing --depth to git fetch. Maybe this code should be off by default or even removed?
• Provide an option to make sure git fetch never happens at all. If no new commits need to be fetched anyway, then nixpkgs-review should simply create a ref without using git fetch. If new commits do need to be fetched, nixpkgs-review could either let the user specify a custom command or simply error out.
• Change the comment in the README about shallow clones. The problem with shallow clones is not so much insufficient history being available to merge topic branches into master, but rather the standard git tools simply don't know how to bring a shallow repository up to date without either breaking the history or breaking the shallowness (and current nixpkgs-review chooses to break the history).

[Mic92]

Can we talk a bit about how these shallow clones are beeing used? Is it about saving diskspace, is it about ci integration?

[howtonotwin]

It's about saving space for me. My use case is running nixpkgs-review rev my-topic-branch on my local nixpkgs clone. Am I doing something wrong?

[Mic92]

cc @azuwis who added this flag.

[azuwis]

• Actually, I can't think of a situation where anyone would benefit from nixpkgs-review passing --depth to git fetch. Maybe this code should be off by default or even removed?

The --depth arg was introduced in PR #426, and fix #267, it works great for nixpkgs-review pr.

I've make a test branch that remove the --depth, master...azuwis:nixpkgs-review:push-wsyovtluppol, run this at github actions to compare:

$ git clone --depth=1 https://github.com/NixOS/nixpkgs.git /tmp/test-nixpkgs-review-with-depth
$ cd /tmp/test-nixpkgs-review-with-depth
$ du -sh .git
66M     .git
$ nix run github:Mic92/nixpkgs-review -- pr --no-shell 461538
real    0m12.770s
user    0m2.987s
sys     0m5.292s
$ du -sh .git
71M     .git

$ git clone --depth=1 https://github.com/NixOS/nixpkgs.git /tmp/test-nixpkgs-review-without-depth
$ cd /tmp/test-nixpkgs-review-without-depth
$ du -sh .git
66M     .git
$ time nix run github:azuwis/nixpkgs-review/push-wsyovtluppol -- pr --no-shell 461538
real    33m4.046s
user    16m29.870s
sys     0m36.607s
$ du -sh .git
2.1G    .git

--depth save 32 minutes and 2.0G bandwidth / disk space.

The code is quite different now compared to the time I made PR #426, back then it need to fetch 2 refs:

git -c fetch.prune=false fetch --no-tags --force https://github.com/NixOS/nixpkgs --depth=1 master:refs/nixpkgs-review/0 pull/351518/merge:refs/nixpkgs-review/1

Now it fetch only 1 ref:

git -c fetch.prune=false fetch --no-tags --force https://github.com/NixOS/nixpkgs --depth=2 60cbb400c8f19032bac35d2ae0ae77ce2f687ee6:refs/nixpkgs-review/0

Maybe we can make the fetch_refs default to no --depth, something like:

diff --git a/nixpkgs_review/review.py b/nixpkgs_review/review.py
index 05f56f2..b85ac32 100644
--- a/nixpkgs_review/review.py
+++ b/nixpkgs_review/review.py
@@ -891,7 +891,7 @@ def resolve_git_dir() -> Path:
             raise NixpkgsReviewError(msg)
 
 
-def fetch_refs(repo: str, *refs: str, shallow_depth: int = 1) -> list[str]:
+def fetch_refs(repo: str, *refs: str, shallow_depth: int = -1) -> list[str]:
     shallow = subprocess.run(
         ["git", "rev-parse", "--is-shallow-repository"],
         text=True,
@@ -911,7 +911,7 @@ def fetch_refs(repo: str, *refs: str, shallow_depth: int = 1) -> list[str]:
         "--force",
         repo,
     ]
-    if shallow.stdout.strip() == "true":
+    if shallow.stdout.strip() == "true" and shallow_depth >= 0:
         fetch_cmd.append(f"--depth={shallow_depth}")
     for i, ref in enumerate(refs):
         fetch_cmd.append(f"{ref}:refs/nixpkgs-review/{i}")

--depth should still work for nixpkgs-review pr.

[howtonotwin]

--depth does appear to let nixpkgs-review pr proceed normally. However, it still leaves permanent breakage in my repository!

# Simulate a shallow clone that's a bit behind NixOS/nixpkgs
$ git clone --origin=upstream --depth=10 'https://github.com/NixOS/nixpkgs.git'; cd nixpkgs
$ git reset --hard HEAD~9

# Note that in this state, master can be fast-forwarded to upstream/master
$ git status
On branch master
Your branch is behind 'upstream/master' by ## commits, and can be fast-forwarded.
  (use "git pull" to update your local branch)

nothing to commit, working tree clean

# Now run nixpkgs-review on some PR
# It will git fetch --depth=2 upstream refs/pull/###/merge:refs/nixpkgs-review/#
# This will add refs/pull/###/merge^ to .git/shallow and this will NOT be cleaned up
$ nix run github:Mic92/nixpkgs-review -- pr --no-shell 461538

# Even if nixpkgs-review and exits cleanly, there is persistent breakage in the repository
# (assuming refs/pull/###/merge^ was in fact somewhere in master..upstream/master)
$ git merge --ff-only # try to update master to upstream/master
fatal: refusing to merge unrelated histories
$ git status # no longer fast-forwardable
On branch master
Your branch and 'upstream/master' have diverged,
and have ## and ## different commits each, respectively.
  (use "git pull" if you want to integrate the remote branch with yours)

If you repeat the above experiment without running nixpkgs-review in the middle, everything is fine and the fast forward works.

Again, git fetch --depth is in general going to make permanent changes to the repository it runs in that can only be rolled back by editing .git/shallow. nixpkgs-review does not do this and I don't know if it reasonably can. If the goal is to leave no lasting damage to the repository, then I think git fetch --depth is simply not an option for nixpkgs-review.

I think the correct way to do what nixpkgs-review is trying to do without making permanent changes to the repository it's running in is to (roughly) git clone https://github.com/NixOS/nixpkgs.git $(mktemp -d) --depth=<n> --reference ..

[azuwis]

I can not reproduce the problem using the above example:

❯ git clone --origin=upstream --depth=10 'https://github.com/NixOS/nixpkgs.git'; cd nixpkgs
Cloning into 'nixpkgs'...
remote: Enumerating objects: 122913, done.
remote: Counting objects: 100% (122913/122913), done.
remote: Compressing objects: 100% (74541/74541), done.
remote: Total 122913 (delta 19860), reused 89448 (delta 9420), pack-reused 0 (from 0)
Receiving objects: 100% (122913/122913), 71.87 MiB | 8.43 MiB/s, done.
Resolving deltas: 100% (19860/19860), done.
Updating files: 100% (49815/49815), done.
❯ git reset --hard HEAD~9 
HEAD is now at 3995c9dac grafanaPlugins.grafana-oncall-app: 1.16.5 -> 1.16.6 (#459955)
❯ nix run github:Mic92/nixpkgs-review -- pr --no-shell 461538
...
-> Fetching eval results from GitHub actions
$ git -c fetch.prune=false fetch --no-tags --force https://github.com/NixOS/nixpkgs --depth=2 eb0df3fd4326e4458ab90205d5fabe70e544f10f:refs/nixpkgs-review/0
remote: Total 0 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
$ git worktree add /Users/azuwis/.cache/nixpkgs-review/pr-461538-1/nixpkgs eb0df3fd4326e4458ab90205d5fabe70e544f10f
Preparing worktree (detached HEAD eb0df3fd4)
Updating files: 100% (49817/49817), done.
HEAD is now at eb0df3fd4 Merge 5d3089765f0e60b6b87b12e6027ef49545b45e8c into 9b1223f17a1533193bb0be25129ccbcab4d9f482
Nothing to be built.

Link to currently reviewing PR:
https://github.com/NixOS/nixpkgs/pull/461538
Logs can be found under:
/Users/azuwis/.cache/nixpkgs-review/pr-461538-1/logs

$ git worktree remove -f /Users/azuwis/.cache/nixpkgs-review/pr-461538-1/nixpkgs
❯ git merge --ff-only       
Updating 3995c9dac..34fc45370
Fast-forward
 maintainers/maintainer-list.nix                              | 6 ------
 nixos/modules/security/ipa.nix                               | 8 +++++++-
 pkgs/by-name/ca/cargo-tauri/package.nix                      | 6 +++---
 pkgs/by-name/ch/charm/package.nix                            | 2 +-
 pkgs/by-name/du/duf/package.nix                              | 5 +----
 pkgs/by-name/gl/glow/package.nix                             | 5 +----
 pkgs/by-name/gr/gradia/package.nix                           | 5 +++--
 pkgs/by-name/im/img-cat/package.nix                          | 2 +-
 pkgs/by-name/me/melt/package.nix                             | 2 +-
 pkgs/by-name/ne/nextinspace/package.nix                      | 2 +-
 pkgs/by-name/pa/pat/package.nix                              | 6 +++---
 pkgs/by-name/sk/skate/package.nix                            | 4 +---
 pkgs/by-name/sl/slides/package.nix                           | 5 +----
 pkgs/by-name/so/soft-serve/package.nix                       | 2 +-
 pkgs/by-name/ve/vector/package.nix                           | 6 +++---
 pkgs/by-name/vh/vhs/package.nix                              | 5 +----
 pkgs/by-name/wi/wishlist/package.nix                         | 5 +----
 pkgs/by-name/ze/zeekscript/package.nix                       | 9 ++++++---
 pkgs/development/interpreters/elixir/1.19.nix                | 4 ++--
 pkgs/development/php-packages/redis/default.nix              | 4 ++--
 pkgs/development/python-modules/tree-sitter-zeek/default.nix | 4 ++--
 21 files changed, 42 insertions(+), 55 deletions(-)

[Mic92]

If this is not reproducible, maybe there is some other unusual .gitconfig setting that causes divergance? I would expect there are more than just one shallow-clone user for nixpkgs-review, but this is the first this issue gets reported.

[howtonotwin]

The refs/pull/###/merge reference is only updated sporadically (I don't know what the actual frequency is). My reproducer only reproduces when refs/pull/###/merge^ is somewhere in master~9..master. If you replace the git clone/git reset with a sequence that simulates "perfect" timing, you should be almost guaranteed to hit the problem (at the cost of realism). (There is still a race if the refs/pull/###/merge ref moves between the git clone and the nixpkgs-review.)

$ git clone --origin=upstream --depth=3 --revision=refs/pull/461538/merge 'https://github.com/NixOS/nixpkgs.git'; cd nixpkgs
$ git config set remote.upstream.fetch '+refs/heads/master:refs/remotes/upstream/master'
$ git update-ref refs/remotes/upstream/master HEAD^
$ git checkout master; git reset --hard HEAD^

# proceed the same as before
$ git status # fast-forwardable
$ git log --oneline --graph HEAD@{2} # this picture is now small enough to be useful
$ nix run github:Mic92/nixpkgs-review -- pr --no-shell 461538 # let run or kill early
$ git merge --ff-only # error
$ git status # not fast-fowardable anymore
$ git log --oneline --graph HEAD@{2} master # history has been cut