Merge pull request #71 from MichaelCurrin/fix-handle-filenames-with-spaces

MichaelCurrin · web-flow · commit 01a8ddff3c6e · 2022-01-04T18:14:56.000+02:00
fix: Handle filenames with spaces
diff --git a/src/generate/action.ts b/src/generate/action.ts
@@ -8,7 +8,7 @@
  * differently.
  */
 import { ACTION, ROOT } from "../lib/constants";
-import { moveOrRenameFromPaths, splitPath } from "../lib/paths";
+import { moveOrRenameFromPaths, quoteForSpaces, splitPath } from "../lib/paths";
 import { ActionKeys } from "./action.d";
 
 /**
@@ -35,13 +35,19 @@ export function moveOrRenameMsg(oldPath: string, newPath: string): string {
 
   let msg;
 
+  const from = quoteForSpaces(oldP.name);
+
   if (moveDesc === "move") {
-    msg = `move ${oldP.name} to ${newP.dirPath}`;
+    const to = quoteForSpaces(newP.dirPath);
+    msg = `move ${from} to ${to}`;
   } else if (moveDesc === "rename") {
-    msg = `rename ${oldP.name} to ${newP.name}`;
+    const to = quoteForSpaces(newP.name);
+    msg = `rename ${from} to ${to}`;
   } else {
-    const target = newP.dirPath === ROOT ? `${newP.name} at ${ROOT}` : newPath;
-    msg = `move and rename ${oldP.name} to ${target}`;
+    const to = quoteForSpaces(newP.name);
+    const target =
+      newP.dirPath === ROOT ? `${to} at ${ROOT}` : quoteForSpaces(newPath);
+    msg = `move and rename ${from} to ${target}`;
   }
 
   return msg;
diff --git a/src/generate/message.ts b/src/generate/message.ts
@@ -42,9 +42,10 @@ export function oneChange(line: string) {
 /**
  * Describe an action and paths for a set of changed files.
  *
- * @param lines Expect one or more lines that came from a `git` subcommand.
+ * @param lines Expect one or more lines that came from a Git subcommand.
  *
- * @return A human-readable sentence decribing file changes. e.g. 'update foo.txt and bar.txt'.
+ * @return A human-readable sentence decribing file changes. e.g. 'update
+ *   foo.txt and bar.txt'.
  */
 export function namedFilesDesc(changes: FileChange[]) {
   const actions = changes.map(item => item.x as ActionKeys);
diff --git a/src/git/parseOutput.ts b/src/git/parseOutput.ts
@@ -62,7 +62,7 @@ export function parseDiffIndex(line: string): FileChange {
   const x = line[0];
   const y = UNMODIFIED;
 
-  const [_, from, to] = line.split(/\s+/);
+  const [_, from, to] = line.split("\t");
   if (!from) {
     // Unlikely in real life, but this helps in development.
     throw new Error(`Invalid input - could not find 'from' path: ${line}`);
diff --git a/src/lib/paths.ts b/src/lib/paths.ts
@@ -7,8 +7,8 @@ import * as path from "path";
 import { ROOT } from "../lib/constants";
 import { MoveOrRename, SplitPathResult } from "./paths.d";
 
-// The starts of filenames which could be repeated in a repo. All lowercase
-// here.
+// The starts of filenames which might be repeated as files in a repo. Kept as
+// all lowercase here.
 const REPEAT_FILENAMES = ["readme", "index", "__init__.py"];
 
 /**
@@ -36,6 +36,15 @@ export function splitPath(filePath: string): SplitPathResult {
   };
 }
 
+/** Format to add quotes if the values contains spaces. */
+export function quoteForSpaces(value: string) {
+  if (value.includes(" ") && value !== ROOT) {
+    return `'${value}'`;
+  }
+
+  return value;
+}
+
 /**
  * Change file path to a more readable format.
  *
@@ -50,10 +59,10 @@ export function friendlyFile(filePath: string) {
 
   for (const p of REPEAT_FILENAMES) {
     if (nameLower.startsWith(p)) {
-      return filePath;
+      return quoteForSpaces(filePath);
     }
   }
-  return name;
+  return quoteForSpaces(name);
 }
 
 /**
diff --git a/src/test/generate/action.test.ts b/src/test/generate/action.test.ts
@@ -39,33 +39,58 @@ describe("Desribe a file using two paths", function () {
         moveOrRenameMsg("foo.txt", "bar.txt"),
         "rename foo.txt to bar.txt"
       );
+
       assert.strictEqual(
         moveOrRenameMsg("buzz/foo.txt", "buzz/bar.txt"),
         "rename foo.txt to bar.txt"
       );
+
+      assert.strictEqual(
+        moveOrRenameMsg("fizz buzz/foo.txt", "fizz buzz/bar.txt"),
+        "rename foo.txt to bar.txt"
+      );
     });
 
     it("can describe a moved file", function () {
       assert.strictEqual(
         moveOrRenameMsg("buzz/foo.txt", "fizz/foo.txt"),
         "move foo.txt to fizz"
       );
+
+      assert.strictEqual(
+        moveOrRenameMsg("buzz/foo bar.txt", "fizz/foo bar.txt"),
+        "move 'foo bar.txt' to fizz"
+      );
+
       assert.strictEqual(
         moveOrRenameMsg("buzz/foo.txt", "foo.txt"),
         "move foo.txt to repo root"
       );
+
+      assert.strictEqual(
+        moveOrRenameMsg("buzz/foo bar.txt", "foo bar.txt"),
+        "move 'foo bar.txt' to repo root"
+      );
     });
 
     it("can describe a remamed and moved file", function () {
       assert.strictEqual(
         moveOrRenameMsg("foo.txt", "fizz/bar.txt"),
         "move and rename foo.txt to fizz/bar.txt"
       );
-      // This is a rare case, so don't bother trying to handle it smarter around paths.
+
+      // This is a rare case, so don't bother trying to handle it smarter around
+      // paths.
       assert.strictEqual(
         moveOrRenameMsg("fuzz/foo.txt", "fizz/bar.txt"),
         "move and rename foo.txt to fizz/bar.txt"
       );
+
+      assert.strictEqual(
+        moveOrRenameMsg("fuzz/foo.txt", "fizz/bar bazz.txt"),
+        "move and rename foo.txt to 'fizz/bar bazz.txt'"
+      );
+
       assert.strictEqual(
         moveOrRenameMsg("fizz/foo.txt", "bar.txt"),
         "move and rename foo.txt to bar.txt at repo root"
diff --git a/src/test/generate/convCommit.test.ts b/src/test/generate/convCommit.test.ts
@@ -81,7 +81,7 @@ describe("Test #ConventionalCommit class for path-based conventional commit logi
   });
 
   describe("#isBuildRelated", function () {
-    it("can recognize a build change fr a build-related filename", function () {
+    it("can recognize a build change for a build-related filename", function () {
       assert.strictEqual(
         new ConventionalCommit("Dockerfile").isBuildRelated(),
         true
@@ -186,10 +186,11 @@ describe("Test #ConventionalCommit class for path-based conventional commit logi
   });
 
   describe("#getType", function () {
-    // Rather than true and false like in above tests this actually categorizes and also it closer
-    // to the real world as it through a hierarchy (for example .yml is config-related unless it is
-    // for a CI file). But, this doesn't care what the action is like create or delete or modify, so
-    // it won't impose meaning based on that.
+    // Rather than true and false like in above tests this actually categorizes
+    // and also it closer to the real world as it through a hierarchy (for
+    // example .yml is config-related unless it is for a CI file). But, this
+    // doesn't care what the action is like create or delete or modify, so it
+    // won't impose meaning based on that.
     it("sees a build file as build", function () {
       assert.strictEqual(
         new ConventionalCommit("Makefile").getType(),
@@ -232,7 +233,7 @@ describe("Test #ConventionalCommit class for path-based conventional commit logi
       );
     });
 
-    // TODO Break into categories
+    // TODO: Break into categories
     it("can tell a type for other types", function () {
       assert.strictEqual(
         new ConventionalCommit("foo").getType(),
diff --git a/src/test/generate/count.test.ts b/src/test/generate/count.test.ts
@@ -30,6 +30,22 @@ describe("Aggregate counts of files as numeric data", function () {
         assert.deepStrictEqual(_countByAction(changes), expected);
       });
 
+      it("should handle a created file with spaces", function () {
+        const changes: FileChange[] = [
+          {
+            x: "A",
+            y: " ",
+            from: "foo bar.txt",
+            to: "",
+          },
+        ];
+        const expected = {
+          create: { fileCount: 1 },
+        };
+
+        assert.deepStrictEqual(_countByAction(changes), expected);
+      });
+
       it("should handle an updated file", function () {
         const changes: FileChange[] = [
           {
diff --git a/src/test/generate/message.test.ts b/src/test/generate/message.test.ts
@@ -10,20 +10,27 @@ import { namedFilesDesc, oneChange } from "../../generate/message";
 
 describe("Generate commit message for a single changed file", function () {
   // Notes:
-  //   - The command `git status --short` expects XY format but this is for `git
-  //     diff-index` which is only X. Also there is just spaces between - no
-  //     '->' symbol.
+  //   - The command `git status --short` expects `XY` format but this is for
+  //     `git diff-index` which is only `X`. Also there is just spaces between -
+  //     no '->' symbol.
   //   - Impossible cases are not covered here, like renaming a file and the
   //     name and path are unchanged, or including two file names for an add
   //     line. But validation on at least file name is done.
   describe("#oneChange", function () {
     it("returns the appropriate commit message for a new file", function () {
       assert.strictEqual(oneChange("A\tfoo.txt"), "create foo.txt");
-      // Maybe create foo.txt in bar, if the dir is not too long?
+
+      // TODO: Maybe 'create foo.txt in bar', if the dir is not too long?
       assert.strictEqual(oneChange("A\tbar/foo.txt"), "create foo.txt");
+
+      assert.strictEqual(oneChange("A\tfoo bar.txt"), "create 'foo bar.txt'");
+      assert.strictEqual(
+        oneChange("A\tfizz buzz/foo bar.txt"),
+        "create 'foo bar.txt'"
+      );
     });
 
-    it("throws an error if no filepath can be no generated", function () {
+    it("throws an error if no file path can be no generated", function () {
       assert.throws(() => oneChange("A    "));
     });
 
@@ -58,7 +65,7 @@ describe("Generate commit message for a single changed file", function () {
       );
     });
 
-    it("describes a file moved out of the repo root", function () {
+    it("describes a file moved out of the repo root to another directory", function () {
       assert.strictEqual(
         oneChange("R\tfoo.txt\tfizz/foo.txt"),
         "move foo.txt to fizz"
@@ -68,6 +75,16 @@ describe("Generate commit message for a single changed file", function () {
         oneChange("R\tfoo.txt\tfizz/buzz/foo.txt"),
         "move foo.txt to fizz/buzz"
       );
+
+      assert.strictEqual(
+        oneChange("R\tfoo.txt\tfizz buzz/foo.txt"),
+        "move foo.txt to 'fizz buzz'"
+      );
+
+      assert.strictEqual(
+        oneChange("R\tfoo bar.txt\tfizz/foo bar.txt"),
+        "move 'foo bar.txt' to fizz"
+      );
     });
 
     it("describes a file moved out of a subdirectory", function () {
@@ -82,9 +99,14 @@ describe("Generate commit message for a single changed file", function () {
       );
 
       assert.strictEqual(
-        oneChange("R\tfizz/buzz/foo.txt\tfizz/buzz/foo.txt"),
+        oneChange("R\tfizz/foo.txt\tfizz/buzz/foo.txt"),
         "move foo.txt to fizz/buzz"
       );
+
+      assert.strictEqual(
+        oneChange("R\tfizz/foo bar.txt\tfizz/buzz baz/foo bar.txt"),
+        "move 'foo bar.txt' to 'fizz/buzz baz'"
+      );
     });
 
     it("describes a file that was both moved and renamed", function () {
@@ -102,6 +124,11 @@ describe("Generate commit message for a single changed file", function () {
         oneChange("R\tbar/foo.txt\tfizz/fuzz.txt"),
         "move and rename foo.txt to fizz/fuzz.txt"
       );
+
+      assert.strictEqual(
+        oneChange("R\tbar/foo.txt\tfizz/baz fuzz.txt"),
+        "move and rename foo.txt to 'fizz/baz fuzz.txt'"
+      );
     });
 
     it("ignores percentage changed value for a file that was both moved and renamed", function () {
@@ -127,6 +154,10 @@ describe("Generate commit message for a single changed file", function () {
       );
 
       assert.strictEqual(oneChange("A\tfoo/index.md"), "create foo/index.md");
+      assert.strictEqual(
+        oneChange("A\tfoo/index bazz.md"),
+        "create 'foo/index bazz.md'"
+      );
       assert.strictEqual(oneChange("A\tfoo/index.js"), "create foo/index.js");
     });
   });
@@ -143,6 +174,14 @@ describe("Generate description for a few changed files which each get named", fu
         "create foo.txt and bar.txt"
       );
 
+      assert.strictEqual(
+        namedFilesDesc([
+          { x: "A", from: "foo bar.txt", y: " ", to: "" },
+          { x: "A", from: "fizz buzz.txt", y: " ", to: "" },
+        ]),
+        "create 'foo bar.txt' and 'fizz buzz.txt'"
+      );
+
       assert.strictEqual(
         namedFilesDesc([
           { x: "M", from: "foo.txt", y: " ", to: "" },
@@ -170,6 +209,15 @@ describe("Generate description for a few changed files which each get named", fu
         "create foo.txt, bar.txt and buzz.js"
       );
 
+      assert.strictEqual(
+        namedFilesDesc([
+          { x: "A", from: "foo.txt", y: " ", to: "" },
+          { x: "A", from: "docs/bar fuzz.txt", y: " ", to: "" },
+          { x: "A", from: "buzz.js", y: " ", to: "" },
+        ]),
+        "create foo.txt, 'bar fuzz.txt' and buzz.js"
+      );
+
       assert.strictEqual(
         namedFilesDesc([
           { x: "D", from: "foo.txt", y: " ", to: "" },
diff --git a/src/test/git/parseOutput.test.ts b/src/test/git/parseOutput.test.ts
diff --git a/src/test/lib/commonPath.test.ts b/src/test/lib/commonPath.test.ts
diff --git a/src/test/lib/paths.test.ts b/src/test/lib/paths.test.ts
diff --git a/src/test/prepareCommitMsg.test.ts b/src/test/prepareCommitMsg.test.ts

Original file line number	Diff line number	Diff line change
`@@ -42,9 +42,10 @@ export function oneChange(line: string) {`
`42`	`42`	`/**`
`43`	`43`	`* Describe an action and paths for a set of changed files.`
`44`	`44`	`*`
`45`		- * @param lines Expect one or more lines that came from a `git` subcommand.
	`45`	`+ * @param lines Expect one or more lines that came from a Git subcommand.`
`46`	`46`	`*`
`47`		`- * @return A human-readable sentence decribing file changes. e.g. 'update foo.txt and bar.txt'.`
	`47`	`+ * @return A human-readable sentence decribing file changes. e.g. 'update`
	`48`	`+ * foo.txt and bar.txt'.`
`48`	`49`	`*/`
`49`	`50`	`export function namedFilesDesc(changes: FileChange[]) {`
`50`	`51`	`const actions = changes.map(item => item.x as ActionKeys);`