From 30e103f0215332fdbea4c3d6ee81fc24ebddbba0 Mon Sep 17 00:00:00 2001 From: Scott Date: Fri, 21 Aug 2026 16:31:22 +1000 Subject: [PATCH 1/2] tests(git-provider): Add duplicated repo registration test --- src/ModuleCore/Git/GitManager.cs | 11 +++++++++-- tests/ModuleTests/Git/AddRegistrationTests.cs | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/ModuleCore/Git/GitManager.cs b/src/ModuleCore/Git/GitManager.cs index 0d5af37..25a5b41 100644 --- a/src/ModuleCore/Git/GitManager.cs +++ b/src/ModuleCore/Git/GitManager.cs @@ -138,8 +138,15 @@ public class GitManager // we add it to the relevant dictionary if (directorySegmentsFromName.Count == 0) { - FullRepositoryPath = absoluteRepositoryLocation; - return this; + // If this is a new registration, set the FullRepositoryPath and return, otherwise we've got a duplicate + // entry and we throw + if (FullRepositoryPath == null) + { + FullRepositoryPath = absoluteRepositoryLocation; + return this; + } + + throw new Exception($"Registration already exists for {InternalPath}"); } var nextSegment = directorySegmentsFromName.Peek(); diff --git a/tests/ModuleTests/Git/AddRegistrationTests.cs b/tests/ModuleTests/Git/AddRegistrationTests.cs index 6de31d6..e04cfc3 100644 --- a/tests/ModuleTests/Git/AddRegistrationTests.cs +++ b/tests/ModuleTests/Git/AddRegistrationTests.cs @@ -74,4 +74,23 @@ public class AddRegistrationTests Assert.Equal("repo", whitespaceName); } + + [Fact] + public void DuplicateRepoRegistrationShouldFail() + { + Settings.UseFileName(nameof(RepoRegistrationWithWhitespaceName)); + + var gitManager = GitManager.InternalFreshInstance; + var testRepoAbsolutePath = "Test:/some/test/repo"; + string[] paths = ["test", "nested", "path"]; + var names = (NormalSeparator: string.Join(Path.DirectorySeparatorChar, paths), AltSeparator: string.Join(Path.AltDirectorySeparatorChar, paths)); + + var firstRegistration = gitManager.RegisterRepo(testRepoAbsolutePath, names.NormalSeparator); + // TODO: make nested registrations fail in both directions and test + var secondRegistration = gitManager.RegisterRepo(testRepoAbsolutePath, Path.Combine(paths[..1])); + + Assert.Equal(Path.Combine(paths), firstRegistration); + + Assert.Throws(() => gitManager.RegisterRepo(testRepoAbsolutePath, names.AltSeparator)); + } } \ No newline at end of file From 2b3c7d0153fbbdfdafe0b69a8bea29f1ada7d625 Mon Sep 17 00:00:00 2001 From: Scott Date: Fri, 21 Aug 2026 16:53:40 +1000 Subject: [PATCH 2/2] feat(git-provider): Remove concept of directory registrations and keep it simple --- src/ModuleCore/Git/GitManager.cs | 158 +++--------------- tests/ModuleTests/Git/AddRegistrationTests.cs | 23 ++- .../BasicRepoRegistration_2.verified.txt | 2 +- 3 files changed, 45 insertions(+), 138 deletions(-) diff --git a/src/ModuleCore/Git/GitManager.cs b/src/ModuleCore/Git/GitManager.cs index 25a5b41..fe1f69e 100644 --- a/src/ModuleCore/Git/GitManager.cs +++ b/src/ModuleCore/Git/GitManager.cs @@ -1,4 +1,6 @@ -namespace ModuleCore.Git; +using System.Collections.Concurrent; + +namespace ModuleCore.Git; // TODO: better name for this public class GitManager @@ -11,23 +13,19 @@ public class GitManager /// internal static GitManager InternalFreshInstance => new(); - /// - /// Simply .ToString() - /// - private static readonly string DirectorySeparator = Path.DirectorySeparatorChar.ToString(); + private class GitRegistration + { + public required string Name { get; set; } + public required string Location { get; set; } + } - private readonly InternalDirectory _repositories; - private readonly Lock _readWriteLock = new(); + private readonly ConcurrentDictionary _registrations; private GitManager() { Console.WriteLine($"{nameof(GitManager)} init"); - // Initialise the root container - _repositories = new InternalDirectory() - { - Name = DirectorySeparator, - InternalPath = DirectorySeparator - }; + + _registrations = new ConcurrentDictionary(); } /// @@ -39,131 +37,19 @@ public class GitManager /// The normalised string the repository was registered against public string RegisterRepo(string absoluteRepositoryLocation, string registrationName) { - // Depending on the caller, it might be possible that they've scripted automatic repo registration. Because I - // don't really want to account to all the subtle ways that can be parallised, I just naively lock on every - // registration attempt. This method should be quick regardless, and I could use ConcurrentDictionary except - // that means every instance of InternalDirectory would need it and yeah nah fuck that I can just lock at the - // top level - lock (_readWriteLock) + registrationName = string.IsNullOrWhiteSpace(registrationName) + ? new DirectoryInfo(absoluteRepositoryLocation).Name + : registrationName; + + if (_registrations.TryAdd(registrationName, new GitRegistration() + { + Name = registrationName, + Location = absoluteRepositoryLocation, + })) { - var normalisedName = NormaliseNamePath(string.IsNullOrWhiteSpace(registrationName) - ? new DirectoryInfo(absoluteRepositoryLocation).Name - : registrationName); - - // Regardless of if we get a name or not, the fully qualified version for us - // starts with a / - var directorySegmentsFromName = NameToSegments(normalisedName); - - var added = _repositories.Add(absoluteRepositoryLocation, directorySegmentsFromName); - - // Not sure about this, the Add should throw any exceptions on duplicate/failures but for now I'll leave this - // here - if (added == null) - { - throw new Exception("Failed to register location"); - } - - return normalisedName; + return registrationName; } - } - /// - /// Takes a name and returns it as a queue of its parts, starting with a root of - /// - /// - /// - private Queue NameToSegments(string name) - { - var segments = name.Split(DirectorySeparator); - - return segments.Length == 1 - ? new Queue([DirectorySeparator, name]) - : new Queue([DirectorySeparator, ..segments]); - } - - /// - /// Normalises the path separators in the given string to use Path.DirectorySeparatorChar - /// - /// - /// - private string NormaliseNamePath(string name) - { - // Feels a bit hacky, but this will actually normalise a path to a valid form. So if the input is - // some/directory/paths, Path.GetRelativePath will normalise it to some\directory\paths, relative to ./ - // which is kind of handy but I also just wish there was a Path method that would do this for me. I know that - // the whole point of Path is that it's based on a file system, but file systems can also be arbitrary and not - // always be drive rooted. - // Either way, this works and saves me having to reimplement a worse method when it's more important that users - // are able to use file paths in whatever form they prefer, which means we leverage the internal implementation - // in a weird way. - return Path.GetRelativePath("./", name); - } - - private class InternalDirectory - { - /// - /// Name of the folder this - /// - public string Name { get; set; } = null!; - - public Dictionary Children { get; set; } = []; - - internal string InternalPath { get; set; } - - /// - /// If not null, this is the absolute location of a registered git repository - /// - public string? FullRepositoryPath { get; set; } - - /// - /// - /// - /// - /// - /// - /// - internal InternalDirectory? Add(string absoluteRepositoryLocation, Queue directorySegmentsFromName) - { - var topStack = directorySegmentsFromName.Dequeue(); - - if (topStack != Name) - { - // logically it shouldn't be possible to have a value on top of the stack that _doesn't_ exist, but - // just incase we throw as this should only happen if an Add is attempted on the root and the queue was - // not correctly rooted to / - throw new Exception($"Directory segment does not seem to exist: {topStack}"); - } - - // We're at the end of the directory segments so we can safely say we're at the end of the tree so - // we add it to the relevant dictionary - if (directorySegmentsFromName.Count == 0) - { - // If this is a new registration, set the FullRepositoryPath and return, otherwise we've got a duplicate - // entry and we throw - if (FullRepositoryPath == null) - { - FullRepositoryPath = absoluteRepositoryLocation; - return this; - } - - throw new Exception($"Registration already exists for {InternalPath}"); - } - - var nextSegment = directorySegmentsFromName.Peek(); - - // Attempt to get the next level of the directory. If we don't have a key entry, create one - if (!Children.TryGetValue(nextSegment, out var nextChild)) - { - nextChild = new InternalDirectory() - { - Name = nextSegment, - InternalPath = Path.Combine(InternalPath, nextSegment) - }; - Children.Add(nextSegment, nextChild); - } - - // add the next - return nextChild.Add(absoluteRepositoryLocation, directorySegmentsFromName); - } + throw new Exception($"Git repo already registered with the name {registrationName}"); } } \ No newline at end of file diff --git a/tests/ModuleTests/Git/AddRegistrationTests.cs b/tests/ModuleTests/Git/AddRegistrationTests.cs index e04cfc3..511833a 100644 --- a/tests/ModuleTests/Git/AddRegistrationTests.cs +++ b/tests/ModuleTests/Git/AddRegistrationTests.cs @@ -90,7 +90,28 @@ public class AddRegistrationTests var secondRegistration = gitManager.RegisterRepo(testRepoAbsolutePath, Path.Combine(paths[..1])); Assert.Equal(Path.Combine(paths), firstRegistration); + Assert.Equal(Path.Combine(paths[..1]), secondRegistration); - Assert.Throws(() => gitManager.RegisterRepo(testRepoAbsolutePath, names.AltSeparator)); + Assert.Throws(() => gitManager.RegisterRepo(testRepoAbsolutePath, Path.Combine(paths[..1]))); + } + + [Fact] + public void DuplicateRepoRegistrationDifferentSlashShouldNotFail() + { + Settings.UseFileName(nameof(RepoRegistrationWithWhitespaceName)); + + var gitManager = GitManager.InternalFreshInstance; + var testRepoAbsolutePath = "Test:/some/test/repo"; + string[] paths = ["test", "nested", "path"]; + var names = (NormalSeparator: string.Join(Path.DirectorySeparatorChar, paths), AltSeparator: string.Join(Path.AltDirectorySeparatorChar, paths)); + + var firstRegistration = gitManager.RegisterRepo(testRepoAbsolutePath, names.NormalSeparator); + // TODO: make nested registrations fail in both directions and test + var secondRegistration = gitManager.RegisterRepo(testRepoAbsolutePath, Path.Combine(paths[..1])); + var differentPathSeparatorRegistration = gitManager.RegisterRepo(testRepoAbsolutePath, names.AltSeparator); + + Assert.Equal(Path.Combine(paths), firstRegistration); + Assert.Equal(Path.Combine(paths[..1]), secondRegistration); + Assert.Equal(names.AltSeparator, differentPathSeparatorRegistration); } } \ No newline at end of file diff --git a/tests/ModuleTests/Git/Snapshots/AddRegistrationTests/BasicRepoRegistration_2.verified.txt b/tests/ModuleTests/Git/Snapshots/AddRegistrationTests/BasicRepoRegistration_2.verified.txt index 3c0b092..7f2c4e6 100644 --- a/tests/ModuleTests/Git/Snapshots/AddRegistrationTests/BasicRepoRegistration_2.verified.txt +++ b/tests/ModuleTests/Git/Snapshots/AddRegistrationTests/BasicRepoRegistration_2.verified.txt @@ -1,2 +1,2 @@ Attempted to register: other/path -Registration result: other\path +Registration result: other/path