Repository: cli/cli
File: pkg/cmd/pr/create/create.go (Go)

Code before:
			renameCmd, err := gitClient.Command(context.Background(), "remote", "rename", "origin", "upstream")
			if err != nil {
				return err
			}
			if _, err = renameCmd.Output(); err != nil {
				return fmt.Errorf("error renaming origin remote: %w", err)
			}
			remoteName = "origin"
			fmt.Fprintf(opts.IO.ErrOut, "Changed %s remote to %q\n", ghrepo.FullName(ctx.BaseRepo), "upstream")
		}

		gitRemote, err := gitClient.AddRemote(context.Background(), remoteName, headRepoURL, []string{})
		if err != nil {
			return fmt.Errorf("error adding remote: %w", err)
		}

		fmt.Fprintf(opts.IO.ErrOut, "Added %s as remote %q\n", ghrepo.FullName(headRepo), remoteName)

		if didForkRepo {
			gitClient.SetRemoteResolution(context.Background(), "upstream", "base")
			if opts.IO.IsStdinTTY() && opts.IO.IsStdoutTTY() {
				cs := opts.IO.ColorScheme()
				stderr := opts.IO.ErrOut
				fmt.Fprintf(stderr, "%s Repository %s set as the default repository. To learn more about the default repository, run: gh repo set-default --help\n", cs.WarningIcon(), cs.Bold(ghrepo.FullName(headRepo)))
			}
		}

		headRemote = &ghContext.Remote{
			Remote: gitRemote,
			Repo:   headRepo,
		}
	}

	// automatically push the branch if it hasn't been pushed anywhere yet
	if ctx.IsPushEnabled {
		pushBranch := func() error {
			w := NewRegexpWriter(opts.IO.ErrOut, gitPushRegexp, "")
			defer w.Flush()
			ref := fmt.Sprintf("HEAD:refs/heads/%s", ctx.HeadBranch)
			bo := backoff.NewConstantBackOff(2 * time.Second)
			ctx := context.Background()
			return backoff.Retry(func() error {
				if err := gitClient.Push(ctx, headRemote.Name, ref, git.WithStderr(w)); err != nil {
					// Only retry if we have forked the repo else the push should succeed the first time.
					if didForkRepo {
						fmt.Fprintf(opts.IO.ErrOut, "waiting 2 seconds before retrying...\n")
						return err
					}
					return backoff.Permanent(err)
				}
				return nil

Review comment (bug):
## Suggestions

Out of all of following idle thoughts, I think only a handful are reasonable to knock out here now versus waiting for new issues being raised:

1. Remove `stdin` requirement for messaging
2. Use local variable versus hardcoding remote name
3. Return the error raised by setting default remote

## Notes

Some of my idle thoughts reviewing this logic:

1. Why does `stdin` being TTY affect whether we output this message?
2. How does such conditional logic apply to the `Added ... as remote` message?
3. When do we use a variable for the remote name versus hardcoding this everywhere?
4. How does this differ from other places in code where we handle this?
5. Does this logic need to ensure no other remote is marked as default before setting `upstream`?

### TTY affecting messaging

It seems like `gh repo fork` care about `stdin`, `stdout`, `stderr`... 

https://github.com/cli/cli/blob/ae2f4b7c5b49dd1b5855a8c2d283e0b02fb636eb/pkg/cmd/repo/fork/fork.go#L187

https://github.com/cli/cli/blob/ae2f4b7c5b49dd1b5855a8c2d283e0b02fb636eb/pkg/cmd/repo/fork/fork.go#L380-L383

whereas `gh repo clone` just cares about `stdout` being TTY:

https://github.com/cli/cli/blob/ae2f4b7c5b49dd1b5855a8c2d283e0b02fb636eb/pkg/cmd/repo/clone/clone.go#L217-L221

Definitely want to revisit where different types or classes of messages are displayed so we had a little more consistency.  In the meantime, I assume `gh pr create` would follow the same behavior as `gh repo clone` without factoring in `stdin` if we even bother to check.


### Setting and unsetting

My main concern with the changes as they stand is comparing this versus `gh repo set-default` which checks `func (r Remotes) ResolvedRemote() (*Remote, error)` to determine if it should unset a remote before setting a new one 🤔 

https://github.com/cli/cli/blob/ae2f4b7c5b49dd1b5855a8c2d283e0b02fb636eb/pkg/cmd/repo/setdefault/setdefault.go#L228-L242

It feels there is an edge case that can emerge around `upstream` remote separately of the server-side repository forking the command does.  However, let's see about users reporting errors before dealing with that concern.

### Other places where we set default repository

Just because I know this messaging is in a few different places, I always look at how consistent our behavior is.

https://github.com/cli/cli/blob/ae2f4b7c5b49dd1b5855a8c2d283e0b02fb636eb/pkg/cmd/repo/clone/clone.go#L209-L221

https://github.com/cli/cli/blob/ae2f4b7c5b49dd1b5855a8c2d283e0b02fb636eb/pkg/cmd/repo/fork/fork.go#L372-L383


Code after:
		if origin != nil && upstream == nil && ghrepo.IsSame(origin, ctx.BaseRepo) {
			renameCmd, err := gitClient.Command(context.Background(), "remote", "rename", "origin", upstreamName)
			if err != nil {
				return err
			}
			if _, err = renameCmd.Output(); err != nil {
				return fmt.Errorf("error renaming origin remote: %w", err)
			}
			remoteName = "origin"
			fmt.Fprintf(opts.IO.ErrOut, "Changed %s remote to %q\n", ghrepo.FullName(ctx.BaseRepo), upstreamName)
		}

		gitRemote, err := gitClient.AddRemote(context.Background(), remoteName, headRepoURL, []string{})
		if err != nil {
			return fmt.Errorf("error adding remote: %w", err)
		}

		fmt.Fprintf(opts.IO.ErrOut, "Added %s as remote %q\n", ghrepo.FullName(headRepo), remoteName)

		// Only mark `upstream` remote as default if `gh pr create` created the remote.
		if didForkRepo {
			err := gitClient.SetRemoteResolution(context.Background(), upstreamName, "base")
			if err != nil {
				return fmt.Errorf("error setting upstream as default: %w", err)
			}

			if opts.IO.IsStdoutTTY() {
				cs := opts.IO.ColorScheme()
				fmt.Fprintf(opts.IO.ErrOut, "%s Repository %s set as the default repository. To learn more about the default repository, run: gh repo set-default --help\n", cs.WarningIcon(), cs.Bold(ghrepo.FullName(headRepo)))
			}
		}

		headRemote = &ghContext.Remote{
			Remote: gitRemote,
			Repo:   headRepo,
		}
	}

	// automatically push the branch if it hasn't been pushed anywhere yet
	if ctx.IsPushEnabled {
		pushBranch := func() error {
			w := NewRegexpWriter(opts.IO.ErrOut, gitPushRegexp, "")
			defer w.Flush()
			ref := fmt.Sprintf("HEAD:refs/heads/%s", ctx.HeadBranch)
			bo := backoff.NewConstantBackOff(2 * time.Second)
			ctx := context.Background()
			return backoff.Retry(func() error {
				if err := gitClient.Push(ctx, headRemote.Name, ref, git.WithStderr(w)); err != nil {
					// Only retry if we have forked the repo else the push should succeed the first time.
					if didForkRepo {
						fmt.Fprintf(opts.IO.ErrOut, "waiting 2 seconds before retrying...\n")