From 5f2e16d59cf2ce681b215e98662c2ebb14f5e485 Mon Sep 17 00:00:00 2001
From: sam boyer <tech@samboyer.org>
Date: Fri, 3 Mar 2017 09:38:45 -0500
Subject: [PATCH] Check pl len during bmi removal from unsel queue

Fixes #174.
---
 selection.go      | 29 ++++++++++++-----------
 selection_test.go | 59 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+), 13 deletions(-)
 create mode 100644 selection_test.go

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)
+	}
+}