Revert "Issue8273 corrupt nu get cache (#8275)" (#8686)

This reverts commit a93882f.

This is a temporary fix for #8684

The current plan is to revert #8275 in 17.6, as it caused some difficulties, and try to bring it back in 17.7 via #8685.

Summary

#8275 fixed a longstanding confusing and unfortunate behavior in MSBuild in which passing the Copy task a symlink as its destination would copy the source file onto the destination of the symlink rather than overwriting the symlink. Unfortunately, it also introduced a new issue in which copying a file onto itself could often just delete the file instead of copying anything. Customers reported this issue.

Customer Impact

Projects that copy a file onto itself using the Copy task without passing identical paths for source and destination instead delete the file without necessarily even logging an error.

Regression?
Yes, from #8275.

Testing

Unit tests and manually tested that the repro described in #8684 no longer works.

Risk
Minimal (straight revert of the commit that caused the bug)
---------

Co-authored-by: Forgind <[email protected]>

Author: Forgind

eng/Versions.props

@@ -2,7 +2,7 @@
 <!-- Copyright (c) .NET Foundation and contributors. All rights reserved. Licensed under the MIT license. See License.txt in the project root for full license information. -->
 <Project>
   <PropertyGroup>
-    <VersionPrefix>17.6.1</VersionPrefix><DotNetFinalVersionKind>release</DotNetFinalVersionKind>
+    <VersionPrefix>17.6.2</VersionPrefix><DotNetFinalVersionKind>release</DotNetFinalVersionKind>
     <PackageValidationBaselineVersion>17.5.0</PackageValidationBaselineVersion>
     <AssemblyVersion>15.1.0.0</AssemblyVersion>
     <PreReleaseVersionLabel>preview</PreReleaseVersionLabel>

src/Tasks.UnitTests/Copy_Tests.cs

@@ -2096,87 +2096,6 @@ public void InvalidErrorIfLinkFailed()
             Assert.False(result);
             engine.AssertLogContains("MSB3892");
         }
-
-        /// <summary>
-        /// An existing link source should not be modified.
-        /// </summary>
-        /// <remarks>
-        /// Related to issue [#8273](https://github.com/dotnet/msbuild/issues/8273)
-        /// </remarks>
-        [Theory]
-        [InlineData(false, false)]
-        [InlineData(false, true)]
-        [InlineData(true, false)]
-        public void DoNotCorruptSourceOfLink(bool useHardLink, bool useSymbolicLink)
-        {
-            string sourceFile1 = FileUtilities.GetTemporaryFile();
-            string sourceFile2 = FileUtilities.GetTemporaryFile();
-            string temp = Path.GetTempPath();
-            string destFolder = Path.Combine(temp, "2A333ED756AF4dc392E728D0F864A398");
-            string destFile = Path.Combine(destFolder, "The Destination");
-
-            try
-            {
-                File.WriteAllText(sourceFile1, "This is the first source temp file."); // HIGHCHAR: Test writes in UTF8 without preamble.
-                File.WriteAllText(sourceFile2, "This is the second source temp file."); // HIGHCHAR: Test writes in UTF8 without preamble.
-
-                // Don't create the dest folder, let task do that
-
-                ITaskItem[] sourceFiles = { new TaskItem(sourceFile1) };
-                ITaskItem[] destinationFiles = { new TaskItem(destFile) };
-
-                var me = new MockEngine(true);
-                var t = new Copy
-                {
-                    RetryDelayMilliseconds = 1, // speed up tests!
-                    BuildEngine = me,
-                    SourceFiles = sourceFiles,
-                    DestinationFiles = destinationFiles,
-                    SkipUnchangedFiles = true,
-                    UseHardlinksIfPossible = useHardLink,
-                    UseSymboliclinksIfPossible = useSymbolicLink,
-                };
-
-                bool success = t.Execute();
-
-                Assert.True(success); // "success"
-                Assert.True(File.Exists(destFile)); // "destination exists"
-
-                string destinationFileContents = File.ReadAllText(destFile);
-                Assert.Equal("This is the first source temp file.", destinationFileContents);
-
-                sourceFiles = new TaskItem[] { new TaskItem(sourceFile2) };
-
-                t = new Copy
-                {
-                    RetryDelayMilliseconds = 1, // speed up tests!
-                    BuildEngine = me,
-                    SourceFiles = sourceFiles,
-                    DestinationFiles = destinationFiles,
-                    SkipUnchangedFiles = true,
-                    UseHardlinksIfPossible = false,
-                    UseSymboliclinksIfPossible = false,
-                };
-
-                success = t.Execute();
-
-                Assert.True(success); // "success"
-                Assert.True(File.Exists(destFile)); // "destination exists"
-
-                destinationFileContents = File.ReadAllText(destFile);
-                Assert.Equal("This is the second source temp file.", destinationFileContents);
-
-                // Read the source file (it should not have been overwritten)
-                string sourceFileContents = File.ReadAllText(sourceFile1);
-                Assert.Equal("This is the first source temp file.", sourceFileContents);
-
-                ((MockEngine)t.BuildEngine).AssertLogDoesntContain("MSB3026"); // Didn't do retries
-            }
-            finally
-            {
-                Helpers.DeleteFiles(sourceFile1, sourceFile2, destFile);
-            }
-        }
     }
 
     public class CopyHardLink_Tests : Copy_Tests

src/Tasks/Copy.cs

@@ -226,6 +226,8 @@ private void LogDiagnostic(string message, params object[] messageArgs)
             FileState sourceFileState,      // The source file
             FileState destinationFileState)  // The destination file
         {
+            bool destinationFileExists = false;
+
             if (destinationFileState.DirectoryExists)
             {
                 Log.LogErrorWithCodeFromResources("Copy.DestinationIsDirectory", sourceFileState.Name, destinationFileState.Name);
@@ -267,14 +269,7 @@ private void LogDiagnostic(string message, params object[] messageArgs)
             if (OverwriteReadOnlyFiles)
             {
                 MakeFileWriteable(destinationFileState, true);
-            }
-
-            // If the destination file is a hard or symbolic link, File.Copy would overwrite the source.
-            // To prevent this, we need to delete the existing entry before we Copy or create a link.
-            // We could try to figure out if the file is a link, but I can't think of a reason to not simply delete it always.
-            if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_6) && destinationFileState.FileExists && !destinationFileState.IsReadOnly)
-            {
-                FileUtilities.DeleteNoThrow(destinationFileState.Name);
+                destinationFileExists = destinationFileState.FileExists;
             }
 
             bool symbolicLinkCreated = false;
@@ -284,7 +279,7 @@ private void LogDiagnostic(string message, params object[] messageArgs)
             // Create hard links if UseHardlinksIfPossible is true
             if (UseHardlinksIfPossible)
             {
-                TryCopyViaLink(HardLinkComment, MessageImportance.Normal, sourceFileState, destinationFileState, out hardLinkCreated, ref errorMessage, (source, destination, errMessage) => NativeMethods.MakeHardLink(destination, source, ref errorMessage, Log));
+                TryCopyViaLink(HardLinkComment, MessageImportance.Normal, sourceFileState, destinationFileState, ref destinationFileExists, out hardLinkCreated, ref errorMessage, (source, destination, errMessage) => NativeMethods.MakeHardLink(destination, source, ref errorMessage, Log));
                 if (!hardLinkCreated)
                 {
                     if (UseSymboliclinksIfPossible)
@@ -302,14 +297,13 @@ private void LogDiagnostic(string message, params object[] messageArgs)
             // Create symbolic link if UseSymboliclinksIfPossible is true and hard link is not created
             if (!hardLinkCreated && UseSymboliclinksIfPossible)
             {
-                TryCopyViaLink(SymbolicLinkComment, MessageImportance.Normal, sourceFileState, destinationFileState, out symbolicLinkCreated, ref errorMessage, (source, destination, errMessage) => NativeMethodsShared.MakeSymbolicLink(destination, source, ref errorMessage));
+                TryCopyViaLink(SymbolicLinkComment, MessageImportance.Normal, sourceFileState, destinationFileState, ref destinationFileExists, out symbolicLinkCreated, ref errorMessage, (source, destination, errMessage) => NativeMethodsShared.MakeSymbolicLink(destination, source, ref errorMessage));
+                if (!NativeMethodsShared.IsWindows)
+                {
+                    errorMessage = Log.FormatResourceString("Copy.NonWindowsLinkErrorMessage", "symlink()", errorMessage);
+                }
                 if (!symbolicLinkCreated)
                 {
-                    if (!NativeMethodsShared.IsWindows)
-                    {
-                        errorMessage = Log.FormatResourceString("Copy.NonWindowsLinkErrorMessage", "symlink()", errorMessage);
-                    }
-
                     Log.LogMessage(MessageImportance.Normal, RetryingAsFileCopy, sourceFileState.Name, destinationFileState.Name, errorMessage);
                 }
             }
@@ -330,28 +324,41 @@ private void LogDiagnostic(string message, params object[] messageArgs)
                 Log.LogMessage(MessageImportance.Normal, FileComment, sourceFilePath, destinationFilePath);
 
                 File.Copy(sourceFileState.Name, destinationFileState.Name, true);
-
-                // If the destinationFile file exists, then make sure it's read-write.
-                // The File.Copy command copies attributes, but our copy needs to
-                // leave the file writeable.
-                if (sourceFileState.IsReadOnly)
-                {
-                    destinationFileState.Reset();
-                    MakeFileWriteable(destinationFileState, false);
-                }
             }
 
             // Files were successfully copied or linked. Those are equivalent here.
             WroteAtLeastOneFile = true;
 
+            destinationFileState.Reset();
+
+            // If the destinationFile file exists, then make sure it's read-write.
+            // The File.Copy command copies attributes, but our copy needs to
+            // leave the file writeable.
+            if (sourceFileState.IsReadOnly)
+            {
+                MakeFileWriteable(destinationFileState, false);
+            }
+
             return true;
         }
 
