From 67e5819a79d53305c05f4715710a9b69b107b494 Mon Sep 17 00:00:00 2001 From: Ben Monro Date: Fri, 24 May 2024 22:44:40 -0700 Subject: [PATCH] use existing generated files logic to improve exlusion of gen'd files in sloc calculations Summary: continuing to iterate on the pro tip to split diffs. this diff is improving the exclusion of gen'd files in the SLOC calculations by using the preexisting gen'd file checks that exist. for now I'm just using the filesSample which is limited to 25 files. future iterations may paginate through the files until we can determine the diff has +100 lines changed. additionally, for now we are only doing this check on diffs with < 25 files (total) as there could be perf issues but also more than likely changes w/ huge # of files are likely to be codemods etc. Reviewed By: quark-zju Differential Revision: D57785433 fbshipit-source-id: 009bb973b32858384f33c97d1398291aaf6c8814 --- addons/isl-server/src/Repository.ts | 17 ++++++++-- addons/isl-server/src/ServerToClientAPI.ts | 2 +- addons/isl-server/src/commands.ts | 2 +- .../src/CommitInfoView/SplitSuggestion.tsx | 31 +++++++++++++++---- addons/isl/src/types.ts | 2 +- 5 files changed, 42 insertions(+), 12 deletions(-) diff --git a/addons/isl-server/src/Repository.ts b/addons/isl-server/src/Repository.ts index 1a495ed3e5d8d..48730057bfb11 100644 --- a/addons/isl-server/src/Repository.ts +++ b/addons/isl-server/src/Repository.ts @@ -1086,21 +1086,32 @@ export class Repository { return result; } - public async fetchSignificantLinesOfCode(ctx: RepositoryContext, hash: Hash): Promise { + public async fetchSignificantLinesOfCode( + ctx: RepositoryContext, + hash: Hash, + generatedFiles: string[], + ): Promise { + const exclusions = generatedFiles.flatMap(file => [ + '-X', + absolutePathForFileInRepo(file, this) ?? file, + ]); + const output = ( await this.runCommand( - ['diff', '--stat', '-B', '-X', '"**__generated__*"', '-c', hash], + ['diff', '--stat', '-B', '-X', '**__generated__**', ...exclusions, '-c', hash], 'SlocCommand', ctx, ) ).stdout; - const lines = output.split('\n'); + const lines = output.trim().split('\n'); const changes = lines[lines.length - 1]; const diffStatRe = /\d+ files changed, (\d+) insertions\(\+\), (\d+) deletions\(-\)/; const diffStatMatch = changes.match(diffStatRe); const insertions = parseInt(diffStatMatch?.[1] ?? '0', 10); const deletions = parseInt(diffStatMatch?.[2] ?? '0', 10); const sloc = insertions + deletions; + + ctx.logger.info('Fetched SLOC for commit:', hash, output, `SLOC: ${sloc}`); return sloc; } public async getAllChangedFiles(ctx: RepositoryContext, hash: Hash): Promise> { diff --git a/addons/isl-server/src/ServerToClientAPI.ts b/addons/isl-server/src/ServerToClientAPI.ts index 53da7092bb1eb..70bd758b7ba63 100644 --- a/addons/isl-server/src/ServerToClientAPI.ts +++ b/addons/isl-server/src/ServerToClientAPI.ts @@ -624,7 +624,7 @@ export default class ServerToClientAPI { case 'fetchSignificantLinesOfCode': { repo - .fetchSignificantLinesOfCode(ctx, data.hash) + .fetchSignificantLinesOfCode(ctx, data.hash, data.generatedFiles) .then(value => { this.postMessage({ type: 'fetchedSignificantLinesOfCode', diff --git a/addons/isl-server/src/commands.ts b/addons/isl-server/src/commands.ts index 58074e8d64697..5f53e401e8582 100644 --- a/addons/isl-server/src/commands.ts +++ b/addons/isl-server/src/commands.ts @@ -56,7 +56,7 @@ export async function runCommand( timeout: number = READ_COMMAND_TIMEOUT_MS, ): Promise> { const {command, args, options} = getExecParams(ctx.cmd, args_, ctx.cwd, options_); - ctx.logger.log('run command: ', command, args[0]); + ctx.logger.log('run command: ', ctx.cwd, command, args[0]); const result = execa(command, args, options); let timedOut = false; diff --git a/addons/isl/src/CommitInfoView/SplitSuggestion.tsx b/addons/isl/src/CommitInfoView/SplitSuggestion.tsx index 2a75840689299..2fc408bae33a9 100644 --- a/addons/isl/src/CommitInfoView/SplitSuggestion.tsx +++ b/addons/isl/src/CommitInfoView/SplitSuggestion.tsx @@ -5,10 +5,9 @@ * LICENSE file in the root directory of this source tree. */ -import type {CommitInfo, Hash, Result} from '../types'; - import {Banner, BannerKind} from '../Banner'; import serverAPI from '../ClientToServerAPI'; +import {useGeneratedFileStatuses} from '../GeneratedFile'; import {Internal} from '../Internal'; import {Tooltip} from '../Tooltip'; import {tracker} from '../analytics'; @@ -16,6 +15,7 @@ import {Divider} from '../components/Divider'; import {useFeatureFlagSync} from '../featureFlags'; import {T} from '../i18n'; import {SplitButton} from '../stackEdit/ui/SplitButton'; +import {GeneratedStatus, type CommitInfo, type Hash, type Result} from '../types'; import {useEffect, useState} from 'react'; import {Icon} from 'shared/Icon'; import {LRU} from 'shared/LRU'; @@ -23,7 +23,7 @@ import {LRU} from 'shared/LRU'; // Cache fetches in progress so we don't double fetch const commitFilesCache = new LRU>>(10); -function fetchSignificantLinesOfCode(hash: Hash) { +function fetchSignificantLinesOfCode(hash: Hash, generatedFiles: string[]) { const foundPromise = commitFilesCache.get(hash); if (foundPromise != null) { return foundPromise; @@ -31,6 +31,7 @@ function fetchSignificantLinesOfCode(hash: Hash) { serverAPI.postMessage({ type: 'fetchSignificantLinesOfCode', hash, + generatedFiles, }); const resultPromise = serverAPI @@ -43,9 +44,19 @@ function fetchSignificantLinesOfCode(hash: Hash) { } function SplitSuggestionImpl({commit}: {commit: CommitInfo}) { + const filesToQueryGeneratedStatus = commit.filesSample.map(f => f.path); + const generatedStatuses = useGeneratedFileStatuses(filesToQueryGeneratedStatus); + const [significantLinesOfCode, setSignificantLinesOfCode] = useState(0); useEffect(() => { - fetchSignificantLinesOfCode(commit.hash).then(result => { + const generatedFiles = commit.filesSample.reduce((filtered, f) => { + // the __generated__ pattern is included in the exclusions, so we don't need to include it here + if (!f.path.match(/__generated__/) && generatedStatuses[f.path] !== GeneratedStatus.Manual) { + filtered.push(f.path); + } + return filtered; + }, []); + fetchSignificantLinesOfCode(commit.hash, generatedFiles).then(result => { if (result.error != null) { tracker.error('SplitSuggestionError', 'SplitSuggestionError', result.error, { extras: { @@ -58,7 +69,7 @@ function SplitSuggestionImpl({commit}: {commit: CommitInfo}) { setSignificantLinesOfCode(result.value); } }); - }, [commit.hash]); + }, [commit.filesSample, commit.hash, generatedStatuses]); if (significantLinesOfCode <= 100) { return null; } @@ -94,7 +105,7 @@ function SplitSuggestionImpl({commit}: {commit: CommitInfo}) { ); } -export default function SplitSuggestion({commit}: {commit: CommitInfo}) { +function GatedSplitSuggestion({commit}: {commit: CommitInfo}) { const showSplitSuggestion = useFeatureFlagSync(Internal.featureFlags?.ShowSplitSuggestion); if (!showSplitSuggestion) { @@ -102,3 +113,11 @@ export default function SplitSuggestion({commit}: {commit: CommitInfo}) { } return ; } + +export default function SplitSuggestion({commit}: {commit: CommitInfo}) { + if (commit.totalFileCount > 25) { + return null; + } + // using a gated component to avoid exposing when diff size is too big to show the split suggestion + return ; +} diff --git a/addons/isl/src/types.ts b/addons/isl/src/types.ts index 07a62551f4227..bfdfa280adc5a 100644 --- a/addons/isl/src/types.ts +++ b/addons/isl/src/types.ts @@ -701,7 +701,7 @@ export type ClientToServerMessage = | {type: 'gotUiState'; state: string} | CodeReviewProviderSpecificClientToServerMessages | PlatformSpecificClientToServerMessages - | {type: 'fetchSignificantLinesOfCode'; hash: Hash}; + | {type: 'fetchSignificantLinesOfCode'; hash: Hash; generatedFiles: string[]}; export type SubscriptionResultsData = { uncommittedChanges: FetchedUncommittedChanges;