Gracefully handle ignored symlinks (#34)

h3rmanj · s0 · web-flow · commit 231d400d0a0f · 2025-05-07T19:51:03.000+01:00
* tests: add a symlink to changes

* tests: Add more tests for symlinks

* fix: check isIgnored first

* fix: Add more useful error for non-ignored symlinks

* chore: add changeset

---------

Co-authored-by: Sam Lanning &lt;sam@samlanning.com&gt;
diff --git a/.changeset/spicy-zoos-wink.md b/.changeset/spicy-zoos-wink.md
@@ -0,0 +1,5 @@
+---
+"@changesets/ghcommit": patch
+---
+
+More gracefully handle symlinks, and ignore them when included in .gitignore
diff --git a/src/git.ts b/src/git.ts
@@ -7,6 +7,16 @@ import {
   CommitFilesResult,
 } from "./interface";
 
+/**
+ * @see https://isomorphic-git.org/docs/en/walk#walkerentry-mode
+ */
+const FILE_MODES = {
+  directory: 0o40000,
+  file: 0o100644,
+  executableFile: 0o100755,
+  symlink: 0o120000,
+} as const;
+
 export const commitChangesFromRepo = async ({
   base,
   repoDirectory = process.cwd(),
@@ -40,12 +50,6 @@ export const commitChangesFromRepo = async ({
     dir: repoDirectory,
     trees,
     map: async (filepath, [commit, workdir]) => {
-      const prevOid = await commit?.oid();
-      const currentOid = await workdir?.oid();
-      // Don't include files that haven't changed, and exist in both trees
-      if (prevOid === currentOid && !commit === !workdir) {
-        return null;
-      }
       // Don't include ignored files
       if (
         await git.isIgnored({
@@ -56,6 +60,20 @@ export const commitChangesFromRepo = async ({
       ) {
         return null;
       }
+      if (
+        (await commit?.mode()) === FILE_MODES.symlink ||
+        (await workdir?.mode()) === FILE_MODES.symlink
+      ) {
+        throw new Error(
+          `Unexpected symlink at ${filepath}, GitHub API only supports files and directories. You may need to add this file to .gitignore`,
+        );
+      }
+      const prevOid = await commit?.oid();
+      const currentOid = await workdir?.oid();
+      // Don't include files that haven't changed, and exist in both trees
+      if (prevOid === currentOid && !commit === !workdir) {
+        return null;
+      }
       // Iterate through anything that may be a directory in either the
       // current commit or the working directory
       if (
diff --git a/src/test/integration/git.test.ts b/src/test/integration/git.test.ts
@@ -78,7 +78,14 @@ const expectParentHasOid = async ({
   expect(commit.parents.nodes).toEqual([{ oid }]);
 };
 
-const makeFileChanges = async (repoDirectory: string) => {
+const makeFileChanges = async (
+  repoDirectory: string,
+  changegroup:
+    | "standard"
+    | "with-ignored-symlink"
+    | "with-included-valid-symlink"
+    | "with-included-invalid-symlink",
+) => {
   // Update an existing file
   await fs.promises.writeFile(
     path.join(repoDirectory, "LICENSE"),
@@ -113,6 +120,34 @@ const makeFileChanges = async (repoDirectory: string) => {
     path.join(repoDirectory, "coverage", "foo", "bar"),
     "This file should be ignored",
   );
+  if (changegroup === "with-ignored-symlink") {
+    // node_modules is ignored in this repo
+    await fs.promises.mkdir(path.join(repoDirectory, "node_modules"), {
+      recursive: true,
+    });
+    await fs.promises.symlink(
+      path.join(repoDirectory, "non-existent"),
+      path.join(repoDirectory, "node_modules", "nested"),
+    );
+  }
+  if (changegroup === "with-included-valid-symlink") {
+    await fs.promises.mkdir(path.join(repoDirectory, "some-dir"), {
+      recursive: true,
+    });
+    await fs.promises.symlink(
+      path.join(repoDirectory, "README.md"),
+      path.join(repoDirectory, "some-dir", "nested"),
+    );
+  }
+  if (changegroup === "with-included-invalid-symlink") {
+    await fs.promises.mkdir(path.join(repoDirectory, "some-dir"), {
+      recursive: true,
+    });
+    await fs.promises.symlink(
+      path.join(repoDirectory, "non-existent"),
+      path.join(repoDirectory, "some-dir", "nested"),
+    );
+  }
 };
 
 const makeFileChangeAssertions = async (branch: string) => {
@@ -154,62 +189,156 @@ describe("git", () => {
   describe("commitChangesFromRepo", () => {
     const testDir = path.join(ROOT_TEMP_DIRECTORY, "commitChangesFromRepo");
 
-    it("should correctly commit all changes", async () => {
-      const branch = `${TEST_BRANCH_PREFIX}-multiple-changes`;
-      branches.push(branch);
-
-      await fs.promises.mkdir(testDir, { recursive: true });
-      const repoDirectory = path.join(testDir, "repo-1");
-
-      // Clone the git repo locally using the git cli and child-process
-      await new Promise<void>((resolve, reject) => {
-        const p = execFile(
-          "git",
-          ["clone", process.cwd(), "repo-1"],
-          { cwd: testDir },
-          (error) => {
-            if (error) {
-              reject(error);
-            } else {
-              resolve();
-            }
+    for (const group of ["standard", "with-ignored-symlink"] as const) {
+      it(`should correctly commit all changes for group: ${group}`, async () => {
+        const branch = `${TEST_BRANCH_PREFIX}-multiple-changes-${group}`;
+        branches.push(branch);
+
+        await fs.promises.mkdir(testDir, { recursive: true });
+        const repoDirectory = path.join(testDir, `repo-1-${group}`);
+
+        // Clone the git repo locally using the git cli and child-process
+        await new Promise<void>((resolve, reject) => {
+          const p = execFile(
+            "git",
+            ["clone", process.cwd(), `repo-1-${group}`],
+            { cwd: testDir },
+            (error) => {
+              if (error) {
+                reject(error);
+              } else {
+                resolve();
+              }
+            },
+          );
+          p.stdout?.pipe(process.stdout);
+          p.stderr?.pipe(process.stderr);
+        });
+
+        await makeFileChanges(repoDirectory, group);
+
+        // Push the changes
+        await commitChangesFromRepo({
+          octokit,
+          ...REPO,
+          branch,
+          message: {
+            headline: "Test commit",
+            body: "This is a test commit",
           },
+          repoDirectory,
+          log,
+        });
+
+        await waitForGitHubToBeReady();
+
+        await makeFileChangeAssertions(branch);
+
+        // Expect the OID to be the HEAD commit
+        const oid =
+          (
+            await git.log({
+              fs,
+              dir: repoDirectory,
+              ref: "HEAD",
+              depth: 1,
+            })
+          )[0]?.oid ?? "NO_OID";
+
+        await expectParentHasOid({ branch, oid });
+      });
+    }
+
+    describe(`should throw appropriate error when symlink is present`, () => {
+      it(`and file does not exist`, async () => {
+        const branch = `${TEST_BRANCH_PREFIX}-invalid-symlink-error`;
+        branches.push(branch);
+
+        await fs.promises.mkdir(testDir, { recursive: true });
+        const repoDirectory = path.join(testDir, `repo-invalid-symlink`);
+
+        // Clone the git repo locally using the git cli and child-process
+        await new Promise<void>((resolve, reject) => {
+          const p = execFile(
+            "git",
+            ["clone", process.cwd(), `repo-invalid-symlink`],
+            { cwd: testDir },
+            (error) => {
+              if (error) {
+                reject(error);
+              } else {
+                resolve();
+              }
+            },
+          );
+          p.stdout?.pipe(process.stdout);
+          p.stderr?.pipe(process.stderr);
+        });
+
+        await makeFileChanges(repoDirectory, "with-included-invalid-symlink");
+
+        // Push the changes
+        await expect(() =>
+          commitChangesFromRepo({
+            octokit,
+            ...REPO,
+            branch,
+            message: {
+              headline: "Test commit",
+              body: "This is a test commit",
+            },
+            repoDirectory,
+            log,
+          }),
+        ).rejects.toThrow(
+          "Unexpected symlink at some-dir/nested, GitHub API only supports files and directories. You may need to add this file to .gitignore",
         );
-        p.stdout?.pipe(process.stdout);
-        p.stderr?.pipe(process.stderr);
       });
 
-      await makeFileChanges(repoDirectory);
-
-      // Push the changes
-      await commitChangesFromRepo({
-        octokit,
-        ...REPO,
-        branch,
-        message: {
-          headline: "Test commit",
-          body: "This is a test commit",
-        },
-        repoDirectory,
-        log,
+      it(`and file exists`, async () => {
+        const branch = `${TEST_BRANCH_PREFIX}-valid-symlink-error`;
+        branches.push(branch);
+
+        await fs.promises.mkdir(testDir, { recursive: true });
+        const repoDirectory = path.join(testDir, `repo-valid-symlink`);
+
+        // Clone the git repo locally using the git cli and child-process
+        await new Promise<void>((resolve, reject) => {
+          const p = execFile(
+            "git",
+            ["clone", process.cwd(), `repo-valid-symlink`],
+            { cwd: testDir },
+            (error) => {
+              if (error) {
+                reject(error);
+              } else {
+                resolve();
+              }
+            },
+          );
+          p.stdout?.pipe(process.stdout);
+          p.stderr?.pipe(process.stderr);
+        });
+
+        await makeFileChanges(repoDirectory, "with-included-valid-symlink");
+
+        // Push the changes
+        await expect(() =>
+          commitChangesFromRepo({
+            octokit,
+            ...REPO,
+            branch,
+            message: {
+              headline: "Test commit",
+              body: "This is a test commit",
+            },
+            repoDirectory,
+            log,
+          }),
+        ).rejects.toThrow(
+          "Unexpected symlink at some-dir/nested, GitHub API only supports files and directories. You may need to add this file to .gitignore",
+        );
       });
-
-      await waitForGitHubToBeReady();
-
-      await makeFileChangeAssertions(branch);
-
-      // Expect the OID to be the HEAD commit
-      const oid =
-        (
-          await git.log({
-            fs,
-            dir: repoDirectory,
-            ref: "HEAD",
-            depth: 1,
-          })
-        )[0]?.oid ?? "NO_OID";
-
-      await expectParentHasOid({ branch, oid });
     });
 
     it("should correctly be able to base changes off specific commit", async () => {
@@ -237,7 +366,7 @@ describe("git", () => {
         p.stderr?.pipe(process.stderr);
       });
 
-      makeFileChanges(repoDirectory);
+      makeFileChanges(repoDirectory, "standard");
 
       // Determine the previous commit hash
       const gitLog = await git.log({

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +"@changesets/ghcommit": patch
 +---
++
 +More gracefully handle symlinks, and ignore them when included in .gitignore