hooks: Add support for running `jj fix` as a pre-upload hook.

[matts1]

We will support arbitrary hooks in the future, but for a few reasons we will special-case jj fix:

• Performance (jj fix is hyper-optimized for running on stacks of commits at the same time)
• Common (This will probably be the most common requested pre-upload check)
• Simplicity (this was much easier to implement, so is a good first hook)

Bug: #3577

Checklist

If applicable:

• [x] I have updated CHANGELOG.md
• [x] I have updated the documentation (README.md, docs/, demos/)
• [x] I have updated the config schema (cli/src/config-schema.json)
• [x] I have added/updated tests to cover my changes

[thoughtpolice]

We already have some "on push" behavior, namely signing. I had an old diff I never upstreamed that added a new flag called on-push-behavior = ["..."] to slightly generalize this; the cropped diff looks like:

diff --git a/cli/src/commands/git/push.rs b/cli/src/commands/git/push.rs
index 88403257ea..fee554e83b 100644
--- a/cli/src/commands/git/push.rs
+++ b/cli/src/commands/git/push.rs
@@ -393,7 +393,7 @@
         return Ok(());
     }

-    let sign_behavior = if tx.settings().get_bool("git.sign-on-push")? {
+    let sign_behavior = if should_sign_on_push(tx.settings())? {
         Some(SignBehavior::Own)
     } else {
         None
@@ -1016,3 +1016,15 @@
         .collect_vec();
     Ok(bookmarks_targeted)
 }
+
+fn should_sign_on_push(settings: &UserSettings) -> Result<bool, CommandError> {
+    // First check the new on-push-behavior configuration
+    if let Ok(behaviors) = settings.get::<Vec<String>>("git.on-push-behavior") {
+        return Ok(behaviors.contains(&"sign".to_string()));
+    } else if let Some(behavior) = settings.get_string("git.on-push-behavior").optional()? {
+        return Ok(behavior == "sign");
+    }
+
+    // Fall back to the old sign-on-push configuration for backward compatibility
+    settings.get_bool("git.sign-on-push").map_err(Into::into)
+}
diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json
index e4ed1a4c81..da11add710 100644
--- a/cli/src/config-schema.json
+++ b/cli/src/config-schema.json
@@ -468,6 +468,22 @@
                     "description": "Whether jj should sign commits before pushing",
                     "default": false
                 },
+                "on-push-behavior": {
+                    "description": "List of behaviors to execute before pushing",
+                    "oneOf": [
+                        {
+                            "type": "string",
+                            "enum": ["sign"]
+                        },
+                        {
+                            "type": "array",
+                            "items": {
+                                "type": "string",
+                                "enum": ["sign"]
+                            }
+                        }
+                    ]
+                },
                 "track-default-bookmark-on-clone": {
                     "type": "boolean",
                     "description": "Whether `jj git clone` creates a local bookmark tracking the default remote bookmark",

I had then wanted to extend jj fix to also use this flag, instead. I think that would be a nicer configuration, all things considered. We could even make the entry a table and allow you to specify different behaviors depending on branch patterns, I suppose... WDYT?

FWIW, I'm not so sure about special casing it only on gerrit upload, that comes across as a bit strange. When would it be bad to run jj fix before a push? I don't think anyone has ever reported any issues about this coming up with the signing behavior (please correct me if I'm wrong), so I'm not sure that special casing it is really necessary, tbh.

[PhilipMetzger]

I had then wanted to extend jj fix to also use this flag, instead. I think that would be a nicer configuration, all things considered. We could even make the entry a table and allow you to specify different behaviors depending on branch patterns, I suppose... WDYT?

I agree but ideally we really shouldn't run untrusted programs but should use user-aliases instead. This will let users plug a bunch of with the model. And matching on bookmark patterns also makes sense.

[matts1]

We already have some "on push" behavior, namely signing. I had an old diff I never upstreamed that added a new flag called on-push-behavior = ["..."] to slightly generalize this; the cropped diff looks like:

I have a follow-up PR that adds support for generalized hooks rather than just supporting fix. I don't like having it as a list though, since that introduces several UX issues:

• To disable one specific hook is difficult
  • You need to set --config=on-push-behaviour = ["foo", "bar"] and know what the names of these behaviours are
• To add on to existing hooks is difficult
  • I set the config in jj config edit --user to ["fix", "lint"]
  • The repo sets the default to ["fix"]
  • The repo has now overridden my config

I believe that an approach such as what jj fix already has for their tools is a better approach.

My intent was to have specifically hooks.pre-upload-fix and hooks.pre-upload.foo for arbitrary tools. Maybe hooks.pre-upload.fix would be a better idea for consistency. I wanted to collect them under hooks because I intended to add more different types of hooks.

I agree but ideally we really shouldn't run untrusted programs but should use user-aliases instead. This will let users plug a bunch of with the model. And matching on bookmark patterns also makes sense.

This is one of the reasons I'm working on secure configuration and secure repo configuration. I intend to do something similar to fix tools in the future, which should make this secure.

[matts1]

@PhilipMetzger I wrote a design doc for hooks today

[benbrittain]

I know it's very standard for Google to do design reviews in google docs and I'm personally pretty comfy with it, but I think if something is being proposed to JJ at large it needs to be written up similar to the other proposal. I don't know if there is any policy here or whatnot, but I think it's far less discoverable and forces folks to tie their identity to comments in a way that doesn't always work in OSS.

[matts1]

I've been writing design docs in google docs, since I just find that it's much better as a collaboration tool, with the intent of moving them to markdown once it gets approval.

I hadn't really considered your points, they seem pretty reasonable though. For future design docs I guess I'll do them in markdown.