Clarify minimum git version #3327
Conversation
Related change: 632f488
|
First of all, welcome, and congratulations on your first pull request to Git LFS! Since I'm relatively new to the project, I'm not sure what the history of our version requirements is. It looks like at some point, we were testing 1.8.5 and have incrementally moved to 2.0.0. The commit messages don't seem to provide a great deal of clarity, so maybe @ttaylorr can say a few words about why those changes were made. The code does indeed seem to support 1.8.2, and I'm not opposed to updating the documentation to reflect this. I expect you want to use RHEL 7's shipped git packages for this, and presumably that works fine, or we would have heard from the EPEL folks by now. If possible, I'd like to see if we can get at least Travis testing this configuration so we can have confidence that we don't break it accidentally. |
|
For what it's worth, the code still enforces 1.8.2 as the minimum. You are spot on, RHEL7 has git 1.8.3.1 (with multiple backported security fixes). I am in fact one of said EPEL folks, I co-maintain the git-lfs package in Fedora and EPEL. I don't know how easy it will be to test with that old a git version in Travis. Isn't that an Ubuntu based environment? |
|
Yeah, I took a look in the code, and we definitely are requiring 1.8.2 there. I just want to be sure that there's not some reason things shouldn't work related to the Travis CI and CircleCI testing. We clone Git and built it ourselves for the very old versions, and 1.8.2 does appear to build on Ubuntu 18.04, if quite noisily. Let's see what @ttaylorr has to say about the testing, and assuming there's no hidden bombshell, I see no reason why we shouldn't take this. |
I don't think that there's anything 💣-like going on here. As I recall, we tried to add some test, and realized that it mysteriously did not work on Git 1.8.5, but did work on Git 2.0.0. I think at the time we decided to leave it (i.e., by testing on Git 2.0.0 as the "minimum version", because the change seemed unrelated to a 1.8.5-specific thing), and come back to it later. I think that we never managed to do the last part, though ;-). All of that is to say that I think that this change looks good to me. |
|
@bk2204 👍/ 👎 for taking this in v2.6.0? I think that it'd be OK to do so. |
|
Yeah, I'm for taking this. It's merely documenting what our current policy is, and so I can't see any reason why we shouldn't. Ideally we'd fix the CI such that it works on 1.8.2 as well, but that can come in separately. |
Related change: 632f488