Merge pull request #6375 from dolthub/fulghum/cherry-pick

Bug fix: Commits created with `cherry-pick` should only have one parent commit

go/libraries/doltcore/doltdb/workingset.go

@@ -35,6 +35,9 @@ type MergeState struct {
 	commitSpecStr    string
 	preMergeWorking  *RootValue
 	unmergableTables []string
+	// isCherryPick is set to true when the in-progress merge is a cherry-pick. This is needed so that
+	// commit knows to NOT create a commit with multiple parents when creating a commit for a cherry-pick.
+	isCherryPick bool
 }
 
 // todo(andy): this might make more sense in pkg merge
@@ -75,6 +78,12 @@ func (m MergeState) CommitSpecStr() string {
 	return m.commitSpecStr
 }
 
+// IsCherryPick returns true if the current merge state is for a cherry-pick operation. Cherry-picks use the same
+// code as merge, but need slightly different behavior (e.g. only recording one commit parent, instead of two).
+func (m MergeState) IsCherryPick() bool {
+	return m.isCherryPick
+}
+
 func (m MergeState) PreMergeWorkingRoot() *RootValue {
 	return m.preMergeWorking
 }
@@ -193,6 +202,20 @@ func (ws WorkingSet) StartMerge(commit *Commit, commitSpecStr string) *WorkingSe
 	return &ws
 }
 
+// StartCherryPick creates and returns a new working set based off of the current |ws| with the specified |commit|
+// and |commitSpecStr| referring to the commit being cherry-picked. The returned WorkingSet records that a cherry-pick
+// operation is in progress (i.e. conflicts being resolved). Note that this function does not update the current
+// session – the returned WorkingSet must still be set using DoltSession.SetWorkingSet().
+func (ws WorkingSet) StartCherryPick(commit *Commit, commitSpecStr string) *WorkingSet {
+	ws.mergeState = &MergeState{
+		commit:          commit,
+		commitSpecStr:   commitSpecStr,
+		preMergeWorking: ws.workingRoot,
+		isCherryPick:    true,
+	}
+	return &ws
+}
+
 func (ws WorkingSet) AbortMerge() *WorkingSet {
 	ws.workingRoot = ws.mergeState.PreMergeWorkingRoot()
 	ws.stagedRoot = ws.workingRoot
@@ -221,6 +244,18 @@ func (ws *WorkingSet) MergeActive() bool {
 	return ws.mergeState != nil
 }
 
+// MergeCommitParents returns true if there is an active merge in progress and
+// the recorded commit being merged into the active branch should be included as
+// a second parent of the created commit. This is the expected behavior for a
+// normal merge, but not for other pseudo-merges, like cherry-picks or reverts,
+// where the created commit should only have one parent.
+func (ws *WorkingSet) MergeCommitParents() bool {
+	if !ws.MergeActive() {
+		return false
+	}
+	return ws.MergeState().IsCherryPick() == false
+}
+
 func (ws WorkingSet) Meta() *datas.WorkingSetMeta {
 	return ws.meta
 }
@@ -299,11 +334,17 @@ func newWorkingSet(ctx context.Context, name string, vrw types.ValueReadWriter,
 			return nil, err
 		}
 
+		isCherryPick, err := dsws.MergeState.IsCherryPick(ctx, vrw)
+		if err != nil {
+			return nil, err
+		}
+
 		mergeState = &MergeState{
 			commit:           commit,
 			commitSpecStr:    commitSpec,
 			preMergeWorking:  preMergeWorkingRoot,
 			unmergableTables: unmergableTables,
+			isCherryPick:     isCherryPick,
 		}
 	}
 
