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

Fix git auth #9806

Merged
merged 4 commits into from
Jan 19, 2024
Merged

Fix git auth #9806

merged 4 commits into from
Jan 19, 2024

Conversation

roberth
Copy link
Member

@roberth roberth commented Jan 18, 2024

Motivation

Private repos should work, as they did.

Done

  • integration test
  • test manually with github

Follow-up:

  • add shallow fetching back in
    • Not sure what to do because shallow fetching had some issues
    • See comment

Context

  • Thank you @DavHau for helping with the tests

  • I've looked at reimplementing the git credentials protocol, but I'm not eager to maintain a custom, security sensitive component in a place where it won't find contributors. That, and we have more important things to do!

Priorities and Process

Add 👍 to pull requests you find important.

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

roberth and others added 4 commits January 18, 2024 22:29
libgit2 is not capable of using git-credentials helpers yet.
This prevents private repositories from being used.

Based on code that was replaced in NixOS#9240
(Introduce libgit2); hence:

Co-authored-by: Eelco Dolstra <[email protected]>
@roberth roberth added bug regression Something doesn't work anymore fetching Networking with the outside (non-Nix) world, input locking labels Jan 18, 2024
@roberth roberth requested a review from edolstra as a code owner January 18, 2024 21:41
@roberth roberth added this to the nix-2.20 milestone Jan 18, 2024
if (git_remote_fetch(remote.get(), &refspecs2, &opts, nullptr))
throw Error("fetching '%s' from '%s': %s", refspec, url, git_error_last()->message);
runProgram(RunOptions {
.program = "git",
Copy link
Member Author

Choose a reason for hiding this comment

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

@DavHau
Copy link
Member

DavHau commented Jan 19, 2024

[ ] add shallow fetching back in?

* Not sure what to do because shallow fetching had some issues

It shouldn't be an issue to add it back. The only problem was, that in order to reliably fetch shallowly nix would have to ignore any ref passed by the user. But the nix team decided that loosening that check isn't an issue.

Another problem to fix: never share the same cache between shallow and non-shallow clones of the same repo, otherwise we will trip into certain issues which computing revCounts etc.

Other than that I don't see any blockers. It should be easy to enable it.

@DavHau
Copy link
Member

DavHau commented Jan 19, 2024

@roberth I re-implemented the shallow fetching via 674354c
It includes several tests ensuring correct behavior
Feel free to cherry-pick and modify as you wish

Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

It's quite sad that we can't have that on top of libgit2. But Fixing the bug is the priority here, so 👍

@DavHau
Copy link
Member

DavHau commented Jan 19, 2024

I opened a separate PR with the shallow changes, just in case you want to deal with it separately: #9811

@roberth roberth merged commit d762caf into NixOS:master Jan 19, 2024
13 checks passed
@edolstra
Copy link
Member

The PR title "Fix git auth" is a bit misleading when what it actually does is "reintroduce the git dependency".

Rather than rip out the libgit code entirely, I would have preferred an #if 0 (as we had before 21bb180), or even better, make it runtime configurable (e.g. use libgit if git doesn't exist in $PATH).

@roberth
Copy link
Member Author

roberth commented Jan 20, 2024

what it actually does is "reintroduce the git dependency".

... is a bit misleading, because that dependency was never removed.

This topic can be discussed constructively at #9807.

#if 0

I did consider this, but I removed it because it would bitrot.

make it runtime configurable (e.g. use libgit if git doesn't exist in $PATH)

This increases the maintenance burden, and requires that we always ask in bug reports whether the user's Nix uses the git CLI or not, which is a non-obvious question, and in some cases hard to answer. We could try engineer the error messages for this, but I'd rather solve a domain problem than spend resources on such extrinsic complexity. However, even then, it is not a complete solution because of the other work on #9807. I'd suggest to work on more relevant problems.

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

Successfully merging this pull request may close these issues.

4 participants