diff validate improvements (#1782)

* Allow relative path handling in diff validation and enhance root determination

- Updated `ValidateRedirects` method to support relative paths.
- Improved handling of source directory root in diff validation.
- Fixed path normalization in `IntegrationGitRepositoryTracker`.

* Do not pop unrelated changes

* Refactor `RedirectFile` initialization and enhance `IRepositoryTracker` for improved redirect handling

- Simplified `RedirectFile` creation logic and constructor overloading.
- Changed `IRepositoryTracker` to return a `IReadOnlyCollection` for consistency.
- Enhanced path normalization across repository trackers.
- Adjusted dependent tests and updated related YAML documentation.

* Exclude `_snippets` folder from missing redirects detection in `DiffCommands`

* Add HasParent extension method on IFileInfo and IDirectoryInfo

* Only consider markdown file changes

* use string.Equals

* update logging

* update filter, include markdowns without parent _snippet folder

* Add StringComparison to file/directory extensions, with hearistic for default for IsSubPathOf

* update tests

* Address a bunch of code-analysis quick fixes (#1761)

Author: Mpdreamz

docs/contribute/redirects.md

@@ -86,7 +86,7 @@ This example strips all anchors from the source page.
 Any remaining links resolving to anchors on `7th-page.md` will fail link validation.
 
 ```yaml
-redirects
+redirects:
   'testing/redirects/7th-page.md':
     to: 'testing/redirects/5th-page.md'
     anchors: '!'

src/Elastic.Documentation.Configuration/Builder/ConfigurationFile.cs

@@ -73,12 +73,10 @@ public ConfigurationFile(IDocumentationSetContext context, VersionsConfiguration
 			return;
 		}
 
-		var sourceFile = context.ConfigurationPath;
-		var redirectFileName = sourceFile.Name.StartsWith('_') ? "_redirects.yml" : "redirects.yml";
-		var redirectFileInfo = sourceFile.FileSystem.FileInfo.New(Path.Combine(sourceFile.Directory!.FullName, redirectFileName));
-		var redirectFile = new RedirectFile(redirectFileInfo, _context);
+		var redirectFile = new RedirectFile(_context);
 		Redirects = redirectFile.Redirects;
 
+		var sourceFile = context.ConfigurationPath;
 		var reader = new YamlStreamReader(sourceFile, _context.Collector);
 		try
 		{

src/Elastic.Documentation.Configuration/Builder/RedirectFile.cs

@@ -10,16 +10,19 @@ namespace Elastic.Documentation.Configuration.Builder;
 
 public record RedirectFile
 {
-	public Dictionary<string, LinkRedirect>? Redirects { get; set; }
-	private IFileInfo Source { get; init; }
-	private IDocumentationSetContext Context { get; init; }
+	public Dictionary<string, LinkRedirect>? Redirects { get; }
+	public IFileInfo Source { get; }
+	private IDocumentationSetContext Context { get; }
 
-	public RedirectFile(IFileInfo source, IDocumentationSetContext context)
+	public RedirectFile(IDocumentationSetContext context, IFileInfo? source = null)
 	{
-		Source = source;
+		var docsetConfigurationPath = context.ConfigurationPath;
+		var redirectFileName = docsetConfigurationPath.Name.StartsWith('_') ? "_redirects.yml" : "redirects.yml";
+		var redirectFileInfo = docsetConfigurationPath.FileSystem.FileInfo.New(Path.Combine(docsetConfigurationPath.Directory!.FullName, redirectFileName));
+		Source = source ?? redirectFileInfo;
 		Context = context;
 
-		if (!source.Exists)
+		if (!Source.Exists)
 			return;
 
 		var reader = new YamlStreamReader(Source, Context.Collector);

src/Elastic.Documentation/Extensions/IFileInfoExtensions.cs

@@ -2,7 +2,9 @@
 // Elasticsearch B.V licenses this file to you under the Apache 2.0 License.
 // See the LICENSE file in the project root for more information
 
+using System.Globalization;
 using System.IO.Abstractions;
+using static System.StringComparison;
 
 namespace Elastic.Documentation.Extensions;
 
@@ -25,17 +27,68 @@ public static bool IsSubPathOf(this IFileInfo file, IDirectoryInfo parentDirecto
 		var parent = file.Directory;
 		return parent is not null && parent.IsSubPathOf(parentDirectory);
 	}
+
+	/// Checks if <paramref name="file"/> has parent directory <paramref name="parentName"/>, defaults to OrdinalIgnoreCase comparison
+	public static bool HasParent(this IFileInfo file, string parentName)
+	{
+		var parent = file.Directory;
+		return parent is not null && parent.HasParent(parentName);
+	}
 }
 
 public static class IDirectoryInfoExtensions
 {
+	private static bool? CaseSensitiveOsCheck;
+	public static bool IsCaseSensitiveFileSystem
+	{
+
+		get
+		{
+			// heuristic to determine if the OS is case-sensitive
+			try
+			{
+				var tmp = Path.GetTempPath();
+				if (CaseSensitiveOsCheck.HasValue)
+					return CaseSensitiveOsCheck.Value;
+				var culture = CultureInfo.CurrentCulture;
+				CaseSensitiveOsCheck = !Directory.Exists(tmp.ToUpper(culture)) || !Directory.Exists(tmp.ToLower(culture));
+				return CaseSensitiveOsCheck ?? false;
+
+			}
+			catch
+			{
+				// fallback to case-insensitive unless it's linux
+				CaseSensitiveOsCheck = Environment.OSVersion.Platform == PlatformID.Unix;
+				return false;
+			}
+		}
+	}
+
+
 	/// Validates <paramref name="directory"/> is subdirectory of <paramref name="parentDirectory"/>
 	public static bool IsSubPathOf(this IDirectoryInfo directory, IDirectoryInfo parentDirectory)
 	{
+		var cmp = IsCaseSensitiveFileSystem ? Ordinal : OrdinalIgnoreCase;
+		var parent = directory;
+		do
+		{
+			if (string.Equals(parent.FullName, parentDirectory.FullName, cmp))
+				return true;
+			parent = parent.Parent;
+		} while (parent != null);
+
+		return false;
+	}
+
+	/// Checks if <paramref name="directory"/> has parent directory <paramref name="parentName"/>, defaults to OrdinalIgnoreCase comparison
+	public static bool HasParent(this IDirectoryInfo directory, string parentName, StringComparison comparison = OrdinalIgnoreCase)
+	{
+		if (string.Equals(directory.Name, parentName, comparison))
+			return true;
 		var parent = directory;
 		do
 		{
-			if (parent.FullName == parentDirectory.FullName)
+			if (string.Equals(parent.Name, parentName, comparison))
 				return true;
 			parent = parent.Parent;
 		} while (parent != null);

src/tooling/docs-builder/Cli/DiffCommands.cs

@@ -10,6 +10,7 @@
 using Elastic.Documentation;
 using Elastic.Documentation.Configuration;
 using Elastic.Documentation.Configuration.Builder;
+using Elastic.Documentation.Extensions;
 using Elastic.Documentation.Tooling.Diagnostics.Console;
 using Microsoft.Extensions.Logging;
 
@@ -26,57 +27,79 @@ IConfigurationContext configurationContext
 	/// <summary>
 	/// Validates redirect updates in the current branch using the redirect file against changes reported by git.
 	/// </summary>
-	/// <param name="path">The baseline path to perform the check</param>
+	/// <param name="path"> -p, Defaults to the`{pwd}/docs` folder</param>
 	/// <param name="ctx"></param>
 	[SuppressMessage("Usage", "CA2254:Template should be a static expression")]
 	[Command("validate")]
-	public async Task<int> ValidateRedirects([Argument] string? path = null, Cancel ctx = default)
+	public async Task<int> ValidateRedirects(string? path = null, Cancel ctx = default)
 	{
 		var runningOnCi = !string.IsNullOrEmpty(Environment.GetEnvironmentVariable("GITHUB_ACTIONS"));
-		path ??= "docs";
 
 		await using var collector = new ConsoleDiagnosticsCollector(logFactory, githubActionsService).StartAsync(ctx);
 
 		var fs = new FileSystem();
-		var root = fs.DirectoryInfo.New(Paths.WorkingDirectoryRoot.FullName);
 
-		var buildContext = new BuildContext(collector, fs, fs, configurationContext, ExportOptions.MetadataOnly, root.FullName, null);
-		var sourceFile = buildContext.ConfigurationPath;
-		var redirectFileName = sourceFile.Name.StartsWith('_') ? "_redirects.yml" : "redirects.yml";
-		var redirectFileInfo = sourceFile.FileSystem.FileInfo.New(Path.Combine(sourceFile.Directory!.FullName, redirectFileName));
-		if (!redirectFileInfo.Exists)
+		var buildContext = new BuildContext(collector, fs, fs, configurationContext, ExportOptions.MetadataOnly, path, null);
+		var redirectFile = new RedirectFile(buildContext);
+		if (!redirectFile.Source.Exists)
 		{
 			await collector.StopAsync(ctx);
 			return 0;
 		}
 
-		var redirectFileParser = new RedirectFile(redirectFileInfo, buildContext);
-		var redirects = redirectFileParser.Redirects;
+		var redirects = redirectFile.Redirects;
 
 		if (redirects is null)
 		{
-			collector.EmitError(redirectFileInfo, "It was not possible to parse the redirects file.");
+			collector.EmitError(redirectFile.Source, "It was not possible to parse the redirects file.");
 			await collector.StopAsync(ctx);
 			return collector.Errors;
 		}
 
-		IRepositoryTracker tracker = runningOnCi ? new IntegrationGitRepositoryTracker(path) : new LocalGitRepositoryTracker(collector, root, path);
-		var changed = tracker.GetChangedFiles().ToArray();
+		var root = Paths.DetermineSourceDirectoryRoot(buildContext.DocumentationSourceDirectory);
+		if (root is null)
+		{
+			collector.EmitError(redirectFile.Source, $"Unable to determine the root of the source directory {buildContext.DocumentationSourceDirectory}.");
+			await collector.StopAsync(ctx);
+			return collector.Errors;
+		}
+		var relativePath = Path.GetRelativePath(root.FullName, buildContext.DocumentationSourceDirectory.FullName);
+		_log.LogInformation("Using relative path {RelativePath} for validating changes", relativePath);
+		IRepositoryTracker tracker = runningOnCi ? new IntegrationGitRepositoryTracker(relativePath) : new LocalGitRepositoryTracker(collector, root, relativePath);
+		var changed = tracker.GetChangedFiles()
+			.Where(c =>
+			{
+				var fi = fs.FileInfo.New(c.FilePath);
+				return fi.Extension is ".md" && !fi.HasParent("_snippets");
+			})
+			.ToArray();
 
 		if (changed.Length != 0)
 			_log.LogInformation("Found {Count} changes to files related to documentation in the current branch.", changed.Length);
 
-		foreach (var notFound in changed.DistinctBy(c => c.FilePath).Where(c => c.ChangeType is GitChangeType.Deleted or GitChangeType.Renamed
-																	&& !redirects.ContainsKey(c is RenamedGitChange renamed ? renamed.OldFilePath : c.FilePath)))
+		var missingRedirects = changed
+			.Where(c =>
+				c.ChangeType is GitChangeType.Deleted or GitChangeType.Renamed
+				&& !redirects.ContainsKey(c is RenamedGitChange renamed ? renamed.OldFilePath : c.FilePath)
+			)
+			.ToArray();
+
+		if (missingRedirects.Length != 0)
+		{
+			var relativeRedirectFile = Path.GetRelativePath(root.FullName, redirectFile.Source.FullName);
+			_log.LogInformation("Found {Count} changes that still require updates to: {RedirectFile}", missingRedirects.Length, relativeRedirectFile);
+		}
+
+		foreach (var notFound in missingRedirects)
 		{
 			if (notFound is RenamedGitChange renamed)
 			{
-				collector.EmitError(redirectFileInfo.Name,
+				collector.EmitError(redirectFile.Source,
 					$"File '{renamed.OldFilePath}' was renamed to '{renamed.NewFilePath}' but it has no redirect configuration set.");
 			}
 			else if (notFound.ChangeType is GitChangeType.Deleted)
 			{
-				collector.EmitError(redirectFileInfo.Name,
+				collector.EmitError(redirectFile.Source,
 					$"File '{notFound.FilePath}' was deleted but it has no redirect targets. This will lead to broken links.");
 			}
 		}

src/tooling/docs-builder/Tracking/IRepositoryTracker.cs

@@ -19,5 +19,5 @@ public record RenamedGitChange(string OldFilePath, string NewFilePath, GitChange
 
 public interface IRepositoryTracker
 {
-	IEnumerable<GitChange> GetChangedFiles();
+	IReadOnlyCollection<GitChange> GetChangedFiles();
 }

src/tooling/docs-builder/Tracking/IntegrationGitRepositoryTracker.cs

@@ -6,39 +6,45 @@ namespace Documentation.Builder.Tracking;
 
 public class IntegrationGitRepositoryTracker(string lookupPath) : IRepositoryTracker
 {
-	private string LookupPath { get; } = $"{lookupPath}/";
-	public IEnumerable<GitChange> GetChangedFiles()
+	private string LookupPath { get; } = $"{lookupPath.Trim(['/', '\\'])}/";
+	public IReadOnlyCollection<GitChange> GetChangedFiles()
 	{
-		var deletedFiles = Environment.GetEnvironmentVariable("DELETED_FILES") ?? string.Empty;
-		if (!string.IsNullOrEmpty(deletedFiles))
-		{
-			foreach (var file in deletedFiles.Split(' ', StringSplitOptions.RemoveEmptyEntries).Where(f => f.StartsWith(LookupPath)))
-				yield return new GitChange(file, GitChangeType.Deleted);
-		}
+		return GetChanges().ToArray();
 
-		var addedFiles = Environment.GetEnvironmentVariable("ADDED_FILES");
-		if (!string.IsNullOrEmpty(addedFiles))
+		IEnumerable<GitChange> GetChanges()
 		{
-			foreach (var file in addedFiles.Split(' ', StringSplitOptions.RemoveEmptyEntries).Where(f => f.StartsWith(LookupPath)))
-				yield return new GitChange(file, GitChangeType.Added);
-		}
+			var deletedFiles = Environment.GetEnvironmentVariable("DELETED_FILES") ?? string.Empty;
+			if (!string.IsNullOrEmpty(deletedFiles))
+			{
+				foreach (var file in deletedFiles.Split(' ', StringSplitOptions.RemoveEmptyEntries).Where(f => f.StartsWith(LookupPath)))
+					yield return new GitChange(file, GitChangeType.Deleted);
+			}
 
-		var modifiedFiles = Environment.GetEnvironmentVariable("MODIFIED_FILES");
-		if (!string.IsNullOrEmpty(modifiedFiles))
-		{
-			foreach (var file in modifiedFiles.Split(' ', StringSplitOptions.RemoveEmptyEntries).Where(f => f.StartsWith(LookupPath)))
-				yield return new GitChange(file, GitChangeType.Modified);
-		}
+			var addedFiles = Environment.GetEnvironmentVariable("ADDED_FILES");
+			if (!string.IsNullOrEmpty(addedFiles))
+			{
+				foreach (var file in addedFiles.Split(' ', StringSplitOptions.RemoveEmptyEntries).Where(f => f.StartsWith(LookupPath)))
+					yield return new GitChange(file, GitChangeType.Added);
+			}
 
-		var renamedFiles = Environment.GetEnvironmentVariable("RENAMED_FILES");
-		if (!string.IsNullOrEmpty(renamedFiles))
-		{
-			foreach (var pair in renamedFiles.Split(' ', StringSplitOptions.RemoveEmptyEntries).Where(f => f.StartsWith(LookupPath)))
+			var modifiedFiles = Environment.GetEnvironmentVariable("MODIFIED_FILES");
+			if (!string.IsNullOrEmpty(modifiedFiles))
 			{
-				var parts = pair.Split(':');
-				if (parts.Length == 2)
-					yield return new RenamedGitChange(parts[0], parts[1], GitChangeType.Renamed);
+				foreach (var file in modifiedFiles.Split(' ', StringSplitOptions.RemoveEmptyEntries).Where(f => f.StartsWith(LookupPath)))
+					yield return new GitChange(file, GitChangeType.Modified);
 			}
+
+			var renamedFiles = Environment.GetEnvironmentVariable("RENAMED_FILES");
+			if (!string.IsNullOrEmpty(renamedFiles))
+			{
+				foreach (var pair in renamedFiles.Split(' ', StringSplitOptions.RemoveEmptyEntries).Where(f => f.StartsWith(LookupPath)))
+				{
+					var parts = pair.Split(':');
+					if (parts.Length == 2)
+						yield return new RenamedGitChange(parts[0], parts[1], GitChangeType.Renamed);
+				}
+			}
+
 		}
 	}
 }

src/tooling/docs-builder/Tracking/LocalGitRepositoryTracker.cs

@@ -10,16 +10,20 @@ namespace Documentation.Builder.Tracking;
 
 public class LocalGitRepositoryTracker(DiagnosticsCollector collector, IDirectoryInfo workingDirectory, string lookupPath) : ExternalCommandExecutor(collector, workingDirectory), IRepositoryTracker
 {
-	private string LookupPath { get; } = lookupPath;
+	private string LookupPath { get; } = lookupPath.Trim('\\', '/');
 
-	public IEnumerable<GitChange> GetChangedFiles()
+	public IReadOnlyCollection<GitChange> GetChangedFiles()
 	{
 		var defaultBranch = GetDefaultBranch();
 		var commitChanges = CaptureMultiple("git", "diff", "--name-status", $"{defaultBranch}...HEAD", "--", $"./{LookupPath}");
 		var localChanges = CaptureMultiple("git", "status", "--porcelain");
-		ExecInSilent([], "git", "stash", "push", "--", $"./{LookupPath}");
-		var localUnstagedChanges = CaptureMultiple("git", "stash", "show", "--name-status", "-u");
-		ExecInSilent([], "git", "stash", "pop");
+		var gitStashDocsFolder = Capture("git", "stash", "push", "--", $"./{LookupPath}");
+		var localUnstagedChanges = Array.Empty<string>();
+		if (!string.Equals(gitStashDocsFolder, "No local changes to save", StringComparison.Ordinal))
+		{
+			localUnstagedChanges = CaptureMultiple("git", "stash", "show", "--name-status", "-u");
+			ExecInSilent([], "git", "stash", "pop");
+		}
 
 		return [.. GetCommitChanges(commitChanges), .. GetLocalChanges(localChanges), .. GetCommitChanges(localUnstagedChanges)];
 	}

tests/Elastic.Markdown.Tests/FileSystemExtensionsTests.cs

@@ -0,0 +1,57 @@
+// Licensed to Elasticsearch B.V under one or more agreements.
+// Elasticsearch B.V licenses this file to you under the Apache 2.0 License.
+// See the LICENSE file in the project root for more information
+
+using System.IO.Abstractions.TestingHelpers;
+using Elastic.Documentation;
+using Elastic.Documentation.Configuration;
+using Elastic.Documentation.Configuration.Versions;
+using Elastic.Documentation.Diagnostics;
+using Elastic.Documentation.Extensions;
+using Elastic.Markdown.IO;
+using FluentAssertions;
+
+namespace Elastic.Markdown.Tests;
+
+public class FileSystemExtensionsTest(ITestOutputHelper output)
+{
+	[Fact]
+	public void IsSubPathOfTests()
+	{
+		var fs = new MockFileSystem();
+
+		fs.DirectoryInfo.New("/a/b/c/d").IsSubPathOf(fs.DirectoryInfo.New("/a/b/c/d/e/f")).Should().BeFalse();
+		fs.DirectoryInfo.New("/a/b/c/d/e/f").IsSubPathOf(fs.DirectoryInfo.New("/a/b/c/d")).Should().BeTrue();
+
+		var caseSensitive = IDirectoryInfoExtensions.IsCaseSensitiveFileSystem;
+
+		if (caseSensitive)
+		{
+			fs.DirectoryInfo.New("/a/b/C/d/e/f").IsSubPathOf(fs.DirectoryInfo.New("/a/b/c/d")).Should().BeFalse();
+			fs.DirectoryInfo.New("/a/b/c/d/e/f").IsSubPathOf(fs.DirectoryInfo.New("/a/b/C/d")).Should().BeFalse();
+		}
+		else
+		{
+			fs.DirectoryInfo.New("/a/b/C/d/e/f").IsSubPathOf(fs.DirectoryInfo.New("/a/b/c/d")).Should().BeTrue();
+			fs.DirectoryInfo.New("/a/b/c/d/e/f").IsSubPathOf(fs.DirectoryInfo.New("/a/b/C/d")).Should().BeTrue();
+		}
+	}
+
+	[Fact]
+	public void HasParentTests()
+	{
+		var fs = new MockFileSystem();
+
+		fs.DirectoryInfo.New("/a/b/c/d").HasParent("c").Should().BeTrue();
+		fs.DirectoryInfo.New("/a/b/c/d/e").HasParent("e").Should().BeTrue();
+
+		fs.DirectoryInfo.New("/a/b/C/d").HasParent("c", StringComparison.Ordinal).Should().BeFalse();
+		var caseSensitive = IDirectoryInfoExtensions.IsCaseSensitiveFileSystem;
+
+		// HasParent is always case-insensitive by default
+		if (caseSensitive)
+			fs.DirectoryInfo.New("/a/b/C/d").HasParent("c").Should().BeTrue();
+		else
+			fs.DirectoryInfo.New("/a/b/C/d").HasParent("c").Should().BeTrue();
+	}
+}