Repository navigation
Set default repo when creating fork during pr creation #10277
Description
Activity
👋 Hey @Vampire, thanks for writing this up.
Affected version
2.49.2This
ghversion is fairly old, would you please try with the latest and let us know if the same behavior persists? ❤- addedmore-info-neededMore info needed from user/contributorMore info needed from user/contributor
on Jan 20, 2025 I updated right before writing this, but it was too late already :-D
Nope, still the same:
$ gh repo clone https://github.com/cli/cli github-cli Cloning into 'github-cli'... remote: Enumerating objects: 63105, done. remote: Counting objects: 100% (645/645), done. remote: Compressing objects: 100% (216/216), done. remote: Total 63105 (delta 576), reused 429 (delta 429), pack-reused 62460 (from 2) Receiving objects: 100% (63105/63105), 42.43 MiB | 11.16 MiB/s, done. Resolving deltas: 100% (43487/43487), done. $ cd github-cli/ $ git remote -v origin https://github.com/cli/cli.git (fetch) origin https://github.com/cli/cli.git (push) $ git sw -c dummy Switched to a new branch 'dummy' $ gh pr create ? Where should we push the 'dummy' branch? Create a fork of cli/cli Creating pull request for Vampire:dummy into trunk in cli/cli ? Title (required) dummy ? Title (required) dummy ? Choose a template Open a blank pull request ? Body <Received> ? What's next? Continue in browser Changed cli/cli remote to "upstream" Added Vampire/cli as remote "origin" remote: remote: branch 'dummy' set up to track 'origin/dummy'. To github.com:Vampire/cli.git * [new branch] HEAD -> dummy Opening https://github.com/cli/cli/compare/trunk...Vampire:dummy in your browser. $ gh pr create X No default remote repository has been set for this directory. please run `gh repo set-default` to select a default remote repository.
- removedmore-info-neededMore info needed from user/contributorMore info needed from user/contributor
on Jan 21, 2025 Actually, it could also be considered to make this setting on
gh repo forkas the 98.7 % case should be for sending PRs upstream.
Or it could ask.Reacted by Kynan WareSorry for the delays on this @Vampire - digging into this today 🙇
Okay! So, I do not see any indication in the code that setting the default repo during
pr createis the intended behavior, so I don't think this is quite a bug.I'm in favour of accepting this enhancement, however - I think it makes sense to set the default repo automatically when we create a fork. This is what
gh repo forkdoes already, so I think we should align with that behavior rather than prompting.Relevant
gh repo forkcode:Lines 372 to 384 in db9dbfa
if err := gc.SetRemoteResolution(ctx, upstreamRemote, "base"); err != nil { return err } if err := gc.Fetch(ctx, upstreamRemote, ""); err != nil { return err } if connectedToTerminal { fmt.Fprintf(stderr, "%s Cloned fork\n", cs.SuccessIcon()) 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(repoToFork))) } } This is where the fork happens in
pr create:cli/pkg/cmd/pr/create/create.go
Lines 942 to 950 in db9dbfa
if headRepo == nil && ctx.IsPushEnabled { opts.IO.StartProgressIndicator() headRepo, err = api.ForkRepo(client, ctx.BaseRepo, "", "", false) opts.IO.StopProgressIndicator() if err != nil { return fmt.Errorf("error forking repo: %w", err) } didForkRepo = true } I suspect that this change would happen somewhere near this code.
Acceptance Criteria
Given I have cloned a repo owned by an actor other than myself
When I rungh pr createand selectCreate a fork of <repo>
Then the upstream repository is set as the default repository, and a useful message is printed to stderr, aligning with the behavior ofgh repo fork- addedenhancementa request to improve CLIa request to improve CLIhelp wantedContributions welcomeContributions welcomegh-prrelating to the gh pr commandrelating to the gh pr commandand removedbugSomething isn't workingSomething isn't workingneeds-triageneeds to be reviewedneeds to be reviewed
on Feb 7, 2025 hi @BagToad, I can take up this enhancement task!
Hey @daviddl9, that is fine you can take this and I'll assign it to you 😁
FYI you were accidentally blocked from the
cliorg because of your testing pull requests. This was not intentional and is only because thecliorganization gets so much spam and so we block anything that looks like spam 😅Please keep the optics of spam in mind as you work on this issue, see #10409 (comment).
Feel free to use one of my testing repos (https://github.com/BagToad/testing-public-1) for forking and pull request testing 😁
- added a commit that references this issue
on Feb 18, 2025 @BagToad @Vampire : Wanted to bubble up a nuanced question that @daviddl9 and I were discussing in #10458 (comment):
If an error arises from setting the upstream branch as the default happens during
gh pr createbefore pushing the branch up, is this 1) a hard error that gives up before pushing the branch or 2) a soft error that we warn the user about but still continue with the rest of the command?The PR as merged treats it as a hard error.
Appreciate additional thoughts!
@andyfeller @daviddl9 Thanks for the interesting discussion!
I don't have strong opinions myself, but I loosely prefer this being an error and not a warning, like you have currently.
I prefer consistency between
gh pr createandgh repo fork. I seegh repo forkloosely as a model of what should happen while forking. It looks like this is considered an error ingh repo fork, so I'd prefer to keep it consistent across all operations that fork (unless, of course, an exception is more valuable than the consistency 😁) -Lines 372 to 374 in 537a222
if err := gc.SetRemoteResolution(ctx, upstreamRemote, "base"); err != nil { return err } I recognize that
gh pr createandgh repo forkhave different goals, so it might make sense to avoid failinggh pr createcompletely because a failure doesn't hinder the PR creation goal. However, I personally do not find that compelling enough for me to prefer a warning over a failure, softly violating the consistency between the two commands.Maybe the main question is, when can it fail for what reason?
And can the reason be mitigated to then successfully send the PR which would anyway work if this error would be a warning.
I don't have right now too strong feelings either way either.
Having it as warning because it just is a secondary convenience operation sounds like a good argument to me.
But consistency also is a good argument.
So I'm actually not really able to stir in one direction.Another thought.
Maybe both arguments can be satisfied.
Also forgh repo forksetting the default repo would just be a secondary convenience operation, so make it a warning for both cases and you have the consistency as well as it only being a warning.Uhm, and actually while thinking about it.
Doesgh repo forkactually set the default repo?
Up in the comment #10277 (comment) I suggested to consider adding this forgh repo forktoo, so I guess I was under the impression thatgh repo forkdoes not set the default repo.
Describe the bug
When creating a fork during PR creation the default repository should be set
Affected version
2.49.2
Steps to reproduce the behavior
gh repo clone ...a 3rd party repository you do not have forkedgh pr createExpected vs actual behavior
After this the PR is created, but if I want to send another PR, I get a complaint that the default repository needs to be set manually using
set-defaultfirst.If so, it would be nice if not an error but directly a selection would be shown.
But actually after creating that fork, I'd expect the upstream to automatically be set as default repository.