Do not report warnings when SourceLink packages are not referenced ex… (#991)

* Do not report warnings when SourceLink packages are not referenced explicitly and the repository has no URL or no commit.

This allows the user to build an app in a new local repo, without having to set origin remote or commit changes first.

Author: tmat

.gitignore

@@ -5,6 +5,7 @@
 *.suo
 *.user
 .vs/
+.vscode/
 
 # Build results
 artifacts/

src/Common/GetSourceLinkUrlGitTask.cs

@@ -85,7 +85,15 @@ private void ExecuteImpl()
             if (string.IsNullOrEmpty(gitUrl))
             {
                 SourceLinkUrl = NotApplicableValue;
-                Log.LogWarning(CommonResources.UnableToDetermineRepositoryUrl);
+
+                // If SourceRoot has commit sha but not repository URL the source control info is available,
+                // but the remote for the repo has not been defined yet. We already reported missing remote in that case
+                // (unless suppressed).
+                if (string.IsNullOrEmpty(SourceRoot.GetMetadata(Names.SourceRoot.RevisionId)))
+                {
+                    Log.LogWarning(CommonResources.UnableToDetermineRepositoryUrl);
+                }
+
                 return;
             }
 

src/Microsoft.Build.Tasks.Git.UnitTests/GitDataTests.cs

@@ -54,7 +54,7 @@ public void MinimalGitData()
             Assert.Equal("1111111111111111111111111111111111111111", repository.GetHeadCommitSha());
 
             var warnings = new List<(string, object?[])>();
-            var sourceRoots = GitOperations.GetSourceRoots(repository, remoteName: null, (message, args) => warnings.Add((message, args)));
+            var sourceRoots = GitOperations.GetSourceRoots(repository, remoteName: null, warnOnMissingCommit: true, (message, args) => warnings.Add((message, args)));
             AssertEx.Equal(new[]
             {
                 $@"'{repoDir.Path}{s}' SourceControl='git' RevisionId='1111111111111111111111111111111111111111' ScmRepositoryUrl='http://github.com/test-org/test-repo'",

src/Microsoft.Build.Tasks.Git.UnitTests/GitOperationsTests.cs

@@ -448,16 +448,53 @@ public void ApplyInsteadOfUrlMapping_Multiple()
             Assert.Equal("A.com", actualMappedUrl);
         }
 
-        [Fact]
-        public void GetSourceRoots_RepoWithoutCommits()
+        [Theory]
+        [CombinatorialData]
+        public void GetSourceRoots_RepoWithoutCommits(bool warnOnMissingCommit)
         {
             var repo = CreateRepository();
 
             var warnings = new List<(string, object?[])>();
-            var items = GitOperations.GetSourceRoots(repo, remoteName: null, (message, args) => warnings.Add((message, args)));
+            var items = GitOperations.GetSourceRoots(repo, remoteName: null, warnOnMissingCommit, (message, args) => warnings.Add((message, args)));
 
             Assert.Empty(items);
-            AssertEx.Equal(new[] { Resources.RepositoryHasNoCommit }, warnings.Select(TestUtilities.InspectDiagnostic));
+            AssertEx.Equal(warnOnMissingCommit ? new[] { Resources.RepositoryHasNoCommit } : Array.Empty<string>(), warnings.Select(TestUtilities.InspectDiagnostic));
+        }
+
+        [Fact]
+        public void GetSourceRoots_RepoWithCommits_WithoutUrl()
+        {
+            var repo = CreateRepository(
+                commitSha: "0000000000000000000000000000000000000000");
+
+            var warnings = new List<(string, object?[])>();
+            var items = GitOperations.GetSourceRoots(repo, remoteName: null, warnOnMissingCommit: true, (message, args) => warnings.Add((message, args)));
+
+            AssertEx.Equal(new[]
+            {
+                $@"'{_workingDir}{s}' SourceControl='git' RevisionId='0000000000000000000000000000000000000000'",
+            }, items.Select(TestUtilities.InspectSourceRoot));
+
+            Assert.Empty(warnings.Select(TestUtilities.InspectDiagnostic));
+        }
+
+        [Fact]
+        public void GetSourceRoots_RepoWithCommits_WithUrl()
+        {
+            var repo = CreateRepository(
+                commitSha: "0000000000000000000000000000000000000000",
+                config: CreateConfig(
+                    ("remote.origin.url", "http://github.com/abc")));
+
+            var warnings = new List<(string, object?[])>();
+            var items = GitOperations.GetSourceRoots(repo, remoteName: null, warnOnMissingCommit: true, (message, args) => warnings.Add((message, args)));
+
+            AssertEx.Equal(new[]
+            {
+                $@"'{_workingDir}{s}' SourceControl='git' RevisionId='0000000000000000000000000000000000000000' ScmRepositoryUrl='http://github.com/abc'",
+            }, items.Select(TestUtilities.InspectSourceRoot));
+
+            Assert.Empty(warnings.Select(TestUtilities.InspectDiagnostic));
         }
 
         [Fact]
@@ -480,7 +517,7 @@ public void GetSourceRoots_RepoWithoutCommitsWithSubmodules()
                     CreateSubmodule("sub6", "sub/6", "", "6666666666666666666666666666666666666666")));
 
             var warnings = new List<(string, object?[])>();
