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

fetchGit: Remove unneeded call to git #2366

Closed
wants to merge 2 commits into from

Conversation

erikarvstedt
Copy link
Member

@erikarvstedt erikarvstedt commented Aug 21, 2018

Originally, I wrote this to fix a nix performance issue, which turned out to have other causes specific to my setup.

This patch might still be useful as it saves a needless call to git in the common case where a rev is given that is already in /nix/store.
In this case, now only ~/.cache/nix/git and /nix/store are queried and no subprocesses are started.

Previously, 'git cat-file -e REV' was always called,
even if the rev was already present in the nix store.
@knedlsepp
Copy link
Member

This sounds related to my problem: #2348

@erikarvstedt
Copy link
Member Author

Your issue has a different scope, but it's still very relevant.
It could be solved with this simple fix, where links to fetched revisions are just identified by their SHA-1 hash (and not by name + SHA-1).
It wouldn't increase the attack surface for SHA-1 collisions in any meaningful way, so I don't see any downsides.

@tomberek
Copy link
Contributor

review?

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I am not a C++ developer but the change is small and looks logically consistent.

/cc the nix-core team: @copumpkin @edolstra @peti @shlevy @vcunat

@stale
Copy link

stale bot commented Feb 13, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Feb 13, 2021
@erikarvstedt
Copy link
Member Author

The current version of the git fetcher (libfetchers/git.cc) doesn't call git on a cache hit, so this PR is obsolete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants