Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 48 additions & 2 deletions src/dataSource.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as path from "node:path";
import * as cp from "node:child_process";
Comment on lines +1 to 2

Copilot AI Mar 27, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Importing node:path as path introduces identifier shadowing with existing parameters named path (e.g., isGitRepository(path: string)). Consider renaming those parameters (e.g., repoPath/dir) to avoid confusion and make future use of the path module inside those methods possible.

Copilot uses AI. Check for mistakes.

import { escapeRefName, getPathFromStr } from "./backend/utils";
Expand Down Expand Up @@ -203,9 +204,9 @@ export class DataSource {
});
}

public isGitRepository(path: string) {
public isGitRepository(repoPath: string) {
return new Promise<boolean>((resolve) => {
this.execGit("rev-parse --git-dir", path, (err) => {
this.execGit("rev-parse --git-dir", repoPath, (err) => {
resolve(!err);
});
});
Expand Down Expand Up @@ -462,4 +463,49 @@ export class DataSource {
});
});
}

public getSubmodules(repo: string) {
return new Promise<string[]>((resolve) => {
cp.execFile(
this.gitPath,
["config", "-f", ".gitmodules", "--get-regexp", "^submodule\\..*\\.path$"],
{ cwd: repo },
async (err, stdout) => {
const submodules: string[] = [];
if (err) {
// Missing .gitmodules or no submodule.path entries.
resolve(submodules);
return;
}

const lines = stdout.split(eolRegex);

for (let i = 0; i < lines.length; i++) {
const line = lines[i];
if (line.trim() === "") continue;

const separatorPos = line.search(/\s/);
if (separatorPos === -1) continue;

const rawPath = line.slice(separatorPos + 1).trim();
if (rawPath === "") continue;

const resolvedPath = path.resolve(repo, rawPath);
const relativeToRepo = path.relative(repo, resolvedPath);
// Ignore entries that escape the repository root.
if (relativeToRepo.startsWith("..") || path.isAbsolute(relativeToRepo)) continue;

Copilot AI Mar 27, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The repo-escape guard is overly broad: relativeToRepo.startsWith("..") will also reject legitimate submodule paths whose first path segment merely starts with .. (e.g. ..my-submodule). Consider tightening the check to only reject .. as an actual path segment (e.g. relativeToRepo === ".." or relativeToRepo.startsWith(".." + path.sep)), while keeping the cross-drive absolute-path guard.

Suggested change
if (relativeToRepo.startsWith("..") || path.isAbsolute(relativeToRepo)) continue;
if (
relativeToRepo === ".." ||
relativeToRepo.startsWith(".." + path.sep) ||
path.isAbsolute(relativeToRepo)
) {
continue;
}

Copilot uses AI. Check for mistakes.

const submodulePath = getPathFromStr(resolvedPath);
if (submodules.includes(submodulePath)) continue;

if (await this.isGitRepository(submodulePath)) {
submodules.push(submodulePath);
}
}

resolve(submodules);
}
);
});
}
}
50 changes: 46 additions & 4 deletions src/repoManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ export class RepoManager {
if (!(await this.checkReposExist())) this.sendRepos();
await this.searchWorkspaceForRepos();
this.startWatchingFolders();
if (await this.checkReposForNewSubmodules()) this.sendRepos();
}