@@ -345,7 +386,6 @@ func (ws *WorkingSet) writeValues(ctx context.Context, db *DoltDB) (
 	mergeState *datas.MergeState,
 	err error,
 ) {
-
 	if ws.stagedRoot == nil || ws.workingRoot == nil {
 		return types.Ref{}, types.Ref{}, nil, fmt.Errorf("StagedRoot and workingRoot must be set. This is a bug.")
 	}
@@ -379,7 +419,7 @@ func (ws *WorkingSet) writeValues(ctx context.Context, db *DoltDB) (
 			return types.Ref{}, types.Ref{}, nil, err
 		}
 
-		mergeState, err = datas.NewMergeState(ctx, db.vrw, preMergeWorking, dCommit, ws.mergeState.commitSpecStr, ws.mergeState.unmergableTables)
+		mergeState, err = datas.NewMergeState(ctx, db.vrw, preMergeWorking, dCommit, ws.mergeState.commitSpecStr, ws.mergeState.unmergableTables, ws.mergeState.isCherryPick)
 		if err != nil {
 			return types.Ref{}, types.Ref{}, nil, err
 		}

go/libraries/doltcore/sqle/dprocedures/dolt_cherry_pick.go

@@ -287,7 +287,7 @@ func cherryPick(ctx *sql.Context, dSess *dsess.DoltSession, roots doltdb.Roots,
 			if err != nil {
 				return nil, "", err
 			}
-			newWorkingSet := ws.StartMerge(cherryCommit, cherryStr)
+			newWorkingSet := ws.StartCherryPick(cherryCommit, cherryStr)
 			err = dSess.SetWorkingSet(ctx, dbName, newWorkingSet)
 			if err != nil {
 				return nil, "", err

go/libraries/doltcore/sqle/dsess/session.go

@@ -657,7 +657,7 @@ func (d *DoltSession) newPendingCommit(ctx *sql.Context, branchState *branchStat
 	}
 
 	var mergeParentCommits []*doltdb.Commit
-	if branchState.WorkingSet().MergeActive() {
+	if branchState.WorkingSet().MergeCommitParents() {
 		mergeParentCommits = []*doltdb.Commit{branchState.WorkingSet().MergeState().Commit()}
 	} else if props.Amend {
 		numParentsHeadForAmend := headCommit.NumParents()

go/libraries/doltcore/sqle/enginetest/dolt_queries.go

@@ -4348,6 +4348,11 @@ var DoltCherryPickTests = []queries.ScriptTest{
 				Query:    "SELECT * FROM t order by pk;",
 				Expected: []sql.Row{{1, "one"}, {2, "two"}},
 			},
+			{
+				// Assert that our new commit only has one parent (i.e. not a merge commit)
+				Query:    "select count(*) from dolt_commit_ancestors where commit_hash = hashof('HEAD');",
+				Expected: []sql.Row{{1}},
+			},
 		},
 	},
 	{
@@ -4395,6 +4400,11 @@ var DoltCherryPickTests = []queries.ScriptTest{
 				Query:    "call dolt_cherry_pick(@commit1);",
 				Expected: []sql.Row{{doltCommit, 0, 0, 0}},
 			},
+			{
+				// Assert that our new commit only has one parent (i.e. not a merge commit)
+				Query:    "select count(*) from dolt_commit_ancestors where commit_hash = hashof('HEAD');",
+				Expected: []sql.Row{{1}},
+			},
 			{
 				Query:    "SHOW TABLES;",
 				Expected: []sql.Row{{"myview"}, {"table_a"}},
@@ -4635,6 +4645,15 @@ var DoltCherryPickTests = []queries.ScriptTest{
 				Query:    "select * from t;",
 				Expected: []sql.Row{{0}},
 			},
+			{
+				Query:    "call dolt_commit('-am', 'committing cherry-pick');",
+				Expected: []sql.Row{{doltCommit}},
+			},
+			{
+				// Assert that our new commit only has one parent (i.e. not a merge commit)
+				Query:    "select count(*) from dolt_commit_ancestors where commit_hash = hashof('HEAD');",
+				Expected: []sql.Row{{1}},
+			},
 		},
 	},
 	{
@@ -4695,6 +4714,15 @@ var DoltCherryPickTests = []queries.ScriptTest{
 				Query:    `SELECT * FROM t;`,
 				Expected: []sql.Row{{1, "ein"}},
 			},
+			{
+				Query:    "call dolt_commit('-am', 'committing cherry-pick');",
+				Expected: []sql.Row{{doltCommit}},
+			},
+			{
+				// Assert that our new commit only has one parent (i.e. not a merge commit)
+				Query:    "select count(*) from dolt_commit_ancestors where commit_hash = hashof('HEAD');",
+				Expected: []sql.Row{{1}},
+			},
 		},
 	},
 	{

go/serial/workingset.fbs

@@ -38,6 +38,8 @@ table MergeState {
   from_commit_spec_str:string;
 
   unmergable_tables:[string];
+
+  is_cherry_pick:bool;
 }
 
 // KEEP THIS IN SYNC WITH fileidentifiers.go

go/store/datas/dataset.go

@@ -165,6 +165,7 @@ type MergeState struct {
 	fromCommitAddr      *hash.Hash
 	fromCommitSpec      string
 	unmergableTables    []string
+	isCherryPick        bool
 
 	nomsMergeStateRef *types.Ref
 	nomsMergeState    *types.Struct
@@ -256,6 +257,13 @@ func (ms *MergeState) FromCommitSpec(ctx context.Context, vr types.ValueReader)
 	return string(commitSpecStr.(types.String)), nil
 }
 
+func (ms *MergeState) IsCherryPick(_ context.Context, vr types.ValueReader) (bool, error) {
+	if vr.Format().UsesFlatbuffers() {
+		return ms.isCherryPick, nil
+	}
+	return false, nil
+}
+
 func (ms *MergeState) UnmergableTables(ctx context.Context, vr types.ValueReader) ([]string, error) {
 	if vr.Format().UsesFlatbuffers() {
 		return ms.unmergableTables, nil
@@ -381,6 +389,7 @@ func (h serialWorkingSetHead) HeadWorkingSet() (*WorkingSetHead, error) {
 		for i := range ret.MergeState.unmergableTables {
 			ret.MergeState.unmergableTables[i] = string(mergeState.UnmergableTables(i))
 		}
+		ret.MergeState.isCherryPick = mergeState.IsCherryPick()
 	}
 	return &ret, nil
 }

go/store/datas/workingset.go

@@ -168,6 +168,7 @@ func workingset_flatbuffer(working hash.Hash, staged *hash.Hash, mergeState *Mer
 		serial.MergeStateAddFromCommitAddr(builder, fromaddroff)
 		serial.MergeStateAddFromCommitSpecStr(builder, fromspecoff)
 		serial.MergeStateAddUnmergableTables(builder, unmergableoff)
+		serial.MergeStateAddIsCherryPick(builder, mergeState.isCherryPick)
 		mergeStateOff = serial.MergeStateEnd(builder)
 	}
 
@@ -202,13 +203,15 @@ func NewMergeState(
 	commit *Commit,
 	commitSpecStr string,
 	unmergableTables []string,
+	isCherryPick bool,
 ) (*MergeState, error) {
 	if vrw.Format().UsesFlatbuffers() {
 		ms := &MergeState{
 			preMergeWorkingAddr: new(hash.Hash),
 			fromCommitAddr:      new(hash.Hash),
 			fromCommitSpec:      commitSpecStr,
 			unmergableTables:    unmergableTables,
+			isCherryPick:        isCherryPick,
 		}
 		*ms.preMergeWorkingAddr = preMergeWorking.TargetHash()
 		*ms.fromCommitAddr = commit.Addr()
@@ -223,6 +226,7 @@ func NewMergeState(
 			return nil, err
 		}
 		return &MergeState{
+			isCherryPick:      isCherryPick,
 			nomsMergeStateRef: &ref,
 			nomsMergeState:    &v,
 		}, nil

proto/README.md

@@ -6,7 +6,9 @@ Protobuf message and service definitions.
 Dependencies
 ------------
 
-Dependencies are git submodules in //proto/third_party.
+Dependencies are git submodules in //proto/third_party. Make sure you have all these
+submodules synced. If not, you can sync them initially with:
+  git submodule update --init 
 
 * You need to build protoc in //proto/third_party/protobuf by running `bazel
   build //:protoc` from that directory. Currently tested with bazel version 3.1.0.