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

fetchgitrev: fix parsing and add certs for https://github.com fetching #30569

Closed
wants to merge 2 commits into from

Conversation

kquick
Copy link
Contributor

@kquick kquick commented Oct 19, 2017

Motivation for this change

Fix fetchgitrevision to work properly. There appear to be no current uses of this in the nixpkgs repo, but attempts to use it locally failed until the patches in this PR were applied.

Things done
  • [] Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
    Note this was tested without using sandbox: when sandboxing is enabled, uses of fetchgitrevision fail with a DNS lookup error for github.com. I believe this is a side-effect of sandboxing and unrelated to the changes here.
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

When the cert is not provided, the git operation for an https
repository (e.g. https://github.com) will fail with an SSL error
because the local signatures are not available.
The git ls-remote command will return results like:

somelonghash    refs/heads/branch1
another_hash    refs/heads/master
another1hash    refs/tags/4.1.3
another2hash    refs/tags/4.1.0

The parsing of the response now accomodates any interim reference
identifier, enabling fetchgitrevision to work for both branches and
tags, whereas previously it would never match anything for modern
github responses (although the old matching behavior is preserved in
case older github was less specific in the ls-remote response).
@kquick kquick requested a review from edolstra as a code owner October 19, 2017 07:14
@kquick kquick changed the title Fgitrev cert fetchgitrev: fix parsing and add certs for https://github.com fetching Oct 19, 2017
Copy link
Member

@matthewbauer matthewbauer left a comment

Choose a reason for hiding this comment

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

There is a merge conflict. Otherwise looks okay to merge.

@c0bw3b
Copy link
Contributor

c0bw3b commented Oct 26, 2018

fetchgitrevision was removed in #30986

@c0bw3b c0bw3b closed this Oct 26, 2018
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.

None yet

4 participants