private removeReposNotInWorkspace() {
Expand Down Expand Up @@ -123,9 +124,12 @@ export class RepoManager {
public getRepos() {
return sortRepos(this.repos);
}
private addRepo(repo: string) {
private async addRepo(repo: string) {
if (typeof this.repos[repo] !== "undefined") return false;
this.repos[repo] = { columnWidths: null };
await this.searchRepoForSubmodules(repo);
this.extensionState.saveRepos(this.repos);
return true;
}
private removeRepo(repo: string) {
delete this.repos[repo];
Expand Down Expand Up @@ -174,6 +178,39 @@ export class RepoManager {
this.repos[repo] = state;
this.extensionState.saveRepos(this.repos);
}
private async searchRepoForSubmodules(repo: string) {
let changes = false;
const submodules = await this.dataSource.getSubmodules(repo);

for (let i = 0; i < submodules.length; i++) {
if (typeof this.repos[submodules[i]] !== "undefined") continue;

this.repos[submodules[i]] = { columnWidths: null };
changes = true;
if (await this.searchRepoForSubmodules(submodules[i])) changes = true;
}

return changes;
}
private async checkReposForNewSubmodules() {
const repoPaths = Object.keys(this.repos);
if (repoPaths.length === 0) return false;

const results = await evalPromises(repoPaths, 2, (repoPath) =>
this.searchRepoForSubmodules(repoPath)
);
const changes = results.indexOf(true) > -1;

if (changes) this.extensionState.saveRepos(this.repos);
return changes;
}
private async checkRepoForNewSubmodules(repo: string) {
if (typeof this.repos[repo] === "undefined") return false;

const changes = await this.searchRepoForSubmodules(repo);
if (changes) this.extensionState.saveRepos(this.repos);
return changes;
}

/* Repo Searching */
private async searchWorkspaceForRepos() {
Expand Down Expand Up @@ -204,8 +241,7 @@ export class RepoManager {
.isGitRepository(directory)
.then((isRepo) => {
if (isRepo) {
this.addRepo(directory);
resolve(true);
this.addRepo(directory).then(resolve);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to change this logic?

} else if (maxDepth > 0) {
fs.readdir(directory, async (err, dirContents) => {
if (err) {
Expand Down Expand Up @@ -289,6 +325,9 @@ export class RepoManager {
while ((path = this.createEventPaths.shift())) {
if (await isDirectory(path)) {
if (await this.searchDirectoryForRepos(path, this.maxDepthOfRepoSearch)) changes = true;
} else if (path.endsWith("/.gitmodules")) {
const repo = path.slice(0, -"/.gitmodules".length);
if (await this.checkRepoForNewSubmodules(repo)) changes = true;
}
}
this.processCreateEventsTimeout = null;
Expand All @@ -298,7 +337,10 @@ export class RepoManager {
let path,
changes = false;
while ((path = this.changeEventPaths.shift())) {
if (!(await doesPathExist(path))) {
if (path.endsWith("/.gitmodules")) {
const repo = path.slice(0, -"/.gitmodules".length);
if (await this.checkRepoForNewSubmodules(repo)) changes = true;
} else if (!(await doesPathExist(path))) {
Comment on lines 326 to +343

Copilot AI Mar 27, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On .gitmodules create/change events, processCreateEvents / processChangeEvents call checkReposForNewSubmodules(), which scans all known repos and runs submodule discovery for each. Since the watcher path is the specific .gitmodules file, this can be much more expensive than necessary in large workspaces. Prefer deriving the repo root from the event path (e.g., dirname(path)), and only scanning that repo (plus optionally its known descendants).

Copilot uses AI. Check for mistakes.
if (this.removeReposWithinFolder(path)) changes = true;
}
}
Expand Down
81 changes: 81 additions & 0 deletions tests/backend/dataSource.submodules.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import * as cp from "node:child_process";
import * as fs from "node:fs";
import * as os from "node:os";
import * as path from "node:path";

import { afterAll, beforeAll, describe, expect, it, vi } from "vitest";

vi.mock("vscode", () => ({
workspace: {
getConfiguration: () => ({
get: (_key: string, defaultValue: unknown) => defaultValue
})
}
}));

import { DataSource } from "../../src/dataSource";

function git(args: string[], cwd: string) {
cp.execFileSync("git", args, { cwd, stdio: "pipe" });
}

function makeRepo(prefix: string): string {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix));
try {
git(["init", "-b", "main"], dir);
} catch {
git(["init"], dir);
git(["checkout", "-b", "main"], dir);
}
git(["config", "user.email", "t@t.com"], dir);
git(["config", "user.name", "T"], dir);
fs.writeFileSync(path.join(dir, "f"), "x");
git(["add", "."], dir);
git(["-c", "commit.gpgsign=false", "commit", "-m", "init"], dir);
return dir;
}

let parentRepo = "";
let submoduleRepo = "";
let standaloneRepo = "";

beforeAll(() => {
submoduleRepo = makeRepo("ngg-submodule-");
parentRepo = makeRepo("ngg-parent-");
standaloneRepo = makeRepo("ngg-plain-");

git(
[
"-c",
"protocol.file.allow=always",
"submodule",
"add",
submoduleRepo,
"modules/child"
],
parentRepo
);
git(["-c", "commit.gpgsign=false", "commit", "-am", "add submodule"], parentRepo);
});

afterAll(() => {
if (parentRepo !== "") fs.rmSync(parentRepo, { recursive: true, force: true });
if (submoduleRepo !== "") fs.rmSync(submoduleRepo, { recursive: true, force: true });
if (standaloneRepo !== "") fs.rmSync(standaloneRepo, { recursive: true, force: true });
});
Comment on lines +61 to +65

Copilot AI Mar 27, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afterAll unconditionally calls fs.rmSync on parentRepo/submoduleRepo/standaloneRepo. If beforeAll throws before these variables are assigned, Vitest will still run afterAll and this teardown will throw (masking the real failure). Guard the cleanup (e.g., check the variable is a non-empty string / path exists) or initialize the vars to empty strings and conditionalize the rmSync calls.

Copilot uses AI. Check for mistakes.

describe("getSubmodules", () => {
it("returns initialized submodules declared in .gitmodules", async () => {
const dataSource = new DataSource();

const submodules = await dataSource.getSubmodules(parentRepo);

expect(submodules).toEqual([path.join(parentRepo, "modules/child").replace(/\\/g, "/")]);
});
Comment on lines +67 to +74

Copilot AI Mar 27, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description mentions nested submodules, but the test suite here only covers a single-level submodule and the no-.gitmodules case. Add coverage for nested submodules (e.g. parent -> child -> grandchild) and/or an uninitialized/missing submodule entry to ensure discovery behaves correctly across the scenarios this feature targets.

Copilot uses AI. Check for mistakes.

it("returns an empty array when a repository has no .gitmodules file", async () => {
const dataSource = new DataSource();

await expect(dataSource.getSubmodules(standaloneRepo)).resolves.toEqual([]);
});
});
Loading