diff --git a/selection.go b/selection.go index cab3e7798f..d1fe95d785 100644 --- a/selection.go +++ b/selection.go @@ -173,30 +173,33 @@ func (u *unselected) Pop() (v interface{}) { return v } -// remove takes a ProjectIdentifier out of the priority queue, if present. +// remove takes a bimodalIdentifier out of the priority queue, if present. Only +// the first matching bmi will be removed. // -// There are, generally, two ways this gets called: to remove the unselected -// item from the front of the queue while that item is being unselected, and -// during backtracking, when an item becomes unnecessary because the item that -// induced it was popped off. +// There are two events that cause this to be called: bmi selection, when the +// bmi at the front of the queue is removed, and backtracking, when a bmi +// becomes unnecessary because the dependency that induced it was backtracked +// and popped off. // // The worst case for both of these is O(n), but in practice the first case is -// be O(1), as we iterate the queue from front to back. +// O(1), as we iterate the queue from front to back. func (u *unselected) remove(bmi bimodalIdentifier) { - for k, pi := range u.sl { - if pi.id.eq(bmi.id) { + plen := len(bmi.pl) +outer: + for i, pi := range u.sl { + if pi.id.eq(bmi.id) && len(pi.pl) == plen { // Simple slice comparison - assume they're both sorted the same - for k, pkg := range pi.pl { - if bmi.pl[k] != pkg { - break + for i2, pkg := range pi.pl { + if bmi.pl[i2] != pkg { + continue outer } } - if k == len(u.sl)-1 { + if i == len(u.sl)-1 { // if we're on the last element, just pop, no splice u.sl = u.sl[:len(u.sl)-1] } else { - u.sl = append(u.sl[:k], u.sl[k+1:]...) + u.sl = append(u.sl[:i], u.sl[i+1:]...) } break } diff --git a/selection_test.go b/selection_test.go new file mode 100644 index 0000000000..6fb727827c --- /dev/null +++ b/selection_test.go @@ -0,0 +1,59 @@ +package gps + +import ( + "reflect" + "testing" +) + +// Regression test for https://github.com/sdboyer/gps/issues/174 +func TestUnselectedRemoval(t *testing.T) { + // We don't need a comparison function for this test + bmi1 := bimodalIdentifier{ + id: mkPI("foo"), + pl: []string{"foo", "bar"}, + } + bmi2 := bimodalIdentifier{ + id: mkPI("foo"), + pl: []string{"foo", "bar", "baz"}, + } + bmi3 := bimodalIdentifier{ + id: mkPI("foo"), + pl: []string{"foo"}, + } + + u := &unselected{ + sl: []bimodalIdentifier{bmi1, bmi2, bmi3}, + } + + u.remove(bimodalIdentifier{ + id: mkPI("other"), + pl: []string{"other"}, + }) + + if len(u.sl) != 3 { + t.Fatalf("len of unselected slice should have been 2 after no-op removal, got %v", len(u.sl)) + } + + u.remove(bmi3) + want := []bimodalIdentifier{bmi1, bmi2} + if len(u.sl) != 2 { + t.Fatalf("removal of matching bmi did not work, slice should have 2 items but has %v", len(u.sl)) + } + if !reflect.DeepEqual(u.sl, want) { + t.Fatalf("wrong item removed from slice:\n\t(GOT): %v\n\t(WNT): %v", u.sl, want) + } + + u.remove(bmi3) + if len(u.sl) != 2 { + t.Fatalf("removal of bmi w/non-matching packages should be a no-op but wasn't; slice should have 2 items but has %v", len(u.sl)) + } + + u.remove(bmi2) + want = []bimodalIdentifier{bmi1} + if len(u.sl) != 1 { + t.Fatalf("removal of matching bmi did not work, slice should have 1 items but has %v", len(u.sl)) + } + if !reflect.DeepEqual(u.sl, want) { + t.Fatalf("wrong item removed from slice:\n\t(GOT): %v\n\t(WNT): %v", u.sl, want) + } +}