feat(sync): add opt-in fast-forward of trunk branch after sync#686
feat(sync): add opt-in fast-forward of trunk branch after sync#686
Conversation
Add --ff-trunk flag and sync.fastForwardTrunk config option to fast-forward the local trunk branch to its remote tracking branch after syncing. This keeps the local trunk up-to-date without requiring a manual checkout and pull. When the local trunk has diverged from remote, a warning message is shown instead of silently skipping.
Current Aviator status
This pull request is currently open (not queued). How to mergeTo merge this PR, comment
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
🔃 FlexReview StatusCommon Owner:
Review SLO: |
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to fast-forward the local trunk branch to match its remote tracking branch during the sync process. It adds a new ff-trunk flag to the sync command, corresponding configuration settings, and a Bubbletea model to manage the git logic. A review comment suggests improving the error handling within the FastForwardTrunkModel to better distinguish between skipped branches, diverged branches, and internal command failures, ensuring more accurate feedback is displayed to the user.
| type FastForwardTrunkModel struct { | ||
| repo *git.Repo | ||
| onDone func() tea.Cmd | ||
|
|
||
| done bool | ||
| skipped bool | ||
| diverged bool | ||
| trunk string | ||
| } | ||
|
|
||
| type ffTrunkDone struct{} | ||
|
|
||
| func NewFastForwardTrunkModel( | ||
| repo *git.Repo, | ||
| onDone func() tea.Cmd, | ||
| ) *FastForwardTrunkModel { | ||
| return &FastForwardTrunkModel{ | ||
| repo: repo, | ||
| onDone: onDone, | ||
| } | ||
| } | ||
|
|
||
| func (m *FastForwardTrunkModel) Init() tea.Cmd { | ||
| return m.run | ||
| } | ||
|
|
||
| func (m *FastForwardTrunkModel) run() tea.Msg { | ||
| ctx := context.Background() | ||
| m.trunk = m.repo.DefaultBranch() | ||
| remote := m.repo.GetRemoteName() | ||
|
|
||
| // Check if the local trunk branch exists. | ||
| exists, err := m.repo.DoesBranchExist(ctx, m.trunk) | ||
| if err != nil || !exists { | ||
| m.skipped = true | ||
| return ffTrunkDone{} | ||
| } | ||
|
|
||
| // Check if the remote tracking branch exists. | ||
| remoteExists, err := m.repo.DoesRemoteBranchExist(ctx, m.trunk) | ||
| if err != nil || !remoteExists { | ||
| m.skipped = true | ||
| return ffTrunkDone{} | ||
| } | ||
|
|
||
| // Check if we are currently on the trunk branch. If so, we need to use | ||
| // "git merge --ff-only" directly. Otherwise, we can use update-ref to | ||
| // update the local ref without checking it out. | ||
| currentBranch, err := m.repo.CurrentBranchName() | ||
| if err != nil { | ||
| // Detached HEAD or other issue; just try the update-ref approach. | ||
| currentBranch = "" | ||
| } | ||
|
|
||
| remoteRef := fmt.Sprintf("%s/%s", remote, m.trunk) | ||
|
|
||
| if currentBranch == m.trunk { | ||
| // We're on the trunk branch, use merge --ff-only. | ||
| _, err := m.repo.Run(ctx, &git.RunOpts{ | ||
| Args: []string{"merge", "--ff-only", remoteRef}, | ||
| ExitError: true, | ||
| }) | ||
| if err != nil { | ||
| m.diverged = true | ||
| return ffTrunkDone{} | ||
| } | ||
| } else { | ||
| // Not on the trunk branch. Verify that the remote is a fast-forward | ||
| // of the local branch using merge-base --is-ancestor, then update the | ||
| // ref. | ||
| _, err := m.repo.Run(ctx, &git.RunOpts{ | ||
| Args: []string{"merge-base", "--is-ancestor", fmt.Sprintf("refs/heads/%s", m.trunk), fmt.Sprintf("refs/remotes/%s/%s", remote, m.trunk)}, | ||
| ExitError: true, | ||
| }) | ||
| if err != nil { | ||
| m.diverged = true | ||
| return ffTrunkDone{} | ||
| } | ||
| _, err = m.repo.Run(ctx, &git.RunOpts{ | ||
| Args: []string{"update-ref", fmt.Sprintf("refs/heads/%s", m.trunk), fmt.Sprintf("refs/remotes/%s/%s", remote, m.trunk)}, | ||
| ExitError: true, | ||
| }) | ||
| if err != nil { | ||
| m.diverged = true | ||
| return ffTrunkDone{} | ||
| } | ||
| } | ||
|
|
||
| m.done = true | ||
| return ffTrunkDone{} | ||
| } | ||
|
|
||
| func (m *FastForwardTrunkModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { | ||
| switch msg.(type) { | ||
| case ffTrunkDone: | ||
| return m, m.onDone() | ||
| } | ||
| return m, nil | ||
| } | ||
|
|
||
| func (m *FastForwardTrunkModel) View() string { | ||
| if m.skipped { | ||
| return "" | ||
| } | ||
| if m.diverged { | ||
| return colors.ProgressStyle.Render(fmt.Sprintf(" Could not fast-forward %s (local branch has diverged from remote)", m.trunk)) + "\n" | ||
| } | ||
| if m.done { | ||
| return colors.SuccessStyle.Render(fmt.Sprintf("✓ Fast-forwarded %s to match remote", m.trunk)) + "\n" | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
The error handling in this model can be improved to provide more accurate feedback to the user. Currently:
- Errors from
DoesBranchExistandDoesRemoteBranchExistare silently treated as a 'skipped' case. This can hide underlying problems (e.g., file permission issues) from the user. - Any error from the
git update-refcommand is categorized as a 'diverged' branch. However,update-refcan fail for reasons other than divergence (e.g., the ref is locked). This can lead to misleading error messages.
I suggest introducing a new state to handle unexpected/internal errors, and updating the logic to distinguish between 'skipped', 'diverged', and 'error' states correctly. This will make the UI more robust and less confusing.
type FastForwardTrunkModel struct {
repo *git.Repo
onDone func() tea.Cmd
done bool
skipped bool
diverged bool
internalError error
trunk string
}
type ffTrunkDone struct{}
func NewFastForwardTrunkModel(
repo *git.Repo,
onDone func() tea.Cmd,
) *FastForwardTrunkModel {
return &FastForwardTrunkModel{
repo: repo,
onDone: onDone,
}
}
func (m *FastForwardTrunkModel) Init() tea.Cmd {
return m.run
}
func (m *FastForwardTrunkModel) run() tea.Msg {
ctx := context.Background()
m.trunk = m.repo.DefaultBranch()
remote := m.repo.GetRemoteName()
// Check if the local trunk branch exists.
exists, err := m.repo.DoesBranchExist(ctx, m.trunk)
if err != nil {
m.internalError = err
return ffTrunkDone{}
}
if !exists {
m.skipped = true
return ffTrunkDone{}
}
// Check if the remote tracking branch exists.
remoteExists, err := m.repo.DoesRemoteBranchExist(ctx, m.trunk)
if err != nil {
m.internalError = err
return ffTrunkDone{}
}
if !remoteExists {
m.skipped = true
return ffTrunkDone{}
}
// Check if we are currently on the trunk branch. If so, we need to use
// "git merge --ff-only" directly. Otherwise, we can use update-ref to
// update the local ref without checking it out.
currentBranch, err := m.repo.CurrentBranchName()
if err != nil {
// Detached HEAD or other issue; just try the update-ref approach.
currentBranch = ""
}
remoteRef := fmt.Sprintf("%s/%s", remote, m.trunk)
if currentBranch == m.trunk {
// We're on the trunk branch, use merge --ff-only.
_, err := m.repo.Run(ctx, &git.RunOpts{
Args: []string{"merge", "--ff-only", remoteRef},
ExitError: true,
})
if err != nil {
m.diverged = true
return ffTrunkDone{}
}
} else {
// Not on the trunk branch. Verify that the remote is a fast-forward
// of the local branch using merge-base --is-ancestor, then update the
// ref.
_, err := m.repo.Run(ctx, &git.RunOpts{
Args: []string{"merge-base", "--is-ancestor", fmt.Sprintf("refs/heads/%s", m.trunk), fmt.Sprintf("refs/remotes/%s/%s", remote, m.trunk)},
ExitError: true,
})
if err != nil {
m.diverged = true
return ffTrunkDone{}
}
_, err = m.repo.Run(ctx, &git.RunOpts{
Args: []string{"update-ref", fmt.Sprintf("refs/heads/%s", m.trunk), fmt.Sprintf("refs/remotes/%s/%s", remote, m.trunk)},
ExitError: true,
})
if err != nil {
m.internalError = err
return ffTrunkDone{}
}
}
m.done = true
return ffTrunkDone{}
}
func (m *FastForwardTrunkModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
switch msg.(type) {
case ffTrunkDone:
return m, m.onDone()
}
return m, nil
}
func (m *FastForwardTrunkModel) View() string {
if m.skipped {
return ""
}
if m.internalError != nil {
return colors.ProgressStyle.Render(fmt.Sprintf(" Error fast-forwarding %s: %v", m.trunk, m.internalError)) + "\n"
}
if m.diverged {
return colors.ProgressStyle.Render(fmt.Sprintf(" Could not fast-forward %s (local branch has diverged from remote)", m.trunk)) + "\n"
}
if m.done {
return colors.SuccessStyle.Render(fmt.Sprintf("✓ Fast-forwarded %s to match remote", m.trunk)) + "\n"
}
return ""
}
Add --ff-trunk flag and sync.fastForwardTrunk config option to
fast-forward the local trunk branch to its remote tracking branch
after syncing. This keeps the local trunk up-to-date without
requiring a manual checkout and pull.
When the local trunk has diverged from remote, a warning message
is shown instead of silently skipping.