-            var items = GitOperations.GetSourceRoots(repo, remoteName: null, (message, args) => warnings.Add((message, args)));
+            var items = GitOperations.GetSourceRoots(repo, remoteName: null, warnOnMissingCommit: false, (message, args) => warnings.Add((message, args)));
 
             // Module without a configuration entry is not initialized.
             // URLs listed in .submodules are ignored (they are used by git submodule initialize to generate URLs stored in config).
@@ -493,9 +530,8 @@ public void GetSourceRoots_RepoWithoutCommitsWithSubmodules()
 
             AssertEx.Equal(new[]
             {
-                Resources.RepositoryHasNoCommit,
                 string.Format(Resources.SourceCodeWontBeAvailableViaSourceLink, string.Format(Resources.InvalidSubmoduleUrl, "sub4", "https:///"))
-            }, warnings.Select(TestUtilities.InspectDiagnostic)); ;
+            }, warnings.Select(TestUtilities.InspectDiagnostic));
         }
 
         [Fact]
@@ -511,7 +547,7 @@ public void GetSourceRoots_RepoWithCommitsWithSubmodules()
                     CreateSubmodule("2", "sub/2", "http://2.com", "2222222222222222222222222222222222222222")));
 
             var warnings = new List<(string, object?[])>();
-            var items = GitOperations.GetSourceRoots(repo, remoteName: null, (message, args) => warnings.Add((message, args)));
+            var items = GitOperations.GetSourceRoots(repo, remoteName: null, warnOnMissingCommit: false, (message, args) => warnings.Add((message, args)));
 
             AssertEx.Equal(new[]
             {
@@ -547,7 +583,7 @@ public void GetSourceRoots_RelativeSubmodulePath()
                     CreateSubmodule("1", "sub/1", "---", "1111111111111111111111111111111111111111", containingRepositoryWorkingDir: repoDir.Path)));
 
             var warnings = new List<(string, object?[])>();
