OrderedSequenceDiff ValueChanged adds NewValue/OldValue (#3468)

This commit is contained in:
Rafael Weinstein
2017-05-09 16:43:04 -07:00
committed by GitHub
parent 78930b2487
commit 9654d372a2
13 changed files with 107 additions and 69 deletions
+3 -3
View File
@@ -482,13 +482,13 @@ func assertMapOfStringToRefOfCommit(proposed, datasets types.Map, vr types.Value
case types.DiffChangeAdded, types.DiffChangeModified:
// Since this is a Map Diff, change.V is the key at which a change was detected.
// Go get the Value there, which should be a Ref<Value>, deref it, and then ensure the target is a Commit.
val := proposed.Get(change.V)
val := change.NewValue
ref, ok := val.(types.Ref)
if !ok {
d.Panic("Root of a Database must be a Map<String, Ref<Commit>>, but key %s maps to a %s", change.V.(types.String), types.TypeOf(val).Describe())
d.Panic("Root of a Database must be a Map<String, Ref<Commit>>, but key %s maps to a %s", change.Key.(types.String), types.TypeOf(val).Describe())
}
if targetValue := ref.TargetValue(vr); !IsCommit(targetValue) {
d.Panic("Root of a Database must be a Map<String, Ref<Commit>>, not the ref at key %s points to a %s", change.V.(types.String), types.TypeOf(targetValue).Describe())
d.Panic("Root of a Database must be a Map<String, Ref<Commit>>, not the ref at key %s points to a %s", change.Key.(types.String), types.TypeOf(targetValue).Describe())
}
}
}
+4 -4
View File
@@ -224,18 +224,18 @@ func (d differ) diffOrdered(p types.Path, ppf pathPartFunc, df diffFunc, kf, v1,
break
}
k := kf(change.V)
k := kf(change.Key)
p1 := p.Append(ppf(k))
switch change.ChangeType {
case types.DiffChangeAdded:
dif := Difference{Path: p1, ChangeType: types.DiffChangeAdded, OldValue: nil, NewValue: v2(change.V), NewKeyValue: k}
dif := Difference{Path: p1, ChangeType: types.DiffChangeAdded, OldValue: nil, NewValue: v2(change.Key), NewKeyValue: k}
stop = !d.sendDiff(dif)
case types.DiffChangeRemoved:
dif := Difference{Path: p1, ChangeType: types.DiffChangeRemoved, OldValue: v1(change.V), NewValue: nil}
dif := Difference{Path: p1, ChangeType: types.DiffChangeRemoved, OldValue: v1(change.Key), NewValue: nil}
stop = !d.sendDiff(dif)
case types.DiffChangeModified:
c1, c2 := v1(change.V), v2(change.V)
c1, c2 := v1(change.Key), v2(change.Key)
if shouldDescend(c1, c2) {
stop = d.diff(p1, c1, c2)
} else {
+8 -8
View File
@@ -36,10 +36,10 @@ func (mc mapCandidate) get(k types.Value) types.Value {
func (mc mapCandidate) pathConcat(change types.ValueChanged, path types.Path) (out types.Path) {
out = append(out, path...)
if kind := change.V.Kind(); kind == types.BoolKind || kind == types.StringKind || kind == types.NumberKind {
out = append(out, types.NewIndexPath(change.V))
if kind := change.Key.Kind(); kind == types.BoolKind || kind == types.StringKind || kind == types.NumberKind {
out = append(out, types.NewIndexPath(change.Key))
} else {
out = append(out, types.NewHashIndexPath(change.V.Hash()))
out = append(out, types.NewHashIndexPath(change.Key.Hash()))
}
return
}
@@ -62,10 +62,10 @@ func (sc setCandidate) get(k types.Value) types.Value {
func (sc setCandidate) pathConcat(change types.ValueChanged, path types.Path) (out types.Path) {
out = append(out, path...)
if kind := change.V.Kind(); kind == types.BoolKind || kind == types.StringKind || kind == types.NumberKind {
out = append(out, types.NewIndexPath(change.V))
if kind := change.Key.Kind(); kind == types.BoolKind || kind == types.StringKind || kind == types.NumberKind {
out = append(out, types.NewIndexPath(change.Key))
} else {
out = append(out, types.NewHashIndexPath(change.V.Hash()))
out = append(out, types.NewHashIndexPath(change.Key.Hash()))
}
return
}
@@ -92,9 +92,9 @@ func (sc structCandidate) get(key types.Value) types.Value {
func (sc structCandidate) pathConcat(change types.ValueChanged, path types.Path) (out types.Path) {
out = append(out, path...)
str, ok := change.V.(types.String)
str, ok := change.Key.(types.String)
if !ok {
d.Panic("Field names must be strings, not %s", types.TypeOf(change.V).Describe())
d.Panic("Field names must be strings, not %s", types.TypeOf(change.Key).Describe())
}
return append(out, types.NewFieldPath(string(str)))
}
+4 -4
View File
@@ -225,9 +225,9 @@ func (m *merger) threeWayMapMerge(a, b, parent types.Map, path types.Path) (merg
defer updateProgress(m.progress)
switch change.ChangeType {
case types.DiffChangeAdded, types.DiffChangeModified:
return mapCandidate{target.getValue().(types.Map).Set(change.V, newVal)}
return mapCandidate{target.getValue().(types.Map).Set(change.Key, newVal)}
case types.DiffChangeRemoved:
return mapCandidate{target.getValue().(types.Map).Remove(change.V)}
return mapCandidate{target.getValue().(types.Map).Remove(change.Key)}
default:
panic("Not Reached")
}
@@ -255,7 +255,7 @@ func (m *merger) threeWayStructMerge(a, b, parent types.Struct, path types.Path)
defer updateProgress(m.progress)
// Right now, this always iterates over all fields to create a new Struct, because there's no API for adding/removing a field from an existing struct type.
targetVal := target.getValue().(types.Struct)
if f, ok := change.V.(types.String); ok {
if f, ok := change.Key.(types.String); ok {
field := string(f)
data := types.StructData{}
targetVal.IterFields(func(name string, v types.Value) {
@@ -268,7 +268,7 @@ func (m *merger) threeWayStructMerge(a, b, parent types.Struct, path types.Path)
}
return structCandidate{types.NewStruct(targetVal.Name(), data)}
}
panic(fmt.Errorf("Bad key type in diff: %s", types.TypeOf(change.V).Describe()))
panic(fmt.Errorf("Bad key type in diff: %s", types.TypeOf(change.Key).Describe()))
}
return m.threeWayOrderedSequenceMerge(structCandidate{a}, structCandidate{b}, structCandidate{parent}, apply, path)
}
+15 -13
View File
@@ -33,30 +33,30 @@ func (m *merger) threeWayOrderedSequenceMerge(a, b, parent candidate, apply appl
aChange, bChange := types.ValueChanged{}, types.ValueChanged{}
for {
// Get the next change from both a and b. If either diff(a, parent) or diff(b, parent) is complete, aChange or bChange will get an empty types.ValueChanged containing a nil Value. Generally, though, this allows us to proceed through both diffs in (key) order, considering the "current" change from both diffs at the same time.
if aChange.V == nil {
if aChange.Key == nil {
aChange = <-aChangeChan
}
if bChange.V == nil {
if bChange.Key == nil {
bChange = <-bChangeChan
}
// Both channels are producing zero values, so we're done.
if aChange.V == nil && bChange.V == nil {
if aChange.Key == nil && bChange.Key == nil {
break
}
// Since diff generates changes in key-order, and we never skip over a change without processing it, we can simply compare the keys at which aChange and bChange occurred to determine if either is safe to apply to the merge result without further processing. This is because if, e.g. aChange.V.Less(bChange.V), we know that the diff of b will never generate a change at that key. If it was going to, it would have done so on an earlier iteration of this loop and been processed at that time.
// It's also obviously OK to apply a change if only one diff is generating any changes, e.g. aChange.V is non-nil and bChange.V is nil.
if aChange.V != nil && (bChange.V == nil || aChange.V.Less(bChange.V)) {
merged = apply(merged, aChange, a.get(aChange.V))
if aChange.Key != nil && (bChange.Key == nil || aChange.Key.Less(bChange.Key)) {
merged = apply(merged, aChange, a.get(aChange.Key))
aChange = types.ValueChanged{}
continue
} else if bChange.V != nil && (aChange.V == nil || bChange.V.Less(aChange.V)) {
merged = apply(merged, bChange, b.get(bChange.V))
} else if bChange.Key != nil && (aChange.Key == nil || bChange.Key.Less(aChange.Key)) {
merged = apply(merged, bChange, b.get(bChange.Key))
bChange = types.ValueChanged{}
continue
}
if !aChange.V.Equals(bChange.V) {
if !aChange.Key.Equals(bChange.Key) {
d.Panic("Diffs have skewed!") // Sanity check.
}
@@ -72,11 +72,12 @@ func (m *merger) threeWayOrderedSequenceMerge(a, b, parent candidate, apply appl
func (m *merger) mergeChanges(aChange, bChange types.ValueChanged, a, b, p candidate, apply applyFunc, path types.Path) (change types.ValueChanged, mergedVal types.Value, err error) {
path = a.pathConcat(aChange, path)
aValue, bValue := a.get(aChange.V), b.get(bChange.V)
aValue, bValue := a.get(aChange.Key), b.get(bChange.Key)
// If the two diffs generate different kinds of changes at the same key, conflict.
if aChange.ChangeType != bChange.ChangeType {
if change, mergedVal, ok := m.resolve(aChange.ChangeType, bChange.ChangeType, aValue, bValue, path); ok {
return types.ValueChanged{change, aChange.V}, mergedVal, nil
// TODO: Correctly encode Old/NewValue with this change report. https://github.com/attic-labs/noms/issues/3467
return types.ValueChanged{change, aChange.Key, nil, nil}, mergedVal, nil
}
return change, nil, newMergeConflict("Conflict:\n%s\nvs\n%s\n", describeChange(aChange), describeChange(bChange))
}
@@ -90,14 +91,15 @@ func (m *merger) mergeChanges(aChange, bChange types.ValueChanged, a, b, p candi
if !unmergeable(aValue, bValue) {
// TODO: Add concurrency.
var err error
if mergedVal, err = m.threeWay(aValue, bValue, p.get(aChange.V), path); err == nil {
if mergedVal, err = m.threeWay(aValue, bValue, p.get(aChange.Key), path); err == nil {
return aChange, mergedVal, nil
}
return change, nil, err
}
if change, mergedVal, ok := m.resolve(aChange.ChangeType, bChange.ChangeType, aValue, bValue, path); ok {
return types.ValueChanged{change, aChange.V}, mergedVal, nil
// TODO: Correctly encode Old/NewValue with this change report. https://github.com/attic-labs/noms/issues/3467
return types.ValueChanged{change, aChange.Key, nil, nil}, mergedVal, nil
}
return change, nil, newMergeConflict("Conflict:\n%s = %s\nvs\n%s = %s", describeChange(aChange), types.EncodedValue(aValue), describeChange(bChange), types.EncodedValue(bValue))
}
@@ -118,5 +120,5 @@ func describeChange(change types.ValueChanged) string {
case types.DiffChangeRemoved:
op = "removed"
}
return fmt.Sprintf("%s %s", op, types.EncodedValue(change.V))
return fmt.Sprintf("%s %s", op, types.EncodedValue(change.Key))
}
+4
View File
@@ -79,6 +79,10 @@ func (ml mapLeafSequence) getKey(idx int) orderedKey {
return newOrderedKey(ml.data[idx].key)
}
func (ml mapLeafSequence) getValue(idx int) Value {
return ml.data[idx].value
}
// Collection interface
func (ml mapLeafSequence) Len() uint64 {
return uint64(len(ml.data))
+3 -3
View File
@@ -310,11 +310,11 @@ func accumulateMapDiffChanges(m1, m2 Map) (added []Value, removed []Value, modif
}()
for change := range changes {
if change.ChangeType == DiffChangeAdded {
added = append(added, change.V)
added = append(added, change.Key)
} else if change.ChangeType == DiffChangeRemoved {
removed = append(removed, change.V)
removed = append(removed, change.Key)
} else {
modified = append(modified, change.V)
modified = append(modified, change.Key)
}
}
return
+8
View File
@@ -80,6 +80,14 @@ func getCurrentKey(cur *sequenceCursor) orderedKey {
return seq.getKey(cur.idx)
}
func getMapValue(cur *sequenceCursor) Value {
if ml, ok := cur.seq.(mapLeafSequence); ok {
return ml.getValue(cur.idx)
}
return nil
}
// If |vw| is not nil, chunks will be eagerly written as they're created. Otherwise they are
// written when the root is written.
func newOrderedMetaSequenceChunkFn(kind NomsKind, vr ValueReader) makeChunkFn {
+7 -7
View File
@@ -20,8 +20,8 @@ const (
)
type ValueChanged struct {
ChangeType DiffChangeType
V Value
ChangeType DiffChangeType
Key, OldValue, NewValue Value
}
func sendChange(changes chan<- ValueChanged, stopChan <-chan struct{}, change ValueChanged) bool {
@@ -170,17 +170,17 @@ func orderedSequenceDiffLeftRight(last orderedSequence, current orderedSequence,
lastKey := getCurrentKey(lastCur)
currentKey := getCurrentKey(currentCur)
if currentKey.Less(lastKey) {
if !sendChange(changes, stopChan, ValueChanged{DiffChangeAdded, currentKey.v}) {
if !sendChange(changes, stopChan, ValueChanged{DiffChangeAdded, currentKey.v, nil, getMapValue(currentCur)}) {
return false
}
currentCur.advance()
} else if lastKey.Less(currentKey) {
if !sendChange(changes, stopChan, ValueChanged{DiffChangeRemoved, lastKey.v}) {
if !sendChange(changes, stopChan, ValueChanged{DiffChangeRemoved, lastKey.v, getMapValue(lastCur), nil}) {
return false
}
lastCur.advance()
} else {
if !sendChange(changes, stopChan, ValueChanged{DiffChangeModified, lastKey.v}) {
if !sendChange(changes, stopChan, ValueChanged{DiffChangeModified, lastKey.v, getMapValue(lastCur), getMapValue(currentCur)}) {
return false
}
lastCur.advance()
@@ -190,13 +190,13 @@ func orderedSequenceDiffLeftRight(last orderedSequence, current orderedSequence,
}
for lastCur.valid() {
if !sendChange(changes, stopChan, ValueChanged{DiffChangeRemoved, getCurrentKey(lastCur).v}) {
if !sendChange(changes, stopChan, ValueChanged{DiffChangeRemoved, getCurrentKey(lastCur).v, getMapValue(lastCur), nil}) {
return false
}
lastCur.advance()
}
for currentCur.valid() {
if !sendChange(changes, stopChan, ValueChanged{DiffChangeAdded, getCurrentKey(currentCur).v}) {
if !sendChange(changes, stopChan, ValueChanged{DiffChangeAdded, getCurrentKey(currentCur).v, nil, getMapValue(currentCur)}) {
return false
}
currentCur.advance()
+7 -7
View File
@@ -48,11 +48,11 @@ func accumulateOrderedSequenceDiffChanges(o1, o2 orderedSequence, df diffFn) (ad
}()
for change := range changes {
if change.ChangeType == DiffChangeAdded {
added = append(added, change.V)
added = append(added, change.Key)
} else if change.ChangeType == DiffChangeRemoved {
removed = append(removed, change.V)
removed = append(removed, change.Key)
} else {
modified = append(modified, change.V)
modified = append(modified, change.Key)
}
}
return
@@ -204,11 +204,11 @@ func TestOrderedSequenceDiffWithMetaNodeGap(t *testing.T) {
}()
expected := []ValueChanged{
{DiffChangeAdded, Number(3)},
{DiffChangeAdded, Number(4)},
{DiffChangeAdded, Number(3), nil, nil},
{DiffChangeAdded, Number(4), nil, nil},
{},
{DiffChangeRemoved, Number(3)},
{DiffChangeRemoved, Number(4)},
{DiffChangeRemoved, Number(3), nil, nil},
{DiffChangeRemoved, Number(4), nil, nil},
}
i := 0
+2 -2
View File
@@ -244,9 +244,9 @@ func accumulateSetDiffChanges(s1, s2 Set) (added []Value, removed []Value) {
}()
for change := range changes {
if change.ChangeType == DiffChangeAdded {
added = append(added, change.V)
added = append(added, change.Key)
} else if change.ChangeType == DiffChangeRemoved {
removed = append(removed, change.V)
removed = append(removed, change.Key)
}
}
return
+5 -5
View File
@@ -267,15 +267,15 @@ func (s Struct) Diff(last Struct, changes chan<- ValueChanged, closeChan <-chan
var change ValueChanged
if fn1 == fn2 {
if !s.values[i1].Equals(last.values[i2]) {
change = ValueChanged{ChangeType: DiffChangeModified, V: String(fn1)}
change = ValueChanged{DiffChangeModified, String(fn1), last.values[i2], s.values[i1]}
}
i1++
i2++
} else if fn1 < fn2 {
change = ValueChanged{ChangeType: DiffChangeAdded, V: String(fn1)}
change = ValueChanged{DiffChangeAdded, String(fn1), nil, s.values[i1]}
i1++
} else {
change = ValueChanged{ChangeType: DiffChangeRemoved, V: String(fn2)}
change = ValueChanged{DiffChangeRemoved, String(fn2), last.values[i2], nil}
i2++
}
@@ -285,13 +285,13 @@ func (s Struct) Diff(last Struct, changes chan<- ValueChanged, closeChan <-chan
}
for ; i1 < len(fn1); i1++ {
if !sendChange(changes, closeChan, ValueChanged{ChangeType: DiffChangeAdded, V: String(fn1[i1])}) {
if !sendChange(changes, closeChan, ValueChanged{DiffChangeAdded, String(fn1[i1]), nil, s.values[i1]}) {
return
}
}
for ; i2 < len(fn2); i2++ {
if !sendChange(changes, closeChan, ValueChanged{ChangeType: DiffChangeRemoved, V: String(fn2[i2])}) {
if !sendChange(changes, closeChan, ValueChanged{DiffChangeRemoved, String(fn2[i2]), last.values[i2], nil}) {
return
}
}
+37 -13
View File
@@ -102,6 +102,21 @@ func TestGenericStructDelete(t *testing.T) {
assert.True(s5.Equals(s6))
}
func assertValueChangeEqual(assert *assert.Assertions, c1, c2 ValueChanged) {
assert.Equal(c1.ChangeType, c2.ChangeType)
assert.Equal(EncodedValue(c1.Key), EncodedValue(c2.Key))
if c1.NewValue == nil {
assert.Nil(c2.NewValue)
} else {
assert.Equal(EncodedValue(c1.NewValue), EncodedValue(c2.NewValue))
}
if c1.OldValue == nil {
assert.Nil(c2.OldValue)
} else {
assert.Equal(EncodedValue(c1.OldValue), EncodedValue(c2.OldValue))
}
}
func TestStructDiff(t *testing.T) {
assert := assert.New(t)
@@ -113,14 +128,14 @@ func TestStructDiff(t *testing.T) {
}()
i := 0
for change := range changes {
assert.Equal(expect[i], change)
assertValueChangeEqual(assert, expect[i], change)
i++
}
assert.Equal(len(expect), i, "Wrong number of changes")
}
vc := func(ct DiffChangeType, fieldName string) ValueChanged {
return ValueChanged{ChangeType: ct, V: String(fieldName)}
vc := func(ct DiffChangeType, fieldName string, oldV, newV Value) ValueChanged {
return ValueChanged{ct, String(fieldName), oldV, newV}
}
s1 := NewStruct("", StructData{"a": Bool(true), "b": String("hi"), "c": Number(4)})
@@ -128,25 +143,25 @@ func TestStructDiff(t *testing.T) {
assertDiff([]ValueChanged{},
s1, NewStruct("", StructData{"a": Bool(true), "b": String("hi"), "c": Number(4)}))
assertDiff([]ValueChanged{vc(DiffChangeModified, "a"), vc(DiffChangeModified, "b")},
assertDiff([]ValueChanged{vc(DiffChangeModified, "a", Bool(false), Bool(true)), vc(DiffChangeModified, "b", String("bye"), String("hi"))},
s1, NewStruct("", StructData{"a": Bool(false), "b": String("bye"), "c": Number(4)}))
assertDiff([]ValueChanged{vc(DiffChangeModified, "b"), vc(DiffChangeModified, "c")},
assertDiff([]ValueChanged{vc(DiffChangeModified, "b", String("bye"), String("hi")), vc(DiffChangeModified, "c", Number(5), Number(4))},
s1, NewStruct("", StructData{"a": Bool(true), "b": String("bye"), "c": Number(5)}))
assertDiff([]ValueChanged{vc(DiffChangeModified, "a"), vc(DiffChangeModified, "c")},
assertDiff([]ValueChanged{vc(DiffChangeModified, "a", Bool(false), Bool(true)), vc(DiffChangeModified, "c", Number(10), Number(4))},
s1, NewStruct("", StructData{"a": Bool(false), "b": String("hi"), "c": Number(10)}))
assertDiff([]ValueChanged{vc(DiffChangeAdded, "a")},
assertDiff([]ValueChanged{vc(DiffChangeAdded, "a", nil, Bool(true))},
s1, NewStruct("NewType", StructData{"b": String("hi"), "c": Number(4)}))
assertDiff([]ValueChanged{vc(DiffChangeAdded, "b")},
assertDiff([]ValueChanged{vc(DiffChangeAdded, "b", nil, String("hi"))},
s1, NewStruct("NewType", StructData{"a": Bool(true), "c": Number(4)}))
assertDiff([]ValueChanged{vc(DiffChangeRemoved, "Z")},
assertDiff([]ValueChanged{vc(DiffChangeRemoved, "Z", Number(17), nil)},
s1, NewStruct("NewType", StructData{"Z": Number(17), "a": Bool(true), "b": String("hi"), "c": Number(4)}))
assertDiff([]ValueChanged{vc(DiffChangeAdded, "b"), vc(DiffChangeRemoved, "d")},
assertDiff([]ValueChanged{vc(DiffChangeAdded, "b", nil, String("hi")), vc(DiffChangeRemoved, "d", Number(5), nil)},
s1, NewStruct("NewType", StructData{"a": Bool(true), "c": Number(4), "d": Number(5)}))
s2 := NewStruct("", StructData{
@@ -162,21 +177,30 @@ func TestStructDiff(t *testing.T) {
"c": NewSet(Number(0), Number(1), String("foo")),
}))
assertDiff([]ValueChanged{vc(DiffChangeModified, "a"), vc(DiffChangeModified, "b")},
assertDiff([]ValueChanged{
vc(DiffChangeModified, "a", NewList(Number(1), Number(1)), NewList(Number(0), Number(1))),
vc(DiffChangeModified, "b", NewMap(String("foo"), Bool(true), String("bar"), Bool(true)), NewMap(String("foo"), Bool(false), String("bar"), Bool(true))),
},
s2, NewStruct("", StructData{
"a": NewList(Number(1), Number(1)),
"b": NewMap(String("foo"), Bool(true), String("bar"), Bool(true)),
"c": NewSet(Number(0), Number(1), String("foo")),
}))
assertDiff([]ValueChanged{vc(DiffChangeModified, "a"), vc(DiffChangeModified, "c")},
assertDiff([]ValueChanged{
vc(DiffChangeModified, "a", NewList(Number(0)), NewList(Number(0), Number(1))),
vc(DiffChangeModified, "c", NewSet(Number(0), Number(2), String("foo")), NewSet(Number(0), Number(1), String("foo"))),
},
s2, NewStruct("", StructData{
"a": NewList(Number(0)),
"b": NewMap(String("foo"), Bool(false), String("bar"), Bool(true)),
"c": NewSet(Number(0), Number(2), String("foo")),
}))
assertDiff([]ValueChanged{vc(DiffChangeModified, "b"), vc(DiffChangeModified, "c")},
assertDiff([]ValueChanged{
vc(DiffChangeModified, "b", NewMap(String("boo"), Bool(false), String("bar"), Bool(true)), NewMap(String("foo"), Bool(false), String("bar"), Bool(true))),
vc(DiffChangeModified, "c", NewSet(Number(0), Number(1), String("bar")), NewSet(Number(0), Number(1), String("foo"))),
},
s2, NewStruct("", StructData{
"a": NewList(Number(0), Number(1)),
"b": NewMap(String("boo"), Bool(false), String("bar"), Bool(true)),