Pass depth down to git clone in order to correctly diff the changes

package.json

@@ -43,7 +43,7 @@
     "@atomist/automation-client": "*"
   },
   "devDependencies": {
-    "@atomist/automation-client": "1.0.0-M.3",
+    "@atomist/automation-client": "1.0.0-master.20180905201706",
     "@types/mocha": "^5.2.5",
     "@types/power-assert": "^1.4.29",
     "axios-mock-adapter": "^1.15.0",

src/api-helper/goal/DefaultGoalImplementationMapper.ts

@@ -40,7 +40,7 @@ export class DefaultGoalImplementationMapper implements GoalImplementationMapper
             m.goal.context === goal.externalKey);
 
         if (matchedNames.length > 1) {
-            throw new Error(`Multiple implementations found for name '${goal.fulfillment.name}' on goal '${goal.name}'`);
+            throw new Error(`Multiple implementations found for name '${goal.fulfillment.name}' on goal '${goal.uniqueName}'`);
         }
         if (matchedNames.length === 0) {
             throw new Error(`No implementation found with name '${goal.fulfillment.name}': ` +

src/api-helper/goal/chooseAndSetGoals.ts

@@ -137,22 +137,28 @@ export async function determineGoals(rules: {
 }> {
     const { projectLoader, repoRefResolver, goalSetter, implementationMapping } = rules;
     const { credentials, id, context, push, addressChannels, goalSetId } = circumstances;
-    return projectLoader.doWithProject({ credentials, id, context, readOnly: true }, async project => {
-        const pli: PushListenerInvocation = {
-            project,
+    return projectLoader.doWithProject({
             credentials,
-            id,
-            push,
-            context,
-            addressChannels,
-        };
-        const determinedGoals = await chooseGoalsForPushOnProject({ goalSetter }, pli);
-        if (!determinedGoals) {
-            return { determinedGoals: undefined, goalsToSave: [] };
-        }
-        const goalsToSave = await sdmGoalsFromGoals(implementationMapping, repoRefResolver, pli, determinedGoals, goalSetId);
-        return { determinedGoals, goalsToSave };
-    });
+            id, context,
+            readOnly: true,
+            depth: push.commits.length + 1, // we need at least the commits of the push + 1 to be able to diff it
+        },
+        async project => {
+            const pli: PushListenerInvocation = {
+                project,
+                credentials,
+                id,
+                push,
+                context,
+                addressChannels,
+            };
+            const determinedGoals = await chooseGoalsForPushOnProject({ goalSetter }, pli);
+            if (!determinedGoals) {
+                return { determinedGoals: undefined, goalsToSave: [] };
+            }
+            const goalsToSave = await sdmGoalsFromGoals(implementationMapping, repoRefResolver, pli, determinedGoals, goalSetId);
+            return { determinedGoals, goalsToSave };
+        });
 
 }
 
@@ -174,19 +180,19 @@ async function sdmGoalsFromGoals(implementationMapping: GoalImplementationMapper
 }
 
 async function fulfillment(rules: {
-    implementationMapping: GoalImplementationMapper,
-},                         g: Goal, inv: PushListenerInvocation): Promise<SdmGoalFulfillment> {
+                                implementationMapping: GoalImplementationMapper,
+                            },
+                           g: Goal,
+                           inv: PushListenerInvocation): Promise<SdmGoalFulfillment> {
     const { implementationMapping } = rules;
     const plan = await implementationMapping.findFulfillmentByPush(g, inv);
     if (isGoalImplementation(plan)) {
         return constructSdmGoalImplementation(plan);
-    }
-    if (isGoalSideEffect(plan)) {
+    } else if (isGoalSideEffect(plan)) {
         return { method: SdmGoalFulfillmentMethod.SideEffect, name: plan.sideEffectName };
+    } else {
+        throw new Error(`No implementation or side-effect found for goal '${g.definition.uniqueName}' `);
     }
-
-    logger.warn("No implementation found for '%s'", g.name);
-    return { method: SdmGoalFulfillmentMethod.Other, name: "unknown" };
 }
 
 /**

src/api-helper/listener/createPushImpactListenerInvocation.ts

@@ -23,10 +23,7 @@ import { Destination } from "@atomist/automation-client/spi/message/MessageClien
 import { messageDestinationsFor } from "../../api/context/addressChannels";
 import { GoalInvocation } from "../../api/goal/GoalInvocation";
 import { PushImpactListenerInvocation } from "../../api/listener/PushImpactListener";
-import {
-    filesChangedSince,
-    filesChangedSinceParentCommit,
-} from "../misc/git/filesChangedSince";
+import { filesChangedSince } from "../misc/git/filesChangedSince";
 import { filteredView } from "../misc/project/filteredView";
 
 /**
@@ -42,9 +39,7 @@ export async function createPushImpactListenerInvocation(goalInvocation: GoalInv
     const smartContext = teachToRespondInEventHandler(context, ...messageDestinationsFor(sdmGoal.push.repo, context));
 
     const push = sdmGoal.push;
-    const filesChanged = push.before ?
-        await filesChangedSince(project, push.before.sha) :
-        await filesChangedSinceParentCommit(project);
+    const filesChanged = await filesChangedSince(project, push);
     const impactedSubProject = !filesChanged ? project : filteredView(project, path => filesChanged.includes(path));
     return {
         id,

src/api-helper/listener/executeAutoInspects.ts

@@ -46,11 +46,16 @@ import { relevantCodeActions } from "./relevantCodeActions";
 export function executeAutoInspects(autoInspectRegistrations: Array<AutoInspectRegistration<any, any>>,
                                     reviewListeners: ReviewListenerRegistration[]): ExecuteGoal {
     return async (goalInvocation: GoalInvocation) => {
-        const { configuration, credentials, id, addressChannels } = goalInvocation;
+        const { sdmGoal, configuration, credentials, id, addressChannels } = goalInvocation;
         try {
             if (autoInspectRegistrations.length > 0) {
                 logger.info("Planning inspection of %j with %d AutoInspects", id, autoInspectRegistrations.length);
-                return configuration.sdm.projectLoader.doWithProject({ credentials, id, readOnly: true }, async project => {
+                return configuration.sdm.projectLoader.doWithProject({
+                    credentials,
+                    id,
+                    readOnly: true,
+                    depth: sdmGoal.push.commits.length + 1,
+                }, async project => {
                     const cri = await createPushImpactListenerInvocation(goalInvocation, project);
                     const relevantAutoInspects = await relevantCodeActions(autoInspectRegistrations, cri);
                     logger.info("Executing review of %j with %d relevant AutoInspects: [%s] of [%s]",

src/api-helper/listener/executeAutofixes.ts

@@ -70,6 +70,7 @@ export function executeAutofixes(registrations: AutofixRegistration[]): ExecuteG
                     id: editableRepoRef,
                     context,
                     readOnly: false,
+                    depth: push.commits.length + 1,
                 },
                 async project => {
                     const cri: PushImpactListenerInvocation = await createPushImpactListenerInvocation(goalInvocation, project);

src/api-helper/listener/executeFingerprinting.ts

@@ -37,13 +37,18 @@ import { relevantCodeActions } from "./relevantCodeActions";
 export function executeFingerprinting(fingerprinters: FingerprinterRegistration[],
                                       listeners: FingerprintListener[]): ExecuteGoal {
     return async (goalInvocation: GoalInvocation) => {
-        const { configuration, id, credentials, context } = goalInvocation;
+        const { sdmGoal, configuration, id, credentials, context } = goalInvocation;
         if (fingerprinters.length === 0) {
             return Success;
         }
 
         logger.debug("About to fingerprint %j using %d fingerprinters", id, fingerprinters.length);
-        await configuration.sdm.projectLoader.doWithProject({ credentials, id, readOnly: true }, async project => {
+        await configuration.sdm.projectLoader.doWithProject({
+            credentials,
+            id,
+            readOnly: true,
+            depth: sdmGoal.push.commits.length + 1,
+        }, async project => {
             const cri = await createPushImpactListenerInvocation(goalInvocation, project);
             const relevantFingerprinters: FingerprinterRegistration[] = await relevantCodeActions(fingerprinters, cri);
             logger.info("Will invoke %d eligible fingerprinters of %d to %j",

src/api-helper/listener/executePushReactions.ts

@@ -29,7 +29,6 @@ import {
     PushReactionResponse,
     toPushReactionRegistration,
 } from "../../api/registration/PushImpactListenerRegistration";
-import { ProjectLoader } from "../../spi/project/ProjectLoader";
 import { createPushImpactListenerInvocation } from "./createPushImpactListenerInvocation";
 import { relevantCodeActions } from "./relevantCodeActions";
 
@@ -44,8 +43,14 @@ export function executePushReactions(registrations: PushImpactListenerRegisterab
             return Success;
         }
 
-        const { configuration, credentials, id, context } = goalInvocation;
-        return configuration.sdm.projectLoader.doWithProject({ credentials, id, context, readOnly: true }, async project => {
+        const { sdmGoal, configuration, credentials, id, context } = goalInvocation;
+        return configuration.sdm.projectLoader.doWithProject({
+            credentials,
+            id,
+            context,
+            readOnly: true,
+            depth: sdmGoal.push.commits.length + 1,
+        }, async project => {
             const cri: PushImpactListenerInvocation = await createPushImpactListenerInvocation(goalInvocation, project);
             const regs = registrations.map(toPushReactionRegistration);
             const relevantCodeReactions: PushImpactListenerRegistration[] = await relevantCodeActions<PushImpactListenerRegistration>(regs, cri);

src/api-helper/misc/git/filesChangedSince.ts

@@ -17,33 +17,32 @@
 import { logger } from "@atomist/automation-client";
 import { runCommand } from "@atomist/automation-client/action/cli/commandLine";
 import { GitProject } from "@atomist/automation-client/project/git/GitProject";
+import { PushFields } from "../../../typings/types";
 
 /**
  * Use git to list the files changed since the given sha
  * or undefined if we cannot determine it
  * @param {GitProject} project
- * @param {string} sha
+ * @param {PushFields.Fragment} push
  * @return {Promise<string[]>}
  */
-export async function filesChangedSince(project: GitProject, sha: string): Promise<string[] | undefined> {
-    if (!sha) {
-        logger.info(`No sha passed in on ${JSON.stringify(project.id)}: Looking for parent sha`);
-        return filesChangedSinceParentCommit(project);
-    }
+export async function filesChangedSince(project: GitProject, push: PushFields.Fragment): Promise<string[] | undefined> {
+    // get the number of commits from the after
+    const commitCount = push && push.commits ? push.commits.length : 1;
+    const sha = push && push.after ? push.after.sha : "HEAD";
 
-    const command = `git diff --name-only ${sha}`;
+    const command = `git diff --name-only ${sha}~${commitCount}`;
     try {
         const cr = await runCommand(command, { cwd: project.baseDir });
         // stdout is nothing but a list of files, one per line
-        logger.debug(`$Output from filesChangedSince ${sha} on ${JSON.stringify(project.id)}:\n${cr.stdout}`);
+        logger.debug(`Output from filesChangedSince ${sha} on ${JSON.stringify(project.id)}:\n${cr.stdout}`);
         return cr.stdout.split("\n")
             .filter(n => !!n);
     } catch (err) {
-        logger.warn("Error diffing project %j since %s: %s", project.id, sha, err.message);
-        logger.warn("Project sha = %s, branch = %s", project.id.sha, project.id.branch);
+        logger.warn("Error diffing project %j since '%s': %s", project.id, sha, err.message);
         try {
             const gs = await project.gitStatus();
-            logger.warn("Git status sha = %s, branch = %s", gs.sha, gs.branch);
+            logger.warn("Git status sha '%s' and branch '%s'", gs.sha, gs.branch);
             const timeOfLastChange = await runCommand("ls -ltr .", { cwd: project.baseDir });
             logger.info("Files with dates: " + timeOfLastChange.stdout);
         } catch (err) {
@@ -53,17 +52,6 @@ export async function filesChangedSince(project: GitProject, sha: string): Promi
     }
 }
 
-// TODO: we should use the earliest commit in the push, and find its parent. See: https://github.com/atomist/github-sdm/issues/293
-// we're using this to list changes for code reactions, and that should include all changes in the push.
-export async function filesChangedSinceParentCommit(project: GitProject): Promise<string[] | undefined> {
-    try {
-        return filesChangedSince(project, "HEAD^");
-    } catch (err) {
-        logger.warn("Error diffing project %j finding parent: %s", project.id, err.message);
-        return undefined;
-    }
-}
-
 export type Mod = "added" | "deleted" | "modified" | "renamed";
 
 export interface Change {
@@ -79,18 +67,6 @@ export class Rename implements Change {
     }
 }
 
-export async function changesSince(project: GitProject, sha: string): Promise<string[]> {
-    const command = `git diff --name-status ${sha}`;
-    const cr = await runCommand(command, { cwd: project.baseDir });
-    // stdout is nothing but a list of files, one per line
-    logger.debug(`$Output from filesChangedSince ${sha} on ${JSON.stringify(project.id)}:\n${cr.stdout}`);
-    if (1 === 1) {
-        throw new Error("Not yet implemented");
-    }
-    return cr.stdout.split("\n")
-        .filter(n => !!n);
-}
-
 /**
  * Does a file satisfying this text exist within the set of changed files?
  * @param {string[]} changedFilePaths

src/api-helper/project/cloningProjectLoader.ts

@@ -22,7 +22,12 @@ import { ProjectLoader } from "../../spi/project/ProjectLoader";
  */
 export const CloningProjectLoader: ProjectLoader = {
     async doWithProject(coords, action) {
-        const p = await GitCommandGitProject.cloned(coords.credentials, coords.id);
+        // This is breaking old internal code but we need the branch, so this error gives us an opportunity
+        // to fix all lingering cloning issues
+        if (!coords.id.branch) {
+            throw new Error(`Repository reference '${JSON.stringify(coords.id)}' is missing required branch`);
+        }
+        const p = await GitCommandGitProject.cloned(coords.credentials, coords.id, { depth: coords.depth });
         return action(p);
     },
 };

src/api-helper/test/fakeGoalInvocation.ts

@@ -15,6 +15,7 @@
  */
 
 import { logger } from "@atomist/automation-client";
+import { guid } from "@atomist/automation-client/internal/util/string";
 import {
     RemoteRepoRef,
     RepoId,
@@ -93,6 +94,7 @@ function fakeSdmGoal(id: RepoId): SdmGoalEvent {
                     },
                 }],
             },
+            commits: [{ sha: guid() }],
         },
     };
 }

src/api/goal/Goal.ts

@@ -85,7 +85,7 @@ export class Goal {
     }
 
     get waitingForApprovalDescription() {
-        return this.definition.waitingForApprovalDescription || (this.successDescription + "(but needs approval)");
+        return this.definition.waitingForApprovalDescription || `Approval required: ${this.successDescription}`;
     }
 
     get retryIntent() {

src/spi/project/ProjectLoader.ts

@@ -32,6 +32,9 @@ export interface ProjectLoadingParameters {
 
     /** Return true to get optimized behavior for read only */
     readOnly: boolean;
+
+    /** Indicate how many commits of the history are required */
+    depth?: number;
 }
 
 /**