Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions go/libraries/doltcore/doltdb/doltdb.go
Original file line number Diff line number Diff line change
Expand Up @@ -642,6 +642,37 @@ func (ddb *DoltDB) ResolveCommitRef(ctx context.Context, doltRef ref.DoltRef) (*
return NewCommit(ctx, ddb.vrw, ddb.ns, commitVal)
}

// ResolveCommitSpecStr resolves the commit spec string |spec| to a commit, using |headRef| to
// anchor relative specs such as HEAD. The pseudo-refs WORKING and STAGED name uncommitted roots
// rather than commits, so when one of them is given it resolves to the commit at |headRef| that
// those roots are built on top of. A working set exists only relative to a branch, so |headRef|
// must be non-nil for WORKING or STAGED, otherwise ResolveCommitSpecStr returns
// ErrOperationNotSupportedInDetachedHead.
func (ddb *DoltDB) ResolveCommitSpecStr(ctx context.Context, spec string, headRef ref.DoltRef) (*Commit, error) {
if IsWorkingSetRef(spec) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm worried this change is overly broad and might result in us treating WORKING or STAGED as HEAD in places we probably shouldn't.

For instance, with this change, HAS_ANCESTOR('HEAD','WORKING') returns true, even though it shouldn't. I think it's debatable whether or not "STAGED" and "WORKING" should be valid values for the ancestor field here, but they definitely shouldn't be treated as "HEAD"

Similarly, DOLT_DIFF('STAGED...WORKING', ...) now uses HEAD as the merge base and will display already-staged changes in the diff. This seems wrong.

I think if the purpose of this function is to generate commits, then it shouldn't accept things that aren't refs to commits. WORKING and STAGED should be checked for at a level above this. Perhaps we need a wrapper around the MergeBase logic that accepts these strings.

if headRef == nil {
return nil, ErrOperationNotSupportedInDetachedHead
}
return ddb.ResolveCommitRef(ctx, headRef)
}

cs, err := NewCommitSpec(spec)
if err != nil {
return nil, err
}

optCmt, err := ddb.Resolve(ctx, cs, headRef)
if err != nil {
return nil, err
}
commit, ok := optCmt.ToCommit()
if !ok {
return nil, ErrGhostCommitEncountered
}

return commit, nil
}

