diff --git a/CLAUDE.md b/CLAUDE.md index 862146d..8e9c7d4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -36,33 +36,46 @@ Octokit, and Azure DevOps, over a raw `HttpClient`). The solution uses: - `GitIntegration/IGitClient.cs`, `GitIntegration/GitClient.cs` — entry point to the local layer: `GetVersionAsync`, `IsRepositoryAsync`, `OpenAsync`, `DiscoverAsync`, plus the repository-creating `Init(AbsoluteDirectoryPath)` and the two `Clone(...)` overloads. -- `GitIntegration/GitRepository.cs` — carries `LocalPath` plus optional hosting metadata, and - exposes one builder factory per read-only verb (`Status()`, `Log()`, `Diff()`, `Branches()`, - `Remotes()`, `RevParse(...)`) and per mutating verb (`Add()`, `Commit(...)`, `CreateBranch(...)`, - `DeleteBranch(...)`, `Checkout(...)`, `AddRemote(...)`, `RemoveRemote(...)`, - `SetRemoteUrl(...)`, `Fetch()`, `Pull()`, `Push()`), plus `IsClonedAsync` and `OpenWebClient`. +- `GitIntegration/GitRepository.cs` — carries an optional `LocalPath` plus optional hosting + metadata, and exposes one builder factory per read-only verb (`Status()`, `Log()`, `Diff()`, + `Branches()`, `Remotes()`, `Tags()`, `Submodules()`, `RevParse(...)`, `RevList(...)`, + `Divergence(...)`) and per mutating verb (`Add()`, `Commit(...)`, `CreateBranch(...)`, + `DeleteBranch(...)`, `CreateTag(...)`, `DeleteTag(...)`, `Checkout(...)`, `AddRemote(...)`, + `RemoveRemote(...)`, `SetRemoteUrl(...)`, `Fetch()`, `Pull()`, `Push()`, `UpdateSubmodules()`), + plus `IsClonedAsync` and `OpenWebClient`. Both `LocalPath` and `ProcessRunner` are nullable and + separately guarded — `RequireLocalPath()` alongside `RequireRunner()` — because a repository a + hosting provider enumerated carries neither, and the two properties are public `init` accessors so + a caller can supply one without the other. - `GitIntegration/Builders/` — public verb-builder interfaces and their internal implementations: `GitInitBuilder`, `GitCloneBuilder`, `GitAddBuilder`, `GitCommitBuilder`, `GitBranchCreateBuilder`, `GitBranchDeleteBuilder`, `GitCheckoutBuilder`, `GitRemoteAddBuilder`, - `GitRemoteRemoveBuilder`, `GitRemoteSetUrlBuilder`, `GitFetchBuilder`, `GitPullBuilder`, and - `GitPushBuilder`. `IGitVersionBuilder` and every concrete `Git*Builder` class are `internal` — - not part of the public API surface. Only the `IGit*Builder` interfaces are public. + `GitRemoteRemoveBuilder`, `GitRemoteSetUrlBuilder`, `GitFetchBuilder`, `GitPullBuilder`, + `GitPushBuilder`, the tag builders (`GitTagListBuilder`, `GitTagCreateBuilder`, + `GitTagDeleteBuilder`), the submodule builders (`GitSubmoduleListBuilder`, + `GitSubmoduleUpdateBuilder`), and the two `rev-list` builders (`GitRevListBuilder`, + `GitRevListDivergenceBuilder`). `IGitVersionBuilder` and every concrete `Git*Builder` class are + `internal` — not part of the public API surface. Only the `IGit*Builder` interfaces are public. - `GitIntegration/Models/` — result records and enums: `GitStatus`, `GitStatusEntry`, `GitCommit`, - `GitSignature`, `GitBranch`, `GitRemote`, `GitDiffEntry`, `GitVersion`, `GitFileState`, - `GitChangeKind`, `GitUntrackedFilesMode`, `GitInitResult`, `GitCompleted` — the shared "unit" - result for mutating verbs whose only outcome is success (C# has no generic `void`) — and the - remote-sync models `GitRefUpdate`, `GitRefUpdateKind`, `GitFetchResult` (`Updates`, - `DetailAvailable`, `IsUpToDate`), and `GitPushResult` (`Updates`, `HasRejections`). + `GitSignature`, `GitBranch`, `GitTag`, `GitSubmodule`, `GitRemote`, `GitDiffEntry`, + `GitDivergence`, `GitVersion`, `GitFileState`, `GitChangeKind`, `GitUntrackedFilesMode`, + `GitSubmoduleState`, `GitSubmoduleRecursion`, `GitSubmodulePushCheck`, `GitInitResult`, + `GitCompleted` — the shared "unit" result for mutating verbs whose only outcome is success (C# has + no generic `void`) — and the remote-sync models `GitRefUpdate`, `GitRefUpdateKind`, + `GitFetchResult` (`Updates`, `DetailAvailable`, `IsUpToDate`), and `GitPushResult` (`Updates`, + `HasRejections`). - `GitIntegration/Execution/` — `GitOptions`, `IGitProcessRunner`, `RunCommandGitProcessRunner`, `GitResult`, and the exception hierarchy (`GitException` → `GitExecutableNotFoundException`, `GitTimeoutException`, `GitParseException`, `GitCommandException` → `GitRepositoryNotFoundException`, `GitNothingToCommitException`, `GitPushRejectedException`, `GitPullConflictException`). - `GitIntegration/Parsing/` — internal parsers turning raw git output into the `Models/` records, including `GitFetchParser` and `GitPushParser` for the two porcelain formats `fetch --porcelain` - and `push --porcelain` emit. + and `push --porcelain` emit, `GitTagParser`, and `GitSubmoduleParser`. - `GitIntegration/SemanticTypes/` — the `ktsu.Semantics` wrapper types for git identifiers, including - the pull-request types (`GitPullRequestNumber`, `GitPullRequestTitle`, `GitPullRequestAuthor`, - `GitPullRequestWebURI`) added in Phase 5b. + `GitTagName`, `GitHostRepositoryId`, and the pull-request types (`GitPullRequestNumber`, + `GitPullRequestTitle`, `GitPullRequestAuthor`, `GitPullRequestWebURI`) added in Phase 5b. Two + validation attributes live here: `NotAnOptionAttribute`, which stops a value being reinterpreted by + git as a flag, and `IsSinglePathSegmentAttribute`, which stops a value being reinterpreted by a URL + or a filesystem as more than one segment. - `GitIntegration/GitProvider.cs` — the abstract hosting base: credential resolution (`TryGetCredential`, `ResolveCredential`), the internal `HttpMessageHandler? Handler` transport seam, and `CreatePullRequest(GitRepositoryName)`. @@ -177,7 +190,7 @@ from `GitCommandBuilder`, which owns argument assembly, execution, and A builder is single-use and not thread-safe; the underlying `IGitProcessRunner` is a shared, thread-safe singleton. -Two non-obvious, load-bearing design points: +Non-obvious, load-bearing design points: 1. **Every command is scoped with `git -C `, never a process working directory.** `ktsu.RunCommand` has no notion of a working directory, and scoping this way means a failing @@ -214,7 +227,8 @@ Two non-obvious, load-bearing design points: should surface rather than forcing a caller to catch an exception for an outcome git already described in full. -7. **`Fetch` degrades below git 2.41 rather than parsing human output.** `fetch --porcelain` exists +7. **`Fetch` degrades below git 2.41 rather than parsing human output, and degrades again when + recursing into submodules.** `fetch --porcelain` exists only from that version onward, so the builder probes the installed git's version before building the command. Below the threshold the fetch still runs and still succeeds, but `GitFetchResult.DetailAvailable` is false and `Updates` is empty — so an empty list is never @@ -226,6 +240,16 @@ Two non-obvious, load-bearing design points: not a guess — `DetailAvailable` already exists to say exactly that. A genuinely broken git still fails loudly moments later, at the fetch itself, in whichever entry point's own idiom. + The version threshold is **not the only** reason detail can be unavailable, and code reading + `DetailAvailable` must not assume it is. `RecursingSubmodules(...)` is the second: git refuses + `--porcelain` and `--recurse-submodules` together outright — `fatal: options '--porcelain' and + '--recurse-submodules' cannot be used together`, verified against git 2.43 — so emitting both + would turn every recursing fetch into a failure. `--porcelain` is dropped instead and the caller's + request wins, since they asked to fetch submodules rather than to be itemised. This degrades + rather than throwing, unlike the `AllRemotes`/`FromRemote` guard, because the caller asked for + nothing contradictory: the conflict is between their request and an internal implementation + detail. `GitFetchBuilder.IsPorcelainAvailable` is the single place both causes are combined. + 8. **`Pull` returns `GitCompleted`, not a parsed result.** Everything `git pull` prints is human prose with no porcelain form, and this design forbids parsing that prose for every other verb — so `pull` does not get a special exemption either. A caller who needs to know what changed uses @@ -235,6 +259,75 @@ Two non-obvious, load-bearing design points: `commit` sets with "nothing to commit" on stderr-vs-stdout, and `LC_ALL=C` is what makes matching the literal word dependable. +9. **`checkout` is the one verb that does not use `--end-of-options`, and that is a git bug rather + than a choice here.** git ≤ 2.43 strips the marker only when `PARSE_OPT_KEEP_DASHDASH` is unset + (`parse-options.c`), and `checkout` sets exactly that flag because it accepts both a revision and + a pathspec — so the marker survived into checkout's own operand list and was read as a path: + `error: pathspec '--end-of-options' did not match any file(s) known to git`. git 2.44 changed the + condition to `PARSE_OPT_KEEP_UNKNOWN_OPT`, which checkout does not set, so it works from there on. + CI never caught this because the runners ship newer git than Ubuntu 24.04 LTS's stock 2.43. + `GitCheckoutBuilder` emits a trailing `--` instead, git's own documented disambiguator for this + verb, which works on every version and additionally guarantees the operand is read as a revision + rather than a path — so a branch or tag whose name also matches a file resolves to the ref. + `GitRefName`'s `NotAnOptionAttribute` remains the layer that keeps a dash-leading target out of + the vector. Every other verb still uses `AppendOperands`, and should. + +10. **One diagnostic seam, so a verb's two entry points cannot describe the same failure + differently.** `GitCommandBuilder.GetDiagnostic` is virtual and is read by both + `ExecuteAsync` (through `CreateException`) and `TryExecuteAsync`. It exists because standard + error is not where git puts a diagnostic for every verb: `commit` announces "nothing to commit" + on standard *output*, and `pull` announces a conflict there too, leaving standard error carrying + only fetch progress or nothing at all. Both verbs previously overrode `TryExecuteAsync` alone, so + the same non-conflict failure reached one entry point in full and the other as + `git exited with code N: ` with nothing after the colon. A verb that relocates its diagnostic now + states that once, in one override, and both paths follow. + +11. **`Submodules()` runs git twice, and matches the second command's output against the first's.** + Neither command answers the whole question. `ls-files --stage -z` is plumbing: NUL-terminated, so + a path may contain anything, with a mode field (`160000`) that says outright which entries are + gitlinks — but it reports only what the superproject *records*. `submodule status` reports what + is actually checked out, but it is a shell wrapper with no `-z` form, and its path is **not** the + last field, because an optional ` (describe)` may follow it. A path containing ` (` therefore + cannot be split out of a status line at all. + + The parser never tries. The exact set of paths is already known from `ls-files` before a status + line is read, so each line's remainder is matched against paths already in hand and the describe + falls out as whatever is left over. Two consequences worth keeping: the status output must **not** + be trimmed, because the marker for a synchronised submodule is a leading space (which is why that + invocation has its own builder rather than reusing `GitTextBuilder`, whose contract is trimmed + output); and the separator scan starts at index 1 for the same reason. + + `GitSubmodule` carries both object ids, since the two commands genuinely disagree when a + submodule has moved: `Sha` is the recorded gitlink, `CheckedOutSha` is what the working directory + holds. `CheckedOutSha` is null when uninitialised, because git prints the recorded gitlink again + on that line and reporting it verbatim would make an uninitialised submodule indistinguishable + from a synchronised one. Listing does not recurse: `ls-files` enumerates only the superproject's + index, so nested paths could only come from the wrapper. A caller recurses by composition, opening + each submodule as its own `GitRepository`. + +12. **`Log()`'s `ExcludingRemoteTrackingRefs()` emits a *closed* `--not --remotes --not`.** Three + behaviours of git make the argument order the whole correctness question here, all verified + against git 2.43. `--not` negates everything that follows it, so `--all` must precede it or the + query means the opposite — and reports an empty log rather than failing, so nothing else would + catch it. `--not` stays in effect until the next `--not`, so without the closing one a revision + from `ForRevision` or a pathspec from `ForPath` would fall inside the negation and be excluded + rather than selected, which quietly answers zero and looks exactly like a correct "nothing + unpushed". And `--not` must precede every non-option argument, so the negation cannot instead be + deferred past the operands: `git log --end-of-options HEAD --not --remotes` dies with + `fatal: option '--not' must come before non-option arguments`. + + `--all` rather than `--branches` is load-bearing too: `--branches` covers only `refs/heads` and + misses a detached HEAD, which is the normal state of a submodule working directory. + +13. **`Diff().WithLineCounts()` switches format rather than adding a flag.** `--name-status` and + `--numstat` are both display formats and git lets the last one win, so asking for both silently + produces only one section. `--raw` is the form that combines, and it carries everything + `--name-status` does. Under `-z` the two sections run together with **no delimiter**: a raw + record begins with `:` and a numstat record with a digit or `-`, and that test is safe only + because it is applied at record boundaries, where a path can never appear. Correlation is + positional, because a rename is spelled differently in each section. A binary file's counts are + `-`, which is why they are nullable — a binary change is not a zero-line change. + **Hosting layer.** `GitProvider` is an abstract base with two implementations: `GitHubProvider` over Octokit, and `AzureDevOpsProvider` over a raw `HttpClient` — Azure DevOps has no client library this library uses (see the dependency note below). Both go through the same shape: every request-issuing @@ -260,6 +353,13 @@ instances so their settings cannot drift, and sets `PooledConnectionLifetime` (t `IHttpClientFactory`) so a process-lifetime handler does not go on using a host's original address after DNS moves it. Neither shared handler is ever disposed, and neither provider is `IDisposable`. +Each provider declares its own handler through `GitProvider`'s `private protected abstract +DefaultHandler`, rather than inheriting one static field from the base. That field used to exist, and +was equivalent to per-provider only by accident: `AzureDevOpsProvider` was its sole user because +`GitHubProvider` already had its own, so adding a third provider would have silently enrolled it in +Azure DevOps's connection pool with nothing in the code to notice. The abstract member makes the +compiler ask. + The two providers express "this transport is not mine to dispose" differently, and that asymmetry is forced, not accidental. `GitProvider.CreateHttpClient` passes `disposeHandler: false` to `HttpClient`'s own constructor. `GitHubProvider` cannot: Octokit's `HttpClientAdapter` disposes @@ -314,6 +414,33 @@ keyed lookup would work only because `HttpResponseMessage` happens to canonicali casing. A `Retry-After` carrying an HTTP date rather than seconds yields `null`, since an unset `ResetsAt` is better than an invented one. +**A repository from a hosting provider carries no `LocalPath`, and is addressed by the host's own +id.** `GitRepository.LocalPath` is nullable because a repository a provider enumerated has never been +cloned. Both providers used to invent one under `Environment.CurrentDirectory`, which made the same +remote repository yield a different record depending on when it was enumerated, and which forced a +containment guard to exist purely to keep a name from a remote response from escaping that +directory. That guard is gone with the reason for it, and `IGitClient.Clone(GitRepository)` now +reports a missing `LocalPath` rather than resurrecting the invented default — where a working copy +goes is the caller's decision. + +`GitRepository.HostRepositoryId` carries what a host documents its own API in terms of. Microsoft's +reference types Azure DevOps's `{repositoryId}` path parameter as `string (uuid)` and draws an +explicit id-or-name distinction for the sibling `project` parameter while withholding it here, so +substituting a name there is unconfirmed against the documented schema rather than sanctioned by it. +`GetPullRequestsAsync(GitRepository)` and `CreatePullRequest(GitRepository)` therefore prefer the id; +the `GitRepositoryName`-taking overloads still pass a name, since that is all they are given. Both +route through one internal core taking the finished path segment, so the choice lives in exactly one +place and the overloads cannot answer the same question differently. + +**A field a host reported goes through `GitProvider.ToHostValue`, never through `As()` directly.** +The hosting counterpart to `GitParseValues.ToSemantic`, and it exists for the same reason: a value +arriving from outside is data to be validated, not an argument a caller got wrong. `GitRepositoryName` +is validated as a single path segment (`IsSinglePathSegmentAttribute`), so a host reporting `/` or +`..` produces a value the semantic type refuses — and calling `As()` on a response directly would +throw `ArgumentException` out of a public hosting method, whose documented failure surface is the +`GitHostingException` hierarchy. An omitted optional field stays null; only a field the host did +report and this library cannot represent is raised. + **Azure DevOps pull request operations require `Project`; repository enumeration does not.** Azure DevOps nests repositories under a project — GitHub has no equivalent — so `AzureDevOpsProvider.Project` is optional: unset, `GetRepositoriesAsync` enumerates the whole @@ -375,8 +502,15 @@ the package, not by working around it. **Deliberately out of scope for the hosting layer:** merging or completing a pull request, comments, reviews, repository creation, and webhooks — none of these are implemented, and none should be documented as present. Deliberately out of scope for the local layer, even later: `commit --amend`, -`add --force`, `switch` (see `IGitCheckoutBuilder`'s remarks for why `checkout` was chosen instead), -and submodule support. +`add --force`, and `switch` (see `IGitCheckoutBuilder`'s remarks for why `checkout` was chosen +instead). + +Submodule support **is** in scope and is implemented — `Submodules()`, `UpdateSubmodules()`, and +`--recurse-submodules` on the verbs that accept it. It used to be on the list above, which +contradicted the v2 design doc's own "deferred to a later version"; that discrepancy is resolved, +and the design doc carries a dated amendment saying so. `git submodule add` is not wrapped: the +verbs exist to inspect and update submodules, not to create them. Still out of scope from the design +doc's non-goals, and genuinely so: `merge`, `rebase`, `bisect`, `stash`, `worktree`. ## Testing diff --git a/GitIntegration.Test/Builders/GitCheckoutBuilderTests.cs b/GitIntegration.Test/Builders/GitCheckoutBuilderTests.cs index 29012d6..c2328d1 100644 --- a/GitIntegration.Test/Builders/GitCheckoutBuilderTests.cs +++ b/GitIntegration.Test/Builders/GitCheckoutBuilderTests.cs @@ -25,8 +25,8 @@ public void BuildsTheDefaultCheckoutVector() "-c", "core.quotepath=false", "-c", "color.ui=false", "checkout", - "--end-of-options", "main", + "--", ]; CollectionAssert.AreEqual(expectedArguments, builder.BuildArguments().ToArray()); } @@ -50,30 +50,80 @@ public void MapsTheOptionFlags() } [TestMethod] - public void KeepsFlagsBeforeTheEndOfOptionsMarker() + public void KeepsFlagsBeforeTheTargetAndTerminatesWithADoubleDash() { - // Anything after --end-of-options is an operand, so a flag emitted there would be handed to - // git as a ref name. + // The target must be the last thing before the "--" terminator, with every flag ahead of it: + // a flag emitted after the target would be handed to git as a pathspec. RecordingGitProcessRunner runner = new(); GitCheckoutBuilder builder = new(runner, TestPaths.Root, Main); _ = builder.CreatingBranch().Force(); string[] arguments = [.. builder.BuildArguments()]; - int marker = Array.IndexOf(arguments, "--end-of-options"); + int target = Array.IndexOf(arguments, "main"); - Assert.IsTrue(Array.IndexOf(arguments, "-b") < marker); - Assert.IsTrue(Array.IndexOf(arguments, "--force") < marker); - Assert.AreEqual("main", arguments[marker + 1]); + Assert.IsTrue(Array.IndexOf(arguments, "-b") < target); + Assert.IsTrue(Array.IndexOf(arguments, "--force") < target); + Assert.AreEqual("--", arguments[target + 1]); + Assert.AreEqual(arguments.Length - 1, target + 1); + } + + [TestMethod] + public void DoesNotEmitTheEndOfOptionsMarker() + { + // git <= 2.43 leaves --end-of-options in checkout's own operand list, because checkout sets + // PARSE_OPT_KEEP_DASHDASH and that release only stripped the marker when the flag was unset. + // The marker then reaches git as a pathspec: "error: pathspec '--end-of-options' did not + // match any file(s) known to git". git 2.44 changed the condition, but emitting the marker + // would make Checkout unusable on every git before it — including Ubuntu 24.04 LTS's stock + // 2.43. GitRefName's NotAnOptionAttribute is what keeps a dash-leading target out of the + // vector instead. + RecordingGitProcessRunner runner = new(); + GitCheckoutBuilder builder = new(runner, TestPaths.Root, Main); + + CollectionAssert.DoesNotContain(builder.BuildArguments().ToArray(), "--end-of-options"); } [TestMethod] public void ConfigurationMethodsReturnTheSameBuilderForChaining() { + // Deliberately not chaining CreatingBranch and Detach together: that combination is one git + // refuses, and BuildArguments rejects it. Chaining it here to check a fluent return value + // would read as an endorsement of a vector that can never run. RecordingGitProcessRunner runner = new(); GitCheckoutBuilder builder = new(runner, TestPaths.Root, Main); - Assert.AreSame(builder, builder.CreatingBranch().Force().Detach()); + Assert.AreSame(builder, builder.CreatingBranch().Force()); + Assert.AreSame(builder, builder.Detach()); + } + + [TestMethod] + public void RejectsAskingForBothCreatingBranchAndDetach() + { + // Real git refuses "-b --detach " with + // "fatal: '--detach' cannot be used with '-b/-B/--orphan'" (verified against git 2.43), so + // without this guard the contradiction surfaces only as an opaque GitCommandException from a + // process that was already spawned. Fetch and pull reject their own equivalent + // contradictions before spawning, and checkout should be no less consistent. + RecordingGitProcessRunner runner = new(); + GitCheckoutBuilder builder = new(runner, TestPaths.Root, Main); + + _ = builder.CreatingBranch().Detach(); + + Assert.ThrowsExactly(() => _ = builder.BuildArguments()); + } + + [TestMethod] + public void RejectsTheContradictionRegardlessOfTheOrderItWasConfiguredIn() + { + // A caller may set either first, so only the finished configuration can detect the + // contradiction — the reason the guard lives in BuildArguments rather than in each setter. + RecordingGitProcessRunner runner = new(); + GitCheckoutBuilder builder = new(runner, TestPaths.Root, Main); + + _ = builder.Detach().CreatingBranch(); + + Assert.ThrowsExactly(() => _ = builder.BuildArguments()); } [TestMethod] diff --git a/GitIntegration.Test/Builders/GitDiffBuilderTests.cs b/GitIntegration.Test/Builders/GitDiffBuilderTests.cs index e7a7728..cd1ec6d 100644 --- a/GitIntegration.Test/Builders/GitDiffBuilderTests.cs +++ b/GitIntegration.Test/Builders/GitDiffBuilderTests.cs @@ -4,6 +4,7 @@ namespace ktsu.GitIntegration.Test; using System; using System.Collections.Generic; +using System.Threading.Tasks; using ktsu.Semantics.Paths; using ktsu.Semantics.Strings; @@ -120,4 +121,81 @@ public void RejectsNullArguments() Assert.ThrowsExactly(() => _ = builder.Between("main".As(), null!)); Assert.ThrowsExactly(() => _ = builder.ForPath(null!)); } + + [TestMethod] + public void SwitchesToRawAndNumstatWhenLineCountsAreAskedFor() + { + // --name-status and --numstat are both display formats and git lets the last one win, so + // asking for both would silently produce only one section. --raw is the form that combines. + RecordingGitProcessRunner runner = new(); + GitDiffBuilder builder = new(runner, TestPaths.Root); + + _ = builder.WithLineCounts(); + + string[] arguments = [.. builder.BuildArguments()]; + + CollectionAssert.Contains(arguments, "--raw"); + CollectionAssert.Contains(arguments, "--numstat"); + CollectionAssert.DoesNotContain(arguments, "--name-status"); + CollectionAssert.Contains(arguments, "-z"); + } + + [TestMethod] + public void LeavesTheDefaultVectorUnchangedWhenLineCountsAreNotAskedFor() + { + // The option is opt-in precisely so the command and its output volume stay as they were for + // callers that only want the path list. + RecordingGitProcessRunner runner = new(); + GitDiffBuilder builder = new(runner, TestPaths.Root); + + string[] arguments = [.. builder.BuildArguments()]; + + CollectionAssert.Contains(arguments, "--name-status"); + CollectionAssert.DoesNotContain(arguments, "--raw"); + CollectionAssert.DoesNotContain(arguments, "--numstat"); + } + + [TestMethod] + public async Task ReportsNoLineCountsUnlessTheyWereAskedForAsync() + { + // The default parser reads --name-status output, which carries no counts at all, so both + // fields stay null rather than defaulting to zero. + RecordingGitProcessRunner runner = new() { StandardOutput = "M\0a.txt\0" }; + GitDiffBuilder builder = new(runner, TestPaths.Root); + + IReadOnlyList entries = + await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.IsNull(entries[0].Insertions); + Assert.IsNull(entries[0].Deletions); + } + + [TestMethod] + public async Task ReportsLineCountsWhenTheyWereAskedForAsync() + { + RecordingGitProcessRunner runner = new() + { + StandardOutput = ":100644 100644 366fd40 fbeb5f4 M\0a.txt\0" + "3\t2\ta.txt\0", + }; + GitDiffBuilder builder = new(runner, TestPaths.Root); + + _ = builder.WithLineCounts(); + + IReadOnlyList entries = + await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.AreEqual(3, entries[0].Insertions); + Assert.AreEqual(2, entries[0].Deletions); + } + + [TestMethod] + public void WithLineCountsReturnsTheSameBuilderForChaining() + { + RecordingGitProcessRunner runner = new(); + GitDiffBuilder builder = new(runner, TestPaths.Root); + + Assert.AreSame(builder, builder.WithLineCounts()); + } + + public TestContext TestContext { get; set; } = null!; } diff --git a/GitIntegration.Test/Builders/GitLogBuilderTests.cs b/GitIntegration.Test/Builders/GitLogBuilderTests.cs index ab8dc74..7ca2661 100644 --- a/GitIntegration.Test/Builders/GitLogBuilderTests.cs +++ b/GitIntegration.Test/Builders/GitLogBuilderTests.cs @@ -133,4 +133,93 @@ public void RejectsANullRevisionOrPath() Assert.ThrowsExactly(() => _ = builder.ForRevision(null!)); Assert.ThrowsExactly(() => _ = builder.ForPath(null!)); } + + [TestMethod] + public void EmitsAllRefsBeforeTheNegation() + { + // Order is the whole correctness question here. git negates everything after a --not, so + // "--not --remotes --all" excludes every reference instead of including them — and reports an + // empty log rather than failing, so nothing else would catch the mistake. Verified against + // git 2.43. + RecordingGitProcessRunner runner = new(); + GitLogBuilder builder = new(runner, TestPaths.Root); + + _ = builder.IncludingAllRefs().ExcludingRemoteTrackingRefs(); + + string[] arguments = [.. builder.BuildArguments()]; + int all = Array.IndexOf(arguments, "--all"); + int not = Array.IndexOf(arguments, "--not"); + + Assert.AreNotEqual(-1, all); + Assert.AreNotEqual(-1, not); + Assert.IsTrue(all < not, "--all must precede --not or the query means the opposite."); + } + + [TestMethod] + public void EmitsTheNegationAsAClosedTriple() + { + // --not reverses every revision specifier that follows it until the next --not, so the pair is + // emitted with a closing --not that scopes the negation to --remotes alone. Without it a + // revision or pathspec emitted below would be excluded rather than selected. + RecordingGitProcessRunner runner = new(); + GitLogBuilder builder = new(runner, TestPaths.Root); + + _ = builder.ExcludingRemoteTrackingRefs(); + + string[] arguments = [.. builder.BuildArguments()]; + int not = Array.IndexOf(arguments, "--not"); + + Assert.AreEqual("--remotes", arguments[not + 1]); + Assert.AreEqual("--not", arguments[not + 2]); + } + + [TestMethod] + public void KeepsARevisionOutsideTheNegation() + { + // The failure the closing --not prevents: "git log --not --remotes " asks for + // commits in neither, which is a different question that quietly returns nothing. The revision + // has to land after the negation has been closed. + RecordingGitProcessRunner runner = new(); + GitLogBuilder builder = new(runner, TestPaths.Root); + + _ = builder.ExcludingRemoteTrackingRefs().ForRevision("HEAD".As()); + + string[] arguments = [.. builder.BuildArguments()]; + int revision = Array.IndexOf(arguments, "HEAD"); + int closingNot = Array.LastIndexOf(arguments, "--not"); + + Assert.IsTrue(closingNot < revision, "The negation must be closed before the revision."); + } + + [TestMethod] + public void KeepsTheNegationBeforeEveryNonOptionArgument() + { + // git refuses --not once a non-option argument has appeared: "git log --end-of-options HEAD + // --not --remotes" dies with "fatal: option '--not' must come before non-option arguments". + // That is why the negation cannot simply be deferred to the end of the vector instead. + RecordingGitProcessRunner runner = new(); + GitLogBuilder builder = new(runner, TestPaths.Root); + + _ = builder.IncludingAllRefs().ExcludingRemoteTrackingRefs().ForRevision("HEAD".As()); + + string[] arguments = [.. builder.BuildArguments()]; + int marker = Array.IndexOf(arguments, "--end-of-options"); + + Assert.IsTrue(Array.LastIndexOf(arguments, "--not") < marker); + Assert.IsTrue(Array.IndexOf(arguments, "--remotes") < marker); + Assert.IsTrue(Array.IndexOf(arguments, "--all") < marker); + } + + [TestMethod] + public void OmitsBothFlagsWhenNeitherWasRequested() + { + RecordingGitProcessRunner runner = new(); + GitLogBuilder builder = new(runner, TestPaths.Root); + + string[] arguments = [.. builder.BuildArguments()]; + + CollectionAssert.DoesNotContain(arguments, "--all"); + CollectionAssert.DoesNotContain(arguments, "--not"); + CollectionAssert.DoesNotContain(arguments, "--remotes"); + } } diff --git a/GitIntegration.Test/Builders/GitPullBuilderTests.cs b/GitIntegration.Test/Builders/GitPullBuilderTests.cs index a8f3221..5c0b801 100644 --- a/GitIntegration.Test/Builders/GitPullBuilderTests.cs +++ b/GitIntegration.Test/Builders/GitPullBuilderTests.cs @@ -291,6 +291,65 @@ public async Task TryExecuteReportsStandardErrorAloneWithNoTrailingNewlineAsync( result.Error?.StandardError); } + [TestMethod] + public async Task ExecuteReportsANonConflictFailureExplainedOnStandardOutputAsync() + { + // The two entry points must describe the identical failure identically. This one is not a + // conflict, so CreateException hands it to the base implementation — which used to build its + // message from standard error alone and produced "git exited with code 1: " with nothing + // after the colon, while TryExecuteAsync returned the real explanation. + const string Explanation = "You have divergent branches and need to specify how to reconcile them.\n"; + RecordingGitProcessRunner runner = new() + { + ExitCode = 128, + StandardOutput = Explanation, + StandardError = string.Empty, + }; + GitPullBuilder builder = new(runner, TestPaths.Root); + + GitCommandException exception = await Assert.ThrowsExactlyAsync( + async () => await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) + .ConfigureAwait(false); + + StringAssert.Contains(exception.Message, "divergent branches"); + StringAssert.Contains(exception.StandardError, "divergent branches"); + } + + [TestMethod] + public async Task ExecuteAndTryExecuteReportTheSameDiagnosticForTheSameFailureAsync() + { + // The point of routing both entry points through one seam: they cannot drift. Asserting the + // two texts against each other pins that directly, rather than pinning each against a + // literal that a future change could update in one place only. + const string FetchProgress = "From /srv/origin\n * branch main -> FETCH_HEAD\n"; + const string Explanation = "error: Your local changes would be overwritten by merge.\n"; + + RecordingGitProcessRunner throwingRunner = new() + { + ExitCode = 1, + StandardOutput = Explanation, + StandardError = FetchProgress, + }; + GitPullBuilder throwing = new(throwingRunner, TestPaths.Root); + + GitCommandException exception = await Assert.ThrowsExactlyAsync( + async () => await throwing.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) + .ConfigureAwait(false); + + RecordingGitProcessRunner resultRunner = new() + { + ExitCode = 1, + StandardOutput = Explanation, + StandardError = FetchProgress, + }; + GitPullBuilder returning = new(resultRunner, TestPaths.Root); + + GitResult result = + await returning.TryExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.AreEqual(result.Error?.StandardError, exception.StandardError); + } + [TestMethod] public async Task ForwardsProgressToTheRequestAsync() { diff --git a/GitIntegration.Test/Builders/GitRevListBuilderTests.cs b/GitIntegration.Test/Builders/GitRevListBuilderTests.cs new file mode 100644 index 0000000..e51d511 --- /dev/null +++ b/GitIntegration.Test/Builders/GitRevListBuilderTests.cs @@ -0,0 +1,213 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitIntegration.Test; + +using System; +using System.Threading.Tasks; + +using ktsu.Semantics.Paths; +using ktsu.Semantics.Strings; + +[TestClass] +public class GitRevListBuilderTests +{ + private static GitRefName Range => "a..b".As(); + + [TestMethod] + public void BuildsTheDefaultCountVector() + { + RecordingGitProcessRunner runner = new(); + GitRevListBuilder builder = new(runner, TestPaths.Root, Range); + + string[] expectedArguments = + [ + "-C", TestPaths.Root.WeakString, + "--no-pager", + "-c", "core.quotepath=false", + "-c", "color.ui=false", + "rev-list", + "--count", + "--end-of-options", + "a..b", + ]; + CollectionAssert.AreEqual(expectedArguments, builder.BuildArguments().ToArray()); + } + + [TestMethod] + public void PutsAPathspecAfterTheRevision() + { + // git reads everything after -- as a filename, so a revision emitted there would count + // nothing rather than failing — the same trap GitLogBuilder documents for its own pathspec. + RecordingGitProcessRunner runner = new(); + GitRevListBuilder builder = new(runner, TestPaths.Root, Range); + + _ = builder.FirstParentOnly().ForPath("src/a.txt".As()); + + string[] arguments = [.. builder.BuildArguments()]; + int separator = Array.IndexOf(arguments, "--"); + + Assert.IsTrue(Array.IndexOf(arguments, "--first-parent") < separator); + Assert.IsTrue(Array.IndexOf(arguments, "a..b") < separator); + + // Compared against the canonicalised form rather than the literal, matching + // GitLogBuilderTests: RelativeFilePath canonicalises to the platform's separator, so the + // literal "src/a.txt" is what reaches the vector on POSIX and "src\a.txt" on Windows. + Assert.AreEqual("src/a.txt".As().WeakString, arguments[separator + 1]); + } + + [TestMethod] + public void EmitsEveryPathThatWasAdded() + { + RecordingGitProcessRunner runner = new(); + GitRevListBuilder builder = new(runner, TestPaths.Root, Range); + + _ = builder.ForPath("a.txt".As()).ForPath("b.txt".As()); + + string[] arguments = [.. builder.BuildArguments()]; + + CollectionAssert.Contains(arguments, "a.txt"); + CollectionAssert.Contains(arguments, "b.txt"); + } + + [TestMethod] + public async Task ParsesTheCountAsync() + { + RecordingGitProcessRunner runner = new() { StandardOutput = "42\n" }; + GitRevListBuilder builder = new(runner, TestPaths.Root, Range); + + int count = await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.AreEqual(42, count); + } + + [TestMethod] + public async Task ParsesAZeroCountAsync() + { + // An empty range is an ordinary answer, not a failure, and zero has to survive as a value + // rather than being confused with "nothing was reported". + RecordingGitProcessRunner runner = new() { StandardOutput = "0\n" }; + GitRevListBuilder builder = new(runner, TestPaths.Root, Range); + + int count = await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.AreEqual(0, count); + } + + [TestMethod] + public async Task ThrowsWhenTheOutputIsNotACountAsync() + { + RecordingGitProcessRunner runner = new() { StandardOutput = "not a number\n" }; + GitRevListBuilder builder = new(runner, TestPaths.Root, Range); + + await Assert.ThrowsExactlyAsync( + async () => await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) + .ConfigureAwait(false); + } + + [TestMethod] + public void RejectsNullArguments() + { + RecordingGitProcessRunner runner = new(); + GitRevListBuilder builder = new(runner, TestPaths.Root, Range); + + Assert.ThrowsExactly(() => _ = new GitRevListBuilder(runner, TestPaths.Root, null!)); + Assert.ThrowsExactly(() => _ = builder.ForPath(null!)); + } + + [TestMethod] + public void ConfigurationMethodsReturnTheSameBuilderForChaining() + { + RecordingGitProcessRunner runner = new(); + GitRevListBuilder builder = new(runner, TestPaths.Root, Range); + + Assert.AreSame(builder, builder.FirstParentOnly().ForPath("a.txt".As())); + } + + public TestContext TestContext { get; set; } = null!; +} + +[TestClass] +public class GitRevListDivergenceBuilderTests +{ + private static GitRefName Upstream => "origin/main".As(); + + private static GitRefName Local => "HEAD".As(); + + [TestMethod] + public void BuildsTheThreeDotExpression() + { + // Three dots, not two. "a..b" names the commits b has and a does not, which is one number; + // "a...b" names the commits either has and the other does not, which is the pair + // --left-right splits. + RecordingGitProcessRunner runner = new(); + GitRevListDivergenceBuilder builder = new(runner, TestPaths.Root, Upstream, Local); + + string[] expectedArguments = + [ + "-C", TestPaths.Root.WeakString, + "--no-pager", + "-c", "core.quotepath=false", + "-c", "color.ui=false", + "rev-list", + "--count", + "--left-right", + "--end-of-options", + "origin/main...HEAD", + ]; + CollectionAssert.AreEqual(expectedArguments, builder.BuildArguments().ToArray()); + } + + [TestMethod] + public async Task MapsTheLeftCountToBehindAndTheRightToAheadAsync() + { + // The mapping is the part that is easy to get backwards, so it is pinned rather than assumed. + // git prints "\t", and the expression is always built as upstream...local, so + // left counts what only the upstream has (behind) and right what only the local has (ahead). + RecordingGitProcessRunner runner = new() { StandardOutput = "3\t5\n" }; + GitRevListDivergenceBuilder builder = new(runner, TestPaths.Root, Upstream, Local); + + GitDivergence divergence = await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.AreEqual(3, divergence.Behind); + Assert.AreEqual(5, divergence.Ahead); + Assert.IsFalse(divergence.IsInSync); + } + + [TestMethod] + public async Task ReportsInSyncWhenNeitherSideHasAnExclusiveCommitAsync() + { + RecordingGitProcessRunner runner = new() { StandardOutput = "0\t0\n" }; + GitRevListDivergenceBuilder builder = new(runner, TestPaths.Root, Upstream, Local); + + GitDivergence divergence = await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.IsTrue(divergence.IsInSync); + } + + [TestMethod] + public async Task ThrowsWhenTheOutputIsNotAPairOfCountsAsync() + { + // A single number is what a plain --count prints, so reaching this parser with one means the + // --left-right flag did not take effect. Silently reading it as one side of the pair would + // report a confident zero for the other. + RecordingGitProcessRunner runner = new() { StandardOutput = "7\n" }; + GitRevListDivergenceBuilder builder = new(runner, TestPaths.Root, Upstream, Local); + + await Assert.ThrowsExactlyAsync( + async () => await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) + .ConfigureAwait(false); + } + + [TestMethod] + public void RejectsNullArguments() + { + RecordingGitProcessRunner runner = new(); + + Assert.ThrowsExactly( + () => _ = new GitRevListDivergenceBuilder(runner, TestPaths.Root, null!, Local)); + Assert.ThrowsExactly( + () => _ = new GitRevListDivergenceBuilder(runner, TestPaths.Root, Upstream, null!)); + } + + public TestContext TestContext { get; set; } = null!; +} diff --git a/GitIntegration.Test/Builders/GitSubmoduleBuilderTests.cs b/GitIntegration.Test/Builders/GitSubmoduleBuilderTests.cs new file mode 100644 index 0000000..dcc4702 --- /dev/null +++ b/GitIntegration.Test/Builders/GitSubmoduleBuilderTests.cs @@ -0,0 +1,409 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitIntegration.Test; + +using System; +using System.Collections.Generic; +using System.ComponentModel; +using System.Threading.Tasks; + +using ktsu.Semantics.Paths; +using ktsu.Semantics.Strings; + +using Testably.Abstractions.Testing; + +[TestClass] +public class GitSubmoduleListBuilderTests +{ + [TestMethod] + public void BuildsThePlumbingVectorRatherThanTheWrapperOne() + { + // ls-files is plumbing and NUL-terminated, so a submodule path may contain anything at all. + // submodule status has no -z form and no stability guarantee, which is why it is consulted + // only for state, never for the authoritative path list. + RecordingGitProcessRunner runner = new(); + GitSubmoduleListBuilder builder = new(runner, TestPaths.Root); + + string[] expectedArguments = + [ + "-C", TestPaths.Root.WeakString, + "--no-pager", + "-c", "core.quotepath=false", + "-c", "color.ui=false", + "ls-files", + "--stage", + "-z", + ]; + CollectionAssert.AreEqual(expectedArguments, builder.BuildArguments().ToArray()); + } + + [TestMethod] + public async Task SkipsEverythingThatIsNotAGitlinkAsync() + { + // ls-files lists the whole index, so the 160000 mode filter is what turns it into a submodule + // listing. A repository with no submodules also costs only this one invocation. + ScriptedGitProcessRunner runner = new ScriptedGitProcessRunner() + .Then(standardOutput: "100644 abc1234000000000000000000000000000000000 0\tREADME.md\0" + + "100755 def5678000000000000000000000000000000000 0\tbuild.sh\0"); + GitSubmoduleListBuilder builder = new(runner, TestPaths.Root); + + IReadOnlyList submodules = + await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.AreEqual(0, submodules.Count); + } + + [TestMethod] + public async Task ResolvesStateFromTheStatusInvocationAsync() + { + // Captured from git 2.43. The leading space marks a submodule whose checkout matches the + // gitlink the superproject records. + ScriptedGitProcessRunner runner = new ScriptedGitProcessRunner() + .Then(standardOutput: "160000 1f2bee80cfcf06ee5ba820b17fe3b6ddca460915 0\tlibs/sub\0") + .Then(standardOutput: " 1f2bee80cfcf06ee5ba820b17fe3b6ddca460915 libs/sub (heads/master)\n"); + GitSubmoduleListBuilder builder = new(runner, TestPaths.Root); + + IReadOnlyList submodules = + await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.AreEqual(1, submodules.Count); + Assert.AreEqual("libs/sub".As(), submodules[0].Path); + Assert.AreEqual(GitSubmoduleState.InSync, submodules[0].State); + Assert.AreEqual(submodules[0].Sha, submodules[0].CheckedOutSha); + Assert.AreEqual("heads/master", submodules[0].Describe); + } + + [TestMethod] + public async Task KeepsTheRecordedGitlinkApartFromTheCheckedOutCommitAsync() + { + // The "+" marker means a different commit is checked out than the superproject records, and + // the two commands genuinely report different ids in that case: ls-files reports the gitlink, + // submodule status reports the checkout. Collapsing them into one field would lose the fact + // that the superproject is out of date. + ScriptedGitProcessRunner runner = new ScriptedGitProcessRunner() + .Then(standardOutput: "160000 1f2bee80cfcf06ee5ba820b17fe3b6ddca460915 0\tlibs/sub\0") + .Then(standardOutput: "+120669ec6b336c651886335053ac0644d7821e09 libs/sub (heads/master)\n"); + GitSubmoduleListBuilder builder = new(runner, TestPaths.Root); + + IReadOnlyList submodules = + await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.AreEqual(GitSubmoduleState.DifferentCommit, submodules[0].State); + Assert.AreEqual("1f2bee80cfcf06ee5ba820b17fe3b6ddca460915".As(), submodules[0].Sha); + Assert.AreEqual("120669ec6b336c651886335053ac0644d7821e09".As(), submodules[0].CheckedOutSha); + } + + [TestMethod] + public async Task ReportsNoCheckedOutCommitForAnUninitialisedSubmoduleAsync() + { + // git prints the recorded gitlink again for an uninitialised submodule, which would make it + // indistinguishable from a synchronised one if it were reported verbatim. Nothing is checked + // out there, so nothing is reported. Note there is no describe suffix either. + ScriptedGitProcessRunner runner = new ScriptedGitProcessRunner() + .Then(standardOutput: "160000 1f2bee80cfcf06ee5ba820b17fe3b6ddca460915 0\tlibs/sub\0") + .Then(standardOutput: "-1f2bee80cfcf06ee5ba820b17fe3b6ddca460915 libs/sub\n"); + GitSubmoduleListBuilder builder = new(runner, TestPaths.Root); + + IReadOnlyList submodules = + await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.AreEqual(GitSubmoduleState.Uninitialised, submodules[0].State); + Assert.IsNull(submodules[0].CheckedOutSha); + Assert.IsNull(submodules[0].Describe); + } + + [TestMethod] + public async Task ReadsAPathContainingTheDescribeDelimiterAsync() + { + // The reason this parser matches lines against known paths instead of splitting them. A + // submodule at "libs/sub (old)" produces a status line whose path and describe suffix cannot + // be told apart by inspection — but the path is already known exactly from ls-files, so no + // guess is needed. + ScriptedGitProcessRunner runner = new ScriptedGitProcessRunner() + .Then(standardOutput: "160000 1f2bee80cfcf06ee5ba820b17fe3b6ddca460915 0\tlibs/sub (old)\0") + .Then(standardOutput: " 1f2bee80cfcf06ee5ba820b17fe3b6ddca460915 libs/sub (old) (heads/master)\n"); + GitSubmoduleListBuilder builder = new(runner, TestPaths.Root); + + IReadOnlyList submodules = + await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.AreEqual("libs/sub (old)".As(), submodules[0].Path); + Assert.AreEqual(GitSubmoduleState.InSync, submodules[0].State); + Assert.AreEqual("heads/master", submodules[0].Describe); + } + + [TestMethod] + public async Task KeepsAGitlinkWithNoStatusLineAsUnknownAsync() + { + // A submodule removed from .gitmodules while its gitlink remains. Dropping the entry would + // hide it from a caller deciding whether the directory on disk is safe to delete, which is + // exactly the question this verb exists to answer. + ScriptedGitProcessRunner runner = new ScriptedGitProcessRunner() + .Then(standardOutput: "160000 1f2bee80cfcf06ee5ba820b17fe3b6ddca460915 0\tlibs/orphan\0") + .Then(standardOutput: string.Empty); + GitSubmoduleListBuilder builder = new(runner, TestPaths.Root); + + IReadOnlyList submodules = + await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.AreEqual(1, submodules.Count); + Assert.AreEqual(GitSubmoduleState.Unknown, submodules[0].State); + Assert.IsNull(submodules[0].CheckedOutSha); + } + + [TestMethod] + public async Task KeepsTheGitlinksWhenTheStatusWrapperFailsAsync() + { + // The gitlinks are the authoritative half of the answer and are already in hand, so failing + // the whole listing because the wrapper could not run would discard a correct result over a + // missing embellishment. Unknown already means exactly "git did not say". + ScriptedGitProcessRunner runner = new ScriptedGitProcessRunner() + .Then(standardOutput: "160000 1f2bee80cfcf06ee5ba820b17fe3b6ddca460915 0\tlibs/sub\0") + .Then(standardError: "fatal: not a git repository\n", exitCode: 128); + GitSubmoduleListBuilder builder = new(runner, TestPaths.Root); + + IReadOnlyList submodules = + await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.AreEqual(1, submodules.Count); + Assert.AreEqual(GitSubmoduleState.Unknown, submodules[0].State); + } + + [TestMethod] + public void MatchesAStatusLineAgainstGitsOwnSpellingOfThePath() + { + // git prints a path with forward slashes on every platform, but RelativeDirectoryPath + // canonicalises to the *platform's* separator — so on Windows the gitlink is "libs\sub" here + // and "libs/sub" in the status output, and matching the two naively fails. That left every + // submodule Unknown on Windows and nowhere else, which no POSIX run could ever catch. + // + // This assertion is trivially satisfied on a POSIX host, where the two spellings coincide, and + // load-bearing on Windows, where CI runs it — the only place the bug can appear. It is written + // as a nested path so the separator is actually exercised there rather than absent. + IReadOnlyList gitlinks = GitSubmoduleParser.ParseGitlinks( + "160000 1f2bee80cfcf06ee5ba820b17fe3b6ddca460915 0\tlibs/nested/sub\0"); + + IReadOnlyList resolved = GitSubmoduleParser.ApplyStatus( + gitlinks, + " 1f2bee80cfcf06ee5ba820b17fe3b6ddca460915 libs/nested/sub (heads/master)\n"); + + Assert.AreEqual(1, resolved.Count); + Assert.AreEqual(GitSubmoduleState.InSync, resolved[0].State); + Assert.AreEqual("heads/master", resolved[0].Describe); + Assert.AreEqual("libs/nested/sub".As(), resolved[0].Path); + } + + [TestMethod] + public void ThrowsForAMalformedGitlinkRecord() + { + _ = Assert.ThrowsExactly( + () => _ = GitSubmoduleParser.ParseGitlinks("160000 abc 0 no-tab-here\0")); + } + + [TestMethod] + public void ThrowsForAMalformedStatusLine() + { + IReadOnlyList gitlinks = + GitSubmoduleParser.ParseGitlinks("160000 1f2bee80cfcf06ee5ba820b17fe3b6ddca460915 0\tlibs/sub\0"); + + _ = Assert.ThrowsExactly( + () => _ = GitSubmoduleParser.ApplyStatus(gitlinks, "nospaceanywhere\n")); + } + + public TestContext TestContext { get; set; } = null!; +} + +[TestClass] +public class GitSubmoduleUpdateBuilderTests +{ + [TestMethod] + public void BuildsTheDefaultUpdateVector() + { + RecordingGitProcessRunner runner = new(); + GitSubmoduleUpdateBuilder builder = new(runner, TestPaths.Root); + + string[] expectedArguments = + [ + "-C", TestPaths.Root.WeakString, + "--no-pager", + "-c", "core.quotepath=false", + "-c", "color.ui=false", + "submodule", + "update", + ]; + CollectionAssert.AreEqual(expectedArguments, builder.BuildArguments().ToArray()); + } + + [TestMethod] + public void MapsEveryOptionToItsFlag() + { + RecordingGitProcessRunner runner = new(); + GitSubmoduleUpdateBuilder builder = new(runner, TestPaths.Root); + + _ = builder.Initialise().Recursive().FromRemote().Force().WithDepth(1); + + string[] arguments = [.. builder.BuildArguments()]; + + CollectionAssert.Contains(arguments, "--init"); + CollectionAssert.Contains(arguments, "--recursive"); + CollectionAssert.Contains(arguments, "--remote"); + CollectionAssert.Contains(arguments, "--force"); + CollectionAssert.Contains(arguments, "--depth"); + CollectionAssert.Contains(arguments, "1"); + } + + [TestMethod] + public void RejectsANonPositiveDepth() + { + RecordingGitProcessRunner runner = new(); + GitSubmoduleUpdateBuilder builder = new(runner, TestPaths.Root); + + Assert.ThrowsExactly(() => _ = builder.WithDepth(0)); + Assert.ThrowsExactly(() => _ = builder.WithDepth(-1)); + } + + [TestMethod] + public void RejectsANullProgressSink() + { + RecordingGitProcessRunner runner = new(); + GitSubmoduleUpdateBuilder builder = new(runner, TestPaths.Root); + + Assert.ThrowsExactly(() => _ = builder.ReportingProgress(null!)); + } + + [TestMethod] + public void ConfigurationMethodsReturnTheSameBuilderForChaining() + { + RecordingGitProcessRunner runner = new(); + GitSubmoduleUpdateBuilder builder = new(runner, TestPaths.Root); + + Assert.AreSame(builder, builder.Initialise().Recursive().FromRemote().Force().WithDepth(1)); + } +} + +[TestClass] +public class GitSubmoduleRecursionOptionTests +{ + [TestMethod] + public void CloneAndCheckoutEmitAPlainFlag() + { + // Neither takes a value: a fresh clone has no prior state for an on-demand decision, and + // git's own clone/checkout flags take none either. + RecordingGitProcessRunner runner = new(); + + GitCloneBuilder clone = new( + runner, + new FakeFileSystemProvider(new MockFileSystem()), + "https://example.com/x.git".As(), + TestPaths.Root); + _ = clone.RecursingSubmodules(); + CollectionAssert.Contains(clone.BuildArguments().ToArray(), "--recurse-submodules"); + + GitCheckoutBuilder checkout = new(runner, TestPaths.Root, "main".As()); + _ = checkout.RecursingSubmodules(); + CollectionAssert.Contains(checkout.BuildArguments().ToArray(), "--recurse-submodules"); + } + + [TestMethod] + public void FetchAndPullEmitTheValuedForm() + { + RecordingGitProcessRunner runner = new(); + + foreach ((GitSubmoduleRecursion recursion, string expected) in new[] + { + (GitSubmoduleRecursion.No, "no"), + (GitSubmoduleRecursion.Yes, "yes"), + (GitSubmoduleRecursion.OnDemand, "on-demand"), + }) + { + GitFetchBuilder fetch = new(runner, TestPaths.Root); + _ = fetch.RecursingSubmodules(recursion); + CollectionAssert.Contains(fetch.BuildArguments().ToArray(), "--recurse-submodules=" + expected); + + GitPullBuilder pull = new(runner, TestPaths.Root); + _ = pull.RecursingSubmodules(recursion); + CollectionAssert.Contains(pull.BuildArguments().ToArray(), "--recurse-submodules=" + expected); + } + } + + [TestMethod] + public void PushEmitsItsOwnValueSet() + { + // push spells the same flag name but means an unrelated thing, so it has its own enum whose + // values git accepts and the other verbs' do not. + RecordingGitProcessRunner runner = new(); + + foreach ((GitSubmodulePushCheck check, string expected) in new[] + { + (GitSubmodulePushCheck.No, "no"), + (GitSubmodulePushCheck.Check, "check"), + (GitSubmodulePushCheck.OnDemand, "on-demand"), + (GitSubmodulePushCheck.Only, "only"), + }) + { + GitPushBuilder push = new(runner, TestPaths.Root); + _ = push.CheckingSubmodules(check); + CollectionAssert.Contains(push.BuildArguments().ToArray(), "--recurse-submodules=" + expected); + } + } + + [TestMethod] + public void RejectsAnUnrecognisedEnumValue() + { + RecordingGitProcessRunner runner = new(); + + Assert.ThrowsExactly( + () => _ = new GitFetchBuilder(runner, TestPaths.Root).RecursingSubmodules((GitSubmoduleRecursion)99)); + Assert.ThrowsExactly( + () => _ = new GitPullBuilder(runner, TestPaths.Root).RecursingSubmodules((GitSubmoduleRecursion)99)); + Assert.ThrowsExactly( + () => _ = new GitPushBuilder(runner, TestPaths.Root).CheckingSubmodules((GitSubmodulePushCheck)99)); + } + + [TestMethod] + public void FetchDropsPorcelainWhenRecursingBecauseGitRefusesBothTogether() + { + // "fatal: options '--porcelain' and '--recurse-submodules' cannot be used together", + // verified against git 2.43. Emitting both would turn every recursing fetch into a failure, + // so the caller's request wins and the itemisation is given up. + RecordingGitProcessRunner runner = new(); + GitFetchBuilder builder = new(runner, TestPaths.Root); + + _ = builder.RecursingSubmodules(GitSubmoduleRecursion.Yes); + + string[] arguments = [.. builder.BuildArguments()]; + + CollectionAssert.DoesNotContain(arguments, "--porcelain"); + CollectionAssert.Contains(arguments, "--recurse-submodules=yes"); + } + + [TestMethod] + public void FetchKeepsPorcelainWhenNotRecursing() + { + RecordingGitProcessRunner runner = new(); + GitFetchBuilder builder = new(runner, TestPaths.Root); + + CollectionAssert.Contains(builder.BuildArguments().ToArray(), "--porcelain"); + } + + [TestMethod] + public async Task FetchReportsDetailUnavailableWhenRecursingAsync() + { + // The itemisation is genuinely gone, and DetailAvailable is what says so — an empty Updates + // list must never be read as "nothing changed". + ScriptedGitProcessRunner runner = new ScriptedGitProcessRunner() + .Then(standardOutput: "git version 2.43.0\n") + .Then(standardOutput: string.Empty); + GitFetchBuilder builder = new(runner, TestPaths.Root); + + _ = builder.RecursingSubmodules(GitSubmoduleRecursion.OnDemand); + + GitFetchResult result = await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.IsFalse(result.DetailAvailable); + Assert.AreEqual(0, result.Updates.Count); + Assert.IsFalse(result.IsUpToDate); + } + + public TestContext TestContext { get; set; } = null!; +} diff --git a/GitIntegration.Test/Builders/GitTagBuilderTests.cs b/GitIntegration.Test/Builders/GitTagBuilderTests.cs new file mode 100644 index 0000000..0ca9bd7 --- /dev/null +++ b/GitIntegration.Test/Builders/GitTagBuilderTests.cs @@ -0,0 +1,247 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitIntegration.Test; + +using System; +using System.Collections.Generic; +using System.Threading.Tasks; + +using ktsu.Semantics.Strings; + +[TestClass] +public class GitTagListBuilderTests +{ + private const string Us = "\u001f"; + + [TestMethod] + public void BuildsTheForEachRefVectorOverTheTagNamespace() + { + RecordingGitProcessRunner runner = new(); + GitTagListBuilder builder = new(runner, TestPaths.Root); + + string[] expectedArguments = + [ + "-C", TestPaths.Root.WeakString, + "--no-pager", + "-c", "core.quotepath=false", + "-c", "color.ui=false", + "for-each-ref", + "--format=" + GitOutputFormats.ForEachTagFormat, + "refs/tags", + ]; + CollectionAssert.AreEqual(expectedArguments, builder.BuildArguments().ToArray()); + } + + [TestMethod] + public void DoesNotGuardALibraryConstantWithTheEndOfOptionsMarker() + { + // refs/tags is a library constant, not a caller-supplied operand, so no caller value can + // reach that position and the marker would only add noise. GitBranchListBuilder makes the + // same call for the same reason. + RecordingGitProcessRunner runner = new(); + GitTagListBuilder builder = new(runner, TestPaths.Root); + + CollectionAssert.DoesNotContain(builder.BuildArguments().ToArray(), "--end-of-options"); + } + + [TestMethod] + public async Task ParsesBothKindsOfTagAsync() + { + // Captured from git 2.43 with this library's own format. The annotated record's objecttype is + // "tag" and its *objectname dereferences to the commit; the lightweight record's objecttype + // is "commit" and its *objectname is empty. + string output = + "ann1" + Us + "d829cd4fe5eccc7b6fa171efdba141ff41f00a71" + Us + "tag" + Us + + "3d61d00b68091ebb86034533f0267778e0addcf3" + Us + "annotated msg\n" + + "light1" + Us + "3d61d00b68091ebb86034533f0267778e0addcf3" + Us + "commit" + Us + Us + "c1\n"; + + RecordingGitProcessRunner runner = new() { StandardOutput = output }; + GitTagListBuilder builder = new(runner, TestPaths.Root); + + IReadOnlyList tags = await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.AreEqual(2, tags.Count); + + Assert.AreEqual("ann1".As(), tags[0].Name); + Assert.IsTrue(tags[0].IsAnnotated); + Assert.AreEqual("annotated msg", tags[0].Message); + + // Sha is the commit and ObjectSha is the tag object, so the two differ for an annotated tag. + Assert.AreEqual("3d61d00b68091ebb86034533f0267778e0addcf3".As(), tags[0].Sha); + Assert.AreEqual("d829cd4fe5eccc7b6fa171efdba141ff41f00a71".As(), tags[0].ObjectSha); + + Assert.AreEqual("light1".As(), tags[1].Name); + Assert.IsFalse(tags[1].IsAnnotated); + Assert.AreEqual(tags[1].Sha, tags[1].ObjectSha); + } + + [TestMethod] + public async Task ReportsNoMessageForALightweightTagEvenThoughGitPrintsOneAsync() + { + // The trap this parser exists to close. git's %(contents:subject) falls through to the + // commit's own subject when the reference names no tag object, so the field arrives populated + // with text that reads like a tag message and is not one. "c1" below is the commit's subject. + string output = + "light1" + Us + "3d61d00b68091ebb86034533f0267778e0addcf3" + Us + "commit" + Us + Us + "c1\n"; + + RecordingGitProcessRunner runner = new() { StandardOutput = output }; + GitTagListBuilder builder = new(runner, TestPaths.Root); + + IReadOnlyList tags = await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.IsNull(tags[0].Message); + } + + [TestMethod] + public async Task KeepsASeparatorEmbeddedInAMessageInThatFieldAsync() + { + // The subject is the only field not covered by git's ban on control characters in a reference + // name, and it is last, so a bounded split absorbs an embedded separator rather than letting + // the field count shift and the record be rejected. + string output = + "ann1" + Us + "d829cd4fe5eccc7b6fa171efdba141ff41f00a71" + Us + "tag" + Us + + "3d61d00b68091ebb86034533f0267778e0addcf3" + Us + "before" + Us + "after\n"; + + RecordingGitProcessRunner runner = new() { StandardOutput = output }; + GitTagListBuilder builder = new(runner, TestPaths.Root); + + IReadOnlyList tags = await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.AreEqual("before" + Us + "after", tags[0].Message); + } + + [TestMethod] + public async Task ReportsAnEmptyListForARepositoryWithNoTagsAsync() + { + RecordingGitProcessRunner runner = new() { StandardOutput = string.Empty }; + GitTagListBuilder builder = new(runner, TestPaths.Root); + + IReadOnlyList tags = await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.AreEqual(0, tags.Count); + } + + [TestMethod] + public async Task ThrowsForAMalformedRecordAsync() + { + RecordingGitProcessRunner runner = new() { StandardOutput = "ann1" + Us + "deadbeef\n" }; + GitTagListBuilder builder = new(runner, TestPaths.Root); + + await Assert.ThrowsExactlyAsync( + async () => await builder.ExecuteAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) + .ConfigureAwait(false); + } + + public TestContext TestContext { get; set; } = null!; +} + +[TestClass] +public class GitTagWriteBuilderTests +{ + private static GitTagName Version => "v1.2.3".As(); + + [TestMethod] + public void BuildsALightweightTagVector() + { + RecordingGitProcessRunner runner = new(); + GitTagCreateBuilder builder = new(runner, TestPaths.Root, Version); + + string[] arguments = [.. builder.BuildArguments()]; + + CollectionAssert.Contains(arguments, "tag"); + CollectionAssert.DoesNotContain(arguments, "--annotate"); + Assert.AreEqual("v1.2.3", arguments[^1]); + } + + [TestMethod] + public void EmitsAnnotateAndMessageTogether() + { + // Either flag alone is a different command: --annotate with no message opens an editor that + // output redirection makes impossible to answer, and --message alone relies on a side effect + // git documents rather than promises. + RecordingGitProcessRunner runner = new(); + GitTagCreateBuilder builder = new(runner, TestPaths.Root, Version); + + _ = builder.Annotating("release 1.2.3".As()); + + string[] arguments = [.. builder.BuildArguments()]; + int annotate = Array.IndexOf(arguments, "--annotate"); + + Assert.AreNotEqual(-1, annotate); + Assert.AreEqual("--message", arguments[annotate + 1]); + Assert.AreEqual("release 1.2.3", arguments[annotate + 2]); + } + + [TestMethod] + public void PutsTheNameBeforeTheTarget() + { + // git tag takes then an optional positionally. Swapping them tags the wrong + // object under the wrong name without complaint, so the order is pinned rather than assumed. + RecordingGitProcessRunner runner = new(); + GitTagCreateBuilder builder = new(runner, TestPaths.Root, Version); + + _ = builder.At("main".As()); + + string[] arguments = [.. builder.BuildArguments()]; + int marker = Array.IndexOf(arguments, "--end-of-options"); + + Assert.AreEqual("v1.2.3", arguments[marker + 1]); + Assert.AreEqual("main", arguments[marker + 2]); + } + + [TestMethod] + public void KeepsEveryFlagBeforeTheEndOfOptionsMarker() + { + RecordingGitProcessRunner runner = new(); + GitTagCreateBuilder builder = new(runner, TestPaths.Root, Version); + + _ = builder.Force().Annotating("release".As()); + + string[] arguments = [.. builder.BuildArguments()]; + int marker = Array.IndexOf(arguments, "--end-of-options"); + + Assert.IsTrue(Array.IndexOf(arguments, "--force") < marker); + Assert.IsTrue(Array.IndexOf(arguments, "--annotate") < marker); + } + + [TestMethod] + public void BuildsTheDeleteVector() + { + RecordingGitProcessRunner runner = new(); + GitTagDeleteBuilder builder = new(runner, TestPaths.Root, Version); + + string[] expectedArguments = + [ + "-C", TestPaths.Root.WeakString, + "--no-pager", + "-c", "core.quotepath=false", + "-c", "color.ui=false", + "tag", + "--delete", + "--end-of-options", + "v1.2.3", + ]; + CollectionAssert.AreEqual(expectedArguments, builder.BuildArguments().ToArray()); + } + + [TestMethod] + public void ConfigurationMethodsReturnTheSameBuilderForChaining() + { + RecordingGitProcessRunner runner = new(); + GitTagCreateBuilder builder = new(runner, TestPaths.Root, Version); + + Assert.AreSame(builder, builder.At("main".As()).Annotating("m".As()).Force()); + } + + [TestMethod] + public void RejectsNullArguments() + { + RecordingGitProcessRunner runner = new(); + GitTagCreateBuilder builder = new(runner, TestPaths.Root, Version); + + Assert.ThrowsExactly(() => _ = new GitTagCreateBuilder(runner, TestPaths.Root, null!)); + Assert.ThrowsExactly(() => _ = new GitTagDeleteBuilder(runner, TestPaths.Root, null!)); + Assert.ThrowsExactly(() => _ = builder.At(null!)); + Assert.ThrowsExactly(() => _ = builder.Annotating(null!)); + } +} diff --git a/GitIntegration.Test/Fakes/FakeHttpMessageHandler.cs b/GitIntegration.Test/Fakes/FakeHttpMessageHandler.cs index 0193a29..0fe1de6 100644 --- a/GitIntegration.Test/Fakes/FakeHttpMessageHandler.cs +++ b/GitIntegration.Test/Fakes/FakeHttpMessageHandler.cs @@ -24,7 +24,12 @@ namespace ktsu.GitIntegration.Test; /// internal sealed class FakeHttpMessageHandler : HttpMessageHandler { - private readonly Queue _responses = new(); + // Concurrent for the same reason _requests is: SendAsync is the one member a caller does not + // control the timing of, and nothing stops a provider issuing two requests through one handler at + // once. Both providers currently await sequentially, so there is no live race — but a plain Queue + // read and dequeued inside SendAsync is one concurrent provider away from corrupting, and the + // reasoning that made _requests concurrent applies here verbatim. + private readonly ConcurrentQueue _responses = new(); // A concurrent queue rather than a List: SendAsync is the one member a caller does not control // the timing of, and nothing stops a provider issuing two requests through one handler at once. @@ -99,15 +104,17 @@ protected override async Task SendAsync(HttpRequestMessage // Running out of queued responses means the code under test issued a request the test did // not anticipate. Failing here names that request; returning a default would hide it. - if (_responses.Count == 0) + // + // TryDequeue rather than a Count check followed by a Dequeue: on a concurrent queue those two + // steps are separately atomic but not atomic together, so the emptiness a Count check observes + // is already stale by the time the Dequeue runs. + if (!_responses.TryDequeue(out QueuedResponse queued)) { throw new InvalidOperationException( $"No queued response for {request.Method} {request.RequestUri}: " + $"{_totalQueued} queued, {_requests.Count} arrived."); } - QueuedResponse queued = _responses.Dequeue(); - HttpResponseMessage response = new(queued.Status) { Content = new StringContent(queued.Body, Encoding.UTF8), @@ -130,10 +137,15 @@ protected override async Task SendAsync(HttpRequestMessage // a second value, which would make HttpContentHeaders.ContentType unparsable. response.Content.Headers.Remove(name); - if (!response.Content.Headers.TryAddWithoutValidation(name, value)) - { - throw new InvalidOperationException($"Header '{name}' fits neither response nor content headers."); - } + // An assertion rather than a throw guarding an "it fits neither collection" branch. + // That branch was unreachable: TryAddWithoutValidation skips name and value validation + // entirely, and reaching this catch at all means response.Headers.Add already accepted + // the name as a well-formed header token before rejecting it for being a content + // header. Untestable defensive code invites the reader to work out when it fires; + // an assertion says plainly that it never should. + Assert.IsTrue( + response.Content.Headers.TryAddWithoutValidation(name, value), + $"Header '{name}' fits neither response nor content headers."); } } diff --git a/GitIntegration.Test/Fixtures/github-repositories.json b/GitIntegration.Test/Fixtures/github-repositories.json index 6c2d126..93b43f5 100644 --- a/GitIntegration.Test/Fixtures/github-repositories.json +++ b/GitIntegration.Test/Fixtures/github-repositories.json @@ -1,7 +1,7 @@ [ { "id": 90000001, - "node_id": "MDEwOlJlcG9zaXRvcnk5MDAxMDAwMQ==", + "node_id": "MDEwOlJlcG9zaXRvcnk5MDAwMDAwMQ==", "name": "example-repo-1", "full_name": "example-user/example-repo-1", "private": false, @@ -104,7 +104,7 @@ }, { "id": 90000002, - "node_id": "MDEwOlJlcG9zaXRvcnk5MDAxMDAwMQ==", + "node_id": "MDEwOlJlcG9zaXRvcnk5MDAwMDAwMg==", "name": "example-repo-2", "full_name": "example-user/example-repo-2", "private": false, @@ -213,7 +213,7 @@ }, { "id": 90000003, - "node_id": "MDEwOlJlcG9zaXRvcnk5MDAxMDAwMQ==", + "node_id": "MDEwOlJlcG9zaXRvcnk5MDAwMDAwMw==", "name": "example-repo-3", "full_name": "example-user/example-repo-3", "private": false, diff --git a/GitIntegration.Test/GitRepositoryMetadataTests.cs b/GitIntegration.Test/GitRepositoryMetadataTests.cs index c0d480e..a160d9f 100644 --- a/GitIntegration.Test/GitRepositoryMetadataTests.cs +++ b/GitIntegration.Test/GitRepositoryMetadataTests.cs @@ -96,6 +96,35 @@ public void BrowsableUriRejectsNull() Assert.IsFalse(GitRepository.IsBrowsableUri(null, out Uri? uri)); Assert.IsNull(uri); } + + [TestMethod] + public void ReportsTheMissingLocalPathWhenAVerbIsCalledWithoutOne() + { + // LocalPath is nullable so a hosting provider need not invent one. Every verb needs a path, so + // each throws for a missing one exactly as it already does for a missing ProcessRunner. Both + // properties are public init accessors, so a caller can supply one and not the other and each + // check has to stand on its own — this repository carries a runner and no path. + GitRepository repository = new() { ProcessRunner = new RecordingGitProcessRunner() }; + + InvalidOperationException exception = + Assert.ThrowsExactly(() => _ = repository.Status()); + + StringAssert.Contains(exception.Message, "no local path"); + } + + [TestMethod] + public void ReportsTheMissingRunnerFirstWhenARepositoryHasNeither() + { + // RequireRunner is evaluated before RequireLocalPath at every call site, so a repository + // carrying hosting metadata only reports the runner — the more specific thing to have got + // wrong, and the one whose message names how to obtain a usable repository. + GitRepository repository = new() { Name = "example".As() }; + + InvalidOperationException exception = + Assert.ThrowsExactly(() => _ = repository.Status()); + + StringAssert.Contains(exception.Message, "no process runner"); + } } /// Paths that exist on every platform the tests run on. diff --git a/GitIntegration.Test/GitRepositoryVerbTests.cs b/GitIntegration.Test/GitRepositoryVerbTests.cs index 1d419d3..8cc974a 100644 --- a/GitIntegration.Test/GitRepositoryVerbTests.cs +++ b/GitIntegration.Test/GitRepositoryVerbTests.cs @@ -27,6 +27,11 @@ [.. repository.Diff().BuildArguments()], [.. repository.Branches().BuildArguments()], [.. repository.Remotes().BuildArguments()], [.. repository.RevParse("HEAD".As()).BuildArguments()], + [.. repository.Tags().BuildArguments()], + [.. repository.Submodules().BuildArguments()], + [.. repository.UpdateSubmodules().BuildArguments()], + [.. repository.RevList("HEAD".As()).BuildArguments()], + [.. repository.Divergence("origin/main".As(), "HEAD".As()).BuildArguments()], ]; foreach (string[] vector in vectors) @@ -70,6 +75,12 @@ public void VerbsOnAMetadataOnlyRepositoryExplainWhatIsMissing() _ = Assert.ThrowsExactly(() => _ = repository.Diff()); _ = Assert.ThrowsExactly(() => _ = repository.Branches()); _ = Assert.ThrowsExactly(() => _ = repository.Remotes()); + _ = Assert.ThrowsExactly(() => _ = repository.Tags()); + _ = Assert.ThrowsExactly(() => _ = repository.Submodules()); + _ = Assert.ThrowsExactly(() => _ = repository.UpdateSubmodules()); + _ = Assert.ThrowsExactly(() => _ = repository.RevList("HEAD".As())); + _ = Assert.ThrowsExactly( + () => _ = repository.Divergence("origin/main".As(), "HEAD".As())); // A valid revision, so the guard is what fires rather than the null check. _ = Assert.ThrowsExactly(() => _ = repository.RevParse("HEAD".As())); @@ -79,6 +90,29 @@ public void VerbsOnAMetadataOnlyRepositoryExplainWhatIsMissing() _ = Assert.ThrowsExactly(() => _ = repository.IsClonedAsync()); } + [TestMethod] + public void TagVerbsRouteThroughTheSameGuardsAsEveryOtherVerb() + { + // Argument validation runs before RequireRunner at every verb taking an operand, so a null + // name on a metadata-only repository reports the null rather than the missing runner — the + // caller's own mistake, not the repository's shape. + GitRepository metadataOnly = new() { Name = "GitIntegration".As() }; + + _ = Assert.ThrowsExactly(() => _ = metadataOnly.CreateTag(null!)); + _ = Assert.ThrowsExactly(() => _ = metadataOnly.DeleteTag(null!)); + + GitTagName version = "v1.0.0".As(); + _ = Assert.ThrowsExactly(() => _ = metadataOnly.CreateTag(version)); + _ = Assert.ThrowsExactly(() => _ = metadataOnly.DeleteTag(version)); + + RecordingGitProcessRunner runner = new(); + GitRepository repository = RepositoryOn(runner); + + Assert.AreNotSame(repository.CreateTag(version), repository.CreateTag(version)); + Assert.AreNotSame(repository.DeleteTag(version), repository.DeleteTag(version)); + Assert.AreNotSame(repository.Tags(), repository.Tags()); + } + [TestMethod] public void RevParseRejectsANullRevision() { diff --git a/GitIntegration.Test/Hosting/AzureDevOpsProviderTests.cs b/GitIntegration.Test/Hosting/AzureDevOpsProviderTests.cs index 9265777..e1b7274 100644 --- a/GitIntegration.Test/Hosting/AzureDevOpsProviderTests.cs +++ b/GitIntegration.Test/Hosting/AzureDevOpsProviderTests.cs @@ -13,7 +13,6 @@ namespace ktsu.GitIntegration.Test; using System.Threading.Tasks; using ktsu.CredentialCache; -using ktsu.Semantics.Paths; using ktsu.Semantics.Strings; // System.Net (for HttpStatusCode) and ktsu.CredentialCache both declare a type named @@ -32,6 +31,15 @@ public sealed class AzureDevOpsProviderTests // parallelism. This class must not add a second call site. /// Reads a captured fixture's raw JSON text from the test output's Fixtures directory. + /// + /// The Azure DevOps fixtures keep two things from Microsoft's own documented samples that look + /// like oversights and are not. The synthetic user npaulk is Microsoft's placeholder, kept + /// so a reader comparing a fixture against the published sample sees the same value rather than + /// wondering which fields this library altered; and homepage retains the real + /// docs.microsoft.com domain with only the organisation path segment swapped, for the same + /// reason. Neither is a credential or an internal hostname, both are test-only assets that are + /// never packed, and no request is ever issued against either. + /// private static string Fixture(string name) => File.ReadAllText(Path.Combine(AppContext.BaseDirectory, "Fixtures", name)); @@ -114,8 +122,13 @@ private static string SingleRepositoryResponse(string? name) } [TestMethod] - public async Task MapsAnOrdinaryRepositoryNameToALocalPathUnderTheCurrentDirectoryAsync() + public async Task ReportsNoLocalPathForAnEnumeratedRepositoryAsync() { + // An enumerated repository has never been cloned, so there is no local path to report. This + // used to be filled with Path.Combine(Environment.CurrentDirectory, name), which made the same + // remote repository yield a different record depending on when it was enumerated — and which + // needed a containment guard purely to keep a name from a remote response from escaping that + // directory. Saying "not known" needs neither. using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() .Respond(HttpStatusCode.OK, SingleRepositoryResponse("my-repo"), ("Content-Type", "application/json")); AzureDevOpsProvider provider = new() { Owner = "contoso".As(), Handler = handler }; @@ -123,82 +136,129 @@ public async Task MapsAnOrdinaryRepositoryNameToALocalPathUnderTheCurrentDirecto IReadOnlyList repositories = await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); - Assert.AreEqual( - Path.Combine(Environment.CurrentDirectory, "my-repo").As(), - repositories[0].LocalPath); + Assert.IsNull(repositories[0].LocalPath); + Assert.AreEqual("my-repo".As(), repositories[0].Name); } [TestMethod] - public async Task ThrowsWhenARootedRepositoryNameHasNoLeafSegmentAsync() + public async Task CarriesAzureDevOpsOwnRepositoryIdAsync() { - // "/" is rooted on every platform .NET runs this suite on (both Windows and POSIX treat "/" - // as a separator), and is exactly the shape that let the old - // Path.Combine(Environment.CurrentDirectory, name) call silently discard - // Environment.CurrentDirectory and report LocalPath as the bare root. Path.GetFileName("/") - // strips the root and leaves nothing behind, so this is treated as the host having sent a - // name this library cannot represent as a directory, rather than as a value that happens to - // look safe once stripped. + // Microsoft's reference types {repositoryId} as string (uuid) and never sanctions a name + // there, so the id has to survive enumeration for a caller to address the repository the way + // the schema documents. using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() - .Respond(HttpStatusCode.OK, SingleRepositoryResponse("/"), ("Content-Type", "application/json")); + .Respond(HttpStatusCode.OK, SingleRepositoryResponse("my-repo"), ("Content-Type", "application/json")); AzureDevOpsProvider provider = new() { Owner = "contoso".As(), Handler = handler }; - GitHostingRequestException exception = await Assert.ThrowsExactlyAsync( - async () => await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) - .ConfigureAwait(false); + IReadOnlyList repositories = + await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); - StringAssert.Contains(exception.Message, "cannot be represented as a local directory"); + Assert.AreEqual( + "5febef5a-833d-4e14-b9c0-14cb638f91e6".As(), + repositories[0].HostRepositoryId); } [TestMethod] - public async Task ThrowsWhenARepositoryNameIsADirectoryTraversalTokenAsync() + public async Task AddressesARepositoryByItsHostIdWhenListingPullRequestsAsync() { - // Path.GetFileName("..") returns ".." unchanged — stripping a rooted prefix does not resolve - // this case, which is exactly why the mapping checks the derived leaf itself rather than - // trusting Path.GetFileName alone. + // The point of carrying the id: it has to reach the {repositoryId} path segment. The + // name-taking overload cannot do this, because it is never given an id. using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() - .Respond(HttpStatusCode.OK, SingleRepositoryResponse(".."), ("Content-Type", "application/json")); - AzureDevOpsProvider provider = new() { Owner = "contoso".As(), Handler = handler }; + .Respond(HttpStatusCode.OK, "{\"count\":0,\"value\":[]}", ("Content-Type", "application/json")); + AzureDevOpsProvider provider = new() + { + Owner = "contoso".As(), + Project = "ExampleProject".As(), + Handler = handler, + }; - GitHostingRequestException exception = await Assert.ThrowsExactlyAsync( - async () => await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) - .ConfigureAwait(false); + GitRepository repository = new() + { + Name = "my-repo".As(), + HostRepositoryId = "5febef5a-833d-4e14-b9c0-14cb638f91e6".As(), + }; - StringAssert.Contains(exception.Message, ".."); + _ = await provider.GetPullRequestsAsync(repository, TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + StringAssert.Contains( + handler.Requests[0].Uri.AbsoluteUri, + "/repositories/5febef5a-833d-4e14-b9c0-14cb638f91e6/pullrequests"); } [TestMethod] - public async Task ThrowsWhenARepositoryNameIsWhitespaceOnlyAsync() + public async Task FallsBackToTheNameWhenARepositoryCarriesNoHostIdAsync() { - // Path.GetFileName(" ") returns " " unchanged — there is no separator to strip, so the - // leaf is exactly the whitespace-only input. GitRepositoryName itself rejects a - // whitespace-only value ([HasNonWhitespaceContent]), so LocalPath is held to the same rule: - // a name the semantic type would refuse is not a name this mapping can turn into a directory - // either. + // A caller may legitimately have built a GitRepository from a name alone. That substitution is + // the same unconfirmed one the name-taking overload already makes, so it is allowed rather + // than rejected — but only as a fallback, never in preference to an id. using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() - .Respond(HttpStatusCode.OK, SingleRepositoryResponse(" "), ("Content-Type", "application/json")); - AzureDevOpsProvider provider = new() { Owner = "contoso".As(), Handler = handler }; + .Respond(HttpStatusCode.OK, "{\"count\":0,\"value\":[]}", ("Content-Type", "application/json")); + AzureDevOpsProvider provider = new() + { + Owner = "contoso".As(), + Project = "ExampleProject".As(), + Handler = handler, + }; - GitHostingRequestException exception = await Assert.ThrowsExactlyAsync( - async () => await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) - .ConfigureAwait(false); + GitRepository repository = new() { Name = "my-repo".As() }; - StringAssert.Contains(exception.Message, "cannot be represented as a local directory"); + _ = await provider.GetPullRequestsAsync(repository, TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + StringAssert.Contains(handler.Requests[0].Uri.AbsoluteUri, "/repositories/my-repo/pullrequests"); } [TestMethod] - public async Task ThrowsWhenARepositoryNameIsMissingAsync() + public void RejectsARepositoryCarryingNeitherIdentifier() { - // Azure DevOps's schema allows an absent name (AzureDevOpsRepository.Name is nullable), unlike - // GitHub's. Before this fix an absent name degraded silently to Environment.CurrentDirectory - // itself; failing loudly is the correct outcome for a value this library cannot represent as a - // directory, matching the same rule a rooted or traversal name is held to. + AzureDevOpsProvider provider = new() + { + Owner = "contoso".As(), + Project = "ExampleProject".As(), + }; + + GitRepository repository = new(); + + _ = Assert.ThrowsExactly(() => _ = provider.GetPullRequestsAsync(repository)); + _ = Assert.ThrowsExactly(() => _ = provider.CreatePullRequest(repository)); + } + + [TestMethod] + public async Task ThrowsAHostingFailureWhenAReportedNameIsNotOneThisLibraryCanRepresentAsync() + { + // GitRepositoryName is validated as a single path segment, so a host reporting "/" or ".." + // produces a value the semantic type refuses. That has to surface as a GitHostingException, + // not as the ArgumentException the semantic type itself raises: a public hosting method's + // documented failure surface is this hierarchy, the same rule that makes an unparsable success + // body a GitHostingRequestException rather than a JsonException. + foreach (string reported in new[] { "/", "..", " ", "owner/repo" }) + { + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .Respond(HttpStatusCode.OK, SingleRepositoryResponse(reported), ("Content-Type", "application/json")); + AzureDevOpsProvider provider = new() { Owner = "contoso".As(), Handler = handler }; + + GitHostingRequestException exception = await Assert.ThrowsExactlyAsync( + async () => await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) + .ConfigureAwait(false); + + StringAssert.Contains(exception.Message, "cannot represent"); + } + } + + [TestMethod] + public async Task ReportsNoNameWhenAzureDevOpsOmitsOneAsync() + { + // Azure DevOps's schema allows an absent name, unlike GitHub's, and GitRepository.Name is + // nullable to match. An omitted optional field is ordinary rather than a failure — only a + // field the host did report and this library cannot represent is worth raising. using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() .Respond(HttpStatusCode.OK, SingleRepositoryResponse(null), ("Content-Type", "application/json")); AzureDevOpsProvider provider = new() { Owner = "contoso".As(), Handler = handler }; - await Assert.ThrowsExactlyAsync( - async () => await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) - .ConfigureAwait(false); + IReadOnlyList repositories = + await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); + + Assert.IsNull(repositories[0].Name); + Assert.IsNotNull(repositories[0].HostRepositoryId); } [TestMethod] diff --git a/GitIntegration.Test/Hosting/GitHubProviderTests.cs b/GitIntegration.Test/Hosting/GitHubProviderTests.cs index 4c94b67..fcb5b35 100644 --- a/GitIntegration.Test/Hosting/GitHubProviderTests.cs +++ b/GitIntegration.Test/Hosting/GitHubProviderTests.cs @@ -9,9 +9,10 @@ namespace ktsu.GitIntegration.Test; using System.Threading.Tasks; using ktsu.CredentialCache; -using ktsu.Semantics.Paths; using ktsu.Semantics.Strings; +using Octokit; + // System.Net (for HttpStatusCode) and ktsu.CredentialCache both declare a type named // CredentialCache; this alias resolves the ambiguity in favour of the credential store. using CredentialCache = ktsu.CredentialCache.CredentialCache; @@ -66,7 +67,7 @@ private static string SinglePullRequestArray(string state, bool merged, bool dra /// The value to send as the repository's name field. private static string SingleRepositoryArray(string name) => $$""" - [{"name":"{{name}}","html_url":"https://github.com/example-user/example-repo","clone_url":"https://github.com/example-user/example-repo.git"}] + [{"id":90000001,"name":"{{name}}","html_url":"https://github.com/example-user/example-repo","clone_url":"https://github.com/example-user/example-repo.git"}] """; [TestMethod] @@ -116,8 +117,13 @@ public async Task EnumeratesRepositoriesForTheOwnerAsync() } [TestMethod] - public async Task MapsAnOrdinaryRepositoryNameToALocalPathUnderTheCurrentDirectoryAsync() + public async Task ReportsNoLocalPathForAnEnumeratedRepositoryAsync() { + // An enumerated repository has never been cloned, so there is no local path to report. This + // used to be filled with Path.Combine(Environment.CurrentDirectory, name), which made the same + // remote repository yield a different record depending on when it was enumerated — and which + // needed a containment guard purely to keep a name from a remote response from escaping that + // directory. Saying "not known" needs neither. using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() .Respond(HttpStatusCode.OK, SingleRepositoryArray("my-repo"), ("Content-Type", "application/json")); GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; @@ -125,66 +131,67 @@ public async Task MapsAnOrdinaryRepositoryNameToALocalPathUnderTheCurrentDirecto IReadOnlyList repositories = await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); - Assert.AreEqual( - Path.Combine(Environment.CurrentDirectory, "my-repo").As(), - repositories[0].LocalPath); + Assert.IsNull(repositories[0].LocalPath); + Assert.AreEqual("my-repo".As(), repositories[0].Name); } [TestMethod] - public async Task ThrowsWhenARootedRepositoryNameHasNoLeafSegmentAsync() + public async Task CarriesGitHubsOwnRepositoryIdAsync() { - // "/" is rooted on every platform .NET runs this suite on (both Windows and POSIX treat "/" - // as a separator), and is exactly the shape that let the old - // Path.Combine(Environment.CurrentDirectory, repository.Name) call silently discard - // Environment.CurrentDirectory and report LocalPath as the bare root. Path.GetFileName("/") - // strips the root and leaves nothing behind, so this is treated as the host having sent a - // name this library cannot represent as a directory, rather than as a value that happens to - // look safe once stripped. + // GitHub's id is a decimal number where Azure DevOps's is a uuid, which is why + // GitHostRepositoryId is a string: the value is handed back to the host it came from and + // never parsed, so a type insisting on either host's shape would exclude the other. using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() - .Respond(HttpStatusCode.OK, SingleRepositoryArray("/"), ("Content-Type", "application/json")); + .Respond(HttpStatusCode.OK, SingleRepositoryArray("my-repo"), ("Content-Type", "application/json")); GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; - GitHostingRequestException exception = await Assert.ThrowsExactlyAsync( - async () => await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) - .ConfigureAwait(false); + IReadOnlyList repositories = + await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false); - StringAssert.Contains(exception.Message, "cannot be represented as a local directory"); + Assert.AreEqual("90000001".As(), repositories[0].HostRepositoryId); } [TestMethod] - public async Task ThrowsWhenARepositoryNameIsADirectoryTraversalTokenAsync() + public async Task ThrowsAHostingFailureWhenAReportedNameIsNotOneThisLibraryCanRepresentAsync() { - // Path.GetFileName("..") returns ".." unchanged — stripping a rooted prefix does not resolve - // this case, which is exactly why the mapping checks the derived leaf itself rather than - // trusting Path.GetFileName alone. - using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() - .Respond(HttpStatusCode.OK, SingleRepositoryArray(".."), ("Content-Type", "application/json")); - GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; + // GitRepositoryName is validated as a single path segment, so a host reporting "/" or ".." + // produces a value the semantic type refuses. That has to surface as a GitHostingException, + // not as the ArgumentException the semantic type itself raises: a public hosting method's + // documented failure surface is this hierarchy, the same rule that makes an unparsable success + // body a GitHostingRequestException rather than a JsonException. + foreach (string reported in new[] { "/", "..", " ", "owner/repo" }) + { + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .Respond(HttpStatusCode.OK, SingleRepositoryArray(reported), ("Content-Type", "application/json")); + GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; - GitHostingRequestException exception = await Assert.ThrowsExactlyAsync( - async () => await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) - .ConfigureAwait(false); + GitHostingRequestException exception = await Assert.ThrowsExactlyAsync( + async () => await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) + .ConfigureAwait(false); - StringAssert.Contains(exception.Message, ".."); + StringAssert.Contains(exception.Message, "cannot represent"); + } } [TestMethod] - public async Task ThrowsWhenARepositoryNameIsWhitespaceOnlyAsync() + public async Task AddressesARepositoryByItsHostIdWhenListingPullRequestsAsync() { - // Path.GetFileName(" ") returns " " unchanged — there is no separator to strip, so the - // leaf is exactly the whitespace-only input. GitRepositoryName itself rejects a - // whitespace-only value ([HasNonWhitespaceContent]), so LocalPath is held to the same rule: - // a name the semantic type would refuse is not a name this mapping can turn into a directory - // either. + // The point of carrying the id: it has to reach the request path. GitHub happens to accept a + // name in this position, but both providers answer the same question the same way under one + // interface, so both prefer the host's own identifier when one is known. using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() - .Respond(HttpStatusCode.OK, SingleRepositoryArray(" "), ("Content-Type", "application/json")); + .Respond(HttpStatusCode.OK, "[]", ("Content-Type", "application/json")); GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; - GitHostingRequestException exception = await Assert.ThrowsExactlyAsync( - async () => await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) - .ConfigureAwait(false); + GitRepository repository = new() + { + Name = "my-repo".As(), + HostRepositoryId = "90000001".As(), + }; + + _ = await provider.GetPullRequestsAsync(repository, TestContext.CancellationTokenSource.Token).ConfigureAwait(false); - StringAssert.Contains(exception.Message, "cannot be represented as a local directory"); + StringAssert.Contains(handler.Requests[0].Uri.AbsoluteUri, "/repos/contoso/90000001/pulls"); } [TestMethod] @@ -469,6 +476,69 @@ public async Task LeavesResetsAtNullWhenATooManyRequestsResponseCarriesNoRetryAf Assert.IsNull(exception.ResetsAt); } + [TestMethod] + public async Task LeavesResetsAtNullWhenRetryAfterCarriesAnHttpDateAsync() + { + // Retry-After may carry an HTTP date rather than a delay in seconds. GitHub sends seconds, so + // TryGetRetryAfterSeconds parses only that form and a date yields null — correct by + // inspection, and now pinned. An invented instant derived from a form this library does not + // actually parse would be worse than no instant at all. + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .Respond( + HttpStatusCode.TooManyRequests, + "{\"message\":\"You have exceeded a rate limit\"}", + ("Content-Type", "application/json"), + ("Retry-After", "Wed, 21 Oct 2026 07:28:00 GMT")); + GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; + + GitHostingRateLimitException exception = await Assert.ThrowsExactlyAsync( + async () => await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) + .ConfigureAwait(false); + + Assert.AreEqual(HttpStatusCode.TooManyRequests, exception.StatusCode); + Assert.IsNull(exception.ResetsAt); + } + + [TestMethod] + public async Task KeepsTheOctokitFailureAsTheInnerExceptionAsync() + { + // The hierarchy carries the provider, status, and body, which is enough to reproduce a + // failure by hand — but not everything the host supplied. ApiError.Errors is often the only + // place GitHub says what was actually wrong with a request, and it has no field here, so + // discarding the Octokit exception discards it too. + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .Respond( + HttpStatusCode.Forbidden, + "{\"message\":\"Resource not accessible by personal access token\"}", + ("Content-Type", "application/json")); + GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; + + GitHostingAuthenticationException exception = await Assert.ThrowsExactlyAsync( + async () => await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) + .ConfigureAwait(false); + + Assert.IsInstanceOfType(exception.InnerException); + } + + [TestMethod] + public async Task KeepsTheInnerExceptionEvenWhenOctokitAttachedNoResponseAsync() + { + // The worst case the inner exception exists for. Octokit reports a 404 on a GET through a + // synthetic exception with no HttpResponse, so ResponseBody is empty and there is no other + // context to fall back on — without the inner exception this failure reaches a caller + // carrying only a message. + using FakeHttpMessageHandler handler = new FakeHttpMessageHandler() + .Respond(HttpStatusCode.NotFound, "{\"message\":\"Not Found\"}", ("Content-Type", "application/json")); + GitHubProvider provider = new() { Owner = "contoso".As(), Handler = handler }; + + GitHostingNotFoundException exception = await Assert.ThrowsExactlyAsync( + async () => await provider.GetRepositoriesAsync(TestContext.CancellationTokenSource.Token).ConfigureAwait(false)) + .ConfigureAwait(false); + + Assert.AreEqual(string.Empty, exception.ResponseBody); + Assert.IsInstanceOfType(exception.InnerException); + } + [TestMethod] public async Task TranslatesANotFoundResponseToGitHostingNotFoundExceptionAsync() { diff --git a/GitIntegration.Test/Hosting/GitProviderTests.cs b/GitIntegration.Test/Hosting/GitProviderTests.cs index 485bbc7..6bac4d2 100644 --- a/GitIntegration.Test/Hosting/GitProviderTests.cs +++ b/GitIntegration.Test/Hosting/GitProviderTests.cs @@ -4,7 +4,9 @@ namespace ktsu.GitIntegration.Test; using System; using System.Collections.Generic; +using System.Linq; using System.Net.Http; +using System.Reflection; using System.Threading; using System.Threading.Tasks; @@ -170,6 +172,31 @@ public void TheSharedTransportBoundsItsPooledConnectionLifetime() handler.AutomaticDecompression); } + [TestMethod] + public void EachProviderOwnsItsSharedTransportRatherThanInheritingOne() + { + // GitProvider used to hold one static SocketsHttpHandler that every subclass shared. That was + // equivalent to per-provider only by accident, because AzureDevOpsProvider was its sole user + // and GitHubProvider already had its own; adding a third provider would have silently + // enrolled it in Azure DevOps's connection pool with nothing in the code to notice. + // + // Reflection rather than reading DefaultHandler directly, because that member is + // private protected and this test class is not a derived type. It also pins the property that + // actually matters — where the handler is declared — rather than what one instance returns. + Assert.IsFalse( + typeof(GitProvider).GetFields(BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public) + .Any(field => typeof(HttpMessageHandler).IsAssignableFrom(field.FieldType)), + "GitProvider must not hold a transport its subclasses would share implicitly."); + + foreach (Type providerType in new[] { typeof(GitHubProvider), typeof(AzureDevOpsProvider) }) + { + Assert.IsTrue( + providerType.GetFields(BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public) + .Any(field => typeof(HttpMessageHandler).IsAssignableFrom(field.FieldType)), + $"{providerType.Name} must declare its own shared transport."); + } + } + private sealed class UnrecognisedCredential : Credential; // The minimal subclass a test needs to reach GitProvider's protected members. Its own three @@ -179,13 +206,19 @@ private sealed class TestProvider : GitProvider { public override GitProviderName Name => "TestProvider".As(); + // Never reached: these tests either inject a Handler or never issue a request at all. The + // member is abstract so that each real provider has to name its own shared transport rather + // than inherit one, which is the point of it existing. + private protected override HttpMessageHandler DefaultHandler => + throw new NotSupportedException("Not exercised by these tests."); + public override Task> GetRepositoriesAsync(CancellationToken cancellationToken = default) => throw new NotSupportedException("Not exercised by these tests."); - public override Task> GetPullRequestsAsync(GitRepositoryName repositoryName, CancellationToken cancellationToken = default) => + internal override Task> GetPullRequestsCoreAsync(string repositoryIdentifier, CancellationToken cancellationToken) => throw new NotSupportedException("Not exercised by these tests."); - internal override Task CreatePullRequestCoreAsync(GitRepositoryName repositoryName, GitPullRequestSpecification specification, CancellationToken cancellationToken) => + internal override Task CreatePullRequestCoreAsync(string repositoryIdentifier, GitPullRequestSpecification specification, CancellationToken cancellationToken) => throw new NotSupportedException("Not exercised by these tests."); public HostingCredential CallResolveCredential() => ResolveCredential(); diff --git a/GitIntegration.Test/Hosting/GitPullRequestCreateBuilderTests.cs b/GitIntegration.Test/Hosting/GitPullRequestCreateBuilderTests.cs index 38a6c8b..296601d 100644 --- a/GitIntegration.Test/Hosting/GitPullRequestCreateBuilderTests.cs +++ b/GitIntegration.Test/Hosting/GitPullRequestCreateBuilderTests.cs @@ -4,6 +4,7 @@ namespace ktsu.GitIntegration.Test; using System; using System.Collections.Generic; +using System.Net.Http; using System.Threading; using System.Threading.Tasks; @@ -171,26 +172,32 @@ public void RejectsANullDescription() /// /// Records the repository and specification handed to CreatePullRequestCoreAsync, so the - /// routing test can assert on what actually passed + /// routing test can assert on what actually passed /// through, not merely that it was called. /// private sealed class RecordingProvider : GitProvider { - public GitRepositoryName? Repository { get; private set; } + public string? Repository { get; private set; } public GitPullRequestSpecification? Specification { get; private set; } public override GitProviderName Name => "RecordingProvider".As(); + // Never reached: these tests either inject a Handler or never issue a request at all. The + // member is abstract so that each real provider has to name its own shared transport rather + // than inherit one, which is the point of it existing. + private protected override HttpMessageHandler DefaultHandler => + throw new NotSupportedException("Not exercised by these tests."); + public override Task> GetRepositoriesAsync(CancellationToken cancellationToken = default) => throw new NotSupportedException("Not exercised by the routing test."); - public override Task> GetPullRequestsAsync(GitRepositoryName repositoryName, CancellationToken cancellationToken = default) => + internal override Task> GetPullRequestsCoreAsync(string repositoryIdentifier, CancellationToken cancellationToken) => throw new NotSupportedException("Not exercised by the routing test."); - internal override Task CreatePullRequestCoreAsync(GitRepositoryName repositoryName, GitPullRequestSpecification specification, CancellationToken cancellationToken) + internal override Task CreatePullRequestCoreAsync(string repositoryIdentifier, GitPullRequestSpecification specification, CancellationToken cancellationToken) { - Repository = repositoryName; + Repository = repositoryIdentifier; Specification = specification; return Task.FromResult(CannedPullRequest); } diff --git a/GitIntegration.Test/Integration/GitRemoteSyncTests.cs b/GitIntegration.Test/Integration/GitRemoteSyncTests.cs index 248f3c2..e7ec0be 100644 --- a/GitIntegration.Test/Integration/GitRemoteSyncTests.cs +++ b/GitIntegration.Test/Integration/GitRemoteSyncTests.cs @@ -40,6 +40,11 @@ private static async Task CreateBareRemoteAsync( Assert.IsFalse(init.AlreadyExisted); + // LocalPath is nullable on GitRepository because a repository a hosting provider enumerated + // has none. One that Init produced always does — it is the path Init was given — so this + // asserts rather than propagating the nullability into every caller of this helper. + Assert.IsNotNull(init.Repository.LocalPath); + return init.Repository.LocalPath; } @@ -85,6 +90,142 @@ private static async Task CommitFileAsync( .ExecuteAsync(cancellationToken).ConfigureAwait(false); } + [TestMethod] + public async Task ReportsOnlyTheCommitsNoRemoteHasAsync() + { + // The query this pair of options exists for, run against a real bare remote so the answer is + // git's rather than this library's idea of it. The argument-order tests pin the vector; this + // pins that the vector means what it is meant to mean — and it is the test that would fail if + // the closing --not were dropped, since the revision would then fall inside the negation. + CancellationToken cancellationToken = TestContext.CancellationTokenSource.Token; + await IntegrationGitFixture.RequireGitAsync(cancellationToken).ConfigureAwait(false); + + using TemporaryRepository remoteDirectory = new(); + using TemporaryRepository workingDirectory = new(); + + AbsoluteDirectoryPath remote = await CreateBareRemoteAsync(remoteDirectory, cancellationToken).ConfigureAwait(false); + GitRepository repository = await CreateWorkingCopyAsync(workingDirectory, remote, cancellationToken).ConfigureAwait(false); + + _ = await CommitFileAsync(repository, workingDirectory, "a.txt", "one\n", "pushed", cancellationToken).ConfigureAwait(false); + + _ = await repository.Push() + .ToRemote(Origin) + .WithBranch(Main) + .SettingUpstream() + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + + // Everything is on the remote, so nothing is at stake yet. + IReadOnlyList beforeLocalWork = await repository.Log() + .IncludingAllRefs() + .ExcludingRemoteTrackingRefs() + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + Assert.AreEqual(0, beforeLocalWork.Count); + + GitCommit unpushed = await CommitFileAsync( + repository, workingDirectory, "b.txt", "two\n", "unpushed", cancellationToken).ConfigureAwait(false); + + IReadOnlyList afterLocalWork = await repository.Log() + .IncludingAllRefs() + .ExcludingRemoteTrackingRefs() + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + + // Exactly the one commit the remote has never seen — not the whole history, and not nothing. + Assert.AreEqual(1, afterLocalWork.Count); + Assert.AreEqual(unpushed.Sha, afterLocalWork[0].Sha); + } + + [TestMethod] + public async Task NarrowsTheUnpushedQueryToARevisionWithoutNegatingItAsync() + { + // Combining ExcludingRemoteTrackingRefs with ForRevision is the case the closing --not + // protects. Without it git would read "--not --remotes " as asking for commits in + // neither, and answer zero — which looks exactly like a correct "nothing unpushed". + CancellationToken cancellationToken = TestContext.CancellationTokenSource.Token; + await IntegrationGitFixture.RequireGitAsync(cancellationToken).ConfigureAwait(false); + + using TemporaryRepository remoteDirectory = new(); + using TemporaryRepository workingDirectory = new(); + + AbsoluteDirectoryPath remote = await CreateBareRemoteAsync(remoteDirectory, cancellationToken).ConfigureAwait(false); + GitRepository repository = await CreateWorkingCopyAsync(workingDirectory, remote, cancellationToken).ConfigureAwait(false); + + _ = await CommitFileAsync(repository, workingDirectory, "a.txt", "one\n", "pushed", cancellationToken).ConfigureAwait(false); + + _ = await repository.Push() + .ToRemote(Origin) + .WithBranch(Main) + .SettingUpstream() + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + + GitCommit unpushed = await CommitFileAsync( + repository, workingDirectory, "b.txt", "two\n", "unpushed", cancellationToken).ConfigureAwait(false); + + IReadOnlyList commits = await repository.Log() + .ExcludingRemoteTrackingRefs() + .ForRevision("HEAD".As()) + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + + Assert.AreEqual(1, commits.Count); + Assert.AreEqual(unpushed.Sha, commits[0].Sha); + } + + [TestMethod] + public async Task CountsCommitsAndDivergenceAgainstARealRemoteAsync() + { + // Runs both rev-list builders against a real repository, so the ahead/behind mapping is + // checked against git's actual output rather than against a scripted string. Divergence is + // also cross-checked against GitStatus, which answers the same question by a different route + // — if the two disagree, one of them is reading git backwards. + CancellationToken cancellationToken = TestContext.CancellationTokenSource.Token; + await IntegrationGitFixture.RequireGitAsync(cancellationToken).ConfigureAwait(false); + + using TemporaryRepository remoteDirectory = new(); + using TemporaryRepository workingDirectory = new(); + + AbsoluteDirectoryPath remote = await CreateBareRemoteAsync(remoteDirectory, cancellationToken).ConfigureAwait(false); + GitRepository repository = await CreateWorkingCopyAsync(workingDirectory, remote, cancellationToken).ConfigureAwait(false); + + _ = await CommitFileAsync(repository, workingDirectory, "a.txt", "one\n", "c1", cancellationToken).ConfigureAwait(false); + + _ = await repository.Push() + .ToRemote(Origin) + .WithBranch(Main) + .SettingUpstream() + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + + _ = await CommitFileAsync(repository, workingDirectory, "b.txt", "two\n", "c2", cancellationToken).ConfigureAwait(false); + _ = await CommitFileAsync(repository, workingDirectory, "c.txt", "three\n", "c3", cancellationToken).ConfigureAwait(false); + + int total = await repository.RevList("HEAD".As()) + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + Assert.AreEqual(3, total); + + // A range counts only what the right side has and the left does not. + int unpushed = await repository.RevList("origin/main..HEAD".As()) + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + Assert.AreEqual(2, unpushed); + + // A pathspec narrows the count to commits touching that path. + int touchingB = await repository.RevList("HEAD".As()) + .ForPath("b.txt".As()) + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + Assert.AreEqual(1, touchingB); + + GitDivergence divergence = await repository + .Divergence("origin/main".As(), "HEAD".As()) + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + + Assert.AreEqual(2, divergence.Ahead); + Assert.AreEqual(0, divergence.Behind); + Assert.IsFalse(divergence.IsInSync); + + // The same numbers GitStatus reports for HEAD against its configured upstream. Reversing the + // left/right mapping would make these disagree. + GitStatus status = await repository.Status().ExecuteAsync(cancellationToken).ConfigureAwait(false); + Assert.AreEqual(status.Ahead, divergence.Ahead); + Assert.AreEqual(status.Behind, divergence.Behind); + } + [TestMethod] public async Task PushCreatesTheBranchOnTheRemoteAsync() { diff --git a/GitIntegration.Test/Integration/GitRoundTripTests.cs b/GitIntegration.Test/Integration/GitRoundTripTests.cs index faf3b13..9180e99 100644 --- a/GitIntegration.Test/Integration/GitRoundTripTests.cs +++ b/GitIntegration.Test/Integration/GitRoundTripTests.cs @@ -3,6 +3,7 @@ namespace ktsu.GitIntegration.Test; using System.Collections.Generic; +using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -185,6 +186,193 @@ public async Task BranchCreateCheckoutAndDeleteRoundTripAsync() Assert.AreEqual(1, afterDelete.Count); } + [TestMethod] + public async Task TagCreateListAndDeleteRoundTripAsync() + { + // Runs both kinds of tag against a real git, which is the only way to confirm the format + // string in GitOutputFormats and the parser reading it agree with what git actually emits — + // including that %(contents:subject) falls through to the commit's own subject for a + // lightweight tag, which the parser has to suppress. + CancellationToken cancellationToken = TestContext.CancellationTokenSource.Token; + await IntegrationGitFixture.RequireGitAsync(cancellationToken).ConfigureAwait(false); + + using TemporaryRepository temporary = new(); + GitRepository repository = await InitialiseAsync(temporary, cancellationToken).ConfigureAwait(false); + + temporary.WriteFile("a.txt", "one\n"); + _ = await repository.Add().All().ExecuteAsync(cancellationToken).ConfigureAwait(false); + GitCommit commit = await repository.Commit("c1".As()) + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + + GitTagName lightweight = "v0.1.0".As(); + GitTagName annotated = "v0.2.0".As(); + + _ = await repository.CreateTag(lightweight).ExecuteAsync(cancellationToken).ConfigureAwait(false); + _ = await repository.CreateTag(annotated) + .Annotating("release 0.2.0".As()) + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + + IReadOnlyList tags = await repository.Tags().ExecuteAsync(cancellationToken).ConfigureAwait(false); + Assert.AreEqual(2, tags.Count); + + GitTag light = tags.Single(tag => tag.Name == lightweight); + Assert.IsFalse(light.IsAnnotated); + Assert.AreEqual(commit.Sha, light.Sha); + + // The reference points straight at the commit, so there is no separate tag object. + Assert.AreEqual(light.Sha, light.ObjectSha); + + // The commit's subject is "c1"; a lightweight tag must report no message rather than that. + Assert.IsNull(light.Message); + + GitTag annotatedTag = tags.Single(tag => tag.Name == annotated); + Assert.IsTrue(annotatedTag.IsAnnotated); + Assert.AreEqual("release 0.2.0", annotatedTag.Message); + + // Sha dereferences to the commit while ObjectSha is the tag object git wrote, so the two + // differ — the distinction the model exists to carry. + Assert.AreEqual(commit.Sha, annotatedTag.Sha); + Assert.AreNotEqual(annotatedTag.Sha, annotatedTag.ObjectSha); + + _ = await repository.DeleteTag(lightweight).ExecuteAsync(cancellationToken).ConfigureAwait(false); + + IReadOnlyList afterDelete = + await repository.Tags().ExecuteAsync(cancellationToken).ConfigureAwait(false); + Assert.AreEqual(1, afterDelete.Count); + Assert.AreEqual(annotated, afterDelete[0].Name); + } + + [TestMethod] + public async Task CheckoutResolvesATagRatherThanAFileOfTheSameNameAsync() + { + // Checkout emits a trailing "--" rather than a leading --end-of-options, and this is the + // behaviour that choice buys beyond compatibility with git <= 2.43: the operand is read as a + // revision, so a tag whose name also matches a path on disk resolves to the tag instead of + // silently restoring the file. + CancellationToken cancellationToken = TestContext.CancellationTokenSource.Token; + await IntegrationGitFixture.RequireGitAsync(cancellationToken).ConfigureAwait(false); + + using TemporaryRepository temporary = new(); + GitRepository repository = await InitialiseAsync(temporary, cancellationToken).ConfigureAwait(false); + + temporary.WriteFile("a.txt", "one\n"); + _ = await repository.Add().All().ExecuteAsync(cancellationToken).ConfigureAwait(false); + _ = await repository.Commit("c1".As()).ExecuteAsync(cancellationToken).ConfigureAwait(false); + + // A committed file and a tag sharing one name is exactly the ambiguity git warns about. + GitTagName ambiguous = "ambiguous".As(); + temporary.WriteFile("ambiguous", "file contents\n"); + _ = await repository.Add().All().ExecuteAsync(cancellationToken).ConfigureAwait(false); + GitCommit second = await repository.Commit("c2".As()) + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + + _ = await repository.CreateTag(ambiguous).ExecuteAsync(cancellationToken).ConfigureAwait(false); + + _ = await repository.Checkout("ambiguous".As()) + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + + // Resolving the tag detaches HEAD at its commit. Resolving the path instead would have left + // HEAD on the branch and merely restored the file. + GitCommitSha head = await repository.RevParse("HEAD".As()) + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + Assert.AreEqual(second.Sha, head); + + GitStatus status = await repository.Status().ExecuteAsync(cancellationToken).ConfigureAwait(false); + Assert.IsTrue(status.IsDetached); + } + + [TestMethod] + public async Task DiffReportsLineCountsFromARealRepositoryAsync() + { + // The parser tests pin the shape of git's output; this pins that git really does emit that + // shape when asked, including the two sections running together with no delimiter and the + // dash a binary file gets instead of a count. + CancellationToken cancellationToken = TestContext.CancellationTokenSource.Token; + await IntegrationGitFixture.RequireGitAsync(cancellationToken).ConfigureAwait(false); + + using TemporaryRepository temporary = new(); + GitRepository repository = await InitialiseAsync(temporary, cancellationToken).ConfigureAwait(false); + + // A NUL byte is what makes git classify a file as binary, and a binary file is the case the + // nullable counts exist for. + temporary.WriteFile("text.txt", "one\ntwo\nthree\n"); + temporary.WriteFile("binary.bin", "\0binary\n"); + temporary.WriteFile("gone.txt", "removed\n"); + _ = await repository.Add().All().ExecuteAsync(cancellationToken).ConfigureAwait(false); + _ = await repository.Commit("c1".As()).ExecuteAsync(cancellationToken).ConfigureAwait(false); + + temporary.WriteFile("text.txt", "one\ntwo\nthree\nfour\n"); + temporary.WriteFile("binary.bin", "\0changed\n"); + temporary.DeleteFile("gone.txt"); + _ = await repository.Add().All().ExecuteAsync(cancellationToken).ConfigureAwait(false); + _ = await repository.Commit("c2".As()).ExecuteAsync(cancellationToken).ConfigureAwait(false); + + IReadOnlyList entries = await repository.Diff() + .Between("HEAD~1".As(), "HEAD".As()) + .WithLineCounts() + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + + Assert.AreEqual(3, entries.Count); + + GitDiffEntry text = entries.Single(entry => entry.Path.WeakString == "text.txt"); + Assert.AreEqual(1, text.Insertions); + Assert.AreEqual(0, text.Deletions); + + GitDiffEntry removed = entries.Single(entry => entry.Path.WeakString == "gone.txt"); + Assert.AreEqual(GitChangeKind.Deleted, removed.Kind); + Assert.AreEqual(0, removed.Insertions); + Assert.AreEqual(1, removed.Deletions); + + // git prints "-" for both counts on a binary file rather than measuring it, which is why the + // counts are nullable: a binary change is not a zero-line change. + GitDiffEntry binary = entries.Single(entry => entry.Path.WeakString == "binary.bin"); + Assert.IsNull(binary.Insertions); + Assert.IsNull(binary.Deletions); + + // Without the option the same diff reports the same paths and no counts at all. + IReadOnlyList withoutCounts = await repository.Diff() + .Between("HEAD~1".As(), "HEAD".As()) + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + + Assert.AreEqual(entries.Count, withoutCounts.Count); + Assert.IsTrue(withoutCounts.All(entry => entry.Insertions is null && entry.Deletions is null)); + } + + [TestMethod] + public async Task DiffReportsLineCountsForARenameAsync() + { + // A rename is spelled differently in each section — two path tokens after the status in the + // raw section, an empty path field then two tokens in the numstat one — so it is the case + // that would break a parser correlating the sections by path rather than by position. + CancellationToken cancellationToken = TestContext.CancellationTokenSource.Token; + await IntegrationGitFixture.RequireGitAsync(cancellationToken).ConfigureAwait(false); + + using TemporaryRepository temporary = new(); + GitRepository repository = await InitialiseAsync(temporary, cancellationToken).ConfigureAwait(false); + + temporary.WriteFile("before.txt", "one\ntwo\nthree\nfour\nfive\n"); + _ = await repository.Add().All().ExecuteAsync(cancellationToken).ConfigureAwait(false); + _ = await repository.Commit("c1".As()).ExecuteAsync(cancellationToken).ConfigureAwait(false); + + temporary.DeleteFile("before.txt"); + temporary.WriteFile("after.txt", "one\ntwo\nthree\nfour\nfive\nsix\n"); + _ = await repository.Add().All().ExecuteAsync(cancellationToken).ConfigureAwait(false); + _ = await repository.Commit("c2".As()).ExecuteAsync(cancellationToken).ConfigureAwait(false); + + IReadOnlyList entries = await repository.Diff() + .Between("HEAD~1".As(), "HEAD".As()) + .DetectRenames() + .WithLineCounts() + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + + GitDiffEntry rename = entries.Single(entry => entry.Kind == GitChangeKind.Renamed); + + Assert.AreEqual("after.txt", rename.Path.WeakString); + Assert.AreEqual("before.txt", rename.OriginalPath?.WeakString); + Assert.AreEqual(1, rename.Insertions); + Assert.AreEqual(0, rename.Deletions); + } + [TestMethod] public async Task RemoteAddSetUrlAndRemoveRoundTripAsync() { diff --git a/GitIntegration.Test/Integration/GitSubmoduleTests.cs b/GitIntegration.Test/Integration/GitSubmoduleTests.cs new file mode 100644 index 0000000..fb803a5 --- /dev/null +++ b/GitIntegration.Test/Integration/GitSubmoduleTests.cs @@ -0,0 +1,246 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitIntegration.Test; + +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; + +using ktsu.Semantics.Paths; +using ktsu.Semantics.Strings; + +/// +/// Exercises the submodule verbs against a real git binary and real submodules. +/// +/// +/// The unit tests pin this library's reading of captured output; only a real repository can confirm +/// git actually emits that output, and that ls-files and submodule status agree the +/// way the parser assumes. Both commands are read here in every state a submodule can be in. +/// +/// Every git invocation that reaches a submodule's remote runs with +/// protocol.file.allow=always, because these tests use a local directory as the submodule's +/// remote and git has refused the file:// transport for submodules by default since the +/// CVE-2022-39253 fix. That is a property of the fixture, not of this library. +/// +/// +[TestClass] +[TestCategory("Integration")] +public class GitSubmoduleTests +{ + private static readonly GitAuthorName AuthorName = "Fixture Author".As(); + private static readonly GitAuthorEmail AuthorEmail = "fixture@example.com".As(); + private static readonly GitBranchName Main = "main".As(); + + /// Creates a repository with one commit, to stand in for a submodule's remote. + private static async Task CreateRepositoryAsync( + TemporaryRepository temporary, + string fileName, + CancellationToken cancellationToken) + { + GitInitResult init = await IntegrationGitFixture.CreateClient() + .Init(temporary.Root) + .WithInitialBranch(Main) + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + + GitRepository repository = init.Repository; + + await IntegrationGitFixture.ConfigureIdentityAsync(repository, AuthorName, AuthorEmail, cancellationToken) + .ConfigureAwait(false); + + temporary.WriteFile(fileName, "one\n"); + _ = await repository.Add().All().ExecuteAsync(cancellationToken).ConfigureAwait(false); + _ = await repository.Commit("c1".As()).ExecuteAsync(cancellationToken).ConfigureAwait(false); + + return repository; + } + + /// + /// Runs git submodule add, which this library does not wrap. + /// + /// + /// Adding a submodule is out of scope for the library — the issues that brought submodules in + /// asked for listing, updating, and the recursion flags, not for creation. The fixture therefore + /// sets one up through the runner directly, so the verbs under test have something real to read. + /// + private static async Task AddSubmoduleAsync( + GitRepository superproject, + AbsoluteDirectoryPath submoduleRemote, + string path, + CancellationToken cancellationToken) + { + GitProcessResult result = await superproject.ProcessRunner!.RunAsync( + new GitProcessRequest + { + Arguments = + [ + "-C", superproject.LocalPath!.WeakString, + "-c", "protocol.file.allow=always", + "submodule", "add", "--", submoduleRemote.WeakString, path, + ], + }, + cancellationToken).ConfigureAwait(false); + + Assert.IsTrue(result.Success, $"submodule add failed: {result.StandardError}"); + + _ = await superproject.Add().All().ExecuteAsync(cancellationToken).ConfigureAwait(false); + _ = await superproject.Commit("add submodule".As()) + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + } + + [TestMethod] + public async Task ReportsASynchronisedSubmoduleAsync() + { + CancellationToken cancellationToken = TestContext.CancellationTokenSource.Token; + await IntegrationGitFixture.RequireGitAsync(cancellationToken).ConfigureAwait(false); + + using TemporaryRepository subDirectory = new(); + using TemporaryRepository superDirectory = new(); + + GitRepository sub = await CreateRepositoryAsync(subDirectory, "s.txt", cancellationToken).ConfigureAwait(false); + GitRepository super = await CreateRepositoryAsync(superDirectory, "m.txt", cancellationToken).ConfigureAwait(false); + + await AddSubmoduleAsync(super, sub.LocalPath!, "libs/sub", cancellationToken).ConfigureAwait(false); + + IReadOnlyList submodules = + await super.Submodules().ExecuteAsync(cancellationToken).ConfigureAwait(false); + + Assert.AreEqual(1, submodules.Count); + Assert.AreEqual("libs/sub".As(), submodules[0].Path); + + // The leading-space marker. This is the assertion that fails if the status output is ever + // trimmed before it reaches the parser, since the marker is itself a space. + Assert.AreEqual(GitSubmoduleState.InSync, submodules[0].State); + Assert.AreEqual(submodules[0].Sha, submodules[0].CheckedOutSha); + Assert.IsNotNull(submodules[0].Describe); + } + + [TestMethod] + public async Task ReportsTheGitlinkAndTheCheckoutSeparatelyWhenTheyDivergeAsync() + { + CancellationToken cancellationToken = TestContext.CancellationTokenSource.Token; + await IntegrationGitFixture.RequireGitAsync(cancellationToken).ConfigureAwait(false); + + using TemporaryRepository subDirectory = new(); + using TemporaryRepository superDirectory = new(); + + GitRepository sub = await CreateRepositoryAsync(subDirectory, "s.txt", cancellationToken).ConfigureAwait(false); + GitRepository super = await CreateRepositoryAsync(superDirectory, "m.txt", cancellationToken).ConfigureAwait(false); + + await AddSubmoduleAsync(super, sub.LocalPath!, "libs/sub", cancellationToken).ConfigureAwait(false); + + IReadOnlyList before = + await super.Submodules().ExecuteAsync(cancellationToken).ConfigureAwait(false); + + // Commit inside the submodule's own working directory. The superproject's gitlink does not + // move, so the two ids diverge — which is the whole reason the model carries both. + GitRepository checkout = new() + { + // Path.Join rather than Path.Combine: the intent is "append these segments to this base", + // and Combine resets to a later segment if one is ever rooted. + LocalPath = System.IO.Path.Join(superDirectory.RootPath, "libs", "sub").As(), + ProcessRunner = super.ProcessRunner, + }; + + await IntegrationGitFixture.ConfigureIdentityAsync(checkout, AuthorName, AuthorEmail, cancellationToken) + .ConfigureAwait(false); + + // A single relative literal: WriteFile combines it under the fixture root itself, and git and + // both platforms accept a forward slash here. + superDirectory.WriteFile("libs/sub/s.txt", "two\n"); + _ = await checkout.Add().All().ExecuteAsync(cancellationToken).ConfigureAwait(false); + GitCommit moved = await checkout.Commit("c2".As()) + .ExecuteAsync(cancellationToken).ConfigureAwait(false); + + IReadOnlyList after = + await super.Submodules().ExecuteAsync(cancellationToken).ConfigureAwait(false); + + Assert.AreEqual(GitSubmoduleState.DifferentCommit, after[0].State); + + // The gitlink is unchanged — the superproject still records what it always did. + Assert.AreEqual(before[0].Sha, after[0].Sha); + + // The checkout is the new commit, and it is not the gitlink. + Assert.AreEqual(moved.Sha, after[0].CheckedOutSha); + Assert.AreNotEqual(after[0].Sha, after[0].CheckedOutSha); + } + + [TestMethod] + public async Task ReportsAnUninitialisedSubmoduleAndThenUpdatesItAsync() + { + CancellationToken cancellationToken = TestContext.CancellationTokenSource.Token; + await IntegrationGitFixture.RequireGitAsync(cancellationToken).ConfigureAwait(false); + + using TemporaryRepository subDirectory = new(); + using TemporaryRepository superDirectory = new(); + using TemporaryRepository cloneDirectory = new(); + + GitRepository sub = await CreateRepositoryAsync(subDirectory, "s.txt", cancellationToken).ConfigureAwait(false); + GitRepository super = await CreateRepositoryAsync(superDirectory, "m.txt", cancellationToken).ConfigureAwait(false); + + await AddSubmoduleAsync(super, sub.LocalPath!, "libs/sub", cancellationToken).ConfigureAwait(false); + + // A plain clone leaves the submodule registered but not checked out, which is the state the + // "-" marker names and the one a caller most needs to be able to detect. + GitProcessResult cloned = await super.ProcessRunner!.RunAsync( + new GitProcessRequest + { + Arguments = + [ + "-c", "protocol.file.allow=always", + "clone", "--", superDirectory.RootPath, cloneDirectory.RootPath, + ], + }, + cancellationToken).ConfigureAwait(false); + Assert.IsTrue(cloned.Success, $"clone failed: {cloned.StandardError}"); + + GitRepository clone = await IntegrationGitFixture.CreateClient() + .OpenAsync(cloneDirectory.Root, cancellationToken).ConfigureAwait(false); + + IReadOnlyList before = + await clone.Submodules().ExecuteAsync(cancellationToken).ConfigureAwait(false); + + Assert.AreEqual(1, before.Count); + Assert.AreEqual(GitSubmoduleState.Uninitialised, before[0].State); + + // Nothing is checked out, so nothing is reported — even though git prints the recorded + // gitlink again on that line, which would otherwise make this look synchronised. + Assert.IsNull(before[0].CheckedOutSha); + Assert.IsNull(before[0].Describe); + + GitProcessResult updated = await clone.ProcessRunner!.RunAsync( + new GitProcessRequest + { + Arguments = + [ + "-C", cloneDirectory.RootPath, + "-c", "protocol.file.allow=always", + .. clone.UpdateSubmodules().Initialise().Recursive().BuildArguments(), + ], + }, + cancellationToken).ConfigureAwait(false); + Assert.IsTrue(updated.Success, $"submodule update failed: {updated.StandardError}"); + + IReadOnlyList after = + await clone.Submodules().ExecuteAsync(cancellationToken).ConfigureAwait(false); + + Assert.AreEqual(GitSubmoduleState.InSync, after[0].State); + Assert.AreEqual(after[0].Sha, after[0].CheckedOutSha); + } + + [TestMethod] + public async Task ReportsNoSubmodulesForARepositoryWithNoneAsync() + { + CancellationToken cancellationToken = TestContext.CancellationTokenSource.Token; + await IntegrationGitFixture.RequireGitAsync(cancellationToken).ConfigureAwait(false); + + using TemporaryRepository temporary = new(); + GitRepository repository = await CreateRepositoryAsync(temporary, "m.txt", cancellationToken).ConfigureAwait(false); + + IReadOnlyList submodules = + await repository.Submodules().ExecuteAsync(cancellationToken).ConfigureAwait(false); + + Assert.AreEqual(0, submodules.Count); + } + + public TestContext TestContext { get; set; } = null!; +} diff --git a/GitIntegration.Test/Integration/TemporaryRepository.cs b/GitIntegration.Test/Integration/TemporaryRepository.cs index d335ca0..99fcbd4 100644 --- a/GitIntegration.Test/Integration/TemporaryRepository.cs +++ b/GitIntegration.Test/Integration/TemporaryRepository.cs @@ -31,7 +31,7 @@ public TemporaryRepository() /// What to write. public void WriteFile(string relativePath, string contents) { - string full = Path.Combine(RootPath, relativePath); + string full = CombineUnderRoot(relativePath); string? directory = Path.GetDirectoryName(full); if (!string.IsNullOrEmpty(directory)) @@ -42,6 +42,52 @@ public void WriteFile(string relativePath, string contents) File.WriteAllText(full, contents); } + /// Deletes a file inside the repository. + /// + /// Deleting through the filesystem rather than through git rm, so the change reaches the + /// index only via the Add().All() the tests already run — which is what exercises the + /// deletion path of the verb under test rather than a second git command's. + /// + /// The path relative to the repository root. + public void DeleteFile(string relativePath) => File.Delete(CombineUnderRoot(relativePath)); + + /// + /// Joins a relative path onto , refusing a rooted one. + /// + /// + /// + /// silently discards every earlier argument when a + /// later one is rooted, so combining with /etc or C:\Windows + /// yields that path rather than one inside the throwaway repository. For + /// that would write outside the fixture; for it would delete + /// outside it. Rejecting a rooted path is the same guard this library used to need in + /// GitProvider for a repository name a host reported, and it is worth having here for the + /// same reason: the failure is silent and the blast radius is a real filesystem. + /// + /// + /// Two defences rather than one, and the second is what makes the first unnecessary to trust. + /// is the API that means "append these segments" — it has + /// no discarding behaviour to guard against at all, so the escape is impossible by construction + /// rather than merely checked for. The explicit rejection is kept above it because a rooted path + /// reaching here is a mistake in the calling test worth naming, and silently nesting it under the + /// root would hide that. + /// + /// + /// The path relative to the repository root. + /// The joined path, guaranteed to be under . + /// is rooted. + private string CombineUnderRoot(string relativePath) + { + if (Path.IsPathRooted(relativePath)) + { + throw new ArgumentException( + $"'{relativePath}' is rooted, so joining it would escape the temporary repository.", + nameof(relativePath)); + } + + return Path.Join(RootPath, relativePath); + } + public void Dispose() { try diff --git a/GitIntegration.Test/Parsing/GitDiffParserTests.cs b/GitIntegration.Test/Parsing/GitDiffParserTests.cs index 2b851cc..6fe83df 100644 --- a/GitIntegration.Test/Parsing/GitDiffParserTests.cs +++ b/GitIntegration.Test/Parsing/GitDiffParserTests.cs @@ -121,4 +121,124 @@ public void RejectsARenameWithNoDestinationPath() { Assert.ThrowsExactly(() => GitDiffParser.Parse("R100" + Nul + "only-source.txt")); } + + [TestMethod] + public void ParsesTheRawAndNumstatSectionsRunTogether() + { + // Captured verbatim from git 2.43. The two sections are emitted back to back with no + // delimiter: the raw section's last path token ("n.txt") runs straight into the numstat + // section's first record ("-\t-\tb.bin"). Telling them apart is the whole job of this parser. + string output = + ":100644 100644 366fd40 fbeb5f4 M\0b.bin\0" + + ":100644 000000 abaddc0 0000000 D\0d.txt\0" + + ":100644 100644 de98044 d68dd40 R075\0f.txt\0g.txt\0" + + ":000000 100644 0000000 8ba3a16 A\0n.txt\0" + + "-\t-\tb.bin\0" + + "0\t1\td.txt\0" + + "1\t0\t\0f.txt\0g.txt\0" + + "1\t0\tn.txt\0"; + + IReadOnlyList entries = GitDiffParser.ParseWithLineCounts(output); + + Assert.AreEqual(4, entries.Count); + + // A binary file: git prints "-" for both counts rather than measuring it, so the counts stay + // null. Reporting 0 would state that nothing changed in a file git declined to measure. + Assert.AreEqual(GitChangeKind.Modified, entries[0].Kind); + Assert.AreEqual("b.bin", entries[0].Path.WeakString); + Assert.IsNull(entries[0].Insertions); + Assert.IsNull(entries[0].Deletions); + + Assert.AreEqual(GitChangeKind.Deleted, entries[1].Kind); + Assert.AreEqual(0, entries[1].Insertions); + Assert.AreEqual(1, entries[1].Deletions); + + // A rename is spelled differently in each section — two path tokens after the status in the + // raw section, an empty path field then two tokens in the numstat one — which is why + // correlation is positional rather than by path. + Assert.AreEqual(GitChangeKind.Renamed, entries[2].Kind); + Assert.AreEqual("g.txt", entries[2].Path.WeakString); + Assert.AreEqual("f.txt", entries[2].OriginalPath?.WeakString); + Assert.AreEqual(75, entries[2].SimilarityPercent); + Assert.AreEqual(1, entries[2].Insertions); + Assert.AreEqual(0, entries[2].Deletions); + + Assert.AreEqual(GitChangeKind.Added, entries[3].Kind); + Assert.AreEqual(1, entries[3].Insertions); + Assert.AreEqual(0, entries[3].Deletions); + } + + [TestMethod] + public void ReadsTheStatusFromTheEndOfARawHeader() + { + // The status is the last space-separated field of ": + // ". Taken from the end rather than by field index, so a future git adding a field + // ahead of it would not shift what is read. + string output = ":100644 100755 8ba3a16 8ba3a16 M\0n.txt\0" + "0\t0\tn.txt\0"; + + IReadOnlyList entries = GitDiffParser.ParseWithLineCounts(output); + + Assert.AreEqual(1, entries.Count); + Assert.AreEqual(GitChangeKind.Modified, entries[0].Kind); + + // A mode-only change really is zero lines either way, which is a different statement from a + // binary file's null. + Assert.AreEqual(0, entries[0].Insertions); + Assert.AreEqual(0, entries[0].Deletions); + } + + [TestMethod] + public void ReadsAPathBeginningWithAColonWithoutMistakingItForARawHeader() + { + // The section test looks at a record's first token only, and a path is consumed by the record + // that owns it, so a file named ":weird" never reaches that check. POSIX permits such a name. + string output = ":000000 100644 0000000 8ba3a16 A\0:weird\0" + "2\t0\t:weird\0"; + + IReadOnlyList entries = GitDiffParser.ParseWithLineCounts(output); + + Assert.AreEqual(1, entries.Count); + Assert.AreEqual(":weird", entries[0].Path.WeakString); + Assert.AreEqual(2, entries[0].Insertions); + } + + [TestMethod] + public void ReportsAnEmptyListForAnEmptyDiff() + { + Assert.AreEqual(0, GitDiffParser.ParseWithLineCounts(string.Empty).Count); + } + + [TestMethod] + public void ThrowsWhenTheTwoSectionsDisagreeOnHowManyChangesThereAre() + { + // One numstat record per raw record is what git emits, so a mismatch means the section + // boundary was read wrongly rather than that a count is genuinely missing. Reporting nulls + // would present a parser bug as an absent measurement. + string output = + ":100644 000000 abaddc0 0000000 D\0d.txt\0" + + ":000000 100644 0000000 8ba3a16 A\0n.txt\0" + + "0\t1\td.txt\0"; + + _ = Assert.ThrowsExactly(() => _ = GitDiffParser.ParseWithLineCounts(output)); + } + + [TestMethod] + public void ThrowsForAMalformedRawHeader() + { + _ = Assert.ThrowsExactly( + () => _ = GitDiffParser.ParseWithLineCounts(":nospaces\0d.txt\0" + "0\t1\td.txt\0")); + } + + [TestMethod] + public void ThrowsForAMalformedNumstatRecord() + { + _ = Assert.ThrowsExactly( + () => _ = GitDiffParser.ParseWithLineCounts(":100644 000000 abaddc0 0000000 D\0d.txt\0" + "0 1 d.txt\0")); + } + + [TestMethod] + public void ThrowsForANumstatCountThatIsNeitherANumberNorADash() + { + _ = Assert.ThrowsExactly( + () => _ = GitDiffParser.ParseWithLineCounts(":100644 000000 abaddc0 0000000 D\0d.txt\0" + "x\t1\td.txt\0")); + } } diff --git a/GitIntegration.Test/SemanticTypes/SemanticTypeTests.cs b/GitIntegration.Test/SemanticTypes/SemanticTypeTests.cs index baf4e13..2d819e0 100644 --- a/GitIntegration.Test/SemanticTypes/SemanticTypeTests.cs +++ b/GitIntegration.Test/SemanticTypes/SemanticTypeTests.cs @@ -138,4 +138,42 @@ public void SemanticTypesAreDistinctAtCompileTime() // worth checking, so check that instead. Assert.IsNotInstanceOfType(branch); } + + [TestMethod] + public void GitRepositoryNameAcceptsAnOrdinarySingleSegmentName() + { + // The denylist must not become an allowlist. GitHub restricts names to A-Z a-z 0-9 . _ -, + // Azure DevOps permits spaces and more, and both shapes have to keep working. + Assert.AreEqual("GitIntegration", GitRepositoryName.Create("GitIntegration").WeakString); + Assert.AreEqual("my.repo_v2-final", GitRepositoryName.Create("my.repo_v2-final").WeakString); + Assert.AreEqual("Team Project Repo", GitRepositoryName.Create("Team Project Repo").WeakString); + + // A relative segment only means anything when a separator delimits it, and separators are + // rejected outright, so dots inside a name are ordinary characters. + Assert.AreEqual("my..repo", GitRepositoryName.Create("my..repo").WeakString); + } + + [TestMethod] + public void GitRepositoryNameRejectsAnythingThatWouldReshapeARequestPath() + { + // Both providers substitute this value straight into a request path and escape it + // differently — Azure DevOps runs Uri.EscapeDataString over it, GitHub hands it to Octokit + // unescaped — so a separator reshapes the request on one host and not the other. Rejecting at + // construction closes both at the source. + foreach (string candidate in new[] + { + "owner/repo", + "..", + ".", + "../../etc", + "repo?query=1", + "repo#fragment", + @"owner\repo", + }) + { + _ = Assert.ThrowsExactly( + () => _ = GitRepositoryName.Create(candidate), + $"'{candidate}' should not be a valid repository name."); + } + } } diff --git a/GitIntegration/Builders/GitCheckoutBuilder.cs b/GitIntegration/Builders/GitCheckoutBuilder.cs index a615f4b..3922ca6 100644 --- a/GitIntegration/Builders/GitCheckoutBuilder.cs +++ b/GitIntegration/Builders/GitCheckoutBuilder.cs @@ -2,6 +2,7 @@ namespace ktsu.GitIntegration; +using System; using System.Collections.Generic; using ktsu.Semantics.Paths; @@ -17,6 +18,12 @@ namespace ktsu.GitIntegration; public interface IGitCheckoutBuilder : IGitCommandBuilder { /// Creates the target as a new branch at the current HEAD and switches to it. + /// + /// Cannot be combined with : git refuses -b alongside + /// --detach outright. A caller may set both in either order, so only the finished + /// configuration can detect the contradiction; BuildArguments throws + /// when both are set, before a process is even spawned. + /// /// The same builder, to allow chaining. public IGitCheckoutBuilder CreatingBranch(); @@ -27,7 +34,30 @@ public interface IGitCheckoutBuilder : IGitCommandBuilder /// The same builder, to allow chaining. public IGitCheckoutBuilder Force(); + /// + /// Also updates the submodules' working trees to match the target. + /// + /// + /// This can destroy work. Unlike the same flag on clone, fetch, and + /// pull, checkout --recurse-submodules overwrites a submodule's checked-out state: + /// a submodule sitting on a commit the target does not record is moved off it, and uncommitted + /// changes in a submodule's working tree can be lost. That is a wider blast radius than + /// , which only concerns the superproject's own working tree. + /// + /// Without this flag, checkout leaves the submodules alone entirely, so their working trees will + /// disagree with the newly checked-out superproject until something updates them — + /// UpdateSubmodules() being the explicit way to do that. + /// + /// + /// The same builder, to allow chaining. + public IGitCheckoutBuilder RecursingSubmodules(); + /// Checks the target out as a detached HEAD rather than switching to a branch. + /// + /// Cannot be combined with , for the same reason: + /// BuildArguments throws when both are set, since + /// only the finished configuration can detect the contradiction. + /// /// The same builder, to allow chaining. public IGitCheckoutBuilder Detach(); } @@ -48,6 +78,7 @@ internal sealed class GitCheckoutBuilder( private bool _creatingBranch; private bool _force; private bool _detach; + private bool _recursingSubmodules; /// public IGitCheckoutBuilder CreatingBranch() @@ -63,6 +94,13 @@ public IGitCheckoutBuilder Force() return this; } + /// + public IGitCheckoutBuilder RecursingSubmodules() + { + _recursingSubmodules = true; + return this; + } + /// public IGitCheckoutBuilder Detach() { @@ -75,6 +113,18 @@ protected override void AppendVerbArguments(ICollection arguments) { Ensure.NotNull(arguments); + // Real git rejects "-b --detach " at the command line with + // "fatal: '--detach' cannot be used with '-b/-B/--orphan'" (verified against git 2.43), so + // the contradiction would otherwise go undetected until a process was already spawned. Fetch + // and pull reject their equivalent contradictions before spawning a process, and checkout + // should be no less consistent. + if (_creatingBranch && _detach) + { + throw new InvalidOperationException( + "CreatingBranch and Detach cannot both be requested: git refuses -b alongside " + + "--detach."); + } + arguments.Add("checkout"); // -b has no long form, unlike every other flag this library emits. @@ -93,9 +143,33 @@ protected override void AppendVerbArguments(ICollection arguments) arguments.Add("--detach"); } - // Every flag must precede the marker: anything after it is an operand, so a flag emitted - // there would reach git as a ref name. - AppendOperands(arguments, _target.WeakString); + if (_recursingSubmodules) + { + arguments.Add("--recurse-submodules"); + } + + // Checkout is the one verb that cannot use AppendOperands, and the reason is a git bug rather + // than a design choice here. + // + // git <= 2.43 only strips "--end-of-options" from the argument vector when + // PARSE_OPT_KEEP_DASHDASH is unset (parse-options.c). checkout sets exactly that flag, + // because it is one of the few verbs accepting both a revision and a pathspec, so the marker + // survives into checkout's own operand list and is read as a path — "git checkout + // --end-of-options main" dies with "error: pathspec '--end-of-options' did not match any + // file(s) known to git". git 2.44 changed the condition to PARSE_OPT_KEEP_UNKNOWN_OPT, which + // checkout does not set, so the marker works from that version onward. Emitting it here would + // therefore make Checkout unusable on, among others, the stock git of Ubuntu 24.04 LTS. + // + // The trailing "--" is git's own documented disambiguator for this verb and works on every + // version. It buys a guarantee the marker did not: the operand is read as a revision and + // never as a path, so checking out a branch whose name also matches a file on disk resolves + // to the branch rather than silently restoring the file. + // + // Option injection is still prevented, by the layer that was always the primary one: + // GitRefName carries NotAnOptionAttribute, which refuses to construct a value beginning with + // a dash at all, so no dash-leading target can reach this vector in the first place. + arguments.Add(_target.WeakString); + arguments.Add("--"); } /// diff --git a/GitIntegration/Builders/GitCloneBuilder.cs b/GitIntegration/Builders/GitCloneBuilder.cs index 81242b2..f327d90 100644 --- a/GitIntegration/Builders/GitCloneBuilder.cs +++ b/GitIntegration/Builders/GitCloneBuilder.cs @@ -34,6 +34,16 @@ public interface IGitCloneBuilder : IGitCommandBuilder /// The same builder, to allow chaining. public IGitCloneBuilder Bare(); + /// Also clones and checks out the repository's submodules. + /// + /// A plain flag here, unlike on Fetch and Pull: a fresh clone has no prior state + /// for an on-demand decision to compare against, and git's own clone --recurse-submodules + /// takes no value either. Git additionally accepts an optional pathspec to clone only some + /// submodules, which can be added later without changing the vector this produces. + /// + /// The same builder, to allow chaining. + public IGitCloneBuilder RecursingSubmodules(); + /// /// Reports git's progress output as it arrives, rather than only when the clone finishes. /// @@ -67,6 +77,7 @@ internal sealed class GitCloneBuilder( private GitBranchName? _branch; private int? _depth; private bool _bare; + private bool _recursingSubmodules; /// public IGitCloneBuilder WithBranch(GitBranchName name) @@ -83,6 +94,13 @@ public IGitCloneBuilder WithDepth(int depth) return this; } + /// + public IGitCloneBuilder RecursingSubmodules() + { + _recursingSubmodules = true; + return this; + } + /// public IGitCloneBuilder Bare() { @@ -121,6 +139,11 @@ protected override void AppendVerbArguments(ICollection arguments) arguments.Add("--bare"); } + if (_recursingSubmodules) + { + arguments.Add("--recurse-submodules"); + } + // Both operands are caller-supplied. The source especially: an unvalidated remote path of // --upload-pack=... reaching git clone is arbitrary code execution, which is what // NotAnOptionAttribute and this marker together close off. diff --git a/GitIntegration/Builders/GitCommandBuilder.cs b/GitIntegration/Builders/GitCommandBuilder.cs index 971886f..e75be0b 100644 --- a/GitIntegration/Builders/GitCommandBuilder.cs +++ b/GitIntegration/Builders/GitCommandBuilder.cs @@ -143,10 +143,32 @@ public virtual async Task> TryExecuteAsync(CancellationToken { ExitCode = result.ExitCode, Arguments = result.Arguments, - StandardError = result.StandardError, + StandardError = GetDiagnostic(result), }); } + /// + /// Extracts the text that explains why an invocation failed. + /// + /// + /// Standard error by default, which is where git puts a diagnostic for most verbs. Virtual + /// because it is not where git puts one for every verb: commit announces "nothing to + /// commit" on standard output, and pull announces a conflict there too, leaving + /// standard error carrying only fetch progress or nothing at all. + /// + /// The seam exists on the base class, used by both (through + /// ) and , so that a verb which + /// relocates its diagnostic states that once and both entry points follow. Overriding one entry + /// point alone is what let the two report different text for the identical failure — the + /// throwing path reporting an empty standard error while the result-based path reported the + /// real explanation. + /// + /// + /// The failed invocation outcome. + /// The diagnostic text. + /// is . + protected virtual string GetDiagnostic(GitProcessResult result) => Ensure.NotNull(result).StandardError; + /// /// Classifies a failed invocation into an exception type. /// @@ -162,7 +184,10 @@ protected virtual GitCommandException CreateException(GitProcessResult result) { Ensure.NotNull(result); - string message = $"git exited with code {result.ExitCode}: {result.StandardError.Trim()}"; + // GetDiagnostic rather than StandardError directly, so a verb that puts its diagnostic on + // standard output reports the same text here as it does through TryExecuteAsync. + string diagnostic = GetDiagnostic(result); + string message = $"git exited with code {result.ExitCode}: {diagnostic.Trim()}"; // Git reports a missing working tree with a stable phrase and exit code 128. Surfacing it // as a distinct type lets callers distinguish "wrong directory" from "command failed". @@ -173,8 +198,8 @@ protected virtual GitCommandException CreateException(GitProcessResult result) // without a forced locale this would silently miss on a non-English machine and degrade to // a generic GitCommandException. Any alternative IGitProcessRunner implementation must // force the locale too, or accept that degradation. - return result.StandardError.Contains("not a git repository", StringComparison.OrdinalIgnoreCase) - ? new GitRepositoryNotFoundException(message, result.ExitCode, result.Arguments, result.StandardError) - : new GitCommandException(message, result.ExitCode, result.Arguments, result.StandardError); + return diagnostic.Contains("not a git repository", StringComparison.OrdinalIgnoreCase) + ? new GitRepositoryNotFoundException(message, result.ExitCode, result.Arguments, diagnostic) + : new GitCommandException(message, result.ExitCode, result.Arguments, diagnostic); } } diff --git a/GitIntegration/Builders/GitCommitBuilder.cs b/GitIntegration/Builders/GitCommitBuilder.cs index f9d9942..ae71669 100644 --- a/GitIntegration/Builders/GitCommitBuilder.cs +++ b/GitIntegration/Builders/GitCommitBuilder.cs @@ -191,17 +191,28 @@ public override async Task> TryExecuteAsync(CancellationTok { ExitCode = result.ExitCode, Arguments = result.Arguments, - // Commit is the one verb whose diagnostic lands on standard output rather than - // standard error — "nothing to commit" is git's headline finding here, and - // CreateException already special-cases it. Falling back to StandardOutput keeps that - // same text reachable on the non-throwing path, instead of handing the caller an empty - // string. - StandardError = string.IsNullOrWhiteSpace(result.StandardError) - ? result.StandardOutput - : result.StandardError, + StandardError = GetDiagnostic(result), }); } + /// + /// Falls back to standard output when git left standard error empty. + /// + /// + /// Commit is the verb whose diagnostic lands on standard output rather than standard error — + /// "nothing to commit" is git's headline finding here, and already + /// special-cases it. Overriding the base class's seam keeps that same text reachable from both + /// entry points: the fallback in builds its message from this + /// method, so a failure git explained only on standard output no longer reaches a caller of + /// as an empty string. + /// + /// The failed invocation outcome. + /// The diagnostic text. + protected override string GetDiagnostic(GitProcessResult result) => + string.IsNullOrWhiteSpace(Ensure.NotNull(result).StandardError) + ? result.StandardOutput + : result.StandardError; + /// /// Classifies a failed commit, recognising the one failure that is an ordinary program state. /// diff --git a/GitIntegration/Builders/GitDiffBuilder.cs b/GitIntegration/Builders/GitDiffBuilder.cs index 05e6d45..7cc61fe 100644 --- a/GitIntegration/Builders/GitDiffBuilder.cs +++ b/GitIntegration/Builders/GitDiffBuilder.cs @@ -48,10 +48,35 @@ public interface IGitDiffBuilder : IGitCommandBuilderThe same builder, to allow chaining. /// is . public IGitDiffBuilder ForPath(RelativeFilePath path); + + /// + /// Reports how many lines each change added and removed, in + /// and . + /// + /// + /// + /// Opt-in rather than always on. Without it the command and its output volume are unchanged for + /// the many callers that only want the path list, and nothing about the default result shifts. + /// + /// + /// Switches the command from --name-status to --raw --numstat, because the two + /// display formats a caller might expect to combine do not: asking for --name-status and + /// --numstat together makes the last one win rather than producing both. --raw is + /// the form that does combine, and it carries everything --name-status does — the change + /// letter and the similarity score — so nothing is lost and no second invocation is needed. + /// + /// + /// Counting how many files changed needs none of this: the result already holds one + /// entry per path, so Count is the file count. + /// + /// + /// The same builder, to allow chaining. + public IGitDiffBuilder WithLineCounts(); } /// -/// Builds git diff --name-status -z. +/// Builds git diff --name-status -z, or git diff --raw --numstat -z when line counts +/// were asked for. /// /// Runs the assembled command. /// The repository to scope the command to. @@ -65,6 +90,7 @@ internal sealed class GitDiffBuilder(IGitProcessRunner runner, AbsoluteDirectory private bool _staged; private bool _detectRenames; private bool _detectCopies; + private bool _withLineCounts; /// public IGitDiffBuilder Staged() @@ -108,13 +134,34 @@ public IGitDiffBuilder ForPath(RelativeFilePath path) return this; } + /// + public IGitDiffBuilder WithLineCounts() + { + _withLineCounts = true; + return this; + } + /// protected override void AppendVerbArguments(ICollection arguments) { Ensure.NotNull(arguments); arguments.Add("diff"); - arguments.Add("--name-status"); + + // --name-status and --numstat are both display formats, and git lets the last one win rather + // than emitting both. --raw is the form that combines with --numstat, and it is a superset of + // --name-status for this library's purposes: the same change letter and similarity score, + // plus modes and blob object ids this parser ignores. Verified against git 2.43 and 2.55. + if (_withLineCounts) + { + arguments.Add("--raw"); + arguments.Add("--numstat"); + } + else + { + arguments.Add("--name-status"); + } + arguments.Add("-z"); if (_staged) @@ -150,5 +197,7 @@ protected override void AppendVerbArguments(ICollection arguments) /// protected override IReadOnlyList ParseResult(GitProcessResult result) => - GitDiffParser.Parse(Ensure.NotNull(result).StandardOutput); + _withLineCounts + ? GitDiffParser.ParseWithLineCounts(Ensure.NotNull(result).StandardOutput) + : GitDiffParser.Parse(Ensure.NotNull(result).StandardOutput); } diff --git a/GitIntegration/Builders/GitFetchBuilder.cs b/GitIntegration/Builders/GitFetchBuilder.cs index 4fb0884..1281e2e 100644 --- a/GitIntegration/Builders/GitFetchBuilder.cs +++ b/GitIntegration/Builders/GitFetchBuilder.cs @@ -4,6 +4,7 @@ namespace ktsu.GitIntegration; using System; using System.Collections.Generic; +using System.ComponentModel; using System.Globalization; using System.Threading; using System.Threading.Tasks; @@ -50,6 +51,20 @@ public interface IGitFetchBuilder : IGitCommandBuilder /// is not positive. public IGitFetchBuilder WithDepth(int depth); + /// + /// Also fetches the objects the repository's submodules need. + /// + /// + /// Emits --recurse-submodules=<value>. Not to be confused with + /// IGitPushBuilder.CheckingSubmodules: git spells that flag with the same name but it + /// governs an unrelated question, which is why this library gives the two different method names + /// and different enums. + /// + /// Whether, and when, to recurse. + /// The same builder, to allow chaining. + /// is not a recognised value. + public IGitFetchBuilder RecursingSubmodules(GitSubmoduleRecursion recursion); + /// Reports git's progress output as it arrives. /// The sink to report to. Must be thread-safe. /// The same builder, to allow chaining. @@ -80,10 +95,43 @@ internal sealed class GitFetchBuilder(IGitProcessRunner runner, AbsoluteDirector private bool _allRemotes; private bool _prune; private bool _tags; + private GitSubmoduleRecursion? _submoduleRecursion; + + /// + /// Gets or sets a value indicating whether the installed git is new enough for + /// fetch --porcelain. + /// + /// + /// Defaults true so BuildArguments — which is pure and cannot probe — emits the modern + /// form. The execution paths set it from an actual version probe before the vector is built. + /// + private bool PorcelainSupportedByVersion { get; set; } = true; - // Defaults true so BuildArguments — which is pure and cannot probe — emits the modern form. - // The execution paths set it from an actual version probe before the vector is built. - private bool _porcelainSupported = true; + /// + /// Gets a value indicating whether this invocation can ask for, and therefore parse, the + /// porcelain account of what was fetched. + /// + /// + /// Two independent reasons it may not be, and both end in the same honest report: + /// false with an empty + /// , so an empty list is never mistaken for "nothing + /// changed". + /// + /// The first is the installed git's version: fetch --porcelain exists only from 2.41, and + /// establishes that before the vector is built. + /// + /// + /// The second is . Git refuses the two flags together outright — + /// "fatal: options '--porcelain' and '--recurse-submodules' cannot be used together", verified + /// against git 2.43 — so emitting both would turn every recursing fetch into a failure. Dropping + /// --porcelain is the only way to honour the caller's request, and it is the caller's + /// request that wins: they asked to fetch submodules, not to be itemised. This is a second reason + /// detail can be unavailable, alongside the version threshold, and it is not something the caller + /// did wrong — so it degrades rather than throwing, unlike the AllRemotes/FromRemote pair above, + /// which is a genuine contradiction between two things the caller asked for. + /// + /// + private bool IsPorcelainAvailable => PorcelainSupportedByVersion && _submoduleRecursion is null; /// public IGitFetchBuilder FromRemote(GitRemoteName name) @@ -121,6 +169,21 @@ public IGitFetchBuilder WithDepth(int depth) return this; } + /// + public IGitFetchBuilder RecursingSubmodules(GitSubmoduleRecursion recursion) + { + // Validated at the fluent call rather than deferred to ToOptionValue inside + // AppendVerbArguments, matching IGitStatusBuilder.WithUntrackedFiles: BuildArguments is + // documented as a pure computation with no exceptions of its own. + if (!Enum.IsDefined(recursion)) + { + throw new InvalidEnumArgumentException(nameof(recursion), (int)recursion, typeof(GitSubmoduleRecursion)); + } + + _submoduleRecursion = recursion; + return this; + } + /// public IGitFetchBuilder ReportingProgress(IProgress progress) { @@ -146,7 +209,7 @@ protected override void AppendVerbArguments(ICollection arguments) arguments.Add("fetch"); - if (_porcelainSupported) + if (IsPorcelainAvailable) { arguments.Add("--porcelain"); } @@ -161,6 +224,11 @@ protected override void AppendVerbArguments(ICollection arguments) arguments.Add("--prune"); } + if (_submoduleRecursion is GitSubmoduleRecursion recursion) + { + arguments.Add("--recurse-submodules=" + ToOptionValue(recursion)); + } + if (_tags) { arguments.Add("--tags"); @@ -185,7 +253,7 @@ protected override GitFetchResult ParseResult(GitProcessResult result) // Without --porcelain there is no machine-readable account to parse, and this library does // not read the human alternative. The fetch still happened; only the itemisation is absent, // which DetailAvailable records so an empty list is not mistaken for "nothing changed". - return _porcelainSupported + return IsPorcelainAvailable ? GitFetchParser.Parse(result.StandardOutput) : new GitFetchResult { Updates = [], DetailAvailable = false }; } @@ -228,6 +296,20 @@ private async Task ProbeVersionAsync(CancellationToken cancellationToken) GitResult probe = await new GitVersionBuilder(Runner) .TryExecuteAsync(cancellationToken).ConfigureAwait(false); - _porcelainSupported = probe.Success && probe.Value!.AtLeast(PorcelainMajor, PorcelainMinor); + PorcelainSupportedByVersion = probe.Success && probe.Value!.AtLeast(PorcelainMajor, PorcelainMinor); } + + /// Maps the recursion mode onto the value git's flag takes. + /// The mode to map. + /// The flag value. + private static string ToOptionValue(GitSubmoduleRecursion recursion) => recursion switch + { + GitSubmoduleRecursion.No => "no", + GitSubmoduleRecursion.Yes => "yes", + GitSubmoduleRecursion.OnDemand => "on-demand", + + // Unreachable once RecursingSubmodules validates: this arm only exists to satisfy the + // compiler's exhaustiveness check over the switch. + _ => throw new InvalidEnumArgumentException(nameof(recursion), (int)recursion, typeof(GitSubmoduleRecursion)), + }; } diff --git a/GitIntegration/Builders/GitLogBuilder.cs b/GitIntegration/Builders/GitLogBuilder.cs index d33a711..3058988 100644 --- a/GitIntegration/Builders/GitLogBuilder.cs +++ b/GitIntegration/Builders/GitLogBuilder.cs @@ -43,6 +43,44 @@ public interface IGitLogBuilder : IGitCommandBuilder> /// Follows only the first parent of a merge, hiding the merged-in history. /// The same builder, to allow chaining. public IGitLogBuilder FirstParentOnly(); + + /// + /// Lists commits reachable from every reference, not only from HEAD or from + /// . + /// + /// + /// Emits --all, which covers everything under refs/, and that breadth is + /// load-bearing rather than incidental. The narrower --branches covers only + /// refs/heads, which misses a commit on a detached HEAD — and a detached HEAD is the + /// normal state of a submodule working directory, since a submodule is checked out at + /// the commit its gitlink records rather than on a branch. Paired with + /// , a check built on --branches would therefore + /// report "nothing unpushed" for a submodule holding commits that exist nowhere else, which is + /// precisely the case where a wrong answer causes data loss. + /// + /// The same builder, to allow chaining. + public IGitLogBuilder IncludingAllRefs(); + + /// + /// Excludes every commit any remote-tracking reference already contains. + /// + /// + /// + /// Emits --not --remotes. Combined with this answers + /// "which commits in this repository does no remote have?" — the check to run before discarding a + /// working copy, to know whether doing so would destroy work that exists nowhere else. Combined + /// with instead, it narrows the same question to one revision's + /// history. + /// + /// + /// Reported as a commit list rather than a bare yes or no, because a caller that only wants the + /// yes or no reads Count > 0, while one that wants to show a user what is at stake + /// already has the commits. It composes with , , and + /// for the same reason a dedicated verb was not added. + /// + /// + /// The same builder, to allow chaining. + public IGitLogBuilder ExcludingRemoteTrackingRefs(); } /// @@ -58,6 +96,8 @@ internal sealed class GitLogBuilder(IGitProcessRunner runner, AbsoluteDirectoryP private int? _skip; private GitRefName? _revision; private bool _firstParentOnly; + private bool _includingAllRefs; + private bool _excludingRemoteTrackingRefs; /// public IGitLogBuilder Take(int maxCount) @@ -96,6 +136,20 @@ public IGitLogBuilder FirstParentOnly() return this; } + /// + public IGitLogBuilder IncludingAllRefs() + { + _includingAllRefs = true; + return this; + } + + /// + public IGitLogBuilder ExcludingRemoteTrackingRefs() + { + _excludingRemoteTrackingRefs = true; + return this; + } + /// protected override void AppendVerbArguments(ICollection arguments) { @@ -120,6 +174,36 @@ protected override void AppendVerbArguments(ICollection arguments) arguments.Add("--first-parent"); } + // --all must precede --not: git negates everything following a --not, so "--not --remotes + // --all" excludes every reference instead of including them, and reports an empty log rather + // than failing. Verified against git 2.43. + if (_includingAllRefs) + { + arguments.Add("--all"); + } + + if (_excludingRemoteTrackingRefs) + { + // The three tokens are emitted together and in this order, and the closing --not is what + // makes the pair safe rather than merely conventional. + // + // --not reverses the sense of every revision specifier that follows it, up to the next + // --not. Without the closing one, a revision from ForRevision or a pathspec from ForPath + // would fall inside the negation and be excluded instead of selected — "--not --remotes + // " asks for commits in neither, which is a different question that quietly + // returns nothing rather than failing. Closing the negation immediately scopes it to + // --remotes alone, so nothing emitted below can be captured by it, now or after a future + // option is added here. + // + // git also requires --not to precede every non-option argument, so this cannot instead be + // deferred until after the operands: "git log --end-of-options HEAD --not --remotes" dies + // with "fatal: option '--not' must come before non-option arguments". Both behaviours + // verified against git 2.43. + arguments.Add("--not"); + arguments.Add("--remotes"); + arguments.Add("--not"); + } + if (_revision is not null) { AppendOperands(arguments, _revision.WeakString); diff --git a/GitIntegration/Builders/GitPullBuilder.cs b/GitIntegration/Builders/GitPullBuilder.cs index 378ed5f..9514cda 100644 --- a/GitIntegration/Builders/GitPullBuilder.cs +++ b/GitIntegration/Builders/GitPullBuilder.cs @@ -4,8 +4,7 @@ namespace ktsu.GitIntegration; using System; using System.Collections.Generic; -using System.Threading; -using System.Threading.Tasks; +using System.ComponentModel; using ktsu.Semantics.Paths; @@ -84,6 +83,25 @@ public interface IGitPullBuilder : IGitCommandBuilder /// The same builder, to allow chaining. public IGitPullBuilder Prune(); + /// + /// Also updates the repository's submodules as part of the pull. + /// + /// + /// Emits --recurse-submodules=<value>, so one invocation moves the superproject and + /// its submodules together rather than leaving the submodules stale until something else notices. + /// + /// Overlaps with UpdateSubmodules() without replacing it: this flag updates submodules + /// that are already registered, while that verb's Initialise() also checks out a submodule + /// added upstream since this working copy was cloned. Not to be confused with + /// IGitPushBuilder.CheckingSubmodules, which git spells with the same flag name but which + /// governs an unrelated question. + /// + /// + /// Whether, and when, to recurse. + /// The same builder, to allow chaining. + /// is not a recognised value. + public IGitPullBuilder RecursingSubmodules(GitSubmoduleRecursion recursion); + /// Reports git's progress output as it arrives. /// The sink to report to. Must be thread-safe. /// The same builder, to allow chaining. @@ -105,6 +123,7 @@ internal sealed class GitPullBuilder(IGitProcessRunner runner, AbsoluteDirectory private bool _rebase; private bool _merge; private bool _prune; + private GitSubmoduleRecursion? _submoduleRecursion; /// public IGitPullBuilder FromRemote(GitRemoteName name) @@ -148,6 +167,21 @@ public IGitPullBuilder Prune() return this; } + /// + public IGitPullBuilder RecursingSubmodules(GitSubmoduleRecursion recursion) + { + // Validated at the fluent call rather than deferred into AppendVerbArguments, matching + // IGitStatusBuilder.WithUntrackedFiles: BuildArguments is documented as a pure computation + // with no exceptions of its own beyond the contradiction guards. + if (!Enum.IsDefined(recursion)) + { + throw new InvalidEnumArgumentException(nameof(recursion), (int)recursion, typeof(GitSubmoduleRecursion)); + } + + _submoduleRecursion = recursion; + return this; + } + /// public IGitPullBuilder ReportingProgress(IProgress progress) { @@ -207,6 +241,11 @@ protected override void AppendVerbArguments(ICollection arguments) arguments.Add("--prune"); } + if (_submoduleRecursion is GitSubmoduleRecursion recursion) + { + arguments.Add("--recurse-submodules=" + ToOptionValue(recursion)); + } + if (_remote is null) { // git pull with no remote reads the first operand as the remote, so a branch @@ -259,36 +298,52 @@ protected override GitCommandException CreateException(GitProcessResult result) : base.CreateException(result); } - /// - public override async Task> TryExecuteAsync(CancellationToken cancellationToken = default) + /// + /// Joins standard error and standard output into one diagnostic. + /// + /// + /// Pull is the second verb whose diagnostic lands on standard output, so an error built only + /// from standard error would carry the fetch progress and say nothing about the conflict. The + /// two are joined on a single newline, with the trailing newline trimmed from standard error + /// first, so the result reads as two legible lines rather than a run-on string. Only the parts + /// that are actually present are joined: a stderr-only failure (the common plain "fatal: ..." + /// case) must not gain a trailing blank line from an empty standard output. + /// + /// Overriding the base class's seam rather than either entry point is what makes the two report + /// the same text. recognises only a conflict and hands everything + /// else to the base implementation, whose message is built from this method — so a non-conflict + /// failure explained on standard output now reaches a caller of + /// as well as one of + /// , instead of arriving as + /// "git exited with code N: " with nothing after the colon. + /// + /// + /// The failed invocation outcome. + /// The joined diagnostic text. + protected override string GetDiagnostic(GitProcessResult result) { - GitProcessResult result = await Runner.RunAsync( - new GitProcessRequest { Arguments = BuildArguments(), Progress = Progress }, - cancellationToken).ConfigureAwait(false); - - if (result.Success) - { - return GitResult.FromValue(ParseResult(result)); - } + Ensure.NotNull(result); - // Pull is the second verb whose diagnostic lands on standard output, so an error built only - // from standard error would carry the fetch progress and say nothing about the conflict. The - // two are joined on a single newline, with the trailing newline trimmed from standard error - // first, so the result reads as two legible lines rather than a run-on string. Only the parts - // that are actually present are joined: a stderr-only failure (the common plain "fatal: ..." - // case) must not gain a trailing blank line from an empty standard output. string standardError = result.StandardError.TrimEnd('\n', '\r'); - string diagnostic = standardError.Length == 0 + + return standardError.Length == 0 ? result.StandardOutput : result.StandardOutput.Length == 0 ? standardError : standardError + "\n" + result.StandardOutput; - - return GitResult.FromError(new GitCommandError - { - ExitCode = result.ExitCode, - Arguments = result.Arguments, - StandardError = diagnostic, - }); } + + /// Maps the recursion mode onto the value git's flag takes. + /// The mode to map. + /// The flag value. + private static string ToOptionValue(GitSubmoduleRecursion recursion) => recursion switch + { + GitSubmoduleRecursion.No => "no", + GitSubmoduleRecursion.Yes => "yes", + GitSubmoduleRecursion.OnDemand => "on-demand", + + // Unreachable once RecursingSubmodules validates: this arm only exists to satisfy the + // compiler's exhaustiveness check over the switch. + _ => throw new InvalidEnumArgumentException(nameof(recursion), (int)recursion, typeof(GitSubmoduleRecursion)), + }; } diff --git a/GitIntegration/Builders/GitPushBuilder.cs b/GitIntegration/Builders/GitPushBuilder.cs index fbe7b85..3797289 100644 --- a/GitIntegration/Builders/GitPushBuilder.cs +++ b/GitIntegration/Builders/GitPushBuilder.cs @@ -4,6 +4,7 @@ namespace ktsu.GitIntegration; using System; using System.Collections.Generic; +using System.ComponentModel; using System.Threading; using System.Threading.Tasks; @@ -66,6 +67,31 @@ public interface IGitPushBuilder : IGitCommandBuilder /// The same builder, to allow chaining. public IGitPushBuilder DryRun(); + /// + /// Sets what git should do about the submodules' own commits before pushing the superproject. + /// + /// + /// + /// Deliberately not called RecursingSubmodules, even though git spells the flag + /// --recurse-submodules here exactly as it does on clone, fetch, + /// checkout, and pull. On those verbs the flag means "also operate on the + /// submodules' working trees or refs". Here it means something else entirely: a superproject + /// commit referencing a submodule commit that no remote has is a commit nobody else can use, and + /// this governs whether git checks for, or pushes, those submodule commits to their own remotes + /// first. Putting one word on two unrelated behaviours would be the easiest possible thing for a + /// caller to get wrong, so the two carry different method names and different enums. + /// + /// + /// Note that pushes the submodules and stops, leaving + /// the superproject unpushed — which means a push that "succeeded" under that value did not push + /// what a caller may have thought it did. + /// + /// + /// What to do about the submodules' commits. + /// The same builder, to allow chaining. + /// is not a recognised value. + public IGitPushBuilder CheckingSubmodules(GitSubmodulePushCheck check); + /// Reports git's progress output as it arrives. /// The sink to report to. Must be thread-safe. /// The same builder, to allow chaining. @@ -88,6 +114,7 @@ internal sealed class GitPushBuilder(IGitProcessRunner runner, AbsoluteDirectory private bool _forceWithLease; private bool _delete; private bool _dryRun; + private GitSubmodulePushCheck? _submoduleCheck; /// public IGitPushBuilder ToRemote(GitRemoteName name) @@ -138,6 +165,21 @@ public IGitPushBuilder DryRun() return this; } + /// + public IGitPushBuilder CheckingSubmodules(GitSubmodulePushCheck check) + { + // Validated at the fluent call rather than deferred into AppendVerbArguments, matching + // IGitStatusBuilder.WithUntrackedFiles: BuildArguments is documented as a pure computation + // with no exceptions of its own. + if (!Enum.IsDefined(check)) + { + throw new InvalidEnumArgumentException(nameof(check), (int)check, typeof(GitSubmodulePushCheck)); + } + + _submoduleCheck = check; + return this; + } + /// public IGitPushBuilder ReportingProgress(IProgress progress) { @@ -182,6 +224,11 @@ protected override void AppendVerbArguments(ICollection arguments) arguments.Add("--dry-run"); } + if (_submoduleCheck is GitSubmodulePushCheck check) + { + arguments.Add("--recurse-submodules=" + ToOptionValue(check)); + } + AppendRefspec(arguments); } @@ -268,4 +315,19 @@ private Task RunAsync(CancellationToken cancellationToken) => Runner.RunAsync( new GitProcessRequest { Arguments = BuildArguments(), Progress = Progress }, cancellationToken); + + /// Maps the submodule check onto the value git's flag takes. + /// The check to map. + /// The flag value. + private static string ToOptionValue(GitSubmodulePushCheck check) => check switch + { + GitSubmodulePushCheck.No => "no", + GitSubmodulePushCheck.Check => "check", + GitSubmodulePushCheck.OnDemand => "on-demand", + GitSubmodulePushCheck.Only => "only", + + // Unreachable once CheckingSubmodules validates: this arm only exists to satisfy the + // compiler's exhaustiveness check over the switch. + _ => throw new InvalidEnumArgumentException(nameof(check), (int)check, typeof(GitSubmodulePushCheck)), + }; } diff --git a/GitIntegration/Builders/GitRevListBuilder.cs b/GitIntegration/Builders/GitRevListBuilder.cs new file mode 100644 index 0000000..2fff337 --- /dev/null +++ b/GitIntegration/Builders/GitRevListBuilder.cs @@ -0,0 +1,219 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitIntegration; + +using System; +using System.Collections.Generic; +using System.Globalization; + +using ktsu.Semantics.Paths; + +/// +/// Counts the commits a revision or range names. +/// +/// +/// rev-list --count prints the number directly. The alternative — listing the commits with +/// Log() and reading Count — runs git with this library's pinned format and puts every +/// record through , building a full with both +/// signatures, the subject, the body, and the parents, for every commit, in order to produce a +/// single integer. Over a few commits the difference is negligible; over thousands it is a great +/// deal of process output, allocation, and parsing thrown away. +/// +/// The two are also not equally reliable. can raise +/// on a commit whose record does not have the expected shape, and a +/// counting query has no reason to be able to fail that way. +/// +/// +/// For the common case — how far the current branch has diverged from its own upstream — +/// and already answer it from a single +/// Status() call and need nothing here. +/// +/// +public interface IGitRevListBuilder : IGitCommandBuilder +{ + /// Counts only first parents, ignoring the history merged in by a merge commit. + /// The same builder, to allow chaining. + public IGitRevListBuilder FirstParentOnly(); + + /// Counts only commits touching this path. May be called more than once. + /// The path, relative to the repository root. + /// The same builder, to allow chaining. + /// is . + public IGitRevListBuilder ForPath(RelativeFilePath path); +} + +/// +/// Builds git rev-list --count. +/// +/// Runs the assembled command. +/// The repository to scope the command to. +/// The revision or range to count. +internal sealed class GitRevListBuilder( + IGitProcessRunner runner, + AbsoluteDirectoryPath repositoryPath, + GitRefName revision) + : GitCommandBuilder(runner, repositoryPath), IGitRevListBuilder +{ + private readonly GitRefName _revision = Ensure.NotNull(revision); + private readonly List _paths = []; + private bool _firstParentOnly; + + /// + public IGitRevListBuilder FirstParentOnly() + { + _firstParentOnly = true; + return this; + } + + /// + public IGitRevListBuilder ForPath(RelativeFilePath path) + { + _paths.Add(Ensure.NotNull(path)); + return this; + } + + /// + protected override void AppendVerbArguments(ICollection arguments) + { + Ensure.NotNull(arguments); + + arguments.Add("rev-list"); + arguments.Add("--count"); + + if (_firstParentOnly) + { + arguments.Add("--first-parent"); + } + + AppendOperands(arguments, _revision.WeakString); + + if (_paths.Count > 0) + { + // A pathspec goes after --, exactly as it does in GitLogBuilder, and the revision must + // already have been emitted: git reads everything after -- as a filename, so a revision + // placed there counts nothing rather than failing. + arguments.Add("--"); + + foreach (RelativeFilePath path in _paths) + { + arguments.Add(path.WeakString); + } + } + } + + /// + protected override int ParseResult(GitProcessResult result) => + GitRevListParser.ParseCount(Ensure.NotNull(result).StandardOutput); +} + +/// +/// Counts how far two revisions have diverged from each other. +/// +/// +/// rev-list --count --left-right <upstream>...<local>, with three dots, which +/// prints both counts from one invocation. This is a genuinely different question from a plain +/// count rather than an option that quietly changes what ExecuteAsync returns, which is why +/// it is a separate builder with its own result type. +/// +/// and answer the same question only +/// for HEAD against its configured upstream. This answers it for any two revisions, including two +/// that have no tracking relationship at all. +/// +/// +public interface IGitRevListDivergenceBuilder : IGitCommandBuilder +{ +} + +/// +/// Builds git rev-list --count --left-right <upstream>...<local>. +/// +/// Runs the assembled command. +/// The repository to scope the command to. +/// The revision whose exclusive commits are counted as Behind. +/// The revision whose exclusive commits are counted as Ahead. +internal sealed class GitRevListDivergenceBuilder( + IGitProcessRunner runner, + AbsoluteDirectoryPath repositoryPath, + GitRefName upstream, + GitRefName local) + : GitCommandBuilder(runner, repositoryPath), IGitRevListDivergenceBuilder +{ + private readonly GitRefName _upstream = Ensure.NotNull(upstream); + private readonly GitRefName _local = Ensure.NotNull(local); + + /// + protected override void AppendVerbArguments(ICollection arguments) + { + Ensure.NotNull(arguments); + + arguments.Add("rev-list"); + arguments.Add("--count"); + arguments.Add("--left-right"); + + // Three dots, not two. "a..b" names the commits b has and a does not — one number. "a...b" + // names the commits either has and the other does not, which is the pair --left-right then + // splits. Building the expression here rather than making the caller write it is what lets + // both revisions stay GitRefName values, each carrying its own NotAnOption validation, + // instead of being concatenated by a caller into one string this library cannot check. + AppendOperands(arguments, $"{_upstream.WeakString}...{_local.WeakString}"); + } + + /// + protected override GitDivergence ParseResult(GitProcessResult result) => + GitRevListParser.ParseDivergence(Ensure.NotNull(result).StandardOutput); +} + +/// +/// Reads the counts git rev-list --count prints. +/// +internal static class GitRevListParser +{ + /// + /// Parses the single integer rev-list --count prints. + /// + /// Everything git wrote to standard output. + /// The commit count. + /// The output was not a single whole number. + internal static int ParseCount(string output) + { + Ensure.NotNull(output); + + string trimmed = output.Trim(); + + return int.TryParse(trimmed, NumberStyles.Integer, CultureInfo.InvariantCulture, out int count) + ? count + : throw new GitParseException($"git rev-list --count reported something that is not a count: '{trimmed}'."); + } + + /// + /// Parses the tab-separated pair rev-list --count --left-right prints. + /// + /// + /// The left number counts commits only the left side of the a...b expression has, and the + /// right number those only the right side has. This library always builds that expression as + /// upstream...local, so left is and right is + /// — the mapping is fixed here rather than left to a caller + /// precisely because it is easy to get backwards. Verified against git 2.43, which separates the + /// two with a single tab. + /// + /// Everything git wrote to standard output. + /// The divergence. + /// The output was not two tab-separated whole numbers. + internal static GitDivergence ParseDivergence(string output) + { + Ensure.NotNull(output); + + string trimmed = output.Trim(); + string[] fields = trimmed.Split('\t'); + + if (fields.Length == 2 && + int.TryParse(fields[0], NumberStyles.Integer, CultureInfo.InvariantCulture, out int leftOnly) && + int.TryParse(fields[1], NumberStyles.Integer, CultureInfo.InvariantCulture, out int rightOnly)) + { + return new GitDivergence(Ahead: rightOnly, Behind: leftOnly); + } + + throw new GitParseException( + $"git rev-list --count --left-right reported something that is not a pair of counts: '{trimmed}'."); + } +} diff --git a/GitIntegration/Builders/GitSubmoduleListBuilder.cs b/GitIntegration/Builders/GitSubmoduleListBuilder.cs new file mode 100644 index 0000000..4a9b241 --- /dev/null +++ b/GitIntegration/Builders/GitSubmoduleListBuilder.cs @@ -0,0 +1,160 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitIntegration; + +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; + +using ktsu.Semantics.Paths; + +/// +/// Lists the repository's submodules, with what each records and what each has checked out. +/// +/// +/// +/// Lists the superproject's own submodules and does not recurse. That is a deliberate limit rather +/// than an omission. The authoritative, NUL-terminated source for a submodule's path is +/// ls-files --stage, and it enumerates only the superproject's own index — a nested +/// submodule's path would have to come from submodule status --recursive instead, whose +/// output has no -z form and cannot be split unambiguously without already knowing the path. +/// Recursing would therefore mean giving up, for nested entries only, the exact property this verb +/// is built on. +/// +/// +/// A caller who needs nested submodules recurses through composition instead: open each submodule as +/// a in its own right — its path is +/// combined with — and call +/// this verb on that. Every level is then exact, and it reuses this code rather than a second, +/// weaker implementation of it. +/// +/// +public interface IGitSubmoduleListBuilder : IGitCommandBuilder> +{ +} + +/// +/// Builds git ls-files --stage -z and reads git submodule status alongside it. +/// +/// +/// Two invocations, for the same reason commit runs git twice: no single command answers the +/// whole question. ls-files reports what the superproject records, NUL-terminated so a path +/// may contain anything; submodule status reports what is actually checked out, which +/// ls-files cannot know. See for how the second command's +/// ambiguous output is made safe to read by already knowing every path from the first. +/// +/// Runs the assembled command. +/// The repository to scope the command to. +internal sealed class GitSubmoduleListBuilder(IGitProcessRunner runner, AbsoluteDirectoryPath repositoryPath) + : GitCommandBuilder>(runner, repositoryPath), IGitSubmoduleListBuilder +{ + /// + protected override void AppendVerbArguments(ICollection arguments) + { + Ensure.NotNull(arguments); + + arguments.Add("ls-files"); + arguments.Add("--stage"); + arguments.Add("-z"); + } + + /// + /// + /// Reads the gitlinks only. The base class calls this with the first invocation's output, and + /// the state each entry needs comes from the second — see . + /// + protected override IReadOnlyList ParseResult(GitProcessResult result) => + GitSubmoduleParser.ParseGitlinks(Ensure.NotNull(result).StandardOutput); + + /// + public override async Task> ExecuteAsync(CancellationToken cancellationToken = default) + { + IReadOnlyList gitlinks = + await base.ExecuteAsync(cancellationToken).ConfigureAwait(false); + + return await ApplyStatusAsync(gitlinks, cancellationToken).ConfigureAwait(false); + } + + /// + public override async Task>> TryExecuteAsync(CancellationToken cancellationToken = default) + { + GitResult> gitlinks = + await base.TryExecuteAsync(cancellationToken).ConfigureAwait(false); + + if (!gitlinks.Success || gitlinks.Value is null) + { + return gitlinks; + } + + return GitResult>.FromValue( + await ApplyStatusAsync(gitlinks.Value, cancellationToken).ConfigureAwait(false)); + } + + /// + /// Runs submodule status and folds its answer into the gitlinks already read. + /// + /// + /// + /// Skipped entirely when there are no gitlinks, so a repository with no submodules — the common + /// case — costs one invocation rather than two. + /// + /// + /// The status probe goes through the result-based TryExecuteAsync rather than the + /// throwing entry point, and does so the same way regardless of which of this builder's own two + /// entry points is running, for the reason GitFetchBuilder's version probe gives: the + /// gitlinks are already known and are the authoritative half of the answer, so failing the whole + /// listing because the wrapper could not run would discard a correct result over a missing + /// embellishment. Every entry keeps in that case, which + /// already means exactly "git did not say". + /// + /// + /// The gitlinks read from the first invocation. + /// Cancels the invocation. + /// The submodules, with state resolved where git reported it. + private async Task> ApplyStatusAsync( + IReadOnlyList gitlinks, + CancellationToken cancellationToken) + { + if (gitlinks.Count == 0) + { + return gitlinks; + } + + GitResult status = await new GitSubmoduleStatusBuilder(Runner, RepositoryPath!) + .TryExecuteAsync(cancellationToken).ConfigureAwait(false); + + return status.Success && status.Value is not null + ? GitSubmoduleParser.ApplyStatus(gitlinks, status.Value) + : gitlinks; + } + + /// + /// Runs git submodule status and returns its standard output untrimmed. + /// + /// + /// Its own type rather than , whose contract is trimmed output. That + /// contract is right for the single-value probes it serves and wrong here: the marker for a + /// synchronised submodule is a leading space, so trimming silently deletes the very field + /// this invocation exists to read, and every in-sync submodule would come back + /// . Widening with a flag + /// would put that trap one careless default away from every other probe instead. + /// + /// Runs the assembled command. + /// The repository to scope the command to. + private sealed class GitSubmoduleStatusBuilder(IGitProcessRunner runner, AbsoluteDirectoryPath repositoryPath) + : GitCommandBuilder(runner, repositoryPath) + { + /// + protected override void AppendVerbArguments(ICollection arguments) + { + Ensure.NotNull(arguments); + + arguments.Add("submodule"); + arguments.Add("status"); + } + + /// + protected override string ParseResult(GitProcessResult result) => + Ensure.NotNull(result).StandardOutput; + } +} diff --git a/GitIntegration/Builders/GitSubmoduleUpdateBuilder.cs b/GitIntegration/Builders/GitSubmoduleUpdateBuilder.cs new file mode 100644 index 0000000..c08de7e --- /dev/null +++ b/GitIntegration/Builders/GitSubmoduleUpdateBuilder.cs @@ -0,0 +1,191 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitIntegration; + +using System; +using System.Collections.Generic; +using System.Globalization; + +using ktsu.Semantics.Paths; + +/// +/// Checks out the commits the superproject's gitlinks record, in each submodule's working directory. +/// +/// +/// +/// Returns only success or failure. Everything git submodule update prints is human prose +/// with no porcelain alternative, and this design forbids parsing that for every other verb — so +/// this one gets no exemption either, exactly as pull does not. A caller who needs to know +/// what moved asks Submodules() before and after, which is precise. +/// +/// +/// A failure here does not mean the repository is unchanged. This verb walks the submodules +/// in turn, so one failing after others have already been checked out leaves the repository in a +/// state that is neither the old one nor the intended new one. That is worth stating plainly, +/// because the natural reading of an exception is "nothing happened", and here that reading is +/// false. carries the exit code, the argument vector, and the +/// diagnostic, which is enough for a caller to act; what it cannot carry is how far the operation +/// got, and Submodules() is how to find that out. +/// +/// +/// Overlaps with Pull().RecursingSubmodules(...) without being equivalent to it. The flag +/// updates submodules as part of a pull; this verb also initialises newly added ones +/// through , which the flag does only for submodules already registered. +/// Both are worth having. +/// +/// +public interface IGitSubmoduleUpdateBuilder : IGitCommandBuilder +{ + /// + /// Initialises any submodule that is registered but has never been checked out. + /// + /// + /// Emits --init. Without it, a submodule added upstream since this working copy was + /// cloned is skipped silently rather than checked out, because update alone only touches + /// submodules that are already initialised. + /// + /// The same builder, to allow chaining. + public IGitSubmoduleUpdateBuilder Initialise(); + + /// Updates submodules nested inside submodules, to any depth. + /// The same builder, to allow chaining. + public IGitSubmoduleUpdateBuilder Recursive(); + + /// + /// Checks out the branch .gitmodules configures for each submodule, instead of the commit + /// the superproject records. + /// + /// + /// Emits --remote, and it changes what this verb is for. The default is to make the + /// working directories agree with the gitlinks the superproject already committed; this instead + /// moves each submodule to the tip of its configured branch, which will normally leave the + /// superproject's gitlinks out of date until someone commits the new ones. + /// + /// The same builder, to allow chaining. + public IGitSubmoduleUpdateBuilder FromRemote(); + + /// + /// Checks out the recorded commit even when it would discard changes in a submodule's working + /// directory. + /// + /// Local modifications inside the submodule are lost. + /// The same builder, to allow chaining. + public IGitSubmoduleUpdateBuilder Force(); + + /// Fetches only this many commits of history for a submodule being cloned. + /// The number of commits to fetch. Must be positive. + /// The same builder, to allow chaining. + /// is not positive. + public IGitSubmoduleUpdateBuilder WithDepth(int depth); + + /// Reports git's progress output as it arrives. + /// + /// Offered because this is a network operation whenever a submodule has to be cloned or fetched, + /// the same reason Fetch, Clone, and Push offer it. + /// + /// The sink to report to. Must be thread-safe. + /// The same builder, to allow chaining. + /// is . + public IGitSubmoduleUpdateBuilder ReportingProgress(IProgress progress); +} + +/// +/// Builds git submodule update. +/// +/// Runs the assembled command. +/// The repository to scope the command to. +internal sealed class GitSubmoduleUpdateBuilder(IGitProcessRunner runner, AbsoluteDirectoryPath repositoryPath) + : GitCommandBuilder(runner, repositoryPath), IGitSubmoduleUpdateBuilder +{ + private bool _initialise; + private bool _recursive; + private bool _fromRemote; + private bool _force; + private int? _depth; + + /// + public IGitSubmoduleUpdateBuilder Initialise() + { + _initialise = true; + return this; + } + + /// + public IGitSubmoduleUpdateBuilder Recursive() + { + _recursive = true; + return this; + } + + /// + public IGitSubmoduleUpdateBuilder FromRemote() + { + _fromRemote = true; + return this; + } + + /// + public IGitSubmoduleUpdateBuilder Force() + { + _force = true; + return this; + } + + /// + public IGitSubmoduleUpdateBuilder WithDepth(int depth) + { + ArgumentOutOfRangeException.ThrowIfNegativeOrZero(depth); + _depth = depth; + return this; + } + + /// + public IGitSubmoduleUpdateBuilder ReportingProgress(IProgress progress) + { + Progress = Ensure.NotNull(progress); + return this; + } + + /// + protected override void AppendVerbArguments(ICollection arguments) + { + Ensure.NotNull(arguments); + + arguments.Add("submodule"); + arguments.Add("update"); + + if (_initialise) + { + arguments.Add("--init"); + } + + if (_recursive) + { + arguments.Add("--recursive"); + } + + if (_fromRemote) + { + arguments.Add("--remote"); + } + + if (_force) + { + arguments.Add("--force"); + } + + if (_depth is int depth) + { + arguments.Add("--depth"); + arguments.Add(depth.ToString(CultureInfo.InvariantCulture)); + } + + // No operands: this verb updates every submodule. A pathspec restricting it to some of them + // would be a caller-supplied value and would need the end-of-options guard; nothing asks for + // that yet, and adding it later changes no existing vector. + } + + /// + protected override GitCompleted ParseResult(GitProcessResult result) => + new() { Arguments = Ensure.NotNull(result).Arguments }; +} diff --git a/GitIntegration/Builders/GitTagCreateBuilder.cs b/GitIntegration/Builders/GitTagCreateBuilder.cs new file mode 100644 index 0000000..701dbd8 --- /dev/null +++ b/GitIntegration/Builders/GitTagCreateBuilder.cs @@ -0,0 +1,132 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitIntegration; + +using System; +using System.Collections.Generic; + +using ktsu.Semantics.Paths; + +/// +/// Creates a tag. +/// +/// +/// Both kinds of git tag are reachable from here. Without the result is a +/// lightweight tag: a reference pointing straight at a commit. With it, git writes a tag object +/// carrying the message, the tagger, and the date, and points the reference at that instead. This is +/// the one place the branch pattern does not map one-to-one, since a branch has no equivalent of an +/// annotation. +/// +public interface IGitTagCreateBuilder : IGitCommandBuilder +{ + /// + /// Points the new tag at this revision instead of at HEAD. + /// + /// The revision the tag should name. + /// The same builder, to allow chaining. + /// is . + public IGitTagCreateBuilder At(GitRefName target); + + /// + /// Writes an annotated tag carrying this message, rather than a lightweight one. + /// + /// + /// The message is what makes the tag annotated, so the two are one method rather than a flag and + /// a separate setter. Git has no annotated tag without a message: git tag -a with none + /// supplied opens an editor, which no invocation this library makes could ever answer. + /// + /// The tag message. + /// The same builder, to allow chaining. + /// is . + public IGitTagCreateBuilder Annotating(GitCommitMessage message); + + /// + /// Replaces the tag if it already exists, instead of failing. + /// + /// + /// Git refuses to overwrite an existing tag and exits with code 128 without this. Note that a tag + /// already fetched by someone else is not moved by this — it only changes the local reference — + /// which is why moving a published tag is a thing git makes deliberately awkward. + /// + /// The same builder, to allow chaining. + public IGitTagCreateBuilder Force(); +} + +/// +/// Builds git tag [-a -m <message>] <name> [<target>]. +/// +/// Runs the assembled command. +/// The repository to scope the command to. +/// The tag to create. +internal sealed class GitTagCreateBuilder( + IGitProcessRunner runner, + AbsoluteDirectoryPath repositoryPath, + GitTagName name) + : GitCommandBuilder(runner, repositoryPath), IGitTagCreateBuilder +{ + private readonly GitTagName _name = Ensure.NotNull(name); + private GitRefName? _target; + private GitCommitMessage? _message; + private bool _force; + + /// + public IGitTagCreateBuilder At(GitRefName target) + { + _target = Ensure.NotNull(target); + return this; + } + + /// + public IGitTagCreateBuilder Annotating(GitCommitMessage message) + { + _message = Ensure.NotNull(message); + return this; + } + + /// + public IGitTagCreateBuilder Force() + { + _force = true; + return this; + } + + /// + protected override void AppendVerbArguments(ICollection arguments) + { + Ensure.NotNull(arguments); + + arguments.Add("tag"); + + if (_force) + { + arguments.Add("--force"); + } + + // --annotate and --message travel together and are emitted adjacently, because either alone + // is a different command: --annotate without a message opens an editor, which output + // redirection makes impossible to answer, and --message without --annotate makes an + // annotated tag anyway but only as a side effect git documents rather than promises. + if (_message is not null) + { + arguments.Add("--annotate"); + arguments.Add("--message"); + arguments.Add(_message.WeakString); + } + + // Order is load-bearing, exactly as it is for branch creation: git tag takes then an + // optional positionally, and swapping them tags the wrong object under the wrong + // name without complaint. + if (_target is null) + { + AppendOperands(arguments, _name.WeakString); + } + else + { + AppendOperands(arguments, _name.WeakString, _target.WeakString); + } + } + + /// + protected override GitCompleted ParseResult(GitProcessResult result) => + new() { Arguments = Ensure.NotNull(result).Arguments }; +} diff --git a/GitIntegration/Builders/GitTagDeleteBuilder.cs b/GitIntegration/Builders/GitTagDeleteBuilder.cs new file mode 100644 index 0000000..4ee031f --- /dev/null +++ b/GitIntegration/Builders/GitTagDeleteBuilder.cs @@ -0,0 +1,57 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitIntegration; + +using System.Collections.Generic; + +using ktsu.Semantics.Paths; + +/// +/// Deletes a tag. +/// +/// +/// Deletes the local reference only. A tag already pushed to a remote stays there; removing it +/// needs a delete refspec through Push(), which this verb deliberately does not do on a +/// caller's behalf. +/// +/// There is no Force() here, unlike . Git has no +/// unmerged-tag concept to refuse over — a tag is not a line of development — so git tag +/// --delete takes no force flag at all. +/// +/// +public interface IGitTagDeleteBuilder : IGitCommandBuilder +{ +} + +/// +/// Builds git tag --delete <name>. +/// +/// Runs the assembled command. +/// The repository to scope the command to. +/// The tag to delete. +internal sealed class GitTagDeleteBuilder( + IGitProcessRunner runner, + AbsoluteDirectoryPath repositoryPath, + GitTagName name) + : GitCommandBuilder(runner, repositoryPath), IGitTagDeleteBuilder +{ + private readonly GitTagName _name = Ensure.NotNull(name); + + /// + protected override void AppendVerbArguments(ICollection arguments) + { + Ensure.NotNull(arguments); + + arguments.Add("tag"); + + // Long form throughout, matching GitBranchDeleteBuilder: --delete is -d, and spelling it out + // keeps the vector readable when it is copied out of a GitCommandException and rerun by hand. + arguments.Add("--delete"); + + AppendOperands(arguments, _name.WeakString); + } + + /// + protected override GitCompleted ParseResult(GitProcessResult result) => + new() { Arguments = Ensure.NotNull(result).Arguments }; +} diff --git a/GitIntegration/Builders/GitTagListBuilder.cs b/GitIntegration/Builders/GitTagListBuilder.cs new file mode 100644 index 0000000..d3b1526 --- /dev/null +++ b/GitIntegration/Builders/GitTagListBuilder.cs @@ -0,0 +1,49 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitIntegration; + +using System.Collections.Generic; + +using ktsu.Semantics.Paths; + +/// +/// Lists tag references. +/// +public interface IGitTagListBuilder : IGitCommandBuilder> +{ +} + +/// +/// Builds git for-each-ref over the tag namespace. +/// +/// +/// for-each-ref rather than tag --list, for the reason +/// gives: it takes an explicit format string, so the output is +/// machine-readable by construction rather than by hoping a human-facing listing keeps its shape. +/// tag --list prints bare names and nothing else without a format of its own, so it could not +/// answer whether a tag is annotated at all. +/// +/// Runs the assembled command. +/// The repository to scope the command to. +internal sealed class GitTagListBuilder(IGitProcessRunner runner, AbsoluteDirectoryPath repositoryPath) + : GitCommandBuilder>(runner, repositoryPath), IGitTagListBuilder +{ + private const string TagPrefix = "refs/tags"; + + /// + protected override void AppendVerbArguments(ICollection arguments) + { + Ensure.NotNull(arguments); + + arguments.Add("for-each-ref"); + arguments.Add("--format=" + GitOutputFormats.ForEachTagFormat); + + // A library constant, not a caller-supplied operand, so it needs no --end-of-options guard: + // no caller value can reach this position. + arguments.Add(TagPrefix); + } + + /// + protected override IReadOnlyList ParseResult(GitProcessResult result) => + GitTagParser.Parse(Ensure.NotNull(result).StandardOutput); +} diff --git a/GitIntegration/GitClient.cs b/GitIntegration/GitClient.cs index ef2d102..8bb5015 100644 --- a/GitIntegration/GitClient.cs +++ b/GitIntegration/GitClient.cs @@ -145,6 +145,18 @@ public IGitCloneBuilder Clone(GitRepository repository) "The repository has no RemotePath, so there is nothing to clone from.", nameof(repository)); - return Clone(source, repository.LocalPath); + // LocalPath is nullable, and a repository a hosting provider enumerated has none: it has + // never been cloned, and the providers no longer invent a path under the process's current + // directory to fill the gap. Reported the same way the missing RemotePath is, rather than + // resurrecting that invented default here — where the working copy goes is the caller's + // decision, and the two-argument overload is where they make it. + AbsoluteDirectoryPath destination = repository.LocalPath + ?? throw new ArgumentException( + "The repository has no LocalPath, so there is nowhere to clone it to. Repositories " + + "enumerated from a hosting provider carry no local path; use the overload taking an " + + "explicit destination.", + nameof(repository)); + + return Clone(source, destination); } } diff --git a/GitIntegration/GitHubProvider.cs b/GitIntegration/GitHubProvider.cs index 4ee14a8..36784ff 100644 --- a/GitIntegration/GitHubProvider.cs +++ b/GitIntegration/GitHubProvider.cs @@ -42,8 +42,17 @@ public sealed class GitHubProvider : GitProvider /// public override GitProviderName Name => "GitHub".As(); + /// + private protected override HttpMessageHandler DefaultHandler => SharedHandler; + /// /// + /// Adds to the interface's remarks rather than restating them. + /// says only that implementations differ + /// in coverage and points here for this host's specifics, so the two texts have one job each and + /// neither is a copy of the other. An edit that moves this explanation must leave that pointer + /// aimed somewhere real. + /// /// Calls GitHub's GET /users/{login}/repos, which returns only 's /// public repositories. Supplying a token does not widen this: that endpoint does not /// honour authentication to reveal private repositories the way GET /user/repos would for @@ -54,6 +63,7 @@ public sealed class GitHubProvider : GitProvider /// through this provider today; do not assume this method's coverage matches an /// AzureDevOpsProvider equivalent, whose token can see everything it has access to under /// the same interface. + /// /// public override async Task> GetRepositoriesAsync(CancellationToken cancellationToken = default) { @@ -74,9 +84,9 @@ public override async Task> GetRepositoriesAsync(Ca } /// - public override async Task> GetPullRequestsAsync(GitRepositoryName repositoryName, CancellationToken cancellationToken = default) + internal override async Task> GetPullRequestsCoreAsync(string repositoryIdentifier, CancellationToken cancellationToken) { - Ensure.NotNull(repositoryName); + Ensure.NotNull(repositoryIdentifier); cancellationToken.ThrowIfCancellationRequested(); (GitHubClient client, IDisposable createdTransport) = CreateClient(); @@ -91,7 +101,7 @@ public override async Task> GetPullRequestsAsync(G PullRequestRequest request = new() { State = ItemStateFilter.Open }; IReadOnlyList pullRequests = await client.PullRequest - .GetAllForRepository(Owner.WeakString, repositoryName.WeakString, request) + .GetAllForRepository(Owner.WeakString, repositoryIdentifier, request) .ConfigureAwait(false); return [.. pullRequests.Select(ToGitPullRequest)]; @@ -103,9 +113,9 @@ public override async Task> GetPullRequestsAsync(G } /// - internal override async Task CreatePullRequestCoreAsync(GitRepositoryName repositoryName, GitPullRequestSpecification specification, CancellationToken cancellationToken) + internal override async Task CreatePullRequestCoreAsync(string repositoryIdentifier, GitPullRequestSpecification specification, CancellationToken cancellationToken) { - Ensure.NotNull(repositoryName); + Ensure.NotNull(repositoryIdentifier); Ensure.NotNull(specification); cancellationToken.ThrowIfCancellationRequested(); @@ -125,7 +135,7 @@ internal override async Task CreatePullRequestCoreAsync(GitRepos }; PullRequest created = await client.PullRequest - .Create(Owner.WeakString, repositoryName.WeakString, newPullRequest) + .Create(Owner.WeakString, repositoryIdentifier, newPullRequest) .ConfigureAwait(false); return ToGitPullRequest(created); @@ -175,7 +185,7 @@ internal override async Task CreatePullRequestCoreAsync(GitRepos Credentials credentials = ToOctokitCredentials(ResolveCredential()); ProductHeaderValue product = new(AppDomain.CurrentDomain.FriendlyName); - HttpMessageHandler transport = Handler ?? SharedHandler; + HttpMessageHandler transport = Handler ?? DefaultHandler; HttpClientAdapter adapter = new(() => new NonOwningHandler(transport)); GitHubClient client = new(new Connection(product, adapter)) { Credentials = credentials }; @@ -236,24 +246,26 @@ protected override void Dispose(bool disposing) /// Maps an Octokit repository onto this library's model. /// /// - /// is required, but a repository this provider enumerates - /// from the host has never been cloned, so there is no real path to report. This uses the same - /// destination git clone <url> would pick with no destination argument of its own — a - /// subdirectory named after the repository, under the current directory — which matches - /// 's own documented "or is intended to be, cloned" meaning. + /// is left unset, because a repository this provider + /// enumerates from the host has never been cloned and there is no local path to report. It used + /// to be filled with the destination git clone <url> would pick given no destination + /// of its own — a subdirectory of the process's current directory — which made the same remote + /// repository yield a different record depending on when it was enumerated, and needed a + /// containment guard to keep a name from a remote response from escaping that directory. Saying + /// "not known" needs neither. + /// + /// carries GitHub's own numeric repository id, so a + /// caller can address the repository the way GitHub documents rather than by name. + /// /// /// The repository Octokit returned. /// The equivalent . - /// - /// 's name cannot be represented as a local directory. See - /// . - /// private GitRepository ToGitRepository(Repository repository) => new() { - LocalPath = ToLocalRepositoryPath(repository.Name, Name), - Name = repository.Name.As(), - WebURI = repository.HtmlUrl.As(), - RemotePath = repository.CloneUrl.As(), + Name = ToHostValue(repository.Name, Name, "repository name"), + HostRepositoryId = ToHostValue(repository.Id.ToString(CultureInfo.InvariantCulture), Name, "repository id"), + WebURI = ToHostValue(repository.HtmlUrl, Name, "repository web URI"), + RemotePath = ToHostValue(repository.CloneUrl, Name, "repository remote path"), }; /// @@ -332,6 +344,14 @@ private static GitPullRequestState ToGitPullRequestState(PullRequest pullRequest /// host-agnostic catch (GitHostingAuthenticationException) would then handle that failure /// on Azure DevOps and miss it on GitHub, and the two providers exist to be interchangeable. /// + /// + /// Every arm keeps the Octokit exception as the inner exception. This hierarchy carries the + /// provider, the status code, and the response body, which is enough to reproduce a failure by + /// hand — but not everything the host supplied. ApiError.Errors is often the only place + /// GitHub explains what was actually wrong with a request, and Octokit's synthetic 404 carries + /// no HttpResponse at all, so without the inner exception that failure would reach a + /// caller with an empty and nothing else to go on. + /// /// /// The failure Octokit reported. /// The equivalent , ready to throw. @@ -344,14 +364,14 @@ private GitHostingException Translate(ApiException exception) return exception switch { - RateLimitExceededException rateLimit => new GitHostingRateLimitException(exception.Message, Name, exception.StatusCode, responseBody, rateLimit.Reset), - SecondaryRateLimitExceededException => new GitHostingRateLimitException(exception.Message, Name, exception.StatusCode, responseBody, resetsAt: null), - AbuseException abuse => new GitHostingRateLimitException(exception.Message, Name, exception.StatusCode, responseBody, ToResetTime(abuse.RetryAfterSeconds)), - { StatusCode: HttpStatusCode.TooManyRequests } => new GitHostingRateLimitException(exception.Message, Name, exception.StatusCode, responseBody, ToResetTime(TryGetRetryAfterSeconds(exception))), - AuthorizationException => new GitHostingAuthenticationException(exception.Message, Name, exception.StatusCode, responseBody), - ForbiddenException => new GitHostingAuthenticationException(exception.Message, Name, exception.StatusCode, responseBody), - NotFoundException => new GitHostingNotFoundException(exception.Message, Name, exception.StatusCode, responseBody), - _ => new GitHostingRequestException(exception.Message, Name, exception.StatusCode, responseBody), + RateLimitExceededException rateLimit => new GitHostingRateLimitException(exception.Message, Name, exception.StatusCode, responseBody, rateLimit.Reset, exception), + SecondaryRateLimitExceededException => new GitHostingRateLimitException(exception.Message, Name, exception.StatusCode, responseBody, resetsAt: null, exception), + AbuseException abuse => new GitHostingRateLimitException(exception.Message, Name, exception.StatusCode, responseBody, ToResetTime(abuse.RetryAfterSeconds), exception), + { StatusCode: HttpStatusCode.TooManyRequests } => new GitHostingRateLimitException(exception.Message, Name, exception.StatusCode, responseBody, ToResetTime(TryGetRetryAfterSeconds(exception)), exception), + AuthorizationException => new GitHostingAuthenticationException(exception.Message, Name, exception.StatusCode, responseBody, exception), + ForbiddenException => new GitHostingAuthenticationException(exception.Message, Name, exception.StatusCode, responseBody, exception), + NotFoundException => new GitHostingNotFoundException(exception.Message, Name, exception.StatusCode, responseBody, exception), + _ => new GitHostingRequestException(exception.Message, Name, exception.StatusCode, responseBody, exception), }; } diff --git a/GitIntegration/GitProvider.cs b/GitIntegration/GitProvider.cs index 24f3544..9e825fe 100644 --- a/GitIntegration/GitProvider.cs +++ b/GitIntegration/GitProvider.cs @@ -4,13 +4,11 @@ namespace ktsu.GitIntegration; using System; using System.Collections.Generic; -using System.IO; using System.Net.Http; using System.Threading; using System.Threading.Tasks; using ktsu.CredentialCache; -using ktsu.Semantics.Paths; using ktsu.Semantics.Strings; /// @@ -31,18 +29,6 @@ namespace ktsu.GitIntegration; /// public abstract class GitProvider : IGitHostingProvider { - /// - /// The transport every provider built on shares when no - /// was injected. - /// - /// - /// One handler for the process, not one per call. A handler owns a connection pool, so building - /// and disposing one per call tears down that pool each time and leaves its sockets in - /// TIME_WAIT — a caller looping over a hundred repositories would build and destroy a - /// hundred pools. See for why it is never disposed. - /// - private static readonly SocketsHttpHandler SharedHandler = CreateDefaultHandler(); - /// public abstract GitProviderName Name { get; } @@ -90,17 +76,83 @@ public abstract class GitProvider : IGitHostingProvider public abstract Task> GetRepositoriesAsync(CancellationToken cancellationToken = default); /// - public abstract Task> GetPullRequestsAsync(GitRepositoryName repositoryName, CancellationToken cancellationToken = default); + public Task> GetPullRequestsAsync(GitRepositoryName repositoryName, CancellationToken cancellationToken = default) => + GetPullRequestsCoreAsync(Ensure.NotNull(repositoryName).WeakString, cancellationToken); + + /// + public Task> GetPullRequestsAsync(GitRepository repository, CancellationToken cancellationToken = default) => + GetPullRequestsCoreAsync(ToRepositoryIdentifier(repository), cancellationToken); /// public IGitPullRequestCreateBuilder CreatePullRequest(GitRepositoryName repositoryName) { - Ensure.NotNull(repositoryName); + string identifier = Ensure.NotNull(repositoryName).WeakString; + + return new GitPullRequestCreateBuilder((specification, cancellationToken) => + CreatePullRequestCoreAsync(identifier, specification, cancellationToken)); + } + + /// + public IGitPullRequestCreateBuilder CreatePullRequest(GitRepository repository) + { + string identifier = ToRepositoryIdentifier(repository); return new GitPullRequestCreateBuilder((specification, cancellationToken) => - CreatePullRequestCoreAsync(repositoryName, specification, cancellationToken)); + CreatePullRequestCoreAsync(identifier, specification, cancellationToken)); + } + + /// + /// Chooses how a repository is addressed in a request path: by the host's own identifier when + /// one is known, and by name otherwise. + /// + /// + /// is what a host documents its own API in terms of. + /// Azure DevOps types its {repositoryId} path parameter as string (uuid), and draws + /// an explicit id-or-name distinction for the sibling project parameter while withholding + /// it here — so substituting a name there is unconfirmed against the documented schema rather + /// than sanctioned by it. Preferring the id closes that gap for every repository a caller got + /// from GetRepositoriesAsync, which is the normal way to obtain one. + /// + /// Falling back to rather than requiring the id, because a + /// caller may legitimately have constructed a by hand from a name + /// alone. That fallback is the same unconfirmed substitution the name-taking overloads make, and + /// it is no worse than what those overloads already do. + /// + /// + /// The repository to address. + /// The path segment identifying the repository. + /// is . + /// + /// carries neither a + /// nor a , so there is nothing to address it by. + /// + private static string ToRepositoryIdentifier(GitRepository repository) + { + Ensure.NotNull(repository); + + return repository.HostRepositoryId?.WeakString + ?? repository.Name?.WeakString + ?? throw new ArgumentException( + "The repository carries neither a HostRepositoryId nor a Name, so there is no way to " + + "address it on the host.", + nameof(repository)); } + /// + /// Retrieves the open pull requests for the repository a path segment identifies. + /// + /// + /// The single implementation behind both public overloads, so the two cannot answer the same + /// question differently. Takes the finished path segment rather than a + /// or a , because choosing between an + /// id and a name is a decision that belongs in one place — — + /// rather than repeated in each provider. + /// + /// The path segment identifying the repository. + /// A token to cancel the request. + /// The repository's open pull requests, as reported by the host. + internal abstract Task> GetPullRequestsCoreAsync(string repositoryIdentifier, CancellationToken cancellationToken); + /// /// Creates the pull request a finished describes. /// @@ -111,11 +163,14 @@ public IGitPullRequestCreateBuilder CreatePullRequest(GitRepositoryName reposito /// this library ships lives in this assembly, so nothing outside it needs to implement this /// member. /// - /// The repository the pull request is opened against. + /// + /// The path segment identifying the repository the pull request is opened against, already chosen + /// by or taken from a caller-supplied name. + /// /// The pull request's finished configuration. /// A token to cancel the request. /// The pull request as the host reports it after creation. - internal abstract Task CreatePullRequestCoreAsync(GitRepositoryName repositoryName, GitPullRequestSpecification specification, CancellationToken cancellationToken); + internal abstract Task CreatePullRequestCoreAsync(string repositoryIdentifier, GitPullRequestSpecification specification, CancellationToken cancellationToken); /// /// Attempts to retrieve the credential for this provider from the credential cache. @@ -190,7 +245,7 @@ internal HostingCredential ResolveCredential() => /// /// The client is per call and the transport underneath it is not. This client never owns its /// handler, whichever branch supplied it: an injected belongs to whoever - /// supplied it, and has to outlive every call this provider makes. + /// supplied it, and has to outlive every call this provider makes. /// One unconditional covers both, so there is no branch here for a later /// change to get wrong. /// @@ -207,54 +262,86 @@ internal HostingCredential ResolveCredential() => /// /// /// An ready to issue requests, which the caller disposes. - protected HttpClient CreateHttpClient() => new(Handler ?? SharedHandler, disposeHandler: false); + protected HttpClient CreateHttpClient() => new(Handler ?? DefaultHandler, disposeHandler: false); + + /// + /// Gets the transport this provider's calls share when no was injected. + /// + /// + /// + /// Abstract rather than a shared field on this base class, so each + /// provider states which handler is its own. A single field here would have every provider + /// deriving from share one connection pool — which is what this + /// library did while was the only provider using it, so the + /// sharing was accidental and invisible rather than intended. Adding a third provider would have + /// silently enrolled it in Azure DevOps's pool, with nothing in the code to notice. + /// + /// + /// Each implementation is expected to return one process-lifetime instance built by + /// , not a fresh handler per read. A handler owns a connection + /// pool, so building and disposing one per call tears down that pool each time and leaves its + /// sockets in TIME_WAIT — a caller looping over a hundred repositories would build and + /// destroy a hundred pools. See for why they are never + /// disposed. + /// + /// + /// rather than : every provider + /// this library ships lives in this assembly, and no externally-defined subclass can be + /// instantiated anyway, so widening this member's reach would add public API surface nothing can + /// use. + /// + /// + /// The handler shared across this provider's calls. + private protected abstract HttpMessageHandler DefaultHandler { get; } /// - /// Builds the local path a repository name maps to under - /// , guaranteeing by construction that the result can - /// never point outside it. + /// Converts a field a host reported into a semantic value, reporting a value this library's own + /// validation rejects as a hosting failure rather than an argument failure. /// /// - /// Owns the whole operation rather than deriving a safe leaf and leaving the combine step to each - /// caller: a helper that only half-solves containment trusts every caller to get the other half - /// right, and a future provider could call it and forget to combine, or combine against something - /// other than . - /// silently discards every earlier argument when a later one is rooted, so combining - /// with a repository name of C:\Windows or - /// /etc directly would otherwise report a that points - /// there instead of under the current directory. resolves - /// that half of the problem by stripping every rooted prefix and directory component, but not the - /// other half: called on ../.. still returns - /// .., which still escapes upward once combined. A repository name comes from a remote - /// host's response, and is a value a caller might hand - /// straight to a clone or a file operation, so a name from which no safe leaf can be derived is - /// treated as the host having reported something this library cannot represent as a directory — - /// this throws rather than returning a value that looks safe but is not. + /// + /// The hosting counterpart to GitParseValues.ToSemantic, and it exists for the same reason: + /// a value arriving from outside is data to be validated, not an argument a caller got wrong. + /// Calling As<T>() directly on a host's response would throw + /// out of a public hosting method, whose documented failure + /// surface is the hierarchy — the same rule that makes + /// AzureDevOpsProvider.DeserializeSuccessBody wrap a JsonException rather than let + /// it escape. + /// + /// + /// A or absent field is not a failure: every field this converts is + /// optional on , and a host omitting one is ordinary. Only a field the + /// host did report and this library cannot represent is an error worth raising, because that is + /// the case where silently dropping the value would hide a real mismatch between this library's + /// model and what the host actually sends. + /// /// - /// The repository name a host reported, which may be . - /// - /// The provider that reported , named in the exception thrown - /// when no leaf can be derived. - /// - /// - /// The local path under , one directory named after the - /// derived leaf segment of . - /// + /// The semantic string type to produce. + /// The raw field as the host reported it, which may be . + /// The provider that reported it, named in the exception. + /// What the field is, used in the failure message. + /// The converted value, or when the host reported none. /// - /// is , empty, consists only of - /// whitespace, or is a name from which no directory segment can otherwise be derived. + /// is non-null but fails 's validation. /// - internal static AbsoluteDirectoryPath ToLocalRepositoryPath(string? repositoryName, GitProviderName providerName) + private protected static TSemantic? ToHostValue(string? value, GitProviderName providerName, string description) + where TSemantic : SemanticString, new() { - string leaf = Path.GetFileName(repositoryName ?? string.Empty); + if (value is null) + { + return null; + } - if (string.IsNullOrWhiteSpace(leaf) || leaf is "." or "..") + // Called on the constructed base type rather than as TSemantic.TryCreate, for the reason + // GitParseValues.ToSemantic gives: TryCreate is a plain static method rather than a static + // abstract interface member, so invoking it through the type parameter is CS0704. + if (SemanticString.TryCreate(value, out TSemantic? result) && result is not null) { - throw new GitHostingRequestException( - $"{providerName} reported a repository name '{repositoryName}' that cannot be represented as a local directory."); + return result; } - return Path.Combine(Environment.CurrentDirectory, leaf).As(); + throw new GitHostingRequestException( + $"{providerName} reported a {description} this library cannot represent: '{value}'."); } /// diff --git a/GitIntegration/GitRepository.cs b/GitIntegration/GitRepository.cs index 5ae04b2..8b527a1 100644 --- a/GitIntegration/GitRepository.cs +++ b/GitIntegration/GitRepository.cs @@ -17,15 +17,50 @@ namespace ktsu.GitIntegration; public class GitRepository { /// - /// Gets the local filesystem path where the repository is, or is intended to be, cloned. + /// Gets the local filesystem path where the repository is, or is intended to be, cloned, or + /// when it is not known. /// - public required AbsoluteDirectoryPath LocalPath { get; init; } + /// + /// Nullable rather than required, because a repository a hosting provider enumerated has never + /// been cloned and there is no local path to report for it. Both providers used to invent one + /// under , which had two consequences: the value + /// depended on process-global mutable state, so the same remote repository yielded a different + /// record depending on when it was enumerated; and because the name came from a remote API + /// response, the providers needed a containment guard purely to make an invented path safe. + /// Saying "not known" is what is actually true, and it removes the reason that guard existed. + /// + /// Every verb on this type needs a path, so each throws + /// when this is unset, exactly as it already does for a missing . + /// + /// + public AbsoluteDirectoryPath? LocalPath { get; init; } /// /// Gets the repository name, or when it is not known. /// public GitRepositoryName? Name { get; init; } + /// + /// Gets the host's own identifier for this repository, or when it is not + /// known. + /// + /// + /// The identifier a host assigns and documents its own API in terms of, as distinct from the + /// human-facing . Azure DevOps's REST reference types its + /// {repositoryId} path parameter as string (uuid), and draws an explicit + /// id-or-name distinction for the sibling project parameter while withholding it here — a + /// distinction stated where it applies reads as deliberate where it is withheld. Substituting a + /// name there works today, but working and being specified are different properties, and only + /// the second survives the vendor's next change. + /// + /// Populated by both providers when they enumerate repositories. A caller that has a + /// from GetRepositoriesAsync should prefer the overloads + /// taking one over those taking a bare , since only the former can + /// address the repository the way the host documents. + /// + /// + public GitHostRepositoryId? HostRepositoryId { get; init; } + /// /// Gets the browser-facing URI, or when it is not known. /// @@ -61,27 +96,28 @@ public Task IsClonedAsync(CancellationToken cancellationToken = default) // a metadata-only repository throws from the call itself rather than only once the returned // task is awaited. IGitProcessRunner runner = RequireRunner(); + AbsoluteDirectoryPath localPath = RequireLocalPath(); - return IsClonedCoreAsync(runner, cancellationToken); + return IsClonedCoreAsync(runner, localPath, cancellationToken); } - private Task IsClonedCoreAsync(IGitProcessRunner runner, CancellationToken cancellationToken) => - GitProbes.IsWorkTreeAsync(runner, LocalPath, cancellationToken); + private static Task IsClonedCoreAsync(IGitProcessRunner runner, AbsoluteDirectoryPath localPath, CancellationToken cancellationToken) => + GitProbes.IsWorkTreeAsync(runner, localPath, cancellationToken); /// Reports the working tree and index state. /// A fresh builder. /// This repository has no . - public IGitStatusBuilder Status() => new GitStatusBuilder(RequireRunner(), LocalPath); + public IGitStatusBuilder Status() => new GitStatusBuilder(RequireRunner(), RequireLocalPath()); /// Lists commits. /// A fresh builder. /// This repository has no . - public IGitLogBuilder Log() => new GitLogBuilder(RequireRunner(), LocalPath); + public IGitLogBuilder Log() => new GitLogBuilder(RequireRunner(), RequireLocalPath()); /// Lists the paths that differ between two states of the repository. /// A fresh builder. /// This repository has no . - public IGitDiffBuilder Diff() => new GitDiffBuilder(RequireRunner(), LocalPath); + public IGitDiffBuilder Diff() => new GitDiffBuilder(RequireRunner(), RequireLocalPath()); /// Resolves a revision to the object id it names. /// The revision to resolve. @@ -95,23 +131,78 @@ public IGitRevParseBuilder RevParse(GitRefName revision) // wrong diagnostic for what the caller got wrong. Ensure.NotNull(revision); - return new GitRevParseBuilder(RequireRunner(), LocalPath, revision); + return new GitRevParseBuilder(RequireRunner(), RequireLocalPath(), revision); + } + + /// Counts the commits a revision or range names. + /// + /// Cheaper than Log() for a count, and unable to fail the way it can — see + /// . For how far the current branch has diverged from its own + /// upstream, and already answer it + /// from a single call. + /// + /// The revision or range to count, such as HEAD or a..b. + /// A fresh builder. + /// is . + /// This repository has no . + public IGitRevListBuilder RevList(GitRefName revision) + { + Ensure.NotNull(revision); + + return new GitRevListBuilder(RequireRunner(), RequireLocalPath(), revision); + } + + /// Counts how far two revisions have diverged from each other. + /// + /// Answers ahead-and-behind for an arbitrary pair of revisions, where + /// answers it only for HEAD against its configured upstream. The two parameters are named so the + /// mapping onto and cannot + /// be read backwards. + /// + /// The revision whose exclusive commits are counted as behind. + /// The revision whose exclusive commits are counted as ahead. + /// A fresh builder. + /// + /// or is . + /// + /// This repository has no . + public IGitRevListDivergenceBuilder Divergence(GitRefName upstream, GitRefName local) + { + Ensure.NotNull(upstream); + Ensure.NotNull(local); + + return new GitRevListDivergenceBuilder(RequireRunner(), RequireLocalPath(), upstream, local); } /// Lists branch references. /// A fresh builder. /// This repository has no . - public IGitBranchListBuilder Branches() => new GitBranchListBuilder(RequireRunner(), LocalPath); + public IGitBranchListBuilder Branches() => new GitBranchListBuilder(RequireRunner(), RequireLocalPath()); /// Lists the configured remotes. /// A fresh builder. /// This repository has no . - public IGitRemoteListBuilder Remotes() => new GitRemoteListBuilder(RequireRunner(), LocalPath); + public IGitRemoteListBuilder Remotes() => new GitRemoteListBuilder(RequireRunner(), RequireLocalPath()); + + /// Lists the repository's submodules. + /// + /// Reports the superproject's own submodules and does not recurse — see + /// for why, and for how to recurse through composition + /// instead. + /// + /// A fresh builder. + /// This repository has no . + public IGitSubmoduleListBuilder Submodules() => new GitSubmoduleListBuilder(RequireRunner(), RequireLocalPath()); + + /// Lists tag references. + /// A fresh builder. + /// This repository has no . + public IGitTagListBuilder Tags() => new GitTagListBuilder(RequireRunner(), RequireLocalPath()); /// Stages changes for the next commit. /// A fresh builder. /// This repository has no . - public IGitAddBuilder Add() => new GitAddBuilder(RequireRunner(), LocalPath); + public IGitAddBuilder Add() => new GitAddBuilder(RequireRunner(), RequireLocalPath()); /// Records the staged changes as a new commit. /// The commit subject. @@ -123,7 +214,7 @@ public IGitCommitBuilder Commit(GitCommitMessage message) // Argument validation precedes the state check so a null argument is reported as such, // rather than as a missing runner. Ensure.NotNull(message); - return new GitCommitBuilder(RequireRunner(), LocalPath, message); + return new GitCommitBuilder(RequireRunner(), RequireLocalPath(), message); } /// Creates a branch. @@ -134,7 +225,7 @@ public IGitCommitBuilder Commit(GitCommitMessage message) public IGitBranchCreateBuilder CreateBranch(GitBranchName name) { Ensure.NotNull(name); - return new GitBranchCreateBuilder(RequireRunner(), LocalPath, name); + return new GitBranchCreateBuilder(RequireRunner(), RequireLocalPath(), name); } /// Deletes a branch. @@ -145,7 +236,36 @@ public IGitBranchCreateBuilder CreateBranch(GitBranchName name) public IGitBranchDeleteBuilder DeleteBranch(GitBranchName name) { Ensure.NotNull(name); - return new GitBranchDeleteBuilder(RequireRunner(), LocalPath, name); + return new GitBranchDeleteBuilder(RequireRunner(), RequireLocalPath(), name); + } + + /// Creates a tag. + /// + /// Lightweight by default; call Annotating on the returned builder for an annotated tag. + /// + /// The tag to create. + /// A fresh builder. + /// is . + /// This repository has no . + public IGitTagCreateBuilder CreateTag(GitTagName name) + { + // Argument validation before RequireRunner(), matching every other verb taking an operand. + Ensure.NotNull(name); + + return new GitTagCreateBuilder(RequireRunner(), RequireLocalPath(), name); + } + + /// Deletes a tag. + /// Deletes the local reference only; a tag already pushed to a remote stays there. + /// The tag to delete. + /// A fresh builder. + /// is . + /// This repository has no . + public IGitTagDeleteBuilder DeleteTag(GitTagName name) + { + Ensure.NotNull(name); + + return new GitTagDeleteBuilder(RequireRunner(), RequireLocalPath(), name); } /// Switches the working tree to a different branch, tag, or commit. @@ -156,7 +276,7 @@ public IGitBranchDeleteBuilder DeleteBranch(GitBranchName name) public IGitCheckoutBuilder Checkout(GitRefName target) { Ensure.NotNull(target); - return new GitCheckoutBuilder(RequireRunner(), LocalPath, target); + return new GitCheckoutBuilder(RequireRunner(), RequireLocalPath(), target); } /// Adds a remote. @@ -171,7 +291,7 @@ public IGitRemoteAddBuilder AddRemote(GitRemoteName name, GitRepositoryRemotePat { Ensure.NotNull(name); Ensure.NotNull(url); - return new GitRemoteAddBuilder(RequireRunner(), LocalPath, name, url); + return new GitRemoteAddBuilder(RequireRunner(), RequireLocalPath(), name, url); } /// Removes a remote and every remote-tracking branch belonging to it. @@ -182,7 +302,7 @@ public IGitRemoteAddBuilder AddRemote(GitRemoteName name, GitRepositoryRemotePat public IGitRemoteRemoveBuilder RemoveRemote(GitRemoteName name) { Ensure.NotNull(name); - return new GitRemoteRemoveBuilder(RequireRunner(), LocalPath, name); + return new GitRemoteRemoveBuilder(RequireRunner(), RequireLocalPath(), name); } /// Changes the URL a remote points at. @@ -197,23 +317,33 @@ public IGitRemoteSetUrlBuilder SetRemoteUrl(GitRemoteName name, GitRepositoryRem { Ensure.NotNull(name); Ensure.NotNull(url); - return new GitRemoteSetUrlBuilder(RequireRunner(), LocalPath, name, url); + return new GitRemoteSetUrlBuilder(RequireRunner(), RequireLocalPath(), name, url); } + /// Checks out the commits the superproject's gitlinks record. + /// + /// A failure does not mean the repository is unchanged — see + /// . + /// + /// A fresh builder. + /// This repository has no . + public IGitSubmoduleUpdateBuilder UpdateSubmodules() => + new GitSubmoduleUpdateBuilder(RequireRunner(), RequireLocalPath()); + /// Downloads objects and refs from a remote without touching the working tree. /// A fresh builder. /// This repository has no . - public IGitFetchBuilder Fetch() => new GitFetchBuilder(RequireRunner(), LocalPath); + public IGitFetchBuilder Fetch() => new GitFetchBuilder(RequireRunner(), RequireLocalPath()); /// Fetches from a remote and integrates the result into the current branch. /// A fresh builder. /// This repository has no . - public IGitPullBuilder Pull() => new GitPullBuilder(RequireRunner(), LocalPath); + public IGitPullBuilder Pull() => new GitPullBuilder(RequireRunner(), RequireLocalPath()); /// Sends local commits to a remote. /// A fresh builder. /// This repository has no . - public IGitPushBuilder Push() => new GitPushBuilder(RequireRunner(), LocalPath); + public IGitPushBuilder Push() => new GitPushBuilder(RequireRunner(), RequireLocalPath()); private IGitProcessRunner RequireRunner() => ProcessRunner ?? throw new InvalidOperationException( @@ -221,6 +351,26 @@ private IGitProcessRunner RequireRunner() => $"from {nameof(IGitClient)}.{nameof(IGitClient.OpenAsync)} or " + $"{nameof(IGitClient)}.{nameof(IGitClient.DiscoverAsync)} before running git commands against it."); + /// + /// Returns , or throws when this repository has none. + /// + /// + /// The companion to , and separately reachable from it. Both are set + /// together on a repository this library produces — and + /// supply both, a hosting provider supplies neither — but + /// the properties are public init accessors, so a caller can construct a + /// carrying one and not the other, and each check has to stand on + /// its own. is evaluated first at every call site, so a repository + /// missing both reports the runner, which is the more specific thing to have got wrong. + /// + /// The local path. + /// This repository has no . + private AbsoluteDirectoryPath RequireLocalPath() => + LocalPath ?? throw new InvalidOperationException( + "This GitRepository has no local path, so there is no working copy to run git against. A " + + "repository enumerated from a hosting provider has never been cloned; clone it, then " + + $"obtain a repository from {nameof(IGitClient)}.{nameof(IGitClient.OpenAsync)}."); + /// /// Opens in the default browser. /// diff --git a/GitIntegration/Hosting/AzureDevOpsJson.cs b/GitIntegration/Hosting/AzureDevOpsJson.cs index cc740e2..711932f 100644 --- a/GitIntegration/Hosting/AzureDevOpsJson.cs +++ b/GitIntegration/Hosting/AzureDevOpsJson.cs @@ -28,14 +28,26 @@ internal sealed class AzureDevOpsRepositoryListResponse /// /// /// Only the fields maps onto are -/// declared here; the rest of the documented schema (id, sshUrl, -/// defaultBranch, project, and so on) is left unread because nothing in this -/// library's model needs it. Field names come from findings section 2. is -/// documented-by-schema-only there: no example response the findings could check populates it, so -/// treat a populated value as unconfirmed shape even though the field name itself is confirmed. +/// declared here; the rest of the documented schema (sshUrl, defaultBranch, +/// project, and so on) is left unread because nothing in this library's model needs it. +/// Field names come from findings section 2. is documented-by-schema-only +/// there: no example response the findings could check populates it, so treat a populated value as +/// unconfirmed shape even though the field name itself is confirmed. /// internal sealed class AzureDevOpsRepository { + /// + /// Gets the repository's host-assigned id, or when the host omitted it. + /// + /// + /// Typed string (uuid) by Microsoft's schema, and read as a string here rather than as a + /// : the value is handed straight back to Azure DevOps as a path + /// segment and never inspected, so parsing it would only add a way for a well-formed response to + /// fail. It fills , which is what the + /// {repositoryId} path parameter is documented to take. + /// + public string? Id { get; init; } + /// Gets the repository's name, or when the host omitted it. public string? Name { get; init; } @@ -205,6 +217,16 @@ internal sealed class AzureDevOpsPullRequestCreateRequest public required string Title { get; init; } /// Gets the pull request's description, or to omit one. + /// + /// Omitted from the payload entirely when unset, rather than sent as "description": null. + /// Microsoft's own documented sample request simply leaves the field out when there is no + /// description, and this attribute is what makes "or to omit one" literally + /// true of the bytes on the wire rather than merely of this property's meaning. Applied here + /// rather than as a context-wide default: the response DTOs deserialize rather than serialize, so + /// a global setting would say nothing about them while quietly changing how any future request + /// type behaves. + /// + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] public string? Description { get; init; } /// Gets a value indicating whether the pull request should be created as a draft. diff --git a/GitIntegration/Hosting/AzureDevOpsProvider.cs b/GitIntegration/Hosting/AzureDevOpsProvider.cs index dbb9977..945cb13 100644 --- a/GitIntegration/Hosting/AzureDevOpsProvider.cs +++ b/GitIntegration/Hosting/AzureDevOpsProvider.cs @@ -44,7 +44,7 @@ public sealed class AzureDevOpsProvider : GitProvider private const string ApiVersion = "7.1"; /// - /// The number of pull requests each page of asks for, sent as + /// The number of pull requests each page of asks for, sent as /// $top. /// /// @@ -56,6 +56,23 @@ public sealed class AzureDevOpsProvider : GitProvider /// private const int PullRequestPageSize = 100; + /// + /// The transport every call shares when no + /// was injected. + /// + /// + /// Its own instance rather than one held on , so that adding a third + /// provider cannot silently enrol it in this one's connection pool. The two providers never talk + /// to the same hosts and a connection pool is per host anyway, so nothing is lost by keeping them + /// separate; what matters is that this is one handler for the process rather than one per call. + /// is what keeps the providers' settings from + /// drifting apart. + /// + private static readonly SocketsHttpHandler SharedHandler = CreateDefaultHandler(); + + /// + private protected override HttpMessageHandler DefaultHandler => SharedHandler; + /// /// Gets the name of this Git provider. /// @@ -72,7 +89,7 @@ public sealed class AzureDevOpsProvider : GitProvider /// same endpoint with the project path segment present or absent, not two different routes, which /// is exactly what being optional models. /// Pull request operations are the asymmetric case: Azure DevOps has no project-less pull-request - /// endpoint, so and + /// endpoint, so and /// both require to be set and throw /// otherwise. Resolving it automatically by re-enumerating repositories and matching names was /// rejected: it costs an extra call and is ambiguous whenever two projects hold a repository of @@ -86,7 +103,7 @@ public sealed class AzureDevOpsProvider : GitProvider /// the project path segment appears only when is set. This endpoint /// documents no pagination parameters at all: the response is the complete repository list for /// the organisation or project on every call, so this method issues exactly one request. That is - /// what makes it differ from , which must page. + /// what makes it differ from , which must page. /// public override async Task> GetRepositoriesAsync(CancellationToken cancellationToken = default) { @@ -128,7 +145,7 @@ public override async Task> GetRepositoriesAsync(Ca /// /// Calls GET .../repositories/{repositoryId}/pullrequests with /// searchCriteria.status=active sent explicitly. The endpoint's documented default is - /// already active, but 's contract is + /// already active, but 's contract is /// defined by this library, not by restating whatever a host happens to default to today. /// /// @@ -139,21 +156,23 @@ public override async Task> GetRepositoriesAsync(Ca /// needs none of this, because its endpoint documents no pagination at all. /// /// - /// {repositoryId} is filled with , which is unconfirmed - /// against the documented schema, not sanctioned by it: Microsoft's reference types that parameter - /// as a repository id (GitRepository.id is a string (uuid)), and draws an - /// explicit id-or-name distinction for the sibling project parameter without drawing one - /// here — a distinction Microsoft states where it applies reads as deliberate where it is - /// withheld. This library passes the name anyway because exposes - /// no repository id for a caller to supply. It very likely works today; that is a different - /// property from being specified, and only the specified kind survives a vendor's next change - /// unannounced. + /// {repositoryId} is filled with , which the caller's + /// choice of overload decides. Microsoft's reference types that parameter as a repository + /// id (GitRepository.id is a string (uuid)), and draws an explicit + /// id-or-name distinction for the sibling project parameter without drawing one here — a + /// distinction Microsoft states where it applies reads as deliberate where it is withheld. + /// therefore + /// passes , which is exactly what the schema + /// documents. + /// still passes a name, since that is all it is given; that very likely works today, but working + /// and being specified are different properties, and only the second survives a vendor's next + /// change unannounced. /// /// /// is . See 's remarks. - public override async Task> GetPullRequestsAsync(GitRepositoryName repositoryName, CancellationToken cancellationToken = default) + internal override async Task> GetPullRequestsCoreAsync(string repositoryIdentifier, CancellationToken cancellationToken) { - Ensure.NotNull(repositoryName); + Ensure.NotNull(repositoryIdentifier); cancellationToken.ThrowIfCancellationRequested(); EnsureProjectIsSet(); @@ -167,7 +186,7 @@ public override async Task> GetPullRequestsAsync(G while (morePages) { - using HttpRequestMessage request = new(HttpMethod.Get, BuildPullRequestListUri(repositoryName, skip)); + using HttpRequestMessage request = new(HttpMethod.Get, BuildPullRequestListUri(repositoryIdentifier, skip)); ApplyAuthentication(request, credential); using HttpResponseMessage response = await client.SendAsync(request, cancellationToken).ConfigureAwait(false); @@ -209,14 +228,14 @@ public override async Task> GetPullRequestsAsync(G /// rather than a specific status code. /// /// is . See 's remarks. - internal override async Task CreatePullRequestCoreAsync(GitRepositoryName repositoryName, GitPullRequestSpecification specification, CancellationToken cancellationToken) + internal override async Task CreatePullRequestCoreAsync(string repositoryIdentifier, GitPullRequestSpecification specification, CancellationToken cancellationToken) { - Ensure.NotNull(repositoryName); + Ensure.NotNull(repositoryIdentifier); Ensure.NotNull(specification); cancellationToken.ThrowIfCancellationRequested(); EnsureProjectIsSet(); - Uri requestUri = BuildPullRequestCreateUri(repositoryName); + Uri requestUri = BuildPullRequestCreateUri(repositoryIdentifier); HostingCredential credential = ResolveCredential(); AzureDevOpsPullRequestCreateRequest requestBody = new() @@ -259,7 +278,7 @@ internal override async Task CreatePullRequestCoreAsync(GitRepos /// Throws when a pull request operation is attempted without set. /// /// - /// Both and call this + /// Both and call this /// before building a request, since neither has a project-less pull-request endpoint to fall back /// to — see 's remarks for why this is not resolved automatically instead. /// @@ -296,11 +315,11 @@ private Uri BuildRepositoriesUri() /// $top is sent on every page, including the first, so a short page always means the last /// page rather than possibly meaning a defaulted one. See . /// - /// The repository the URI is scoped to. + /// The repository the URI is scoped to. /// How many pull requests the service should pass over before filling this page. /// The request URI, carrying . - private Uri BuildPullRequestListUri(GitRepositoryName repositoryName, int skip) => - new($"{PullRequestsPath(repositoryName)}?searchCriteria.status=active&$top={PullRequestPageSize}&$skip={skip.ToString(CultureInfo.InvariantCulture)}&api-version={ApiVersion}"); + private Uri BuildPullRequestListUri(string repositoryIdentifier, int skip) => + new($"{PullRequestsPath(repositoryIdentifier)}?searchCriteria.status=active&$top={PullRequestPageSize}&$skip={skip.ToString(CultureInfo.InvariantCulture)}&api-version={ApiVersion}"); /// /// Builds the URI a pull request is created against. @@ -309,10 +328,10 @@ private Uri BuildPullRequestListUri(GitRepositoryName repositoryName, int skip) /// The same path as the listing, without the listing's query parameters: Microsoft's reference /// documents no search criteria and no pagination on the creating POST. /// - /// The repository the pull request is opened against. + /// The repository the pull request is opened against. /// The request URI, carrying . - private Uri BuildPullRequestCreateUri(GitRepositoryName repositoryName) => - new($"{PullRequestsPath(repositoryName)}?api-version={ApiVersion}"); + private Uri BuildPullRequestCreateUri(string repositoryIdentifier) => + new($"{PullRequestsPath(repositoryIdentifier)}?api-version={ApiVersion}"); /// /// Builds the pull request endpoint's path for a repository, scoped to this provider's @@ -323,13 +342,16 @@ private Uri BuildPullRequestCreateUri(GitRepositoryName repositoryName) => /// — since a pull request URI has no project-less form to fall /// back to. /// - /// The repository the path is scoped to. + /// + /// The repository the path is scoped to, already resolved to the host's own id where one was + /// known — see . + /// /// The endpoint path, with no query string. - private string PullRequestsPath(GitRepositoryName repositoryName) + private string PullRequestsPath(string repositoryIdentifier) { string organization = Uri.EscapeDataString(Owner.WeakString); string project = Uri.EscapeDataString(Project!.WeakString); - string repository = Uri.EscapeDataString(repositoryName.WeakString); + string repository = Uri.EscapeDataString(repositoryIdentifier); return $"https://dev.azure.com/{organization}/{project}/_apis/git/repositories/{repository}/pullrequests"; } @@ -361,13 +383,15 @@ private string PullRequestsPath(GitRepositoryName repositoryName) } catch (JsonException exception) { - // The parse failure's own message is folded into the text rather than carried as an inner - // exception, because this hierarchy's four-argument constructors take no inner exception. + // The parse failure travels as the inner exception, so its path and line offset survive for + // anyone debugging a malformed body. Its message is also folded into the text, because a + // caller who only logs Message would otherwise see nothing about what failed to parse. throw new GitHostingRequestException( $"Azure DevOps reported success but returned a body that is not the expected JSON: {exception.Message}", Name, statusCode, - body); + body, + exception); } } @@ -415,23 +439,25 @@ private static AuthenticationHeaderValue BasicAuthenticationHeader(string userna /// Maps an Azure DevOps repository onto this library's model. /// /// - /// is required, but a repository this provider enumerates - /// from the host has never been cloned, so there is no real path to report — mirrors - /// 's own mapping, using the same destination a bare - /// git clone <url> would pick. + /// is left unset, because a repository this provider + /// enumerates from the host has never been cloned and there is no local path to report — + /// mirroring 's own mapping. Both used to invent one under the + /// process's current directory, which made the same remote repository yield a different record + /// depending on when it was enumerated, and needed a containment guard to keep a name from a + /// remote response from escaping that directory. + /// + /// carries Azure DevOps's own id, which is + /// what its {repositoryId} path parameter is documented to take. + /// /// /// The repository Azure DevOps returned. /// The equivalent . - /// - /// 's name is missing or cannot be represented as a local directory. - /// See . - /// private GitRepository ToGitRepository(AzureDevOpsRepository repository) => new() { - LocalPath = ToLocalRepositoryPath(repository.Name, Name), - Name = repository.Name is string repositoryName ? repositoryName.As() : null, - WebURI = repository.WebUrl is string webUrl ? webUrl.As() : null, - RemotePath = repository.RemoteUrl is string remoteUrl ? remoteUrl.As() : null, + Name = ToHostValue(repository.Name, Name, "repository name"), + HostRepositoryId = ToHostValue(repository.Id, Name, "repository id"), + WebURI = ToHostValue(repository.WebUrl, Name, "repository web URI"), + RemotePath = ToHostValue(repository.RemoteUrl, Name, "repository remote path"), }; /// diff --git a/GitIntegration/Hosting/GitHostingExceptions.cs b/GitIntegration/Hosting/GitHostingExceptions.cs index 7a0c823..b702760 100644 --- a/GitIntegration/Hosting/GitHostingExceptions.cs +++ b/GitIntegration/Hosting/GitHostingExceptions.cs @@ -18,12 +18,29 @@ namespace ktsu.GitIntegration; public class GitHostingException : Exception { /// Gets the provider that reported the failure. + /// + /// unless a constructor taking a was used. + /// The parameterless and message-only constructors exist because the analyzers require the + /// standard exception constructor set, not because this library ever calls them, and neither has + /// a provider to record. + /// + /// The provider, or when none was recorded. public GitProviderName? ProviderName { get; } /// Gets the HTTP status code the provider returned. + /// + /// Defaults to 0, which is not a real status code, under the same constructors that leave + /// . Test rather than + /// this property when distinguishing a failure carrying host context from one that does not: + /// has no member meaning "unset", so 0 cannot be told apart + /// from a status by its type alone. + /// + /// The status code, or 0 when none was recorded. public HttpStatusCode StatusCode { get; } /// Gets the raw response body the provider returned. + /// Empty when none was recorded, and empty when the provider sent no body. + /// The response body, never . public string ResponseBody { get; } = string.Empty; /// Initializes a new instance of the class. @@ -44,7 +61,26 @@ public GitHostingException(string message, Exception innerException) : base(mess /// The HTTP status code the provider returned. /// The raw response body the provider returned. public GitHostingException(string message, GitProviderName providerName, HttpStatusCode statusCode, string responseBody) - : base(message) + : this(message, providerName, statusCode, responseBody, innerException: null) + { + } + + /// Initializes a new instance of the class. + /// + /// The overload a provider translating a vendor SDK's own exception uses. Without it the + /// translated exception is all a caller ever sees, and detail the host supplied but this + /// hierarchy has no field for is lost — Octokit's ApiError.Errors, which is often the only + /// place GitHub explains what was actually wrong with a request, and Octokit's synthetic 404, + /// which carries no HttpResponse at all and so reaches a caller with an empty + /// and nothing else. + /// + /// The message describing the failure. + /// The provider that reported the failure. + /// The HTTP status code the provider returned. + /// The raw response body the provider returned. + /// The underlying failure, or when there is none. + public GitHostingException(string message, GitProviderName providerName, HttpStatusCode statusCode, string responseBody, Exception? innerException) + : base(message, innerException) { ProviderName = providerName; StatusCode = statusCode; @@ -77,6 +113,15 @@ public GitHostingAuthenticationException(string message, Exception innerExceptio /// The raw response body the provider returned. public GitHostingAuthenticationException(string message, GitProviderName providerName, HttpStatusCode statusCode, string responseBody) : base(message, providerName, statusCode, responseBody) { } + + /// Initializes a new instance of the class. + /// The message describing the failure. + /// The provider that reported the failure. + /// The HTTP status code the provider returned. + /// The raw response body the provider returned. + /// The underlying failure, or when there is none. + public GitHostingAuthenticationException(string message, GitProviderName providerName, HttpStatusCode statusCode, string responseBody, Exception? innerException) + : base(message, providerName, statusCode, responseBody, innerException) { } } /// @@ -104,6 +149,15 @@ public GitHostingNotFoundException(string message, Exception innerException) : b /// The raw response body the provider returned. public GitHostingNotFoundException(string message, GitProviderName providerName, HttpStatusCode statusCode, string responseBody) : base(message, providerName, statusCode, responseBody) { } + + /// Initializes a new instance of the class. + /// The message describing the failure. + /// The provider that reported the failure. + /// The HTTP status code the provider returned. + /// The raw response body the provider returned. + /// The underlying failure, or when there is none. + public GitHostingNotFoundException(string message, GitProviderName providerName, HttpStatusCode statusCode, string responseBody, Exception? innerException) + : base(message, providerName, statusCode, responseBody, innerException) { } } /// @@ -144,7 +198,28 @@ public GitHostingRateLimitException( HttpStatusCode statusCode, string responseBody, DateTimeOffset? resetsAt) - : base(message, providerName, statusCode, responseBody) => ResetsAt = resetsAt; + : this(message, providerName, statusCode, responseBody, resetsAt, innerException: null) + { + } + + /// Initializes a new instance of the class. + /// The message describing the failure. + /// The provider that reported the failure. + /// The HTTP status code the provider returned. + /// The raw response body the provider returned. + /// + /// The time at which the provider expects the caller's quota to reset, or + /// when the provider did not report one. + /// + /// The underlying failure, or when there is none. + public GitHostingRateLimitException( + string message, + GitProviderName providerName, + HttpStatusCode statusCode, + string responseBody, + DateTimeOffset? resetsAt, + Exception? innerException) + : base(message, providerName, statusCode, responseBody, innerException) => ResetsAt = resetsAt; } /// @@ -172,4 +247,13 @@ public GitHostingRequestException(string message, Exception innerException) : ba /// The raw response body the provider returned. public GitHostingRequestException(string message, GitProviderName providerName, HttpStatusCode statusCode, string responseBody) : base(message, providerName, statusCode, responseBody) { } + + /// Initializes a new instance of the class. + /// The message describing the failure. + /// The provider that reported the failure. + /// The HTTP status code the provider returned. + /// The raw response body the provider returned. + /// The underlying failure, or when there is none. + public GitHostingRequestException(string message, GitProviderName providerName, HttpStatusCode statusCode, string responseBody, Exception? innerException) + : base(message, providerName, statusCode, responseBody, innerException) { } } diff --git a/GitIntegration/Hosting/GitPullRequestCreateBuilder.cs b/GitIntegration/Hosting/GitPullRequestCreateBuilder.cs index 99c8974..b6059a5 100644 --- a/GitIntegration/Hosting/GitPullRequestCreateBuilder.cs +++ b/GitIntegration/Hosting/GitPullRequestCreateBuilder.cs @@ -65,6 +65,10 @@ public IGitPullRequestCreateBuilder AsDraft() /// public async Task ExecuteAsync(CancellationToken cancellationToken = default) { + // Three near-identical blocks rather than one loop or a shared helper, deliberately. Each + // message names the member that is missing and the method that sets it, and the tests pin + // them individually — collapsing them would either lose that specificity or reintroduce it as + // a table, which is the same three facts with a layer of indirection over them. if (_source is null) { throw new InvalidOperationException( diff --git a/GitIntegration/Hosting/IGitHostingProvider.cs b/GitIntegration/Hosting/IGitHostingProvider.cs index 9172650..efc3242 100644 --- a/GitIntegration/Hosting/IGitHostingProvider.cs +++ b/GitIntegration/Hosting/IGitHostingProvider.cs @@ -93,12 +93,56 @@ public interface IGitHostingProvider /// The repository's open pull requests, as reported by the host. public Task> GetPullRequestsAsync(GitRepositoryName repositoryName, CancellationToken cancellationToken = default); + /// + /// Retrieves the open pull requests for a repository, addressing it the way its host documents. + /// + /// + /// The overload to prefer when the caller has a from + /// . It addresses the repository by + /// when one is known, which is what a host documents + /// its own API in terms of — Azure DevOps types its {repositoryId} path parameter as + /// string (uuid) and never sanctions a name there. The name-taking overload substitutes a + /// name into that same position, which works today but is unconfirmed against the documented + /// schema; this one is not. Everything else about the two is identical, including the open-only + /// filter described above. + /// + /// + /// The repository to list pull requests for, carrying a + /// or at least a . + /// + /// A token to cancel the request. + /// The repository's open pull requests, as reported by the host. + /// is . + /// + /// carries neither identifier. + /// + public Task> GetPullRequestsAsync(GitRepository repository, CancellationToken cancellationToken = default); + /// /// Starts building a pull request for a repository. /// /// The repository the pull request is opened against. /// A builder that collects the pull request's details before submitting it. public IGitPullRequestCreateBuilder CreatePullRequest(GitRepositoryName repositoryName); + + /// + /// Starts building a pull request for a repository, addressing it the way its host documents. + /// + /// + /// The overload to prefer when the caller has a from + /// , for the same reason as + /// . + /// + /// + /// The repository the pull request is opened against, carrying a + /// or at least a . + /// + /// A builder that collects the pull request's details before submitting it. + /// is . + /// + /// carries neither identifier. + /// + public IGitPullRequestCreateBuilder CreatePullRequest(GitRepository repository); } /// diff --git a/GitIntegration/IGitClient.cs b/GitIntegration/IGitClient.cs index 8f3ae6d..b734400 100644 --- a/GitIntegration/IGitClient.cs +++ b/GitIntegration/IGitClient.cs @@ -87,15 +87,23 @@ public interface IGitClient /// /// /// This is the one seam between the library's two layers: a provider produces a - /// carrying remote metadata and an intended local path, and this - /// turns it into a working copy. + /// carrying remote metadata, and this turns it into a working copy. + /// + /// Both and must be + /// set. A repository straight from a hosting provider carries the first and not the second — it + /// has never been cloned, so there is no local path to report and the providers no longer invent + /// one under the process's current directory. Supply a destination with + /// repository with { LocalPath = … }, or use + /// directly. + /// /// /// The repository to clone, carrying a non-null - /// . + /// and . /// A fresh builder. /// is . /// - /// has no . + /// has no , or no + /// . /// public IGitCloneBuilder Clone(GitRepository repository); } diff --git a/GitIntegration/Models/GitDiffEntry.cs b/GitIntegration/Models/GitDiffEntry.cs index 0bee963..705a440 100644 --- a/GitIntegration/Models/GitDiffEntry.cs +++ b/GitIntegration/Models/GitDiffEntry.cs @@ -26,4 +26,24 @@ public sealed record GitDiffEntry /// when git reported none. /// public int? SimilarityPercent { get; init; } + + /// + /// Gets how many lines were added to this path, or when git reported no + /// count. + /// + /// + /// in two cases, and they are worth telling apart from zero. Line counts + /// are opt-in, so this is unset unless IGitDiffBuilder.WithLineCounts() was called; and + /// git declines to count a binary file at all, printing - where a number would go. A + /// binary change is not a zero-line change, so reporting 0 for one would state that nothing + /// changed in a file git simply did not measure. + /// + public int? Insertions { get; init; } + + /// + /// Gets how many lines were removed from this path, or when git reported + /// no count. + /// + /// See for when this is unset. + public int? Deletions { get; init; } } diff --git a/GitIntegration/Models/GitDivergence.cs b/GitIntegration/Models/GitDivergence.cs new file mode 100644 index 0000000..44dfea2 --- /dev/null +++ b/GitIntegration/Models/GitDivergence.cs @@ -0,0 +1,29 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitIntegration; + +/// +/// How far two revisions have diverged from each other, counted in commits. +/// +/// +/// The same pair of numbers and report, +/// for an arbitrary pair of revisions rather than for HEAD against its configured upstream. The +/// vocabulary is deliberately 's rather than git's own left-and-right, which +/// names the two sides by their position in a a...b expression: a caller comparing this +/// against a is comparing like with like, and does not have to work out +/// which side of the expression became which number. +/// +/// +/// How many commits the local revision has that the upstream one does not. +/// +/// +/// How many commits the upstream revision has that the local one does not. +/// +public readonly record struct GitDivergence(int Ahead, int Behind) +{ + /// + /// Gets a value indicating whether the two revisions name the same set of commits. + /// + /// when neither side holds a commit the other lacks. + public bool IsInSync => Ahead == 0 && Behind == 0; +} diff --git a/GitIntegration/Models/GitEnums.cs b/GitIntegration/Models/GitEnums.cs index 9de79b2..3b465a2 100644 --- a/GitIntegration/Models/GitEnums.cs +++ b/GitIntegration/Models/GitEnums.cs @@ -82,3 +82,95 @@ public enum GitUntrackedFilesMode /// Report every untracked file individually, including inside untracked directories. All, } + +/// +/// What state a submodule's working directory is in, relative to the commit the superproject +/// records for it. +/// +/// +/// The four states git submodule status distinguishes with its leading marker character. +/// +public enum GitSubmoduleState +{ + /// + /// Git reported a marker this library does not recognise. + /// + /// + /// Also the state of a gitlink the superproject records but that submodule status did not + /// report at all — a submodule removed from .gitmodules while its gitlink remains, for + /// instance. Reporting the entry with an unknown state beats dropping it, since a caller + /// deciding whether a directory is safe to remove needs to know the gitlink is still there. + /// + Unknown, + + /// The checked-out commit is the one the superproject records. + InSync, + + /// + /// A different commit is checked out than the one the superproject records. + /// + /// Git's + marker. The superproject's gitlink and the working directory disagree. + DifferentCommit, + + /// + /// The submodule is registered but not initialised, so its working directory holds no checkout. + /// + /// Git's - marker. + Uninitialised, + + /// The submodule has conflicting changes from an unfinished merge. + /// Git's U marker. + Conflicted, +} + +/// +/// Whether a verb should also operate on the working trees and refs of the repository's submodules. +/// +/// +/// The values fetch and pull accept for --recurse-submodules. Distinct from +/// , which spells a different set for a different question — see +/// that type's remarks. +/// +public enum GitSubmoduleRecursion +{ + /// Do not touch submodules at all. + No, + + /// Always recurse into every submodule. + Yes, + + /// + /// Recurse only into a submodule whose recorded commit the superproject's own update changed. + /// + OnDemand, +} + +/// +/// Whether push should verify or push the submodules' own commits before pushing the +/// superproject. +/// +/// +/// push --recurse-submodules spells the same flag name as fetch and pull but +/// means something else entirely, which is why it has its own type and its own method name. The +/// other verbs' flag asks "also operate on the submodules' working trees or refs". This one asks +/// what to do about the fact that a superproject commit referencing a submodule commit no remote +/// has is a commit nobody else can use — so it governs whether git checks for, or pushes, the +/// submodules' own commits to their own remotes first. +/// +public enum GitSubmodulePushCheck +{ + /// Push the superproject without considering the submodules at all. + No, + + /// + /// Refuse the push when any submodule commit the superproject references is missing from that + /// submodule's remote. + /// + Check, + + /// Push any such missing submodule commit first, then push the superproject. + OnDemand, + + /// Push the submodules' commits and stop, leaving the superproject unpushed. + Only, +} diff --git a/GitIntegration/Models/GitSubmodule.cs b/GitIntegration/Models/GitSubmodule.cs new file mode 100644 index 0000000..30499e1 --- /dev/null +++ b/GitIntegration/Models/GitSubmodule.cs @@ -0,0 +1,56 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitIntegration; + +using ktsu.Semantics.Paths; + +/// +/// One submodule of a repository. +/// +/// +/// A submodule is recorded in the superproject as a gitlink: a tree entry with mode +/// 160000 naming a path and a commit id, rather than a blob. What is actually checked out at +/// that path is a separate question, and the two can disagree — which is what +/// and the difference between and +/// describe. +/// +public sealed record GitSubmodule +{ + /// Gets the submodule's path, relative to the superproject's root. + public required RelativeDirectoryPath Path { get; init; } + + /// Gets the commit the superproject records for this submodule. + /// + /// The gitlink itself, read from the superproject's index. This is what a + /// submodule update would check out, and it does not change when someone commits inside + /// the submodule's working directory — is what moves then. + /// + public required GitCommitSha Sha { get; init; } + + /// + /// Gets the commit actually checked out in the submodule's working directory, or + /// when nothing is checked out. + /// + /// + /// Equal to when is + /// , and different when it is + /// . for an uninitialised + /// submodule, whose working directory holds no checkout to report — git prints the recorded + /// gitlink again in that case, which would otherwise make an uninitialised submodule look + /// indistinguishable from a synchronised one. + /// + public GitCommitSha? CheckedOutSha { get; init; } + + /// Gets how the working directory relates to the recorded commit. + public GitSubmoduleState State { get; init; } + + /// + /// Gets the reference expression git reports alongside the commit, such as heads/main or + /// a tag description, or when git reported none. + /// + /// + /// Git's own git describe-style suffix, purely descriptive. It is absent for an + /// uninitialised submodule, since there is nothing checked out to describe. + /// + public string? Describe { get; init; } +} diff --git a/GitIntegration/Models/GitTag.cs b/GitIntegration/Models/GitTag.cs new file mode 100644 index 0000000..f0e47ec --- /dev/null +++ b/GitIntegration/Models/GitTag.cs @@ -0,0 +1,53 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitIntegration; + +/// +/// One tag reference. +/// +/// +/// Git has two kinds of tag and this record describes both. A lightweight tag is a reference +/// pointing straight at a commit, exactly like a branch that never moves. An annotated tag is a +/// reference pointing at a tag object, which is a real object in the database carrying its +/// own author, date, and message, and which in turn points at the commit. and +/// are what tell the two apart by value; says it +/// directly. +/// +public sealed record GitTag +{ + /// Gets the short name, such as v1.2.3, without the refs/tags/ prefix. + public required GitTagName Name { get; init; } + + /// Gets the object id of the commit this tag ultimately names. + /// + /// The commit in both cases, which is almost always what a caller wants. For an annotated tag + /// this is the dereferenced target rather than the tag object's own id, so + /// Tags() and Branches() report the same kind of value in the same field and a + /// caller comparing a tag against a branch is comparing like with like. + /// + public required GitCommitSha Sha { get; init; } + + /// Gets the object id the reference itself points at. + /// + /// Equal to for a lightweight tag, and the tag object's own id for an + /// annotated one. Carried because it is the only way to address the tag object itself — to read + /// its tagger or its signature, say — and because deriving it later would need a second + /// invocation. + /// + public required GitCommitSha ObjectSha { get; init; } + + /// Gets a value indicating whether this tag is annotated rather than lightweight. + public bool IsAnnotated { get; init; } + + /// + /// Gets the annotated tag's message subject, or for a lightweight tag. + /// + /// + /// Null rather than the empty string for a lightweight tag, and that distinction is load-bearing. + /// Git's %(contents:subject) falls through to the commit's subject when the + /// reference names no tag object, so reporting the field verbatim would present a commit message + /// as though the tagger had written it. The parser gates this on the object type for exactly that + /// reason. + /// + public string? Message { get; init; } +} diff --git a/GitIntegration/Parsing/GitDiffParser.cs b/GitIntegration/Parsing/GitDiffParser.cs index 348ed47..328d0c0 100644 --- a/GitIntegration/Parsing/GitDiffParser.cs +++ b/GitIntegration/Parsing/GitDiffParser.cs @@ -7,7 +7,8 @@ namespace ktsu.GitIntegration; using System.Globalization; /// -/// Reads git diff --name-status -z. +/// Reads git diff --name-status -z, and git diff --raw --numstat -z when line counts +/// were asked for. /// /// /// The output is a stream of NUL-terminated tokens rather than fixed-size records: an ordinary @@ -41,44 +42,219 @@ internal static IReadOnlyList Parse(string output) continue; } - GitChangeKind kind = ToChangeKind(status[0]); - int? similarity = ReadSimilarity(status); + entries.Add(ReadEntry(tokens, ref index, status)); + } + + return entries; + } + + /// + /// Parses NUL-terminated --raw --numstat output, correlating the two sections. + /// + /// + /// + /// Asking for --name-status and --numstat together does not produce both: + /// they are display formats and the last one wins. --raw is the one that combines, and it + /// is a superset of --name-status for this library's purposes — the same change letter and + /// similarity score, plus the modes and blob object ids — so both sections come from one process + /// with no second invocation to keep consistent. + /// + /// + /// Under -z the two sections are emitted back to back with no delimiter between + /// them: the raw section's final path token runs straight into the numstat section's first + /// record. What tells them apart is the shape of a record's first token, and this is the trap + /// worth stating plainly — a raw record begins with :, and a numstat record begins with a + /// digit or -. That test is safe because it is only ever applied at a record boundary: a + /// path is consumed by the record that owns it and never reaches this check, so a file literally + /// named :something cannot be mistaken for a raw header. + /// + /// + /// Correlation between the sections is positional, because both list the same changes in the same + /// order. Matching on path string would be the fragile choice: a rename appears in the raw + /// section as two path tokens after the status, and in the numstat section as an empty + /// path field followed by two more tokens, so the two sections do not spell a rename the same way. + /// + /// + /// Every shape here was verified against git 2.43 and git 2.55. + /// + /// + /// Everything git wrote to standard output. + /// The changed paths with their line counts, in the order git listed them. + /// A record was malformed, or the two sections disagree. + internal static IReadOnlyList ParseWithLineCounts(string output) + { + Ensure.NotNull(output); + + string[] tokens = output.Split('\0'); + int index = 0; + + List entries = ReadRawSection(tokens, ref index); + List<(int? Insertions, int? Deletions)> counts = ReadNumstatSection(tokens, ref index); + + if (counts.Count != entries.Count) + { + // git emits one numstat record per raw record, so a mismatch means the section boundary + // was read wrongly rather than that a count is genuinely missing. Reporting nulls instead + // would present a parser bug as an absent measurement. + throw new GitParseException( + $"git reported {entries.Count} raw diff records but {counts.Count} numstat records."); + } + + for (int entry = 0; entry < entries.Count; entry++) + { + entries[entry] = entries[entry] with + { + Insertions = counts[entry].Insertions, + Deletions = counts[entry].Deletions, + }; + } + + return entries; + } - if (kind is GitChangeKind.Renamed or GitChangeKind.Copied) + /// + /// Reads the raw section, stopping at the first token that is not a raw header. + /// + /// Every NUL-separated token. + /// The read position, advanced past the raw section. + /// The entries the raw section describes, without line counts. + private static List ReadRawSection(string[] tokens, ref int index) + { + List entries = []; + + while (index < tokens.Length) + { + string header = tokens[index]; + + // Tokens are NUL-terminated, so the trailing element is empty. + if (header.Length == 0) + { + index++; + continue; + } + + if (header[0] != ':') + { + // The numstat section has begun. There is no delimiter to consume. + break; + } + + index++; + + // ": " — the status is the last + // space-separated field, and everything before it is metadata this library's model has no + // place for. Taking it from the end rather than by field index means a future git adding + // a field ahead of the status does not shift what is read. + int lastSpace = header.LastIndexOf(' '); + + if (lastSpace < 0 || lastSpace == header.Length - 1) + { + throw new GitParseException($"Malformed raw diff header: '{header}'."); + } + + entries.Add(ReadEntry(tokens, ref index, header[(lastSpace + 1)..])); + } + + return entries; + } + + /// + /// Reads the numstat section from the position the raw section stopped at. + /// + /// Every NUL-separated token. + /// The read position, advanced past the numstat section. + /// One insertion and deletion pair per record, in order. + private static List<(int? Insertions, int? Deletions)> ReadNumstatSection(string[] tokens, ref int index) + { + List<(int? Insertions, int? Deletions)> counts = []; + + while (index < tokens.Length) + { + string record = tokens[index++]; + + if (record.Length == 0) + { + continue; + } + + // "\t\t", or "\t\t" followed by two + // path tokens for a rename or a copy. The path is not read here at all: correlation is + // positional, so only the counts and the number of tokens each record consumes matter. + string[] fields = record.Split('\t'); + + if (fields.Length != 3) + { + throw new GitParseException($"Malformed numstat diff record: '{record}'."); + } + + // An empty path field is how -z spells a rename or a copy: the old and new paths follow + // as their own tokens. Without -z the same record uses the "old => new" arrow form, which + // is ambiguous against a filename containing " => " — which is why this builder asks for + // -z here as it already does everywhere else. + if (fields[2].Length == 0) { if (index + 1 >= tokens.Length) { throw new GitParseException( - $"A '{status}' diff record is missing its source or destination path."); + $"A renamed numstat record is missing its source or destination path: '{record}'."); } - string originalPath = tokens[index++]; + index += 2; + } + + counts.Add((ReadCount(fields[0]), ReadCount(fields[1]))); + } - entries.Add(new GitDiffEntry - { - Kind = kind, - Path = GitParseValues.ToRelativeFilePath(tokens[index++]), - OriginalPath = GitParseValues.ToRelativeFilePath(originalPath), - SimilarityPercent = similarity, - }); + return counts; + } + + /// + /// Reads one entry's paths, given the status token that introduced it. + /// + /// + /// Shared by both formats, because --raw spells a status exactly as + /// --name-status does once the leading metadata is stripped: the same change letter and + /// the same optional similarity score. + /// + /// Every NUL-separated token. + /// The read position, advanced past this entry's path or paths. + /// The status token, such as M or R075. + /// The entry, without line counts. + private static GitDiffEntry ReadEntry(string[] tokens, ref int index, string status) + { + GitChangeKind kind = ToChangeKind(status[0]); + int? similarity = ReadSimilarity(status); + + if (kind is GitChangeKind.Renamed or GitChangeKind.Copied) + { + if (index + 1 >= tokens.Length) + { + throw new GitParseException( + $"A '{status}' diff record is missing its source or destination path."); } - else + + string originalPath = tokens[index++]; + + return new GitDiffEntry { - if (index >= tokens.Length) - { - throw new GitParseException($"A '{status}' diff record is missing its path."); - } + Kind = kind, + Path = GitParseValues.ToRelativeFilePath(tokens[index++]), + OriginalPath = GitParseValues.ToRelativeFilePath(originalPath), + SimilarityPercent = similarity, + }; + } - entries.Add(new GitDiffEntry - { - Kind = kind, - Path = GitParseValues.ToRelativeFilePath(tokens[index++]), - SimilarityPercent = similarity, - }); - } + if (index >= tokens.Length) + { + throw new GitParseException($"A '{status}' diff record is missing its path."); } - return entries; + return new GitDiffEntry + { + Kind = kind, + Path = GitParseValues.ToRelativeFilePath(tokens[index++]), + SimilarityPercent = similarity, + }; } private static GitChangeKind ToChangeKind(char code) => code switch @@ -102,4 +278,27 @@ internal static IReadOnlyList Parse(string output) int.TryParse(status.AsSpan(1), NumberStyles.None, CultureInfo.InvariantCulture, out int score) ? score : null; + + /// + /// Reads one numstat count, which git writes as - for a binary file. + /// + /// + /// A binary change is not a zero-line change, which is the whole reason the counts are nullable + /// rather than defaulting to zero: reporting 0 here would state that nothing changed in a file + /// that git declined to measure. + /// + /// The count field as git printed it. + /// The count, or when git reported none. + /// The field is neither a whole number nor -. + private static int? ReadCount(string field) + { + if (string.Equals(field, "-", StringComparison.Ordinal)) + { + return null; + } + + return int.TryParse(field, NumberStyles.None, CultureInfo.InvariantCulture, out int count) + ? count + : throw new GitParseException($"A numstat count is neither a number nor '-': '{field}'."); + } } diff --git a/GitIntegration/Parsing/GitOutputFormats.cs b/GitIntegration/Parsing/GitOutputFormats.cs index 0ca4ddf..e310bb8 100644 --- a/GitIntegration/Parsing/GitOutputFormats.cs +++ b/GitIntegration/Parsing/GitOutputFormats.cs @@ -49,4 +49,34 @@ internal static class GitOutputFormats /// internal const string ForEachRefFormat = "%(refname)%1f%(refname:short)%1f%(objectname)%1f%(upstream:short)%1f%(HEAD)"; + + /// + /// The for-each-ref format for tags: short name, the reference's own object id, the + /// object type, the dereferenced object id, and the message subject. + /// + /// + /// + /// A separate format from rather than a widening of it, because + /// the two share only the name. A tag has no upstream and no current-branch marker, and a branch + /// has no dereferenced target — a single format carrying both sets would leave half its fields + /// empty on every record and invite a parser to guess which half it was reading. + /// + /// + /// %(objecttype) is what distinguishes the two kinds of tag: tag for an annotated + /// tag, whose reference points at a tag object, and commit for a lightweight one, whose + /// reference points straight at the commit. %(*objectname) dereferences a tag object to + /// the commit beneath it and is empty for a lightweight tag, so the commit id is the starred + /// field when it is populated and the plain one otherwise. + /// + /// + /// %(contents:subject) is the trap in this format. For an annotated tag it is the tag + /// message's subject, which is what it looks like it means. For a lightweight tag there is no tag + /// object to read, and git falls through to the commit's subject instead of leaving the + /// field empty — so a parser that reported it verbatim would present a commit message as the + /// tagger's words. gates it on the object type for that reason. + /// Verified against git 2.43. + /// + /// + internal const string ForEachTagFormat = + "%(refname:short)%1f%(objectname)%1f%(objecttype)%1f%(*objectname)%1f%(contents:subject)"; } diff --git a/GitIntegration/Parsing/GitParseValues.cs b/GitIntegration/Parsing/GitParseValues.cs index 6b0e125..0ad0ee9 100644 --- a/GitIntegration/Parsing/GitParseValues.cs +++ b/GitIntegration/Parsing/GitParseValues.cs @@ -64,4 +64,30 @@ internal static RelativeFilePath ToRelativeFilePath(string value) throw new GitParseException( $"git reported a path that cannot be represented as a relative file path: '{value}'."); } + + /// + /// Converts a raw path field into a repository-relative directory path. + /// + /// + /// The directory counterpart to , used for a submodule's path: + /// a submodule occupies a directory, and typing it as a file path would misdescribe it to every + /// caller that goes on to combine it with something. + /// + /// The raw path as git printed it. + /// The converted path. + /// + /// is empty or cannot be represented as a relative directory path. + /// + internal static RelativeDirectoryPath ToRelativeDirectoryPath(string value) + { + if (!string.IsNullOrEmpty(value) && + RelativeDirectoryPath.TryCreate(value, out RelativeDirectoryPath? path) && + path is not null) + { + return path; + } + + throw new GitParseException( + $"git reported a path that cannot be represented as a relative directory path: '{value}'."); + } } diff --git a/GitIntegration/Parsing/GitSubmoduleParser.cs b/GitIntegration/Parsing/GitSubmoduleParser.cs new file mode 100644 index 0000000..88b8038 --- /dev/null +++ b/GitIntegration/Parsing/GitSubmoduleParser.cs @@ -0,0 +1,260 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitIntegration; + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; + +using ktsu.Semantics.Paths; + +/// +/// Reads a repository's submodules from two invocations: git ls-files --stage -z for the +/// recorded gitlinks, and git submodule status for what is actually checked out. +/// +/// +/// +/// The split exists because neither command answers the whole question. ls-files is plumbing: +/// it is NUL-terminated, so a path may contain anything at all, and its mode field states outright +/// which entries are gitlinks. But it reports only what the superproject records — it +/// cannot say whether a submodule is initialised, or whether a different commit is checked out. +/// submodule status answers exactly that, but it is a shell wrapper rather than plumbing, so +/// its output carries no stability guarantee of the kind for-each-ref and +/// status --porcelain=v2 do, and — worse — it has no -z form at all. +/// +/// +/// That missing -z is the trap. A submodule status line is a marker character, an +/// object id, a space, the path, and then an optional trailing " (describe)". The +/// path is therefore not the last field, and a path containing " (" is genuinely ambiguous +/// with the describe suffix — there is no way to tell, from the line alone, where one ends and the +/// other begins. +/// +/// +/// This parser closes that ambiguity rather than documenting it, by never asking the question. The +/// set of paths is already known exactly, from ls-files, before a status line is read: so +/// instead of splitting a line to discover its path, each line's remainder is matched against the +/// paths already in hand. A path containing " (" matches itself and the describe suffix falls +/// out as whatever follows. The wrapper is consulted only for the marker and the checked-out object +/// id, which are unambiguous wherever the line came from. +/// +/// +internal static class GitSubmoduleParser +{ + /// The tree entry mode git uses for a gitlink, as opposed to a blob or a tree. + private const string GitlinkMode = "160000"; + + /// + /// Reads the recorded gitlinks from NUL-terminated ls-files --stage output. + /// + /// + /// Each record is <mode> <object> <stage>\t<path>. Everything that + /// is not a gitlink is skipped, which is most of a repository: this command lists the whole index, + /// and the mode filter is what turns that into a submodule listing. + /// + /// Everything git wrote to standard output. + /// One entry per gitlink, in the order git listed them, with no state resolved yet. + /// A gitlink record did not have the expected shape. + internal static IReadOnlyList ParseGitlinks(string output) + { + Ensure.NotNull(output); + + List submodules = []; + + foreach (string record in output.Split('\0')) + { + // Tokens are NUL-terminated, so the trailing element is empty. + if (record.Length == 0) + { + continue; + } + + int tab = record.IndexOf('\t', StringComparison.Ordinal); + + if (tab < 0) + { + throw new GitParseException($"Malformed ls-files record: '{record}'."); + } + + // The path is everything after the tab, which is why a path containing a space, or + // indeed a tab-free anything, is safe here: the split point is the first tab, and the + // three fields before it never contain one. + string[] fields = record[..tab].Split(' '); + + if (fields.Length != 3) + { + throw new GitParseException($"Malformed ls-files record: '{record}'."); + } + + if (!string.Equals(fields[0], GitlinkMode, StringComparison.Ordinal)) + { + continue; + } + + submodules.Add(new GitSubmodule + { + Path = GitParseValues.ToRelativeDirectoryPath(record[(tab + 1)..]), + Sha = GitParseValues.ToSemantic(fields[1], "submodule gitlink object id"), + State = GitSubmoduleState.Unknown, + }); + } + + return submodules; + } + + /// + /// Resolves each gitlink's working-directory state from submodule status output. + /// + /// + /// Matching is by known path rather than by parsing a path out of the line — see this class's own + /// remarks for why that is what makes the wrapper's output safe to read at all. A gitlink with no + /// matching status line keeps rather than being dropped: + /// a submodule removed from .gitmodules while its gitlink remains is exactly the case a + /// caller deciding whether a directory is safe to delete needs to see. + /// + /// The gitlinks read from ls-files. + /// Everything submodule status wrote to standard output. + /// The same submodules, with state, checked-out id, and describe filled in. + /// A status line did not have the expected shape. + internal static IReadOnlyList ApplyStatus(IReadOnlyList submodules, string output) + { + Ensure.NotNull(submodules); + Ensure.NotNull(output); + + Dictionary byPath = []; + + foreach (string record in output.Split('\n').Select(static line => line.TrimEnd('\r'))) + { + if (record.Length == 0) + { + continue; + } + + (string remainder, StatusLine parsed) = ParseStatusLine(record); + byPath[remainder] = parsed; + } + + List resolved = []; + + foreach (GitSubmodule submodule in submodules) + { + string path = ToGitSpelling(submodule.Path); + + // An exact match first: a submodule with no describe suffix leaves the remainder equal to + // the path itself. Otherwise the remainder is the path followed by " (describe)", and + // because the path is known the describe is simply what is left over — no guess about + // where the path ended is ever made. + if (byPath.TryGetValue(path, out StatusLine exact)) + { + resolved.Add(Apply(submodule, exact, describe: null)); + continue; + } + + // FirstOrDefault rather than a loop that breaks on its first iteration. Only one status + // line can describe a given path, so "the first match, if any" is the whole operation, and + // saying it directly is both clearer than a loop-and-break and free of the flag that + // carried the result out of one. + string prefix = path + " ("; + + KeyValuePair match = byPath.FirstOrDefault( + candidate => candidate.Key.StartsWith(prefix, StringComparison.Ordinal) && + candidate.Key.EndsWith(')', StringComparison.Ordinal)); + + // A no-match default leaves Key null, since Key is a string. The submodule is then kept + // with its Unknown state rather than dropped — see this method's remarks. + resolved.Add(match.Key is null + ? submodule + : Apply(submodule, match.Value, match.Key[prefix.Length..^1])); + } + + return resolved; + } + + /// + /// Splits one status line into its marker, its object id, and everything after them. + /// + /// + /// The remainder is deliberately left whole rather than split into a path and a describe: doing + /// that split here is precisely the ambiguity this parser avoids. The object id's length is found + /// rather than assumed, because a repository created with --object-format=sha256 emits + /// 64-character ids where the default emits 40. + /// + /// One line of submodule status output. + /// The remainder, paired with the marker and object id read from the line. + /// The line did not have the expected shape. + private static (string Remainder, StatusLine Parsed) ParseStatusLine(string record) + { + // Searched from index 1, not 0. The marker for a synchronised submodule is itself a space, so + // scanning from the start would find the marker rather than the separator after the object + // id, and every in-sync submodule would be misread. + int space = record.IndexOf(' ', 1); + + if (space < 2 || space == record.Length - 1) + { + throw new GitParseException($"Malformed submodule status line: '{record}'."); + } + + return ( + record[(space + 1)..], + new StatusLine(ToState(record[0]), record[1..space])); + } + + /// + /// Applies a matched status line to a gitlink entry. + /// + /// The gitlink read from ls-files. + /// The matched status line. + /// The describe expression, or when there was none. + /// The completed submodule. + private static GitSubmodule Apply(GitSubmodule submodule, StatusLine status, string? describe) => submodule with + { + State = status.State, + + // git prints the recorded gitlink again for an uninitialised submodule, which would make it + // indistinguishable from a synchronised one. Nothing is checked out there, so nothing is + // reported. + CheckedOutSha = status.State == GitSubmoduleState.Uninitialised + ? null + : GitParseValues.ToSemantic(status.ObjectId, "submodule checked-out object id"), + Describe = describe, + }; + + /// + /// Spells a path the way git prints it, so a status line can be matched against it. + /// + /// + /// git always prints a path with forward slashes, on every platform. + /// canonicalises to the platform's separator, so on Windows the same submodule is + /// libs\sub here and libs/sub in the status output, and every match would fail — + /// leaving every submodule on Windows and nowhere else. + /// The bug is invisible on a POSIX host, where the two spellings coincide. + /// + /// Converting rather than a literal backslash is what + /// makes this safe on POSIX too: there the separator is already /, so this is a no-op, and + /// a backslash that is a legitimate character in a POSIX filename is left alone rather than being + /// rewritten into a separator. + /// + /// + /// The canonicalised path read from ls-files. + /// The same path spelled as git prints it. + private static string ToGitSpelling(RelativeDirectoryPath path) => + path.WeakString.Replace(Path.DirectorySeparatorChar, '/'); + + private static GitSubmoduleState ToState(char marker) => marker switch + { + ' ' => GitSubmoduleState.InSync, + '+' => GitSubmoduleState.DifferentCommit, + '-' => GitSubmoduleState.Uninitialised, + 'U' => GitSubmoduleState.Conflicted, + + // Not a closed set the way the change-kind letters are: submodule status is a shell wrapper, + // and a marker it gains later should report the submodule with an unrecognised state rather + // than fail the whole listing. + _ => GitSubmoduleState.Unknown, + }; + + /// The parts of a status line this parser actually trusts. + /// The state its marker character names. + /// The object id it reports as checked out. + private readonly record struct StatusLine(GitSubmoduleState State, string ObjectId); +} diff --git a/GitIntegration/Parsing/GitTagParser.cs b/GitIntegration/Parsing/GitTagParser.cs new file mode 100644 index 0000000..37b2df1 --- /dev/null +++ b/GitIntegration/Parsing/GitTagParser.cs @@ -0,0 +1,82 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitIntegration; + +using System; +using System.Collections.Generic; +using System.Linq; + +/// +/// Reads git for-each-ref emitted with . +/// +/// +/// Records are newline-separated rather than NUL-separated, which is safe for the same reason it is +/// safe in : git forbids control characters in a reference name, so a +/// line break can never occur inside one. +/// +/// The message subject is the one field that is not a reference name and so is not covered by that +/// guarantee. It is the last field of the record, so an embedded unit separator can only extend it +/// rather than shift the field count — the same reasoning +/// records for a commit body. An embedded newline would +/// split the record, which is why the format asks for the message's subject rather than its +/// whole body: a subject is a single line by construction. +/// +/// +internal static class GitTagParser +{ + private const string AnnotatedObjectType = "tag"; + private const int FieldCount = 5; + + /// + /// Parses unit-separated tag records. + /// + /// Everything git wrote to standard output. + /// The tags, in the order git listed them. + /// A record did not have the expected shape. + internal static IReadOnlyList Parse(string output) + { + Ensure.NotNull(output); + + List tags = []; + + foreach (string record in output.Split('\n').Select(static line => line.TrimEnd('\r'))) + { + if (record.Length == 0) + { + continue; + } + + // Bounded so an embedded separator in the trailing subject is absorbed by that field + // rather than shifting the count, matching how GitLogParser bounds its own split. + string[] fields = record.Split(GitOutputFormats.UnitSeparator, FieldCount); + + if (fields.Length < FieldCount) + { + throw new GitParseException($"Malformed for-each-ref tag record: '{record}'."); + } + + bool isAnnotated = string.Equals(fields[2], AnnotatedObjectType, StringComparison.Ordinal); + + // %(*objectname) dereferences a tag object to the commit beneath it, and is empty when + // there is no tag object to dereference. Falling back to %(objectname) is what makes Sha + // mean "the commit this tag names" for both kinds of tag rather than only for one. + string commitSha = fields[3].Length == 0 ? fields[1] : fields[3]; + + tags.Add(new GitTag + { + Name = GitParseValues.ToSemantic(fields[0], "tag name"), + Sha = GitParseValues.ToSemantic(commitSha, "tag target object id"), + ObjectSha = GitParseValues.ToSemantic(fields[1], "tag object id"), + IsAnnotated = isAnnotated, + + // Gated on the object type rather than on emptiness. git falls through to the + // commit's own subject when the reference names no tag object, so a lightweight tag + // arrives here carrying text that reads like a tag message and is not one. Reporting + // null is the only honest answer for a tag that has no message. + Message = isAnnotated ? fields[4] : null, + }); + } + + return tags; + } +} diff --git a/GitIntegration/SemanticTypes/GitRefTypes.cs b/GitIntegration/SemanticTypes/GitRefTypes.cs index c83d83f..5d2878f 100644 --- a/GitIntegration/SemanticTypes/GitRefTypes.cs +++ b/GitIntegration/SemanticTypes/GitRefTypes.cs @@ -18,6 +18,17 @@ public sealed record GitBranchName : SemanticString { } [NotAnOption] public sealed record GitRemoteName : SemanticString { } +/// +/// A strongly-typed git tag name, such as v1.2.3. +/// +/// +/// The short name, without the refs/tags/ prefix, matching how +/// carries a branch's short name. +/// +[HasNonWhitespaceContent] +[NotAnOption] +public sealed record GitTagName : SemanticString { } + /// /// A strongly-typed git reference, which may be a branch, tag, SHA, or revision expression. /// diff --git a/GitIntegration/SemanticTypes/GitRepositoryTypes.cs b/GitIntegration/SemanticTypes/GitRepositoryTypes.cs index 15b9d1e..9e22bb0 100644 --- a/GitIntegration/SemanticTypes/GitRepositoryTypes.cs +++ b/GitIntegration/SemanticTypes/GitRepositoryTypes.cs @@ -7,7 +7,15 @@ namespace ktsu.GitIntegration; /// /// A strongly-typed repository name. /// +/// +/// One path segment, enforced by . Both hosting providers +/// substitute this value straight into a request path, and they escape it differently — Azure DevOps +/// runs over it, GitHub hands it to Octokit +/// unescaped — so a name carrying a separator would reshape the request on one host and not the +/// other. Validating here fixes both at the source. +/// [HasNonWhitespaceContent] +[IsSinglePathSegment] public sealed record GitRepositoryName : SemanticString { } /// @@ -23,3 +31,22 @@ public sealed record GitRepositoryWebURI : SemanticString { [HasNonWhitespaceContent] [NotAnOption] public sealed record GitRepositoryRemotePath : SemanticString { } + +/// +/// A strongly-typed identifier a host assigns to a repository, as distinct from its human-facing +/// name. +/// +/// +/// A string rather than a , because the two hosts do not agree on its +/// shape: Azure DevOps types its {repositoryId} path parameter as string (uuid), +/// while GitHub's repository id is a decimal number. The value is opaque to this library — it is +/// obtained from a host and handed back to that same host, never parsed — so a type that insisted +/// on one host's shape would simply exclude the other. +/// +/// Validated as a single path segment for the same reason is: it is +/// substituted directly into a request path. +/// +/// +[HasNonWhitespaceContent] +[IsSinglePathSegment] +public sealed record GitHostRepositoryId : SemanticString { } diff --git a/GitIntegration/SemanticTypes/IsSinglePathSegmentAttribute.cs b/GitIntegration/SemanticTypes/IsSinglePathSegmentAttribute.cs new file mode 100644 index 0000000..124c6e2 --- /dev/null +++ b/GitIntegration/SemanticTypes/IsSinglePathSegmentAttribute.cs @@ -0,0 +1,76 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitIntegration; + +using System; + +using ktsu.Semantics.Strings; + +/// +/// Validates that a value is a single path segment, so it cannot reshape a request path it is +/// substituted into. +/// +/// +/// +/// The hosting counterpart to . Where that attribute stops a +/// value being reinterpreted by git as a flag, this one stops a value being reinterpreted by +/// a URL or a filesystem as more than one segment. A repository name is substituted directly into +/// a request path by both providers, and the two escape it differently: +/// AzureDevOpsProvider runs over every path +/// component, while GitHubProvider hands the raw value to Octokit, which formats it into a +/// relative URI without escaping. Validating at construction fixes both sites at the source rather +/// than at each use, and removes the inconsistency between them. +/// +/// +/// A denylist of structural characters rather than an allowlist of permitted ones, because the two +/// hosts do not agree on what a name may contain: GitHub restricts names to +/// A-Z a-z 0-9 . _ -, while Azure DevOps permits spaces and a wider set besides. An +/// allowlist tight enough to describe GitHub would reject legitimate Azure DevOps repositories, and +/// one loose enough for Azure DevOps would not be an allowlist worth having. What both hosts agree +/// on is that a repository name is one segment, which is exactly what this rejects violations of. +/// +/// +/// Percent-encoded separators are deliberately not part of the check. Azure DevOps escapes the +/// value before it reaches a URL, so a %2F in a name arrives at the service as %252F +/// and addresses a repository literally named %2F rather than traversing anywhere; and +/// GitHub's own naming rules exclude % entirely, so no such name can name a real repository +/// there. Rejecting % here would therefore close no reachable path while risking a +/// legitimate name on a host this library has not surveyed. +/// +/// +[AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = true)] +public sealed class IsSinglePathSegmentAttribute : SemanticStringValidationAttribute +{ + /// + /// The characters that separate one path segment from the next, or a path from what follows it. + /// + /// + /// / and \ separate segments in a URL and on a filesystem respectively; ? + /// and # end the path entirely and begin a query string or a fragment, so a name carrying + /// one could append query parameters to a request rather than merely redirect it. + /// + private static readonly char[] SegmentSeparators = ['/', '\\', '?', '#']; + + /// + /// Validates that the supplied value names one path segment and no more. + /// + /// The semantic string to validate. + /// + /// when the value is non-empty, contains no path separator, and is + /// neither of the relative segments . and ..; otherwise, . + /// + public override bool Validate(ISemanticString semanticString) + { + string? value = semanticString?.WeakString; + + if (string.IsNullOrEmpty(value)) + { + return false; + } + + // "." and ".." are rejected as whole values rather than as substrings. A name such as + // "my..repo" carries no traversal — a relative segment only means anything when a separator + // delimits it, and every separator is already rejected above. + return value.IndexOfAny(SegmentSeparators) < 0 && value is not ("." or ".."); + } +} diff --git a/README.md b/README.md index 51ea9ad..8d442a5 100644 --- a/README.md +++ b/README.md @@ -38,11 +38,15 @@ builds and parses its requests by hand instead. `IsRepositoryAsync`, `OpenAsync`, `DiscoverAsync` — and creates new ones — `Init(...)`, `Clone(...)` — by delegating every invocation to `ktsu.RunCommand`. - **Fluent Verb Builders**: `GitRepository` exposes one builder per read-only verb — `Status()`, - `Log()`, `Diff()`, `Branches()`, `Remotes()`, `RevParse(...)` — and one per mutating verb — - `Add()`, `Commit(...)`, `CreateBranch(...)`, `DeleteBranch(...)`, `Checkout(...)`, - `AddRemote(...)`, `RemoveRemote(...)`, `SetRemoteUrl(...)`, `Fetch()`, `Pull()`, `Push()` — each - configurable via chained method calls and run with `ExecuteAsync` or the non-throwing - `TryExecuteAsync`. + `Log()`, `Diff()`, `Branches()`, `Tags()`, `Remotes()`, `Submodules()`, `RevParse(...)`, + `RevList(...)`, `Divergence(...)` — and one per mutating verb — `Add()`, `Commit(...)`, + `CreateBranch(...)`, `DeleteBranch(...)`, `CreateTag(...)`, `DeleteTag(...)`, `Checkout(...)`, + `AddRemote(...)`, `RemoveRemote(...)`, `SetRemoteUrl(...)`, `Fetch()`, `Pull()`, `Push()`, + `UpdateSubmodules()` — each configurable via chained method calls and run with `ExecuteAsync` or + the non-throwing `TryExecuteAsync`. +- **Tags and Submodules**: `Tags()`, `CreateTag(...)`, and `DeleteTag(...)` cover both lightweight + and annotated tags; `Submodules()` reports each submodule's recorded gitlink alongside what is + actually checked out, and `UpdateSubmodules()` checks the recorded commits out. - **Remote Sync**: `Fetch()` downloads objects and refs without touching the working tree, `Pull()` fetches and integrates into the current branch, and `Push()` sends local commits — `fetch` and `push` report a machine-readable, per-reference account of what happened via @@ -244,6 +248,137 @@ await repository.DeleteBranch("feature/new-thing".As()) .ExecuteAsync(); ``` +### Tagging a Release + +Git has two kinds of tag, and `GitTag` describes both. A lightweight tag points straight at a +commit; an annotated tag points at a tag object carrying its own tagger, date, and message. + +```csharp +using ktsu.GitIntegration; +using ktsu.Semantics.Strings; + +// Lightweight: just a reference. +await repository.CreateTag("v1.2.3".As()).ExecuteAsync(); + +// Annotated: the message is what makes it annotated, so the two are one call. +await repository.CreateTag("v1.3.0".As()) + .Annotating("Release 1.3.0".As()) + .At("main".As()) + .ExecuteAsync(); + +IReadOnlyList tags = await repository.Tags().ExecuteAsync(); + +foreach (GitTag tag in tags) +{ + // Sha is the commit the tag ultimately names, dereferenced through the tag object where there + // is one, so it is directly comparable with a GitBranch.Sha. ObjectSha is what the reference + // itself points at, which differs only for an annotated tag. + Console.WriteLine($"{tag.Name} -> {tag.Sha} ({(tag.IsAnnotated ? tag.Message : "lightweight")})"); +} + +await repository.DeleteTag("v1.2.3".As()).ExecuteAsync(); +``` + +### Inspecting and Updating Submodules + +`Submodules()` reports what the superproject *records* alongside what is actually checked out. The +two disagree whenever someone has committed inside a submodule without updating the gitlink. + +```csharp +using ktsu.GitIntegration; + +IReadOnlyList submodules = await repository.Submodules().ExecuteAsync(); + +foreach (GitSubmodule submodule in submodules) +{ + Console.WriteLine($"{submodule.Path}: {submodule.State}"); + + if (submodule.State == GitSubmoduleState.DifferentCommit) + { + Console.WriteLine($" recorded {submodule.Sha}, checked out {submodule.CheckedOutSha}"); + } +} + +// Check out the recorded commits, initialising any submodule added upstream since this clone. +await repository.UpdateSubmodules() + .Initialise() + .Recursive() + .ExecuteAsync(); +``` + +A failure here does **not** mean the repository is unchanged: the update walks the submodules in +turn, so one failing after others succeeded leaves a state that is neither the old nor the intended +new one. Ask `Submodules()` again to find out how far it got. + +### Checking for Work That Exists Nowhere Else + +Before discarding a working copy, the question worth asking is whether it holds commits no remote +has. + +```csharp +using ktsu.GitIntegration; + +IReadOnlyList atRisk = await repository.Log() + .IncludingAllRefs() + .ExcludingRemoteTrackingRefs() + .ExecuteAsync(); + +if (atRisk.Count > 0) +{ + Console.WriteLine($"{atRisk.Count} commit(s) exist only here:"); + + foreach (GitCommit commit in atRisk) + { + Console.WriteLine($" {commit.Sha} {commit.Subject}"); + } +} +``` + +`IncludingAllRefs()` emits `--all` rather than the narrower `--branches`, which would miss a commit +on a detached HEAD — the normal state of a submodule working directory, and exactly the case where a +wrong answer costs work. + +### Counting Commits Without Listing Them + +```csharp +using ktsu.GitIntegration; +using ktsu.Semantics.Strings; + +// One integer from git, rather than building a full GitCommit for every commit in the range. +int behindMain = await repository.RevList("HEAD..main".As()).ExecuteAsync(); + +// Ahead and behind for any two revisions, where GitStatus answers it only for HEAD against its +// own configured upstream. +GitDivergence divergence = await repository + .Divergence("origin/main".As(), "HEAD".As()) + .ExecuteAsync(); + +Console.WriteLine($"ahead {divergence.Ahead}, behind {divergence.Behind}"); +``` + +### Line Counts on a Diff + +```csharp +using ktsu.GitIntegration; +using ktsu.Semantics.Strings; + +IReadOnlyList entries = await repository.Diff() + .Between("HEAD~1".As(), "HEAD".As()) + .WithLineCounts() + .ExecuteAsync(); + +foreach (GitDiffEntry entry in entries) +{ + // Null rather than zero for a binary file: git declines to count one, and a binary change is + // not a zero-line change. + string counts = entry.Insertions is int added && entry.Deletions is int removed + ? $"+{added} -{removed}" + : "binary"; + + Console.WriteLine($"{entry.Kind} {entry.Path} {counts}"); +} +``` + ### Managing Remotes ```csharp @@ -544,16 +679,27 @@ IReadOnlyList arguments = repository.Status().BuildArguments(); ### Metadata-Only Repositories A `GitRepository` produced by a hosting provider (rather than `IGitClient.OpenAsync` or -`DiscoverAsync`) carries hosting metadata but no `ProcessRunner`. Calling any verb on it throws +`DiscoverAsync`) carries hosting metadata but neither a `ProcessRunner` nor a `LocalPath` — it +describes a repository that has never been cloned. Calling any verb on it throws `InvalidOperationException` immediately, rather than failing later inside git: ```csharp -GitRepository metadataOnly = new() { LocalPath = somePath, Name = "GitIntegration".As() }; +GitRepository metadataOnly = new() { Name = "GitIntegration".As() }; // Throws InvalidOperationException — obtain a runnable repository from IGitClient first. _ = metadataOnly.Status(); ``` +Cloning one needs a destination, since there is no local path to infer: + +```csharp +IGitCloneBuilder clone = client.Clone( + metadataOnly.RemotePath!, + "/src/GitIntegration".As()); + +GitRepository working = await clone.ExecuteAsync(); +``` + ## API Reference ### `IGitClient` / `GitClient` @@ -574,14 +720,15 @@ The entry point to the local layer: finds and opens repositories, and reports on ### `GitRepository` -Carries `LocalPath` plus optional hosting metadata, and exposes one builder factory per verb. +Carries an optional `LocalPath` plus optional hosting metadata, and exposes one builder factory per verb. #### Properties | Name | Type | Description | |------|------|-------------| -| `LocalPath` | `AbsoluteDirectoryPath` | The working tree's local filesystem path. | +| `LocalPath` | `AbsoluteDirectoryPath?` | The working tree's local filesystem path; `null` on a repository a hosting provider enumerated, which has never been cloned. | | `Name` | `GitRepositoryName?` | The repository name, when known. | +| `HostRepositoryId` | `GitHostRepositoryId?` | The host's own identifier, when known — what Azure DevOps's `{repositoryId}` path parameter is documented to take. | | `WebURI` | `GitRepositoryWebURI?` | The browser-facing URI, when known. | | `RemotePath` | `GitRepositoryRemotePath?` | The remote clone path, when known. | | `ProcessRunner` | `IGitProcessRunner?` | The runner this repository's verbs execute through; `null` on a metadata-only repository. | @@ -594,12 +741,18 @@ Carries `LocalPath` plus optional hosting metadata, and exposes one builder fact | `Log()` | `IGitLogBuilder` | Builds `git log -z` with this library's pinned format. | | `Diff()` | `IGitDiffBuilder` | Builds `git diff --name-status -z`. | | `Branches()` | `IGitBranchListBuilder` | Builds `git for-each-ref` over the branch namespaces. | +| `Tags()` | `IGitTagListBuilder` | Builds `git for-each-ref` over `refs/tags`. | | `Remotes()` | `IGitRemoteListBuilder` | Builds `git remote -v`. | +| `Submodules()` | `IGitSubmoduleListBuilder` | Reads `git ls-files --stage -z` for the recorded gitlinks and `git submodule status` for what is checked out. | | `RevParse(GitRefName)` | `IGitRevParseBuilder` | Builds `git rev-parse --verify` for a revision. | +| `RevList(GitRefName)` | `IGitRevListBuilder` | Builds `git rev-list --count`, counting commits without listing them. | +| `Divergence(GitRefName, GitRefName)` | `IGitRevListDivergenceBuilder` | Builds `git rev-list --count --left-right`, reporting ahead and behind for any two revisions. | | `Add()` | `IGitAddBuilder` | Builds `git add`. | | `Commit(GitCommitMessage)` | `IGitCommitBuilder` | Builds `git commit`, then reads the new commit back with `git log -1`. | | `CreateBranch(GitBranchName)` | `IGitBranchCreateBuilder` | Builds `git branch []`. | | `DeleteBranch(GitBranchName)` | `IGitBranchDeleteBuilder` | Builds `git branch --delete `. | +| `CreateTag(GitTagName)` | `IGitTagCreateBuilder` | Builds `git tag`, lightweight or annotated. | +| `DeleteTag(GitTagName)` | `IGitTagDeleteBuilder` | Builds `git tag --delete `. | | `Checkout(GitRefName)` | `IGitCheckoutBuilder` | Builds `git checkout`. | | `AddRemote(GitRemoteName, GitRepositoryRemotePath)` | `IGitRemoteAddBuilder` | Builds `git remote add `. | | `RemoveRemote(GitRemoteName)` | `IGitRemoteRemoveBuilder` | Builds `git remote remove `. | @@ -607,6 +760,7 @@ Carries `LocalPath` plus optional hosting metadata, and exposes one builder fact | `Fetch()` | `IGitFetchBuilder` | Builds `git fetch`, with `--porcelain` where the installed git supports it. | | `Pull()` | `IGitPullBuilder` | Builds `git pull`. | | `Push()` | `IGitPushBuilder` | Builds `git push --porcelain`. | +| `UpdateSubmodules()` | `IGitSubmoduleUpdateBuilder` | Builds `git submodule update`. | | `IsClonedAsync(CancellationToken)` | `Task` | Decides whether `LocalPath` currently holds a git working tree. | | `OpenWebClient()` | `void` | Opens `WebURI` in the default browser, when it is an absolute `http`/`https` URI. | @@ -627,24 +781,31 @@ The shared contract every verb builder implements. A builder is single-use and n | Interface | Extra Methods | Result | |-----------|----------------|--------| | `IGitStatusBuilder` | `WithUntrackedFiles(GitUntrackedFilesMode)`, `IncludeIgnored()` | `GitStatus` | -| `IGitLogBuilder` | `Take(int)`, `Skip(int)`, `ForRevision(GitRefName)`, `ForPath(RelativeFilePath)`, `FirstParentOnly()` | `IReadOnlyList` | -| `IGitDiffBuilder` | `Staged()`, `Against(GitRefName)`, `Between(GitRefName, GitRefName)`, `DetectRenames()`, `DetectCopies()`, `ForPath(RelativeFilePath)` | `IReadOnlyList` | +| `IGitLogBuilder` | `Take(int)`, `Skip(int)`, `ForRevision(GitRefName)`, `ForPath(RelativeFilePath)`, `FirstParentOnly()`, `IncludingAllRefs()`, `ExcludingRemoteTrackingRefs()` | `IReadOnlyList` | +| `IGitDiffBuilder` | `Staged()`, `Against(GitRefName)`, `Between(GitRefName, GitRefName)`, `DetectRenames()`, `DetectCopies()`, `ForPath(RelativeFilePath)`, `WithLineCounts()` | `IReadOnlyList` | | `IGitBranchListBuilder` | `LocalOnly()`, `RemoteOnly()` | `IReadOnlyList` | +| `IGitTagListBuilder` | *(none)* | `IReadOnlyList` | | `IGitRemoteListBuilder` | *(none)* | `IReadOnlyList` | +| `IGitSubmoduleListBuilder` | *(none)* | `IReadOnlyList` | | `IGitRevParseBuilder` | *(none — revision supplied via `GitRepository.RevParse`)* | `GitCommitSha` | +| `IGitRevListBuilder` | `FirstParentOnly()`, `ForPath(RelativeFilePath)` | `int` | +| `IGitRevListDivergenceBuilder` | *(none — revisions supplied via `GitRepository.Divergence`)* | `GitDivergence` | | `IGitInitBuilder` | `Bare()`, `WithInitialBranch(GitBranchName)` | `GitInitResult` | -| `IGitCloneBuilder` | `WithBranch(GitBranchName)`, `WithDepth(int)`, `Bare()`, `ReportingProgress(IProgress)` | `GitRepository` | +| `IGitCloneBuilder` | `WithBranch(GitBranchName)`, `WithDepth(int)`, `Bare()`, `RecursingSubmodules()`, `ReportingProgress(IProgress)` | `GitRepository` | | `IGitAddBuilder` | `ForPath(RelativeFilePath)`, `All()`, `UpdateTrackedOnly()` | `GitCompleted` | | `IGitCommitBuilder` | `WithBody(string)`, `AllowEmpty()`, `StageTrackedFiles()`, `WithAuthor(GitAuthorName, GitAuthorEmail)` | `GitCommit` | | `IGitBranchCreateBuilder` | `StartingAt(GitRefName)`, `Force()` | `GitCompleted` | | `IGitBranchDeleteBuilder` | `Force()` | `GitCompleted` | -| `IGitCheckoutBuilder` | `CreatingBranch()`, `Force()`, `Detach()` | `GitCompleted` | +| `IGitTagCreateBuilder` | `At(GitRefName)`, `Annotating(GitCommitMessage)`, `Force()` | `GitCompleted` | +| `IGitTagDeleteBuilder` | *(none)* | `GitCompleted` | +| `IGitCheckoutBuilder` | `CreatingBranch()`, `Force()`, `Detach()`, `RecursingSubmodules()` | `GitCompleted` | | `IGitRemoteAddBuilder` | `WithFetch()` | `GitCompleted` | | `IGitRemoteRemoveBuilder` | *(none)* | `GitCompleted` | | `IGitRemoteSetUrlBuilder` | `ForPushOnly()` | `GitCompleted` | -| `IGitFetchBuilder` | `FromRemote(GitRemoteName)`, `AllRemotes()`, `Prune()`, `WithTags()`, `WithDepth(int)`, `ReportingProgress(IProgress)` | `GitFetchResult` | -| `IGitPullBuilder` | `FromRemote(GitRemoteName)`, `WithBranch(GitBranchName)`, `FastForwardOnly()`, `Rebase()`, `Prune()`, `ReportingProgress(IProgress)` | `GitCompleted` | -| `IGitPushBuilder` | `ToRemote(GitRemoteName)`, `WithBranch(GitBranchName)`, `SettingUpstream()`, `Force()`, `ForceWithLease()`, `DeletingRemoteBranch()`, `DryRun()`, `ReportingProgress(IProgress)` | `GitPushResult` | +| `IGitFetchBuilder` | `FromRemote(GitRemoteName)`, `AllRemotes()`, `Prune()`, `WithTags()`, `WithDepth(int)`, `RecursingSubmodules(GitSubmoduleRecursion)`, `ReportingProgress(IProgress)` | `GitFetchResult` | +| `IGitPullBuilder` | `FromRemote(GitRemoteName)`, `WithBranch(GitBranchName)`, `FastForwardOnly()`, `Rebase()`, `Merge()`, `Prune()`, `RecursingSubmodules(GitSubmoduleRecursion)`, `ReportingProgress(IProgress)` | `GitCompleted` | +| `IGitPushBuilder` | `ToRemote(GitRemoteName)`, `WithBranch(GitBranchName)`, `SettingUpstream()`, `Force()`, `ForceWithLease()`, `DeletingRemoteBranch()`, `DryRun()`, `CheckingSubmodules(GitSubmodulePushCheck)`, `ReportingProgress(IProgress)` | `GitPushResult` | +| `IGitSubmoduleUpdateBuilder` | `Initialise()`, `Recursive()`, `FromRemote()`, `Force()`, `WithDepth(int)`, `ReportingProgress(IProgress)` | `GitCompleted` | ### Result and Execution Models diff --git a/docs/superpowers/specs/2026-08-19-gitintegration-v2-design.md b/docs/superpowers/specs/2026-08-19-gitintegration-v2-design.md index 466246c..9c190db 100644 --- a/docs/superpowers/specs/2026-08-19-gitintegration-v2-design.md +++ b/docs/superpowers/specs/2026-08-19-gitintegration-v2-design.md @@ -704,3 +704,36 @@ so a failing command can be copied out of a `GitCommandException` and run verbat 2. Add a working-directory parameter to `ktsu.RunCommand`, removing the need for `git -C`. 3. Consider an argv-aware overload on `ktsu.Essentials.ICommandExecutor`, after which a `ktsu.Essentials.CommandExecutors.RunCommand` package would be worth shipping. + +## Amendment — 2026-09-08: `tag` and `submodule` are no longer non-goals + +This document is left as it was written, as a record of what was decided on 2026-08-19. This note +records what has changed since, rather than editing the text above. + +The [Non-goals](#non-goals) section groups seven commands together: + +> Interactive or conflict-resolving commands: `merge`, `rebase`, `bisect`, `stash`, `worktree`, +> `submodule`, `tag`. Deferred to a later version. + +Two things about that entry have since proved wrong. + +**The grouping misdescribes two of its members.** `submodule` and `tag` are not interactive and do +not resolve conflicts. `git tag -l` and `git submodule status` are ordinary read-only queries with +machine-readable output, and `git tag ` and `git submodule update` are ordinary mutating verbs +with no interactive step. The grouping probably made both look more expensive to support than they +were: the five commands it genuinely describes — `merge`, `rebase`, `bisect`, `stash`, `worktree` — +remain non-goals for exactly the stated reason, and nothing here reopens them. + +**Both are now implemented.** `tag` landed with `Tags()`, `CreateTag(...)`, and `DeleteTag(...)`, +mirroring the branch builders. `submodule` landed with `Submodules()` and `UpdateSubmodules()`, +alongside `--recurse-submodules` on `clone`, `checkout`, `fetch`, `pull`, and `push`. + +`CLAUDE.md` separately listed submodule support under "deliberately out of scope, even later", +which contradicted "deferred to a later version" here. That list now names only `commit --amend`, +`add --force`, and `switch`, which remain out of scope. + +One design consequence worth recording, because it changes something this document's successors +should not have to rediscover: `git fetch` refuses `--porcelain` and `--recurse-submodules` +together, so a recursing fetch reports `GitFetchResult.DetailAvailable` as `false`. That is now the +second reason detail can be unavailable, alongside the git 2.41 threshold for `fetch --porcelain` +itself.