Skip to content

Fix RepositoryPaths.BuildVariables temp file deletion issue #151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions IntelliTect.Multitool.Tests/RepositoryPaths.Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,38 @@ public void TrySearchForGitContainingDirectory_ReturnsFalseWhenNotFound()
Assert.False(RepositoryPaths.TrySearchForGitContainingDirectory(new DirectoryInfo(path), out string gitParentDirectory));
Assert.Empty(gitParentDirectory);
}

[Fact]
public void BuildVariables_HandlesFileNotFound_Gracefully()
{
// Save current temp file if it exists
string tempFilePath = Path.Combine(Path.GetTempPath(), RepositoryPaths.BuildVariableFileName);
string? backupContent = null;
bool fileExisted = File.Exists(tempFilePath);
if (fileExisted)
{
backupContent = File.ReadAllText(tempFilePath);
File.Delete(tempFilePath);
}

try
{
// Reset the static field to force re-initialization
var field = typeof(RepositoryPaths).GetField("_buildVariables", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static);
Copy link
Preview

Copilot AI Jun 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Using reflection to reset a private static field makes the test brittle. Consider adding an internal reset method or using a public test hook to clear the cache more robustly.

Copilot uses AI. Check for mistakes.

field?.SetValue(null, null);

// Access BuildVariables when file doesn't exist - should not throw
var buildVars = RepositoryPaths.BuildVariables;
Assert.NotNull(buildVars);
Assert.Empty(buildVars);
}
finally
{
// Restore the file if it existed
if (fileExisted && backupContent != null)
{
File.WriteAllText(tempFilePath, backupContent);
}
}
}
}
42 changes: 38 additions & 4 deletions IntelliTect.Multitool/RepositoryPaths.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,44 @@ public static class RepositoryPaths
/// <summary>
/// Holds the key value pairs of the build variables that this class can use.
/// </summary>
public static ReadOnlyDictionary<string, string?> BuildVariables { get; } = new(File.ReadAllLines(Path.Combine(Path.GetTempPath(), BuildVariableFileName))
.Select(line => line.Split("::"))
.ToDictionary(split => split[0].Trim(),
split => !string.IsNullOrEmpty(split[1]) ? split[1].Trim() : null));
public static ReadOnlyDictionary<string, string?> BuildVariables
{
get
{
if (_buildVariables == null)
{
_buildVariables = LoadBuildVariables();
}
return _buildVariables;
}
}

private static ReadOnlyDictionary<string, string?>? _buildVariables;
Comment on lines +18 to +30
Copy link
Preview

Copilot AI Jun 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lazy initialization of _buildVariables isn’t thread-safe. Consider using Lazy or a lock to prevent race conditions if BuildVariables is accessed concurrently.

Suggested change
public static ReadOnlyDictionary<string, string?> BuildVariables
{
get
{
if (_buildVariables == null)
{
_buildVariables = LoadBuildVariables();
}
return _buildVariables;
}
}
private static ReadOnlyDictionary<string, string?>? _buildVariables;
public static ReadOnlyDictionary<string, string?> BuildVariables => _buildVariables.Value;
private static readonly Lazy<ReadOnlyDictionary<string, string?>> _buildVariables =
new Lazy<ReadOnlyDictionary<string, string?>>(LoadBuildVariables);

Copilot uses AI. Check for mistakes.


private static ReadOnlyDictionary<string, string?> LoadBuildVariables()
{
try
{
string filePath = Path.Combine(Path.GetTempPath(), BuildVariableFileName);
var lines = File.ReadAllLines(filePath);
var dictionary = lines
.Select(line => line.Split("::"))
Copy link
Preview

Copilot AI Jun 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Split("::") splits on individual ':' characters rather than the literal "::" token. If the intent is to split on "::", use Split(new[] { "::" }, StringSplitOptions.None) for correct behavior.

Suggested change
.Select(line => line.Split("::"))
.Select(line => line.Split(new[] { "::" }, StringSplitOptions.None))

Copilot uses AI. Check for mistakes.

.ToDictionary(split => split[0].Trim(),
split => !string.IsNullOrEmpty(split[1]) ? split[1].Trim() : null);
return new ReadOnlyDictionary<string, string?>(dictionary);
}
catch (FileNotFoundException)
{
// Return empty dictionary if the build variables file doesn't exist
// This can happen when the temp file is cleaned up by the OS
return new ReadOnlyDictionary<string, string?>(new Dictionary<string, string?>());
}
catch (DirectoryNotFoundException)
{
// Return empty dictionary if the temp directory doesn't exist
return new ReadOnlyDictionary<string, string?>(new Dictionary<string, string?>());
Comment on lines +48 to +53
Copy link
Preview

Copilot AI Jun 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Allocating a new empty dictionary on each catch can be wasteful. Consider defining a static readonly empty ReadOnlyDictionary to reuse and avoid repeated allocations.

Suggested change
return new ReadOnlyDictionary<string, string?>(new Dictionary<string, string?>());
}
catch (DirectoryNotFoundException)
{
// Return empty dictionary if the temp directory doesn't exist
return new ReadOnlyDictionary<string, string?>(new Dictionary<string, string?>());
return _emptyReadOnlyDictionary;
}
catch (DirectoryNotFoundException)
{
// Return empty dictionary if the temp directory doesn't exist
return _emptyReadOnlyDictionary;

Copilot uses AI. Check for mistakes.

}
}

/// <summary>
/// Finds the root of the repository by looking for the directory containing the .git directory.
Expand Down