General cleanup and docs

git/client.go

@@ -95,14 +95,36 @@ func (c *Client) Command(ctx context.Context, args ...string) (*Command, error)
 	return &Command{cmd}, nil
 }
 
+// CredentialPattern is used to indicate to AuthenticatedCommand which patterns git should match
+// against when trying to find credentials. It is a little overengineered as a type because we
+// want AuthenticatedCommand to have a clear complication error when this is not provided,
+// as opposed to using a string which might compile with `client.AuthenticatedCommand(ctx, "fetch")`.
+//
+// It is only usable when constructed by another function in the package because the empty pattern,
+// without insecure set to true, will result in an error in AuthenticatedCommand.
+//
+// Callers can currently opt-in to an insecure mode for backwards compatability by using
+// InsecureAllMatchingCredentialsPattern.
 type CredentialPattern struct {
 	insecure bool // should only be constructable via InsecureAllMatchingCredentialPattern
 	pattern  string
 }
 
+// InsecureAllMatchingCredentialsPattern allows for opting in to an insecure mode for backwards compatability.
 var InsecureAllMatchingCredentialsPattern = CredentialPattern{insecure: true, pattern: ""}
 var disallowedCredentialPattern = CredentialPattern{insecure: false, pattern: ""}
 
+// WM-TODO: Should this handle command modifiers, e.g. RepoDir being set in Clone
+// WM-TODO: Are there any funny remotes that might not resolve to a URL?
+// WM-TODO: This should probably have its own tests
+func CredentialPatternFromRemote(ctx context.Context, c *Client, remote string) (CredentialPattern, error) {
+	gitURL, err := c.GetRemoteURL(ctx, remote)
+	if err != nil {
+		return CredentialPattern{}, err
+	}
+	return CredentialPatternFromGitURL(gitURL)
+}
+
 // AuthenticatedCommand is a wrapper around Command that included configuration to use gh
 // as the credential helper for git.
 func (c *Client) AuthenticatedCommand(ctx context.Context, credentialPattern CredentialPattern, args ...string) (*Command, error) {
@@ -652,17 +674,6 @@ func (c *Client) Clone(ctx context.Context, cloneURL string, args []string, mods
 	return target, nil
 }
 
-// WM-TODO: Bit of a weird method to hang off the client?
-// WM-TODO: We need to make sure this handles command modifiers everywhere...
-// WM-TODO: Are there any funny refspec usages that might not resolve via get-url
-func (c *Client) CredentialPatternFromRemote(ctx context.Context, remote string) (CredentialPattern, error) {
-	gitURL, err := c.GetRemoteURL(ctx, remote)
-	if err != nil {
-		return CredentialPattern{}, err
-	}
-	return CredentialPatternFromGitURL(gitURL)
-}
-
 func resolveGitPath() (string, error) {
 	path, err := safeexec.LookPath("git")
 	if err != nil {

git/client_test.go

@@ -1499,17 +1499,16 @@ func TestCommandMocking(t *testing.T) {
 		return
 	}
 
-	// WM-TODO: maybe don't use 255, I only picked it because it was distinct
 	jsonVar, ok := os.LookupEnv("GH_HELPER_PROCESS_RICH_COMMANDS")
 	if !ok {
 		fmt.Fprint(os.Stderr, "missing GH_HELPER_PROCESS_RICH_COMMANDS")
-		os.Exit(255)
+		os.Exit(1)
 	}
 
 	var commands mockedCommands
 	if err := json.Unmarshal([]byte(jsonVar), &commands); err != nil {
 		fmt.Fprint(os.Stderr, "failed to unmarshal GH_HELPER_PROCESS_RICH_COMMANDS")
-		os.Exit(255)
+		os.Exit(1)
 	}
 
 	// The discarded args are those for the go test binary itself, e.g. `-test.run=TestHelperProcessRich`
@@ -1518,12 +1517,17 @@ func TestCommandMocking(t *testing.T) {
 	commandResult, ok := commands[args(strings.Join(realArgs, " "))]
 	if !ok {
 		fmt.Fprintf(os.Stderr, "unexpected command: %s\n", strings.Join(realArgs, " "))
-		os.Exit(255)
+		os.Exit(1)
+	}
+
+	if commandResult.Stdout != "" {
+		fmt.Fprint(os.Stdout, commandResult.Stdout)
+	}
+
+	if commandResult.Stderr != "" {
+		fmt.Fprint(os.Stderr, commandResult.Stderr)
 	}
 
-	// WM-TODO: maybe pointer on these fields, or only print if not-empty
-	fmt.Fprint(os.Stdout, commandResult.Stdout)
-	fmt.Fprint(os.Stderr, commandResult.Stderr)
 	os.Exit(commandResult.ExitStatus)
 }