Skip to content

Commit

Permalink
use existing generated files logic to improve exlusion of gen'd files…
Browse files Browse the repository at this point in the history
… 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
  • Loading branch information
Ben Monro authored and facebook-github-bot committed May 25, 2024
1 parent c65179b commit 67e5819
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 12 deletions.
17 changes: 14 additions & 3 deletions addons/isl-server/src/Repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1086,21 +1086,32 @@ export class Repository {
return result;
}

public async fetchSignificantLinesOfCode(ctx: RepositoryContext, hash: Hash): Promise<number> {
public async fetchSignificantLinesOfCode(
ctx: RepositoryContext,
hash: Hash,
generatedFiles: string[],
): Promise<number> {
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<Array<ChangedFile>> {
Expand Down
2 changes: 1 addition & 1 deletion addons/isl-server/src/ServerToClientAPI.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
2 changes: 1 addition & 1 deletion addons/isl-server/src/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export async function runCommand(
timeout: number = READ_COMMAND_TIMEOUT_MS,
): Promise<execa.ExecaReturnValue<string>> {
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;
Expand Down
31 changes: 25 additions & 6 deletions addons/isl/src/CommitInfoView/SplitSuggestion.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,32 +5,33 @@
* 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';
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';

// Cache fetches in progress so we don't double fetch
const commitFilesCache = new LRU<Hash, Promise<Result<number>>>(10);

function fetchSignificantLinesOfCode(hash: Hash) {
function fetchSignificantLinesOfCode(hash: Hash, generatedFiles: string[]) {
const foundPromise = commitFilesCache.get(hash);
if (foundPromise != null) {
return foundPromise;
}
serverAPI.postMessage({
type: 'fetchSignificantLinesOfCode',
hash,
generatedFiles,
});

const resultPromise = serverAPI
Expand All @@ -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<string[]>((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: {
Expand All @@ -58,7 +69,7 @@ function SplitSuggestionImpl({commit}: {commit: CommitInfo}) {
setSignificantLinesOfCode(result.value);
}
});
}, [commit.hash]);
}, [commit.filesSample, commit.hash, generatedStatuses]);
if (significantLinesOfCode <= 100) {
return null;
}
Expand Down Expand Up @@ -94,11 +105,19 @@ 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) {
return null;
}
return <SplitSuggestionImpl commit={commit} />;
}

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 <GatedSplitSuggestion commit={commit} />;
}
2 changes: 1 addition & 1 deletion addons/isl/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit 67e5819

Please sign in to comment.