From 4d562428f8bfebae5504f6189acef68833a6450d Mon Sep 17 00:00:00 2001
From: Sander <hey@sandydoo.me>
Date: Sun, 15 Dec 2024 18:54:18 +0400
Subject: [PATCH 1/2] Simplify internal `before`/`after` code

Consolidates previous work from #533 and #536.

- Removes `before` and `after` from the `raw` config. These are not valid
  pre-commit configuration options and should not be here.
- Removes code to filter out `before` and `after` from the `raw` config.
  There's nothing to filter out if they're not in the config in the
  first place.
- Set `id` to the attr name of the hook, instead of the `name`. This
  corrects a mistake made in the module transition (#397). Technically,
  a breaking change, but we haven't had feedback from the initial
  mistake.
- Expose `id` in options.
---
 modules/hook.nix       | 35 +++++++++++++++++++++++++++++++----
 modules/pre-commit.nix | 21 ++-------------------
 2 files changed, 33 insertions(+), 23 deletions(-)

diff --git a/modules/hook.nix b/modules/hook.nix
index b1d7ab6b..297a7cfc 100644
--- a/modules/hook.nix
+++ b/modules/hook.nix
@@ -25,10 +25,39 @@ in
         '';
     };
 
+    id = mkOption {
+      type = types.str;
+      default = name;
+      defaultText = "the attribute name the hook submodule is bound to";
+      description =
+        ''
+          The unique identifier for the hook.
+
+          You do not need to set or modify this value.
+
+          The `id` is used to reference a hook when using `pre-commit run <id>`.
+          It can also be used to reference the hook in other hooks' `before` and `after` fields to define the order in which hooks run.
+
+          The `id` is set to the attribute name the hook submodule is bound to in the parent module.
+          For example, the `id` of following hook would be `my-hook`.
+
+          ```nix
+          {
+            hooks = {
+              my-hook = {
+                enable = true;
+                entry = "my-hook";
+              };
+            }
+          }
+          ```
+        '';
+    };
+
     name = mkOption {
       type = types.str;
-      defaultText = lib.literalMD "internal name, same as `id`";
       default = name;
+      defaultText = lib.literalMD "the attribute name the hook submodule is bound to, same as `id`";
       description =
         ''
           The name of the hook. Shown during hook execution.
@@ -202,14 +231,12 @@ in
         '';
       default = [ ];
     };
-
   };
 
   config = {
     raw =
       {
-        inherit (config) name entry language files types types_or exclude_types pass_filenames fail_fast require_serial stages verbose always_run args before after;
-        id = config.name;
+        inherit (config) id name entry language files types types_or exclude_types pass_filenames fail_fast require_serial stages verbose always_run args;
         exclude = mergeExcludes config.excludes;
       };
   };
diff --git a/modules/pre-commit.nix b/modules/pre-commit.nix
index a9b2b2ba..0e50e738 100644
--- a/modules/pre-commit.nix
+++ b/modules/pre-commit.nix
@@ -9,7 +9,6 @@ let
     mapAttrsToList
     mkOption
     types
-    removeAttrs
     remove
     ;
 
@@ -34,25 +33,9 @@ let
     let
       sortedHooks = lib.toposort
         (a: b: builtins.elem b.id a.before || builtins.elem a.id b.after)
-        (mapAttrsToList
-          (id: value:
-            value.raw // {
-              inherit id;
-              before = value.raw.before;
-              after = value.raw.after;
-            }
-          )
-          enabledHooks
-        );
-      cleanedHooks = builtins.map (
-        hook:
-        removeAttrs hook [
-          "before"
-          "after"
-        ]
-      ) sortedHooks.result;
+        (builtins.attrValues enabledHooks);
     in
-    cleanedHooks;
+    builtins.map (value: value.raw) sortedHooks.result;
 
   configFile =
     performAssertions (

From 96209c15446f3fa6985306fbb9034635cf69c1fc Mon Sep 17 00:00:00 2001
From: Sander <hey@sandydoo.me>
Date: Sun, 15 Dec 2024 22:33:54 +0400
Subject: [PATCH 2/2] Throw a pretty error if the hooks cannot be sorted

If the hooks form a cycle, we now throw an error with the hooks in
question and show their `before` and `after` fields to help identify the
cycle.
---
 modules/pre-commit.nix | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/modules/pre-commit.nix b/modules/pre-commit.nix
index 0e50e738..b38319c6 100644
--- a/modules/pre-commit.nix
+++ b/modules/pre-commit.nix
@@ -35,7 +35,32 @@ let
         (a: b: builtins.elem b.id a.before || builtins.elem a.id b.after)
         (builtins.attrValues enabledHooks);
     in
-    builtins.map (value: value.raw) sortedHooks.result;
+    if sortedHooks ? result then
+      builtins.map (value: value.raw) sortedHooks.result
+    else
+      let
+        getIds = builtins.map (value: value.id);
+
+        prettyPrintCycle = opts: cycle:
+          lib.pipe cycle [
+            (builtins.map (hook:
+              lib.nameValuePair hook.id { before = hook.before; after = hook.after; }
+            ))
+            lib.listToAttrs
+            (lib.generators.toPretty opts)
+          ];
+      in
+      throw ''
+        The hooks can't be sorted because of a cycle in the dependency graph:
+
+          ${concatStringsSep " -> " (getIds sortedHooks.cycle)}
+
+          which leads to a loop back to: ${concatStringsSep ", " (getIds sortedHooks.loops)}
+
+        Try removing the conflicting hook ids from the `before` and `after` attributes of these hooks:
+
+          ${prettyPrintCycle { indent = "  "; } sortedHooks.cycle}
+      '';
 
   configFile =
     performAssertions (