New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: simplify scheme lookup in internal/get/vcs.go #26123

Open
rsc opened this Issue Jun 29, 2018 · 1 comment

Comments

Projects
None yet
2 participants
@rsc
Contributor

rsc commented Jun 29, 2018

After CL 120998 is merged into the main tree, the vcs ping code will look like:

	if srv.ping {
		if scheme != "" {
			match["repo"] = scheme + "://" + match["repo"]
		} else {
			for _, scheme := range vcs.scheme {
				if security == web.Secure && !vcs.isSecureScheme(scheme) {
					continue
				}
				if vcs.pingCmd != "" && vcs.ping(scheme, match["repo"]) == nil {
					match["repo"] = scheme + "://" + match["repo"]
					goto Found
				}
			}
			// No scheme found. Fall back to the first one.
			match["repo"] = vcs.scheme[0] + "://" + match["repo"]
		Found:
		}
	}

I believe it can be simplified to:

	if srv.ping {
		scheme = vcs.scheme[0] // default to first scheme
		if vcs.pingCmd != "" {
			// If we know how to test schemes, scan to find one.
			for _, scm := range vcs.scheme {
			if security == web.Secure && !vcs.isSecureScheme(scm) {
				continue
			}
			if vcs.ping(scheme, match["repo"]) == nil {
				scheme = scm
				break
			}
		}
		match["repo"] = scheme + "://" + match["repo"]
	}

but I don't want to do that this late in the cycle. We should try the rewrite when the Go 1.12 cycle opens.

@rsc rsc added this to the Go1.12 milestone Jun 29, 2018

@rsc rsc added the early-in-cycle label Jun 29, 2018

@bcmills bcmills modified the milestones: Go1.12, Go1.13 Nov 15, 2018

@bcmills

This comment has been minimized.

Member

bcmills commented Nov 15, 2018

This fell off my radar. Will put it on my list for early in 1.13.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment