Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add av stack split <new branch> to split last commit into a new branch #472

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Chafid
Copy link

@Chafid Chafid commented Nov 29, 2024

Adding new feature to split the last commit into a new branch and revert the old branch to 1 previous commit

@Chafid Chafid requested a review from a team as a code owner November 29, 2024 09:03
Copy link
Contributor

aviator-app bot commented Nov 29, 2024

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 pull request is currently open (not queued).

How to merge

To merge this PR, comment /aviator merge or add the mergequeue label.


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.

Copy link
Contributor

aviator-app bot commented Nov 29, 2024

🔃 FlexReview Status

Warning

  • This PR modifies 203 lines, which is larger than the Review SLO threshold.

This might slow down the review process. If possible, split the PR into smaller ones

Owner: aviator-co/engineering (expertise assignment)
Review SLO: 7 business hours if PR size is <= 200 LOC for the first response.

@aviator-app aviator-app bot requested a review from tulioz November 29, 2024 09:04
@tulioz tulioz requested a review from draftcode December 2, 2024 18:42
cmd/av/stack_split.go Outdated Show resolved Hide resolved
@draftcode
Copy link
Contributor

There's another big change going on. Let me hold this until it's merged.

@Chafid
Copy link
Author

Chafid commented Dec 6, 2024

Noted @draftcode , thanks for the update

cmd/av/stack_split.go Outdated Show resolved Hide resolved
cmd/av/av Outdated Show resolved Hide resolved
cmd/av/stack_split.go Outdated Show resolved Hide resolved
tulioz added a commit that referenced this pull request Dec 7, 2024
@tulioz
Copy link
Contributor

tulioz commented Dec 7, 2024

Some more items:

  • after running av stack split branch-2 we stay on branch-1, we should checkout branch-2 instead so that the user is on the correct head branch (this also resolves staged file oddities in the current setup)
  • we should make sure we adopt the branch so that we have it stored in av.db
  • at least for now I believe we're only supporting this action on the tip, so we should ensure there are no child branches so users don't end up in unexpected states

- switch to new branch if split successful
@Chafid
Copy link
Author

Chafid commented Dec 7, 2024

hi @draftcode @tulioz I've updated the PR as per your comments. But I think there's a conflict because of the other branch that got committed in the PR. Should I fix the conflict or you guys should do it?

cmd/av/av Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the binary is still part of the PR, you may have to do some squashing to get rid of it

@tulioz
Copy link
Contributor

tulioz commented Dec 9, 2024

@Chafid thanks for adding support for no branch name submitted, it looks really good!

To answer your question from this weekend, if you could rebase and migrate the command to av branch --split <optional-name> then this will be almost ready to merge.

Three requests still:

  • Adding the new branch to the aviator database (essentially av adopt --parent original-branch) so that we are tracking it in av.db
  • Preventing someone from running this command when not on the tip, so that they don't unexpectedly end up in a state where their stack is branching
  • Remove the binary file from the PR

@Chafid
Copy link
Author

Chafid commented Dec 12, 2024

Hi @tulioz , I migrated the split command to branch, and I added av adopt command once the split is finished.
I also add additional check to make sure that the user is on the latest commit. If not the command will fail

Copy link
Contributor

@tulioz tulioz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for joining everything under the av branch command, and addressing the other feedback. Left one comment for code change, and then this following scenario still seems to not work as I'd expect.

av branch commit-1

echo "First commit" > commit-1.txt
git add commit-1.txt
git commit -m "commit 1"

echo "Second commit" > commit-2.txt
git add commit-2.txt
git commit -m "commit 2"
av branch --split

av switch commit-1

echo "Third commit" > commit-3.txt
git add commit-3.txt
git commit -m "commit 3"
av branch --split

av tree
image

Looks like we still get into a split state when we'd expect that on trying to split commit 3 we'd get an error since the branch commit-1 is not the tip any more

cmd/av/branch.go Outdated
)

// Adopt new branch to av database
err = adoptCmd.RunE(nil, []string{"parent", currentBranchName})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can call adoptForceAdoption(repo, db, newBranch, oldBranch) directly

Copy link
Author

@Chafid Chafid Dec 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tulioz , I update to use adoptForceAdoption for updating the av database.
Also, maybe I misunderstand what you mean by "tip of the branch". I assumed that the branch needs to be on its last commit for this split command to run. Is this correct? Because if that's correct then this should work as expected.
When you switch to another branch then the user automatically is on the last commit of that branch, so the split should work.
But if you switch to another commit, like this:

chafid@WINDOWS-4D6659B:~/repository/test_repo$ av switch commit-2
chafid@WINDOWS-4D6659B:~/repository/test_repo$ git log --oneline
ccf90a5 (HEAD -> commit-2) commit 2
6714a51 (commit-1) commit 1
0e32223 (origin/main, origin/HEAD, main) 1st commit
chafid@WINDOWS-4D6659B:~/repository/test_repo$ av switch 6714a51
chafid@WINDOWS-4D6659B:~/repository/test_repo$ echo "Third commit" > commit-3.txt
chafid@WINDOWS-4D6659B:~/repository/test_repo$ git add commit-3.txt
chafid@WINDOWS-4D6659B:~/repository/test_repo$ git commit -m "commit 3"
[detached HEAD e17cd79] commit 3
 1 file changed, 1 insertion(+)
 create mode 100644 commit-3.txt
chafid@WINDOWS-4D6659B:~/repository/test_repo$ av branch --split
The split operation was aborted.
The original branch  remains intact.

This should work as expected

add condition to check detached head
@tulioz
Copy link
Contributor

tulioz commented Dec 19, 2024

@Chafid sorry for the delay in my response, been a hectic week for me. You are correct we want to force tip of branch, but I think we should also enforce tip of stack. Today if a stack looks like this:

master -> first_branch -> second_branch

and someone runs the command on first branch they'll end up with

master -> first_branch -> second_branch
                      | -> third_branch

and now they have a fork to resolve, ideally we'd only want to let them run on the tip of second_branch so that their stack can only end up like this

master-> first branch -> second_branch -> third_branch

Let me know what you think!

@Chafid
Copy link
Author

Chafid commented Dec 20, 2024

Ah got it @tulioz . Is there already a mechanism in av to check whether the user is already on the tip stack? Or I need to figure it out myself?

@Chafid
Copy link
Author

Chafid commented Dec 20, 2024

Hi @tulioz , I've added the check to only split when in tip of the stack. Please check it

@tulioz
Copy link
Contributor

tulioz commented Dec 20, 2024

Hey @Chafid, there should be code in av next that does checking for being on tip of stack. I ran the same test as earlier:

av branch commit-1

echo "First commit" > commit-1.txt
git add commit-1.txt
git commit -m "commit 1"

echo "Second commit" > commit-2.txt
git add commit-2.txt
git commit -m "commit 2"
av branch --split

av switch commit-1

echo "Third commit" > commit-3.txt
git add commit-3.txt
git commit -m "commit 3"
av branch --split

av tree

and I'm still seeing the forking behavior:
image

@Chafid
Copy link
Author

Chafid commented Dec 21, 2024

Hi @tulioz , are you sure you have the latest update? It works correctly on my local

chafid@WINDOWS-4D6659B:~/repository/test_repo$ av tree

  * test3
  │ No pull request
  │
  * test-2
  │ No pull request
  │
  * main (HEAD)

chafid@WINDOWS-4D6659B:~/repository/test_repo$ av switch test-2
chafid@WINDOWS-4D6659B:~/repository/test_repo$ av tree

  * test3
  │ No pull request
  │
  * test-2 (HEAD)
  │ No pull request
  │
  * main

chafid@WINDOWS-4D6659B:~/repository/test_repo$ av branch --split

The split operation was aborted.
branch 'test-2' has a descendant branch 'test3'

error: split failed: branch 'test-2' has a descendant branch 'test3'
chafid@WINDOWS-4D6659B:~/repository/test_repo$

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants