Merge pull request #1079 from sdboyer/warn-case-mismatch

Add satisfiability check for case variants

Author: sdboyer

internal/gps/identifier.go

@@ -217,6 +217,9 @@ type completeDep struct {
 	pl []string
 }
 
+// dependency represents an incomplete edge in the depgraph. It has a
+// fully-realized atom as the depender (the tail/source of the edge), and a set
+// of requirements that any atom to be attached at the head/target must satisfy.
 type dependency struct {
 	depender atom
 	dep      completeDep

internal/gps/manager_test.go

@@ -5,6 +5,7 @@
 package gps
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 	"io/ioutil"
@@ -13,9 +14,11 @@ import (
 	"path"
 	"path/filepath"
 	"runtime"
+	"sort"
 	"sync"
 	"sync/atomic"
 	"testing"
+	"text/tabwriter"
 	"time"
 
 	"github.com/golang/dep/internal/test"
@@ -349,8 +352,8 @@ func TestMgrMethodsFailWithBadPath(t *testing.T) {
 }
 
 type sourceCreationTestFixture struct {
-	roots              []ProjectIdentifier
-	urlcount, srccount int
+	roots               []ProjectIdentifier
+	namecount, srccount int
 }
 
 func (f sourceCreationTestFixture) run(t *testing.T) {
@@ -365,13 +368,33 @@ func (f sourceCreationTestFixture) run(t *testing.T) {
 		}
 	}
 
-	if len(sm.srcCoord.nameToURL) != f.urlcount {
-		t.Errorf("want %v names in the name->url map, but got %v. contents: \n%v", f.urlcount, len(sm.srcCoord.nameToURL), sm.srcCoord.nameToURL)
+	if len(sm.srcCoord.nameToURL) != f.namecount {
+		t.Errorf("want %v names in the name->url map, but got %v. contents: \n%v", f.namecount, len(sm.srcCoord.nameToURL), sm.srcCoord.nameToURL)
 	}
 
 	if len(sm.srcCoord.srcs) != f.srccount {
 		t.Errorf("want %v gateways in the sources map, but got %v", f.srccount, len(sm.srcCoord.srcs))
 	}
+
+	var keys []string
+	for k := range sm.srcCoord.nameToURL {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+
+	var buf bytes.Buffer
+	w := tabwriter.NewWriter(&buf, 0, 4, 2, ' ', 0)
+	fmt.Fprint(w, "NAME\tMAPPED URL\n")
+	for _, r := range keys {
+		fmt.Fprintf(w, "%s\t%s\n", r, sm.srcCoord.nameToURL[r])
+	}
+	w.Flush()
+	t.Log("\n", buf.String())
+
+	t.Log("SRC KEYS")
+	for k := range sm.srcCoord.srcs {
+		t.Log(k)
+	}
 }
 
 // This test is primarily about making sure that the logic around folding
@@ -390,16 +413,27 @@ func TestSourceCreationCounts(t *testing.T) {
 				mkPI("gopkg.in/sdboyer/gpkt.v2"),
 				mkPI("gopkg.in/sdboyer/gpkt.v3"),
 			},
-			urlcount: 6,
-			srccount: 3,
+			namecount: 6,
+			srccount:  3,
 		},
 		"gopkgin separation from github": {
 			roots: []ProjectIdentifier{
 				mkPI("gopkg.in/sdboyer/gpkt.v1"),
 				mkPI("github.com/sdboyer/gpkt"),
 			},
-			urlcount: 4,
-			srccount: 2,
+			namecount: 4,
+			srccount:  2,
+		},
+		"case variance across path and URL-based access": {
+			roots: []ProjectIdentifier{
+				ProjectIdentifier{ProjectRoot: ProjectRoot("github.com/sdboyer/gpkt"), Source: "https://github.com/Sdboyer/gpkt"},
+				ProjectIdentifier{ProjectRoot: ProjectRoot("github.com/sdboyer/gpkt"), Source: "https://github.com/SdbOyer/gpkt"},
+				mkPI("github.com/sdboyer/gpkt"),
+				ProjectIdentifier{ProjectRoot: ProjectRoot("github.com/sdboyer/gpkt"), Source: "https://github.com/sdboyeR/gpkt"},
+				mkPI("github.com/sdboyeR/gpkt"),
+			},
+			namecount: 6,
+			srccount:  1,
 		},
 	}
 
@@ -467,13 +501,70 @@ func TestGetSources(t *testing.T) {
 	})
 
 	// nine entries (of which three are dupes): for each vcs, raw import path,
