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

Enforce minimum git version check (currently >= 1.8.2) when running a… #1212

Closed

Conversation

javabrett
Copy link
Contributor

…ny command. Fixed #1210 .

@technoweenie
Copy link
Contributor

Thanks for the PR! @sinbad: Any thoughts on checking the git version on every command, or just specific ones? It seems fast, but may become an issue when running the git lfs smudge command on thousands of files on a Windows machine. At the least, I'd add this check to these commands: install, update, env, track, pre-push, pull, checkout.

Unfortunately, this is adding a couple extra git version calls to every git lfs call. The reason is that IsGitVersionAtLeast also runs git version. Instead, the check could use IsVersionAtLeast() directly.

@sinbad
Copy link
Contributor

sinbad commented May 10, 2016

I'm amazed anyone is still running git that old :) I'd prefer to keep the checks to a minimum - although git --version is a relatively fast operation, on Windows we pay a high cost for every fork so it's better if we don't have to add any we don't have to. At a minimum it would be a good idea to cache the current git version in git.Version() rather than always forking (we can assume it's not going to change in the life of the process) if we're starting to make this more common - some specific functions do check the version in other places and it would be best not to call it more than once.

As an even tighter optimisation I could suggest adding the check only to git lfs install and git lfs update since those are gatekeepers and it's unlikely that people will downgrade. However since it's only once I can live with a universal call if it's cached for other uses later.

@technoweenie
Copy link
Contributor

At a minimum it would be a good idea to cache the current git version in git.Version()

👍

I could suggest adding the check only to git lfs install and git lfs update

I picked the other commands to guard against cases where people just dump the binary into their PATH and try to push ahead without checking things. I don't think you can get very far without running those. They're also not ever called multiple times in a tight loop like git lfs smudge.

@sinbad
Copy link
Contributor

sinbad commented May 10, 2016

Yeah, that's fair - I'm happy so long as we avoid smudge & clean then.

@technoweenie
Copy link
Contributor

I'd probably add clone and push to that list too.

@samrocketman
Copy link

samrocketman commented Jul 13, 2016

RedHat 6 machines are pretty wide spread and in use. The version of git in the RH6/CentOS6 repos is 1.7.x. I typically compile git in those cases.

@technoweenie
Copy link
Contributor

@samrocketman This PR is about communicating to end users what the minimum git version is, not setting a new minimum git version. It's a good idea to let them know ahead of time, and not later when they're wondering why LFS isn't working correctly.

Closing this in favor of #1461, which only runs in the commands specified above, and adds git version caching. Here's sample output, proving that git version is only run once:

$ GIT_TRACE=1 script/run track
16:20:45.908860 git.c:350               trace: built-in: git 'rev-parse' '--short' 'HEAD'
trace git-lfs: run_command: 'git' version
trace git-lfs: run_command: 'git' config -l
Listing tracked paths

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