-            var items = GitOperations.GetSourceRoots(repo, remoteName: null, (message, args) => warnings.Add((message, args)));
+            var items = GitOperations.GetSourceRoots(repo, remoteName: null, warnOnMissingCommit: false, (message, args) => warnings.Add((message, args)));
 
             AssertEx.Equal(new[]
             {

src/Microsoft.Build.Tasks.Git/GitOperations.cs

@@ -25,14 +25,14 @@ internal static class GitOperations
         private const string UrlSectionName = "url";
         private const string UrlVariableName = "url";
 
-        public static string? GetRepositoryUrl(GitRepository repository, string? remoteName, Action<string, object?[]>? logWarning = null)
-            => GetRepositoryUrl(repository, remoteName, recursionDepth: 0, logWarning);
+        public static string? GetRepositoryUrl(GitRepository repository, string? remoteName, bool warnOnMissingRemote = true, Action<string, object?[]>? logWarning = null)
+            => GetRepositoryUrl(repository, remoteName, recursionDepth: 0, warnOnMissingRemote, logWarning);
 
-        private static string? GetRepositoryUrl(GitRepository repository, string? remoteName, int recursionDepth, Action<string, object?[]>? logWarning = null)
+        private static string? GetRepositoryUrl(GitRepository repository, string? remoteName, int recursionDepth, bool warnOnMissingRemote, Action<string, object?[]>? logWarning)
         {
             NullableDebug.Assert(repository.WorkingDirectory != null);
 
-            var remoteUrl = GetRemoteUrl(repository, ref remoteName, logWarning);
+            var remoteUrl = GetRemoteUrl(repository, ref remoteName, warnOnMissingRemote, logWarning);
             if (remoteUrl == null)
             {
                 return null;
@@ -45,10 +45,10 @@ internal static class GitOperations
                 return null;
             }
 
-            return ResolveUrl(uri, repository.Environment, remoteName, recursionDepth, logWarning);
+            return ResolveUrl(uri, repository.Environment, remoteName, recursionDepth, warnOnMissingRemote, logWarning);
         }
 
-        private static string? GetRemoteUrl(GitRepository repository, ref string? remoteName, Action<string, object?[]>? logWarning)
+        private static string? GetRemoteUrl(GitRepository repository, ref string? remoteName, bool warnOnMissingRemote, Action<string, object?[]>? logWarning)
         {
             string? unknownRemoteName = null;
             string? remoteUrl = null;
@@ -63,7 +63,11 @@ internal static class GitOperations
 
             if (remoteUrl == null && !TryGetRemote(repository.Config, out remoteName, out remoteUrl))
             {
-                logWarning?.Invoke(Resources.RepositoryHasNoRemote, new[] { repository.WorkingDirectory });
+                if (warnOnMissingRemote)
+                {
+                    logWarning?.Invoke(Resources.RepositoryHasNoRemote, new[] { repository.WorkingDirectory });
+                }
+
                 return null;
             }
 
@@ -75,7 +79,7 @@ internal static class GitOperations
             return remoteUrl;
         }
 
-        private static string? ResolveUrl(Uri uri, GitEnvironment environment, string? remoteName, int recursionDepth, Action<string, object?[]>? logWarning)
+        private static string? ResolveUrl(Uri uri, GitEnvironment environment, string? remoteName, int recursionDepth, bool warnOnMissingRemote, Action<string, object?[]>? logWarning)
         {
             if (!uri.IsFile)
             {
@@ -85,7 +89,11 @@ internal static class GitOperations
             var repositoryPath = uri.LocalPath;
             if (!GitRepository.TryGetRepositoryLocation(repositoryPath, out var remoteRepositoryLocation))
             {
-                logWarning?.Invoke(Resources.RepositoryHasNoRemote, new[] { repositoryPath });
+                if (warnOnMissingRemote)
+                {
+                    logWarning?.Invoke(Resources.RepositoryHasNoRemote, new[] { repositoryPath });
+                }
+
                 return uri.AbsoluteUri;
             }
 
@@ -102,7 +110,7 @@ internal static class GitOperations
                 return null;
             }
 
-            return GetRepositoryUrl(remoteRepository, remoteName, recursionDepth + 1, logWarning) ?? uri.AbsoluteUri;
+            return GetRepositoryUrl(remoteRepository, remoteName, recursionDepth + 1, warnOnMissingRemote, logWarning) ?? uri.AbsoluteUri;
         }
 
         private static bool TryGetRemote(GitConfig config, [NotNullWhen(true)]out string? remoteName, [NotNullWhen(true)]out string? remoteUrl)
@@ -239,7 +247,7 @@ private static bool TryParseScp(string value, [NotNullWhen(true)]out Uri? uri)
             return Uri.TryCreate(url, UriKind.Absolute, out uri);
         }
 
-        public static ITaskItem[] GetSourceRoots(GitRepository repository, string? remoteName, Action<string, object?[]> logWarning)
+        public static ITaskItem[] GetSourceRoots(GitRepository repository, string? remoteName, bool warnOnMissingCommit, Action<string, object?[]> logWarning)
         {
             // Not supported for repositories without a working directory.
             NullableDebug.Assert(repository.WorkingDirectory != null);
@@ -262,7 +270,7 @@ public static ITaskItem[] GetSourceRoots(GitRepository repository, string? remot
                 item.SetMetadata(Names.SourceRoot.RevisionId, revisionId);
                 result.Add(item);
             }
-            else
+            else if (warnOnMissingCommit)
             {
                 logWarning(Resources.RepositoryHasNoCommit, Array.Empty<object>());
             }
@@ -298,7 +306,7 @@ public static ITaskItem[] GetSourceRoots(GitRepository repository, string? remot
                     continue;
                 }
 
-                var submoduleUrl = ResolveUrl(submoduleUri, repository.Environment, remoteName, recursionDepth: 0, logWarning);
+                var submoduleUrl = ResolveUrl(submoduleUri, repository.Environment, remoteName, recursionDepth: 0, warnOnMissingRemote: true, logWarning);
                 if (submoduleUrl == null)
                 {
                     logWarning(Resources.SourceCodeWontBeAvailableViaSourceLink,

src/Microsoft.Build.Tasks.Git/LocateRepository.cs

@@ -53,8 +53,8 @@ private protected override void Execute(GitRepository repository)
 
             RepositoryId = repository.GitDirectory;
             WorkingDirectory = repository.WorkingDirectory;
-            Url = GitOperations.GetRepositoryUrl(repository, RemoteName, Log.LogWarning);
-            Roots = GitOperations.GetSourceRoots(repository, RemoteName, Log.LogWarning);
+            Url = GitOperations.GetRepositoryUrl(repository, RemoteName, warnOnMissingRemote: !NoWarnOnMissingInfo, Log.LogWarning);
+            Roots = GitOperations.GetSourceRoots(repository, RemoteName, warnOnMissingCommit: !NoWarnOnMissingInfo, Log.LogWarning);
             RevisionId = repository.GetHeadCommitSha();
         }
     }

src/Microsoft.Build.Tasks.Git/RepositoryTask.cs

@@ -30,9 +30,9 @@ public abstract class RepositoryTask : Task
 #endif
 
         /// <summary>
-        /// True to report a warning when the repository can't be located.
+        /// True to report a warning when the repository can't be located, it's missing remote or a commit.
         /// </summary>
-        public bool NoWarnOnMissingRepository { get; set; }
+        public bool NoWarnOnMissingInfo { get; set; }
 
         public sealed override bool Execute()
         {
@@ -62,7 +62,7 @@ bool logAssemblyLoadingErrors()
 
         private void ReportMissingRepositoryWarning(string initialPath)
         {
-            if (!NoWarnOnMissingRepository)
+            if (!NoWarnOnMissingInfo)
             {
                 Log.LogWarning(Resources.UnableToLocateRepository, initialPath);
             }

src/Microsoft.Build.Tasks.Git/build/Microsoft.Build.Tasks.Git.targets

@@ -27,7 +27,7 @@
       Path="$(MSBuildProjectDirectory)"
       RemoteName="$(GitRepositoryRemoteName)"
       ConfigurationScope="$(GitRepositoryConfigurationScope)"
-      NoWarnOnMissingRepository="$(PkgMicrosoft_Build_Tasks_Git.Equals(''))">
+      NoWarnOnMissingInfo="$(PkgMicrosoft_Build_Tasks_Git.Equals(''))">
 
       <Output TaskParameter="RepositoryId" PropertyName="_GitRepositoryId" />
       <Output TaskParameter="Url" PropertyName="ScmRepositoryUrl" />

src/SourceLink.Common.UnitTests/GenerateSourceLinkFileTests.cs

@@ -33,9 +33,44 @@ public void Empty(bool noWarning)
 
             Assert.True(task.Execute());
 
-            AssertEx.AssertEqualToleratingWhitespaceDifferences(
-                noWarning ? "" : "WARNING : " + string.Format(Resources.SourceControlInformationIsNotAvailableGeneratedSourceLinkEmpty),
-                engine.Log);
+            var expectedOutput = 
+                (noWarning ? "" : "WARNING : " + string.Format(Resources.SourceControlInformationIsNotAvailableGeneratedSourceLinkEmpty) + Environment.NewLine) +
+                string.Format(Resources.SourceLinkEmptyNoExistingFile, sourceLinkFilePath);
+
+            AssertEx.AssertEqualToleratingWhitespaceDifferences(expectedOutput, engine.Log);
+
+            Assert.Null(task.SourceLink);
+            Assert.Null(task.FileWrite);
+        }
+
+        [Theory]
+        [CombinatorialData]
+        public void NoRepositoryUrl(bool noWarning)
+        {
+            var sourceLinkFilePath = Path.Combine(TempRoot.Root, Guid.NewGuid().ToString());
+
+            var engine = new MockEngine();
+
+            var task = new GenerateSourceLinkFile()
+            {
+                BuildEngine = engine,
+                OutputFile = sourceLinkFilePath,
+                SourceRoots = new[]
+                {
+                    new MockItem("/1/", KVP("MappedPath", "/1/")),
+                    new MockItem("/2/", KVP("MappedPath", "/2/"), KVP("RevisionId", "f3dbcdfdd5b1f75613c7692f969d8df121fc3731"), KVP("SourceControl", "git")),
+                    new MockItem("/3/", KVP("MappedPath", "/3/"), KVP("RevisionId", "f3dbcdfdd5b1f75613c7692f969d8df121fc3731"), KVP("SourceControl", "git"), KVP("RepositoryUrl", "")),
+                },
+                NoWarnOnMissingSourceControlInformation = noWarning,
+            };
+
+            Assert.True(task.Execute());
+
+            var expectedOutput = 
+                (noWarning ? "" : "WARNING : " + string.Format(Resources.SourceControlInformationIsNotAvailableGeneratedSourceLinkEmpty) + Environment.NewLine) +
+                string.Format(Resources.SourceLinkEmptyNoExistingFile, sourceLinkFilePath);
+
+            AssertEx.AssertEqualToleratingWhitespaceDifferences(expectedOutput, engine.Log);
 
             Assert.Null(task.SourceLink);
             Assert.Null(task.FileWrite);
@@ -61,7 +96,8 @@ public void Empty_DeleteExistingFile()
 
             Assert.True(task.Execute());
 
-            AssertEx.AssertEqualToleratingWhitespaceDifferences("", engine.Log);
+            AssertEx.AssertEqualToleratingWhitespaceDifferences(
+                string.Format(Resources.SourceLinkEmptyDeletingExistingFile, sourceLinkFile.Path), engine.Log);
 
             Assert.Null(task.SourceLink);
             Assert.Equal(sourceLinkFile.Path, task.FileWrite);

src/SourceLink.Common/GenerateSourceLinkFile.cs

@@ -124,6 +124,8 @@ private void WriteSourceLinkFile(string? content)
                 {
                     if (content == null)
                     {
+                        Log.LogMessage(Resources.SourceLinkEmptyDeletingExistingFile, OutputFile);
+
                         File.Delete(OutputFile);
                         FileWrite = OutputFile;
                         return;
@@ -133,6 +135,8 @@ private void WriteSourceLinkFile(string? content)
                     if (originalContent == content)
                     {
                         // Don't rewrite the file if the contents is the same, just pass it to the compiler.
+                        Log.LogMessage(Resources.SourceLinkFileUpToDate, OutputFile);
+
                         SourceLink = OutputFile;
                         return;
                     }
@@ -141,9 +145,11 @@ private void WriteSourceLinkFile(string? content)
                 {
                     // File doesn't exist and the output is empty:
                     // Do not write the file and don't pass it to the compiler.
+                    Log.LogMessage(Resources.SourceLinkEmptyNoExistingFile, OutputFile);
                     return;
                 }
 
+                Log.LogMessage(Resources.SourceLinkFileUpdated, OutputFile);
                 File.WriteAllText(OutputFile, content);
                 FileWrite = SourceLink = OutputFile;
             }