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:

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

[PhilipMetzger]

Minor stuff, I also don't really agree with how the code currently was rewritten but I'm unable to point at it.

cc @hooper

[PhilipMetzger]

nit: this is missing that it only runs for gerrit upload which isn't such a common workflow for most people.

[PhilipMetzger]

nit: In the commit description you could mention that "you" (i.e the chromium project) scripted around that which is imo a good motivation.

[PhilipMetzger]

I don't think we should advertise this as "general hook" support. Since it is the only support we'll ever have

[PhilipMetzger]

nit: this is also missing the major caveat of it being gerrit upload only

[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.

[scott2000]

I don't think it's necessary to use a revset for this. Maybe a better way would be to use self.repo().store().get_commit(commit_id) directly? That's what evaluate_to_commits() uses internally anyway.