From c6a79e4100206e1264b25fad5f48bc56a1818dfe Mon Sep 17 00:00:00 2001 From: Cheng Liu Date: Thu, 27 Jun 2024 16:15:28 -0700 Subject: [PATCH 1/6] chore: rename function --- .../src/services/GitRemoteFetchConfigService.ts | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/apps/sparo-lib/src/services/GitRemoteFetchConfigService.ts b/apps/sparo-lib/src/services/GitRemoteFetchConfigService.ts index 70125b0..5926550 100644 --- a/apps/sparo-lib/src/services/GitRemoteFetchConfigService.ts +++ b/apps/sparo-lib/src/services/GitRemoteFetchConfigService.ts @@ -18,7 +18,7 @@ export class GitRemoteFetchConfigService { @inject(GracefulShutdownService) private _gracefulShutdownService!: GracefulShutdownService; public addRemoteBranchIfNotExists(remote: string, branch: string): void { - const remoteFetchGitConfig: string[] | undefined = this._loadForRemote(remote); + const remoteFetchGitConfig: string[] | undefined = this._getRemoteFetchInGitConfig(remote); if (remoteFetchGitConfig) { const targetConfig: string = `+refs/heads/${branch}:refs/remotes/${remote}/${branch}`; @@ -38,7 +38,7 @@ export class GitRemoteFetchConfigService { } public pruneRemoteBranchesInGitConfigAsync = async (remote: string): Promise => { - const remoteFetchConfig: string[] | undefined = this._loadForRemote(remote); + const remoteFetchConfig: string[] | undefined = this._getRemoteFetchInGitConfig(remote); if (!remoteFetchConfig) { return; } @@ -103,12 +103,12 @@ export class GitRemoteFetchConfigService { * It's used in "sparo fetch --all" command */ public revertSingleBranchIfNecessary = (remote: string): (() => void) | undefined => { - let remoteFetchGitConfig: string[] | undefined = this._loadForRemote(remote); + let remoteFetchGitConfig: string[] | undefined = this._getRemoteFetchInGitConfig(remote); let callback: (() => void) | undefined; if (remoteFetchGitConfig && !remoteFetchGitConfig.includes(`+refs/heads/*:refs/remotes/${remote}/*`)) { this._setAllBranchFetch(remote); - callback = () => { + callback = (): void => { if (remoteFetchGitConfig) { this._setRemoteFetchInGitConfig(remote, remoteFetchGitConfig); @@ -147,7 +147,7 @@ export class GitRemoteFetchConfigService { return branchToValues; } - private _loadForRemote(remote: string): string[] | undefined { + private _getRemoteFetchInGitConfig(remote: string): string[] | undefined { const result: string | undefined = this._gitService.getGitConfig(`remote.${remote}.fetch`, { array: true }); @@ -160,9 +160,10 @@ export class GitRemoteFetchConfigService { * So, delete all remote.origin.fetch configuration and restores expected value */ private _setRemoteFetchInGitConfig(remote: string, remoteFetchGitConfig: string[]): void { - this._gitService.unsetGitConfig(`remote.${remote}.fetch`); + const key: string = `remote.${remote}.fetch`; + this._gitService.unsetGitConfig(key); for (const value of remoteFetchGitConfig) { - this._gitService.setGitConfig(`remote.${remote}.fetch`, value, { add: true }); + this._gitService.setGitConfig(key, value, { add: true }); } } From 56272952f2ea73891eacd2f912d93976b6eadf5d Mon Sep 17 00:00:00 2001 From: Cheng Liu Date: Thu, 27 Jun 2024 16:22:36 -0700 Subject: [PATCH 2/6] feat: improve check remote branch existence logic --- apps/sparo-lib/src/services/GitService.ts | 49 ++++++++++++++----- .../sparo-lib/src/services/TerminalService.ts | 4 ++ 2 files changed, 40 insertions(+), 13 deletions(-) diff --git a/apps/sparo-lib/src/services/GitService.ts b/apps/sparo-lib/src/services/GitService.ts index f7a52c0..204b336 100644 --- a/apps/sparo-lib/src/services/GitService.ts +++ b/apps/sparo-lib/src/services/GitService.ts @@ -487,31 +487,54 @@ Please specify a directory on the command line public checkRemoteBranchExistenceAsync = async (remote: string, branch: string): Promise => { const gitPath: string = this.getGitPathOrThrow(); const currentWorkingDirectory: string = this.getRepoInfo().root; - const childProcess: child_process.ChildProcess = Executable.spawn( - gitPath, - ['ls-remote', '--exit-code', remote, branch], - { + const isDebug: boolean = this._terminalService.isDebug; + const lsRemoteArgs: string[] = ['ls-remote', '--exit-code', '--heads', remote, branch]; + const { terminal } = this._terminalService; + terminal.writeDebugLine(`Running git ${lsRemoteArgs.join(' ')}...`); + const childProcess: child_process.ChildProcess = Executable.spawn(gitPath, lsRemoteArgs, { currentWorkingDirectory, stdio: ['ignore', 'pipe', 'pipe'] - } - ); + }); if (!childProcess.stdout || !childProcess.stderr) { - this._terminalService.terminal.writeDebugLine(`Failed to spawn git process, fallback to spawnSync`); + terminal.writeDebugLine(`Failed to spawn git process, fallback to spawnSync`); const result: string = this.executeGitCommandAndCaptureOutput({ args: ['ls-remote', remote, branch] }).trim(); return Promise.resolve(!!result); } + if (isDebug) { + childProcess.stdout.on('data', (data: Buffer) => { + const text: string = data.toString(); + terminal.writeDebugLine(text); + }); + childProcess.stderr.on('data', (data: Buffer) => { + const text: string = data.toString(); + terminal.writeDebugLine(text); + }); + } return await new Promise((resolve, reject) => { // Only care about exit code since specifying --exit-code childProcess.on('close', (exitCode: number | null) => { - if (exitCode) { - this._terminalService.terminal.writeDebugLine(`Branch "${branch}" doesn't exist remotely`); - resolve(false); - } else { - this._terminalService.terminal.writeDebugLine(`Branch "${branch}" exists remotely`); - resolve(true); + // Allow exitCode 128. It indicates permission issue + switch (exitCode) { + case 0: { + terminal.writeDebugLine(`Branch "${branch}" exists remotely`); + break; + } + case 2: { + terminal.writeDebugLine(`Branch "${branch}" doesn't exist remotely`); + return resolve(false); + } + case 128: { + terminal.writeDebugLine(`Check "${branch}" failed because of permission issue`); + break; + } + default: { + terminal.writeDebugLine(`Check "${branch}" returns unknown exit code ${exitCode}`); + break; + } } + resolve(true); }); }); }; diff --git a/apps/sparo-lib/src/services/TerminalService.ts b/apps/sparo-lib/src/services/TerminalService.ts index 3656820..a133386 100644 --- a/apps/sparo-lib/src/services/TerminalService.ts +++ b/apps/sparo-lib/src/services/TerminalService.ts @@ -65,6 +65,10 @@ export class TerminalService { this._terminal.writeLine(Colorize.gray('-'.repeat(this._asciiHeaderWidth))); this._terminal.writeLine(); } + + public get isDebug(): boolean { + return this._terminalProvider.debugEnabled; + } } export type { ITerminal }; From f0847976707b037b9a125d7f5e09df7a0a75e4c6 Mon Sep 17 00:00:00 2001 From: Cheng Liu Date: Thu, 27 Jun 2024 16:24:12 -0700 Subject: [PATCH 3/6] chore: rush build & rush change --- .../sparo/feat-sparo-metric_2024-06-27-23-23.json | 10 ++++++++++ common/reviews/api/sparo-lib.api.md | 2 ++ 2 files changed, 12 insertions(+) create mode 100644 common/changes/sparo/feat-sparo-metric_2024-06-27-23-23.json diff --git a/common/changes/sparo/feat-sparo-metric_2024-06-27-23-23.json b/common/changes/sparo/feat-sparo-metric_2024-06-27-23-23.json new file mode 100644 index 0000000..de3ad2f --- /dev/null +++ b/common/changes/sparo/feat-sparo-metric_2024-06-27-23-23.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "sparo", + "comment": "Improve logic to check existence of a branch", + "type": "none" + } + ], + "packageName": "sparo" +} \ No newline at end of file diff --git a/common/reviews/api/sparo-lib.api.md b/common/reviews/api/sparo-lib.api.md index 18ee7e6..2665465 100644 --- a/common/reviews/api/sparo-lib.api.md +++ b/common/reviews/api/sparo-lib.api.md @@ -122,6 +122,8 @@ export class Sparo { export class TerminalService { constructor(); // (undocumented) + get isDebug(): boolean; + // (undocumented) setIsDebug(value: boolean): void; // (undocumented) setIsVerbose(value: boolean): void; From 8a307923a71399e7b3ef3ae64378bcb26bc29000 Mon Sep 17 00:00:00 2001 From: Cheng Liu Date: Thu, 27 Jun 2024 16:46:28 -0700 Subject: [PATCH 4/6] feat: improve metric logic to accurately report success data --- apps/sparo-lib/src/services/CommandService.ts | 23 +++++++--- apps/sparo-lib/src/services/GitService.ts | 44 +++++++++++-------- 2 files changed, 42 insertions(+), 25 deletions(-) diff --git a/apps/sparo-lib/src/services/CommandService.ts b/apps/sparo-lib/src/services/CommandService.ts index 0dd2e3a..56fe551 100644 --- a/apps/sparo-lib/src/services/CommandService.ts +++ b/apps/sparo-lib/src/services/CommandService.ts @@ -21,6 +21,7 @@ export class CommandService { @inject(HelpTextService) private _helpTextService!: HelpTextService; @inject(TerminalService) private _terminalService!: TerminalService; @inject(TelemetryService) private _telemetryService!: TelemetryService; + private _hasInternalError: boolean = false; public register(command: ICommand): void { const { cmd, description, builder, handler, getHelp } = command; @@ -36,19 +37,23 @@ export class CommandService { }, async (args) => { process.exitCode = 1; + this._hasInternalError = false; try { terminal.writeVerboseLine(`Invoking command "${commandName}" with args ${JSON.stringify(args)}`); const stopwatch: Stopwatch = Stopwatch.start(); await handler(args, terminalService); terminal.writeVerboseLine(`Invoked command "${commandName}" done (${stopwatch.toString()})`); stopwatch.stop(); - this._telemetryService.collectTelemetry({ - commandName, - args: process.argv.slice(2), - durationInSeconds: stopwatch.duration, - startTimestampMs: stopwatch.startTime, - endTimestampMs: stopwatch.endTime - }); + if (!this._hasInternalError) { + // Only report success data + this._telemetryService.collectTelemetry({ + commandName, + args: process.argv.slice(2), + durationInSeconds: stopwatch.duration, + startTimestampMs: stopwatch.startTime, + endTimestampMs: stopwatch.endTime + }); + } // eslint-disable-next-line require-atomic-updates process.exitCode = 0; } catch (e) { @@ -60,4 +65,8 @@ export class CommandService { ); this._helpTextService.set(commandName, getHelp()); } + + public setHasInternalError(): void { + this._hasInternalError = true; + } } diff --git a/apps/sparo-lib/src/services/GitService.ts b/apps/sparo-lib/src/services/GitService.ts index 204b336..f9ce626 100644 --- a/apps/sparo-lib/src/services/GitService.ts +++ b/apps/sparo-lib/src/services/GitService.ts @@ -6,6 +6,7 @@ import { Service } from '../decorator'; import { TerminalService } from './TerminalService'; import { Stopwatch } from '../logic/Stopwatch'; import { TelemetryService } from './TelemetryService'; +import { CommandService } from './CommandService'; /** * @alpha @@ -34,6 +35,7 @@ export class GitService { private _isSparseCheckoutMode: boolean | undefined; @inject(TerminalService) private _terminalService!: TerminalService; @inject(TelemetryService) private _telemetryService!: TelemetryService; + @inject(CommandService) private _commandService!: CommandService; public setGitConfig( k: string, @@ -248,14 +250,18 @@ export class GitService { this._terminalService.terminal.writeDebugLine(`Invoked git command done (${stopwatch.toString()})`); this._terminalService.writeTaskFooter(); stopwatch.stop(); - this._telemetryService.collectTelemetry({ - commandName: args[0], - args: args.slice(1), - durationInSeconds: stopwatch.duration, - startTimestampMs: stopwatch.startTime, - endTimestampMs: stopwatch.endTime, - isRawGitCommand: true - }); + if (result.status === 0) { + this._telemetryService.collectTelemetry({ + commandName: args[0], + args: args.slice(1), + durationInSeconds: stopwatch.duration, + startTimestampMs: stopwatch.startTime, + endTimestampMs: stopwatch.endTime, + isRawGitCommand: true + }); + } else { + this._commandService.setHasInternalError(); + } return result; } @@ -274,14 +280,16 @@ export class GitService { }); this._terminalService.terminal.writeDebugLine(`Invoked git command done (${stopwatch.toString()})`); stopwatch.stop(); - this._telemetryService.collectTelemetry({ - commandName: args[0], - args: args.slice(1), - durationInSeconds: stopwatch.duration, - startTimestampMs: stopwatch.startTime, - endTimestampMs: stopwatch.endTime, - isRawGitCommand: true - }); + if (result.status === 0) { + this._telemetryService.collectTelemetry({ + commandName: args[0], + args: args.slice(1), + durationInSeconds: stopwatch.duration, + startTimestampMs: stopwatch.startTime, + endTimestampMs: stopwatch.endTime, + isRawGitCommand: true + }); + } this._processResult(result); return result.stdout.toString(); } @@ -492,8 +500,8 @@ Please specify a directory on the command line const { terminal } = this._terminalService; terminal.writeDebugLine(`Running git ${lsRemoteArgs.join(' ')}...`); const childProcess: child_process.ChildProcess = Executable.spawn(gitPath, lsRemoteArgs, { - currentWorkingDirectory, - stdio: ['ignore', 'pipe', 'pipe'] + currentWorkingDirectory, + stdio: ['ignore', 'pipe', 'pipe'] }); if (!childProcess.stdout || !childProcess.stderr) { terminal.writeDebugLine(`Failed to spawn git process, fallback to spawnSync`); From b645eeeb3dc076a277f8cfcb6efbe79f3953e86b Mon Sep 17 00:00:00 2001 From: Cheng Liu Date: Thu, 27 Jun 2024 16:46:53 -0700 Subject: [PATCH 5/6] chore: rush change --- .../sparo/feat-sparo-metric_2024-06-27-23-46.json | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 common/changes/sparo/feat-sparo-metric_2024-06-27-23-46.json diff --git a/common/changes/sparo/feat-sparo-metric_2024-06-27-23-46.json b/common/changes/sparo/feat-sparo-metric_2024-06-27-23-46.json new file mode 100644 index 0000000..e7ea2b2 --- /dev/null +++ b/common/changes/sparo/feat-sparo-metric_2024-06-27-23-46.json @@ -0,0 +1,10 @@ +{ + "changes": [ + { + "packageName": "sparo", + "comment": "Enhance metric logic to accurately report success data", + "type": "none" + } + ], + "packageName": "sparo" +} \ No newline at end of file From 15addee110f5295aba6497c9e8a1d9addf8a3c60 Mon Sep 17 00:00:00 2001 From: Cheng Liu Date: Thu, 27 Jun 2024 16:56:46 -0700 Subject: [PATCH 6/6] chore: print to terminal when checking existence --- apps/sparo-lib/src/services/GitRemoteFetchConfigService.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apps/sparo-lib/src/services/GitRemoteFetchConfigService.ts b/apps/sparo-lib/src/services/GitRemoteFetchConfigService.ts index 5926550..f01c8dd 100644 --- a/apps/sparo-lib/src/services/GitRemoteFetchConfigService.ts +++ b/apps/sparo-lib/src/services/GitRemoteFetchConfigService.ts @@ -50,6 +50,8 @@ export class GitRemoteFetchConfigService { ); const checkBranches: string[] = Array.from(branchToValues.keys()).filter((x) => x !== '*'); + this._terminalService.terminal.writeLine(`Checking tracking branches...`); + const remoteBranchExistenceInfo: Record = await this._gitService.checkRemoteBranchesExistenceAsync(remote, checkBranches);