-
Notifications
You must be signed in to change notification settings - Fork 86
fix(plugin-git): support submodule #621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
08ad89f
ee60c9e
79d56fb
eeafcb8
aaa7746
bc24cb1
e3f22c1
59f88e1
19afa26
366a4f5
0fa382d
b8fbd36
12d2d17
affa47c
52e6a36
80fc507
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,12 +2,16 @@ import { spawn } from 'node:child_process' | |||||
| import type { GitContributorInfo } from '../../shared/index.js' | ||||||
| import type { GitPluginOptions } from '../options.js' | ||||||
| import type { MergedRawCommit, RawCommit } from '../typings.js' | ||||||
| import { path } from 'vuepress/utils' | ||||||
| import { logger } from './logger.js' | ||||||
|
|
||||||
| const INFO_SPLITTER = '[|]' | ||||||
| const COMMIT_SPLITTER = String.raw`\|/` | ||||||
| const RE_CO_AUTHOR = /^ *Co-authored-by: ?([^<]*)<([^>]*)> */gim | ||||||
|
|
||||||
| const gitRepoRootResultCache = new Map<string, string | null>() | ||||||
| const gitRepoRootTaskCache = new Map<string, Promise<string | null>>() | ||||||
|
|
||||||
| const getCoAuthorsFromCommitBody = ( | ||||||
| body: string, | ||||||
| ): Pick<GitContributorInfo, 'email' | 'name'>[] => | ||||||
|
|
@@ -77,6 +81,77 @@ const runGitLog = (args: string[], cwd: string): Promise<string> => | |||||
| }) | ||||||
| }) | ||||||
|
|
||||||
| /** | ||||||
| * Get git repository root directory for a given file path. | ||||||
| * | ||||||
| * This function runs `git rev-parse --show-toplevel` in the directory of the | ||||||
| * target file to determine the top-level directory of the git repository. | ||||||
| * | ||||||
| * @param filePath - File path (relative or absolute) whose repository root is requested | ||||||
| * @param cwd - Current working directory | ||||||
| * @returns Promise that resolves to normalized git root path, or null if not in a git repository | ||||||
| */ | ||||||
| const getGitRepoRoot = ( | ||||||
| filePath: string, | ||||||
| cwd: string, | ||||||
| ): Promise<string | null> => { | ||||||
| const absFilePath = path.isAbsolute(filePath) | ||||||
| ? filePath | ||||||
| : path.resolve(cwd, filePath) | ||||||
|
|
||||||
| const dir = path.normalize(path.dirname(absFilePath)) | ||||||
| const cachedResult = gitRepoRootResultCache.get(dir) | ||||||
|
|
||||||
| if (cachedResult !== undefined || gitRepoRootResultCache.has(dir)) | ||||||
| return Promise.resolve(cachedResult ?? null) | ||||||
|
|
||||||
| const cachedTask = gitRepoRootTaskCache.get(dir) | ||||||
| if (cachedTask) return cachedTask | ||||||
|
|
||||||
| const task = new Promise<string | null>((resolve) => { | ||||||
| const gitProcess = spawn('git', ['rev-parse', '--show-toplevel'], { | ||||||
| cwd: dir, | ||||||
| stdio: ['ignore', 'pipe', 'pipe'], | ||||||
| }) | ||||||
|
|
||||||
| let stdoutData = '' | ||||||
| let stderrData = '' | ||||||
|
|
||||||
| gitProcess.stdout.on('data', (chunk: Buffer) => { | ||||||
| stdoutData += chunk.toString('utf-8') | ||||||
| }) | ||||||
|
|
||||||
| gitProcess.stderr.on('data', (chunk: Buffer) => { | ||||||
| stderrData += chunk.toString('utf-8') | ||||||
| }) | ||||||
|
|
||||||
| gitProcess.on('error', (error) => { | ||||||
| logger.error(`Failed to spawn git rev-parse: ${error.message}`) | ||||||
| resolve(null) | ||||||
| }) | ||||||
|
|
||||||
| gitProcess.on('close', (code) => { | ||||||
| if (code === 0) { | ||||||
| resolve(path.normalize(stdoutData.trim())) | ||||||
| } else { | ||||||
| logger.error( | ||||||
|
||||||
| logger.error( | |
| logger.debug( |
Copilot
AI
Mar 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When git rev-parse --show-toplevel fails, the error log currently doesn’t include which directory it was executed in. Including dir (or the original filePath) in the logged message would make it much easier to diagnose submodule / monorepo edge cases, especially since failures may be intermittent depending on which file is being processed.
Copilot
AI
Mar 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getGitRepoRoot() is executed for every file in getRawCommits(), which adds an extra git spawn per file and can noticeably slow down builds on large sites (especially since getCommits() runs for every page). Consider memoizing repo roots by directory (e.g. a Map keyed by dir/cwd) or computing the repo root once per getCommits() call and reusing it for all filepaths.
Copilot
AI
Mar 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getCommits() can now fetch commit histories from different git roots within a single call (because getGitRepoRoot() is computed per filepath). However, mergeRawCommits() still merges commits solely by hash, which assumes all commits come from the same repository. If filepaths ever span multiple repos (e.g. via gitInclude pointing outside a submodule), identical hashes across repos could be merged incorrectly. Consider either enforcing that all filepaths resolve to the same gitRoot (and warn/skip otherwise), or include the repo identity (e.g. gitRoot) in the merge key / raw commit data so merges remain correct across repos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cache hit check is overly complex:
gitRepoRootResultCachestores onlystring | null, soget(dir)returningundefinedalready means “not cached”. The extra|| gitRepoRootResultCache.has(dir)branch is redundant and makes it harder to reason about cachingnullvs “missing”. Consider rewriting this to a singlehas(dir)check (or a singlecachedResult !== undefinedcheck) for clarity.