Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fetchTree/fetchGit: re-enable shallow fetching #9811

Merged
merged 1 commit into from
Jan 19, 2024

Conversation

DavHau
Copy link
Member

@DavHau DavHau commented Jan 19, 2024

Motivation

Add back shallow fetching after it has been removed by #9806
based on #9806

Context

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@DavHau DavHau requested a review from edolstra as a code owner January 19, 2024 09:06
@github-actions github-actions bot added the fetching Networking with the outside (non-Nix) world, input locking label Jan 19, 2024
@DavHau DavHau mentioned this pull request Jan 19, 2024
2 tasks
&& {repo.git} push origin --all
""")

# memoize the revisions
Copy link
Member

@roberth roberth Jan 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# memoize the revisions
# save the revision

Not lazy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@@ -55,7 +55,8 @@ in
def create(self):
# create ssh remote repo
gitea.succeed(f"""
git init --bare -b main /root/{self.name}
git init --bare -b main /root/{self.name} \
&& git config --global uploadpack.allowAnySHA1InWant true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of uploadpack.allowAnySHA1InWant here; could you add a comment?

I assume it's not a problem for Nix users because this happens on the gitea host for some initialization purpose - not on a clone that is a normal client.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this line.
I thought we need this setting to enable shallow cloning via ssh by revision, but it seems it's not needed.

Add several tests for git fetching:
- shallow-cache-separation: can fetch the same repo shallowly and non-shallowly
- shallow-ignore-ref: ensure that ref gets ignored when shallow=true is set
- ssh-shallow: can fetch a git repo via ssh using shallow=1
@thufschmitt
Copy link
Member

Discussed during the maintainers meeting today. Good to go 🚀

- @edolstra: Worried that this entranches us in a Git-cli specific behavior and makes it harder to move to libGit2 - @thufschmitt: It's quite unlikely that we'll be able to use libgit everywhere given the missing features. So it shouldn't be too much of a problem - That leaves us with 2 dependencies for the same thing, but probably won't be avoidable because lazy trees isn't really implementable with the Git cli - @Ericson2314: Could we add the missing features in libgit2? - ~500 lines of python elsewhere; probably more C++ needed, and it's security sensitive... Too much effort to make this a priority - @edolstra: We could also not support credential helpers at all - But would be really annoying for a number of users
  • Agreement to merge

@thufschmitt thufschmitt merged commit 10165c7 into NixOS:master Jan 19, 2024
8 checks passed
@DavHau DavHau deleted the fix-git-auth branch January 22, 2024 04:21
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-01-19-nix-team-meeting-minutes-116/38837/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fetching Networking with the outside (non-Nix) world, input locking
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants