Skip to content

Conversation

@johanvdw
Copy link
Contributor

Closes #5182

Aligns the project with curl and git.

@johanvdw johanvdw requested a review from a team as a code owner May 19, 2025 13:38
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.
@chrisd8088
Copy link
Member

chrisd8088 commented May 21, 2025

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 config/netrc.go file entirely as it's not used anywhere. That will eliminate some duplicative code and another whole source code file.


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 t/t-credentials.sh test script which use either .netrc or _netrc, depending on the OS platform.

One option might be to run all of these tests nested inside a for loop, which iterates over just .netrc on Unix platforms but over .netrc and _netrc on Windows, and so runs each test twice on Windows.

I think that's probably a bit excessive, though; it's probably sufficient just to repeat the initial credentials from netrc test for this purpose with both possible file names.

(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 if [ "$IS_WINDOWS" -eq 1 ], which is slightly more readable than what we have now, and also covers more POSIX environments on Windows.)

So this might look something like the following:

t/t-credentials.sh edits
NETRCFILES=".netrc"
if [ "$IS_WINDOWS" -eq 1 ]; then
  NETRCFILES+=" _netrc"
fi

for netrcfile in $NETRCFILES; do
  begin_test "credentials from netrc ($netrcfile)"
  (
    set -e

    printf "machine localhost\nlogin netrcuser\npassword netrcpass\n" >"$HOME/$netrcfile"
    ...
  )
  end_test
done

begin_test "credentials from netrc with unknown keyword"
(
  set -e

  printf "machine localhost\nlogin netrcuser\nnot-a-key something\npassword netrcpass\n" >"$HOME/.netrc"
  ...
)
end_test

# ... and then also use "$HOME/.netrc" in the subsequent tests instead of "$NETRCFILE"

I've added two commits on top of yours in my windows-netrc branch and I see that our Windows CI job ran the t/t-credentials.sh script with my changes successfully, so I think these commits could be added to this PR, if you're willing to pull them into your branch. (Or if you prefer to issue me an invitation to push to your fork, I can do that instead.)

I tried to include some relevant details in the commit messages so we won't have to dig around in the Git history as much in the future if there are any concerns down the road.

Thanks again very much for tackling this issue and putting this PR together!

Copy link
Member

@chrisd8088 chrisd8088 left a 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!

@johanvdw
Copy link
Contributor Author

I've added two commits on top of yours in my windows-netrc branch and I see that our Windows CI job ran the t/t-credentials.sh script with my changes successfully, so I think these commits could be added to this PR, if you're willing to pull them into your branch. (Or if you prefer to issue me an invitation to push to your fork, I can do that instead.)

As you noted, I merged your suggestions (and also run an extra test on our system where the issue surfaced).

Copy link
Member

@chrisd8088 chrisd8088 left a 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!

@chrisd8088 chrisd8088 merged commit 684449b into git-lfs:main May 21, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

.netrc files ignored by Git LFS on Windows but used by Git via cURL

2 participants