Support prepare-commit-msg hooks - #2795
Conversation
Run prepare-commit-msg when GitUp prepares the commit message editor so hook output appears before the user submits the commit. Keep commit-msg validation at commit time using Git's standard COMMIT_EDITMSG file, and preserve user edits if the async prepare hook finishes after typing begins.
|
@DCVortexxx Thank you for the candor in using AI. It gives me the appropriate expectation. I assume you've tested locally? It sounds like a good addition and I will look at the changes soon. This Thursday I'll likely be releasing the 1.5.0 release of GitUp to the stable channel so it won't go in before that. Probably next week is a realistic time for me to start testing out this change. |
|
Hey @lucasderraugh, thanks for getting back to me. Yeah, I tested it locally, through all paths that came to mind :
I also have validated that the implementation is reasonable, according to my knowledge of the codebase of course. Let me know if I can help in any way, and of course, no worries about the timing, |
|
Wonderful, thank you! |
|
@greptile Can you review this PR? |
|
| Filename | Overview |
|---|---|
| GitUpKit/Views/GICommitViewController.m | Adds async prepare-commit-msg hook execution with generation-based cancellation and COMMIT_EDITMSG-based commit-msg hook path; two issues around double error presentation and a sticky unrecoverable error state block users from committing after a hook failure. |
Sequence Diagram
sequenceDiagram
participant UI as GICommitViewController (main)
participant BG as Background Queue
participant FS as COMMIT_EDITMSG
UI->>UI: viewWillAppear (message empty)
UI->>UI: ++generation
UI->>BG: dispatch_async (global queue)
BG->>FS: writeToFile: initialMessage
BG->>BG: runHookWithName: prepare-commit-msg
BG->>FS: hook may modify COMMIT_EDITMSG
BG->>FS: read back COMMIT_EDITMSG
BG->>UI: dispatch_async (main queue)
alt generation matches
alt hook succeeded
UI->>UI: _applyPreparedCommitMessage:
else hook failed
UI->>UI: presentError (first display)
end
else generation mismatch
UI->>UI: discard result
end
UI->>UI: user clicks Commit
alt hook still running
UI->>UI: alert: still running
else hook error stored
UI->>UI: presentError (duplicate display)
else
UI->>UI: pre-commit hook
UI->>FS: write message for commit-msg
UI->>UI: commit-msg hook
UI->>FS: read back message
UI->>UI: performCommitCreation
UI->>UI: post-commit hook
UI->>UI: didCreateCommit:
end
Reviews (1): Last reviewed commit: "Support prepare-commit-msg hooks" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| dispatch_async(dispatch_get_main_queue(), ^{ | ||
| if (generation != _prepareCommitMessageHookGeneration) { | ||
| return; | ||
| } | ||
| _prepareCommitMessageHookRunning = NO; | ||
| _prepareCommitMessageHookError = error; | ||
| if (preparedMessage) { | ||
| [self _applyPreparedCommitMessage:preparedMessage replacingInitialMessage:initialMessage]; | ||
| } else if (error) { | ||
| [self presentError:error]; | ||
| } |
There was a problem hiding this comment.
When prepare-commit-msg fails, [self presentError:error] is called here in the async completion block, and _prepareCommitMessageHookError is also stored. If the user dismisses that alert and then clicks Commit, createCommitFromHEADWithMessage: checks _prepareCommitMessageHookError and calls [self presentError:_prepareCommitMessageHookError] a second time. The user sees the identical error dialog twice with no indication of how to recover other than navigating away and back.
Consider showing the error only in one place: either eagerly (here, and not re-checking in createCommitFromHEADWithMessage:), or lazily (only in createCommitFromHEADWithMessage:, silently storing the failure). The current code does both.
| if (_prepareCommitMessageHookRunning) { | ||
| [self presentAlertWithType:kGIAlertType_Caution title:NSLocalizedString(@"The prepare-commit-msg hook is still running", nil) message:NSLocalizedString(@"Please wait for it to finish before committing.", nil)]; | ||
| return; | ||
| } | ||
| if (_prepareCommitMessageHookError) { | ||
| [self presentError:_prepareCommitMessageHookError]; | ||
| return; | ||
| } |
There was a problem hiding this comment.
_prepareCommitMessageHookError never cleared on failed commit attempt
After a hook failure the user is blocked from committing, but createCommitFromHEADWithMessage: shows the error and returns without clearing _prepareCommitMessageHookError. Every subsequent commit attempt will show the same error and abort. The only implicit recovery paths are toggling Amend (the else branch calls _cancelPrepareCommitMessageHook) or navigating away (which calls viewDidDisappear). Neither path is surfaced to the user in the error message, so they are left with no visible way forward.
At minimum, the error should be cleared after being presented here, or the alert should include a recovery suggestion (e.g., a "Retry" action that re-runs the hook).
| dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ | ||
| NSError* error = nil; | ||
| NSString* preparedMessage = nil; | ||
| GCRepository* repository = [[GCRepository alloc] initWithExistingLocalRepository:repositoryPath error:&error]; | ||
| if (repository && [initialMessage writeToFile:messagePath atomically:YES encoding:NSUTF8StringEncoding error:&error] && [repository runHookWithName:@"prepare-commit-msg" arguments:arguments standardInput:nil error:&error]) { | ||
| preparedMessage = [[NSString alloc] initWithContentsOfFile:messagePath encoding:NSUTF8StringEncoding error:&error]; | ||
| } | ||
|
|
||
| dispatch_async(dispatch_get_main_queue(), ^{ | ||
| if (generation != _prepareCommitMessageHookGeneration) { | ||
| return; | ||
| } | ||
| _prepareCommitMessageHookRunning = NO; | ||
| _prepareCommitMessageHookError = error; | ||
| if (preparedMessage) { | ||
| [self _applyPreparedCommitMessage:preparedMessage replacingInitialMessage:initialMessage]; | ||
| } else if (error) { | ||
| [self presentError:error]; | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
self in nested dispatch blocks
The outer dispatch_async block accesses _prepareCommitMessageHookGeneration and _prepareCommitMessageHookRunning via implicit self->_ivar syntax, capturing self strongly. The inner dispatch_get_main_queue block does the same. This is safe against crashes (the generation check guards stale application), but it extends the view controller's lifetime until the background task finishes, even after the view has been dismissed and ownership released. The idiomatic ObjC pattern is to capture a __weak reference, then create a __strong local at the top of the inner block and bail early if it is nil.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| - (NSArray*)_prepareCommitMessageHookArgumentsWithMessagePath:(NSString*)path amendCommitSHA1:(NSString*)sha1 { | ||
| if (_amendButton.state) { | ||
| return sha1 ? @[ path, @"commit", sha1 ] : @[ path, @"commit" ]; | ||
| } | ||
| if (self.repository.state == kGCRepositoryState_Merge) { | ||
| return @[ path, @"merge" ]; | ||
| } | ||
| return @[ path ]; | ||
| } |
There was a problem hiding this comment.
squash and template source types for prepare-commit-msg
Git defines five source types for prepare-commit-msg: message (from -m/-F), template, merge, squash, and commit (amend). This implementation only handles merge, amend (commit), and the default (no source). Squash commits — which Git generates during interactive rebase — are a common use case for prepare-commit-msg hooks. If GitUp drives squash commits, those paths will invoke the hook with only the path argument instead of path squash, producing incorrect hook behavior for hooks that branch on the source type.
2 participants
Disclosure
I AGREE TO THE GITUP CONTRIBUTOR LICENSE AGREEMENT.
Full disclosure, this has been drafted by AI because I haven't played with ObjC for yeeeeeaaaaars, but I believe that it respects the coding standard and contribution rules.
Of course, I'm open to suggestions and will happily update it to make it into master.
I LOVE GitUp and having to rely on another editor just because a project I work on needs this hook on it is a pain 😅
Summary
This adds support for Git's
prepare-commit-msghook when creating commits from GitUp.Git documents
prepare-commit-msgas running after the default commit message is prepared and before the editor starts.To match that lifecycle, GitUp now runs the hook when the commit view prepares the message editor in
viewWillAppear:prepare-commit-msghook should be allowed to finish.It also runs when the user toggles Amend:
prepare-commit-msgwith Git's amend-stylecommitsourceprepare-commit-msgrun, GitUp replaces it with the amend message flow.This also has a side effect on the
commit-msghook, because it used to rely on a app-owned, temporary file for writing the commit message, instead the the$GIT_DIR/COMMIT_EDITMSGfile Git uses.Since
prepare-commit-msgrelies on this file,commit-msgshould read it as well, as the single source of truth for commit message edition.Note
My first implementation ran it when the user actually commits, right before
commit-msg, and it made the code significantly simpler, since we don't need to handle in-flight edit, and "just" update the message in the same waycommit-msgdoes.However, I changed it to be closer to Git's semantics.
Feel free to let me know if you feel like this is a tradeoff worth having.