-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Prefer .netrc on windows if present #6055
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
Conversation
Support for reading and parsing ".netrc" files (or "_netrc" files on Windows) was first added to the Git LFS client in PR git-lfs#715, and the parsing code was later moved into our "config" package in PR git-lfs#1226. Subsequently, almost identical parsing code was also added to our "lfsapi" package in commit 2275188 of PR git-lfs#1839, which was later moved to its current location in our "creds" package in PR git-lfs#3307. Meanwhile, the only caller of the original ".netrc" parser, which was implemented in the parseNetrc() method of the Configuration structure in our "config" package, was removed in commit afcd211 PR git-lfs#1846. As a consequence, since PR git-lfs#1846 the parseNetrc() method in the "config" package has remained unused, along with the related "netrcfinder" interface and associated structure and method in the same source file, so we can simply delete that file now.
In previous commits in this PR we revised the ParseNetrc() function in our "creds" package to support both ".netrc" and "_netrc" files on Windows, whereas it has previously only supported "_netrc" files. This change will align the behaviour of the Git LFS client with that of contemporary versions of curl and Git, as they now support both filenames on Windows. Specifically, this support was added in commit curl/curl@f9c7ba9, which was included in curl v7.66.0. To validate these changes, we also update our t/t-credentials.sh test script so it runs the key "credentials from netrc" test twice on Windows, once with a ".netrc" file and once with a legacy "_netrc" file. As well, we adjust the other tests which previously used a "_netrc" file on Windows to simply use a ".netrc" file, as this should work on all systems now.
|
Hey, thanks very much for this PR and proposed enhancement! It would excellent if we can include this in a future release. It's always great to see some files being fully deleted! As it happens, these changes highlight another thing we can simplify, because it turns out that we can drop the We will want some updates to our shell tests as part of this PR, to make sure it has the intended effect. Right now there are a few tests in the One option might be to run all of these tests nested inside a I think that's probably a bit excessive, though; it's probably sufficient just to repeat the initial (While we're at it, we can also make the condtition which checks the OS platform a little more consistent with our other tests by using So this might look something like the following:
|
chrisd8088
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this great improvement, and welcome to the Git LFS project!
I think if we add some test changes like the ones I drafted, that should cover the main thing we'll want to include in this PR.
We might as well also just drop the config/netrc.go file, too, I think; there's no point in keeping it around. I also drafted a commit to do that, with some notes on the tangled history of these files.
Thanks again very much for working on this!
Co-authored-by: Chris Darroch <[email protected]>
As you noted, I merged your suggestions (and also run an extra test on our system where the issue surfaced). |
chrisd8088
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thank you!
I usually try to solicit a second opinion from another @git-lfs/core member when I've contributed to a PR, but in this case I think my changes are sufficiently minimal (just deleting an unused file and tweaking the test cases) that I can approve this PR as it stands.
Thanks again very much!
Closes #5182
Aligns the project with curl and git.