Merge pull request #601 from forcedotcom/sm/file-moves-again

feat: handle file move then edit scenario

Author: iowillhoit

CHANGELOG.md

@@ -62,6 +62,7 @@
   "devDependencies": {
     "@salesforce/cli-plugins-testkit": "^5.3.8",
     "@salesforce/dev-scripts": "^10.1.0",
+    "@salesforce/schemas": "^1.9.0",
     "@types/graceful-fs": "^4.1.9",
     "eslint-plugin-sf-plugin": "^1.18.5",
     "ts-node": "^10.9.2",

package.json

@@ -5,7 +5,6 @@
  * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
  */
 import {
-  SourceComponent,
   MetadataMember,
   FileResponse,
   ComponentStatus,
@@ -15,9 +14,6 @@ import {
 import { ChangeResult } from './types';
 import { ChangeResultWithNameAndType } from './types';
 
-export const sourceComponentGuard = (input: SourceComponent | undefined): input is SourceComponent =>
-  input instanceof SourceComponent;
-
 export const metadataMemberGuard = (
   input: MetadataMember | undefined | Partial<MetadataMember>
 ): input is MetadataMember =>
@@ -42,3 +38,5 @@ export const FileResponseHasPath = (
 
 export const isChangeResultWithNameAndType = (cr?: ChangeResult): cr is ChangeResultWithNameAndType =>
   typeof cr === 'object' && typeof cr.name === 'string' && typeof cr.type === 'string';
+
+export const isDefined = <T>(x: T | undefined): x is T => x !== undefined;

src/shared/guards.ts

@@ -15,7 +15,7 @@ import git from 'isomorphic-git';
 import { Performance } from '@oclif/core/performance';
 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',
     });
 

src/shared/local/localShadowRepo.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,
@@ -16,21 +17,28 @@ import {
 import git from 'isomorphic-git';
 import * as fs from 'graceful-fs';
 import { Performance } from '@oclif/core/performance';
-import { sourceComponentGuard } from '../guards';
+import { isDefined } 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,72 @@ 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 => {
-  const matches: StringMap = new Map();
+/**
+ * builds a map of the values from both maps
+ * side effect: mutates the passed-in maps!
+ */
+const compareHashes = ({ addedMap, deletedMap }: AddAndDeleteMaps): StringMapsForMatches => {
+  const matches = new Map<string, string>(
+    [...addedMap.entries()]
+      .map(([addedKey, addedValue]) => {
+        const deletedValue = deletedMap.get(addedKey);
+        if (deletedValue) {
+          // these are an exact basename and hash match
+          deletedMap.delete(addedKey);
+          addedMap.delete(addedKey);
+          return [addedValue, deletedValue] as const;
+        }
+      })
+      .filter(isDefined)
+  );
 
-  for (const [addedKey, addedValue] of addedMap) {
-    const deletedValue = deletedMap.get(addedKey);
-    if (deletedValue) {
-      matches.set(addedValue, deletedValue);
-    }
+  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 matches;
+  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 +206,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 +232,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(isDefined);
 
 /** where we don't have git objects to use, read the file contents to generate the hash */
 const getHashFromActualFileContents =
@@ -218,3 +257,7 @@ 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;

src/shared/local/moveDetection.ts

@@ -14,7 +14,7 @@ import {
   DestructiveChangesType,
   RegistryAccess,
 } from '@salesforce/source-deploy-retrieve';
-import { sourceComponentGuard } from './guards';
+import { isDefined } from './guards';
 import { supportsPartialDelete, pathIsInFolder } from './functions';
 
 type GroupedFileInput = {
@@ -83,7 +83,7 @@ export const getComponentSets = ({
 
       grouping.deletes
         .flatMap((filename) => resolverForDeletes.getComponentsFromPath(filename))
-        .filter(sourceComponentGuard)
+        .filter(isDefined)
         .map((component) => {
           // if the component supports partial delete AND there are files that are not deleted,
           // set the component for deploy, not for delete.
@@ -92,7 +92,7 @@ export const getComponentSets = ({
             try {
               resolverForNonDeletes
                 .getComponentsFromPath(resolve(component.content))
-                .filter(sourceComponentGuard)
+                .filter(isDefined)
                 .map((nonDeletedComponent) => componentSet.add(nonDeletedComponent));
             } catch (e) {
               logger.warn(
@@ -113,7 +113,7 @@ export const getComponentSets = ({
             return undefined;
           }
         })
-        .filter(sourceComponentGuard)
+        .filter(isDefined)
         .map((component) => componentSet.add(component));
       // there may have been ignored files, but componentSet.add doesn't automatically track them.
       // We'll manually set the ignored paths from what the resolver has been tracking

src/shared/localComponentSetArray.ts

@@ -13,7 +13,7 @@ import {
   RegistryAccess,
 } from '@salesforce/source-deploy-retrieve';
 import { ChangeResult } from './types';
-import { isChangeResultWithNameAndType, sourceComponentGuard } from './guards';
+import { isChangeResultWithNameAndType, isDefined } from './guards';
 import {
   ensureRelative,
   excludeLwcLocalOnlyTest,
@@ -68,7 +68,7 @@ export const populateTypesAndNames =
           return undefined;
         }
       })
-      .filter(sourceComponentGuard);
+      .filter(isDefined);
 
     logger.debug(` matching SourceComponents have ${sourceComponents.length} items from local`);
 

src/shared/populateTypesAndNames.ts

@@ -46,8 +46,8 @@ import {
   FileResponseIsDeleted,
   FileResponseIsNotDeleted,
   isChangeResultWithNameAndType,
+  isDefined,
   isSdrSuccess,
-  sourceComponentGuard,
 } from './shared/guards';
 import { removeIgnored } from './shared/remoteChangeIgnoring';
 import {
@@ -309,7 +309,7 @@ export class SourceTracking extends AsyncCreatable {
               return undefined;
             }
           })
-          .filter(sourceComponentGuard);
+          .filter(isDefined);
       }
     }