[Feat] Implement worktree hook execution with real-time output#51
Conversation
Add Hook type, AddHook/RemoveHook methods, and runHooks engine that streams stdout/stderr in real time so users see progress during long-running hooks like `pnpm install`. Hooks run after shared resource sync on `wt add` but are skipped during `wt share sync` bulk operations.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience for worktree operations by introducing support for configurable hooks with real-time output. Previously, long-running hook commands could make the tool appear unresponsive, but now users receive immediate feedback as commands execute. This change improves transparency and usability, particularly for tasks like dependency installation, by providing visibility into the progress of background operations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'hooks' feature for worktrees, allowing users to define and execute commands as part of the shared resource synchronization process. It adds a Hook struct, integrates hooks into the SharedConfig, and provides AddHook, RemoveHook, and runHooks methods. The syncSharedResourcesToPath function is updated to conditionally run these hooks, and SyncAllSharedResources is modified to explicitly not run hooks. New tests cover invalid hook index rejection and confirm that SyncAllSharedResources does not execute hooks. Feedback suggests improving the readability of a complex conditional statement and refactoring duplicated hook label logic into a dedicated method for better maintainability.
| if len(cfg.Resources) == 0 && (!runHooks || len(cfg.Hooks) == 0) { | ||
| return report, nil | ||
| } |
There was a problem hiding this comment.
This conditional logic is a bit complex and can be hard to parse at a glance. To improve readability and make the intent clearer, consider refactoring it to use intermediate variables that describe the conditions.
| if len(cfg.Resources) == 0 && (!runHooks || len(cfg.Hooks) == 0) { | |
| return report, nil | |
| } | |
| shouldSyncResources := len(cfg.Resources) > 0 | |
| shouldRunHooks := runHooks && len(cfg.Hooks) > 0 | |
| if !shouldSyncResources && !shouldRunHooks { | |
| return report, nil | |
| } |
| desc := hook.Desc | ||
| if desc == "" { | ||
| desc = hook.Cmd | ||
| } |
There was a problem hiding this comment.
This logic to determine the hook's descriptive label (falling back from Desc to Cmd) is duplicated in RemoveHook (lines 330-333) and runHooks (lines 600-603). To improve maintainability and reduce code duplication, consider adding a method to the Hook struct to encapsulate this logic.
For example, you could add this method to the Hook struct:
func (h *Hook) Label() string {
if h.Desc != "" {
return h.Desc
}
return h.Cmd
}Then you can simplify this part to desc := hook.Label() and do the same in RemoveHook and runHooks.
1 participant
What's changed?
Hookstruct,SharedConfig.Hooksfield, andAddHook/RemoveHookmethods ininternal/worktree/resource.gorunHooksengine that streams stdout/stderr to terminal in real time (replacesCombinedOutputwhich buffered all output until completion)wt add; skipped duringwt share syncbulk operationsWhy
pnpm installcan take a long time; users had zero visibility into progress, making the tool appear frozen