Conversation
…ed, and tracking working set rebase metadata.
… them to the branch HEAD, and not to the ontoCommit yet
…ing in detatched head)
…into a new cherry_pick package
…till messy, but moving in the good direction
zachmu
left a comment
There was a problem hiding this comment.
This looks pretty great, nice implementation
Just a couple comments and suggestions about testing and stuff.
| return err | ||
| } | ||
|
|
||
| err = copyABranch(ctx, dbData, rebaseWorkingBranch, rebaseBranch, true, nil) |
There was a problem hiding this comment.
You didn't create this problem, but this is a race because the underlying operation (doltdb.NewBranchAtCommit) is not atomic. It sets the branch head, then updates the working set head with whatever it finds there, in two separate optimistic locking noms transactions. So concurrent users of e.g. call dolt_branch(-f) and this procedure could overwrite each other's change as well as end up with an inconsistent working set (doesn't match the branch head).
Probably worth adding a TODO that we need to clean that up at some point, the entire model needs to be overhauled to update more than a single head atomically. We could do part of this now, by changing the NewBranchAtCommit call to use database.CommitWithWorkingSet instead of two separate atomic operations.
There was a problem hiding this comment.
Thanks for pointing this out. I've added a TODO and dug into the code to see where the race condition is. It looks like database.CommitWithWorkingSet creates a new commit though, which seems different from what we want here. I'm happy to follow up in another PR and dig in deeper and see if we can get this spot fixed.
| Assertions: []queries.ScriptTestAssertion{ | ||
| { | ||
| Query: "call dolt_rebase('-i', 'main');", | ||
| ExpectedErrStr: "fatal: A branch named 'dolt_rebase_branch1' already exists.", |
There was a problem hiding this comment.
This isn't great, these are supposed to be strictly temporary identifiers to get around us not supporting a detached head operation. Would be better to just choose a branch name at random, put a number on the end, something else. No reason to ever make the customer care about our branch namespace choice here.
There was a problem hiding this comment.
I'm on the fence about this. It's an easy change to use a different working branch name, but I liked that the pattern was predictable, i.e. "dolt_rebase_" + rebaseBranchName. The main reason I thought a predictable pattern would be useful was if a user needed to reconnect in a different session and would have to check out this working branch to continue the rebase.
We don't want to allow multiple active rebases to the same branch at the same time, and the "dolt_" branch namespace shouldn't be used by customers, so I don't think the naming restriction causes a problem.
…olt_rebase to make it more clear what users should do next
Adds support for an interactive rebase workflow with Dolt. This allows users to edit their commit history, including rewording commit messages, reordering commits, dropping commits, and squashing multiple commits together. At the end of an interactive rebase, the current branch points to a new commit history created by executing the rebase plan the user specified.
Resolves #3467