rtree: always store compact remote URLs in the index file

Includes a refactor to use a type RemoteURL with a more natural API.

6 files changed, 90 insertions, 65 deletions

diff --git a/internal/rtree/get_test.go b/internal/rtree/get_test.go
index bdd0f2e..a438234 100644
--- a/internal/rtree/get_test.go
+++ b/internal/rtree/get_test.go
@@ -60,24 +60,27 @@ func TestGetExistingRepoWithoutShortcut(t *testing.T) {
 func TestGetNewRepo(t *testing.T) {
 	target := filepath.Join(RootPath, "/github.com/another/repo")
 
-	Test{
-		Args:            []string{"get", "gh:another/repo"},
-		Index:           testIndexWithTwoRepos,
-		ExpectOutput:    target + "\n",
-		ExpectExecution: Recorded("git clone gh:another/repo " + target),
-		ExpectIndex: &Index{
-			Repos: []*Repo{
-				{
-					CheckoutPath: "github.com/another/repo",
-					Remotes: []Remote{
-						{Name: "origin", URL: "gh:another/repo"},
+	for _, remoteURL := range []string{"gh:another/repo", "https://github.com/another/repo"} {
+		Test{
+			Args:            []string{"get", remoteURL},
+			Index:           testIndexWithTwoRepos,
+			ExpectOutput:    target + "\n",
+			ExpectExecution: Recorded("git clone gh:another/repo " + target),
+			ExpectIndex: &Index{
+				Repos: []*Repo{
+					{
+						CheckoutPath: "github.com/another/repo",
+						Remotes: []Remote{
+							//regardless of the remote URL used, we expect the contracted form to be used
+							{Name: "origin", URL: "gh:another/repo"},
+						},
 					},
+					testIndexWithTwoRepos.Repos[0],
+					testIndexWithTwoRepos.Repos[1],
 				},
-				testIndexWithTwoRepos.Repos[0],
-				testIndexWithTwoRepos.Repos[1],
 			},
-		},
-	}.Run(t)
+		}.Run(t)
+	}
 }
 
 func TestGetNewForkAsRemote(t *testing.T) {
diff --git a/internal/rtree/index.go b/internal/rtree/index.go
index 4ca7a41..64c8c74 100644
--- a/internal/rtree/index.go
+++ b/internal/rtree/index.go
@@ -146,10 +146,10 @@ func (i *Index) Rebuild() error {
 		var remoteURLs []string
 		for _, remote := range repo.Remotes {
 			if remote.Name == "origin" {
-				remoteURLs = []string{remote.URL}
+				remoteURLs = []string{remote.URL.CompactURL()}
 				break
 			}
-			remoteURLs = append(remoteURLs, remote.URL)
+			remoteURLs = append(remoteURLs, remote.URL.CompactURL())
 		}
 
 		var selection string
@@ -212,12 +212,12 @@ func (i *Index) Rebuild() error {
 //FindRepo locates the repo with the given remote if it exists on disk or (if
 //allowClone is set) clones it and adds it to the index. This is the meat of
 //`rtree get`, and is also used by `rtree drop`.
-func (i *Index) FindRepo(remoteURL string, allowClone bool) (*Repo, error) {
+func (i *Index) FindRepo(rawRemoteURL string, allowClone bool) (*Repo, error) {
 	//make sure that stdout is not used for prompts
 	cli.Interface.StdoutProtected = true
 
-	expandedRemoteURL := ExpandRemoteURL(remoteURL)
-	basename := path.Base(expandedRemoteURL)
+	remoteURL := ParseRemoteURL(rawRemoteURL)
+	basename := path.Base(remoteURL.CanonicalURL())
 
 	//is this remote already checked out directly? also look for repos with the
 	//same basename that could be forks
@@ -225,11 +225,10 @@ func (i *Index) FindRepo(remoteURL string, allowClone bool) (*Repo, error) {
 	for _, repo := range i.Repos {
 		isCandidate := false
 		for _, remote := range repo.Remotes {
-			otherExpandedRemoteURL := ExpandRemoteURL(remote.URL)
-			if expandedRemoteURL == otherExpandedRemoteURL {
+			if remoteURL == remote.URL {
 				return repo, nil
 			}
-			if basename == path.Base(otherExpandedRemoteURL) {
+			if basename == path.Base(remote.URL.CanonicalURL()) {
 				isCandidate = true
 			}
 		}
@@ -310,7 +309,7 @@ func (i *Index) FindRepo(remoteURL string, allowClone bool) (*Repo, error) {
 	//report the existing remotes, and ask for the name of the new remote
 	prompt := "Existing remotes:\n"
 	for _, remote := range target.Remotes {
-		prompt += fmt.Sprintf("\t(%s) %s\n", remote.Name, remote.URL)
+		prompt += fmt.Sprintf("\t(%s) %s\n", remote.Name, remote.URL.CompactURL())
 	}
 	prompt += fmt.Sprintf("Enter remote name for %s:", remoteURL)
 	remoteName, err := cli.Interface.ReadLine(prompt)
@@ -319,7 +318,7 @@ func (i *Index) FindRepo(remoteURL string, allowClone bool) (*Repo, error) {
 	}
 
 	err = cli.Interface.Run(cli.Command{
-		Program: []string{"git", "remote", "add", remoteName, remoteURL},
+		Program: []string{"git", "remote", "add", remoteName, remoteURL.CompactURL()},
 		WorkDir: target.AbsolutePath(),
 	})
 	if err != nil {
@@ -363,7 +362,7 @@ func (i *Index) ImportRepo(dirPath string) error {
 	choices := make([]cli.Choice, len(repo.Remotes))
 	var checkoutPath string
 	for idx, remote := range repo.Remotes {
-		thisPath, err := CheckoutPathForRemoteURL(ExpandRemoteURL(remote.URL))
+		thisPath, err := remote.URL.CheckoutPath()
 		if err != nil {
 			return err
 		}
diff --git a/internal/rtree/main.go b/internal/rtree/main.go
index d1692cd..de92ead 100644
--- a/internal/rtree/main.go
+++ b/internal/rtree/main.go
@@ -144,7 +144,7 @@ func commandRemotes(index *Index) {
 	var items []string
 	for _, repo := range index.Repos {
 		for _, remote := range repo.Remotes {
-			items = append(items, remote.URL)
+			items = append(items, remote.URL.CompactURL())
 		}
 	}
 	cli.Interface.ShowResultsSorted(items)
diff --git a/internal/rtree/remote.go b/internal/rtree/remote.go
index b2b3340..484dd92 100644
--- a/internal/rtree/remote.go
+++ b/internal/rtree/remote.go
@@ -25,47 +25,55 @@ import (
 	"strings"
 )
 
-//ExpandRemoteURL derive the canonical URL for a given remote by substituting
-//aliases defined in the system-wide and user-global Git config. For example,
-//with
+//RemoteURL is the URL of a remote of a Git repository.
+type RemoteURL string
+
+//ParseRemoteURL parses the given remote URL by substituting aliases defined in
+//the system-wide and user-global Git config. For example, with
 //
 //    $ cat /etc/gitconfig
 //    [url "git://github.com/"]
 //    insteadOf = gh:
 //
-//and the input "gh:foo/bar", this function returns "git://github.com/foo/bar".
-func ExpandRemoteURL(remoteURL string) string {
+//and the input "gh:foo/bar", the result has a canonical URL of
+//"git://github.com/foo/bar".
+func ParseRemoteURL(input string) RemoteURL {
 	var best *RemoteAlias
 	for _, current := range RemoteAliases {
-		if strings.HasPrefix(remoteURL, current.Alias) {
+		if strings.HasPrefix(input, current.Alias) {
 			if best == nil || len(best.Alias) < len(current.Alias) {
 				best = current
 			}
 		}
 	}
 	if best == nil {
-		return remoteURL
+		return RemoteURL(input)
 	}
-	return best.Replacement + strings.TrimPrefix(remoteURL, best.Alias)
+	return RemoteURL(best.Replacement + strings.TrimPrefix(input, best.Alias))
+}
+
+//CanonicalURL returns the URL where the remote will be fetched from.
+func (u RemoteURL) CanonicalURL() string {
+	return string(u)
 }
 
-//ContractRemoteURL takes the canonical URL for a given remote and shortens it
-//as much as possible by substituting an alias from the system-wide and
-//user-global Git config. This function is pretty much the reverse of
-//ExpandRemoteURL().
-func ContractRemoteURL(remoteURL string) string {
+//CompactURL returns the most compact representation of this remote URL,
+//obtained by substituting the longest matching alias defined in the
+//system-wide or user-global Git config. This function is mostly the reverse of
+//ParseRemoteURL().
+func (u RemoteURL) CompactURL() string {
 	var best *RemoteAlias
 	for _, current := range RemoteAliases {
-		if strings.HasPrefix(remoteURL, current.Replacement) {
+		if strings.HasPrefix(string(u), current.Replacement) {
 			if best == nil || len(best.Replacement) < len(current.Replacement) {
 				best = current
 			}
 		}
 	}
 	if best == nil {
-		return remoteURL
+		return string(u)
 	}
-	return best.Alias + strings.TrimPrefix(remoteURL, best.Replacement)
+	return best.Alias + strings.TrimPrefix(string(u), best.Replacement)
 }
 
 //This regex recognizes the scp-like syntax for git remotes
@@ -73,22 +81,37 @@ func ContractRemoteURL(remoteURL string) string {
 //section of man:git-clone(1).
 var scpSyntaxRx = regexp.MustCompile(`^(?:[^/@:]+@)?([^/:]+\.[^/:]+):(.+)$`)
 
-//CheckoutPathForRemoteURL derives the checkout path for a remote URL that has
-//already been expanded with ExpandRemoteURL() if necessary.
+//CheckoutPath derives the checkout path for a remote URL.
 //
-//  "https://example.org/foo/bar" -> "example.org/foo/bar"
-//  "git@example.org:foo/bar"     -> "example.org/foo/bar"
+//    RemoteURL("https://example.org/foo/bar") -> "example.org/foo/bar"
+//    RemoteURL("git@example.org:foo/bar")     -> "example.org/foo/bar"
 //
-func CheckoutPathForRemoteURL(remoteURL string) (string, error) {
-	match := scpSyntaxRx.FindStringSubmatch(remoteURL)
+func (u RemoteURL) CheckoutPath() (string, error) {
+	match := scpSyntaxRx.FindStringSubmatch(u.CanonicalURL())
 	if match != nil {
 		//match[1] is the hostname, match[2] is the path to the repo
 		return filepath.Join(match[1], match[2]), nil
 	}
 
-	u, err := url.Parse(remoteURL)
+	parsed, err := url.Parse(u.CanonicalURL())
 	if err != nil {
 		return "", err
 	}
-	return filepath.Join(u.Hostname(), u.Path), nil
+	return filepath.Join(parsed.Hostname(), parsed.Path), nil
+}
+
+//MarshalYAML implements the yaml.Marshaler interface.
+func (u RemoteURL) MarshalYAML() (interface{}, error) {
+	//store URLs in the index in the compact format
+	return u.CompactURL(), nil
+}
+
+//UnmarshalYAML implements the yaml.Unmarshaler interface.
+func (u *RemoteURL) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	var s string
+	err := unmarshal(&s)
+	if err == nil {
+		*u = ParseRemoteURL(s)
+	}
+	return err
 }
diff --git a/internal/rtree/remote_test.go b/internal/rtree/remote_test.go
index 3841c5b..5910a5c 100644
--- a/internal/rtree/remote_test.go
+++ b/internal/rtree/remote_test.go
@@ -28,7 +28,7 @@ var remoteAliasesForExpansionTest = []*RemoteAlias{
 	{Alias: "test:", Replacement: "test:test:"},
 }
 
-var testExpansions = map[string]string{
+var testExpansions = map[string]RemoteURL{
 	"gh:foo/bar":                 "https://github.com/foo/bar",
 	"gh:f:bar":                   "https://github.com/foo/bar",
 	"gh:b:foo":                   "https://github.com/bar/foo",
@@ -37,10 +37,10 @@ var testExpansions = map[string]string{
 	"https://github.com/foo/bar": "https://github.com/foo/bar",    //no expansion at all
 }
 
-func TestExpandRemoteURL(t *testing.T) {
+func TestParseRemoteURL(t *testing.T) {
 	RemoteAliases = remoteAliasesForExpansionTest
 	for input, expected := range testExpansions {
-		actual := ExpandRemoteURL(input)
+		actual := ParseRemoteURL(input)
 		if actual != expected {
 			t.Errorf("expected %q to expand into %q, but got %q", input, expected, actual)
 		}
@@ -48,7 +48,7 @@ func TestExpandRemoteURL(t *testing.T) {
 }
 
 //Most of those are just reversed from `testExpansions`.
-var testContractions = map[string]string{
+var testContractions = map[RemoteURL]string{
 	"https://github.com/foo/bar":      "gh:f:bar",
 	"https://github.com/bar/foo":      "gh:b:foo",
 	"https://github.com/qux/foobar":   "gh:qux/foobar",
@@ -57,10 +57,10 @@ var testContractions = map[string]string{
 	"git://somewhereelse.com/foo/bar": "git://somewhereelse.com/foo/bar",
 }
 
-func TestContractRemoteURL(t *testing.T) {
+func TestCompactRemoteURL(t *testing.T) {
 	RemoteAliases = remoteAliasesForExpansionTest
 	for input, expected := range testContractions {
-		actual := ContractRemoteURL(input)
+		actual := input.CompactURL()
 		if actual != expected {
 			t.Errorf("expected %q to contract into %q, but got %q", input, expected, actual)
 		}
diff --git a/internal/rtree/repo.go b/internal/rtree/repo.go
index 4af51c1..8529bfa 100644
--- a/internal/rtree/repo.go
+++ b/internal/rtree/repo.go
@@ -40,8 +40,8 @@ type Repo struct {
 
 //Remote describes a remote that is configured in a Repo.
 type Remote struct {
-	Name string `yaml:"name"`
-	URL  string `yaml:"url"`
+	Name string    `yaml:"name"`
+	URL  RemoteURL `yaml:"url"`
 }
 
 //AbsolutePath returns the absolute CheckoutPath of this repo.
@@ -73,7 +73,7 @@ func NewRepoFromAbsolutePath(path string) (repo Repo, err error) {
 		}
 		repo.Remotes = append(repo.Remotes, Remote{
 			Name: match[1],
-			URL:  match[2],
+			URL:  ParseRemoteURL(match[2]),
 		})
 	}
 	return
@@ -81,8 +81,8 @@ func NewRepoFromAbsolutePath(path string) (repo Repo, err error) {
 
 //NewRepoFromRemoteURL initializes a Repo instance for checking out a remote
 //for the first time. The checkout does not happen until Checkout() is called.
-func NewRepoFromRemoteURL(remoteURL string) (Repo, error) {
-	checkoutPath, err := CheckoutPathForRemoteURL(ExpandRemoteURL(remoteURL))
+func NewRepoFromRemoteURL(remoteURL RemoteURL) (Repo, error) {
+	checkoutPath, err := remoteURL.CheckoutPath()
 	return Repo{
 		CheckoutPath: checkoutPath,
 		Remotes: []Remote{
@@ -130,7 +130,7 @@ func ForeachPhysicalRepo(action func(repo Repo) error) error {
 //working copy will only be initialized if there is an "origin" remote.
 func (r Repo) Checkout() error {
 	//check if we have an "origin" remote to clone from
-	var originURL string
+	var originURL RemoteURL
 	for _, remote := range r.Remotes {
 		if remote.Name == "origin" {
 			originURL = remote.URL
@@ -148,7 +148,7 @@ func (r Repo) Checkout() error {
 		cli.Interface.ShowWarning(`will not checkout anything since there is no remote named "origin"`)
 	} else {
 		err := cli.Interface.Run(cli.Command{
-			Program: []string{"git", "clone", originURL, r.AbsolutePath()},
+			Program: []string{"git", "clone", originURL.CompactURL(), r.AbsolutePath()},
 		})
 		if err != nil {
 			return err
@@ -159,7 +159,7 @@ func (r Repo) Checkout() error {
 	for _, remote := range r.Remotes {
 		if remote.Name != "origin" {
 			err := cli.Interface.Run(cli.Command{
-				Program: []string{"git", "remote", "add", remote.Name, remote.URL},
+				Program: []string{"git", "remote", "add", remote.Name, remote.URL.CompactURL()},
 				WorkDir: r.AbsolutePath(),
 			})
 			if err != nil {