IsContentOfFilesTheSame assumes full Stream.Read

[KalleOlaviNiemitalo]

In the Nerdbank.GitVersioning.Tasks.CompareFiles class, the IsContentOfFilesTheSame method calls Stream.Read and assumes that a return value smaller than requested means that it got to the end of the file. As mentioned in #797 (comment), such an assumption is generally a bug. Here though, the stream refers to a regular file, which makes the assumption more likely to hold.

Nerdbank.GitVersioning/src/Nerdbank.GitVersioning.Tasks/CompareFiles.cs

Lines 127 to 133 in 719b0d5

	bytesRead = fileStream1.Read(buffer1, 0, buffer1.Length);
	if (fileStream2.Read(buffer2, 0, buffer2.Length) != bytesRead)
	{
	// We should never get here since we compared file lengths, but
	// this is a sanity check.
	return false;
	}

The CompareFiles task is used in Nerdbank.GitVersioning.targets, for deciding whether to run the Copy task. The bug might cause the task to consider the files different when they are actually identical; the target would then needlessly copy the file and cause MSBuild to execute other targets, resulting in a slower incremental build but eventually the correct artefacts.

It might be possible to delete the CompareFiles task altogether and use the SkipUnchangedFiles parameter of the Copy task instead.

Nerdbank.GitVersioning/src/Nerdbank.GitVersioning.Tasks/build/Nerdbank.GitVersioning.targets

Lines 144 to 149 in a908c6d

	<!-- Avoid applying the newly generated AssemblyVersionInfo.cs file to the build
	unless it has changed in order to allow for incremental building. -->
	<Nerdbank.GitVersioning.Tasks.CompareFiles OriginalItems="$(VersionSourceFile)" NewItems="$(NewVersionSourceFile)">
	<Output TaskParameter="AreChanged" PropertyName="AssemblyVersionInfoChanged" />
	</Nerdbank.GitVersioning.Tasks.CompareFiles>
	<Copy Condition=" '$(AssemblyVersionInfoChanged)' == 'true' " SourceFiles="$(NewVersionSourceFile)" DestinationFiles="$(VersionSourceFile)" />

Nerdbank.GitVersioning/src/Nerdbank.GitVersioning.Tasks/build/Nerdbank.GitVersioning.targets

Lines 181 to 186 in a908c6d

	<!-- Avoid applying the newly generated Version.rc file to the build
	unless it has changed in order to allow for incremental building. -->
	<Nerdbank.GitVersioning.Tasks.CompareFiles OriginalItems="$(VersionSourceFile)" NewItems="$(NewVersionSourceFile)">
	<Output TaskParameter="AreChanged" PropertyName="_NativeVersionInfoChanged" />
	</Nerdbank.GitVersioning.Tasks.CompareFiles>
	<Copy Condition=" '$(_NativeVersionInfoChanged)' == 'true' " SourceFiles="$(NewVersionSourceFile)" DestinationFiles="$(VersionSourceFile)" />

[AArnott]

Thanks for finding the bug. It turns out that Copy.SkipUnchangedFiles employs a cheap trick (probably for perf reasons) and will assume that the files are different when the timestamp is different, so it defeats the incremental build preservation that I'm attempting to do. I'll have to fix my task.