Skip to content

Commit

Permalink
file-search: fix not ignoring .git
Browse files Browse the repository at this point in the history 
- Fix issue where ripgrep would look into `.git` when using
  `.gitignore`.
- Use `files.exclude`.

Signed-off-by: Paul Maréchal <[email protected]>
  • Loading branch information
@paul-marechal
paul-marechal committed Nov 6, 2020
1 parent fea40a2 commit 3071218
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 43 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Change Log

## v1.8.0

<a name="breaking_changes_1.8.0">[Breaking Changes:](#breaking_changes_1.8.0)</a>

- [file-search] Deprecate dependency on `@theia/process` and replaced its usage by node's `child_process` api.

## v1.7.0 - 29/10/2020

<a name="release_milestone_1.7.0">[1.7.0 Release Milestone](https://github.com/eclipse-theia/theia/milestone/12?closed=1)</a>
Expand Down
7 changes: 6 additions & 1 deletion packages/file-search/src/browser/quick-file-open.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import { Command } from '@theia/core/lib/common';
import { NavigationLocationService } from '@theia/editor/lib/browser/navigation/navigation-location-service';
import * as fuzzy from 'fuzzy';
import { MessageService } from '@theia/core/lib/common/message-service';
import { FileSystemPreferences } from '@theia/filesystem/lib/browser';

export const quickFileOpen: Command = {
id: 'file-search.openFile',
Expand All @@ -55,6 +56,8 @@ export class QuickFileOpenService implements QuickOpenModel, QuickOpenHandler {
protected readonly navigationLocationService: NavigationLocationService;
@inject(MessageService)
protected readonly messageService: MessageService;
@inject(FileSystemPreferences)
protected readonly fsPreferences: FileSystemPreferences;

/**
* Whether to hide .gitignored (and other ignored) files.
Expand Down Expand Up @@ -198,7 +201,9 @@ export class QuickFileOpenService implements QuickOpenModel, QuickOpenHandler {
fuzzyMatch: true,
limit: 200,
useGitIgnore: this.hideIgnoredFiles,
excludePatterns: ['*.git*']
excludePatterns: this.hideIgnoredFiles
? Object.keys(this.fsPreferences['files.exclude'])
: undefined,
}, token).then(handler);
} else {
if (roots.length !== 0) {
Expand Down
34 changes: 21 additions & 13 deletions packages/file-search/src/node/file-search-service-impl.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,18 @@ import { FileUri } from '@theia/core/lib/node';
import { Container, ContainerModule } from 'inversify';
import { CancellationTokenSource } from '@theia/core';
import { bindLogger } from '@theia/core/lib/node/logger-backend-module';
import processBackendModule from '@theia/process/lib/node/process-backend-module';
import URI from '@theia/core/lib/common/uri';
import { FileSearchService } from '../common/file-search-service';
import { RawProcessFactory } from '@theia/process/lib/node';

/* eslint-disable no-unused-expressions */

const testContainer = new Container();

bindLogger(testContainer.bind.bind(testContainer));
testContainer.load(processBackendModule);
testContainer.bind(RawProcessFactory).toConstantValue(() => {
throw new Error('should not be used anymore');
});
testContainer.load(new ContainerModule(bind => {
bind(FileSearchServiceImpl).toSelf().inSingletonScope();
}));
Expand All @@ -52,16 +54,16 @@ describe('search-service', function (): void {
// eslint-disable-next-line deprecation/deprecation
const expectedFile = FileUri.create(__filename).displayName;
const testFile = matches.find(e => e.endsWith(expectedFile));
expect(testFile).to.be.not.undefined;
expect(testFile).to.not.be.undefined;
});

it.skip('should respect nested .gitignore', async () => {
const rootUri = FileUri.create(path.resolve(__dirname, '../../test-resources')).toString();
const matches = await service.find('foo', { rootUris: [rootUri], fuzzyMatch: false });

expect(matches.find(match => match.endsWith('subdir1/sub-bar/foo.txt'))).to.be.undefined;
expect(matches.find(match => match.endsWith('subdir1/sub2/foo.txt'))).to.be.not.undefined;
expect(matches.find(match => match.endsWith('subdir1/foo.txt'))).to.be.not.undefined;
expect(matches.find(match => match.endsWith('subdir1/sub2/foo.txt'))).to.not.be.undefined;
expect(matches.find(match => match.endsWith('subdir1/foo.txt'))).to.not.be.undefined;
});

it('should cancel searches', async () => {
Expand All @@ -78,7 +80,7 @@ describe('search-service', function (): void {
const dirB = FileUri.create(path.resolve(__dirname, '../../test-resources/subdir1/sub2')).toString();

const matches = await service.find('foo', { rootUris: [dirA, dirB] });
expect(matches).to.be.not.undefined;
expect(matches).to.not.be.undefined;
expect(matches.length).to.eq(2);
});

Expand All @@ -87,19 +89,19 @@ describe('search-service', function (): void {
const rootUri = FileUri.create(path.resolve(__dirname, '../../test-resources/subdir1/sub2')).toString();

const matches = await service.find('', { rootUris: [rootUri], includePatterns: ['**/*oo.*'] });
expect(matches).to.be.not.undefined;
expect(matches).to.not.be.undefined;
expect(matches.length).to.eq(1);
});

it('should NOT support file searches with globs without the prefixed or trailing star (*)', async () => {
const rootUri = FileUri.create(path.resolve(__dirname, '../../test-resources/subdir1/sub2')).toString();

const trailingMatches = await service.find('', { rootUris: [rootUri], includePatterns: ['*oo'] });
expect(trailingMatches).to.be.not.undefined;
expect(trailingMatches).to.not.be.undefined;
expect(trailingMatches.length).to.eq(0);

const prefixedMatches = await service.find('', { rootUris: [rootUri], includePatterns: ['oo*'] });
expect(prefixedMatches).to.be.not.undefined;
expect(prefixedMatches).to.not.be.undefined;
expect(prefixedMatches.length).to.eq(0);
});
});
Expand All @@ -109,7 +111,7 @@ describe('search-service', function (): void {
const rootUri = FileUri.create(path.resolve(__dirname, '../../test-resources/subdir1/sub2')).toString();

const matches = await service.find('', { rootUris: [rootUri], includePatterns: ['**/*oo.*'], excludePatterns: ['foo'] });
expect(matches).to.be.not.undefined;
expect(matches).to.not.be.undefined;
expect(matches.length).to.eq(1);
});

Expand Down Expand Up @@ -153,7 +155,7 @@ describe('search-service', function (): void {

async function assertIgnoreGlobs(options: FileSearchService.Options): Promise<void> {
const matches = await service.find('', options);
expect(matches).to.be.not.undefined;
expect(matches).to.not.be.undefined;
expect(matches.length).to.eq(0);
}
});
Expand All @@ -180,11 +182,17 @@ describe('search-service', function (): void {
const relativeMatch = relativeUri!.toString();
let position = 0;
for (const ch of 'shell') {
position = relativeMatch.indexOf(ch, position);
assert.notStrictEqual(position, -1, relativeMatch);
position = relativeMatch.toLowerCase().indexOf(ch, position);
assert.notStrictEqual(position, -1, `character "${ch}" not found in "${relativeMatch}"`);
}
}
});

it('should not look into .git', async () => {
const matches = await service.find('master', { rootUris: [rootUri.toString()], fuzzyMatch: false, useGitIgnore: true, limit: 200 });
// `**/.git/refs/remotes/*/master` files should not be picked up
assert.deepStrictEqual([], matches);
});
});

});
65 changes: 36 additions & 29 deletions packages/file-search/src/node/file-search-service-impl.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
********************************************************************************/

import * as cp from 'child_process';
import * as fuzzy from 'fuzzy';
import * as readline from 'readline';
import { rgPath } from 'vscode-ripgrep';
Expand All @@ -30,7 +31,9 @@ export class FileSearchServiceImpl implements FileSearchService {

constructor(
@inject(ILogger) protected readonly logger: ILogger,
@inject(RawProcessFactory) protected readonly rawProcessFactory: RawProcessFactory) { }
/** @deprecated since 1.7.0 */
@inject(RawProcessFactory) protected readonly rawProcessFactory: RawProcessFactory,
) { }

async find(searchPattern: string, options: FileSearchService.Options, clientToken?: CancellationToken): Promise<string[]> {
const cancellationSource = new CancellationTokenSource();
Expand Down Expand Up @@ -111,37 +114,38 @@ export class FileSearchServiceImpl implements FileSearchService {
return [...exactMatches, ...fuzzyMatches].slice(0, opts.limit);
}

private doFind(rootUri: URI, options: FileSearchService.BaseOptions,
accept: (fileUri: string) => void, token: CancellationToken): Promise<void> {
private doFind(rootUri: URI, options: FileSearchService.BaseOptions, accept: (fileUri: string) => void, token: CancellationToken): Promise<void> {
return new Promise((resolve, reject) => {
try {
const cwd = FileUri.fsPath(rootUri);
const args = this.getSearchArgs(options);
// TODO: why not just child_process.spawn, theia process are supposed to be used for user processes like tasks and terminals, not internal
const process = this.rawProcessFactory({ command: rgPath, args, options: { cwd } });
process.onError(reject);
process.outputStream.on('close', resolve);
token.onCancellationRequested(() => process.kill());

const lineReader = readline.createInterface({
input: process.outputStream,
output: process.inputStream
});
lineReader.on('line', line => {
if (token.isCancellationRequested) {
process.kill();
} else {
accept(line);
}
});
} catch (e) {
reject(e);
}
const cwd = FileUri.fsPath(rootUri);
const args = this.getSearchArgs(options);
const ripgrep = cp.spawn(rgPath, args, { cwd, stdio: ['pipe', 'pipe', 'inherit'] });
ripgrep.on('error', reject);
ripgrep.on('exit', (code, signal) => {
if (typeof code === 'number' && code !== 0) {
reject(new Error(`"${rgPath}" exited with code: ${code}`));
} else if (typeof signal === 'string') {
reject(new Error(`"${rgPath}" was terminated by signal: ${signal}`));
}
});
token.onCancellationRequested(() => {
ripgrep.kill(); // most likely sends a signal.
resolve(); // avoid rejecting for no good reason.
});
const lineReader = readline.createInterface({
input: ripgrep.stdout,
crlfDelay: Infinity,
});
lineReader.on('line', line => {
if (!token.isCancellationRequested) {
accept(line);
}
});
lineReader.on('close', () => resolve());
});
}

private getSearchArgs(options: FileSearchService.BaseOptions): string[] {
const args = ['--files', '--hidden', '--case-sensitive'];
const args = ['--files', '--hidden'];
if (options.includePatterns) {
for (const includePattern of options.includePatterns) {
if (includePattern) {
Expand All @@ -156,8 +160,11 @@ export class FileSearchServiceImpl implements FileSearchService {
}
}
}
if (!options.useGitIgnore) {
args.push('-uu');
if (options.useGitIgnore) {
// ripgrep follows `.gitignore` by default, but it doesn't exclude `.git`:
args.push('--glob', '!.git');
} else {
args.push('--no-ignore');
}
return args;
}
Expand Down

0 comments on commit 3071218

Please sign in to comment.