fix(vcs): select the root directory a clone command was given - #252
fix(vcs): select the root directory a clone command was given#252HarshMN2345 wants to merge 2 commits into
Conversation
generateCloneCommand writes the root directory into .git/info/sparse-checkout, where git matches it gitignore-style. A './' prefix therefore looks for a directory literally named '.', and the clone checks out an empty tree: pattern 'docs' -> docs pattern './docs' -> (nothing) pattern '.' -> (nothing) pattern './' -> (nothing) '.' and './' are the conventional spellings of the repository root, and a caller that reads a path from a directory picker or a config file routinely holds the './' form. The five adapters each tested for the root themselves, in two different shapes - '' or '0' on GitHub, Gitea and Origin, and ['', '0', '/'] on GitLab and Bitbucket - and none of them handled either. Resolve the pattern through normalizeRepositoryPath instead, which the contents APIs have used for these same sentinels since c2c60ae. The '0' test went with it: that was an empty() leftover, and a repository whose top-level directory is named '0' now checks out rather than silently cloning whole. The clone command needs no provider, so the shared unit base covers it for every adapter. Building one there also means the Gitea-family adapters must carry an endpoint, as every other operation on them already assumes.
|
| { | ||
| $command = $this->vcsAdapter->generateCloneCommand('owner', 'repo', 'main', Git::CLONE_TYPE_BRANCH, '/tmp/clone', $rootDirectory); | ||
|
|
||
| $this->assertStringContainsString(escapeshellarg($pattern), $command); |
There was a problem hiding this comment.
This test checks only that the shell-escaped pattern occurs somewhere in the generated command. It can therefore pass if the pattern is written to the wrong file, duplicated, or later overridden, and it never verifies the checkout tree that this regression actually broke. This violates the repository directive to test observable behavior instead of mirroring source or configuration. The requirement must be satisfied before merging by exercising the command against a local Git fixture and asserting the resulting files.
Context Used: Call out and harshly judge implementation-coupled ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/vcs/tests/Unit/Base.php
Line: 232
Comment:
**Test Mirrors Command Text**
This test checks only that the shell-escaped pattern occurs somewhere in the generated command. It can therefore pass if the pattern is written to the wrong file, duplicated, or later overridden, and it never verifies the checkout tree that this regression actually broke. This violates the repository directive to test observable behavior instead of mirroring source or configuration. The requirement must be satisfied before merging by exercising the command against a local Git fixture and asserting the resulting files.
**Context Used:** Call out and harshly judge implementation-coupled ... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Each adapter resolves the root directory through normalizeRepositoryPath and tests the result for the repository root itself, rather than calling a one-line wrapper around it. The test is an explicit === '' instead of ?:, since '0' is a real directory name and PHP treats it as falsy.
The bug
generateCloneCommand()writes the root directory straight into.git/info/sparse-checkout, where git matches it gitignore-style. A./prefix looks for a directory literally named., so the clone checks out nothing.Reproduced against real git — bare repo,
core.sparseCheckout, depth-1 pull:.and./are the conventional spellings of the repository root, and a caller reading a path from a directory picker or config file routinely holds the./form. The failure is silent at clone time and surfaces later as a missing entrypoint —npm ENOENT ... package.jsonand the like.Why it slipped through
Each adapter tested for the root itself, in two different shapes, and neither handled
.or./:GitHub.php:1192,Gitea.php:1060,Origin.php:894$rootDirectory === '' || $rootDirectory === '0'GitLab.php:981,Bitbucket.php:1508\in_array($rootDirectory, ['', '0', '/'], true)Meanwhile the package already answers this exact question correctly one file over:
Git::normalizeRepositoryPath()has resolved these sentinels for the contents APIs since c2c60ae ("Resolve path sentinels the same way on every provider … a caller passing'.','./'or'src//'gets the same answer everywhere"). It was applied to the 11 contents call sites and not to the one clone call site.The change
Each adapter resolves the root directory through
normalizeRepositoryPath()first, and its root test becomes=== ''. That test is explicit rather than?:, because'0'is a real directory name and PHP treats it as falsy.'0'goes with it. That was anempty()leftover — a repository whose top-level directory is named0was treated as root and cloned whole. It now checks out that directory. Behaviour change, deliberate, called out in case anyone disagrees.'/'still resolves to*on every adapter, so GitLab and Bitbucket keep their existing handling and the other three gain it.Tests
generateCloneCommandneeds no provider, so the shared unit base covers it for every adapter — 60 cases, 6 adapters, over'',.,./,/,docs,docs/,./docs,./docs/,./astro/starter, and0.Building a clone command in the shared base also means the Gitea-family adapters must carry an endpoint, as every other operation on them already assumes —
Gitea,GogsandForgejotest factories now callsetEndpoint().phpunit --testsuite unit: 145 passing.pint: clean.phpstan: no errors.bin/monorepo validate: all packages valid.Context
Found while tracing a root-directory bug in Appwrite (appwrite/appwrite#13574). Appwrite does not hit this today only because its builds worker canonicalizes one line before the call — other consumers are exposed.