fix sync --all failing when deleted branch exists in tree#685
Conversation
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 fix to prevent av sync --all from failing when a branch tracked in metadata has been deleted from Git. It includes an E2E test and updates GetTargetBranches to verify branch existence. The review feedback points out that errors from repo.DoesRefExist are ignored, which could lead to silent failures; handling these errors is recommended to ensure the tool behaves correctly during Git communication issues.
| ref := plumbing.NewBranchReferenceName(br.Name) | ||
| if exists, _ := repo.DoesRefExist(ctx, ref.String()); exists { | ||
| ret = append(ret, ref) | ||
| } |
There was a problem hiding this comment.
The error returned by repo.DoesRefExist is being ignored. If DoesRefExist encounters an error (e.g., a problem communicating with Git), exists will be false, and the branch will be silently skipped. This can lead to incorrect behavior where branches are skipped due to an error rather than actual deletion. Errors should be handled explicitly, either by propagating them or logging them appropriately, to prevent silent failures.
ref := plumbing.NewBranchReferenceName(br.Name)
if exists, err := repo.DoesRefExist(ctx, ref.String()); err != nil {
return nil, errors.Wrapf(err, "failed to check existence of branch %q", br.Name)
} else if exists {
ret = append(ret, ref)
}| ref := plumbing.NewBranchReferenceName(n) | ||
| if exists, _ := repo.DoesRefExist(ctx, ref.String()); exists { | ||
| ret = append(ret, ref) | ||
| } |
There was a problem hiding this comment.
Similar to the previous comment, the error returned by repo.DoesRefExist is ignored here. This can lead to branches being incorrectly skipped if there's an underlying issue with Git communication, rather than the branch truly not existing. It's important to handle errors explicitly to ensure the correct behavior of the sync --all operation.
ref := plumbing.NewBranchReferenceName(n)
if exists, err := repo.DoesRefExist(ctx, ref.String()); err != nil {
return nil, errors.Wrapf(err, "failed to check existence of branch %q", n)
} else if exists {
ret = append(ret, ref)
}
WHY
av sync --all --rebase-to-trunkcrashes witherror: git merge-base: exit status 128when a branch has been deleted from git but not yet cleaned up withav tidy. This happens becauseGetTargetBranchesincludes all branches from av's metadata DB without checking if the git refs still exist.WHAT
added ref existence checks in
GetTargetBranchesforAllBranchesmode — branches whose git refs no longer exist are silently skipped instead of being passed to the sequencer where they'd causegit merge-baseto fail.TESTING
sync --all --rebase-to-trunksucceeds