Austinamoruso/gitresethead #40
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @austinamorusoyardstick, 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 introduces a new, user-initiated git reset --hard feature to the application. The primary goal is to provide users with a direct way to revert their local branch to match its remote origin counterpart, while prioritizing data safety through an automatic backup branch creation. The changes span backend logic for Git operations, updates to the application's UI and message handling, and the addition of a new keybinding for ease of use.
Highlights
- New Git Reset Functionality: Implemented a
ResetToOriginmethod in theGitWorktreeto executegit reset --hard origin/branch. This operation includes a crucial safety measure: creating and pushing a uniquely named backup branch before performing the reset, ensuring data recoverability. - Interactive User Flow: Integrated the
git resetfeature into the application's user interface. This includes displaying a confirmation modal that clearly shows the target branch before execution, and utilizing a dedicatedstartGitResetMsgmessage type to manage the asynchronous reset process and provide user feedback. - Keybinding Integration: Introduced a new keybinding (
h) to trigger thegit reset --hardoperation. This involved updating key constant definitions, global key mappings, and help text configurations to ensure the new functionality is discoverable and accessible to users.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a git reset --hard feature, which is a powerful and potentially destructive operation. The implementation correctly includes safety measures like creating a backup branch. The code is well-structured and follows the existing patterns in the application.
My review includes two main suggestions for improvement:
- Simplifying and improving the performance of the backup branch name generation in
session/git/worktree_git.go. - Enhancing user feedback in
app/app.goby showing an immediate success message in the status bar after a successful reset, instead of only logging it.
These changes will make the new feature more robust, performant, and user-friendly.
app/app.go
Outdated
| // Show success | ||
| timestamp := time.Now().Format("15:04:05") | ||
| m.errorLog = append(m.errorLog, fmt.Sprintf("[%s] Git reset completed successfully for branch %s", timestamp, branchName)) | ||
|
|
||
| // Refresh instances after successful reset | ||
| return m, m.instanceChanged() |
There was a problem hiding this comment.
Currently, the success message for a git reset is only added to the errorLog, which the user has to open manually to see. For better user experience, it would be beneficial to display an immediate, temporary success message in the status bar (the errBox), similar to how other operations provide feedback. This makes the result of the action immediately obvious to the user.
// Show success message in the status bar
successMsg := fmt.Sprintf("✓ Git reset for branch %s completed successfully", branchName)
m.errBox.SetError(fmt.Errorf(successMsg))
// Also add to log for history
timestamp := time.Now().Format("15:04:05")
m.errorLog = append(m.errorLog, fmt.Sprintf("[%s] %s", timestamp, successMsg))
// Refresh instances and hide message after a delay
return m, tea.Batch(
m.instanceChanged(),
func() tea.Msg {
time.Sleep(3 * time.Second)
return hideErrMsg{}
},
)
session/git/worktree_git.go
Outdated
| timestamp := time.Now().Unix() | ||
| backupBranch := fmt.Sprintf("%s-backup-%d", g.branchName, timestamp) | ||
|
|
||
| // Ensure the backup branch name is unique by checking if it exists | ||
| for { | ||
| // Check if the branch already exists locally or remotely | ||
| localExists := false | ||
| remoteExists := false | ||
|
|
||
| if _, err := g.runGitCommand(g.worktreePath, "rev-parse", "--verify", backupBranch); err == nil { | ||
| localExists = true | ||
| } | ||
| if _, err := g.runGitCommand(g.worktreePath, "rev-parse", "--verify", fmt.Sprintf("origin/%s", backupBranch)); err == nil { | ||
| remoteExists = true | ||
| } | ||
|
|
||
| if !localExists && !remoteExists { | ||
| break | ||
| } | ||
|
|
||
| // If it exists, add a counter to make it unique | ||
| timestamp++ | ||
| backupBranch = fmt.Sprintf("%s-backup-%d", g.branchName, timestamp) | ||
| } |
There was a problem hiding this comment.
The current logic for generating a unique backup branch name by looping and executing git commands can be inefficient. A simpler and more performant approach is to use a more unique timestamp from the start, like time.Now().UnixNano(). This effectively eliminates the chance of a name collision for a user's actions and removes the need for the loop and multiple git command executions.
| timestamp := time.Now().Unix() | |
| backupBranch := fmt.Sprintf("%s-backup-%d", g.branchName, timestamp) | |
| // Ensure the backup branch name is unique by checking if it exists | |
| for { | |
| // Check if the branch already exists locally or remotely | |
| localExists := false | |
| remoteExists := false | |
| if _, err := g.runGitCommand(g.worktreePath, "rev-parse", "--verify", backupBranch); err == nil { | |
| localExists = true | |
| } | |
| if _, err := g.runGitCommand(g.worktreePath, "rev-parse", "--verify", fmt.Sprintf("origin/%s", backupBranch)); err == nil { | |
| remoteExists = true | |
| } | |
| if !localExists && !remoteExists { | |
| break | |
| } | |
| // If it exists, add a counter to make it unique | |
| timestamp++ | |
| backupBranch = fmt.Sprintf("%s-backup-%d", g.branchName, timestamp) | |
| } | |
| // First, create a backup branch with a unique name. | |
| // Using UnixNano is sufficient to avoid collisions and is more efficient than checking for existence. | |
| backupBranch := fmt.Sprintf("%s-backup-%d", g.branchName, time.Now().UnixNano()) |
There was a problem hiding this comment.
Code Review
This pull request introduces a git reset --hard feature, which is a valuable addition for developers. The implementation is well-structured and follows existing patterns in the codebase, including creating a backup branch for safety before performing the destructive operation. The UI and keybinding changes are correctly implemented.
I've identified a couple of areas for improvement. In session/git/worktree_git.go, the loop for generating a unique backup branch name could be made more robust and efficient. In app/app.go, a success message is logged to a field named errorLog, which could be confusing for future maintenance. These are not critical issues but are worth considering for improving code quality.
app/app.go
Outdated
|
|
||
| // Show success | ||
| timestamp := time.Now().Format("15:04:05") | ||
| m.errorLog = append(m.errorLog, fmt.Sprintf("[%s] Git reset completed successfully for branch %s", timestamp, branchName)) |
There was a problem hiding this comment.
A success message is being appended to m.errorLog. While this seems to be a pattern in the app for displaying notifications, the field name is misleading. This can make debugging and maintenance harder, as one would expect only errors to be in a variable named errorLog.
While it's fine to keep it consistent with the existing pattern for this PR, consider renaming errorLog to something more general like notifications or statusMessages in a future refactoring to improve clarity.
session/git/worktree_git.go
Outdated
| for { | ||
| // Check if the branch already exists locally or remotely | ||
| localExists := false | ||
| remoteExists := false | ||
|
|
||
| if _, err := g.runGitCommand(g.worktreePath, "rev-parse", "--verify", backupBranch); err == nil { | ||
| localExists = true | ||
| } | ||
| if _, err := g.runGitCommand(g.worktreePath, "rev-parse", "--verify", fmt.Sprintf("origin/%s", backupBranch)); err == nil { | ||
| remoteExists = true | ||
| } | ||
|
|
||
| if !localExists && !remoteExists { | ||
| break | ||
| } | ||
|
|
||
| // If it exists, add a counter to make it unique | ||
| timestamp++ | ||
| backupBranch = fmt.Sprintf("%s-backup-%d", g.branchName, timestamp) | ||
| } |
There was a problem hiding this comment.
This loop to find a unique backup branch name has a couple of areas for improvement:
- Unbounded Loop: An unbounded
for {}loop can be risky. It's good practice to add a limit to the number of attempts (e.g., 100) to prevent potential infinite loops in unexpected edge cases. - Collision Resolution: Relying on incrementing a Unix timestamp to resolve collisions is a bit unusual. A more conventional and clearer approach would be to append a simple counter to a base branch name (e.g.,
...-backup-12345-1,...-backup-12345-2). - Inefficiency: The loop executes two
gitcommands per iteration, which could be slow if there are many existing backup branches.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a git reset --hard feature, which is a valuable addition. The implementation is well-integrated with the existing application structure, including a confirmation dialog and keybindings.
My review focuses on two main points:
- Preventing an unhandled error by adding a check for paused instances before attempting the reset.
- Improving the backup strategy by suggesting local-only backup branches to avoid cluttering the remote repository.
Overall, the changes are solid and with these adjustments, the feature will be more robust and user-friendly.
| if selected == nil { | ||
| return m, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
The git reset operation will fail on a paused instance because its worktree directory has been removed. This will result in a generic 'directory does not exist' error, which is not very user-friendly.
To improve the user experience and prevent this error, please add a check to ensure the instance is not paused before proceeding with the reset operation. You can see a similar check for the KeyPRReview action.
if selected.Paused() {
return m, m.handleError(fmt.Errorf(instancePausedError, selected.Title))
}
session/git/worktree_git.go
Outdated
| // Push the backup branch with --no-verify for speed | ||
| if _, err := g.runGitCommand(g.worktreePath, "push", "origin", backupBranch, "--no-verify"); err != nil { | ||
| // If push fails, just log it but continue | ||
| log.WarningLog.Printf("failed to push backup branch %s: %v", backupBranch, err) | ||
| } |
There was a problem hiding this comment.
Pushing a backup branch to the remote for every reset operation can quickly clutter the remote repository with many ...-backup-12345 branches that are unlikely to be cleaned up automatically.
A local-only backup branch provides a strong safety net for the user without affecting the shared remote repository. The user is already informed about the backup branch creation in the log messages, and they can manually push it if recovery from another machine is needed.
Consider removing the push operation to keep the backup local.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a git reset --hard feature, complete with UI, backend logic, and keybindings. The implementation is well-structured, particularly the safety feature of creating a backup branch before a destructive operation.
My review focuses on improving maintainability and robustness:
- I've identified significant code duplication in the git backup logic and suggested refactoring it into a shared helper function.
- There's an opportunity to make error handling more precise in one of the new git helper functions.
- I've also pointed out some semantic confusion in the UI layer where error-handling components are used for success messages, suggesting a refactoring for better clarity.
Overall, these are solid changes that add valuable functionality.
session/git/worktree_git.go
Outdated
| // Get current commit hash to check if we need a backup | ||
| currentCommit, err := g.runGitCommand(g.worktreePath, "rev-parse", "HEAD") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get current commit: %w", err) | ||
| } | ||
| currentCommit = strings.TrimSpace(currentCommit) | ||
|
|
||
| // Check if the current commit is already backed up on a remote branch | ||
| isBackedUp, existingBackup, err := g.isCommitBackedUp(currentCommit) | ||
| if err != nil { | ||
| log.WarningLog.Printf("Failed to check for existing backups: %v", err) | ||
| // Continue with creating a new backup | ||
| isBackedUp = false | ||
| } | ||
|
|
||
| var backupBranch string | ||
| if isBackedUp { | ||
| // Use the existing backup branch name | ||
| backupBranch = existingBackup | ||
| log.InfoLog.Printf("Current commit already backed up in branch: %s", backupBranch) | ||
| } else { | ||
| // Create a new backup branch with a unique name | ||
| backupBranch = fmt.Sprintf("%s-backup-%d", g.branchName, time.Now().UnixNano()) | ||
|
|
||
| // Create backup branch from current state | ||
| if _, err := g.runGitCommand(g.worktreePath, "branch", backupBranch); err != nil { | ||
| return fmt.Errorf("failed to create backup branch: %w", err) | ||
| } | ||
|
|
||
| // Push the backup branch with --no-verify for speed | ||
| if _, err := g.runGitCommand(g.worktreePath, "push", "origin", backupBranch, "--no-verify"); err != nil { | ||
| // If push fails, just log it but continue | ||
| log.WarningLog.Printf("failed to push backup branch %s: %v", backupBranch, err) | ||
| } else { | ||
| log.InfoLog.Printf("Created and pushed new backup branch: %s", backupBranch) | ||
| } | ||
| } |
There was a problem hiding this comment.
There is significant code duplication between this function (ResetToOrigin) and RebaseWithMain. The entire logic for checking for and creating a backup branch is repeated.
To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, this logic should be extracted into a private helper function.
For example, you could create a function like ensureBackupBranch:
// ensureBackupBranch checks if the current commit is backed up, creates a backup branch if not,
// and returns the name of the backup branch.
func (g *GitWorktree) ensureBackupBranch() (backupBranchName string, isNew bool, err error) {
currentCommit, err := g.runGitCommand(g.worktreePath, "rev-parse", "HEAD")
if err != nil {
return "", false, fmt.Errorf("failed to get current commit: %w", err)
}
currentCommit = strings.TrimSpace(currentCommit)
isBackedUp, existingBackup, err := g.isCommitBackedUp(currentCommit)
if err != nil {
// Log as a warning and proceed to create a new backup.
log.WarningLog.Printf("Failed to check for existing backups: %v", err)
isBackedUp = false
}
if isBackedUp {
log.InfoLog.Printf("Current commit already backed up in branch: %s", existingBackup)
return existingBackup, false, nil
}
// Create a new backup branch.
backupBranch := fmt.Sprintf("%s-backup-%d", g.branchName, time.Now().UnixNano())
if _, err := g.runGitCommand(g.worktreePath, "branch", backupBranch); err != nil {
return "", true, fmt.Errorf("failed to create backup branch: %w", err)
}
// Push the backup branch.
if _, err := g.runGitCommand(g.worktreePath, "push", "origin", backupBranch, "--no-verify"); err != nil {
// If push fails, just log it but continue as the local backup exists.
log.WarningLog.Printf("failed to push backup branch %s: %v", backupBranch, err)
} else {
log.InfoLog.Printf("Created and pushed new backup branch: %s", backupBranch)
}
return backupBranch, true, nil
}Then, both ResetToOrigin and RebaseWithMain can be simplified by calling this helper.
| successMsg := fmt.Sprintf("✓ Git reset for branch %s completed successfully", branchName) | ||
| m.errBox.SetError(fmt.Errorf(successMsg)) | ||
|
|
||
| // Also add to log for history | ||
| timestamp := time.Now().Format("15:04:05") | ||
| m.errorLog = append(m.errorLog, fmt.Sprintf("[%s] %s", timestamp, successMsg)) |
There was a problem hiding this comment.
Using m.errBox.SetError() and m.errorLog for success messages is semantically confusing and can make debugging harder, as one would expect these to only handle actual errors.
This pattern appears elsewhere in the codebase as well. For better code clarity and maintainability, consider the following refactoring:
- Rename
errBoxto something more generic likestatusBoxandSetErrortoSetMessage. - Rename
errorLogtoactivityLogorstatusLogto reflect that it contains more than just errors.
While this is a larger change, it would make the code's intent clearer. At a minimum, you could consider creating a new component for status messages to separate them from errors.
| if err != nil { | ||
| // If the command fails, it might be because the commit doesn't exist | ||
| // In this case, we should create a backup | ||
| return false, "", nil |
There was a problem hiding this comment.
This function currently swallows any error from runGitCommand, assuming it's because the commit doesn't exist. This could hide other problems, such as issues with the git executable or a corrupted repository.
It's better to inspect the error to confirm the cause before ignoring it. You should only ignore the specific error that indicates an unknown commit and propagate other unexpected errors.
| if err != nil { | |
| // If the command fails, it might be because the commit doesn't exist | |
| // In this case, we should create a backup | |
| return false, "", nil | |
| if err != nil { | |
| // If the command fails because the commit doesn't exist, it's not an error for our purposes. | |
| // We just need to create a new backup. For other errors, we should propagate them. | |
| if strings.Contains(err.Error(), "no such commit") || strings.Contains(err.Error(), "malformed object name") { | |
| return false, "", nil | |
| } | |
| return false, "", err | |
| } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature to perform a git reset --hard on a selected branch, including UI and backend changes. The implementation includes a backup mechanism that avoids redundant backups. The UI and application flow are updated to support this new action, including a confirmation prompt. The changes are well-structured and the backup logic is a significant improvement. I suggest improving code clarity by separating success/status messaging from error handling mechanisms, and enhancing logging for better maintainability.
| successMsg := fmt.Sprintf("✓ Git reset for branch %s completed successfully", branchName) | ||
| m.errBox.SetError(fmt.Errorf(successMsg)) | ||
|
|
||
| // Also add to log for history | ||
| timestamp := time.Now().Format("15:04:05") | ||
| m.errorLog = append(m.errorLog, fmt.Sprintf("[%s] %s", timestamp, successMsg)) |
There was a problem hiding this comment.
Using m.errBox.SetError and appending to m.errorLog for success messages can be confusing, as the names imply these are for errors only. This makes the code's intent less clear and can complicate debugging from logs.
For better maintainability, consider introducing a separate UI component for status/success messages, distinct from error handling. A potential refactor could be renaming errBox to a more generic statusBox and errorLog to something like activityLog or eventLog to accurately reflect that it logs various events, not just errors. While this is likely a larger refactoring, it's a valuable improvement for future code clarity.
| if _, err := g.runGitCommand(g.worktreePath, "push", "origin", backupBranch, "--no-verify"); err != nil { | ||
| // If push fails, just log it but continue | ||
| // If push fails, just log it but continue as the local backup exists. | ||
| log.WarningLog.Printf("failed to push backup branch %s: %v", backupBranch, err) |
There was a problem hiding this comment.
Consider adding a more descriptive log message or using a structured logging approach to provide more context when the push fails. This would aid in debugging and monitoring the backup process.
1 participant
This pull request introduces a new feature to perform a
git reset --hardoperation on a selected branch, along with necessary UI and backend changes. The most important changes include adding a new method to handle the reset operation, updating the keybindings to trigger the reset, and integrating the feature into the application flow with confirmation prompts and error handling.Backend changes for git reset functionality:
ResetToOrigininGitWorktreeto performgit reset --hard origin/branchwith a backup branch created for safety (session/git/worktree_git.go, session/git/worktree_git.goR264-R316).homestruct to include apendingResetInstancefield for tracking the instance to reset (app/app.go, app/app.goR146-R148).startGitResetMsgto trigger the reset operation after user confirmation (app/app.go, app/app.goR1650-R1652).UI and application flow updates:
Updatemethod to handle thestartGitResetMsg, execute the reset operation, and log success or errors (app/app.go, app/app.goR490-R519).handleKeyPressmethod to show a confirmation modal when the reset key is pressed and store the selected instance for reset (app/app.go, app/app.goR1305-R1328).Keybinding updates:
hfor triggering thegit reset --hardoperation, with corresponding help text and configuration updates (keys/keys.go, [1] [2] [3] [4] [5] [6].