diff --git a/src/plugin.test.ts b/src/plugin.test.ts index 46eae40a..aaa8ddce 100644 --- a/src/plugin.test.ts +++ b/src/plugin.test.ts @@ -1914,6 +1914,42 @@ describe('getDirtyFiles', () => { expect(pluginModule.getDirtyFiles('/some/dir')).toEqual(['M foo.js', '?? untracked.js']); }); + it('ignores the node_modules/package-lock.json that install itself created', () => { + mockExecFileSync.mockImplementation((cmd, args) => { + if (Array.isArray(args) && args[0] === 'rev-parse') return '.git\n'; + return '?? node_modules/\n?? package-lock.json\n?? packages/alpha/node_modules/\n?? packages/alpha/package-lock.json\n'; + }); + expect(pluginModule.getDirtyFiles('/some/dir')).toEqual([]); + }); + + it('still reports user work that merely looks like an install artifact', () => { + mockExecFileSync.mockImplementation((cmd, args) => { + if (Array.isArray(args) && args[0] === 'rev-parse') return '.git\n'; + return '?? node_modules_notes.md\n M src/package-lock.json.bak\n'; + }); + expect(pluginModule.getDirtyFiles('/some/dir')).toEqual(['?? node_modules_notes.md', 'M src/package-lock.json.bak']); + }); + + // Only `??` is npm's own output. Every tracked status at the same path is + // user work that updatePlugin would destroy, so it must keep blocking. + it.each([ + [' M package-lock.json', 'M package-lock.json'], + ['M package-lock.json', 'M package-lock.json'], + [' D package-lock.json', 'D package-lock.json'], + ['D package-lock.json', 'D package-lock.json'], + ['A package-lock.json', 'A package-lock.json'], + [' M packages/alpha/package-lock.json', 'M packages/alpha/package-lock.json'], + [' M node_modules/vendored/patch.js', 'M node_modules/vendored/patch.js'], + ['R old-lock.json -> package-lock.json', 'R old-lock.json -> package-lock.json'], + ['UU package-lock.json', 'UU package-lock.json'], + ])('keeps tracked entry %j dirty', (porcelain, expected) => { + mockExecFileSync.mockImplementation((cmd, args) => { + if (Array.isArray(args) && args[0] === 'rev-parse') return '.git\n'; + return `${porcelain}\n`; + }); + expect(pluginModule.getDirtyFiles('/some/dir')).toEqual([expected]); + }); + it('does not pass --untracked-files=no, so untracked files are reported (git already omits gitignored paths)', () => { mockExecFileSync.mockImplementation((cmd, args) => { expect(args).not.toContain('--untracked-files=no'); diff --git a/src/plugin.ts b/src/plugin.ts index 4208ca9c..746b2a26 100644 --- a/src/plugin.ts +++ b/src/plugin.ts @@ -533,11 +533,10 @@ function describeGitError(error: unknown): string { /** * Report tracked-file modifications and untracked files within `dir` in a git checkout. * - * Untracked files are included on purpose: `git status` already excludes - * gitignored paths (build output like node_modules/dist never shows up), so - * anything untracked that does show up is real, unsaved user work — e.g. a - * new command file that hasn't been `git add`ed yet — which updating would - * destroy just as surely as an uncommitted edit to a tracked file. + * Untracked files are included on purpose: anything untracked is real, unsaved + * user work — e.g. a new command file that hasn't been `git add`ed yet — which + * updating would destroy just as surely as an uncommitted edit to a tracked + * file. The exception is `installArtifacts`: those are ours, not the user's. * * The `-- .` pathspec on `git status` restricts the report to `dir` itself. * Without it, git reports the *entire enclosing repository* — e.g. a plugin @@ -572,7 +571,9 @@ export function getDirtyFiles(dir: string): string[] { encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'], }); - return out.split('\n').map((line) => line.trim()).filter(Boolean); + return out.split('\n').filter((line) => line.trim()) + .filter((line) => !isInstallArtifact(line)) + .map((line) => line.trim()); } catch (error) { throw new PluginError( `Could not determine whether "${dir}" has uncommitted changes: git failed with: ${describeGitError(error)}`, @@ -581,10 +582,35 @@ export function getDirtyFiles(dir: string): string[] { } } +/** + * Artifacts `installDependencies` creates by running `npm install` in the + * checkout, at the repo root and in every sub-plugin of a monorepo. A plugin + * repo without a .gitignore reports them as dirty, so without this the guard + * fires on webcmd's own output and every such plugin is permanently + * un-updatable — blaming the user for work they never did. + */ +const installArtifacts = /(?:^|\/)(?:node_modules(?:\/|$)|package-lock\.json$)/; + +/** + * True only for an artifact npm itself created: a `??` (untracked) porcelain + * entry at an artifact path. The status columns are read from the raw line + * before any trimming, because every other status — ` M`, `M `, ` D`, `A `, + * `R `, `UU` — is tracked work the user could lose when `updatePlugin` + * replaces the directory, no matter what the path looks like. + */ +function isInstallArtifact(line: string): boolean { + if (line.slice(0, 2) !== '??') return false; + return installArtifacts.test(line.slice(2).trim()); +} + +/** Path portion of a `git status --porcelain` entry (already trimmed of its leading space). */ +function dirtyEntryPath(entry: string): string { + return entry.startsWith('??') ? entry.slice(2).trim() : entry.replace(/^[MADRCU!]{1,2}\s+/, ''); +} + function describeDirtyEntry(entry: string): string { - const isUntracked = entry.startsWith('??'); - const file = entry.replace(/^\?\?\s*/, '').replace(/^[MADRCU! ]+\s*/, ''); - return isUntracked ? `${file} (new, unstaged)` : `${file} (modified)`; + const file = dirtyEntryPath(entry); + return entry.startsWith('??') ? `${file} (new, unstaged)` : `${file} (modified)`; } function assertPluginNotDirty(name: string, dir: string, force: boolean): void {