aviator-co / av

A command line tool to manage stacked PRs with Aviator
https://aviator.co
MIT License
149 stars 12 forks source link

Improve the stack branch command to handle trunk branches as parents #344

Closed draftcode closed 2 months ago

draftcode commented 2 months ago

When creating a new branch from a trunk branch, the stack branch command is better to start off from the remote tracking branch instead of the local branch. This way, the new branch will be created from the latest state of the trunk branch.

While we are here, accept origin/HEAD as the parent branch name and resolve it to the default branch.

Previously, this command automatically "adopts" the parent branch to av automatically, assuming that the parent branch's parent is the default branch, which might not be true. Instead, this command checks if the parent branch is already adopted or not.

aviator-app[bot] commented 2 months ago

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes. Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged using Aviator.

Stack

  1. 👉 Improve the stack branch command to handle trunk branches as parents #344 👈 (this pr)

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.
aviator-app[bot] commented 2 months ago

FlexReview Summary

Based on the code complexity and the author's expertise score, these are the suggested reviewers:

See the list of alternate reviewers in the detailed breakdown below.

Detailed Breakdown Author’s expertise score for the modified files: - `cmd/av/stack_branch.go` (1.00) - `e2e_tests/stack_orphan_test.go` (1.00) - `e2e_tests/stack_restack_test.go` (1.00) - `e2e_tests/stack_sync_amend_test.go` (1.00) - `e2e_tests/stack_sync_delete_merged_test.go` (1.00) - `e2e_tests/stack_sync_delete_parent_test.go` (1.00) - `e2e_tests/stack_sync_merge_commit_test.go` (1.00) - `e2e_tests/stack_sync_merged_parent_test.go` (1.00) - `e2e_tests/stack_sync_test.go` (1.00) - `e2e_tests/stack_sync_trunk_test.go` (1.00) | Files | Reviewers | | --- | --- | | `cmd/av/stack_branch.go` †,
`e2e_tests/stack_orphan_test.go` †,
`e2e_tests/stack_restack_test.go` †,
`e2e_tests/stack_sync_amend_test.go` †,
`e2e_tests/stack_sync_delete_merged_test.go` †,
`e2e_tests/stack_sync_delete_parent_test.go` †,
`e2e_tests/stack_sync_merge_commit_test.go` †,
`e2e_tests/stack_sync_merged_parent_test.go` †,
`e2e_tests/stack_sync_test.go` †,
`e2e_tests/stack_sync_trunk_test.go` † | **`@jainankit`** (score: 0.86, current review load: 0) | † Indicates that the file doesn't need an expert review. [(?)](https://docs.aviator.co/flexreview/concepts/approval-requirements) See full breakdown of the reviewers on the Aviator webapp.
jainankit commented 2 months ago

A few questions (user experience related):

draftcode commented 2 months ago

A few questions (user experience related):

  • With this new functionality, would we internally do a git fetch for that remote ref in case our local trunk branch is not up to date?

Answered offline, but this is using the remote tracking branches, which is a local ref. It doesn't do any git-fetch.

  • A separate minor UX note that now the behavior is slightly different for trunk branch vs feature branches. We should show this as a message to the user when running av stack branch on trunk.

This is consistent with av-stack-sync behavior.

  • If we report an error when parent is not adopted, we should recommend user with the command to adopt the entire stack. There could be scenario where the user wants to push multiple branches together. Or there could be a param within av stack branch to support that behavior.

I will update the error message.