// ResolveCommitRefAtRoot takes a DoltRef and returns a Commit, or an error if the commit cannot be found. The ref given must
// point to a Commit.
func (ddb *DoltDB) ResolveCommitRefAtRoot(ctx context.Context, ref ref.DoltRef, nomsRoot hash.Hash) (*Commit, error) {
Expand Down
4 changes: 2 additions & 2 deletions go/libraries/doltcore/sqle/dfunctions/has_ancestor.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func (a *HasAncestor) Eval(ctx *sql.Context, row sql.Row) (interface{}, error) {
if err != nil {
return nil, err
}
headCommit, err = ResolveRefSpec(ctx, headRef, ddb, headStr.(string))
headCommit, err = ddb.ResolveCommitSpecStr(ctx, headStr.(string), headRef)
if err != nil {
return nil, err
}
Expand All @@ -85,7 +85,7 @@ func (a *HasAncestor) Eval(ctx *sql.Context, row sql.Row) (interface{}, error) {
if err != nil {
return nil, err
}
ancCommit, err = ResolveRefSpec(ctx, headRef, ddb, ancStr.(string))
ancCommit, err = ddb.ResolveCommitSpecStr(ctx, ancStr.(string), headRef)
if err != nil {
return nil, err
}
Expand Down
23 changes: 2 additions & 21 deletions go/libraries/doltcore/sqle/dfunctions/merge_base.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,7 @@ import (
"github.com/dolthub/go-mysql-server/sql/expression"
"github.com/dolthub/go-mysql-server/sql/types"

"github.com/dolthub/dolt/go/libraries/doltcore/doltdb"
"github.com/dolthub/dolt/go/libraries/doltcore/merge"
"github.com/dolthub/dolt/go/libraries/doltcore/ref"
"github.com/dolthub/dolt/go/libraries/doltcore/sqle/dsess"
)

Expand Down Expand Up @@ -77,11 +75,11 @@ func (d MergeBase) Eval(ctx *sql.Context, row sql.Row) (interface{}, error) {
return nil, errors.New("right value is not a string")
}

left, err := ResolveRefSpec(ctx, headRef, doltDB, leftStr)
left, err := doltDB.ResolveCommitSpecStr(ctx, leftStr, headRef)
if err != nil {
return nil, err
}
right, err := ResolveRefSpec(ctx, headRef, doltDB, rightStr)
right, err := doltDB.ResolveCommitSpecStr(ctx, rightStr, headRef)
if err != nil {
return nil, err
}
Expand All @@ -94,23 +92,6 @@ func (d MergeBase) Eval(ctx *sql.Context, row sql.Row) (interface{}, error) {
return mergeBase.String(), nil
}

// ResolveRefSpec resolves a commit spec string to a Commit object using the provided headRef and doltDB.
func ResolveRefSpec(ctx *sql.Context, headRef ref.DoltRef, doltDB *doltdb.DoltDB, spec string) (*doltdb.Commit, error) {
cs, err := doltdb.NewCommitSpec(spec)
if err != nil {
return nil, err
}
optCmt, err := doltDB.Resolve(ctx, cs, headRef)
if err != nil {
return nil, err
}
commit, ok := optCmt.ToCommit()
if !ok {
return nil, doltdb.ErrGhostCommitEncountered
}
return commit, err
}

// String implements the sql.Expression interface.
func (d MergeBase) String() string {
return fmt.Sprintf("DOLT_MERGE_BASE(%s,%s)", d.Left().String(), d.Right().String())
Expand Down
3 changes: 1 addition & 2 deletions go/libraries/doltcore/sqle/dprocedures/dolt_rebase.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import (
"github.com/dolthub/dolt/go/libraries/doltcore/merge"
"github.com/dolthub/dolt/go/libraries/doltcore/rebase"
"github.com/dolthub/dolt/go/libraries/doltcore/ref"
"github.com/dolthub/dolt/go/libraries/doltcore/sqle/dfunctions"
"github.com/dolthub/dolt/go/libraries/doltcore/sqle/dsess"
"github.com/dolthub/dolt/go/libraries/utils/argparser"
)
Expand Down Expand Up @@ -875,7 +874,7 @@ func commitManuallyStagedChangesForStep(ctx *sql.Context, step rebase.RebasePlan
return err
}

originalCommit, err := dfunctions.ResolveRefSpec(ctx, headRef, doltDB, step.CommitHash)
originalCommit, err := doltDB.ResolveCommitSpecStr(ctx, step.CommitHash, headRef)
if err != nil {
return err
}
Expand Down
25 changes: 3 additions & 22 deletions go/libraries/doltcore/sqle/dtablefunctions/dolt_diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (
"github.com/dolthub/dolt/go/libraries/doltcore/diff"
"github.com/dolthub/dolt/go/libraries/doltcore/doltdb"
"github.com/dolthub/dolt/go/libraries/doltcore/merge"
"github.com/dolthub/dolt/go/libraries/doltcore/ref"
"github.com/dolthub/dolt/go/libraries/doltcore/schema"
"github.com/dolthub/dolt/go/libraries/doltcore/sqle/dsess"
"github.com/dolthub/dolt/go/libraries/doltcore/sqle/dtables"
Expand Down Expand Up @@ -345,17 +344,17 @@ func resolveCommitStrings(ctx *sql.Context, fromRef, toRef, dotRef interface{},
return "", "", err
}

rightCm, err := resolveCommit(ctx, db.DbData().Ddb, headRef, refs[0])
leftCm, err := db.DbData().Ddb.ResolveCommitSpecStr(ctx, refs[0], headRef)
if err != nil {
return "", "", err
}

leftCm, err := resolveCommit(ctx, db.DbData().Ddb, headRef, refs[1])
rightCm, err := db.DbData().Ddb.ResolveCommitSpecStr(ctx, refs[1], headRef)
if err != nil {
return "", "", err
}

mergeBase, err := merge.MergeBase(ctx, rightCm, leftCm)
mergeBase, err := merge.MergeBase(ctx, leftCm, rightCm)
if err != nil {
return "", "", err
}
Expand Down Expand Up @@ -404,24 +403,6 @@ func interfaceToString(r interface{}) (string, error) {
return str, nil
}

func resolveCommit(ctx *sql.Context, ddb *doltdb.DoltDB, headRef ref.DoltRef, cSpecStr string) (*doltdb.Commit, error) {
cs, err := doltdb.NewCommitSpec(cSpecStr)
if err != nil {
return nil, err
}

optCmt, err := ddb.Resolve(ctx, cs, headRef)
if err != nil {
return nil, err
}
cm, ok := optCmt.ToCommit()
if !ok {
return nil, doltdb.ErrGhostCommitEncountered
}

return cm, nil
}

// WithChildren implements the sql.Node interface
func (dtf *DiffTableFunction) WithChildren(ctx *sql.Context, node ...sql.Node) (sql.Node, error) {
if len(node) != 0 {
Expand Down
33 changes: 3 additions & 30 deletions go/libraries/doltcore/sqle/dtablefunctions/dolt_log.go
Original file line number Diff line number Diff line change
Expand Up @@ -496,38 +496,20 @@ func (ltf *LogTableFunction) RowIter(ctx *sql.Context, row sql.Row) (sql.RowIter
}

for _, revisionStr := range revisionValStrs {
cs, err := doltdb.NewCommitSpec(revisionStr)
commit, err := sqledb.DbData().Ddb.ResolveCommitSpecStr(ctx, revisionStr, headRef)
if err != nil {
return nil, err
}

optCmt, err := sqledb.DbData().Ddb.Resolve(ctx, cs, headRef)
if err != nil {
return nil, err
}
commit, ok = optCmt.ToCommit()
if err != nil {
return nil, doltdb.ErrGhostCommitEncountered
}

commits = append(commits, commit)
}

var notCommits []*doltdb.Commit
for _, notRevisionStr := range notRevisionValStrs {
cs, err := doltdb.NewCommitSpec(notRevisionStr)
if err != nil {
return nil, err
}

optCmt, err := sqledb.DbData().Ddb.Resolve(ctx, cs, headRef)
notCommit, err := sqledb.DbData().Ddb.ResolveCommitSpecStr(ctx, notRevisionStr, headRef)
if err != nil {
return nil, err
}
notCommit, ok := optCmt.ToCommit()
if !ok {
return nil, doltdb.ErrGhostCommitEncountered
}

notCommits = append(notCommits, notCommit)
}
Expand All @@ -538,20 +520,11 @@ func (ltf *LogTableFunction) RowIter(ctx *sql.Context, row sql.Row) (sql.RowIter
return nil, err
}

mergeCs, err := doltdb.NewCommitSpec(mergeBase.String())
if err != nil {
return nil, err
}

// Use merge base as excluding commit
optCmt, err := sqledb.DbData().Ddb.Resolve(ctx, mergeCs, nil)
mergeCommit, err := sqledb.DbData().Ddb.ResolveCommitSpecStr(ctx, mergeBase.String(), nil)
if err != nil {
return nil, err
}
mergeCommit, ok := optCmt.ToCommit()
if !ok {
return nil, doltdb.ErrGhostCommitEncountered
}

notCommits = append(notCommits, mergeCommit)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -297,12 +297,12 @@ func resolveBranchesToRoots(ctx *sql.Context, db dsess.SqlDatabase, leftBranch,
return rootInfo{}, err
}

leftCm, err := resolveCommit(ctx, db.DbData().Ddb, headRef, leftBranch)
leftCm, err := db.DbData().Ddb.ResolveCommitSpecStr(ctx, leftBranch, headRef)
if err != nil {
return rootInfo{}, err
}

rightCm, err := resolveCommit(ctx, db.DbData().Ddb, headRef, rightBranch)
rightCm, err := db.DbData().Ddb.ResolveCommitSpecStr(ctx, rightBranch, headRef)
if err != nil {
return rootInfo{}, err
}
Expand Down
62 changes: 62 additions & 0 deletions go/libraries/doltcore/sqle/enginetest/dolt_queries.go
Original file line number Diff line number Diff line change
Expand Up @@ -5714,6 +5714,68 @@ var LogTableFunctionScriptTests = []queries.ScriptTest{
},
},
},
{
// See https://github.com/dolthub/dolt/issues/11204
Name: "dolt_log: three dot diff accepts WORKING and STAGED",
SetUpScript: []string{
"create table t (pk int primary key);",
"call dolt_add('.');",
"call dolt_commit('-m', 'commit 1');",
"call dolt_checkout('-b', 'new-branch');",
"insert into t values (1);",
"call dolt_commit('-am', 'commit 2');",
"insert into t values (2);",
"call dolt_commit('-am', 'commit 3');",
"call dolt_checkout('main');",
"insert into t values (99);",
},
Assertions: []queries.ScriptTestAssertion{
{
Query: "SELECT message from dolt_log('main...new-branch') ORDER BY date DESC;",
Expected: []sql.Row{{"commit 3"}, {"commit 2"}},
},
{
Query: "SELECT message from dolt_log('WORKING...new-branch') ORDER BY date DESC;",
// WORKING is not a commit, so it resolves to the current branch HEAD and
// the merge base excludes exactly the commits main...new-branch does.
Expected: []sql.Row{{"commit 3"}, {"commit 2"}},
},
{
Query: "SELECT message from dolt_log('STAGED...new-branch') ORDER BY date DESC;",
Expected: []sql.Row{{"commit 3"}, {"commit 2"}},
},
},
},
{
// See https://github.com/dolthub/dolt/issues/11204
Name: "dolt_log: three dot WORKING on a detached head",
SetUpScript: []string{
"create table t (pk int primary key);",
"call dolt_add('.');",
"call dolt_commit('-m', 'commit 1');",
"call dolt_tag('v1');",
"call dolt_checkout('-b', 'new-branch');",
"insert into t values (1);",
"call dolt_commit('-am', 'commit 2');",
"use `mydb/v1`;",
},
Assertions: []queries.ScriptTestAssertion{
{
// A working set exists only relative to a branch, so resolving WORKING
// without a checked out branch returns an error today.
Query: "SELECT message from dolt_log('WORKING...new-branch') ORDER BY date DESC;",
ExpectedErrStr: "this operation is not supported while in a detached head state",
},
{
// TODO: dolt cannot yet resolve a relative ref for a merge base on a detached
// head, so unlike git this errors instead of listing what new-branch added.
// Remove Skip once dolt resolves WORKING against the detached HEAD commit.
Skip: true,
Query: "SELECT message from dolt_log('WORKING...new-branch') ORDER BY date DESC;",
Expected: []sql.Row{{"commit 2"}},
},
},
},
}

var JsonDiffTableFunctionScriptTests = []queries.ScriptTest{
Expand Down
Loading
Loading