Idea: maybe we can be more specific with flag naming here and have the flag betray the specific operation that it applies to? E.g. repo fork --clone or repo fork --clone=false

I feel like --yes and --no could prove hard to maintain over time if it turns out they mean different things in different commands, and in case those commands have more than one boolean prompt

just having clone doesn't help the remote adding case. i based y/n on apt-get and i like how it's easy to reason about in different contexts.

that said, i could be ok with --add-remote and --clone. i know @ampinsk pointed this out too; what do you think?

Yeah, I like the idea of us being explicit with our flag language like this. I'm into --add-remote and --clone with the options to set them to false as well

👍 I'll switch to --add-remote / -a and --clone / -c

i'm not super thrilled about this but here's what I came up:

Flags:
  -c, --clone string    true: clone fork. false: never clone fork (default "prompt")
  -r, --remote string   true: add remote for fork. false: never add remote fork (default "prompt")

I tried tweaking a bit:

Flags:
  -c, --clone string    Clone fork. Pass false to not clone fork. No flag will prompt.
  -r, --remote string   Add remote for fork. Pass false to not add remote fork. No flag will prompt.

distinguishing between "user specified flag but passed nothing" and "user did not specify flag" is weirdly unintuitive in our codebase; I got frustrated and did the explicit true/false thing. I can take another swing at it though.

ah I immediately figured out how to do your suggestion, @ampinsk 🤦‍♀️ so now a bare --clone or --remote implies true.

thanks nate!!

Ah, I just realized something. We do not have to check whether the fork already exists if you are going to fork otherwise. You can simply initiate the API fork operation and it will be a no-op and return the existing repo name if your fork already exists. That makes it simpler for you I think.

that's a good point, i can play with that implementation.

The create fork endpoint seems to behave exactly the same whether or not it created the repo; it returns 202 and the fork's information. This means I can't tell whether or not the fork already existed and we lose the ability to tell the user "hey, that thing you wanted to do is already done."

I personally like telling them that the fork already exists. Pretending that we created it (and offering to add a remote or clone) seems only like it will create confusion.

@ampinsk I'm curious what you think?

It's a good point that we would prefer to communicate to the user what just happened.

One problem with looking up YOURUSER/REPONAME as possible fork repo is that a person might have a fork of a repo, but that fork will not have the same repository name.

Example: I fork vilmibm/dotfiles. Because I already have a dotfiles repo, the fork will be created with the name https://github.com/mislav/dotfiles-1. Thus, we cannot assume that someone's fork will always have the same repository name.

Agreed that I'd rather be transparent and clear about what's going on - so +1 on telling them the fork already exists in this scenario!

I'm going to leave the repo-existence check in. That's a good point about the repo names; it seems like the right approach is to actually list the forks for the repo-to-fork and check for one belonging to the executing user. I'll take a stab at that.

unfortunately there isn't an efficient way to do a more sophisticated check. the best i came up with was:

query { 
  viewer {
    repositories(isFork: true, first:100) {
      edges {
        node {
          name
          parent {
            owner {
              login
            }          }        }      }    }  }}

and then comparing the forks' login with the target repo's owner's login. I don't know that it's worth it at this point given that most of the time an existing fork is going to have the same name.

There is an efficient way with GraphQL, at least in theory; in practice it looks like it's broken since it returns wrong results:

{
  repository(owner:"github", name:"fetch") {
    forks(first: 1, affiliations: OWNER) {
      nodes {
        nameWithOwner
      }
    }
  }
}

I would advise against doing a lookup which we know in advance is imprecise. Right now the only precise API lookup we have is to kick off the fork operation and read the resulting repository. We could read the created_at attribute of the result to find out if the fork was just created so we can communicate that to the user.

The created_at approach worked out pretty well. Once that gql query is fixed I'd rather switch to that.

TIL NoOptDefVal!

I wonder, though, if the users will understand that the default value for --clone omitted is "prompt" (as indicated in help docs), but that the default value for --clone passed without value is "true" 🤔

@mislav Yep, same ambivalence as you here, and @ampinsk and @vilmibm went back and forth on it starting here: #549 (comment)

It didn't seem like anything we thought of was obviously very elegant, but this feels like a reasonable starting point at least to me and we can always update it if we see evidence of confusion.

I wonder if there should exist a shared variant of FromFullName that raises an error like this so we can consistently parse user input with that

I do think FromFullName should be stricter; I can't think of a time when an empty owner or name is anything but an error.

Just noticed that AddRemote already does auto-fetch, so we don't have to do git fetch here

Nit: could we generally avoid punctuation and multiple sentences within flags docs? This is for the same reason why we would avoid them when raising error messages in Go: for readability and so that the end formatter (in this case, gh help) has more control over the final output.

I'm also thinking that it would be useful to list possible flag values like we do elsewhere. I wonder if this would do the trick:

    repoForkCmd.Flags().String("clone", "prompt", "Clone the new fork: {true|false|prompt}")
    repoForkCmd.Flags().String("remote", "prompt", "Add a git remote for the fork: {true|false|prompt}")

The help formatter would then append the default value in the final output:

Flags:
  --clone string    Clone the new fork: {true|false|prompt} (default "prompt")
  --remote string   Add a git remote for the fork: {true|false|prompt} (default "prompt")

curious what @ampinsk thinks. i have zero strong feelings. i'll switch to this and we can always change it.