Close the open issue backlog (17 issues) - #99
Merged
Merged
Conversation
…tics [patch] Closes #91. GitCheckoutBuilder let a caller chain CreatingBranch() and Detach() even though git refuses "-b" alongside "--detach" ("fatal: '--detach' cannot be used with '-b/-B/--orphan'", verified against git 2.43). The contradiction surfaced only as an opaque GitCommandException from a process that had already been spawned. AppendVerbArguments now throws InvalidOperationException, matching the guards GitFetchBuilder and GitPullBuilder already carry for their own equivalent pairs. Closes #90. GitPullBuilder overrode TryExecuteAsync to join standard error and standard output, but left ExecuteAsync on the base implementation, whose message is built from standard error alone. A non-conflict pull failure explained on standard output therefore reached one entry point in full and the other as "git exited with code N: " with nothing after the colon. Rather than duplicate the join into CreateException, GitCommandBuilder gains a protected virtual GetDiagnostic seam that both ExecuteAsync (through CreateException) and TryExecuteAsync read. A verb that relocates its diagnostic states that once and both entry points follow, so they cannot drift apart again. GitPullBuilder's TryExecuteAsync override disappears entirely, since the join was all it did. GitCommitBuilder had the identical divergence for "nothing to commit" and is fixed by the same seam. Also fixes a defect found while running the integration tier: Checkout could not run at all on git <= 2.43. Those releases strip "--end-of-options" only when PARSE_OPT_KEEP_DASHDASH is unset, and checkout sets exactly that flag, so the marker survived into checkout's operand list and was read as a pathspec. git 2.44 changed the condition to PARSE_OPT_KEEP_UNKNOWN_OPT, which is why CI never caught it — the runners ship newer git than Ubuntu 24.04 LTS's stock 2.43, whose failure looks like: error: pathspec '--end-of-options' did not match any file(s) known to git Checkout now 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 as a path. GitRefName's NotAnOptionAttribute remains the layer that keeps a dash-leading target out of the vector. This restores GitRoundTripTests.BranchCreateCheckoutAndDeleteRoundTripAsync, which fails on main in that environment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XbbZVAiHU2jXTumaKViB3W
Closes #86. GitProvider held one static SocketsHttpHandler that every subclass shared. That was equivalent to per-provider only by accident — AzureDevOpsProvider was its sole user and GitHubProvider already had its own — so adding a third provider would have silently enrolled it in Azure DevOps's connection pool. The field is gone, replaced by a private protected abstract DefaultHandler each provider implements with its own static, so the compiler now requires a new provider to say which transport is its. A reflection test pins the declaration site rather than one instance's value. Closes #83. GitHostingException and its four subtypes gain constructors carrying an inner exception alongside the provider, status, and body, and GitHubProvider.Translate passes the Octokit exception through on every arm. The hierarchy's own fields are enough to reproduce a failure by hand but not to explain one: ApiError.Errors is often the only place GitHub says what was wrong with a request, and Octokit's synthetic 404 carries no HttpResponse at all, so that failure previously reached a caller carrying only a message. AzureDevOpsProvider's DeserializeSuccessBody now keeps its JsonException the same way, and the comment saying it could not has gone with it. Closes #85. GitRepositoryName carried only [HasNonWhitespaceContent], so a name containing "/", "?", or ".." passed validation. Both providers substitute it straight into a request path and escape it differently — Azure DevOps runs Uri.EscapeDataString over it, GitHub hands it to Octokit unescaped — so such a name reshaped the request on one host and not the other. A new IsSinglePathSegmentAttribute rejects path separators and the relative segments at construction, fixing both sites at the source. It is a denylist of structural characters rather than an allowlist, because GitHub and Azure DevOps do not agree on what a name may contain and the only thing they do agree on is that it is one segment. Closes #87. FakeHttpMessageHandler._responses becomes a ConcurrentQueue for the reason its sibling _requests already was, read with TryDequeue since a Count check followed by a Dequeue is not atomic on a concurrent queue. An HTTP-date Retry-After is now pinned as yielding a null ResetsAt. The unreachable "fits neither collection" throw becomes an assertion: TryAddWithoutValidation skips validation entirely, and reaching that catch means the name was already accepted as a well-formed header token. Closes #88. ProviderName and StatusCode document what they carry under the analyzer-required constructors, StatusCode noting that 0 is not tellable from a real status by type. AzureDevOpsPullRequestCreateRequest.Description gains JsonIgnoreCondition.WhenWritingNull, matching Microsoft's documented sample, which omits the field rather than sending null. GitHubProvider's local remarks now state that they add to the interface's rather than restating them. The three validation blocks in GitPullRequestCreateBuilder and the Azure DevOps fixtures' retained Microsoft placeholders are recorded as decisions in the code. The GitHub repository fixture's three entries no longer share one node_id, and each now encodes its own repository id, the way the owner node_ids already did. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XbbZVAiHU2jXTumaKViB3W
… [major] Closes #82. GitRepository.LocalPath was required and non-nullable, so both hosting providers invented one under Environment.CurrentDirectory for a repository that had never been cloned. That made the same remote repository yield a different record depending on when it was enumerated, and it forced a containment guard — GitProvider.ToLocalRepositoryPath — to exist purely to keep a name from a remote API response from escaping that directory. LocalPath is now nullable, which is what is actually true of an enumerated repository, and the guard is gone with the reason for it. Every verb needs a path, so GitRepository gains RequireLocalPath alongside the existing RequireRunner; both properties are public init accessors, so a caller can supply one and not the other and each check stands on its own. RequireRunner is evaluated first everywhere, so a metadata-only repository still reports the runner. IGitClient.Clone(GitRepository) now reports a missing LocalPath the same way it already reported a missing RemotePath, rather than silently cloning into a process-state-dependent directory. Where the working copy goes is the caller's decision, and the two-argument overload is where they make it. Closes #84. AzureDevOpsProvider filled Azure DevOps's {repositoryId} path segment with the repository name, because IGitHostingProvider exposed no other identifier. Microsoft's reference types that parameter as an id, draws an explicit id-or-name distinction for the sibling project parameter, and withholds it here — a distinction stated where it applies reads as deliberate where it is withheld. It works today; working and being specified are different properties. GitRepository now carries HostRepositoryId, populated by both providers (Azure DevOps's uuid, GitHub's decimal id — hence a string, since the value is handed back to the host it came from and never parsed). IGitHostingProvider gains GetPullRequestsAsync(GitRepository) and CreatePullRequest(GitRepository) overloads, which address the repository by that id when one is known and fall back to the name when it is not. Both providers now implement one internal core taking the finished path segment, so the choice between an id and a name lives in exactly one place and the two overloads cannot answer the same question differently. Removing ToLocalRepositoryPath left a gap: GitRepositoryName is now validated as a single path segment, so a host reporting "/" or ".." produces a value the semantic type refuses, and calling As<T>() on a response directly would throw ArgumentException out of a public hosting method. A new GitProvider.ToHostValue — the hosting counterpart to GitParseValues.ToSemantic — reports such a value as GitHostingRequestException, keeping the documented failure surface intact, while an omitted optional field stays null rather than failing. BREAKING: LocalPath is no longer required and no longer non-nullable; IGitHostingProvider gains two members; a repository from a hosting provider no longer carries a usable LocalPath. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XbbZVAiHU2jXTumaKViB3W
Closes #89. Tags were named as deferred rather than ruled out in the v2 design doc, and the gap had grown conspicuous: GitFetchBuilder.WithTags, GitPushBuilder's refspec handling, and GitRefUpdateKind.TagUpdate all treat tags as first-class, and IGitCheckoutBuilder's own remarks say the target may be a tag — so the library could already fetch, report, and check out a tag, but a caller had no way to create, list, or delete one. The three builders mirror the branch ones exactly: GitTagListBuilder over for-each-ref refs/tags, GitTagCreateBuilder, GitTagDeleteBuilder, reached through GitRepository.Tags(), CreateTag(...), and DeleteTag(...) alongside Branches()/CreateBranch(...)/DeleteBranch(...). GitTagName joins GitBranchName and GitRemoteName in SemanticTypes. Three places the tag pattern deliberately departs from the branch one: Annotation. git has two kinds of tag, and the difference is real rather than cosmetic: a lightweight tag is a reference pointing straight at a commit, while an annotated tag points at a tag object carrying its own tagger, date, and message. Annotating(GitCommitMessage) sets both --annotate and --message together, because either alone is a different command — --annotate with no message opens an editor no invocation this library makes could answer, and --message alone relies on a side effect git documents rather than promises. The model carries both object ids. Sha is the commit the tag ultimately names, dereferenced through the tag object where there is one, so a tag and a branch report the same kind of value in the same field. ObjectSha is what the reference itself points at, which is the only way to address the tag object and would otherwise need a second invocation to recover. Message is null for a lightweight tag, and that gate is load-bearing rather than tidiness. git's %(contents:subject) falls through to the *commit's* subject when a reference names no tag object, so reporting the field verbatim would present a commit message as though the tagger had written it. Verified against git 2.43 and pinned by a test using a real repository whose commit subject differs from every tag message. There is no Force() on the delete builder: git has no unmerged-tag concept to refuse over, so git tag --delete takes no force flag at all. The integration tier gains a tag round trip and a second test covering the ambiguity the trailing "--" in Checkout now resolves — a tag whose name also matches a path on disk resolves to the tag rather than silently restoring the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XbbZVAiHU2jXTumaKViB3W
…minor] Closes #95. IGitLogBuilder gains IncludingAllRefs() and ExcludingRemoteTrackingRefs(), which together answer "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. The existing surface could not express it and correctly refused to: GitRefName's NotAnOptionAttribute rejects a leading dash, and AppendOperands emits --end-of-options before caller-supplied operands. Both behaviours are right, so these three tokens belong in the builder as options. The argument order is the whole correctness question, and it turned out to need more than the two flags in sequence. Three behaviours of git 2.43, each verified against a real repository: --not negates every revision specifier that follows it. So --all must precede it, or "--not --remotes --all" excludes every reference instead of including them — and reports an empty log rather than failing, so nothing else would catch it. --not stays in effect until the next --not. So a revision from ForRevision or a pathspec from ForPath would otherwise fall inside the negation: "--not --remotes <revision>" asks for commits in neither, a different question that quietly answers zero, which looks exactly like a correct "nothing unpushed". The builder emits a closing --not that scopes the negation to --remotes alone, so nothing emitted below can be captured by it, now or after a future option is added there. --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 likewise load-bearing: --branches covers only refs/heads and misses a detached HEAD, which is the normal state of a submodule working directory. A check built on it would report "nothing unpushed" for a submodule holding commits that exist nowhere else, the exact case where a wrong answer causes data loss. Closes #96. GitRepository.RevList(revision) counts commits with rev-list --count instead of listing them through Log() and reading Count, which builds a full GitCommit — both signatures, subject, body, parents — for every commit to produce one integer, and which can raise GitParseException on a record a counting query has no reason to be able to fail over. FirstParentOnly() and ForPath() mirror IGitLogBuilder. Divergence(upstream, local) is a separate builder with its own result type rather than an option that changes what ExecuteAsync returns. It runs rev-list --count --left-right with a three-dot expression and reports GitDivergence(Ahead, Behind) — GitStatus's vocabulary rather than git's positional left-and-right, so a caller comparing it against a GitStatus is comparing like with like. The left-to-Behind mapping is fixed in the parser because it is easy to get backwards, and the integration test cross-checks it against GitStatus, which reaches the same numbers by a different route. The closing --not was mutation-checked by substitution: replacing it with a duplicate --remotes makes the ForRevision integration test fail, and the tree was re-verified clean after reverting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XbbZVAiHU2jXTumaKViB3W
Closes #97. GitDiffEntry carried which paths changed and how, but not how much: GitDiffBuilder built --name-status, which reports no line counts, so there was no way to ask this library how many lines a change touched. IGitDiffBuilder.WithLineCounts() switches the command from --name-status -z to --raw --numstat -z and fills the new nullable Insertions and Deletions. Opt-in rather than always on, so the default command and its output volume are unchanged for callers that only want the path list. --raw rather than --name-status 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 combines, and it is a superset of --name-status here — the same change letter and similarity score, plus modes and blob ids the parser ignores — so the whole result still comes from one process. Three things about the -z output the parser has to get right, each verified against git 2.43 and cross-checked against the shapes reported in the issue from 2.55: The two sections are emitted back to back with no delimiter. The raw section's final path token runs straight into the numstat section's first record. What separates them is the shape of a record's first token — a raw record begins with ":", a numstat record with a digit or "-" — and that test is only ever applied at a record boundary, so a file named ":weird" is consumed by the record that owns it and never mistaken for a header. 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. Correlation is therefore positional, since matching on path string would break on exactly that case. A binary file gets "-" for both counts rather than a number, which is why the counts are nullable. A binary change is not a zero-line change, and reporting 0 would state that nothing changed in a file git declined to measure. A mode-only change, by contrast, genuinely is 0 and 0. A count mismatch between the sections throws rather than reporting nulls: git emits one numstat record per raw record, so a mismatch means the section boundary was read wrongly, and reporting nulls would present a parser bug as an absent measurement. Counting how many files changed still needs none of this — the result holds one entry per path, so Count is the file count. TemporaryRepository gains DeleteFile so the integration tests can exercise the deletion path through Add().All() rather than through a second git command. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XbbZVAiHU2jXTumaKViB3W
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Closes #92, #93, #94. All three assume submodules are in scope for the local layer, which reverses CLAUDE.md's "out of scope, even later" wording — see the documentation change that follows for the reconciliation #98 asks for. #92, listing. GitRepository.Submodules() reads two commands, for the same reason commit runs git twice: neither answers the whole question. ls-files --stage -z 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. The issue flagged submodule status's ambiguity — no -z form, and the path is not the last field because an optional " (describe)" may follow it, so a path containing " (" cannot be split out of the line. This parser closes that rather than documenting it, by never asking the question: the exact set of paths is already known from ls-files before any status line is read, so each line's remainder is matched against paths already in hand and the describe is whatever is left over. A submodule at "libs/sub (old)" parses correctly, and there is a test for exactly that. GitSubmodule carries both object ids, because the two commands genuinely report different ones when a submodule has moved: Sha is the recorded gitlink, CheckedOutSha is what the working directory holds. CheckedOutSha is null for an uninitialised submodule, since git prints the recorded gitlink again on that line and reporting it verbatim would make an uninitialised submodule indistinguishable from a synchronised one. Two subtleties the integration tests caught. The marker for a synchronised submodule is a leading space, so the status output must not be trimmed — GitTextBuilder's contract is trimmed output, which is right for the probes it serves and would silently delete this field, so the status invocation has its own builder. And the separator scan starts at index 1 for the same reason: scanning from zero finds the marker rather than the separator after the object id. Recursion is deliberately not offered. ls-files enumerates only the superproject's index, so nested paths would have to come from the wrapper — giving up, for nested entries only, the exact property this verb is built on. A caller recurses by composition instead, opening each submodule as its own GitRepository, which is exact at every level and reuses this code. #93, update. Returns GitCompleted, since everything git submodule update prints is human prose with no porcelain form, and this design forbids parsing that for every other verb. The builder's remarks state plainly that a failure does not mean the repository is unchanged: the verb walks submodules in turn, so one failing after others succeeded leaves a state that is neither the old nor the intended new one, and the natural reading of an exception is "nothing happened". #94, the flags. Clone and checkout take a plain flag; fetch and pull take GitSubmoduleRecursion; push takes GitSubmodulePushCheck through a deliberately different method name, CheckingSubmodules, because git spells the same flag there for an unrelated question — whether to verify or push the submodules' own commits — and one word on two behaviours is the easiest thing for a caller to get wrong. checkout's remarks say plainly that its flag can destroy work, which the other four cannot. The issue asked whether fetch --porcelain still behaves with --recurse-submodules. It does not: git refuses them 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. The caller's request wins and --porcelain is dropped, with GitFetchResult.DetailAvailable false and Updates empty, exactly as it already reports the git 2.41 version threshold. That is a second reason detail can be unavailable, and it degrades rather than throwing because the caller did nothing contradictory. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XbbZVAiHU2jXTumaKViB3W
Closes #98. Two documents disagreed about whether submodule support was ever coming: the v2 design doc called `submodule` and `tag` "deferred to a later version", while CLAUDE.md listed submodule support under "deliberately out of scope, even later". CLAUDE.md is the file an agent reads first and acts on, so the disagreement was load-bearing rather than cosmetic. CLAUDE.md's out-of-scope list now names only `commit --amend`, `add --force`, and `switch`. Submodule support is stated as in scope and implemented, with a note that `git submodule add` is deliberately not wrapped — the verbs exist to inspect and update submodules, not to create them. The design doc is left as it was written and carries a dated amendment instead, since a historical design record is worth more unedited. The amendment also corrects the category error the issue identified: `submodule` and `tag` were grouped under "interactive or conflict-resolving commands", which neither is — `git tag -l` and `git submodule status` are ordinary read-only queries with machine-readable output. That grouping is probably what made both look more expensive to support than they were. The five commands it genuinely describes remain non-goals. Key Files, the models list, the parsers list, and the semantic types list all gain the new members. The architecture section gains five points, each for a decision that would otherwise have to be rediscovered: 9. why `checkout` is the one verb not using `--end-of-options` 10. the `GetDiagnostic` seam that keeps a verb's two entry points agreeing 11. how `Submodules()` reads a wrapper's ambiguous output safely 12. why `ExcludingRemoteTrackingRefs()` emits a closed `--not --remotes --not` 13. why `WithLineCounts()` switches format rather than adding a flag Point 7 is updated as the issue asked: the git 2.41 threshold is no longer the only reason `GitFetchResult.DetailAvailable` can be false. Recursing into submodules is the second, because git refuses `--porcelain` and `--recurse-submodules` together. Code reading that flag must not assume one cause. The list's introduction said "Two" while listing eight; it now says what it means. The hosting section gains three notes made necessary by the changes closing #82, #84, #85, and #86: that a provider-enumerated repository carries no `LocalPath` and is addressed by the host's own id, that each provider declares its own transport rather than inheriting one, and that a field a host reported goes through `ToHostValue` rather than `As<T>()` directly. README.md is brought up to date alongside: the features list, both API reference tables, the metadata-only section (which now also has to explain the missing `LocalPath`), and five new usage sections covering tags, submodules, the unpushed-commit query, commit counting, and diff line counts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XbbZVAiHU2jXTumaKViB3W
matt-edmondson
changed the title
Fixes the `Test on windows-latest` failure on ccc1ead, and a production bug of the same root cause that the failure exposed before CI could reach it. The visible failure was a test assumption: GitRevListBuilderTests.PutsAPathspecAfterTheRevision compared the emitted pathspec against the literal "src/a.txt". RelativeFilePath canonicalises to the platform's separator, so the vector carries "src\a.txt" on Windows. It now compares against the canonicalised form, which is what the repository's own GitLogBuilderTests has always done for exactly this reason. The real bug is in GitSubmoduleParser. ApplyStatus matched `submodule.Path.WeakString` — canonicalised, so "libs\sub" on Windows — against `git submodule status` output, which spells paths with forward slashes on every platform. The match therefore failed on Windows and only there, leaving every submodule reported as Unknown: no state, no CheckedOutSha, no Describe. Nothing in a POSIX run could catch it, and CI had not yet reached the commit that introduced it. Paths are now converted to git's spelling before matching, via Path.DirectorySeparatorChar rather than a literal backslash. That makes the conversion a no-op on POSIX, where the separator is already "/", so a backslash that is a legitimate character in a POSIX filename is left alone rather than being rewritten into a separator. The accompanying test uses a nested path so the separator is actually present. It is trivially satisfied on POSIX and load-bearing on Windows, which its comment says rather than implying the test proves more than it can. Also addresses both review findings from github-code-quality: TemporaryRepository combined RootPath with a caller-supplied relative path directly, and Path.Combine silently discards every earlier argument when a later one is rooted. For WriteFile that would write outside the fixture; for DeleteFile — added in this PR — it would *delete* outside it. Both now go through CombineUnderRoot, which rejects a rooted path. This is the same class of silent failure the library used to guard against in GitProvider for a repository name a host reported. GitTagParser projects the split sequence with Select rather than remapping the iteration variable inside the loop, as flagged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XbbZVAiHU2jXTumaKViB3W
Four findings on the submodule code, all local and behaviour-preserving. GitSubmoduleParser.ApplyStatus projects the split sequence with Select rather than remapping the iteration variable inside the loop, matching the change already made to GitTagParser, and filters the byPath scan with Where rather than an in-loop predicate. First-match behaviour, the matched flag, the describe extraction, and the no-match fallback are all unchanged. GitSubmoduleTests uses Path.Join rather than Path.Combine when appending segments to the fixture root — Combine resets to a later segment if one is ever rooted, which is not the intent here — and passes a single relative literal to WriteFile, which combines it under the fixture root itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XbbZVAiHU2jXTumaKViB3W
The third github-code-quality finding on this file, and this one landed on the Path.Combine inside the CombineUnderRoot guard added for the previous one — the analyzer cannot see the Path.IsPathRooted check directly above it. Rather than suppress a finding the analyzer is right to make from where it sits, the call becomes Path.Join, which is the API that means "append these segments" and has no discarding behaviour to guard against at all. The escape is now impossible by construction rather than merely checked for. This is the same fix the bot itself recommended for GitSubmoduleTests, applied to the root cause here: using Combine where Join expresses the intent. The explicit rejection is kept above it. A rooted path reaching this helper is a mistake in the calling test worth naming, and silently nesting it under the fixture root would hide that. This repository carries zero suppression attributes, deliberately, and this change keeps it that way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XbbZVAiHU2jXTumaKViB3W
…oop [patch] SonarCloud's quality gate failed on eaca0ea with a C Reliability Rating: csharpsquid:S1751 (Major, Bug) at GitSubmoduleParser.cs:163 — "Refactor the containing loop to do more than one iteration." Self-inflicted, and worth recording why. Applying github-code-quality's Where suggestion moved the predicate out of the loop body, which left the break unconditional — so the loop provably executed at most once, which is exactly what S1751 detects. The two analyzers were pulling in opposite directions: one wanted the filter expressed as a sequence operation, the other objects to a loop that cannot iterate. FirstOrDefault satisfies both, and is the better expression regardless. Only one status line can describe a given path, so "the first match, if any" is the whole operation; saying it directly is clearer than a loop-and-break and removes the matched flag that existed only to carry the result out of one. A no-match default leaves Key null, since Key is a string, and the submodule is then kept with its Unknown state rather than dropped — the behaviour ApplyStatus's remarks already describe. Both branches stay covered by ReadsAPathContainingTheDescribeDelimiterAsync and KeepsAGitlinkWithNoStatusLineAsUnknownAsync. No suppression, keeping this repository's zero-suppression record intact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XbbZVAiHU2jXTumaKViB3W
|
This was referenced Sep 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes every open issue: #82, #83, #84, #85, #86, #87, #88, #89, #90, #91, #92, #93, #94, #95, #96, #97, #98.
Twelve commits: seven of substance, then five made in response to CI and the review bots (summarised at the end). Each is reviewable on its own.
Bugs
GitCheckoutBuilderlet a caller chainCreatingBranch()andDetach()even though git refuses-balongside--detach(fatal: '--detach' cannot be used with '-b/-B/--orphan', verified against git 2.43).AppendVerbArgumentsnow throws, matching the guardsGitFetchBuilderandGitPullBuilderalready carry.GitPullBuilderjoined stdout and stderr inTryExecuteAsyncbut leftExecuteAsyncon the base implementation, so a non-conflict failure explained on stdout reached one entry point in full and the other asgit exited with code N:with nothing after the colon. Rather than duplicate the join,GitCommandBuildergains aGetDiagnosticseam both entry points read — so a verb that relocates its diagnostic states that once and they cannot drift again.GitPullBuilder'sTryExecuteAsyncoverride disappears entirely, andGitCommitBuilderhad the identical divergence, fixed by the same seam.Hosting layer
SocketsHttpHandlermoves offGitProvideronto each provider, behind aprivate protected abstractthe compiler now requires a new provider to answer. The base field was equivalent to per-provider only by accident.GitHubProvider.Translatepasses the Octokit exception through on every arm. Octokit's synthetic 404 carries noHttpResponse, so that failure previously reached a caller carrying only a message.IsSinglePathSegmentAttributestops aGitRepositoryNamecontaining/,?, or..reshaping a request path, closing the inconsistency between Azure DevOps (which escapes) and GitHub (which does not)._responsesbecomes aConcurrentQueueread withTryDequeue; an HTTP-dateRetry-Afteris pinned; the unreachable header-routing throw becomes an assertion.JsonIgnoreCondition.WhenWritingNullon the Azure DevOps create request'sDescription, and distinctnode_ids in the GitHub fixture.Breaking (major)
GitRepository.LocalPathis nullable, so providers stop inventing one underEnvironment.CurrentDirectory.GitProvider.ToLocalRepositoryPathis gone with the reason for it, andClone(GitRepository)now reports a missingLocalPathrather than silently cloning into a process-state-dependent directory.GitRepository.HostRepositoryIdcarries the host's own identifier, and newGitRepository-taking overloads address a repository the way its host documents. Both providers implement one internal core taking the finished path segment, so the id-or-name choice lives in one place.Features
Tags(),CreateTag(...),DeleteTag(...).GitTagcarries both object ids, andMessageis null for a lightweight tag — git's%(contents:subject)falls through to the commit's subject when there is no tag object, so reporting it verbatim would present a commit message as the tagger's words.Submodules(),UpdateSubmodules(), and--recurse-submoduleson the five verbs that accept it, withpushgiven its own method name and enum because git spells the same flag there for an unrelated question.IncludingAllRefs()andExcludingRemoteTrackingRefs()onLog().RevList(...)andDivergence(...), counting commits without building aGitCommitfor each.Insertions/DeletionsonGitDiffEntryviaDiff().WithLineCounts().CLAUDE.mdand a refreshedREADME.md.Three findings from running against real git
Each changed the implementation, so they are worth calling out rather than leaving in the commits.
Checkoutwas broken on git ≤ 2.43. Not filed as an issue — found becauseGitRoundTripTests.BranchCreateCheckoutAndDeleteRoundTripAsyncfails onmainin this environment:Those releases strip
--end-of-optionsonly whenPARSE_OPT_KEEP_DASHDASHis unset, andgit checkoutsets exactly that flag, so the marker survived into checkout's operand list and was read as a pathspec. git 2.44 changed the condition toPARSE_OPT_KEEP_UNKNOWN_OPT— which is why CI never caught it, the runners shipping newer git than Ubuntu 24.04 LTS's stock 2.43. Checkout now emits a trailing--, 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.git fetchrefuses--porcelainwith--recurse-submodules. #94 asked this to be verified rather than assumed, and the answer is that git rejects the pair outright. Emitting both would turn every recursing fetch into a failure, so--porcelainis dropped andGitFetchResult.DetailAvailablereports false — a second reason detail can be unavailable alongside the git 2.41 threshold, whichCLAUDE.mdarchitecture point 7 now says.--notneeds closing. #95 suggested emitting--allthen--not --remotesin that order. That is necessary but not sufficient:--notstays in effect until the next--not, so a revision fromForRevisionwould fall inside the negation and the query would quietly answer zero — indistinguishable from a correct "nothing unpushed". The builder emits a closed--not --remotes --nottriple. Mutation-checked by substitution (replacing the closing--notwith a duplicate--remotesmakes the integration test fail), with the tree re-verified clean afterwards.Design decisions worth a reviewer's attention
ls-files --stage -zsupplies paths and gitlinks (NUL-terminated, unambiguous);submodule statussupplies state. The wrapper's(describe)ambiguity is closed rather than documented, because the exact path set is already known before a status line is read, so each line's remainder is matched against paths in hand. A submodule atlibs/sub (old)parses correctly, and there is a test for it.ls-filesenumerates only the superproject's index — recursion would mean giving up that exactness for nested entries. Callers recurse by composition instead.GitDivergenceusesGitStatus's ahead/behind vocabulary rather than git's positional left-and-right, so the two are directly comparable. The integration test cross-checks them against each other.Fixed during review
Three problems surfaced after the first push. Recording them because each has a different character and the last two were self-inflicted.
A Windows-only production bug in the submodule parser.
ApplyStatusmatchedsubmodule.Path.WeakString— canonicalised, solibs\subon Windows — againstgit submodule statusoutput, which spells paths with forward slashes on every platform. Every submodule reportedUnknownon Windows and nowhere else; no POSIX run could catch it. The Windows job confirmed it exactly, withexpected: Uninitialised / actual: Unknownandexpected: DifferentCommit / actual: Unknown. Paths now convert to git's spelling viaPath.DirectorySeparatorChar, which is a no-op on POSIX and so leaves a backslash that is a legitimate character in a POSIX filename alone.Test assertions that assumed a forward slash. Same root cause, on the assertion side. These now compare against the canonicalised form, which is what
GitLogBuilderTestshas always done.An S1751 bug introduced while applying review feedback. Applying
github-code-quality'sWheresuggestion moved the predicate out of the loop body, leaving thebreakunconditional — so the loop provably ran at most once, which SonarCloud flags as a Major bug and which dropped the quality gate to a C Reliability Rating.FirstOrDefaultsatisfies both analyzers and is the better expression anyway: only one status line can describe a given path, so "the first match, if any" is the whole operation.No suppressions were added; this repository's zero-
[SuppressMessage]record is intact.Testing
dotnet test— 576 passing, 0 failing (up from 484 passing / 1 failing onmainin this environment).Integration tier verified separately with
KTSU_GIT_INTEGRATION_TESTS_REQUIRED=1 GIT_CONFIG_NOSYSTEM=1— 29 passing, including new real-git coverage for tags, submodules in every state, the unpushed-commit query, rev-list, and diff line counts.CI is green on windows-latest, ubuntu-latest, and macos-latest, with SonarCloud reporting 94.6% coverage on new code, 0 duplication, and 0 security hotspots.
🤖 Generated with Claude Code
https://claude.ai/code/session_01XbbZVAiHU2jXTumaKViB3W