fix: distributed .gitignore and loose pkgDir matching

Author: mshanemc

.mocharc.json

@@ -1,6 +1,7 @@
 {
   "require": "test/init.js, ts-node/register, source-map-support/register",
   "watch-extensions": "ts",
+  "watch-files": ["src", "test"],
   "recursive": true,
   "reporter": "spec",
   "timeout": 5000

package.json

@@ -24,7 +24,8 @@
     "pretest": "sf-compile-test",
     "prune:dead": "ts-prune | grep -v 'source-deploy-retrieve' | grep -v 'index.ts'",
     "test": "sf-test",
-    "test:nuts": "nyc mocha \"**/*.nut.ts\" --slow 4500 --timeout 600000 --parallel"
+    "test:nuts": "nyc mocha \"**/*.nut.ts\" --slow 4500 --timeout 600000 --parallel",
+    "test:nuts:local": "mocha \"**/local*.nut.ts\" --slow 4500 --timeout 600000 --parallel"
   },
   "keywords": [
     "force",

src/shared/functions.ts

@@ -5,9 +5,10 @@
  * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
  */
 
+import { sep } from 'path';
+import { isString } from '@salesforce/ts-types';
 import { SourceComponent } from '@salesforce/source-deploy-retrieve';
 import { RemoteChangeElement, ChangeResult } from './types';
-
 export const getMetadataKey = (metadataType: string, metadataName: string): string => {
   return `${metadataType}__${metadataName}`;
 };
@@ -20,3 +21,19 @@ export const getKeyFromObject = (element: RemoteChangeElement | ChangeResult): s
 };
 
 export const isBundle = (cmp: SourceComponent): boolean => cmp.type.strategies?.adapter === 'bundle';
+
+/**
+ * Verify that a filepath starts exactly with a complete parent path
+ * ex: '/foo/bar-extra/baz'.startsWith('foo/bar') would be true, but this function understands that they are not in the same folder
+ */
+export const pathIsInFolder = (filePath: string, folder: string, separator = sep): boolean => {
+  const biggerStringParts = filePath.split(separator).filter(nonEmptyStringFilter);
+  return folder
+    .split(separator)
+    .filter(nonEmptyStringFilter)
+    .every((part, index) => part === biggerStringParts[index]);
+};
+
+const nonEmptyStringFilter = (value: string): boolean => {
+  return isString(value) && value.length > 0;
+};

src/shared/localShadowRepo.ts

@@ -10,6 +10,7 @@ import * as path from 'path';
 import * as os from 'os';
 import { NamedPackageDir, Logger, fs } from '@salesforce/core';
 import * as git from 'isomorphic-git';
+import { pathIsInFolder } from './functions';
 
 const gitIgnoreFileName = '.gitignore';
 const stashedGitIgnoreFileName = '.BAK.gitignore';
@@ -43,6 +44,9 @@ interface CommitRequest {
   message?: string;
 }
 
+// const gitIgnoreLocator = async (filepath: string) => {
+//   ( return filepath.endsWith(gitIgnoreFileName) ? filepath : null).filter(isString);
+// }
 export class ShadowRepo {
   private static instance: ShadowRepo;
 
@@ -52,6 +56,7 @@ export class ShadowRepo {
   private packageDirs!: NamedPackageDir[];
   private status!: StatusRow[];
   private logger!: Logger;
+  private gitIgnoreLocations: string[] = [];
 
   private constructor(options: ShadowRepoOptions) {
     this.gitDir = getGitDir(options.orgId, options.projectPath);
@@ -84,6 +89,26 @@ export class ShadowRepo {
   public async gitInit(): Promise<void> {
     await fs.promises.mkdir(this.gitDir, { recursive: true });
     await git.init({ fs, dir: this.projectPath, gitdir: this.gitDir, defaultBranch: 'main' });
+    // set the forceIgnoreLocations so we only have to do it once
+    // this looks through
+    this.gitIgnoreLocations = (
+      (await git.walk({
+        fs,
+        dir: this.projectPath,
+        gitdir: this.gitDir,
+        trees: [git.WORKDIR()],
+        // TODO: this can be marginally faster if we limit it to pkgDirs and toplevel project files
+        // eslint-disable-next-line @typescript-eslint/require-await
+        map: async (filepath: string) => filepath,
+      })) as string[]
+    )
+      .filter(
+        (filepath) =>
+          filepath.includes(gitIgnoreFileName) &&
+          // can be top-level like '.' (no sep) OR must be in one of the package dirs
+          (!filepath.includes(path.sep) || this.packageDirs.some((dir) => pathIsInFolder(filepath, dir.path)))
+      )
+      .map((ignoreFile) => path.join(this.projectPath, ignoreFile));
   }
 
   /**
@@ -124,8 +149,13 @@ export class ShadowRepo {
           dir: this.projectPath,
           gitdir: this.gitDir,
           filepaths,
-          // filter out hidden files and __tests__ patterns, regardless of gitignore
-          filter: (f) => !f.includes(`${path.sep}.`) && !f.includes('__tests__'),
+          // filter out hidden files and __tests__ patterns, regardless of gitignore, and the gitignore files themselves
+          filter: (f) =>
+            !f.includes(`${path.sep}.`) &&
+            !f.includes('__tests__') &&
+            ![gitIgnoreFileName, stashedGitIgnoreFileName].includes(path.basename(f)) &&
+            // isogit uses `startsWith` for filepaths so it's possible to get a false positive
+            filepaths.some((pkgDir) => pathIsInFolder(f, pkgDir, path.posix.sep)),
         });
         // isomorphic-git stores things in unix-style tree.  Convert to windows-style if necessary
         if (isWindows) {
@@ -246,18 +276,20 @@ export class ShadowRepo {
   }
 
   private async stashIgnoreFile(): Promise<void> {
-    const originalLocation = path.join(this.projectPath, gitIgnoreFileName);
-    // another process may have already stashed the file
-    if (fs.existsSync(originalLocation)) {
-      await fs.promises.rename(originalLocation, path.join(this.projectPath, stashedGitIgnoreFileName));
-    }
+    // allSettled allows them to fail (example, the file wasn't where it was expected).
+    await Promise.allSettled(
+      this.gitIgnoreLocations.map((originalLocation) =>
+        fs.promises.rename(originalLocation, originalLocation.replace(gitIgnoreFileName, stashedGitIgnoreFileName))
+      )
+    );
   }
 
   private async unStashIgnoreFile(): Promise<void> {
-    const stashedLocation = path.join(this.projectPath, stashedGitIgnoreFileName);
-    // another process may have already un-stashed the file
-    if (fs.existsSync(stashedLocation)) {
-      await fs.promises.rename(stashedLocation, path.join(this.projectPath, gitIgnoreFileName));
-    }
+    // allSettled allows them to fail (example, the file wasn't where it was expected).
+    await Promise.allSettled(
+      this.gitIgnoreLocations.map((originalLocation) =>
+        fs.promises.rename(originalLocation.replace(gitIgnoreFileName, stashedGitIgnoreFileName), originalLocation)
+      )
+    );
   }
 }

src/sourceTracking.ts

@@ -34,7 +34,7 @@ import {
   RemoteChangeElement,
 } from './shared/types';
 import { sourceComponentGuard, metadataMemberGuard } from './shared/guards';
-import { getKeyFromObject, getMetadataKey, isBundle } from './shared/functions';
+import { getKeyFromObject, getMetadataKey, isBundle, pathIsInFolder } from './shared/functions';
 
 export interface SourceTrackingOptions {
   org: Org;
@@ -103,8 +103,8 @@ export class SourceTracking extends AsyncCreatable {
       byPackageDir
         ? this.packagesDirs.map((pkgDir) => ({
             path: pkgDir.name,
-            nonDeletes: allNonDeletes.filter((f) => f.startsWith(pkgDir.name)),
-            deletes: allDeletes.filter((f) => f.startsWith(pkgDir.name)),
+            nonDeletes: allNonDeletes.filter((f) => pathIsInFolder(f, pkgDir.name)),
+            deletes: allDeletes.filter((f) => pathIsInFolder(f, pkgDir.name)),
           }))
         : [
             {
@@ -367,7 +367,7 @@ export class SourceTracking extends AsyncCreatable {
           await this.localRepo.getDeleteFilenames()
         ).filter(
           (deployedFile) =>
-            bundlesWithDeletedFiles.some((bundlePath) => deployedFile.startsWith(bundlePath)) &&
+            bundlesWithDeletedFiles.some((bundlePath) => pathIsInFolder(deployedFile, bundlePath)) &&
             !relativeOptions.files.includes(deployedFile)
         )
       ),

test/nuts/ignoreInSubfolder

@@ -0,0 +1 @@
+Subproject commit 3dd4590a61872c00f0b385be958fab0d2af309c7

test/nuts/localNonTopLevelIgnore.nut.ts

@@ -0,0 +1,45 @@
+/*
+ * Copyright (c) 2020, salesforce.com, inc.
+ * All rights reserved.
+ * Licensed under the BSD 3-Clause license.
+ * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
+ */
+import * as path from 'path';
+import { TestSession } from '@salesforce/cli-plugins-testkit';
+import { fs } from '@salesforce/core';
+import { expect } from 'chai';
+import { ShadowRepo } from '../../src/shared/localShadowRepo';
+
+describe('handles non-top-level ignore', () => {
+  let session: TestSession;
+  let repo: ShadowRepo;
+
+  before(async () => {
+    session = await TestSession.create({
+      project: {
+        sourceDir: path.join('test', 'nuts', 'ignoreInSubfolder', 'nested-classes'),
+      },
+      authStrategy: 'NONE',
+    });
+  });
+
+  it('initialize the local tracking', async () => {
+    repo = await ShadowRepo.getInstance({
+      orgId: 'fakeOrgId2',
+      projectPath: session.project.dir,
+      packageDirs: [{ path: 'classes', name: 'classes', fullPath: path.join(session.project.dir, 'classes') }],
+    });
+    // verify the local tracking files/directories
+    expect(fs.existsSync(repo.gitDir));
+  });
+
+  it('should not be influenced by gitignore', async () => {
+    expect(await repo.getChangedFilenames())
+      .to.be.an('array')
+      .with.length(2);
+  });
+
+  after(async () => {
+    await session?.clean();
+  });
+});

test/nuts/localPkgDirMatching.nut.ts

@@ -0,0 +1,48 @@
+/*
+ * Copyright (c) 2020, salesforce.com, inc.
+ * All rights reserved.
+ * Licensed under the BSD 3-Clause license.
+ * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
+ */
+import * as path from 'path';
+import { TestSession } from '@salesforce/cli-plugins-testkit';
+import { fs } from '@salesforce/core';
+import { expect } from 'chai';
+import { ShadowRepo } from '../../src/shared/localShadowRepo';
+
+describe('verifies exact match of pkgDirs', () => {
+  let session: TestSession;
+  let repo: ShadowRepo;
+
+  before(async () => {
+    session = await TestSession.create({
+      project: {
+        sourceDir: path.join('test', 'nuts', 'ignoreInSubfolder', 'extra-classes'),
+      },
+      authStrategy: 'NONE',
+    });
+  });
+
+  it('initialize the local tracking', async () => {
+    repo = await ShadowRepo.getInstance({
+      orgId: 'fakeOrgId3',
+      projectPath: session.project.dir,
+      packageDirs: [{ path: 'force-app', name: 'force-app', fullPath: path.join(session.project.dir, 'force-app') }],
+    });
+    // verify the local tracking files/directories
+    expect(fs.existsSync(repo.gitDir));
+  });
+
+  it('should not include files from force-app-extra', async () => {
+    const changedFilenames = await repo.getChangedFilenames();
+
+    // expect(changedFilenames).to.be.an('array').with.length(2);
+    changedFilenames.map((f) => {
+      expect(f).to.not.contain('force-app-extra');
+    });
+  });
+
+  after(async () => {
+    await session?.clean();
+  });
+});

test/unit/stringStartsWithString.test.ts

@@ -0,0 +1,37 @@
+/*
+ * Copyright (c) 2020, salesforce.com, inc.
+ * All rights reserved.
+ * Licensed under the BSD 3-Clause license.
+ * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
+ */
+import { normalize } from 'path';
+import { expect } from 'chai';
+import { pathIsInFolder } from '../../src/shared/functions';
+
+describe('stringStartsWithString', () => {
+  it('does not misidentify partial strings', () => {
+    expect(pathIsInFolder(normalize('/foo/bar-extra/baz'), normalize('/foo/bar'))).to.equal(false);
+  });
+
+  it('does not misidentify partial strings (inverse)', () => {
+    expect(pathIsInFolder(normalize('/foo/bar-extra/baz'), normalize('/foo/bar-extra'))).to.equal(true);
+  });
+
+  it('single top-level dir is ok', () => {
+    expect(pathIsInFolder(normalize('/foo/bar-extra/baz'), normalize('/foo'))).to.equal(true);
+  });
+
+  it('no initial separator on 1st arg is ok', () => {
+    expect(pathIsInFolder(normalize('/foo/bar-extra/baz'), 'foo')).to.equal(true);
+  });
+
+  it('no initial separator on 2nd arg is ok', () => {
+    expect(pathIsInFolder(normalize('foo/bar-extra/baz'), '/foo')).to.equal(true);
+  });
+
+  it('works for deep children', () => {
+    expect(pathIsInFolder(normalize('/foo/bar-extra/baz/some/deep/path'), normalize('/foo/bar-extra/baz'))).to.equal(
+      true
+    );
+  });
+});