Skip to content

Set default repo when creating fork during pr creation #10277

Description

@Vampire

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

  1. gh repo clone ... a 3rd party repository you do not have forked
  2. gh pr create

Expected 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-default first.
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.

Activity

  1. BagToad commented on Jan 20, 2025

    @BagToad
    Member

    👋 Hey @Vampire, thanks for writing this up.

    Affected version
    2.49.2

    This gh version is fairly old, would you please try with the latest and let us know if the same behavior persists? ❤

  2. self-assigned this
    on Jan 20, 2025
  3. Vampire commented on Jan 20, 2025

    @Vampire
    Author

    I updated right before writing this, but it was too late already :-D

  4. Vampire commented on Jan 20, 2025

    @Vampire
    Author

    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.
  5. Vampire commented on Jan 23, 2025

    @Vampire
    Author

    Actually, it could also be considered to make this setting on gh repo fork as the 98.7 % case should be for sending PRs upstream.
    Or it could ask.

  6. BagToad commented on Feb 7, 2025

    @BagToad
    Member

    Sorry for the delays on this @Vampire - digging into this today 🙇

  7. BagToad commented on Feb 7, 2025

    @BagToad
    Member

    Okay! So, I do not see any indication in the code that setting the default repo during pr create is 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 fork does already, so I think we should align with that behavior rather than prompting.

    Relevant gh repo fork code:

    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:

    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 run gh pr create and select Create 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 of gh repo fork

  8. added
    enhancementa request to improve CLI
    gh-prrelating to the gh pr command
    and removed
    bugSomething isn't working
    on Feb 7, 2025
  9. removed their assignment
    on Feb 7, 2025
  10. daviddl9 commented on Feb 9, 2025

    @daviddl9
    Contributor

    hi @BagToad, I can take up this enhancement task!

  11. BagToad commented on Feb 11, 2025

    @BagToad
    Member

    Hey @daviddl9, that is fine you can take this and I'll assign it to you 😁

    FYI you were accidentally blocked from the cli org because of your testing pull requests. This was not intentional and is only because the cli organization 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 😁

  12. andyfeller commented on Feb 21, 2025

    @andyfeller
    Contributor

    @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 create before 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!

  13. BagToad commented on Feb 22, 2025

    @BagToad
    Member

    @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 create and gh repo fork. I see gh repo fork loosely as a model of what should happen while forking. It looks like this is considered an error in gh 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 😁) -

    if err := gc.SetRemoteResolution(ctx, upstreamRemote, "base"); err != nil {
    return err
    }

    I recognize that gh pr create and gh repo fork have different goals, so it might make sense to avoid failing gh pr create completely 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.

  14. Vampire commented on Feb 24, 2025

    @Vampire
    Author

    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.

  15. Vampire commented on Feb 24, 2025

    @Vampire
    Author

    Another thought.
    Maybe both arguments can be satisfied.
    Also for gh repo fork setting 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.

  16. Vampire commented on Feb 24, 2025

    @Vampire
    Author

    Uhm, and actually while thinking about it.
    Does gh repo fork actually set the default repo?
    Up in the comment #10277 (comment) I suggested to consider adding this for gh repo fork too, so I guess I was under the impression that gh repo fork does not set the default repo.

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

Metadata

Metadata

Assignees

Labels

enhancementa request to improve CLIgh-prrelating to the gh pr commandhelp wantedContributions welcome

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions