Support git commands run in submodules#2090
Conversation
|
You've broken all tests that use the function you modified. |
|
You are certainly right! Tests are failing on the When trying this locally, I do have I was debating adding a warning earlier, and so should include it now. I am usually programming in python or bash, not nodejs. But we need this feature, as end-users. The issue has been open multiple years, and it seems like fun to contribute. Your collaboration is more than welcome. Do you have ideas? I will try to investigate the errors. |
|
If you want to contribute to a project, you first need to understand how it works, and if you modify something you need to test if it works. Not just the code snippet inside Node.js, but the library itself. |
|
This is the code for the |
|
Uploaded a new commit..
Agreed. Locally I have a copy of the library and then ran something like this when testing: |
jcubic
left a comment
jcubic
left a comment
There was a problem hiding this comment.
It would be nice to have some unit test that check if what you added actually work.
|
|
||
| const oid = await GitRefManager.resolve({ | ||
| fs: new FileSystem(fs), | ||
| gitdir, |
There was a problem hiding this comment.
new FileSystem(fs) is called twice for no reason.
There was a problem hiding this comment.
"new FileSystem(fs) twice"
Should be fixed.
Yes. |
|
To compose a "superproject" that has "submodules" it's necessary to check out multiple git repos. |
src/api/currentBranch.js
Outdated
| assertParameter('fs', fs) | ||
| assertParameter('gitdir', gitdir) | ||
| const fsp = new FileSystem(fs) | ||
| const updatedgitdir = await discoverGitdir(fsp, dir, gitdir) |
There was a problem hiding this comment.
discoverGitdir is camel case, and updatedgitdir is all lowercase.
There was a problem hiding this comment.
Set to updatedGitdir.
sdarwin
force-pushed
the
submodules
branch
2 times, most recently
from
f059de2 to
509ddbb
Compare
|
Migrating everything from BrowserFS to ZenFS is likely somewhat complicated. That is the latest and preferred filesystem so it should be done. Then 'symlink' would be available. |
|
There are three sets of tests to consider:
It's easy to forget about the first one, but in fact the existing tests will verify this PR doesn't break current functionality, which is quite important. |
|
Whatever you implemented needs to be tested that it's correct. It can be one test or multiple tests if there are multiple use case that your feature addresses. |
|
Also, can you rebase with the latest changes? |
|
The plan forward is uncertain, since the idea would be to include those submodule updates to all commands from https://isomorphic-git.org/docs/en/alphabetic which is around 66 commands. |
|
The file system is abstracted away. If one is working, it will work for the other. Submodules are related to the file system, so you don't need to test every command from the docs. I think it will be fine to test how you use submodules IRL, you clone the repo and add submodule and commit and test if everything is ok. The test cloning with submodules. In canonical git you can clone recursively or init submodules after the repo was cloned. I would just test what is allowed and document how to use submodules with the library. |
|
But if you add your feature to 66 commands, then all should be tested. |
Ok, then it's sufficient to test on the regular filesystem, and we will assume the browser filesystem is functional, although it is not explicitly tested. This is reasonable.
maybe not "clone recursively", we'll see, because that is getting into providing "full submodule support". I am focusing on a subset of that, hopefully a stand-alone, self-sufficient subset of submodule features. Namely "support git commands run in submodules". Assuming you already have submodules checked out, you want to be able to run certain commands like "git branch". Referring to the original Issue, the title is "work with submodules that have been checked out by other means". Therefore maybe this will be very limited, and won't affect 66 commands. It will be the few commands that can be fixed by providing them the location of their git folder. Like |
sdarwin
changed the title
|
Removed the word "draft" from the description of the pull request. It's ready to test. |
|
Hi @jcubic, could you please review this pull request when you have a chance |
|
Tests failed, and I don't see the longs anymore. |
|
@jcubic , it appears the Github Actions didn't run on the latest commit. "2 workflows awaiting approval. This workflow requires approval from a maintainer. Can you trigger another test run? Also, by the way, tests are expected to fail. Compare to this unrelated one: 4 succeeded and 4 failed. Edit: I wasn't aware of "Azure Pipelines". Triggering another build now. Let's see. Merely re-running those tests allowed them to pass. Green checkmark. |
|
Don't worry about the errors, only isomorphic-git-PR is important, this is an old pipeline from Azure. I will need to review the code again. |
sdarwin
force-pushed
the
submodules
branch
2 times, most recently
from
b18785d to
ecb82fe
Compare
|
In this PR I have put back the official "main" version of ci.yml. (which is missing karma). As an alternative, to see functioning GHA jobs with (test.node, test.chrome), here is a demo: That branch "feature/demo" includes the submodule commit. |
There was a problem hiding this comment.
Pull Request Overview
This pull request adds submodule support to isomorphic-git by implementing git directory discovery functionality. When git commands are run within a submodule directory, the library now correctly resolves the actual git directory location (which may be in .git/modules/), enabling proper operation within submodules.
Key Changes
- Added
discoverGitdirutility function to resolve actual git directory paths for submodules - Applied submodule support to 30+ API functions by integrating git directory discovery
- Added comprehensive test suite with 40+ test files covering submodule scenarios
- Updated test configuration with increased timeouts and memory limits
Reviewed Changes
Copilot reviewed 119 out of 152 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/api/*.js (30+ files) | Added discoverGitdir calls to resolve actual git directory before operations |
| tests/test-*-in-submodule.js (40+ files) | New test files verifying each API function works correctly in submodules |
| package-scripts.cjs | Increased test timeout from 5 to 15 minutes and added memory configuration |
| azure-pipelines.yml | Doubled pipeline timeout from 60 to 120 minutes |
| .config/jest.js | Added 60s test timeout and 200MB worker memory limit |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Note Reviews pausedUse the following commands to manage reviews:
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds runtime gitdir discovery for submodule support by calling a new discoverGitdir helper and wrapping fs as FileSystem in many core APIs; introduces a submodule test fixture helper and ~100+ new submodule-focused tests; and increases CI/test timeouts and related scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller API (e.g., add)
participant FS as Provided fs
participant FSP as FileSystem wrapper (fsp)
participant DGD as discoverGitdir
participant Internal as Internal implementation (e.g., _add)
Caller->>FS: pass fs, gitdir
Caller->>FSP: new FileSystem(fs)
Caller->>DGD: await discoverGitdir({ fsp, dotgit: gitdir })
note right of DGD `#E6F0FF`: If `.git` is a file, parse "gitdir: ..." and resolve path
DGD-->>Caller: updatedGitdir
Caller->>Internal: call with fs: fsp, gitdir: updatedGitdir, other args
Internal-->>Caller: result / throw
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/addNote.js (1)
63-83: Double-wrapped FileSystem and inconsistent gitdir use break submodule support in addNoteTwo related problems here:
fsis already aFileSystem(const fs = new FileSystem(_fs)), but you then doconst fsp = new FileSystem(fs). That second wrap is likely unintended and may misalign with whatFileSystemexpects as its underlying client.
normalizeAuthorObject/normalizeCommitterObjectare still called with the unresolvedgitdir, while only_addNotegetsgitdir: updatedGitdir. In a submodule where.gitis a file, the normalize helpers will continue to look in the wrong place, defeating the purpose ofdiscoverGitdirhere.You probably want a single
FileSysteminstance and to use the discovered gitdir consistently for all gitdir-dependent helpers:export async function addNote({ @@ cache = {}, }) { try { assertParameter('fs', _fs) assertParameter('gitdir', gitdir) @@ if (signingKey) { assertParameter('onSign', onSign) } - const fs = new FileSystem(_fs) - - const author = await normalizeAuthorObject({ fs, gitdir, author: _author }) + const fs = new FileSystem(_fs) + const updatedGitdir = await discoverGitdir({ fsp: fs, dotgit: gitdir }) + + const author = await normalizeAuthorObject({ + fs, + gitdir: updatedGitdir, + author: _author, + }) @@ - const committer = await normalizeCommitterObject({ - fs, - gitdir, - author, - committer: _committer, - }) + const committer = await normalizeCommitterObject({ + fs, + gitdir: updatedGitdir, + author, + committer: _committer, + }) @@ - const fsp = new FileSystem(fs) - const updatedGitdir = await discoverGitdir({ fsp, dotgit: gitdir }) return await _addNote({ - fs: fsp, + fs, cache, onSign, gitdir: updatedGitdir, ref,This keeps a single
FileSystemwrapper and ensures all operations (config/author/committer resolution and note writing) use the same, correctly discovered gitdir.
🧹 Nitpick comments (24)
__tests__/test-GitRefSpecSet-in-submodule.js (1)
1-38: RefSpec translation tests are clear and valuableThese tests cover the important translation cases for
GitRefSpecSet(wildcard, combined specific+wildcard, explicit HEAD) and provide nice, readable expectations. Theasyncon theitcallbacks is redundant since nothing is awaited, but that’s purely cosmetic.__tests__/test-GitError-in-submodule.js (1)
5-35: Solid coverage ofErrors; async keywords here are unnecessaryThe assertions on
Errorsstatic codes andNotFoundError.toJSON()look good and self‑contained. Since neither test performs anyawait, you can drop theasyncon theitcallbacks to avoid implying asynchronous behavior:- it('have the correct value for their static property code', async () => { + it('have the correct value for their static property code', () => { @@ - it('create a NotFoundError', async () => { + it('create a NotFoundError', () => {__tests__/test-GitSideBand-in-submodule.js (1)
5-43: Demux tests look good; minor naming/readability nitThese two tests give good coverage of
GitSideBand.demuxfor normal, progress, and error flows; the expectations onpacketlines,progress,packfile, andpackfile.errorare clear.One small readability tweak: inside the
lines.forEachloops you shadow the globalittest function name:lines.forEach((it, idx) => { it = it.slice(4) + (idx === lastLineIdx ? '' : '\n') … })Consider renaming the loop variable to something like
lineto avoid confusion:- lines.forEach((it, idx) => { - it = it.slice(4) + (idx === lastLineIdx ? '' : '\n') - if (it.startsWith('\x01')) { - expectedPackfile.push(it.slice(1)) + lines.forEach((line, idx) => { + line = line.slice(4) + (idx === lastLineIdx ? '' : '\n') + if (line.startsWith('\x01')) { + expectedPackfile.push(line.slice(1)) } else if (it.startsWith('\x02')) { - expectedProgress.push(it.slice(1)) + expectedProgress.push(line.slice(1)) } else { - expectedPacketlines.push(it) + expectedPacketlines.push(line) } })(and similarly in the second test).
Also applies to: 45-77
__tests__/test-GitPktLine-in-submodule.js (1)
5-117: ComprehensiveGitPktLinecoverage; consider tightening expectations for clarityThese tests exercise the reader/encoder paths (including realistic pkt streams and error propagation) very thoroughly, and the
process.stdouthook is correctly shielded with atry/finallyunhook.For readability and clearer failure output, you could gradually refactor some of the assertions of the form:
expect((await read()).toString('utf8') === '...').toBe(true) expect((await read()) === null).toBe(true) expect(Buffer.compare(foo, Buffer.from('0010hello world\n')) === 0).toBe(true)into more idiomatic matchers:
expect((await read()).toString('utf8')).toBe('...') expect(await read()).toBeNull() expect(Buffer.compare(foo, Buffer.from('0010hello world\n'))).toBe(0)No behavioral changes needed; this is purely a style/maintainability improvement you can apply incrementally.
__tests__/test-GitRemoteManager-in-submodule.js (1)
5-94: Remote helper tests cover key URL cases; small assertion tweak possibleThe matrix of URLs here (http/https, override syntax, unknown transport, and unparseable URL) gives good coverage of
GitRemoteManager.getRemoteHelperForbehavior.In the negative cases you currently rely on
error.codeaccess to fail if no error is thrown. For slightly clearer intent and error messages, you could add an explicit non‑null check before asserting on the code:- expect(helper).toBeNull() - expect(error.code).toBe(Errors.UnknownTransportError.code) + expect(helper).toBeNull() + expect(error).not.toBeNull() + expect(error.code).toBe(Errors.UnknownTransportError.code)(similarly for the
UrlParseErrorcase). Behavior is fine as is; this would just make future regressions easier to interpret.__tests__/test-exports-in-submodule.js (1)
1-83: Consider whether the extra exports snapshot test justifies the duplication.This test snapshots the top-level
isomorphic-gitexports but doesn’t exercise submodule-specific behavior, so any API surface change will require updating two nearly identical snapshots; if that redundancy isn’t intentional, you could consolidate or document why both are needed.__tests__/test-utils-join-in-submodule.js (1)
1-57: Test naming may be misleading.The test file is named
test-utils-join-in-submodule.js, but it doesn't actually usemakeFixtureAsSubmoduleor test join behavior specifically in a submodule context. It simply validates that the internaljoinfunction matchespath.posix.joinacross various edge cases.The test coverage is good (empty paths, dots, relative paths, absolute paths, mixed separators, whitespace), but consider:
- Renaming to
test-utils-join.jsif submodule context is not required- Or adding actual submodule-specific path joining tests if there are unique behaviors to validate
__tests__/test-flatFileListToDirectoryStructure-in-submodule.js (1)
1-479: Test naming may be misleading.Similar to
test-utils-join-in-submodule.js, this test file is named with-in-submodulesuffix but doesn't actually usemakeFixtureAsSubmoduleor test submodule-specific behavior. The functionflatFileListToDirectoryStructureis a pure utility that transforms flat file lists into hierarchical directory structures, independent of repository context.Consider renaming to
test-flatFileListToDirectoryStructure.jsfor consistency, or clarify if there's a specific reason for the submodule naming convention.__tests__/test-validate-in-submodule.js (1)
57-79: Consider relaxing the checksum error message assertion.The test at lines 76-78 asserts the exact checksum values in the error message. While this validates the error thoroughly, it may be brittle if the error message format changes.
Consider using a less specific assertion:
- expect(error.data.message).toContain( - 'Invalid checksum in GitIndex buffer: expected 444952437878787878 but saw da39a3ee5e6b4b0d3255bfef95601890afd80709' - ) + expect(error.data.message).toContain('Invalid checksum in GitIndex buffer')__tests__/test-remove-in-submodule.js (1)
12-68: Consider reducing inline snapshot duplication.The "before" snapshot is duplicated across both test cases (lines 12-39 and 76-103). This makes the tests more brittle and harder to maintain. Consider extracting the expected initial file list to a shared constant or using a more focused assertion that only verifies the removed files are absent.
Example refactor:
+const INITIAL_FILES = [ + ".babelrc", + ".editorconfig", + // ... rest of files +] + describe('remove', () => { it('file', async () => { const { fs, gitdir } = await makeFixtureAsSubmodule('test-remove') const before = await listFiles({ fs, gitdir }) - expect(before).toMatchInlineSnapshot(`...`) + expect(before).toEqual(INITIAL_FILES) await remove({ fs, gitdir, filepath: 'LICENSE.md' }) const after = await listFiles({ fs, gitdir }) - expect(after).toMatchInlineSnapshot(`...`) + expect(after).not.toContain('LICENSE.md') expect(before.length === after.length + 1).toBe(true) })__tests__/test-resolveRef-in-submodule.js (1)
85-101: Consider more specific error assertions.The error validation only checks that
error.messageis defined anderror.callermatches. Consider asserting the specific error type (e.g.,NotFoundError) and/or the error message content for more robust error handling validation.Example:
+import { Errors } from 'isomorphic-git' it('non-existant refs', async () => { const { fs, gitdir } = await makeFixtureAsSubmodule('test-resolveRef') let error = {} try { await resolveRef({ fs, gitdir, ref: 'this-is-not-a-ref', }) } catch (err) { error = err } - expect(error.message).toBeDefined() + expect(error).toBeInstanceOf(Errors.NotFoundError) + expect(error.message).toContain('this-is-not-a-ref') expect(error.caller).toEqual('git.resolveRef') })__tests__/test-writeRef-in-submodule.js (1)
20-43: Remove redundant type check after assertion.Line 40's null check is redundant because Line 38 already asserts that
newBranchequals'refs/heads/another'. If the assertion passes,newBranchcannot be null or undefined. This check is dead code and adds no value.Apply this diff to remove the redundant check:
const newBranch = await currentBranch({ fs, gitdir, fullname: true }) expect(newBranch).toBe('refs/heads/another') - if (!newBranch) throw new Error('type error') const ref = await resolveRef({ fs, gitdir, ref: newBranch })__tests__/test-listNotes-in-submodule.js (1)
6-59: listNotes submodule coverage is good (minor description typo only)Default/alternate/nonexistent notes refs are all validated with exact expected note lists; only nit is the
"non-existant branch"description string, which could be corrected if you touch this file again.__tests__/test-log-file-in-submodule.js (1)
8-1100: Comprehensive log submodule tests; consider cleaning commented assertionThe log scenarios (new, deleted, forced, renames, multi‑rename) are thoroughly exercised against the submodule fixture with stable inline snapshots. Only nit: in the last test, the commented
// expect(commits.length).toBe(2)can be removed or updated to match the snapshot to avoid stale expectations.__tests__/test-unicode-paths-in-submodule.js (1)
17-145: Unicode path handling in submodules is well coveredThe tests mirror the existing unicode-path scenarios (index, commits, trees, and checkout) while running against the submodule fixture, which should catch regressions in how gitdir discovery interacts with non-ASCII paths. The structure and use of the API look correct.
__tests__/test-pull-in-submodule.js (1)
14-136: pull behavior in submodules mirrors existing scenarios correctlyThese tests exercise the standard pull cases—simple fast-forward, fast-forward-only failure on divergence, and a forced merge with
fastForward: false—against the submodule fixture, validating both history shape and error reporting. The flows and expectations line up with the non-submodule tests, so this should catch regressions introduced by the new gitdir handling.__tests__/test-writeObject-in-submodule.js (1)
1-350: Submodule writeObject coverage looks solid and consistentThese tests mirror the existing
test-writeObjectcoverage while exercising a submodule gitdir viamakeFixtureAsSubmodule, and the expectations/fixtures line up cleanly. Only optional thought: if runtime ever becomes a concern, you could share a fixture viabeforeAll, but as-is this is clear and robust.__tests__/test-walk-in-submodule.js (1)
539-548: Re‑check why STAGEmodeis expected to change when onlycore.autocrlfand workdir contents change.In the autocrlf test you update:
expectedMatrix[2][3].mode = FILEMODE expectedMatrix[7][3].mode = FILEMODEeven though only config and the workdir blob changed, not the index. If STAGE is meant to reflect index entries, its mode may be expected to remain at
0o100644. If the change toFILEMODEis intentional (e.g., due to BrowserFS quirks), a short comment explaining that contract here would help future readers.Also applies to: 572-580
__tests__/test-GitRefManager-in-submodule.js (1)
258-308: Consider making the concurrency stress test more deterministic and lighter.The loop with
iterations = 500andMath.random()in the read/write ref test gives good stress coverage but may slow the suite and still leaves some non‑determinism. You might consider:
- Using a fixed pattern (e.g., alternate awaited vs non‑awaited calls) instead of
Math.random(), and/or- Reducing
iterationsif CI time becomes an issue.The behavior under test (no
NotFoundError) should still be exercised without relying on randomness.__tests__/test-branch-in-submodule.js (1)
11-20: Clarify the roles ofgitdirvsgitdirsmfullpathfor future maintainers.Some tests read refs/HEAD via
gitdirsmfullpathwhile others read viagitdir, while API calls consistently usegitdir. It would help readability to either:
- Add a brief comment near
makeFixtureAsSubmoduleexplaining exactly what each path represents, or- Use
gitdirsmfullpathexclusively for direct filesystem reads and reservegitdironly for passing into isomorphic‑git APIs.That will make it obvious which path is “what a caller would pass” vs “where the real git dir lives on disk.”
Also applies to: 24-35, 132-148
__tests__/test-statusMatrix-in-submodule.js (1)
349-350: Preferfs.rmoverfs.rmdir({ recursive: true })for future Node compatibility.Using:
await fs.rmdir(path.join(dir, 'a'), { recursive: true })may run into deprecation/removal issues on newer Node versions. If
fshere supports it, switching to:await fs.rm(path.join(dir, 'a'), { recursive: true, force: true })would be more forward‑compatible while keeping the behavior.
__tests__/test-wire-in-submodule.js (1)
86-436: Consider softening expectations around the hard‑codedagentcapabilityThe round‑trip and error‑path coverage for the wire helpers looks good, but several tests assert packetlines including
agent=git/[email protected]verbatim. If the library ever changes how it formats the agent capability (or its version string), these tests will start failing despite the protocol behavior being correct. You might consider asserting the presence/shape of the agent capability separately (or masking the version) to reduce brittleness.__tests__/test-abortMerge-in-submodule.js (1)
208-234: Unreachablepath === 'b'branch in walk callbackIn “abort merge after modifying files in a directory”, the walker immediately returns when
head.type() === 'tree', but the special‑case branch checksif (path === 'b'). Sincebis a directory, that condition will never be hit; the interesting file is likelyb/b. This doesn’t break overall assertions (you verify contents explicitly afterward), but you may want to adjust the condition (e.g., check'b/b') or remove it to avoid confusion.__tests__/test-GitConfig-in-submodule.js (1)
168-173:odd number of quotestest currently doesn’t exercise the quoted keyHere you define
keyaaa = "val" a "aabut then callconfig.get('foo.keybbb'), which is undefined regardless of how the odd quotes are parsed. If the intent is to assert that this malformed line does not produce a usable value forkeyaaa, it would be clearer to query that key directly.For example:
- const a = await config.get('foo.keybbb') - expect(a).toBeUndefined() + const a = await config.get('foo.keyaaa') + expect(a).toBeUndefined()This keeps the expectation the same while tying the assertion to the line under test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (107)
.config/jest.js(1 hunks)__tests__/__helpers__/FixtureFSSubmodule.js(1 hunks)__tests__/test-GitAnnotatedTag-in-submodule.js(1 hunks)__tests__/test-GitConfig-in-submodule.js(1 hunks)__tests__/test-GitError-in-submodule.js(1 hunks)__tests__/test-GitIndex-in-submodule.js(1 hunks)__tests__/test-GitPackIndex-in-submodule.js(1 hunks)__tests__/test-GitPktLine-in-submodule.js(1 hunks)__tests__/test-GitRefManager-in-submodule.js(1 hunks)__tests__/test-GitRefSpecSet-in-submodule.js(1 hunks)__tests__/test-GitRemoteManager-in-submodule.js(1 hunks)__tests__/test-GitSideBand-in-submodule.js(1 hunks)__tests__/test-abortMerge-in-submodule.js(1 hunks)__tests__/test-add-in-submodule.js(1 hunks)__tests__/test-addNote-in-submodule.js(1 hunks)__tests__/test-addRemote-in-submodule.js(1 hunks)__tests__/test-annotatedTag-in-submodule.js(1 hunks)__tests__/test-branch-in-submodule.js(1 hunks)__tests__/test-checkout-in-submodule.js(1 hunks)__tests__/test-clone-checkout-huge-repo-in-submodule.js(1 hunks)__tests__/test-clone-in-submodule.js(1 hunks)__tests__/test-commit-in-submodule.js(1 hunks)__tests__/test-config-in-submodule.js(1 hunks)__tests__/test-currentBranch-in-submodule.js(1 hunks)__tests__/test-deleteBranch-in-submodule.js(1 hunks)__tests__/test-deleteRef-in-submodule.js(1 hunks)__tests__/test-deleteRemote-in-submodule.js(1 hunks)__tests__/test-deleteTag-in-submodule.js(1 hunks)__tests__/test-expandOid-in-submodule.js(1 hunks)__tests__/test-exports-in-submodule.js(1 hunks)__tests__/test-fetch-in-submodule.js(1 hunks)__tests__/test-findMergeBase-in-submodule.js(1 hunks)__tests__/test-findRoot-in-submodule.js(1 hunks)__tests__/test-flatFileListToDirectoryStructure-in-submodule.js(1 hunks)__tests__/test-getRemoteInfo-in-submodule.js(1 hunks)__tests__/test-getRemoteInfo2-in-submodule.js(1 hunks)__tests__/test-hashBlob-in-submodule.js(1 hunks)__tests__/test-hosting-providers-in-submodule.js(1 hunks)__tests__/test-init-in-submodule.js(1 hunks)__tests__/test-isBinary-in-submodule.js(1 hunks)__tests__/test-isIgnored-in-submodule.js(1 hunks)__tests__/test-listBranches-in-submodule.js(1 hunks)__tests__/test-listCommitsAndTags-in-submodule.js(1 hunks)__tests__/test-listFiles-in-submodule.js(1 hunks)__tests__/test-listNotes-in-submodule.js(1 hunks)__tests__/test-listObjects-in-submodule.js(1 hunks)__tests__/test-listRefs-in-submodule.js(1 hunks)__tests__/test-listRemotes-in-submodule.js(1 hunks)__tests__/test-listServerRefs-in-submodule.js(1 hunks)__tests__/test-listTags-in-submodule.js(1 hunks)__tests__/test-log-file-in-submodule.js(1 hunks)__tests__/test-log-in-submodule.js(1 hunks)__tests__/test-merge-in-submodule.js(1 hunks)__tests__/test-mergeFile-in-submodule.js(1 hunks)__tests__/test-normalizeAuthorObject-in-submodule.js(1 hunks)__tests__/test-normalizeCommitterObject-in-submodule.js(1 hunks)__tests__/test-packObjects-in-submodule.js(1 hunks)__tests__/test-pull-in-submodule.js(1 hunks)__tests__/test-push-in-submodule.js(1 hunks)__tests__/test-readBlob-in-submodule.js(1 hunks)__tests__/test-readCommit-in-submodule.js(1 hunks)__tests__/test-readNote-in-submodule.js(1 hunks)__tests__/test-readObject-in-submodule.js(1 hunks)__tests__/test-readTag-in-submodule.js(1 hunks)__tests__/test-readTree-in-submodule.js(1 hunks)__tests__/test-remove-in-submodule.js(1 hunks)__tests__/test-removeNote-in-submodule.js(1 hunks)__tests__/test-renameBranch-in-submodule.js(1 hunks)__tests__/test-resetIndex-in-submodule.js(1 hunks)__tests__/test-resolveRef-in-submodule.js(1 hunks)__tests__/test-stash-in-submodule.js(1 hunks)__tests__/test-status-in-submodule.js(1 hunks)__tests__/test-statusMatrix-in-submodule.js(1 hunks)__tests__/test-submodules-in-submodule.js(1 hunks)__tests__/test-tag-in-submodule.js(1 hunks)__tests__/test-unicode-paths-in-submodule.js(1 hunks)__tests__/test-updateIndex-in-submodule.js(1 hunks)__tests__/test-uploadPack-in-submodule.js(1 hunks)__tests__/test-utils-join-in-submodule.js(1 hunks)__tests__/test-validate-in-submodule.js(1 hunks)__tests__/test-version-in-submodule.js(1 hunks)__tests__/test-walk-in-submodule.js(1 hunks)__tests__/test-wire-in-submodule.js(1 hunks)__tests__/test-writeBlob-in-submodule.js(1 hunks)__tests__/test-writeCommit-in-submodule.js(1 hunks)__tests__/test-writeObject-in-submodule.js(1 hunks)__tests__/test-writeRef-in-submodule.js(1 hunks)__tests__/test-writeTag-in-submodule.js(1 hunks)__tests__/test-writeTree-in-submodule.js(1 hunks)azure-pipelines.yml(1 hunks)package-scripts.cjs(3 hunks)src/api/abortMerge.js(4 hunks)src/api/add.js(2 hunks)src/api/addNote.js(2 hunks)src/api/addRemote.js(2 hunks)src/api/annotatedTag.js(2 hunks)src/api/branch.js(2 hunks)src/api/checkout.js(2 hunks)src/api/clone.js(3 hunks)src/api/commit.js(2 hunks)src/api/currentBranch.js(2 hunks)src/api/deleteBranch.js(2 hunks)src/api/deleteRef.js(2 hunks)src/api/deleteRemote.js(2 hunks)src/api/deleteTag.js(2 hunks)src/api/expandOid.js(2 hunks)src/api/expandRef.js(2 hunks)
⛔ Files not processed due to max files limit (11)
- src/api/fastForward.js
- src/api/fetch.js
- src/api/findMergeBase.js
- src/api/getConfig.js
- src/api/getConfigAll.js
- src/api/indexPack.js
- src/api/init.js
- src/api/isDescendent.js
- src/api/isIgnored.js
- src/api/listBranches.js
- src/api/listFiles.js
🧰 Additional context used
🪛 ESLint
package-scripts.cjs
[error] 63-63: Insert ⏎·
(prettier/prettier)
🔇 Additional comments (79)
azure-pipelines.yml (1)
11-14: Linux job timeout bump looks reasonable; monitor for masked hangsRaising
timeoutInMinutesto120matches the heavier test matrix in this PR and seems fine, but it can also hide hung tests for longer. Consider watching CI duration metrics and, if runs start approaching this limit, tightening test-level timeouts or splitting suites rather than further increasing the job timeout.src/api/deleteRef.js (1)
7-32: discoverGitdir wiring in deleteRef looks correctUsing a
FileSystemwrapper (fsp) plusdiscoverGitdirand then passing{ fs: fsp, gitdir: updatedGitdir }intoGitRefManager.deleteRefis consistent with the new submodule-aware pattern and should makedeleteRefwork correctly when.gitis a gitdir file.src/api/abortMerge.js (1)
12-69: abortMerge correctly switches all gitdir users to the discovered gitdirUsing
discoverGitdirwith theFileSysteminstance and then consistently passinggitdir: updatedGitdirinto bothGitIndexManager.acquirecalls and_walkaligns with the submodule strategy and avoids mixing unresolved.gitpaths with real git directories. The worktree pathdirremains as-is, which is appropriate.Also applies to: 92-94
.config/jest.js (1)
1-24: Jest timeout and memory limit look aligned with heavier testsAdding
testTimeout: 60000andworkerIdleMemoryLimit: '200MB'makes sense given the expanded, heavier test suites in this PR. Just ensure the project’s Jest version supportsworkerIdleMemoryLimit; if not, this key will be ignored and you may need an alternative memory guard.src/api/deleteTag.js (1)
7-35: deleteTag now correctly targets the resolved gitdirWrapping
fsinFileSystem, resolvingupdatedGitdirviadiscoverGitdir, and passing both into_deleteTagis consistent with the new submodule-aware APIs and should make tag deletion work when.gitis a gitdir file.src/api/addRemote.js (1)
7-51: addRemote follows the correct discoverGitdir patternWrapping the incoming
fsinFileSystem, resolvingupdatedGitdirviadiscoverGitdir, and passing both into_addRemoteensures remote configuration touches the real git directory even when.gitis a file (submodules). The internal wiring looks consistent with the rest of the PR.src/api/deleteBranch.js (1)
7-7:discoverGitdirwiring indeleteBranchlooks correctCreating a single
FileSysteminstance (fsp) and using it both fordiscoverGitdirand_deleteBranchkeeps the usual API/command boundary intact while makingdeleteBranchsubmodule‑aware via the.gitindirection.As long as
discoverGitdir({ fsp, dotgit })is implemented to:
- return
dotgitunchanged when it already points at a real gitdir directory, and- resolve
.gitfiles that containgitdir: …(including relative paths, e.g., in submodules),this change should be backward‑compatible while enabling the desired submodule behavior. No further changes needed here from my side.
Also applies to: 37-42
__tests__/test-listTags-in-submodule.js (1)
1-61: LGTM! Clean submodule test for listTags.The test structure is clear and consistent with other submodule tests in the PR. The inline snapshot provides good validation coverage for tag listing functionality in submodules.
__tests__/test-listObjects-in-submodule.js (1)
1-200: LGTM! Comprehensive test for listObjects in submodules.The test correctly validates object listing within a submodule context. The use of
gitdirsmfullpathis appropriate for submodule scenarios where the actual git directory path differs from the standard location.__tests__/test-deleteRemote-in-submodule.js (2)
1-16: LGTM! Test correctly validates remote deletion in submodules.The test structure is clear and validates both the successful deletion of a remote and the remaining remotes list.
17-31: Fixture name is acceptable; fixtures are identical.Verification confirms that both
test-addRemoteandtest-deleteRemotefixtures contain identical git configurations (same core settings and remote definitions). Usingtest-addRemotein the missing argument test is intentional and valid, as the fixtures are structurally equivalent.__tests__/test-findRoot-in-submodule.js (1)
1-36: LGTM! Well-structured tests for findRoot in submodules.The tests clearly validate root finding behavior in submodule contexts. The comment at lines 8-10 helpfully explains why
.gitdirectories are created dynamically. Both test cases provide good coverage for immediate and ancestor.gitfolder scenarios.__tests__/test-uploadPack-in-submodule.js (1)
1-23: LGTM! Protocol validation test is precise and correct.The test appropriately validates the git protocol output format for ref advertisement in submodules. The use of
gitdirsmfullpathand exact string matching ensures correct protocol behavior.__tests__/test-listRemotes-in-submodule.js (1)
1-17: LGTM! Clean and straightforward test for listRemotes.The test structure is simple and effectively validates remote listing functionality in submodule contexts.
src/api/commit.js (2)
7-7: LGTM! Appropriate import for submodule support.The
discoverGitdirutility is correctly imported to enable git directory resolution in submodule contexts.
78-84: LGTM! Clean implementation of submodule git directory resolution.The change correctly resolves the actual git directory before performing the commit operation. This pattern aligns with the PR's objective of supporting git commands in submodules and is consistent with similar changes across other API functions.
__tests__/test-deleteTag-in-submodule.js (1)
1-37: LGTM! Comprehensive tests for deleteTag in submodules.Both test cases are well-structured and provide good coverage: the successful deletion scenario validates both the operation and its side effects, while the error case ensures proper parameter validation.
src/api/branch.js (1)
7-47: Submodule-aware gitdir resolution wiring inbranchlooks correct.
FileSystemis wrapped once,discoverGitdiris awaited, andupdatedGitdiris passed into_branch, so behavior stays backward-compatible while gaining submodule support for.gitfiles.__tests__/test-currentBranch-in-submodule.js (1)
1-32: Good coverage ofcurrentBranchbehavior in submodule contexts.The three cases (default,
fullname: true, detached HEAD) usingmakeFixtureAsSubmoduleshould catch regressions in submodule-awarecurrentBranchresolution.src/api/expandOid.js (1)
7-45:expandOidnow correctly honors discovered gitdir (e.g., submodules).Wrapping the fs client, resolving
updatedGitdirviadiscoverGitdir, and passing it into_expandOidis consistent with other APIs and should be transparent to callers while fixing.gitfile scenarios.__tests__/test-readTag-in-submodule.js (1)
1-44:readTagsubmodule test is thorough via inline snapshot.The fixture setup and inline snapshot cover both the raw tag payload and parsed structure, so regressions in annotated tag handling for submodules should be caught quickly.
src/api/add.js (1)
12-69:addcorrectly propagatesupdatedGitdirthrough index, config, and write paths.
discoverGitdiris invoked once, and the resolved gitdir is used consistently inGitIndexManager.acquire,GitConfigManager.get,GitIgnoreManager.isIgnored, and_writeObject, giving submodule support without changing the externaladdAPI.__tests__/test-init-in-submodule.js (1)
1-44:initsubmodule tests exercise key modes and config preservation.The normal,
bare, and “does not overwrite existing config” scenarios usingmakeFixtureAsSubmoduleandgitdirsmfullpathgive good coverage of howinitshould behave when the repo lives in a submodule layout.__tests__/test-version-in-submodule.js (1)
1-24: Simple but effectiveversion()regression check.Parsing
package.jsononce and assertingversion()equalspkg.versionis a straightforward way to keep the reported version in sync with the package metadata, even if the test itself isn’t submodule-specific.src/api/clone.js (1)
7-7: LGTM! Submodule support correctly integrated.The implementation correctly follows the established pattern for submodule support:
- Creates a FileSystem wrapper once for reuse
- Resolves the actual git directory via
discoverGitdirbefore delegating to internal operations- Passes the resolved gitdir to the underlying
_clonefunctionThis ensures clone operations work correctly when the gitdir is a submodule reference (
.gitfile pointing to the actual git directory).Also applies to: 91-92, 94-94, 104-104
__tests__/test-listRefs-in-submodule.js (1)
1-83: LGTM! Well-structured submodule test.The test correctly:
- Uses the
makeFixtureAsSubmodulehelper to create a submodule environment- Calls
listRefswith the resolved gitdir- Validates the results against an inline snapshot that includes both lightweight and annotated tags (with
^{}suffixes for dereferenced annotated tags)src/api/expandRef.js (1)
7-7: LGTM! Consistent submodule support implementation.The changes mirror the pattern used across other API functions in this PR:
- Creates a FileSystem wrapper for consistent fs operations
- Resolves the actual git directory before expanding refs
- Properly passes the resolved values to the underlying GitRefManager
Also applies to: 31-37
__tests__/test-writeCommit-in-submodule.js (1)
1-51: LGTM! Comprehensive writeCommit test.The test correctly:
- Creates a submodule fixture using
makeFixtureAsSubmodule- Provides a complete commit object with author, committer, GPG signature, message, parent, and tree
- Validates that the written commit produces the expected OID
This ensures
writeCommitworks correctly in submodule contexts.__tests__/test-findMergeBase-in-submodule.js (1)
1-247: LGTM! Excellent test coverage for merge base scenarios.This test suite is comprehensive and well-structured:
- Covers edge cases (single OID, duplicates)
- Tests no-common-ancestor scenarios
- Validates fast-forward detection
- Checks diverging branch scenarios
- Handles merge commits correctly
- Tests recursive merge bases
- Validates fork-and-rejoin patterns
The comment on line 7 indicating verification against actual git commands adds confidence. All tests properly use
makeFixtureAsSubmoduleto ensure submodule context.__tests__/test-normalizeCommitterObject-in-submodule.js (2)
1-168: Otherwise comprehensive test coverage.Beyond the variable naming issue, the test suite properly covers all normalization scenarios:
- Returns committer when fully populated
- Falls back to author when committer is absent
- Uses commit's committer when parameters are missing
- Derives from config with current timestamp
- Returns undefined when no values are available
126-155: No changes needed - gitdirsmfullpath is a valid property.The helper function
makeFixtureAsSubmoduleexplicitly returnsgitdirsmfullpathas a property, alongsidegitdir. The helper's own comment notes that "gitdirsmfullpath should only rarely be needed in tests," but it is intentionally provided. This test deliberately usesgitdirsmfullpath(as does the test at line 159), while earlier tests usegitdir—both are valid properties, and the choice between them appears intentional.Likely an incorrect or invalid review comment.
__tests__/test-addRemote-in-submodule.js (1)
1-58: Test coverage looks solid.The test suite comprehensively validates addRemote behavior in submodule contexts, including error cases. The fixture setup and assertions follow established patterns.
__tests__/test-clone-checkout-huge-repo-in-submodule.js (1)
1-43: Clone test implementation is appropriate.The test validates the complete clone workflow for large repositories in submodule contexts, with proper assertions on directory structure and content. The use of
gitdirsmfullpathcorrectly targets the actual git directory.__tests__/test-isBinary-in-submodule.js (1)
1-36: Binary detection tests are well-structured.The parameterized tests cover both binary and text file types, with clear assertions. The fixture-based approach ensures consistent test data.
__tests__/test-listBranches-in-submodule.js (1)
1-37: Branch listing tests look good.The tests appropriately use snapshot assertions for both local and remote branch listings, providing clear expected values and proper fixture setup.
__tests__/test-validate-in-submodule.js (1)
1-80: Invalid index tests are thorough.The test suite covers three important corruption scenarios with proper error type and message validation. The use of
gitdirsmfullpathcorrectly targets the actual git directory in submodule contexts.__tests__/test-annotatedTag-in-submodule.js (1)
1-76: Annotated tag tests are comprehensive.The test suite covers basic tag creation, blob tagging, and PGP signing with proper signature verification. The dynamic import of the PGP plugin (line 50) is appropriate for conditional functionality.
src/api/checkout.js (2)
90-91: Git directory discovery implementation looks correct.The addition of
discoverGitdirenables checkout to work with submodules by resolving the actual git directory when.gitis a file rather than a directory. Creating the FileSystem wrapper before discovery is appropriate.
93-98: FileSystem wrapper is fully compatible with_checkoutusage.All methods called by
_checkoutare available in the FileSystem wrapper class:rm(),rmdir(),mkdir(),write(),writelink(), andlstat(). The wrapper provides a consistent promise-based interface over the underlying fs implementation and properly handles both callback-based and promise-based filesystems through thebindFs()mechanism.Passing the wrapped
fspinstead of rawfsto_checkoutis safe and maintains full compatibility with the downstream code.__tests__/test-isIgnored-in-submodule.js (1)
1-37: Ignore pattern tests are thorough.The tests validate
.gitignorebehavior including negation patterns, directory patterns, and subdirectory.gitignorefiles. The helper function for writing.gitignorefiles keeps the test setup clean.__tests__/test-config-in-submodule.js (1)
6-62: LGTM!The tests provide good coverage of config operations in submodule contexts, including reading single/multiple values, setting booleans, handling undefined, and modifying remotes.
__tests__/test-expandOid-in-submodule.js (1)
6-64: LGTM!The tests provide comprehensive coverage of expandOid functionality in submodule contexts, including proper error type validation for not-found and ambiguous cases, and packfile scenarios.
__tests__/test-deleteRef-in-submodule.js (1)
6-44: LGTM!The tests appropriately cover different ref storage scenarios (loose, packed, and both) and validate that only the intended refs are deleted.
__tests__/test-readCommit-in-submodule.js (1)
6-136: LGTM!The tests provide comprehensive coverage of commit reading in submodule contexts, including error handling, GPG signature parsing, packfile commits, and tag peeling. The large inline snapshot is appropriate for validating complete commit parsing.
__tests__/test-writeBlob-in-submodule.js (1)
6-59: LGTM!The tests appropriately validate blob writing in submodule contexts, covering both empty blobs and content-filled blobs with correct SHA-1 hash verification.
__tests__/test-writeTree-in-submodule.js (1)
6-170: LGTM!The tests provide excellent coverage of tree writing in submodule contexts. The sorting test is particularly valuable as it validates correct handling of edge cases (spaces, dots, tildes) that are critical for Git compatibility.
__tests__/test-writeTag-in-submodule.js (1)
1-48: LGTM! Well-structured submodule test.The test correctly validates annotated tag creation within a submodule context. The fixture setup, tag object structure, and GPG signature are all properly configured.
__tests__/test-writeRef-in-submodule.js (1)
7-19: LGTM! Clear tag ref test.The test correctly validates writing a tag reference and resolving it in a submodule context.
__tests__/test-addNote-in-submodule.js (1)
1-252: LGTM! Comprehensive note management test suite.This test suite provides excellent coverage of the addNote functionality in submodule contexts, including:
- Adding notes to different object types (commit, tree, blob)
- Note accumulation behavior
- Alternate branch support
- Error handling for duplicates
- Force-overwrite capability
The tests are well-structured with clear assertions and appropriate use of error handling patterns.
__tests__/test-removeNote-in-submodule.js (1)
1-64: LGTM! Clear note removal tests.The tests correctly validate note removal from both default and alternate branches in a submodule context. The before/after count checks and OID assertions provide good validation.
__tests__/test-mergeFile-in-submodule.js (1)
1-38: LGTM! Clear merge scenario tests.The tests correctly validate both clean and conflicted merge scenarios in a submodule context. The assertions on
cleanMergeflags and comparison with expected output files provide appropriate validation.__tests__/test-listFiles-in-submodule.js (1)
1-89: LGTM! Comprehensive file listing tests.The tests correctly validate listFiles behavior in submodule contexts, covering both index-based listing and ref-specific listing. The inline snapshots provide clear, verifiable expected outputs.
__tests__/test-submodules-in-submodule.js (1)
1-90: LGTM! Solid integration tests for submodule workflows.These integration tests provide valuable validation of high-level submodule behavior including:
- Clone preserving submodule staging
- Submodule persistence across commits
- Branch checkout with/without submodules
The localhost detection (lines 7-9) correctly handles both Node and browser test environments.
__tests__/test-resetIndex-in-submodule.js (1)
1-122: LGTM! Comprehensive resetIndex test coverage.The test suite provides thorough validation of resetIndex behavior in submodule contexts, covering:
- Modified file reset
- New file reset
- New repository scenarios
- OID-based reset
The inline snapshots and before/after assertions provide clear validation of the expected behavior.
__tests__/test-GitAnnotatedTag-in-submodule.js (1)
1-70: GitAnnotatedTag parse/render roundtrip tests look solidThe tag string and object are consistent and exercise both parse and render without any fs/submodule coupling; no issues found.
src/api/deleteRemote.js (1)
7-40: deleteRemote integration with discoverGitdir is consistent and non‑breakingUsing a
FileSystemwrapper (fsp) plusdiscoverGitdir({ fsp, dotgit: gitdir })before delegating to_deleteRemotepreserves existing behavior while adding submodule/gitdir‑file support; signature and error handling stay the same.__tests__/test-tag-in-submodule.js (1)
1-65: Submodule tag tests cover key success and error pathsCreating, conflicting, and force‑overwriting tags (including packed refs) are all exercised with appropriate
Errors.AlreadyExistsErrorchecks; fixtures and expectations look coherent.__tests__/test-getRemoteInfo2-in-submodule.js (1)
1-88: getRemoteInfo2 protocol and error‑handling tests are well structuredProtocol v1/v2 behavior, dumb HTTP detection, and
UnknownTransportErrorfor scp‑style URLs are all covered with appropriate assertions; nothing problematic spotted.__tests__/test-listCommitsAndTags-in-submodule.js (1)
6-27: listCommitsAndTags submodule test is precise and minimalThe test correctly drives
listCommitsAndTagswith a start/finish range and asserts the resulting commit sequence via inline snapshot; no issues found.__tests__/test-hashBlob-in-submodule.js (1)
1-61: LGTM! Comprehensive test coverage for hashBlob in submodule context.The test suite properly validates hashBlob functionality with both Buffer and String inputs, verifying oid, format, and wrapped blob structure. The tests follow established patterns and include appropriate assertions.
__tests__/test-GitIndex-in-submodule.js (1)
1-126: LGTM! Thorough GitIndex testing in submodule context.The test suite comprehensively validates GitIndex operations including simple/full index parsing, round-trip consistency, and unmerged index states. Tests properly verify internal structures like entriesFlat, entriesMap, and stage arrays.
__tests__/test-listServerRefs-in-submodule.js (1)
1-329: LGTM! Excellent test coverage for listServerRefs across protocol versions.The test suite thoroughly validates listServerRefs behavior for both protocol versions 1 and 2, including edge cases and option combinations. The documentation of protocol version 1 limitations (lines 9-12) is particularly helpful.
__tests__/test-deleteBranch-in-submodule.js (1)
1-88: LGTM! Comprehensive deleteBranch test coverage.The test suite validates all critical deleteBranch scenarios including error conditions (NotFoundError, MissingParameterError), edge cases (same-named tags, checked out branch), and proper cleanup of associated configuration entries.
__tests__/test-getRemoteInfo-in-submodule.js (1)
1-57: LGTM! Solid test coverage for getRemoteInfo.The test suite validates getRemoteInfo functionality including basic remote information retrieval, dumb HTTP server detection, and proper error handling for unsupported transport protocols. The environment-aware conditional test execution is appropriately implemented.
__tests__/test-readTree-in-submodule.js (1)
1-581: LGTM! Excellent comprehensive coverage for readTree.The test suite thoroughly validates readTree functionality including direct tree reads, tag peeling, filepath navigation at various depths, and comprehensive error handling for invalid filepath scenarios (directory as file, non-existent paths, leading/trailing slashes).
__tests__/test-readBlob-in-submodule.js (1)
1-197: LGTM! Thorough test coverage for readBlob.The test suite comprehensively validates readBlob functionality including direct blob reads, tag peeling, filepath navigation at various depths, and extensive error handling for all invalid filepath scenarios. The error validation properly distinguishes between ObjectTypeError, NotFoundError, and InvalidFilepathError with specific reasons.
__tests__/__helpers__/FixtureFSSubmodule.js (1)
35-77: Submodule fixture wiring looks structurally soundThe way
makeFixtureAsSubmoduleclones the superproject, relocates the submodule gitdir into.git/modules/mysubmodule, and writes a.gitfile withgitdir: ../.git/modules/mysubmodulematches how real submodules are laid out, and returning bothgitdir(the.gitfile) andgitdirsmfullpathgives tests enough flexibility without leaking too much detail.__tests__/test-GitPackIndex-in-submodule.js (1)
1-161: Good coverage of pack index behavior in submodulesThis suite exercises
.idxand.packinitialization, truncation behavior, round-triptoBuffer, and both undeltified and deeply deltified objects using the submodule fixture, which is exactly what we need to validate pack handling under the new gitdir discovery logic.__tests__/test-renameBranch-in-submodule.js (1)
8-161: renameBranch submodule scenarios are comprehensively exercisedYou cover the key edge cases (existing target, invalid names, missing params) along with successful renames, rename+checkout, and renaming the current branch while checking both
currentBranchand the underlyingrefs/headscontents. This gives solid confidence that branch ref rewrites behave correctly in a submodule context.__tests__/test-readObject-in-submodule.js (1)
1-626: Comprehensive readObject submodule tests, no issuesThis suite thoroughly exercises readObject in a submodule (happy paths, packfile scenarios, filepath resolution, and detailed error cases), and the structure closely tracks the existing non-submodule tests. I don’t see correctness or coverage gaps here.
src/api/currentBranch.js (1)
7-47: currentBranch wiring to discoverGitdir looks correctUsing a single
FileSystemwrapper (fsp) and resolvingupdatedGitdirviadiscoverGitdirbefore calling_currentBranchis consistent and should transparently support submodule.gitpointer files without changing the public API. Error handling and parameter checks remain intact.__tests__/test-readNote-in-submodule.js (1)
1-60: readNote submodule tests are clear and adequateThe tests cover notes on commits, trees, blobs, and an alternate notes ref within a submodule fixture, with straightforward UTF‑8 checks. This is a good, focused addition with no apparent issues.
__tests__/test-status-in-submodule.js (1)
1-81: status behavior in submodules is well coveredThis test suite closely tracks the existing status tests while running against a submodule fixture, covering normal changes, edge “star” states, empty-repo behavior, and ignore handling. The fixture usage and expectations look correct.
__tests__/test-branch-in-submodule.js (1)
8-183: Solid coverage ofbranchbehavior in submodules.The scenarios (basic creation, start point, force, checkout, invalid names, empty repo, remote‑named branches,
HEADbranch) track the existing branch tests well and exercise the submodule fixture via bothdirandgitdir. I don’t see functional issues here.__tests__/test-statusMatrix-in-submodule.js (1)
126-247: Nice exhaustive coverage ofstatusMatrix+ignored+filepathspermutations in submodules.The nested
ignored:truecases with multiple folders and order permutations give strong confidence that submodules behave identically to the main repo for ignore semantics and filtering. No functional issues spotted here.__tests__/test-add-in-submodule.js (1)
15-278:addsubmodule tests look correct and closely aligned with the main suite.These tests exercise the same edge cases as the primary
addtests (ignores, force, folders, symlinks, autocrlf + binary files) but throughmakeFixtureAsSubmodule, which is exactly what we want to validate submodule behavior. I don’t see any functional problems here.__tests__/test-log-in-submodule.js (1)
1-725: Comprehensive submodulelogcoverage looks solidThe tests exercise HEAD, depth, since, shallow refs, complex histories, and GPG verification against submodule fixtures, with async usage and expectations wired correctly. No issues from my side.
src/api/annotatedTag.js (1)
8-86: Gitdir discovery is correctly threaded through annotatedTagResolving
updatedGitdirviadiscoverGitdirand consistently passing it tonormalizeAuthorObjectand_annotatedTagaligns this API with the new submodule-aware pattern, without changing the external signature. Looks good.__tests__/test-updateIndex-in-submodule.js (1)
1-337: SubmoduleupdateIndexparity tests look correctThese tests faithfully mirror the existing
updateIndexbehavior in a submodule fixture (add/remove/force/update plus error paths), with proper async handling and clear expectations. I don’t see any issues.
| const localhost = | ||
| typeof window === 'undefined' ? 'localhost' : window.location.hostname | ||
|
|
||
| const copyRecursiveSyncShell = async function (src, dest) { | ||
| spawnSync('cp -rp ' + String(src) + ' ' + String(dest) + ' ', { | ||
| shell: '/bin/bash', | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Node-/POSIX-only cp usage undermines browser and Windows portability
copyRecursiveSyncShell depends on child_process.spawnSync('cp -rp …', { shell: '/bin/bash' }), which assumes a POSIX shell and cp binary. This won’t work in real browsers and is likely to fail on Windows, despite the eslint-env node, browser header and the intention to reuse fixtures across environments. It also ignores the child process exit status, so failures in cp will surface later as confusing fixture errors.
Consider:
- Implementing recursive copy via the injected
fsabstraction (or a small recursive helper) so it works consistently across backends, or - Guarding this helper so it’s only used in Node/POSIX test runs and failing fast otherwise, and
- Checking
spawnSync’sstatus/stderrand throwing a clear error when the copy fails.
🏁 Script executed:
#!/bin/bash
# Examine the target file to confirm the code matches the review snippet
cat __tests__/__helpers__/FixtureFSSubmodule.jsLength of output: 2797
🏁 Script executed:
#!/bin/bash
# Search for usage of copyRecursiveSyncShell
rg -n 'copyRecursiveSyncShell' -C3Length of output: 1585
🏁 Script executed:
#!/bin/bash
# Search for other spawnSync usages to understand patterns
rg -n 'spawnSync' -C2Length of output: 800
🏁 Script executed:
#!/bin/bash
# Check for fs abstraction or alternative copy implementations
rg -n 'copyRecursive|recursiveCopy|fs\.copy' -C2Length of output: 1268
🏁 Script executed:
#!/bin/bash
# Look for test environment configuration and setup
fd -e 'jest.config|jasmine|test.*setup|\.babelrc' -type fLength of output: 170
🏁 Script executed:
#!/bin/bash
# Check if there's any environment detection or Node-only guards
rg -n 'typeof window|process\.env|NODE_ENV' __tests__/__helpers__/ -C2Length of output: 1276
Node/POSIX-only cp usage breaks browser and Windows compatibility despite eslint-env declaration
The review comment is accurate and well-founded. The code explicitly imports spawnSync from Node's child_process and uses /bin/bash with the cp command—neither available in browsers or Windows environments. Despite the /* eslint-env node, browser, jasmine */ header, there is no guard preventing copyRecursiveSyncShell from being imported and called in browser tests, which would silently fail (no exit status is checked).
However, the required tools exist to fix this: the makeFixtureAsSubmodule function already receives fssp (an fs abstraction) and successfully uses fssp._mkdir() and fssp._writeFile(), proving a cross-platform fs layer is available. A recursive copy implementation using this abstraction would work consistently across environments, eliminating both the portability issue and the unhandled failure case.
Lines requiring attention:
- Lines 29–32: Replace
copyRecursiveSyncShellimplementation with fs-abstraction-based recursive copy - Lines 57, 61: These calls will then use the corrected implementation
- Consider adding a Node-only guard if makeFixtureAsSubmodule is intentionally Node-exclusive, though fixing the underlying function is preferred
🤖 Prompt for AI Agents
In __tests__/__helpers__/FixtureFSSubmodule.js around lines 26 to 33, the
current copyRecursiveSyncShell uses spawnSync with /bin/bash and cp which breaks
browser/Windows environments; replace it with an async recursive copy that uses
the provided fssp abstraction: stat the src with fssp.stat or fssp._stat, create
destination directories with fssp._mkdir when encountering directories, iterate
directory entries and recurse for each entry, and use fssp._writeFile (or
fssp.writeFile) to copy file contents; ensure the function returns/rejects its
promise on errors and update the callers at lines ~57 and ~61 to await the new
async implementation (or adapt to its promise) so no shell or Node-only APIs are
used.
| fs.write(`${dir}/c`, 'new changes to file c') | ||
| await GitIndexManager.acquire( | ||
| { fs, gitdir: gitdirsmfullpath, cache: {} }, | ||
| async function (index) { | ||
| index.delete({ filepath: 'c' }) | ||
| } | ||
| ) | ||
|
|
||
| const fileAWorkdirVersion = await fs.read(`${dir}/a`).then(buffer => { | ||
| return buffer.toString() | ||
| }) | ||
| const fileBWorkdirVersion = await fs.read(`${dir}/b`).then(buffer => { | ||
| return buffer.toString() | ||
| }) | ||
| const fileCWorkdirVersion = await fs.read(`${dir}/c`).then(buffer => { | ||
| return buffer.toString() | ||
| }) | ||
|
|
||
| try { | ||
| await abortMerge({ fs, dir, gitdir }) | ||
| } catch (e) { | ||
| error = e | ||
| } | ||
|
|
||
| expect(error).not.toBeNull() | ||
| expect(error.code).toBe(Errors.IndexResetError.code) | ||
|
|
||
| const fileAContent = await fs.read(`${dir}/a`).then(buffer => { | ||
| return buffer.toString() | ||
| }) | ||
| const fileBContent = await fs.read(`${dir}/b`).then(buffer => { | ||
| return buffer.toString() | ||
| }) | ||
| const fileCContent = await fs.read(`${dir}/c`).then(buffer => { | ||
| return buffer.toString() | ||
| }) | ||
|
|
||
| const dirContents = await fs.readdir(dir) | ||
|
|
||
| expect(dirContents.length).toBe(4) | ||
| expect(fileAContent).toEqual(fileAWorkdirVersion) | ||
| expect(fileBContent).toEqual(fileBWorkdirVersion) | ||
| expect(fileCContent).toEqual(fileCWorkdirVersion) | ||
| }) |
There was a problem hiding this comment.
Await fs.write to avoid race conditions in this test
In the “uncache a file that has changes in the workdir” test, fs.write(${dir}/c, 'new changes to file c') is not awaited, whereas all other fs operations are. Since this filesystem is async, the write can race with the subsequent index manipulation and reads, making the test flaky or nondeterministic. It should be awaited:
- fs.write(`${dir}/c`, 'new changes to file c')
+ await fs.write(`${dir}/c`, 'new changes to file c')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fs.write(`${dir}/c`, 'new changes to file c') | |
| await GitIndexManager.acquire( | |
| { fs, gitdir: gitdirsmfullpath, cache: {} }, | |
| async function (index) { | |
| index.delete({ filepath: 'c' }) | |
| } | |
| ) | |
| const fileAWorkdirVersion = await fs.read(`${dir}/a`).then(buffer => { | |
| return buffer.toString() | |
| }) | |
| const fileBWorkdirVersion = await fs.read(`${dir}/b`).then(buffer => { | |
| return buffer.toString() | |
| }) | |
| const fileCWorkdirVersion = await fs.read(`${dir}/c`).then(buffer => { | |
| return buffer.toString() | |
| }) | |
| try { | |
| await abortMerge({ fs, dir, gitdir }) | |
| } catch (e) { | |
| error = e | |
| } | |
| expect(error).not.toBeNull() | |
| expect(error.code).toBe(Errors.IndexResetError.code) | |
| const fileAContent = await fs.read(`${dir}/a`).then(buffer => { | |
| return buffer.toString() | |
| }) | |
| const fileBContent = await fs.read(`${dir}/b`).then(buffer => { | |
| return buffer.toString() | |
| }) | |
| const fileCContent = await fs.read(`${dir}/c`).then(buffer => { | |
| return buffer.toString() | |
| }) | |
| const dirContents = await fs.readdir(dir) | |
| expect(dirContents.length).toBe(4) | |
| expect(fileAContent).toEqual(fileAWorkdirVersion) | |
| expect(fileBContent).toEqual(fileBWorkdirVersion) | |
| expect(fileCContent).toEqual(fileCWorkdirVersion) | |
| }) | |
| await fs.write(`${dir}/c`, 'new changes to file c') | |
| await GitIndexManager.acquire( | |
| { fs, gitdir: gitdirsmfullpath, cache: {} }, | |
| async function (index) { | |
| index.delete({ filepath: 'c' }) | |
| } | |
| ) | |
| const fileAWorkdirVersion = await fs.read(`${dir}/a`).then(buffer => { | |
| return buffer.toString() | |
| }) | |
| const fileBWorkdirVersion = await fs.read(`${dir}/b`).then(buffer => { | |
| return buffer.toString() | |
| }) | |
| const fileCWorkdirVersion = await fs.read(`${dir}/c`).then(buffer => { | |
| return buffer.toString() | |
| }) | |
| try { | |
| await abortMerge({ fs, dir, gitdir }) | |
| } catch (e) { | |
| error = e | |
| } | |
| expect(error).not.toBeNull() | |
| expect(error.code).toBe(Errors.IndexResetError.code) | |
| const fileAContent = await fs.read(`${dir}/a`).then(buffer => { | |
| return buffer.toString() | |
| }) | |
| const fileBContent = await fs.read(`${dir}/b`).then(buffer => { | |
| return buffer.toString() | |
| }) | |
| const fileCContent = await fs.read(`${dir}/c`).then(buffer => { | |
| return buffer.toString() | |
| }) | |
| const dirContents = await fs.readdir(dir) | |
| expect(dirContents.length).toBe(4) | |
| expect(fileAContent).toEqual(fileAWorkdirVersion) | |
| expect(fileBContent).toEqual(fileBWorkdirVersion) | |
| expect(fileCContent).toEqual(fileCWorkdirVersion) | |
| }) |
🤖 Prompt for AI Agents
In __tests__/test-abortMerge-in-submodule.js around lines 327 to 370, the call
to fs.write(`${dir}/c`, 'new changes to file c') is not awaited which can race
with the subsequent index operations and reads; change it to await the fs.write
call so the write completes before acquiring the index and before subsequent
fs.read/fs.readdir assertions.
| it('fetch --prune-tags from git-http-mock-server', async () => { | ||
| const { fs, dir, gitdir, gitdirsmfullpath } = | ||
| await makeFixtureAsSubmodule('test-fetch-client') | ||
| await setConfig({ | ||
| fs, | ||
| gitdir, | ||
| path: 'remote.origin.url', | ||
| value: `http://${localhost}:8888/test-fetch-server.git`, | ||
| }) | ||
| expect(await fs.exists(`${gitdirsmfullpath}/refs/tags/v1.0.0-beta1`)).toBe( | ||
| true | ||
| ) | ||
| const oldValue = await fs.read( | ||
| `${gitdirsmfullpath}/refs/tags/v1.0.0`, | ||
| 'utf8' | ||
| ) | ||
| try { | ||
| await fetch({ | ||
| fs, | ||
| http, | ||
| dir, | ||
| gitdir, | ||
| depth: 1, | ||
| tags: true, | ||
| pruneTags: true, | ||
| }) | ||
| } catch (err) { | ||
| // shrug | ||
| } | ||
| // assert that tag was deleted | ||
| expect(await fs.exists(`${gitdir}/refs/tags/v1.0.0-beta1`)).toBe(false) | ||
| // assert that tags was force-updated | ||
| const newValue = await fs.read(`${gitdir}/refs/tags/v1.0.0`, 'utf8') | ||
| expect(oldValue).not.toEqual(newValue) |
There was a problem hiding this comment.
🧩 Analysis chain
Use gitdirsmfullpath for tag filesystem checks in the prune-tags test
In this test you correctly use gitdirsmfullpath for the initial tag existence checks, but the post-fetch assertions still read from ${gitdir}/refs/.... In a real submodule, gitdir is typically the .git file, so these checks can stay false even if pruneTags fails to delete or update the underlying tag in the actual gitdir.
Align the final assertions with the earlier ones to ensure the test really validates tag pruning in submodules:
- // assert that tag was deleted
- expect(await fs.exists(`${gitdir}/refs/tags/v1.0.0-beta1`)).toBe(false)
- // assert that tags was force-updated
- const newValue = await fs.read(`${gitdir}/refs/tags/v1.0.0`, 'utf8')
+ // assert that tag was deleted
+ expect(
+ await fs.exists(`${gitdirsmfullpath}/refs/tags/v1.0.0-beta1`)
+ ).toBe(false)
+ // assert that tag was force-updated
+ const newValue = await fs.read(
+ `${gitdirsmfullpath}/refs/tags/v1.0.0`,
+ 'utf8'
+ )
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine the test file and locate makeFixtureAsSubmodule
fd -t f "test-fetch-in-submodule.js" --exec head -100 {}
Length of output: 2981
🏁 Script executed:
#!/bin/bash
# Search for makeFixtureAsSubmodule definition
rg -n "makeFixtureAsSubmodule" --type=js -A 10Length of output: 267348
🏁 Script executed:
#!/bin/bash
# Look at the setup and fixture code around line 338
rg -n "gitdirsmfullpath|gitdir" __tests__/test-fetch-in-submodule.js -B 3 -A 3 | head -80Length of output: 2527
🏁 Script executed:
#!/bin/bash
# Check other tag-related tests to see the pattern used
rg -n "refs/tags" __tests__/test-fetch-in-submodule.js -B 5 -A 5Length of output: 800
Fix inconsistent tag path references in prune-tags test
The review comment is valid. The test correctly uses gitdirsmfullpath for the initial tag checks (lines 347, 351) but then inconsistently switches to gitdir for the post-fetch assertions (lines 368, 370). In submodule context, gitdir is typically the .git file path, while gitdirsmfullpath is the actual gitdir location. This inconsistency means the final assertions could pass even if pruneTags fails to properly update tags in the real directory.
Apply the suggested changes to align lines 368 and 370 with the earlier pattern.
🤖 Prompt for AI Agents
In __tests__/test-fetch-in-submodule.js around lines 338 to 371, the post-fetch
assertions incorrectly reference gitdir instead of gitdirsmfullpath used
earlier; update the two assertions at lines ~368 and ~370 to use
`${gitdirsmfullpath}/refs/tags/...` (i.e., check existence and read the tag file
from gitdirsmfullpath) so the test consistently verifies tag pruning and updates
in the actual submodule gitdir.
| xit('push', async () => { | ||
| // Setup | ||
| const { fs, gitdir } = await makeFixtureAsSubmodule( | ||
| 'test-hosting-providers' | ||
| ) | ||
| // Test | ||
| const res = await push({ | ||
| fs, | ||
| http, | ||
| gitdir, | ||
| // corsProxy: process.browser ? `http://${localhost}:9999` : undefined, | ||
| remote: 'azure', | ||
| ref: 'master', | ||
| force: true, | ||
| onAuth: () => ({ username, password }), | ||
| }) | ||
| expect(res).toBeTruthy() | ||
| expect(res.ok).toBe(true) | ||
| expect(res.refs['refs/heads/master'].ok).toBe(true) | ||
| }) |
There was a problem hiding this comment.
Reconsider using xit for disabled tests.
Based on PR discussion, the maintainer expressed concerns about skipped tests using xit. If this test needs to remain disabled due to issue #1876, consider either removing it entirely or adding a clear plan for re-enabling it.
🤖 Prompt for AI Agents
In __tests__/test-hosting-providers-in-submodule.js lines 89-108 the test is
disabled with xit; remove the disabled test block entirely if we don't intend to
keep it, OR if we must keep it, replace xit with a clearly skipped test
(it.skip) and add a single-line TODO comment referencing issue #1876 plus a
short plan and re-enable criteria (e.g., "TODO: re-enable when auth flow for
azure fixed per #1876 - add mocked credentials and CI-friendly fixture"); ensure
the test body remains intact so it can be re-enabled easily.
| await setConfig({ | ||
| fs, | ||
| gitdir, | ||
| path: 'user.name', | ||
| value: `user-config`, | ||
| }) | ||
|
|
||
| await setConfig({ | ||
| fs, | ||
| gitdir, | ||
| path: 'user.name', | ||
| value: `[email protected]`, | ||
| }) |
There was a problem hiding this comment.
Fix config key typo and inconsistent destructuring in normalizeAuthorObject tests
Two small issues that don’t currently break the tests but are worth correcting:
-
Config key typo in first test
The secondsetConfigcall repeatspath: 'user.name'but sets an email value, which looks like it was meant to configureuser.email:await setConfig({ fs, gitdir,
- path: 'user.name',
- value:
[email protected],
- path: 'user.email',
- value:
[email protected],
})
2. **Inconsistent destructuring in the “no value can be retrieved” test**
Other tests use the `gitdirsmfullpath` property from `makeFixtureAsSubmodule`, but this one aliases `gitdir` instead, which is confusing and may not be what you intended:
```diff
- const { fs, gitdir: gitdirsmfullpath } = await makeFixtureAsSubmodule(
- 'test-normalizeAuthorObject'
- )
+ const { fs, gitdirsmfullpath } = await makeFixtureAsSubmodule(
+ 'test-normalizeAuthorObject'
+ )
This keeps usage consistent and ensures you’re always exercising the full submodule gitdir path in these tests.
Also applies to: 116-123
🤖 Prompt for AI Agents
In __tests__/test-normalizeAuthorObject-in-submodule.js around lines 15 to 27
(and likewise adjust 116 to 123), fix the config key and the destructuring:
change the second setConfig call that currently uses path: 'user.name' to path:
'user.email' so the email is stored under the correct key, and in the “no value
can be retrieved” test use the same destructuring as other tests
(gitdirsmfullpath from makeFixtureAsSubmodule) instead of aliasing it to gitdir
so the test consistently uses the full submodule gitdir path.
| it('return undefined if no value can be retrieved', async () => { | ||
| // Setup | ||
| const { fs, gitdirsmfullpath } = await makeFixtureAsSubmodule( | ||
| 'test-normalizeAuthorObject' | ||
| ) | ||
|
|
||
| // Test | ||
| expect( | ||
| await normalizeCommitterObject({ fs, gitdir: gitdirsmfullpath }) | ||
| ).toBeUndefined() | ||
| }) |
There was a problem hiding this comment.
Same variable name issue: gitdirsmfullpath.
Lines 159 and 165 also use gitdirsmfullpath. This should likely be gitdir to match the rest of the test file and other submodule tests in the PR.
🤖 Prompt for AI Agents
In __tests__/test-normalizeCommitterObject-in-submodule.js around lines 157 to
167, the test uses the inconsistent variable name `gitdirsmfullpath` on lines
159 and 165; change those occurrences to `gitdir` (the name used elsewhere in
this test file and other submodule tests) so the fixture return value is
referenced correctly and the test matches the repository naming convention.
| // BrowserFS has a design quirk where HTTPRequestFS has a default mode of 555 for everything, | ||
| // meaning that files have the executable bit set by default! | ||
| const isBrowserFS = !!fs._original_unwrapped_fs.getRootFS | ||
| const FILEMODE = isBrowserFS ? 0o100755 : 0o100644 | ||
| const SYMLINKMODE = isBrowserFS ? 0o100755 : 0o120000 |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Guard access to fs._original_unwrapped_fs before reading getRootFS.
Right now const isBrowserFS = !!fs._original_unwrapped_fs.getRootFS will throw if _original_unwrapped_fs is missing on some backends. Safer would be something like:
const isBrowserFS =
!!fs._original_unwrapped_fs &&
typeof fs._original_unwrapped_fs.getRootFS === 'function'so non‑BrowserFS implementations don’t crash these tests.
Also applies to: 290-292
🤖 Prompt for AI Agents
In __tests__/test-walk-in-submodule.js around lines 44 to 48, accessing
fs._original_unwrapped_fs.getRootFS directly can throw when
_original_unwrapped_fs is missing; change the isBrowserFS check to first ensure
fs._original_unwrapped_fs exists and that getRootFS is a function (e.g. check
for truthiness of _original_unwrapped_fs and typeof getRootFS === 'function'),
and apply the same guarded check at the other occurrence around lines 290-292 so
non-BrowserFS backends don't crash the tests.
|
@sdarwin This is big PR, decided to add CodeRabbit as a help. |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (8)
__tests__/test-hosting-providers-in-submodule.js (2)
62-109: Disabled Azure push test: align with maintainer preference on skipped testsThe Azure push test is still disabled via
xit, which maintainers have previously pushed back on. Either remove it, or keep it but convert to an explicit skipped test (e.g.,it.skip) with a clear TODO referencing issue #1876 and conditions for re‑enabling.
207-253: Disabled GitLab suite: decide whether to keep, remove, or properly skipThe entire GitLab block is disabled via
xdescribe. Given earlier feedback about skipped tests, either:
- Remove this suite if it’s no longer supported, or
- Re‑enable it with appropriate credentials/CI handling, or
- Keep it but switch to
describe.skip(or similar) and add a brief top‑line comment/TODO with a reason, linked issue/PR, and re‑enable criteria.This keeps the intent clear and avoids silently forgotten disabled suites.
__tests__/test-commit-in-submodule.js (1)
158-187: Past review issue has been resolved.The dry run test now correctly destructures
gitdirsmfullpath(line 160) and uses it in the object existence assertion (lines 182-186), ensuring the test verifies the actual submodule git directory rather than the.gitfile pointer. This matches the correct pattern used in the "without updating branch" test above.package-scripts.cjs (1)
63-65: Address the formatting and redundant nesting issues flagged in the previous review.The previous review comment on these lines (and lines 73, 181) remains valid:
- Line 63 needs a newline fix to satisfy ESLint/Prettier.
- Lines 73 and 181 have redundant template literal wrapping around
timeout15(...)that should be simplified.Please apply the fixes suggested in the earlier review.
__tests__/test-normalizeAuthorObject-in-submodule.js (1)
22-27: Fix config key typo: should beuser.emailnotuser.name.The second
setConfigcall incorrectly usespath: 'user.name'when setting an email value, which overwrites the name configured in the first call and leavesuser.emailundefined.Apply this diff:
await setConfig({ fs, gitdir, - path: 'user.name', + path: 'user.email', value: `[email protected]`, })__tests__/test-walk-in-submodule.js (1)
44-49: Guard access tofs._original_unwrapped_fswhen detecting BrowserFS.
const isBrowserFS = !!fs._original_unwrapped_fs.getRootFSwill throw if_original_unwrapped_fsis missing, which is possible for non‑BrowserFS backends. That would break these tests in such environments.Use a guarded check here (and at the later occurrence) so the tests remain portable:
- const isBrowserFS = !!fs._original_unwrapped_fs.getRootFS + const isBrowserFS = + !!fs._original_unwrapped_fs && + typeof fs._original_unwrapped_fs.getRootFS === 'function'Apply the same change to the
isBrowserFSdeclaration in theautocrlftest.Also applies to: 290-293
__tests__/test-abortMerge-in-submodule.js (1)
327-333: Awaitfs.writebefore mutating the index to avoid a raceHere
fs.writeis asynchronous but not awaited:fs.write(`${dir}/c`, 'new changes to file c')Subsequent index deletion and reads can race with this write, making the test flaky or nondeterministic.
- fs.write(`${dir}/c`, 'new changes to file c') + await fs.write(`${dir}/c`, 'new changes to file c')__tests__/test-merge-in-submodule.js (1)
211-242:--noUpdateBranchtest still usingdryRuninstead ofnoUpdateBranchThe test name and intent here are to exercise
noUpdateBranch, but the options passdryRun: true, which just duplicates the previous dry‑run fast‑forward test and never covers the distinctnoUpdateBranchbehavior.Switch this to
noUpdateBranch: trueso the test matches its purpose:const m = await merge({ fs, gitdir, ours: 'master', theirs: 'newest', fastForwardOnly: true, - dryRun: true, + noUpdateBranch: true, })This keeps
masteratoriginalOidwhile still validating that the reportedoidisdesiredOidundernoUpdateBranch.
🧹 Nitpick comments (25)
__tests__/test-listObjects-in-submodule.js (3)
7-7: Use a more descriptive test name.The test name "listObjects" within
describe('listObjects', ...)is redundant and doesn't convey what specific behavior is being verified. Consider a name that describes the scenario, such as "walks complete object graph from multiple oids in submodule" or "returns all reachable objects from given commits".- it('listObjects', async () => { + it('walks complete object graph from multiple starting oids', async () => {
8-22: Add documentation explaining the test scenario.The test lacks comments explaining what scenario is being verified and why these specific oids were chosen. Consider adding comments that describe:
- What git objects these 4 input oids represent (commits, tags, trees?)
- What aspect of listObjects behavior this test validates
- Why these particular oids form a good test case for submodule support
This will help future maintainers understand the test's purpose if it fails or needs modification.
23-198: Consider alternatives to the large inline snapshot for improved maintainability.The 197-line inline snapshot makes this test file difficult to read and maintain. If the test fails or the fixture changes, debugging will be challenging since the snapshot contains only opaque hashes without semantic context.
Consider these alternatives:
- Verify specific properties instead: object count, presence of key objects, or object type distribution
- Move expected results to a separate JSON fixture file with documentation
- Test a smaller, more focused object graph that still validates the core behavior
However, if comprehensive validation of the complete object graph is important for submodule correctness, the current approach provides thorough coverage. In that case, adding a comment explaining this tradeoff would help future maintainers.
__tests__/test-config-in-submodule.js (2)
7-33: Good coverage of basic config operations.The test effectively validates:
- Boolean config values (core.symlinks)
- Subsection paths (remote.origin.url)
- Multi-value config retrieval with getConfigAll
- Single value retrieval from multi-value configs
Consider adding tests for edge cases such as:
- Non-existent config keys (should return undefined)
- Case sensitivity of sections vs. variable names
- Config values with special characters or quotes
35-61: Good validation of setConfig operations.The test covers essential scenarios including boolean values, deletion via undefined, and URL modification. The pattern of setting then verifying with getConfig is solid.
Consider adding a test for the
appendparameter of setConfig (line 67 in src/api/setConfig.js showsappend = falseoption), which would validate multi-value config management:// Test appending to multi-value config await setConfig({ fs, gitdir, path: 'remote.upstream.fetch', value: '+refs/heads/feature/*:refs/remotes/upstream/feature/*', append: true }) const allFetches = await getConfigAll({ fs, gitdir, path: 'remote.upstream.fetch' }) expect(allFetches).toHaveLength(4)package-scripts.cjs (1)
18-18: Consider removing commented-out code.The old
timeout5declaration is no longer needed and can be deleted for cleanliness.Apply this diff:
-// const timeout5 = timeout(5) const timeout15 = timeout(15)__tests__/test-hashBlob-in-submodule.js (2)
41-60: File name suggests submodule behavior but test doesn’t set up a submodule contextThis suite never creates or
cwds into a submodule (no use of the submodule fixtures), andhashBlobitself is gitdir‑agnostic. If this is meant to participate in the “run commands from submodules” coverage, consider wiring it through the submodule fixture helper; otherwise, renaming/moving it to the generichashBlobtests would better match its true scope.
47-49: SimplifyBuffer.compareassertions for clarityYou can write these expectations more directly, e.g.:
expect(Buffer.compare(Buffer.from(object), wrapped)).toBe(0) // or expect(Buffer.from(object).equals(wrapped)).toBe(true)Functionally identical, but a bit clearer than asserting on
=== 0inside theexpect.Also applies to: 57-59
__tests__/test-readCommit-in-submodule.js (4)
7-23: Consider more descriptive test name.The test name "test missing" is somewhat vague. Consider renaming to something more explicit like "throws NotFoundError for unknown OID" to better describe the expected behavior and improve test readability.
Apply this diff:
- it('test missing', async () => { + it('throws NotFoundError for unknown OID', async () => {
24-82: Large inline snapshot - consider submodule-specific assertions.The inline snapshot provides comprehensive validation but includes a 45+ line GPG signature. While this is valid, consider whether:
- The full GPG signature is necessary for this submodule-specific test, or if a subset of fields would suffice
- Adding an explicit assertion that verifies submodule gitdir discovery occurred (e.g., checking that the fixture's
.gitfile was followed)The test is functionally correct but could be clearer about what submodule-specific behavior it validates beyond standard
readCommitfunctionality.
125-136: LGTM - tag peeling test is appropriate.The test correctly validates that tags are peeled to their underlying commit OID in a submodule context. The minimal assertion is appropriate since tag peeling is the specific behavior being tested.
Optional: Consider adding a comment indicating what tag is being tested (e.g.,
// Test peeling annotated tag v0.0.5).
4-9: Submodule fixture verification shows proper structure; optional documentation suggested.The fixture helper
makeFixtureAsSubmodulecorrectly creates proper submodule structure—it writes a.gitfile withgitdir: ../.git/modules/mysubmodule\npointing to the superproject's modules directory. This verification confirms the tests are running on correctly-formed submodule fixtures.However, the original review's suggestion to add clarifying comments is still valid: the test file itself doesn't indicate why it uses
makeFixtureAsSubmoduleor what submodule-specific behavior is being tested. Adding a brief comment explaining the setup (e.g., "This test usesmakeFixtureAsSubmoduleto test readCommit against a proper submodule structure where .git is a file with gitdir pointer") would improve maintainability without adding test assertions.The existing tests work correctly as written; the refactoring is optional for clarity.
__tests__/test-status-in-submodule.js (1)
8-52: Ensure this actually exercises.gitfile discovery in submodulesThis mirrors the core
statusmatrix nicely in a submodule context, including staged/unstaged and starred states, and the flow looks sound. One thing to double‑check: whatgitdirfrommakeFixtureAsSubmodule('test-status')points to.To get coverage for the new gitdir‑discovery logic,
gitdirhere should either:
- Be
${dir}/.gitwhen that path is a file containinggitdir: …, or- Be omitted entirely so
status/add/removeuse their defaultgitdir = join(dir, '.git')and have to resolve the.gitfile themselves.If
makeFixtureAsSubmodulealready returns the resolved modules directory (e.g.,.git/modules/...), these tests will validate behavior inside a submodule repo but won’t hit the.git‑file resolution branch you just added. In that case, consider dropping the explicitgitdirargument here or adjusting the fixture to return the pointer path so the discovery code is exercised.__tests__/test-deleteRef-in-submodule.js (1)
6-6: Consider clarifying that this test suite is submodule-specific.The describe block is named
'deleteRef', but the file name istest-deleteRef-in-submodule.js. Consider renaming to'deleteRef in submodule'so test output clearly indicates these are submodule-specific tests, improving clarity when reviewing test results.Apply this diff:
-describe('deleteRef', () => { +describe('deleteRef in submodule', () => {__tests__/test-clone-in-submodule.js (3)
24-109: Good reuse of existing clone scenarios in submodule context; watch external GitHub usageAdapting the “noTags”, “noCheckout”, and “clone a tag” scenarios to run via
makeFixtureAsSubmoduleis a solid way to validateclone+discoverGitdirbehavior in submodules. These three tests also hithttps://github.com/isomorphic-git/isomorphic-git.git, which duplicates live network coverage already present in the non‑submodule clone tests and increases CI’s dependency on external connectivity.If CI time or flakiness becomes a problem, consider switching these to the local git-http-mock-server fixtures (like
test-clone-karma) or gating them more aggressively, since the functional behavior under test is largely orthogonal to the actual GitHub host.
174-399: Guardresult.valuebefore calling.indexOf(...)in Proxy wrappersIn the network‑error simulation tests, the Proxy around
body.nextassumesresult.valueis always truthy and has anindexOfmethod:if (~result.value.indexOf('shallow')) { ... }If the iterator ever yields
{ done: true, value: undefined }or a non‑Buffer/array chunk (e.g., due to fixture changes), this will throw aTypeErrorinstead of the intendedECONNRESET-annotated error, making the tests brittle.You can keep the same behavior but make the guard more robust:
- body.next = new Proxy(body.next, { - apply(target, self, args) { - const result = target.apply(self, args) - if (~result.value.indexOf('shallow')) { - throw Object.assign(new Error('aborted'), { - code: 'ECONNRESET', - }) - } - return result - }, - }) + body.next = new Proxy(body.next, { + apply(target, self, args) { + const result = target.apply(self, args) + const value = result && result.value + if ( + value && + typeof value.indexOf === 'function' && + value.indexOf('shallow') !== -1 + ) { + throw Object.assign(new Error('aborted'), { + code: 'ECONNRESET', + }) + } + return result + }, + })and similarly for the other two proxies that check for
'shallow'and'PACK'. This keeps the semantics but avoids accidentalTypeErrors if the iterator shape changes.
485-506: Remove redundantawaitinsidePromise.allHere:
const [merge, remote] = await Promise.all([ await getConfig({ fs, dir, gitdir, path: 'branch.main.merge' }), await getConfig({ fs, dir, gitdir, path: 'branch.main.remote' }), ])the inner
awaits force the twogetConfigcalls to run sequentially beforePromise.allsees them, defeating the purpose ofPromise.all.They can be run concurrently and the code simplified:
- const [merge, remote] = await Promise.all([ - await getConfig({ fs, dir, gitdir, path: 'branch.main.merge' }), - await getConfig({ fs, dir, gitdir, path: 'branch.main.remote' }), - ]) + const [merge, remote] = await Promise.all([ + getConfig({ fs, dir, gitdir, path: 'branch.main.merge' }), + getConfig({ fs, dir, gitdir, path: 'branch.main.remote' }), + ])Functionally it’s the same here, but this avoids unnecessary sequential waits and matches typical Promise usage.
__tests__/test-GitRefManager-in-submodule.js (1)
7-308: ThoroughGitRefManagersubmodule coverage; concurrency test is acceptable but a bit heavy.The tests exercise packed refs, lists of refs/branches/tags, and direct
gitdirsmfullpathusage, which is exactly what we need to validate submodule behavior. The concurrent read/write test with 500 randomized iterations is a solid stress test; if it ever becomes a bottleneck or flaky, you could switch to a deterministic interleaving pattern, but it’s fine as written for now.__tests__/test-checkout-in-submodule.js (2)
14-17: Remove duplicateeslint-envcomment.There are two identical
/* eslint-env node, browser, jasmine */comments back‑to‑back. It’s harmless but redundant; dropping one will keep the file slightly cleaner.
586-633: Strengthen thetrack: falsetest by also checkingbranch.test-branch.remote.This test currently asserts that
branch.test-branch.mergeandbranch.main.remoteare undefined after checking out withtrack: false. To fully pin down the “no tracking config” behavior for the target branch, consider also asserting that:
branch.test-branch.remoteis undefined.That would catch any future regression where
track: falsestill writes abranch.test-branch.remoteentry even ifmergeremains unset.__tests__/test-fetch-in-submodule.js (1)
41-89: Make the shallow‑fetch progress assertions less brittle (and possibly avoid fixed sleeps).The shallow‑fetch tests correctly use
gitdirsmfullpathfor theshallowfile, but two aspects are somewhat fragile:
output[output.length - 1].split(' ')[1] === '551'depends on the exact wording and tokenization of server messages, which can change between Git versions or hosting providers.- Fixed
await sleep(1000)calls add latency and still assume that the side effects (writingshallow) complete within 1s.If these ever cause flakes, consider:
- Asserting that some message in
outputmatches a regexp like/\b551\b/instead of inspecting a specific token position, and/or- Replacing the fixed
sleepwith a small polling loop that waits until${gitdirsmfullpath}/shallowexists (or a short timeout elapses).Functionally everything is correct today; this would just harden the tests against future environment changes.
Also applies to: 226-264
__tests__/test-abortMerge-in-submodule.js (1)
28-38: Compare only OIDs (or content) instead of entirereadBlobresult objects
fileAVersionsandfileAStages/fileBStageshold fullreadBlobreturn objects. Deep‑equality on these couples the test to the exact object shape (e.g., presence/absence offilepath) and may fail ifreadBlob’s structure changes, even when the staged OIDs are correct.To make the test more robust and focused on intent, assert just the OIDs (or blobs), for example:
- const fileAStages = [ - await readBlob({ fs, gitdir, oid: index.entriesMap.get('a').stages[1].oid }), - await readBlob({ fs, gitdir, oid: index.entriesMap.get('a').stages[2].oid }), - await readBlob({ fs, gitdir, oid: index.entriesMap.get('a').stages[3].oid }), - ] + const fileAStageOids = [ + (await readBlob({ fs, gitdir, oid: index.entriesMap.get('a').stages[1].oid })).oid, + (await readBlob({ fs, gitdir, oid: index.entriesMap.get('a').stages[2].oid })).oid, + (await readBlob({ fs, gitdir, oid: index.entriesMap.get('a').stages[3].oid })).oid, + ] @@ - const fileBStages = [ - await readBlob({ fs, gitdir, oid: index.entriesMap.get('b').stages[1].oid }), - await readBlob({ fs, gitdir, oid: index.entriesMap.get('b').stages[2].oid }), - await readBlob({ fs, gitdir, oid: index.entriesMap.get('b').stages[3].oid }), - ] + const fileBStageOids = [ + (await readBlob({ fs, gitdir, oid: index.entriesMap.get('b').stages[1].oid })).oid, + (await readBlob({ fs, gitdir, oid: index.entriesMap.get('b').stages[2].oid })).oid, + (await readBlob({ fs, gitdir, oid: index.entriesMap.get('b').stages[3].oid })).oid, + ] @@ - expect(fileAVersions).toEqual(fileAStages) - expect(fileBVersions).toEqual(fileBStages) + expect(fileAVersions.map(v => v.oid)).toEqual(fileAStageOids) + expect(fileBVersions.map(v => v.oid)).toEqual(fileBStageOids)This keeps the assertion aligned with the test’s goal (correct staging of the three versions) and avoids brittleness around object shape.
Also applies to: 73-108
__tests__/test-stash-in-submodule.js (3)
16-41: ClarifymakeFixtureStashresponsibilities and parameter namingThe helper correctly routes everything through
makeFixtureAsSubmoduleand handles the browser special-case copying of both working tree and gitdir, which is what you want for submodule tests.However, the
testDirparameter is only used to derive target directory names in theprocess.browserbranch and is not passed down tomakeFixtureAsSubmodule, whilefixtureDiris hardcoded to'test-stash'. Given the parameter name, this is a bit surprising and makes it harder to see which parts are “fixture name” vs “per-test instance id”.Consider either:
- Passing
testDir(or another explicit fixture name) through tomakeFixtureAsSubmodule, or- Renaming
testDirto something likebrowserTestId(and documenting that all tests share the'test-stash'fixture) to make intent clearer.This is only about clarity/maintainability; behavior looks fine as-is.
54-111: Tighten upstashChangessignature and naming for readability
stashChangesis the core setup helper used across many tests and the behavior is non-trivial (staged vs unstaged changes, same-file changes, error handling). The implementation looks correct, but a couple of details hurt readability:
- The
defaltparameter is both misspelled and not very descriptive of its purpose (controlling inclusion of unstaged changes /m.xmlwrites).- The
againparameter’s meaning (“add a second unstaged change to a.txt”) is only obvious once you read through the whole function.Given how heavily this helper is used, consider an optional refactor such as:
- Renaming the booleans to something self-describing, e.g.
includeUnstagedandincludeSecondUnstagedChange, or- Switching to an options object for the last parameters, e.g.:
-const stashChanges = async ( - fs, - dir, - gitdir, - defalt = true, - again = true, - message = '' -) => { +const stashChanges = async ( + fs, + dir, + gitdir, + { + includeUnstaged = true, + includeSecondUnstagedChange = true, + message = '', + } = {} +) => { // ... - if (defalt) { + if (includeUnstaged) { // ... - if (again) { + if (includeSecondUnstagedChange) { // ... } }Call sites would then pass
{ includeUnstaged: false },{ includeSecondUnstagedChange: false }, etc., which is easier to reason about when skimming the tests.
117-172: Consider a small helper for repeated error capture patternsThroughout the file there’s a repeated pattern of:
let error = null try { await someAsyncOp() } catch (e) { error = e } expect(error).toBeNull() // or not.toBeNull(), plus optional code checksFunctionally this is fine, but it’s quite verbose and duplicated across many tests. For future readability you might consider:
- Introducing a tiny helper like
await expectAsyncOpToFail(fn)/expectAsyncOpToSucceed(fn)that wraps this pattern, or- Where the assertion is only about failure, using Jasmine/Jest’s native async helpers (
await expectAsync(...).toBeRejected()/.toBeResolved()equivalents, depending on the test runner configuration).Not urgent, but it would make these long test files easier to scan.
Also applies to: 227-252, 246-252, 319-332, 373-381, 407-413, 442-449, 479-486, 517-523, 652-657, 676-681, 705-709, 737-753, 774-787, 812-825, 855-863, 884-903, 913-918, 940-945, 1013-1018, 1028-1033, 1045-1050, 1070-1075, 1091-1096, 1106-1111, 1124-1129, 1141-1146, 1156-1161, 1175-1179, 1192-1197, 1220-1225, 1247-1266, 1273-1297
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (107)
.config/jest.js(1 hunks)__tests__/__helpers__/FixtureFSSubmodule.js(1 hunks)__tests__/test-GitAnnotatedTag-in-submodule.js(1 hunks)__tests__/test-GitConfig-in-submodule.js(1 hunks)__tests__/test-GitError-in-submodule.js(1 hunks)__tests__/test-GitIndex-in-submodule.js(1 hunks)__tests__/test-GitPackIndex-in-submodule.js(1 hunks)__tests__/test-GitPktLine-in-submodule.js(1 hunks)__tests__/test-GitRefManager-in-submodule.js(1 hunks)__tests__/test-GitRefSpecSet-in-submodule.js(1 hunks)__tests__/test-GitRemoteManager-in-submodule.js(1 hunks)__tests__/test-GitSideBand-in-submodule.js(1 hunks)__tests__/test-abortMerge-in-submodule.js(1 hunks)__tests__/test-add-in-submodule.js(1 hunks)__tests__/test-addNote-in-submodule.js(1 hunks)__tests__/test-addRemote-in-submodule.js(1 hunks)__tests__/test-annotatedTag-in-submodule.js(1 hunks)__tests__/test-branch-in-submodule.js(1 hunks)__tests__/test-checkout-in-submodule.js(1 hunks)__tests__/test-clone-checkout-huge-repo-in-submodule.js(1 hunks)__tests__/test-clone-in-submodule.js(1 hunks)__tests__/test-commit-in-submodule.js(1 hunks)__tests__/test-config-in-submodule.js(1 hunks)__tests__/test-currentBranch-in-submodule.js(1 hunks)__tests__/test-deleteBranch-in-submodule.js(1 hunks)__tests__/test-deleteRef-in-submodule.js(1 hunks)__tests__/test-deleteRemote-in-submodule.js(1 hunks)__tests__/test-deleteTag-in-submodule.js(1 hunks)__tests__/test-expandOid-in-submodule.js(1 hunks)__tests__/test-exports-in-submodule.js(1 hunks)__tests__/test-fetch-in-submodule.js(1 hunks)__tests__/test-findMergeBase-in-submodule.js(1 hunks)__tests__/test-findRoot-in-submodule.js(1 hunks)__tests__/test-flatFileListToDirectoryStructure-in-submodule.js(1 hunks)__tests__/test-getRemoteInfo-in-submodule.js(1 hunks)__tests__/test-getRemoteInfo2-in-submodule.js(1 hunks)__tests__/test-hashBlob-in-submodule.js(1 hunks)__tests__/test-hosting-providers-in-submodule.js(1 hunks)__tests__/test-init-in-submodule.js(1 hunks)__tests__/test-isBinary-in-submodule.js(1 hunks)__tests__/test-isIgnored-in-submodule.js(1 hunks)__tests__/test-listBranches-in-submodule.js(1 hunks)__tests__/test-listCommitsAndTags-in-submodule.js(1 hunks)__tests__/test-listFiles-in-submodule.js(1 hunks)__tests__/test-listNotes-in-submodule.js(1 hunks)__tests__/test-listObjects-in-submodule.js(1 hunks)__tests__/test-listRefs-in-submodule.js(1 hunks)__tests__/test-listRemotes-in-submodule.js(1 hunks)__tests__/test-listServerRefs-in-submodule.js(1 hunks)__tests__/test-listTags-in-submodule.js(1 hunks)__tests__/test-log-file-in-submodule.js(1 hunks)__tests__/test-log-in-submodule.js(1 hunks)__tests__/test-merge-in-submodule.js(1 hunks)__tests__/test-mergeFile-in-submodule.js(1 hunks)__tests__/test-normalizeAuthorObject-in-submodule.js(1 hunks)__tests__/test-normalizeCommitterObject-in-submodule.js(1 hunks)__tests__/test-packObjects-in-submodule.js(1 hunks)__tests__/test-pull-in-submodule.js(1 hunks)__tests__/test-push-in-submodule.js(1 hunks)__tests__/test-readBlob-in-submodule.js(1 hunks)__tests__/test-readCommit-in-submodule.js(1 hunks)__tests__/test-readNote-in-submodule.js(1 hunks)__tests__/test-readObject-in-submodule.js(1 hunks)__tests__/test-readTag-in-submodule.js(1 hunks)__tests__/test-readTree-in-submodule.js(1 hunks)__tests__/test-remove-in-submodule.js(1 hunks)__tests__/test-removeNote-in-submodule.js(1 hunks)__tests__/test-renameBranch-in-submodule.js(1 hunks)__tests__/test-resetIndex-in-submodule.js(1 hunks)__tests__/test-resolveRef-in-submodule.js(1 hunks)__tests__/test-stash-in-submodule.js(1 hunks)__tests__/test-status-in-submodule.js(1 hunks)__tests__/test-statusMatrix-in-submodule.js(1 hunks)__tests__/test-submodules-in-submodule.js(1 hunks)__tests__/test-tag-in-submodule.js(1 hunks)__tests__/test-unicode-paths-in-submodule.js(1 hunks)__tests__/test-updateIndex-in-submodule.js(1 hunks)__tests__/test-uploadPack-in-submodule.js(1 hunks)__tests__/test-utils-join-in-submodule.js(1 hunks)__tests__/test-validate-in-submodule.js(1 hunks)__tests__/test-version-in-submodule.js(1 hunks)__tests__/test-walk-in-submodule.js(1 hunks)__tests__/test-wire-in-submodule.js(1 hunks)__tests__/test-writeBlob-in-submodule.js(1 hunks)__tests__/test-writeCommit-in-submodule.js(1 hunks)__tests__/test-writeObject-in-submodule.js(1 hunks)__tests__/test-writeRef-in-submodule.js(1 hunks)__tests__/test-writeTag-in-submodule.js(1 hunks)__tests__/test-writeTree-in-submodule.js(1 hunks)azure-pipelines.yml(1 hunks)package-scripts.cjs(3 hunks)src/api/abortMerge.js(4 hunks)src/api/add.js(2 hunks)src/api/addNote.js(2 hunks)src/api/addRemote.js(2 hunks)src/api/annotatedTag.js(2 hunks)src/api/branch.js(2 hunks)src/api/checkout.js(2 hunks)src/api/clone.js(3 hunks)src/api/commit.js(2 hunks)src/api/currentBranch.js(2 hunks)src/api/deleteBranch.js(2 hunks)src/api/deleteRef.js(2 hunks)src/api/deleteRemote.js(2 hunks)src/api/deleteTag.js(2 hunks)src/api/expandOid.js(2 hunks)src/api/expandRef.js(2 hunks)
⛔ Files not processed due to max files limit (11)
- src/api/fastForward.js
- src/api/fetch.js
- src/api/findMergeBase.js
- src/api/getConfig.js
- src/api/getConfigAll.js
- src/api/indexPack.js
- src/api/init.js
- src/api/isDescendent.js
- src/api/isIgnored.js
- src/api/listBranches.js
- src/api/listFiles.js
✅ Files skipped from review due to trivial changes (3)
- tests/test-log-file-in-submodule.js
- tests/test-wire-in-submodule.js
- tests/test-currentBranch-in-submodule.js
🚧 Files skipped from review as they are similar to previous changes (67)
- tests/test-uploadPack-in-submodule.js
- tests/test-listCommitsAndTags-in-submodule.js
- src/api/deleteRef.js
- src/api/commit.js
- tests/test-addNote-in-submodule.js
- src/api/expandOid.js
- tests/test-removeNote-in-submodule.js
- src/api/addRemote.js
- tests/test-readObject-in-submodule.js
- tests/test-deleteTag-in-submodule.js
- tests/test-clone-checkout-huge-repo-in-submodule.js
- tests/test-resolveRef-in-submodule.js
- src/api/clone.js
- tests/test-isBinary-in-submodule.js
- tests/test-writeRef-in-submodule.js
- src/api/checkout.js
- tests/test-readTree-in-submodule.js
- tests/test-getRemoteInfo-in-submodule.js
- tests/test-mergeFile-in-submodule.js
- tests/test-isIgnored-in-submodule.js
- tests/test-readBlob-in-submodule.js
- tests/test-listTags-in-submodule.js
- tests/test-writeTree-in-submodule.js
- tests/test-resetIndex-in-submodule.js
- tests/test-GitSideBand-in-submodule.js
- tests/test-normalizeCommitterObject-in-submodule.js
- tests/test-log-in-submodule.js
- tests/test-annotatedTag-in-submodule.js
- src/api/deleteTag.js
- tests/test-deleteRemote-in-submodule.js
- tests/test-listServerRefs-in-submodule.js
- tests/test-tag-in-submodule.js
- tests/test-readNote-in-submodule.js
- tests/test-writeObject-in-submodule.js
- tests/test-expandOid-in-submodule.js
- tests/test-renameBranch-in-submodule.js
- tests/test-readTag-in-submodule.js
- src/api/currentBranch.js
- tests/test-listBranches-in-submodule.js
- src/api/deleteRemote.js
- tests/test-listFiles-in-submodule.js
- .config/jest.js
- tests/test-add-in-submodule.js
- tests/test-GitRefSpecSet-in-submodule.js
- tests/test-exports-in-submodule.js
- tests/test-pull-in-submodule.js
- tests/test-listRemotes-in-submodule.js
- tests/test-statusMatrix-in-submodule.js
- tests/test-utils-join-in-submodule.js
- azure-pipelines.yml
- tests/test-listNotes-in-submodule.js
- tests/test-packObjects-in-submodule.js
- src/api/add.js
- tests/test-unicode-paths-in-submodule.js
- tests/test-GitPktLine-in-submodule.js
- tests/helpers/FixtureFSSubmodule.js
- tests/test-validate-in-submodule.js
- tests/test-GitPackIndex-in-submodule.js
- tests/test-remove-in-submodule.js
- tests/test-branch-in-submodule.js
- tests/test-getRemoteInfo2-in-submodule.js
- tests/test-GitRemoteManager-in-submodule.js
- tests/test-submodules-in-submodule.js
- tests/test-deleteBranch-in-submodule.js
- tests/test-push-in-submodule.js
- tests/test-GitError-in-submodule.js
- tests/test-version-in-submodule.js
🧰 Additional context used
🧬 Code graph analysis (36)
__tests__/test-abortMerge-in-submodule.js (11)
src/api/resolveRef.js (1)
resolveRef(27-52)src/api/readBlob.js (1)
readBlob(45-71)src/api/merge.js (1)
merge(125-196)src/managers/GitIndexManager.js (1)
GitIndexManager(65-116)__tests__/__helpers__/FixtureFSSubmodule.js (1)
gitdirsmfullpath(56-56)src/api/abortMerge.js (1)
abortMerge(40-126)src/commands/TREE.js (1)
TREE(12-21)src/commands/WORKDIR.js (1)
WORKDIR(10-19)src/commands/STAGE.js (1)
STAGE(10-19)src/utils/modified.js (1)
modified(8-23)src/api/add.js (1)
add(36-74)
__tests__/test-addRemote-in-submodule.js (2)
src/api/addRemote.js (1)
addRemote(33-59)src/api/listRemotes.js (1)
listRemotes(25-40)
__tests__/test-readCommit-in-submodule.js (1)
src/api/readCommit.js (1)
readCommit(32-56)
__tests__/test-hosting-providers-in-submodule.js (3)
src/utils/extractAuthFromUrl.js (1)
username(11-11)src/api/fetch.js (1)
fetch(68-130)src/api/push.js (1)
push(58-110)
__tests__/test-checkout-in-submodule.js (6)
src/api/checkout.js (1)
checkout(66-114)src/api/listFiles.js (1)
listFiles(34-57)__tests__/__helpers__/FixtureFSSubmodule.js (1)
gitdirsmfullpath(56-56)src/api/add.js (1)
add(36-74)src/api/setConfig.js (1)
setConfig(52-79)src/api/getConfig.js (1)
getConfig(35-52)
__tests__/test-stash-in-submodule.js (4)
src/api/setConfig.js (1)
setConfig(52-79)src/api/add.js (1)
add(36-74)src/api/stash.js (1)
stash(74-132)src/api/readCommit.js (1)
readCommit(32-56)
__tests__/test-GitIndex-in-submodule.js (3)
src/models/GitIndex.js (1)
GitIndex(33-301)src/managers/GitIndexManager.js (1)
GitIndexManager(65-116)__tests__/__helpers__/FixtureFSSubmodule.js (1)
gitdirsmfullpath(56-56)
__tests__/test-config-in-submodule.js (3)
src/api/getConfig.js (1)
getConfig(35-52)src/api/getConfigAll.js (1)
getConfigAll(26-48)src/api/setConfig.js (1)
setConfig(52-79)
__tests__/test-status-in-submodule.js (2)
src/api/add.js (1)
add(36-74)src/api/remove.js (1)
remove(29-53)
__tests__/test-merge-in-submodule.js (7)
src/api/merge.js (1)
merge(125-196)src/api/resolveRef.js (1)
resolveRef(27-52)src/api/log.js (1)
log(39-73)__tests__/__helpers__/FixtureFSSubmodule.js (1)
gitdirsmfullpath(56-56)src/utils/mergeFile.js (5)
LINEBREAKS(3-3)ours(13-13)base(14-14)theirs(15-15)mergedText(23-23)src/api/add.js (1)
add(36-74)src/api/statusMatrix.js (2)
statusMatrix(158-247)entry(237-237)
__tests__/test-findRoot-in-submodule.js (1)
src/api/findRoot.js (1)
findRoot(28-38)
__tests__/test-normalizeAuthorObject-in-submodule.js (3)
src/api/setConfig.js (1)
setConfig(52-79)src/utils/normalizeAuthorObject.js (1)
normalizeAuthorObject(20-46)__tests__/__helpers__/FixtureFSSubmodule.js (1)
gitdirsmfullpath(56-56)
__tests__/test-findMergeBase-in-submodule.js (1)
src/api/findMergeBase.js (1)
findMergeBase(21-45)
__tests__/test-walk-in-submodule.js (5)
__tests__/__helpers__/FixtureFSSubmodule.js (1)
gitdirsmfullpath(56-56)src/commands/WORKDIR.js (1)
WORKDIR(10-19)src/commands/TREE.js (1)
TREE(12-21)src/commands/STAGE.js (1)
STAGE(10-19)src/api/setConfig.js (1)
setConfig(52-79)
__tests__/test-hashBlob-in-submodule.js (1)
src/api/hashBlob.js (1)
hashBlob(35-58)
__tests__/test-init-in-submodule.js (4)
src/api/init.js (1)
init(26-53)__tests__/__helpers__/FixtureFSSubmodule.js (1)
gitdirsmfullpath(56-56)src/api/setConfig.js (1)
setConfig(52-79)src/api/getConfig.js (1)
getConfig(35-52)
src/api/annotatedTag.js (1)
src/utils/discoverGitdir.js (1)
discoverGitdir(27-50)
__tests__/test-listRefs-in-submodule.js (1)
src/api/listRefs.js (1)
listRefs(24-40)
src/api/abortMerge.js (2)
src/utils/discoverGitdir.js (1)
discoverGitdir(27-50)src/managers/GitIndexManager.js (1)
GitIndexManager(65-116)
__tests__/test-GitRefManager-in-submodule.js (1)
__tests__/__helpers__/FixtureFSSubmodule.js (1)
gitdirsmfullpath(56-56)
__tests__/test-writeTag-in-submodule.js (1)
src/api/writeTag.js (1)
writeTag(47-64)
__tests__/test-updateIndex-in-submodule.js (3)
src/api/updateIndex.js (1)
updateIndex(52-171)src/api/add.js (1)
add(36-74)src/api/writeBlob.js (1)
writeBlob(32-51)
__tests__/test-clone-in-submodule.js (6)
src/api/clone.js (1)
clone(55-124)__tests__/__helpers__/FixtureFSSubmodule.js (1)
gitdirsmfullpath(56-56)src/api/resolveRef.js (1)
resolveRef(27-52)src/api/currentBranch.js (1)
currentBranch(32-54)src/api/checkout.js (1)
checkout(66-114)src/api/getConfig.js (1)
getConfig(35-52)
__tests__/test-writeCommit-in-submodule.js (1)
src/api/writeCommit.js (1)
writeCommit(23-45)
__tests__/test-flatFileListToDirectoryStructure-in-submodule.js (1)
src/utils/flatFileListToDirectoryStructure.js (1)
flatFileListToDirectoryStructure(15-58)
__tests__/test-fetch-in-submodule.js (4)
src/api/setConfig.js (1)
setConfig(52-79)src/api/fetch.js (1)
fetch(68-130)__tests__/__helpers__/FixtureFSSubmodule.js (1)
gitdirsmfullpath(56-56)src/utils/sleep.js (1)
sleep(1-3)
__tests__/test-commit-in-submodule.js (6)
src/api/log.js (1)
log(39-73)src/api/init.js (1)
init(26-53)src/api/add.js (1)
add(36-74)src/api/resolveRef.js (1)
resolveRef(27-52)__tests__/__helpers__/FixtureFSSubmodule.js (1)
gitdirsmfullpath(56-56)src/api/readCommit.js (1)
readCommit(32-56)
__tests__/test-deleteRef-in-submodule.js (2)
src/api/deleteRef.js (1)
deleteRef(26-37)src/api/listTags.js (1)
listTags(23-34)
src/api/branch.js (3)
src/models/FileSystem.js (1)
FileSystem(63-308)src/utils/discoverGitdir.js (1)
discoverGitdir(27-50)src/commands/branch.js (1)
_branch(29-72)
__tests__/test-GitConfig-in-submodule.js (1)
src/models/GitConfig.js (1)
GitConfig(148-296)
src/api/expandRef.js (2)
src/models/FileSystem.js (1)
FileSystem(63-308)src/utils/discoverGitdir.js (1)
discoverGitdir(27-50)
__tests__/test-writeBlob-in-submodule.js (1)
src/api/writeBlob.js (1)
writeBlob(32-51)
src/api/deleteBranch.js (3)
src/models/FileSystem.js (1)
FileSystem(63-308)src/utils/discoverGitdir.js (1)
discoverGitdir(27-50)src/commands/deleteBranch.js (1)
_deleteBranch(16-39)
__tests__/test-GitAnnotatedTag-in-submodule.js (1)
src/models/GitAnnotatedTag.js (1)
GitAnnotatedTag(6-118)
src/api/addNote.js (2)
src/models/FileSystem.js (1)
FileSystem(63-308)src/utils/discoverGitdir.js (1)
discoverGitdir(27-50)
__tests__/test-listObjects-in-submodule.js (2)
src/commands/listObjects.js (1)
listObjects(16-59)__tests__/__helpers__/FixtureFSSubmodule.js (1)
gitdirsmfullpath(56-56)
| /* eslint-env node, browser, jasmine */ | ||
| import { flatFileListToDirectoryStructure } from 'isomorphic-git/internal-apis' | ||
|
|
||
| describe('flatFileListToDirectoryStructure', () => { | ||
| it('simple', async () => { | ||
| const inode = flatFileListToDirectoryStructure([ | ||
| { path: 'hello/there.txt' }, | ||
| ]).get('.') | ||
| expect(inode.fullpath).toBe('.') | ||
| expect(inode.type).toBe('tree') | ||
| expect(inode.children.length).toBe(1) | ||
| const hello = inode.children[0] | ||
| expect(hello.type).toBe('tree') | ||
| expect(hello.fullpath).toBe('hello') | ||
| expect(hello.basename).toBe('hello') | ||
| expect(hello.parent).toBe(inode) | ||
| expect(hello.children.length).toBe(1) | ||
| const there = hello.children[0] | ||
| expect(there.type).toBe('blob') | ||
| expect(there.fullpath).toBe('hello/there.txt') | ||
| expect(there.basename).toBe('there.txt') | ||
| }) | ||
|
|
||
| it('advanced', async () => { | ||
| const filelist = [ | ||
| '.babelrc', | ||
| '.editorconfig', | ||
| '.flowconfig', | ||
| '.gitignore', | ||
| '.travis.yml', | ||
| 'LICENSE.md', | ||
| 'README.md', | ||
| 'package-lock.json', | ||
| 'package.json', | ||
| 'shrinkwrap.yaml', | ||
| 'src/commands/checkout.js', | ||
| 'src/commands/config.js', | ||
| 'src/commands/fetch.js', | ||
| 'src/commands/init.js', | ||
| 'src/index.js', | ||
| 'src/models/GitBlob.js', | ||
| 'src/models/GitCommit.js', | ||
| 'src/models/GitConfig.js', | ||
| 'src/models/GitObject.js', | ||
| 'src/models/GitTree.js', | ||
| 'src/utils/exists.js', | ||
| 'src/utils/mkdirs.js', | ||
| 'src/utils/read.js', | ||
| 'src/utils/resolveRef.js', | ||
| 'src/utils/write.js', | ||
| 'test/_helpers.js', | ||
| 'test/snapshots/test-resolveRef.js.md', | ||
| 'test/snapshots/test-resolveRef.js.snap', | ||
| 'test/test-clone.js', | ||
| 'test/test-config.js', | ||
| 'test/test-init.js', | ||
| 'test/test-resolveRef.js', | ||
| ] | ||
| const files = filelist.map(f => ({ path: f, someMeta: f.length })) | ||
| const inodes = flatFileListToDirectoryStructure(files) | ||
| expect(inodes.get('.')).toMatchInlineSnapshot(` | ||
| { | ||
| "basename": ".", | ||
| "children": [ | ||
| { | ||
| "basename": ".babelrc", | ||
| "children": [], | ||
| "fullpath": ".babelrc", | ||
| "metadata": { | ||
| "path": ".babelrc", | ||
| "someMeta": 8, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": ".editorconfig", | ||
| "children": [], | ||
| "fullpath": ".editorconfig", | ||
| "metadata": { | ||
| "path": ".editorconfig", | ||
| "someMeta": 13, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": ".flowconfig", | ||
| "children": [], | ||
| "fullpath": ".flowconfig", | ||
| "metadata": { | ||
| "path": ".flowconfig", | ||
| "someMeta": 11, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": ".gitignore", | ||
| "children": [], | ||
| "fullpath": ".gitignore", | ||
| "metadata": { | ||
| "path": ".gitignore", | ||
| "someMeta": 10, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": ".travis.yml", | ||
| "children": [], | ||
| "fullpath": ".travis.yml", | ||
| "metadata": { | ||
| "path": ".travis.yml", | ||
| "someMeta": 11, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "LICENSE.md", | ||
| "children": [], | ||
| "fullpath": "LICENSE.md", | ||
| "metadata": { | ||
| "path": "LICENSE.md", | ||
| "someMeta": 10, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "README.md", | ||
| "children": [], | ||
| "fullpath": "README.md", | ||
| "metadata": { | ||
| "path": "README.md", | ||
| "someMeta": 9, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "package-lock.json", | ||
| "children": [], | ||
| "fullpath": "package-lock.json", | ||
| "metadata": { | ||
| "path": "package-lock.json", | ||
| "someMeta": 17, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "package.json", | ||
| "children": [], | ||
| "fullpath": "package.json", | ||
| "metadata": { | ||
| "path": "package.json", | ||
| "someMeta": 12, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "shrinkwrap.yaml", | ||
| "children": [], | ||
| "fullpath": "shrinkwrap.yaml", | ||
| "metadata": { | ||
| "path": "shrinkwrap.yaml", | ||
| "someMeta": 15, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "src", | ||
| "children": [ | ||
| { | ||
| "basename": "commands", | ||
| "children": [ | ||
| { | ||
| "basename": "checkout.js", | ||
| "children": [], | ||
| "fullpath": "src/commands/checkout.js", | ||
| "metadata": { | ||
| "path": "src/commands/checkout.js", | ||
| "someMeta": 24, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "config.js", | ||
| "children": [], | ||
| "fullpath": "src/commands/config.js", | ||
| "metadata": { | ||
| "path": "src/commands/config.js", | ||
| "someMeta": 22, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "fetch.js", | ||
| "children": [], | ||
| "fullpath": "src/commands/fetch.js", | ||
| "metadata": { | ||
| "path": "src/commands/fetch.js", | ||
| "someMeta": 21, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "init.js", | ||
| "children": [], | ||
| "fullpath": "src/commands/init.js", | ||
| "metadata": { | ||
| "path": "src/commands/init.js", | ||
| "someMeta": 20, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| ], | ||
| "fullpath": "src/commands", | ||
| "metadata": {}, | ||
| "parent": [Circular], | ||
| "type": "tree", | ||
| }, | ||
| { | ||
| "basename": "index.js", | ||
| "children": [], | ||
| "fullpath": "src/index.js", | ||
| "metadata": { | ||
| "path": "src/index.js", | ||
| "someMeta": 12, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "models", | ||
| "children": [ | ||
| { | ||
| "basename": "GitBlob.js", | ||
| "children": [], | ||
| "fullpath": "src/models/GitBlob.js", | ||
| "metadata": { | ||
| "path": "src/models/GitBlob.js", | ||
| "someMeta": 21, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "GitCommit.js", | ||
| "children": [], | ||
| "fullpath": "src/models/GitCommit.js", | ||
| "metadata": { | ||
| "path": "src/models/GitCommit.js", | ||
| "someMeta": 23, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "GitConfig.js", | ||
| "children": [], | ||
| "fullpath": "src/models/GitConfig.js", | ||
| "metadata": { | ||
| "path": "src/models/GitConfig.js", | ||
| "someMeta": 23, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "GitObject.js", | ||
| "children": [], | ||
| "fullpath": "src/models/GitObject.js", | ||
| "metadata": { | ||
| "path": "src/models/GitObject.js", | ||
| "someMeta": 23, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "GitTree.js", | ||
| "children": [], | ||
| "fullpath": "src/models/GitTree.js", | ||
| "metadata": { | ||
| "path": "src/models/GitTree.js", | ||
| "someMeta": 21, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| ], | ||
| "fullpath": "src/models", | ||
| "metadata": {}, | ||
| "parent": [Circular], | ||
| "type": "tree", | ||
| }, | ||
| { | ||
| "basename": "utils", | ||
| "children": [ | ||
| { | ||
| "basename": "exists.js", | ||
| "children": [], | ||
| "fullpath": "src/utils/exists.js", | ||
| "metadata": { | ||
| "path": "src/utils/exists.js", | ||
| "someMeta": 19, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "mkdirs.js", | ||
| "children": [], | ||
| "fullpath": "src/utils/mkdirs.js", | ||
| "metadata": { | ||
| "path": "src/utils/mkdirs.js", | ||
| "someMeta": 19, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "read.js", | ||
| "children": [], | ||
| "fullpath": "src/utils/read.js", | ||
| "metadata": { | ||
| "path": "src/utils/read.js", | ||
| "someMeta": 17, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "resolveRef.js", | ||
| "children": [], | ||
| "fullpath": "src/utils/resolveRef.js", | ||
| "metadata": { | ||
| "path": "src/utils/resolveRef.js", | ||
| "someMeta": 23, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "write.js", | ||
| "children": [], | ||
| "fullpath": "src/utils/write.js", | ||
| "metadata": { | ||
| "path": "src/utils/write.js", | ||
| "someMeta": 18, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| ], | ||
| "fullpath": "src/utils", | ||
| "metadata": {}, | ||
| "parent": [Circular], | ||
| "type": "tree", | ||
| }, | ||
| ], | ||
| "fullpath": "src", | ||
| "metadata": {}, | ||
| "parent": [Circular], | ||
| "type": "tree", | ||
| }, | ||
| { | ||
| "basename": "test", | ||
| "children": [ | ||
| { | ||
| "basename": "_helpers.js", | ||
| "children": [], | ||
| "fullpath": "test/_helpers.js", | ||
| "metadata": { | ||
| "path": "test/_helpers.js", | ||
| "someMeta": 16, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "snapshots", | ||
| "children": [ | ||
| { | ||
| "basename": "test-resolveRef.js.md", | ||
| "children": [], | ||
| "fullpath": "test/snapshots/test-resolveRef.js.md", | ||
| "metadata": { | ||
| "path": "test/snapshots/test-resolveRef.js.md", | ||
| "someMeta": 36, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "test-resolveRef.js.snap", | ||
| "children": [], | ||
| "fullpath": "test/snapshots/test-resolveRef.js.snap", | ||
| "metadata": { | ||
| "path": "test/snapshots/test-resolveRef.js.snap", | ||
| "someMeta": 38, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| ], | ||
| "fullpath": "test/snapshots", | ||
| "metadata": {}, | ||
| "parent": [Circular], | ||
| "type": "tree", | ||
| }, | ||
| { | ||
| "basename": "test-clone.js", | ||
| "children": [], | ||
| "fullpath": "test/test-clone.js", | ||
| "metadata": { | ||
| "path": "test/test-clone.js", | ||
| "someMeta": 18, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "test-config.js", | ||
| "children": [], | ||
| "fullpath": "test/test-config.js", | ||
| "metadata": { | ||
| "path": "test/test-config.js", | ||
| "someMeta": 19, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "test-init.js", | ||
| "children": [], | ||
| "fullpath": "test/test-init.js", | ||
| "metadata": { | ||
| "path": "test/test-init.js", | ||
| "someMeta": 17, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| { | ||
| "basename": "test-resolveRef.js", | ||
| "children": [], | ||
| "fullpath": "test/test-resolveRef.js", | ||
| "metadata": { | ||
| "path": "test/test-resolveRef.js", | ||
| "someMeta": 23, | ||
| }, | ||
| "parent": [Circular], | ||
| "type": "blob", | ||
| }, | ||
| ], | ||
| "fullpath": "test", | ||
| "metadata": {}, | ||
| "parent": [Circular], | ||
| "type": "tree", | ||
| }, | ||
| ], | ||
| "fullpath": ".", | ||
| "metadata": {}, | ||
| "parent": [Circular], | ||
| "type": "tree", | ||
| } | ||
| `) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Filename is inconsistent with test content—no submodule-specific behavior tested.
The filename suffix "-in-submodule" suggests this test validates submodule-specific behavior, but the code contains no submodule setup or context. The test directly invokes flatFileListToDirectoryStructure with hardcoded data arrays, identical to how a non-submodule test would operate. Since flatFileListToDirectoryStructure is a pure utility function that builds an in-memory tree from a flat list (confirmed by the relevant code snippet), it has no dependency on filesystem or git directory context.
This appears to be either a duplicate test file or an incomplete implementation. If there is an existing non-submodule version of this test, consider removing this file. Otherwise, rename it to remove the "-in-submodule" suffix.
</review_comment_end>
🤖 Prompt for AI Agents
In __tests__/test-flatFileListToDirectoryStructure-in-submodule.js lines 1-479
the filename implies submodule-specific behavior but the test contains only
plain flatFileListToDirectoryStructure unit tests with no submodule setup; fix
by either deleting this file if it duplicates an existing non-submodule test, or
rename the file (and update the describe/it titles if needed) to remove the
"-in-submodule" suffix so filename matches the test content; ensure there are no
other tests with the same assertions to avoid duplication.
| /* eslint-env node, browser, jasmine */ | ||
| import { GitAnnotatedTag } from 'isomorphic-git/internal-apis' | ||
|
|
||
| const tagString = `object af4d84a6a9fa7a74acdad07fddf9f17ff3a974ae | ||
| type commit | ||
| tag v0.0.9 | ||
| tagger Will Hilton <[email protected]> 1507071414 -0400 | ||
|
|
||
| 0.0.9 | ||
| -----BEGIN PGP SIGNATURE----- | ||
| Version: GnuPG v1 | ||
|
|
||
| iQIcBAABAgAGBQJZ1BW2AAoJEJYJuKWSi6a5S6EQAJQkK+wIXijDf4ZfVeP1E7Be | ||
| aDDdOLga0/gj5p2p081TLLlaKKLcYj2pub8BfFVpEmvT0QRaKaMb+wAtO5PBHTbn | ||
| y2s3dCmqqAPQa0AXrChverKomK/gUYZfFzckS8GaJTiw2RyvheXOLOEGSLTHOwy2 | ||
| wjP8KxGOWfHlXZEhn/Z406OlcYMzMSL70H26pgyggSTe5RNfpXEBAgWmIAA51eEM | ||
| 9tF9xuijc0mlr6vzxYVmfwat4u38nrwX7JvWp2CvD/qwILMAYGIcZqRXK5jWHemD | ||
| /x5RtUGU4cr47++FD3N3zBWx0dBiCMNUwT/v68kmhrBVX20DhcC6UX38yf1sdDfZ | ||
| yapht2+TakKQuw/T/K/6bFjoa8MIHdAx7WCnMV84M0qfMr+e9ImeH5Hj592qw4Gh | ||
| vSY80gKslkXjRnVes7VHXoL/lVDvCM2VNskWTTLGHqt+rIvSXNFGP05OGtdFYu4d | ||
| K9oFVEoRPFTRSeF/9EztyeLb/gtSdBmWP2AhZn9ip0a7rjbyv5yeayZTsedoUfe5 | ||
| o8cB++UXreD+h3c/F6mTRs8aVELhQTZNZ677PY71HJKsCLbQJAd4n+gS1n8Y/7wv | ||
| Zp4YxnShDkMTV3rxZc27vehq2g9gKJzQsueLyZPJTzCHqujumiLbdYV4i4X4CZjy | ||
| dBWrLc3kdnemrlhSRzR2 | ||
| =PrR1 | ||
| -----END PGP SIGNATURE----- | ||
| ` | ||
|
|
||
| const tagObject = { | ||
| object: 'af4d84a6a9fa7a74acdad07fddf9f17ff3a974ae', | ||
| type: 'commit', | ||
| tag: 'v0.0.9', | ||
| tagger: { | ||
| name: 'Will Hilton', | ||
| email: '[email protected]', | ||
| timestamp: 1507071414, | ||
| timezoneOffset: 240, | ||
| }, | ||
| message: '0.0.9', | ||
| gpgsig: `-----BEGIN PGP SIGNATURE----- | ||
| Version: GnuPG v1 | ||
|
|
||
| iQIcBAABAgAGBQJZ1BW2AAoJEJYJuKWSi6a5S6EQAJQkK+wIXijDf4ZfVeP1E7Be | ||
| aDDdOLga0/gj5p2p081TLLlaKKLcYj2pub8BfFVpEmvT0QRaKaMb+wAtO5PBHTbn | ||
| y2s3dCmqqAPQa0AXrChverKomK/gUYZfFzckS8GaJTiw2RyvheXOLOEGSLTHOwy2 | ||
| wjP8KxGOWfHlXZEhn/Z406OlcYMzMSL70H26pgyggSTe5RNfpXEBAgWmIAA51eEM | ||
| 9tF9xuijc0mlr6vzxYVmfwat4u38nrwX7JvWp2CvD/qwILMAYGIcZqRXK5jWHemD | ||
| /x5RtUGU4cr47++FD3N3zBWx0dBiCMNUwT/v68kmhrBVX20DhcC6UX38yf1sdDfZ | ||
| yapht2+TakKQuw/T/K/6bFjoa8MIHdAx7WCnMV84M0qfMr+e9ImeH5Hj592qw4Gh | ||
| vSY80gKslkXjRnVes7VHXoL/lVDvCM2VNskWTTLGHqt+rIvSXNFGP05OGtdFYu4d | ||
| K9oFVEoRPFTRSeF/9EztyeLb/gtSdBmWP2AhZn9ip0a7rjbyv5yeayZTsedoUfe5 | ||
| o8cB++UXreD+h3c/F6mTRs8aVELhQTZNZ677PY71HJKsCLbQJAd4n+gS1n8Y/7wv | ||
| Zp4YxnShDkMTV3rxZc27vehq2g9gKJzQsueLyZPJTzCHqujumiLbdYV4i4X4CZjy | ||
| dBWrLc3kdnemrlhSRzR2 | ||
| =PrR1 | ||
| -----END PGP SIGNATURE----- | ||
| `, | ||
| } | ||
|
|
||
| describe('GitAnnotatedTag', () => { | ||
| it('parse', async () => { | ||
| const tag = GitAnnotatedTag.from(tagString) | ||
| expect(tag.parse()).toEqual(tagObject) | ||
| }) | ||
|
|
||
| it('render', async () => { | ||
| const tag = GitAnnotatedTag.from(tagObject) | ||
| expect(tag.render()).toEqual(tagString) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Critical: Test file doesn't match its name or description.
The filename test-GitAnnotatedTag-in-submodule.js and the AI summary indicate this should test GitAnnotatedTag functionality within a submodule context, but the actual implementation only contains basic unit tests that parse and render static tag data with no submodule operations.
Missing elements described in the AI summary:
- No
makeFixtureAsSubmodulefixture setup - No git operations (
resolveRef,readTag, etc.) - No integration with
dir/gitdirparameters for submodule context - No tests creating annotated tags on HEAD or blob objects
- No signed tag creation or verification
The current tests are just unit tests of the GitAnnotatedTag class that would work identically regardless of submodule context. Either:
- This file needs to be populated with actual submodule integration tests, or
- The filename should be changed to remove the "-in-submodule" suffix
🤖 Prompt for AI Agents
In __tests__/test-GitAnnotatedTag-in-submodule.js lines 1-70: the test file name
implies submodule integration but the file only contains basic unit parse/render
tests; fix by either (A) converting it to a proper submodule integration test:
use the existing makeFixtureAsSubmodule helper to create a submodule fixture,
operate with dir/gitdir params, create annotated tags on HEAD and a blob
(including a signed tag), exercise git operations like
resolveRef/readTag/GitAnnotatedTag.from/render and verify behavior in the
submodule context; or (B) if integration is not intended, rename the file to
remove the "-in-submodule" suffix to reflect it is a plain unit test. Ensure
imports and fixtures are added for option A and that tests assert the same
serialized/deserialized results while using dir/gitdir.
| it('odd number of quotes', async () => { | ||
| const config = GitConfig.from(`[foo] | ||
| keyaaa = "val" a "aa`) | ||
| const a = await config.get('foo.keybbb') | ||
| expect(a).toBeUndefined() | ||
| }) |
There was a problem hiding this comment.
"odd number of quotes" test is not exercising the intended scenario
This test configures only keyaaa but calls get('foo.keybbb'), so it just reasserts that an unknown key returns undefined (already covered in handle errors / get unknown key) and never checks behavior for the odd-quoted keyaaa line.
If the intent is to ensure the malformed line is ignored, this should likely target keyaaa:
- const a = await config.get('foo.keybbb')
+ const a = await config.get('foo.keyaaa')The expectation of undefined can stay as-is.
🤖 Prompt for AI Agents
In __tests__/test-GitConfig-in-submodule.js around lines 168-173, the "odd
number of quotes" test sets keyaaa but calls get('foo.keybbb'), so it never
validates handling of the malformed keyaaa line; change the test to call
get('foo.keyaaa') (keeping the expect toBeUndefined()) so the test actually
asserts that the malformed line is ignored.
| it('GitIndex round trip', async () => { | ||
| const { fs, dir } = await makeFixtureAsSubmodule('test-GitIndex') | ||
| const buffer = await fs.read(path.join(dir, 'index')) | ||
| const index = await GitIndex.from(buffer) | ||
| const buffer2 = await index.toObject() | ||
| const index2 = await GitIndex.from(buffer2) | ||
| const buffer3 = await index2.toObject() | ||
| expect(buffer2.buffer).toEqual(buffer3.buffer) | ||
| }) |
There was a problem hiding this comment.
Fix buffer comparison to compare content, not references.
Line 72 compares buffer2.buffer with buffer3.buffer, which compares the underlying ArrayBuffer references rather than their contents. This comparison will fail even when the buffers contain identical data because each Buffer has its own ArrayBuffer instance.
Apply this diff to fix the comparison:
- expect(buffer2.buffer).toEqual(buffer3.buffer)
+ expect(buffer2).toEqual(buffer3)Alternatively, if you need to compare the underlying data explicitly:
- expect(buffer2.buffer).toEqual(buffer3.buffer)
+ expect(buffer2.equals(buffer3)).toBe(true)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('GitIndex round trip', async () => { | |
| const { fs, dir } = await makeFixtureAsSubmodule('test-GitIndex') | |
| const buffer = await fs.read(path.join(dir, 'index')) | |
| const index = await GitIndex.from(buffer) | |
| const buffer2 = await index.toObject() | |
| const index2 = await GitIndex.from(buffer2) | |
| const buffer3 = await index2.toObject() | |
| expect(buffer2.buffer).toEqual(buffer3.buffer) | |
| }) | |
| it('GitIndex round trip', async () => { | |
| const { fs, dir } = await makeFixtureAsSubmodule('test-GitIndex') | |
| const buffer = await fs.read(path.join(dir, 'index')) | |
| const index = await GitIndex.from(buffer) | |
| const buffer2 = await index.toObject() | |
| const index2 = await GitIndex.from(buffer2) | |
| const buffer3 = await index2.toObject() | |
| expect(buffer2).toEqual(buffer3) | |
| }) |
🤖 Prompt for AI Agents
__tests__/test-GitIndex-in-submodule.js around lines 65 to 73: the test
currently compares underlying ArrayBuffer references (buffer2.buffer ===
buffer3.buffer) which can fail even when contents are identical; update the
assertion to compare buffer contents instead (for example use Buffer.equals or
Buffer.compare) so the test asserts that the two buffers contain the same bytes
(e.g., expect(buffer2.buffer.equals(buffer3.buffer)).toBe(true) or
expect(Buffer.compare(buffer2.buffer, buffer3.buffer)).toBe(0)).
| it('status in an fresh git repo with no commits', async () => { | ||
| // Setup | ||
| const { fs, dir, gitdir } = await makeFixtureAsSubmodule('test-empty') | ||
| await fs.write(path.join(dir, 'a.txt'), 'Hi') | ||
| await fs.write(path.join(dir, 'b.txt'), 'Hi') | ||
| await add({ fs, dir, gitdir, filepath: 'b.txt' }) | ||
| // Test | ||
| const a = await status({ fs, dir, gitdir, filepath: 'a.txt' }) | ||
| expect(a).toEqual('*added') | ||
| const b = await status({ fs, dir, gitdir, filepath: 'b.txt' }) | ||
| expect(b).toEqual('added') | ||
| }) |
There was a problem hiding this comment.
Minor test name wording and intent clarity
The scenario is useful (fresh submodule repo with no commits, with one staged and one unstaged file). Two minor polish points:
- The description string reads
"status in an fresh git repo with no commits"; consider"status in a fresh git repo with no commits"for grammar. - If this is specifically a submodule fixture (via
makeFixtureAsSubmodule('test-empty')), you might rename it to"status in a fresh submodule repo with no commits"to make its purpose obvious when scanning test output.
Both are purely cosmetic; behavior looks correct.
🤖 Prompt for AI Agents
In __tests__/test-status-in-submodule.js around lines 69 to 80, update the test
description string to correct grammar and clarify intent: change "status in an
fresh git repo with no commits" to "status in a fresh submodule repo with no
commits" so it reads properly and indicates this uses the submodule fixture.
| it('should not throw if we force remove a directory', async () => { | ||
| // Setup | ||
| const { fs, dir } = await makeFixtureAsSubmodule('test-empty') | ||
| await fs.mkdir(path.join(dir, 'hello-world')) | ||
| await fs.write(path.join(dir, 'hello-world/a'), 'a') | ||
| await add({ | ||
| fs, | ||
| dir, | ||
| filepath: 'hello-world/a', | ||
| }) | ||
| // Test | ||
| let fileStatus = await status({ | ||
| fs, | ||
| dir, | ||
| filepath: 'hello-world/a', | ||
| }) | ||
| expect(fileStatus).toBe('added') | ||
| await updateIndex({ | ||
| fs, | ||
| dir, | ||
| remove: true, | ||
| force: true, | ||
| filepath: 'hello-world', | ||
| }) | ||
| fileStatus = await status({ | ||
| fs, | ||
| dir, | ||
| filepath: 'hello-world/a', | ||
| }) | ||
| expect(fileStatus).toBe('added') | ||
| }) |
There was a problem hiding this comment.
Inconsistent force-remove directory behavior should be clarified or fixed.
The test documents that force-removing a directory silently does nothing (line 336 expects status still 'added'), while the previous test (lines 269-305) shows that removing a directory without force throws InvalidFilepathError. This creates confusing behavior:
remove: trueon directory → throws errorremove: true, force: trueon directory → silent no-op
Expected behavior should be one of:
- Consistent rejection: Force-remove should still throw since directories aren't supported
- Recursive removal: Force-remove should remove all files under the directory (matching git's behavior)
The current silent no-op behavior is likely to surprise users who expect force to either enable the operation or provide clearer feedback.
Consider either:
- Making force-remove of directories throw the same error for consistency
- Implementing recursive removal when force is used
- At minimum, documenting this edge case behavior clearly in the API docs
Based on the relevant code snippet from src/api/updateIndex.js (lines 63-78), the implementation checks if the file is a directory and throws when !force, but when force is true it only deletes if index.has({ filepath }). Since the directory path itself isn't in the index (only hello-world/a is), nothing happens. This seems unintentional.
🤖 Prompt for AI Agents
In __tests__/test-updateIndex-in-submodule.js around lines 307 to 337, the test
exposes that updateIndex currently treats remove:true on directories as an error
unless force:true, and when force:true it silently no-ops because directory
entries aren't in the index; fix updateIndex.js so force-remove on a directory
recursively removes all indexed entries under that directory path: detect when
filepath is a directory and force===true, query the index for all entries whose
filepath starts with filepath + '/' and remove each of those entries from the
index (and update any on-disk refs/state accordingly); keep the existing
behavior of throwing InvalidFilepathError when remove===true and force!==true;
update or add tests to assert that force removal deletes nested files from the
index (or alternatively change tests/docs if you prefer to preserve the throw
behavior for force, but pick one consistent behavior and make code/tests/docs
match).
src/api/addNote.js
Outdated
| const fsp = new FileSystem(fs) | ||
| const updatedGitdir = await discoverGitdir({ fsp, dotgit: gitdir }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Eliminate redundant FileSystem wrapper creation.
Line 76 creates a new FileSystem wrapper from fs, which is already a FileSystem instance (created on line 63). While the FileSystem constructor has a guard that returns the same object if already wrapped, this pattern is confusing and suggests fsp is a distinct instance when it's actually the same reference as fs.
Apply this diff to use the existing wrapper directly:
- const fsp = new FileSystem(fs)
- const updatedGitdir = await discoverGitdir({ fsp, dotgit: gitdir })
+ const updatedGitdir = await discoverGitdir({ fsp: fs, dotgit: gitdir })
return await _addNote({
- fs: fsp,
+ fs,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/api/addNote.js around lines 76-77, a new FileSystem wrapper is
redundantly created via `new FileSystem(fs)` even though `fs` on line 63 is
already a FileSystem instance; remove the extra wrapper and pass the existing
`fs` directly to `discoverGitdir` (or rename the existing variable for clarity)
so there is no unnecessary reassignment or confusing duplicate reference.
|
Notes about each file:
coderabbit agrees that
coderabbit is requesting an additional
Made the suggested changes. Added gitdirsmfullpath.
Made the suggested changes. Added gitdirsmfullpath.
coderabbit complains about "skipped tests using xit." This is not new, and applies equally to normal tests and submodule tests. Nothing was modified. It's not specific to submodules. Please advise. Let's refer to the above message as "pre-existing condition" for the next few items. If you'd like me to modify standard tests in addition to the new ones, let me know, and what changes should be made.
coderabbit complains about "The entire GitLab test suite is disabled." Pre-existing condition.
coderabbit suggests removing
Made the suggested changes. Added gitdirsmfullpath.
coderabbit has been brilliant up until now, however the code it mentions in lines 159 and 165 matches other tests and doesn't appear to be erroneous or need to be changed.
coderabbit suggests modifying
Made the suggested changes. |
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
|
From the second round of reviews, I also updated The coderabbit feedback is getting into more details about pre-existing conditions. That would be beneficial to improve tests. But could it be done later? And in a separate pull request. |
There was a problem hiding this comment.
The code looks ok. I was expecting the AI to check the code, not tests.
One thing is to remove the shell exec, the browser test doesn't run at all (someone probably disable them.), but the fs module in the library is for a reason.
I don't recall when Browser/Device test were disabled, but I will check if I can turn them on again. There used to be Matrix run on all devices, now only one test runs.
OK. But it would preferable when modifying core features of FileSystem.js, that someone who considers themselves quite knowledgeable in nodejs do this properly and correctly. It seems like "Enhance FileSystem.js to support copy-related commands" would be a separate pull request. And within it, tests to ensure various permutations of copy (browser, non-browser, recursive, non-recursive) are working. |
|
If you don't want to add copy function using fs then I can't merge your PR. You can create an issue related to this, and hope that someone will write that code. I make take quite a while, though. |
|
@jcubic the shell command has been removed and tests are using recursive copy from the file system module. |
|
You force pushed and rewrite everything. It will require some time to review everything again. I can't just check one commit. Every time you amend the same commit, it requires manual review of 19k lines of code you've added. |
|
@jcubic OK next time I will avoid force push and use regular push. With this PR it would be dozens of commits. But that can be done. For now though there is a quick solution. Notice this text above.
Click the "compare" link to see the most recent changes, which are not extensive. Use "compare". |
|
Thanks for the tip. And it doesn't matter how many commits you made. This project uses a squash and merge strategy when merging PR. So there is only one commit that lands in the main branch. |
|
🎉 This PR is included in version 1.36.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
Hi,
So far, this pull request adds submodule support to
currentBranchandresolveRefas a first step towards including the same function in all the other command as well (https://isomorphic-git.org/docs/en/alphabetic)Enable the ability to run git commands within submodule directories: fixes #1647
About worktrees:
It would definitely be nice to parse both submodules and worktrees. The complexity with worktrees is that sometimes git needs to know the worktree's own git directory (.git/worktrees/my_worktree) and other times the commondir ( ../.. ) . It varies. If each git function used one or other of those choices, it would be possible to go through all supported commands and set the selection to either 1 or 2 (worktree dir or commondir). However there is a third category - when information is needed from both. In those cases, it would require fiddling with the internals of each command or adding more variables inside the functions. Not impossible. However with limited availability of developer time there is value in just doing an easier step - supporting submodules - which would be immediately useful. Nothing is blocking worktrees later, in the future.
I have tested this and it works, but please review, let me know. Then I could apply the code to other files.
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.