Properly reject NuGet newline-only changes. #10332
Conversation
brettfo
changed the title
brettfo
force-pushed
the
dev/brettfo/nuget-reject-whitespace-changes
branch
from
208ae8f
to
f834d5c
Compare
|
|
||
| it "does not update the project" do | ||
| run_update_test do |updater| | ||
| expect(updater.updated_dependency_files.map(&:name)).to be_empty |
There was a problem hiding this comment.
This test case was wrong; it was the case where a file was updated and a trailing newline was removed and it was erroneously detected as just deleting the trailing newline, which wasn't correct because the contents actually changed. This scenario is covered in the new tests below.
| @@ -45,7 +59,7 @@ def updated_dependency_files | |||
| normalized_content = normalize_content(f, updated_content) | |||
| next if normalized_content == f.content | |||
|
|
|||
| next if only_deleted_lines?(f.content, normalized_content) | |||
| next unless FileUpdater.differs_in_more_than_blank_lines?(f.content, normalized_content) | |||
There was a problem hiding this comment.
Does the original scenario (#9162) still pass here?
If the updated content has less lines than the original content, then a package has been removed somewhere and we likely don't want to create a pull request.
Maybe there needs to be a test for this as well, unless there are other checks elsewhere to deal with this problem now.
There was a problem hiding this comment.
Good call, that was a test case I missed. I just pushed a commit that adds this test case if you want to double check I understood the issue.
There was a problem hiding this comment.
Re-reading your comment I think there is a separate issue to address; what if a package is removed? I think that should be handled in the C# code that actually checks the file content. This part of the code is to handle silliness around empty lines. There's likely a bigger work item to ensure that a dependency isn't removed, but we have better unit tests in the C# code for that. Do you have a scenario where a package was removed?
There was a problem hiding this comment.
My understanding of the description in #9162 is that if you had a project like this:
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFramework>net461</TargetFramework>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="DependencyA" Version="1.0.0" />
</ItemGroup>
</Project>And NuGetUpdater attempted to update "DependencyA" to "2.0.0" but failed and rolled back e.g.
WARNING: Install failed. Rolling back...
It would remove the dependency entirely and result in a project file like:
~~~
@@ -2,7 +2,4 @@
<PropertyGroup>
<TargetFramework>net461</TargetFramework>
</PropertyGroup>
- <ItemGroup>
- <PackageReference Include="DependencyA" Version="1.0.0" />
- </ItemGroup>
</Project>
\ No newline at end of fileIn this scenario, the file is different, but it is different in a way that the PR should not be created because the change isn't desired. I could be misunderstanding this problem though.
Maybe the original issue shouldn't have been solved in file_updater and instead the "rollback" warning should be escalated as an error by the native NuGet updater instead? Maybe this scenario isn't even valid anymore, in which case, ignore me.
I'll let you decide what is correct here as you understand this all better than me.
Ultimately this change solves my specific problem, so I'm happy with it.
There was a problem hiding this comment.
I'd say the scenario you presented should be handled purely in C#. Let's take this PR as is, because it does fix a class of errors that we're currently seeing and I'll look at the rollback scenario tomorrow and see if I can nail down what might cause that. There appears to be enough information in the linked issue to sort it out.
There was a problem hiding this comment.
@rhyskoedijk I filed issue #10342 to track this other scenario and I'm working on it now. Rough ETA, sometime next week. I think the best mechanism to prevent this kind of error is to only report available package upgrades if all transitive dependencies can be resolved so that we never try to update when it's not fully resolvable (the base scenario that #9162 was trying to address).
An update to #10324, but since I didn't have permissions to update that fork/branch, I've created a new PR here with the same commit history.
Fixes #10323
Thanks to @rhyskoedijk for identifying the issue and figuring out a solution.
A short description of the original problem
The NuGet updater attempts to detect newline-only changes and reject them, but the logic for that detection was flawed. An example of this was a file in a repo with a trailing newline where the contents were updated and the trailing newline was deleted. This was erroneously determined to be a newline-only change and wasn't included in the PR.
What to do about it
The fix is to compare the original and updated lines, ignoring blank lines and whitespace, and if there are no differences, reject the change, otherwise keep them.
There was also a rearranging in the product code to enable direct testing of the relevant method, along with a bunch of test cases.