Respect empty user-wide NuGet.Config file, no matter if there is a track file or not

[heng-liu]

Bug

Fixes: NuGet/Home#10745

Regression? Last working version:

Description

1.Only create default user-wide NuGet.Config file when it doesn't exist. If the user-wide NuGet.Config file exists, but there is no feed, no matter there is a track file or not, we will respect the empty file. (Do we need to show a warning/message saying no source exists in this case?)
2.Remove track file.
3.Add tests for the following cases:
user-wide NuGet.Config file exists and is empty, and there is a track file => the empty config file should be respected
user-wide NuGet.Config file exists and is empty, and there is a track file => the empty config file should be respected
user-wide NuGet.Config file doesn't exist => the default config file with nuget.org feed will be created
4.Fix an existing test and correct the case so that it will pass on Linux.

Backgroud:
The nuget blog shows one of the changes in V3.4: The Visual Studio extension no longer are automatically adds the nuget.org repository source to your nuget.config files when you use the NuGet configuration user interface.
So in NuGet 3.3, client adds nuget.org by default if it's not there. (see NuGet/Home#2053)
In NuGet 3.4, client tried to respect the config file, even it's empty. But for users migrate from 3.3 to 3.4, they might encounter an issue like NuGet/Home#2445
So Zhi li added the tracker file for users migrate from 3.3 to 3.4, to add nuget.org into user-wide NuGet.Config for the first time, but do not add nuget.org afterwards (when track file is there). The PR is #458 The commit is 1da590a

Analysis:

The comparison between current behavior and the future behavior with this change:

Case	the current behavior without this commit	the future behavior with this commit
The config file exists. And it is not empty	The config file is respected	The config file is respected
The config file exists and it is empty, and there is a tracker file.	The empty config file is respected	The empty config file is respected
The config file exists and it is empty, and there is no tracker file	The empty config file is not respected. A default nuget.org feed will be added to this config file	The empty config file is respected
The config file doesn't exist	will create a default config file	will create a default config file

So for customers using NuGet version 3.4 and later versions(doesn't contain this change) migrating to the new version(contains this change), the behavior changes only for the 3rd case. And this case is very rare, it means the user has an empty user-wide NuGet.Config file, and has never run any commands approaching user-wide NuGet.Client. If users run any commands which will approach config file just once, it will turn into the 2nd case.

For customers using NuGet version 3.3 and earlier versions migrating to the new versioin(contains this change, that is, 5.10), it will break them. But it is also very rare.

PR Checklist

• PR has a meaningful title

• PR has a linked issue.

• Described changes

• Tests

  • Automated tests added
  • OR
  • Test exception
  • OR
  • N/A

• Documentation

  • Documentation PR or issue filled
  • OR
  • N/A

[erdembayar]

Can you check your logic again? I commented on it.

[zivkan]

I find the two EmptyUserWideConfigFile tests weird. We're asserting that calling a method named LoadSettings doesn't modify the file. If we were writing this code from scratch, we wouldn't write tests to assert that a Read method doesn't write to the file. So, my gut feeling is that in removing this tracking file that causes the file to be modified on read, we should remove the test(s) that assert the write on read, rather than changing it to assert no-modify on read. On the other hand, writing tests for the expected new behaviour is generally the right thing to do.

I'd like input from other people in the team,

[erdembayar]

Just 1 more question

[heng-liu]

I find the two EmptyUserWideConfigFile tests weird. We're asserting that calling a method named LoadSettings doesn't modify the file. If we were writing this code from scratch, we wouldn't write tests to assert that a Read method doesn't write to the file. So, my gut feeling is that in removing this tracking file that causes the file to be modified on read, we should remove the test(s) that assert the write on read, rather than changing it to assert no-modify on read. On the other hand, writing tests for the expected new behaviour is generally the right thing to do.

I'd like input from other people in the team,

Thanks @zivkan. I agree with you, so the extra test is removed.

[erdembayar]

@heng-liu
Are 4 cases described in PR body still up to date?

[heng-liu]

@heng-liu
Are 4 cases described in PR body still up to date?

Yes, they're up to date. Thanks for asking!

[lokst]

@heng-liu I'm not sure if this is an appropriate place to seek clarifications, but in the case where the user-wide NuGet.Config file doesn't exist, is running nuget restore expected to create a default one? According to my tests, in such a situation, running nuget sources will, but nuget restore will not. May I ask if this is expected behaviour?

[heng-liu]

Hi @lokst , this is not the expected behavior.
The expected behavior is:
If the user-wide NuGet.Config file doesn't exist, the first command asking for config file will create a default user-wide NuGet.Config with nuget.org as the only source.
If the user-wide NuGet.Config file exists, even it's empty(no source), NuGet will just respect it as it is.

Please note, before this change, the NuGet tool doesn't respect empty NuGet.Config file(exists but without any source) and adds nuget.org for the first time.

I just tried nuget restore but was not able to repro.
If you still see the issue, could you please raise an issue in https://github.com/NuGet/Home/issues and provide the repro steps? Thanks!

[lokst]

@heng-liu Thank you so much for confirming that it is not expected behaviour! I will raise an issue in https://github.com/NuGet/Home/issues

[lokst]

@heng-liu After more investigation, I found that NuGet is behaving as per the spec in this PR. The problem I encountered only happens when using NuGet together with Chocolatey. Specifically, nuget restore can fail if we run choco install before nuget restore, because choco install creates an empty user-wide NuGet.config file.

Since this incompatibility of nuget restore with choco install doesn't seem like ideal behaviour, I have filed an issue here: NuGet/Home#10804