Don't panic when in detached head state. #5943
Conversation
I'm not proud of it, it could use a rewrite, but it's outside the scope of this PR.
go/cmd/dolt/commands/branch.go
Outdated
| @@ -172,7 +175,11 @@ func printBranches(ctx context.Context, dEnv *env.DoltEnv, apr *argparser.ArgPar | |||
| } | |||
|
|
|||
| if verbose { | |||
| cm, err := dEnv.DoltDB.Resolve(ctx, cs, dEnv.RepoStateReader().CWBHeadRef()) | |||
| headRef, err := dEnv.RepoStateReader().CWBHeadRef() | |||
There was a problem hiding this comment.
You can re-use currentBranch from above here
go/cmd/dolt/commands/checkout.go
Outdated
| @@ -195,7 +202,11 @@ func checkoutNewBranch(ctx context.Context, dEnv *env.DoltEnv, apr *argparser.Ar | |||
| if !remoteOk { | |||
| return nil | |||
| } | |||
| verr = SetRemoteUpstreamForBranchRef(dEnv, remoteName, remoteBranchName, dEnv.RepoStateReader().CWBHeadRef()) | |||
| headRef, err := dEnv.RepoStateReader().CWBHeadRef() | |||
There was a problem hiding this comment.
You can re-use the headRef from above
go/cmd/dolt/commands/push.go
Outdated
| remoteName = defRemote.Name | ||
| currentBranch, err := dEnv.RepoStateReader().CWBHeadRef() | ||
| if err != nil { | ||
| verr = errhand.BuildDError("fatal: The current branch could not be identified").Build() |
There was a problem hiding this comment.
You probably want the err in here as a cause
| @@ -654,6 +661,9 @@ func (db Database) GetWorkingSet(ctx *sql.Context) (*doltdb.WorkingSet, error) { | |||
| if !ok { | |||
| return nil, fmt.Errorf("no root value found in session") | |||
| } | |||
| if dbState.WorkingSet == nil { | |||
There was a problem hiding this comment.
Don't know if we actually want this behavior. In general an accessor like this shouldn't second-guess what a client is going to do with the things it returns. It's a valid state for a session to not have a working set, and returning a nil value here is fine, it doesn't need to be an error. Generally speaking we don't want the presence of an error to signal needed changes in business logic, only actual errors that mean something went wrong, which isn't the case here.
| workingSet, err := s.session.WorkingSet(sql.NewContext(context.Background()), s.dbName) | ||
| if err != nil { | ||
| // TODO: fix this interface | ||
| panic(err) | ||
| return nil, err |
There was a problem hiding this comment.
This is where the error should originate, not session.WorkingSet
There was a problem hiding this comment.
In principle, I agree with your comments. It should be up to the client to decide whether the lack of a working set is valid, and up the api to return results in a form that require clients to consider this.
The trouble is that a lot of places call session.Workingset without calling CWBHeadRef, and don't do any checking on the result, and adding checks to all of them would complicate all those commands while doing nothing to prevent this same mistake from happening if we add another command that calls session.Workingset.
The long term solution is to change the interface to one where instead of clients requesting the working set, they request what they want the working set for, and the and session returns the best result. But until we have that, the priority is to prevent panics and make it harder to introduce panics in the future. This does that.
There was a problem hiding this comment.
I'm fine with this for now, please add a TODO to this effect in the method doc for Database.GetWorkingSet()
|
Where we at with this one? |
|
@coffeegoddd DOLT
|
|
@coffeegoddd DOLT
|
|
@coffeegoddd DOLT
|
|
@coffeegoddd DOLT
|
Fixes #5871
Many commands currently panic when in a detached head state, such as commit (#5744), checkout (#5839), and many others. This turns those panics into errors.