feat: handle the move-then-modify scenario

mshanemc · mshanemc · commit 7ba53ae0c1dd · 2024-06-04T14:21:27.000-05:00
diff --git a/src/shared/local/localShadowRepo.ts b/src/shared/local/localShadowRepo.ts
@@ -15,7 +15,7 @@ import git from 'isomorphic-git';
 import { Performance } from '@oclif/core';
 import { RegistryAccess } from '@salesforce/source-deploy-retrieve';
 import { chunkArray, excludeLwcLocalOnlyTest, folderContainsPath } from '../functions';
-import { filenameMatchesToMap, getMatches } from './moveDetection';
+import { filenameMatchesToMap, getLogMessage, getMatches } from './moveDetection';
 import { StatusRow } from './types';
 import { isDeleted, isAdded, toFilenames } from './functions';
 
@@ -348,21 +348,19 @@ export class ShadowRepo {
     const movedFilesMarker = Performance.mark('@salesforce/source-tracking', 'localShadowRepo.detectMovedFiles');
     const matches = await filenameMatchesToMap(IS_WINDOWS)(this.registry)(this.projectPath)(this.gitDir)(matchingFiles);
 
-    if (matches.size === 0) return movedFilesMarker?.stop();
+    if (matches.deleteOnly.size === 0 && matches.fullMatches.size === 0) return movedFilesMarker?.stop();
 
-    this.logger.debug(
-      [
-        'Files have moved. Committing moved files:',
-        [...matches.entries()].map(([add, del]) => `- File ${del} was moved to ${add}`).join(os.EOL),
-      ].join(os.EOL)
-    );
+    this.logger.debug(getLogMessage(matches));
 
-    movedFilesMarker?.addDetails({ filesMoved: matches.size });
+    movedFilesMarker?.addDetails({
+      filesMoved: matches.fullMatches.size,
+      filesMovedAndEdited: matches.deleteOnly.size,
+    });
 
     // Commit the moved files and refresh the status
     await this.commitChanges({
-      deletedFiles: [...matches.values()],
-      deployedFiles: [...matches.keys()],
+      deletedFiles: [...matches.fullMatches.values(), ...matches.deleteOnly.values()],
+      deployedFiles: [...matches.fullMatches.keys()],
       message: 'Committing moved files',
     });
 
diff --git a/src/shared/local/moveDetection.ts b/src/shared/local/moveDetection.ts
@@ -5,6 +5,7 @@
  * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
  */
 import path from 'node:path';
+import { EOL } from 'node:os';
 import { Logger, Lifecycle } from '@salesforce/core';
 import {
   MetadataResolver,
@@ -15,22 +16,29 @@ import {
 // @ts-expect-error isogit has both ESM and CJS exports but node16 module/resolution identifies it as ESM
 import git from 'isomorphic-git';
 import * as fs from 'graceful-fs';
-import { Performance } from '@oclif/core';
+import { Performance } from '@oclif/core/performance';
 import { sourceComponentGuard } from '../guards';
 import { isDeleted, isAdded, ensureWindows, toFilenames } from './functions';
 import { AddAndDeleteMaps, FilenameBasenameHash, StatusRow, StringMap } from './types';
 
+const JOIN_CHAR = '#__#'; // the __ makes it unlikely to be used in metadata names
 type AddAndDeleteFileInfos = { addedInfo: FilenameBasenameHash[]; deletedInfo: FilenameBasenameHash[] };
 type AddedAndDeletedFilenames = { added: Set<string>; deleted: Set<string> };
+type StringMapsForMatches = {
+  /** these matches filename=>basename, metadata type/name, and git object hash */
+  fullMatches: StringMap;
+  /** these did not match the hash.  They *probably* are matches where the "add" is also modified */
+  deleteOnly: StringMap;
+};
 
 /** composed functions to simplified use by the shadowRepo class */
 export const filenameMatchesToMap =
   (isWindows: boolean) =>
   (registry: RegistryAccess) =>
   (projectPath: string) =>
   (gitDir: string) =>
-  async ({ added, deleted }: AddedAndDeletedFilenames): Promise<StringMap> =>
-    removeNonMatches(isWindows)(registry)(
+  async ({ added, deleted }: AddedAndDeletedFilenames): Promise<StringMapsForMatches> =>
+    excludeNonMatchingTypes(isWindows)(registry)(
       compareHashes(
         await buildMaps(
           await toFileInfo({
@@ -73,7 +81,14 @@ export const getMatches = (status: StatusRow[]): AddedAndDeletedFilenames => {
   return { added: addedFilenamesWithMatches, deleted: deletedFilenamesWithMatches };
 };
 
-/** build maps of the add/deletes with filenames, returning the matches  Logs if non-matches */
+export const getLogMessage = (matches: StringMapsForMatches): string =>
+  [
+    'Files have moved. Committing moved files:',
+    ...[...matches.fullMatches.entries()].map(([add, del]) => `- File ${del} was moved to ${add}`),
+    ...[...matches.deleteOnly.entries()].map(([add, del]) => `- File ${del} was moved to ${add} and modified`),
+  ].join(EOL);
+
+/** build maps of the add/deletes with filenames, returning the matches  Logs if we can't make a match because buildMap puts them in the ignored bucket */
 const buildMaps = async ({ addedInfo, deletedInfo }: AddAndDeleteFileInfos): Promise<AddAndDeleteMaps> => {
   const [addedMap, addedIgnoredMap] = buildMap(addedInfo);
   const [deletedMap, deletedIgnoredMap] = buildMap(deletedInfo);
@@ -96,51 +111,70 @@ const buildMaps = async ({ addedInfo, deletedInfo }: AddAndDeleteFileInfos): Pro
   return { addedMap, deletedMap };
 };
 
-/** builds a map of the values from both maps */
-const compareHashes = ({ addedMap, deletedMap }: AddAndDeleteMaps): StringMap => {
+/**
+ * builds a map of the values from both maps
+ * side effect: mutates the passed-in maps!
+ */
+const compareHashes = ({ addedMap, deletedMap }: AddAndDeleteMaps): StringMapsForMatches => {
   const matches: StringMap = new Map();
 
-  for (const [addedKey, addedValue] of addedMap) {
+  [...addedMap.entries()].map(([addedKey, addedValue]) => {
     const deletedValue = deletedMap.get(addedKey);
     if (deletedValue) {
+      // these are an exact basename and hash match
       matches.set(addedValue, deletedValue);
+      deletedMap.delete(addedKey);
+      addedMap.delete(addedKey);
     }
-  }
+  });
 
-  return matches;
+  if (addedMap.size && deletedMap.size) {
+    // the remaining deletes didn't match the basename+hash of an add, and vice versa.
+    // They *might* match the basename of an add, in which case we *could* have the "move, then edit" case.
+    const addedBasenameMap = new Map([...addedMap.entries()].map(hashEntryToBasenameEntry));
+    const deletedBasenameMap = new Map([...deletedMap.entries()].map(hashEntryToBasenameEntry));
+    const deleteOnly = new Map<string, string>(
+      Array.from(deletedBasenameMap.entries())
+        .filter(([k]) => addedBasenameMap.has(k))
+        .map(([k, v]) => [addedBasenameMap.get(k) as string, v])
+    );
+    return { fullMatches: matches, deleteOnly };
+  }
+  return { fullMatches: matches, deleteOnly: new Map<string, string>() };
 };
 
 /** given a StringMap, resolve the metadata types and return things that having matching type/parent  */
-const removeNonMatches =
+const excludeNonMatchingTypes =
   (isWindows: boolean) =>
   (registry: RegistryAccess) =>
-  (matches: StringMap): StringMap => {
-    if (!matches.size) return matches;
-    const addedFiles = isWindows ? [...matches.keys()].map(ensureWindows) : [...matches.keys()];
-    const deletedFiles = isWindows ? [...matches.values()].map(ensureWindows) : [...matches.values()];
-    const resolverAdded = new MetadataResolver(registry, VirtualTreeContainer.fromFilePaths(addedFiles));
-    const resolverDeleted = new MetadataResolver(registry, VirtualTreeContainer.fromFilePaths(deletedFiles));
-
-    return new Map(
-      [...matches.entries()].filter(([addedFile, deletedFile]) => {
-        // we're only ever using the first element of the arrays
-        const [resolvedAdded] = resolveType(resolverAdded, isWindows ? [ensureWindows(addedFile)] : [addedFile]);
-        const [resolvedDeleted] = resolveType(
-          resolverDeleted,
-          isWindows ? [ensureWindows(deletedFile)] : [deletedFile]
-        );
-        return (
-          // they could match, or could both be undefined (because unresolved by SDR)
-          resolvedAdded?.type.name === resolvedDeleted?.type.name &&
-          // parent names match, if resolved and there are parents
-          resolvedAdded?.parent?.name === resolvedDeleted?.parent?.name &&
-          // parent types match, if resolved and there are parents
-          resolvedAdded?.parent?.type.name === resolvedDeleted?.parent?.type.name
-        );
-      })
-    );
+  ({ fullMatches: matches, deleteOnly }: StringMapsForMatches): StringMapsForMatches => {
+    if (!matches.size && !deleteOnly.size) return { fullMatches: matches, deleteOnly };
+    const [resolvedAdded, resolvedDeleted] = [
+      [...matches.keys(), ...deleteOnly.keys()], // the keys/values are only used for the resolver, so we use 1 for both add and delete
+      [...matches.values(), ...deleteOnly.values()],
+    ]
+      .map((filenames) => filenames.map(isWindows ? ensureWindows : stringNoOp))
+      .map((filenames) => new MetadataResolver(registry, VirtualTreeContainer.fromFilePaths(filenames)))
+      .map(resolveType);
+
+    return {
+      fullMatches: new Map([...matches.entries()].filter(typeFilter(isWindows)(resolvedAdded, resolvedDeleted))),
+      deleteOnly: new Map([...deleteOnly.entries()].filter(typeFilter(isWindows)(resolvedAdded, resolvedDeleted))),
+    };
   };
 
+const typeFilter =
+  (isWindows: boolean) =>
+  (resolveAdd: ReturnType<typeof resolveType>, resolveDelete: ReturnType<typeof resolveType>) =>
+  ([added, deleted]: [string, string]): boolean => {
+    const [resolvedAdded] = resolveAdd(isWindows ? [ensureWindows(added)] : [added]);
+    const [resolvedDeleted] = resolveDelete(isWindows ? [ensureWindows(deleted)] : [deleted]);
+    return (
+      resolvedAdded?.type.name === resolvedDeleted?.type.name &&
+      resolvedAdded?.parent?.name === resolvedDeleted?.parent?.name &&
+      resolvedAdded?.parent?.type.name === resolvedDeleted?.parent?.type.name
+    );
+  };
 /** enrich the filenames with basename and oid (hash)  */
 const toFileInfo = async ({
   projectPath,
@@ -170,11 +204,12 @@ const toFileInfo = async ({
   return { addedInfo, deletedInfo };
 };
 
+/** returns a map of <hash+basename, filepath>.  If two items result in the same hash+basename, return that in the ignore bucket */
 const buildMap = (info: FilenameBasenameHash[]): StringMap[] => {
   const map: StringMap = new Map();
   const ignore: StringMap = new Map();
   info.map((i) => {
-    const key = `${i.hash}#${i.basename}`;
+    const key = `${i.hash}${JOIN_CHAR}${i.basename}`;
     // If we find a duplicate key, we need to remove it and ignore it in the future.
     // Finding duplicate hash#basename means that we cannot accurately determine where it was moved to or from
     if (map.has(key) || ignore.has(key)) {
@@ -195,18 +230,20 @@ const getHashForAddedFile =
     hash: (await git.hashBlob({ object: await fs.promises.readFile(path.join(projectPath, filepath)) })).oid,
   });
 
-const resolveType = (resolver: MetadataResolver, filenames: string[]): SourceComponent[] =>
-  filenames
-    .flatMap((filename) => {
-      try {
-        return resolver.getComponentsFromPath(filename);
-      } catch (e) {
-        const logger = Logger.childFromRoot('ShadowRepo.compareTypes');
-        logger.warn(`unable to resolve ${filename}`);
-        return undefined;
-      }
-    })
-    .filter(sourceComponentGuard);
+const resolveType =
+  (resolver: MetadataResolver) =>
+  (filenames: string[]): SourceComponent[] =>
+    filenames
+      .flatMap((filename) => {
+        try {
+          return resolver.getComponentsFromPath(filename);
+        } catch (e) {
+          const logger = Logger.childFromRoot('ShadowRepo.compareTypes');
+          logger.warn(`unable to resolve ${filename}`);
+          return undefined;
+        }
+      })
+      .filter(sourceComponentGuard);
 
 /** where we don't have git objects to use, read the file contents to generate the hash */
 const getHashFromActualFileContents =
@@ -218,3 +255,8 @@ const getHashFromActualFileContents =
     basename: path.basename(filepath),
     hash: (await git.readBlob({ fs, dir: projectPath, gitdir, filepath, oid })).oid,
   });
+
+const hashEntryToBasenameEntry = ([k, v]: [string, string]): [string, string] => [hashToBasename(k), v];
+const hashToBasename = (hash: string): string => hash.split(JOIN_CHAR)[1];
+
+const stringNoOp = (s: string): string => s;
diff --git a/test/nuts/local/localTrackingFileMoveThenEdit.nut.ts b/test/nuts/local/localTrackingFileMoveThenEdit.nut.ts
@@ -0,0 +1,91 @@
+/*
+ * 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 'node:path';
+import { TestSession } from '@salesforce/cli-plugins-testkit';
+import { expect } from 'chai';
+import * as fs from 'graceful-fs';
+import { RegistryAccess } from '@salesforce/source-deploy-retrieve';
+import { ProjectJson } from '@salesforce/schemas';
+import { NamedPackageDir, PackageDir } from '@salesforce/core';
+import { ShadowRepo } from '../../../src/shared/local/localShadowRepo';
+
+describe('handles local files moves that also change the file', () => {
+  let session: TestSession;
+  let repo: ShadowRepo;
+  let modifiedPackageDirs: PackageDir[];
+  const NOT_PROJECT_DIR = 'not-project-dir';
+  before(async () => {
+    session = await TestSession.create({
+      project: {
+        sourceDir: path.join('test', 'nuts', 'ebikes-lwc'),
+      },
+      devhubAuthStrategy: 'NONE',
+    });
+    // create the other dir
+    const notProjectDir = path.join(session.project.dir, NOT_PROJECT_DIR, 'main', 'default', 'classes');
+    await fs.promises.mkdir(notProjectDir, { recursive: true });
+
+    // modify the project json to include the new dir
+    const sfdxProjectJsonPath = path.join(session.project.dir, 'sfdx-project.json');
+    const originalProject = JSON.parse(await fs.promises.readFile(sfdxProjectJsonPath, 'utf8')) as ProjectJson;
+    modifiedPackageDirs = [...originalProject.packageDirectories, { path: NOT_PROJECT_DIR }];
+    await fs.promises.writeFile(
+      sfdxProjectJsonPath,
+      JSON.stringify({
+        ...originalProject,
+        packageDirectories: modifiedPackageDirs,
+      } satisfies ProjectJson)
+    );
+  });
+
+  after(async () => {
+    // await session?.clean();
+  });
+
+  it('initialize the local tracking', async () => {
+    expect(typeof process.env.SF_BETA_TRACK_FILE_MOVES).to.equal('undefined');
+    process.env.SF_BETA_TRACK_FILE_MOVES = 'true';
+
+    repo = await ShadowRepo.getInstance({
+      orgId: 'fakeOrgId',
+      projectPath: session.project.dir,
+      packageDirs: modifiedPackageDirs.map(
+        (pd): NamedPackageDir => ({ ...pd, name: NOT_PROJECT_DIR, fullPath: path.join(session.project.dir, pd.path) })
+      ),
+      registry: new RegistryAccess(),
+    });
+
+    // Commit the existing status
+    const filesToSync = await repo.getChangedFilenames();
+    await repo.commitChanges({ deployedFiles: filesToSync });
+
+    expect(await repo.getChangedFilenames()).to.have.lengthOf(0);
+  });
+
+  it('move a file and edit it.  Only the delete is committed', async () => {
+    // move all two classes to the new folder
+    const classFolder = path.join('main', 'default', 'classes');
+    ['OrderController.cls', 'OrderController.cls-meta.xml', 'PagedResult.cls', 'PagedResult.cls-meta.xml'].map((f) =>
+      fs.renameSync(
+        path.join(session.project.dir, 'force-app', classFolder, f),
+        path.join(session.project.dir, NOT_PROJECT_DIR, classFolder, f)
+      )
+    );
+    const editedFilePath = path.join(NOT_PROJECT_DIR, classFolder, 'OrderController.cls');
+    // edit the contents of OrderController.cls
+    fs.appendFileSync(path.join(session.project.dir, editedFilePath), '//comment');
+    await repo.getStatus(true);
+
+    // all the deletes were committed
+    expect(await repo.getDeleteFilenames()).to.deep.equal([]);
+    // this is still considered an "add" because the moved file was changed
+    expect(await repo.getAddFilenames()).to.deep.equal([editedFilePath]);
+
+    delete process.env.SF_BETA_TRACK_FILE_MOVES;
+  });
+});