-	// the https url, and the http url
-	if len(sm.srcCoord.nameToURL) != 9 {
-		t.Errorf("Should have nine discrete entries in the nameToURL map, got %v", len(sm.srcCoord.nameToURL))
+	// the https url, and the http url. also three more from case folding of
+	// github.com/Masterminds/VCSTestRepo -> github.com/masterminds/vcstestrepo
+	if len(sm.srcCoord.nameToURL) != 12 {
+		t.Errorf("Should have twelve discrete entries in the nameToURL map, got %v", len(sm.srcCoord.nameToURL))
 	}
 	clean()
 }
 
+func TestFSCaseSensitivityConvergesSources(t *testing.T) {
+	if testing.Short() {
+		t.Skip("Skipping slow test in short mode")
+	}
+
+	f := func(name string, pi1, pi2 ProjectIdentifier) {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+			sm, clean := mkNaiveSM(t)
+			defer clean()
+
+			sm.SyncSourceFor(pi1)
+			sg1, err := sm.srcCoord.getSourceGatewayFor(context.Background(), pi1)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			sm.SyncSourceFor(pi2)
+			sg2, err := sm.srcCoord.getSourceGatewayFor(context.Background(), pi2)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			path1 := sg1.src.(*gitSource).repo.LocalPath()
+			t.Log("path1:", path1)
+			stat1, err := os.Stat(path1)
+			if err != nil {
+				t.Fatal(err)
+			}
+			path2 := sg2.src.(*gitSource).repo.LocalPath()
+			t.Log("path2:", path2)
+			stat2, err := os.Stat(path2)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			same, count := os.SameFile(stat1, stat2), len(sm.srcCoord.srcs)
+			if same && count != 1 {
+				t.Log("are same, count", count)
+				t.Fatal("on case-insensitive filesystem, case-varying sources should have been folded together but were not")
+			}
+			if !same && count != 2 {
+				t.Log("not same, count", count)
+				t.Fatal("on case-sensitive filesystem, case-varying sources should not have been folded together, but were")
+			}
+		})
+	}
+
+	folded := mkPI("github.com/sdboyer/deptest").normalize()
+	casevar1 := mkPI("github.com/Sdboyer/deptest").normalize()
+	casevar2 := mkPI("github.com/SdboyeR/deptest").normalize()
+	f("folded first", folded, casevar1)
+	f("folded second", casevar1, folded)
+	f("both unfolded", casevar1, casevar2)
+}
+
 // Regression test for #32
 func TestGetInfoListVersionsOrdering(t *testing.T) {
 	// This test is quite slow, skip it on -short

internal/gps/satisfy.go

@@ -54,6 +54,9 @@ func (s *solver) check(a atomWithPackages, pkgonly bool) error {
 		if err = s.checkIdentMatches(a, dep); err != nil {
 			return err
 		}
+		if err = s.checkRootCaseConflicts(a, dep); err != nil {
+			return err
+		}
 		if err = s.checkDepsConstraintsAllowable(a, dep); err != nil {
 			return err
 		}
@@ -218,6 +221,57 @@ func (s *solver) checkIdentMatches(a atomWithPackages, cdep completeDep) error {
 	return nil
 }
 
+// checkRootCaseConflicts ensures that the ProjectRoot specified in the completeDep
+// does not have case conflicts with any existing dependencies.
+//
+// We only need to check the ProjectRoot, rather than any packages therein, as
+// the later check for package existence is case-sensitive.
+func (s *solver) checkRootCaseConflicts(a atomWithPackages, cdep completeDep) error {
+	pr := cdep.workingConstraint.Ident.ProjectRoot
+	hasConflict, current := s.sel.findCaseConflicts(pr)
+	if !hasConflict {
+		return nil
+	}
+
+	curid, _ := s.sel.getIdentFor(current)
+	deps := s.sel.getDependenciesOn(curid)
+	for _, d := range deps {
+		s.fail(d.depender.id)
+	}
+
+	// If a project has multiple packages that import each other, we treat that
+	// as establishing a canonical case variant for the ProjectRoot. It's possible,
+	// however, that that canonical variant is not the same one that others
+	// imported it under. If that's the situation, then we'll have arrived here
+	// when visiting the project, not its dependers, having misclassified its
+	// internal imports as external. That means the atomWithPackages will
+	// be the wrong case variant induced by the importers, and the cdep will be
+	// a link pointing back at the canonical case variant.
+	//
+	// If this is the case, use a special failure, wrongCaseFailure, that
+	// makes a stronger statement as to the correctness of case variants.
+	//
+	// TODO(sdboyer) This approach to marking failure is less than great, as
+	// this will mark the current atom as failed, as well, causing the
+	// backtracker to work through it. While that could prove fruitful, it's
+	// quite likely just to be wasted effort. Addressing this - if that's a good
+	// idea - would entail creating another path back out of checking to enable
+	// backjumping directly to the incorrect importers.
+	if current == a.a.id.ProjectRoot {
+		return &wrongCaseFailure{
+			correct: pr,
+			goal:    dependency{depender: a.a, dep: cdep},
+			badcase: deps,
+		}
+	}
+
+	return &caseMismatchFailure{
+		goal:    dependency{depender: a.a, dep: cdep},
+		current: current,
+		failsib: deps,
+	}
+}
+
 // checkPackageImportsFromDepExist ensures that, if the dep is already selected,
 // the newly-required set of packages being placed on it exist and are valid.
 func (s *solver) checkPackageImportsFromDepExist(a atomWithPackages, cdep completeDep) error {

internal/gps/selection.go

@@ -5,9 +5,19 @@
 package gps
 
 type selection struct {
+	// projects is a stack of the atoms that have currently been selected by the
+	// solver. It can also be thought of as the vertex set of the current
+	// selection graph.
 	projects []selected
-	deps     map[ProjectRoot][]dependency
-	vu       *versionUnifier
+	// deps records the set of dependers on a given ProjectRoot. It is
+	// essentially an adjacency list of *inbound* edges.
+	deps map[ProjectRoot][]dependency
+	// foldRoots records a mapping from a canonical, case-folded form of
+	// ProjectRoots to the particular case variant that has currently been
+	// selected.
+	foldRoots map[string]ProjectRoot
+	// The versoinUnifier in use for this solve run.
+	vu *versionUnifier
 }
 
 type selected struct {
@@ -59,13 +69,35 @@ func (s *selection) popSelection() (atomWithPackages, bool) {
 	return sel.a, sel.first
 }
 
+// findCaseConflicts checks to see if the given ProjectRoot has a
+// case-insensitive overlap with another, different ProjectRoot that's already
+// been picked.
+func (s *selection) findCaseConflicts(pr ProjectRoot) (bool, ProjectRoot) {
+	if current, has := s.foldRoots[toFold(string(pr))]; has && pr != current {
+		return true, current
+	}
+
+	return false, ""
+}
+
 func (s *selection) pushDep(dep dependency) {
-	s.deps[dep.dep.Ident.ProjectRoot] = append(s.deps[dep.dep.Ident.ProjectRoot], dep)
+	pr := dep.dep.Ident.ProjectRoot
+	deps := s.deps[pr]
+	if len(deps) == 0 {
+		s.foldRoots[toFold(string(pr))] = pr
+	}
+
+	s.deps[pr] = append(deps, dep)
 }
 
 func (s *selection) popDep(id ProjectIdentifier) (dep dependency) {
 	deps := s.deps[id.ProjectRoot]
-	dep, s.deps[id.ProjectRoot] = deps[len(deps)-1], deps[:len(deps)-1]
+	dlen := len(deps)
+	if dlen == 1 {
+		delete(s.foldRoots, toFold(string(id.ProjectRoot)))
+	}
+
+	dep, s.deps[id.ProjectRoot] = deps[dlen-1], deps[:dlen-1]
 	return dep
 }