Skip to content

fully restore fork remote renaming behavior - #2982

Merged
vilmibm merged 5 commits into
trunkfrom
renaming
Feb 17, 2021
Merged

fully restore fork remote renaming behavior#2982
vilmibm merged 5 commits into
trunkfrom
renaming

Conversation

@vilmibm

@vilmibm vilmibm commented Feb 16, 2021

Copy link
Copy Markdown
Contributor

This fully restores the fork remote renaming behavior altered originally in #1882 but leaves the --remote-name flag intact; it also adds a note about this behavior to the fork longform help.

Closes #2904

@vilmibm
vilmibm requested review from mislav and samcoe February 16, 2021 18:04

@samcoe samcoe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one question. The code itself looks good to me.

Comment thread pkg/cmd/repo/fork/fork.go Outdated
return err
}
renameTarget := "upstream"
renameCmd, err := git.GitCommand("remote", "rename", remoteName, renameTarget)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens here if remoteName is blank? If remoteName is blank should we use "origin"? I think that would mimic the behavior before #1882.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing the only way for remoteName to be blank is if the user passed --remote-name="", and in that case I expect that the command should fail since that isn't a valid value.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was my expectation as well, but we could notice and do a nicer error

@mislav mislav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, but I had one additional expectation when it comes to --remote-name, and I'm wondering what your thoughts on it would be: what if passing a --remote-name unconditionally opted out of remote renaming behavior? i.e.

  • gh repo fork: renames "origin" remote if it already exists
  • gh repo fork --remote-name=foo - doesn't ever rename any existing git remotes, even if "foo" existed
    • corollary: gh repo fork --remote-name=origin - doesn't ever rename the "origin" remote

It's a subtle difference from the current state of this PR, but I think that having a way to opt out of any magic-ness it will go a long way towards making --remote-name suitable for scripting.

Comment thread pkg/cmd/repo/fork/fork.go Outdated

With no argument, creates a fork of the current repository. Otherwise, forks the specified repository.

By default, the new fork is set to be your 'origin' remote and any existing origin remote is renamed to 'upstream'. To alter this behavior, you can set a name for the new fork's remote with --remote-name.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our help docs currently have lines hard-wrapped to 80 columns, so maybe this line should wrap too. #1469

Comment thread pkg/cmd/repo/fork/fork.go Outdated
return err
}
renameTarget := "upstream"
renameCmd, err := git.GitCommand("remote", "rename", remoteName, renameTarget)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing the only way for remoteName to be blank is if the user passed --remote-name="", and in that case I expect that the command should fail since that isn't a valid value.

@vilmibm

vilmibm commented Feb 17, 2021

Copy link
Copy Markdown
Contributor Author

I've:

  • implemented the behavior @mislav requested
  • add argument processing tests to fork_test
  • word wrapped the help text

@vilmibm
vilmibm requested review from mislav and samcoe February 17, 2021 18:12

@mislav mislav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! 🎉

Comment thread pkg/cmd/repo/fork/fork.go Outdated
@vilmibm
vilmibm merged commit e91b97b into trunk Feb 17, 2021
@vilmibm
vilmibm deleted the renaming branch February 17, 2021 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gh repo fork broken in v1.5

3 participants