-        private void TryCopyViaLink(string linkComment, MessageImportance messageImportance, FileState sourceFileState, FileState destinationFileState, out bool linkCreated, ref string errorMessage, Func<string, string, string, bool> createLink)
+        private void TryCopyViaLink(string linkComment, MessageImportance messageImportance, FileState sourceFileState, FileState destinationFileState, ref bool destinationFileExists, out bool linkCreated, ref string errorMessage, Func<string, string, string, bool> createLink)
         {
             // Do not log a fake command line as well, as it's superfluous, and also potentially expensive
             Log.LogMessage(MessageImportance.Normal, linkComment, sourceFileState.Name, destinationFileState.Name);
 
+            if (!OverwriteReadOnlyFiles)
+            {
+                destinationFileExists = destinationFileState.FileExists;
+            }
+
+            // CreateHardLink and CreateSymbolicLink cannot overwrite an existing file or link
+            // so we need to delete the existing entry before we create the hard or symbolic link.
+            if (destinationFileExists)
+            {
+                FileUtilities.DeleteNoThrow(destinationFileState.Name);
+            }
+
             linkCreated = createLink(sourceFileState.Name, destinationFileState.Name, errorMessage);
         }
 
@@ -819,11 +826,6 @@ private bool DoCopyWithRetries(FileState sourceFileState, FileState destinationF
                                     LogDiagnostic("Retrying on ERROR_ACCESS_DENIED because MSBUILDALWAYSRETRY = 1");
                                 }
                             }
-                            else if (code == NativeMethods.ERROR_INVALID_FILENAME)
-                            {
-                                // Invalid characters used in file name, no point retrying.
-                                throw;
-                            }
 
                             if (e is UnauthorizedAccessException)
                             {

src/Tasks/NativeMethods.cs

@@ -537,7 +537,6 @@ internal static class NativeMethods
 
         internal const int HRESULT_E_CLASSNOTREGISTERED = -2147221164;
 
-        internal const int ERROR_INVALID_FILENAME = -2147024773; // Illegal characters in name
         internal const int ERROR_ACCESS_DENIED = -2147024891; // ACL'd or r/o
         internal const int ERROR_SHARING_VIOLATION = -2147024864; // File locked by another use