Skip to content

feat: implement prompt for remote renaming - #1882

Merged
vilmibm merged 4 commits into
cli:trunkfrom
dfireBird:remote-renaming-847
Jan 21, 2021
Merged

feat: implement prompt for remote renaming#1882
vilmibm merged 4 commits into
cli:trunkfrom
dfireBird:remote-renaming-847

Conversation

@dfireBird

@dfireBird dfireBird commented Sep 21, 2020

Copy link
Copy Markdown
Contributor

fixes #847

Changes:
A prompt asking for the name of fork remote, if there is already remote with same name, another prompt asking for name of source remote to rename it, otherwise just add the remote name from the first prompt.

Behaviour if the entered name of remote is already a remote name:
remote-renaming

Behaviour if the entered name of remote is not a remote name:
remote-renaming-2

@vilmibm

vilmibm commented Sep 21, 2020

Copy link
Copy Markdown
Contributor

I think this is a nice direction but would rather @mislav and @ampinsk weigh in.

@sonyapieklik sonyapieklik left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

failing for ubuntu, windows, and macros

@dfireBird

dfireBird commented Sep 25, 2020

Copy link
Copy Markdown
Contributor Author

As I mentioned in issue #847, I didn't learn Go yet! The offline fork I had is mostly based on the previous implementations.
So I don't know how to fix the tests! If someone can give guidance on how to fix, I will try to fix them!

@scottrigby

Copy link
Copy Markdown

I came here looking for an issue to add an optional name to the fork itself, not only the remote name – for example, when several repos have the same name, but within different user or org workspaces. Don't want to derail this PR, just checking to see if there's some common use cases here. A few other things that feel related:

@vilmibm vilmibm self-assigned this Jan 19, 2021
@vilmibm

vilmibm commented Jan 20, 2021

Copy link
Copy Markdown
Contributor

i'll see this through. TODO:

  • add option for accepting remote name
  • in case of conflict, error and tell to use remote name flag
  • tests

@vilmibm

vilmibm commented Jan 20, 2021

Copy link
Copy Markdown
Contributor

new behavior:

image

@vilmibm vilmibm 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.

self approving since we went over the implementation synchronously and there are no huge changes here

@vilmibm
vilmibm merged commit 6557289 into cli:trunk Jan 21, 2021
@dfireBird

Copy link
Copy Markdown
Contributor Author

Thanks @vilmibm for your work! 😄

@kubukoz

kubukoz commented Jan 30, 2021

Copy link
Copy Markdown

So this is changing the previous behavior, right? If you cloned a repo from origin, then added a fork, you would have the old origin be added to the upstream remote, and the new origin would be your fork. As I understand, this isn't possible anymore:

https://asciinema.org/a/DRebWnUq73hVcNAY8lZdVXolV

As much as I like having the option to pass a name, this is marked as an addition, but it's also a feature removal, and it'll be missed :c

Comment thread pkg/cmd/repo/fork/fork.go
Comment on lines -235 to -236
renameTarget := "upstream"
renameCmd, err := git.GitCommand("remote", "rename", remoteName, renameTarget)

@jetersen jetersen Feb 3, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This clearly broke existing functionality to rename existing remote to upstream 😭
The pattern of origin, upstream is very common when forking due to scripts and muscle memory.

@vilmibm

vilmibm commented Feb 3, 2021

Copy link
Copy Markdown
Contributor

Is a compromise here a --rename flag that restores and opts into the old behavior?

@kubukoz

kubukoz commented Feb 3, 2021

Copy link
Copy Markdown

Good enough for me :)

@jetersen

jetersen commented Feb 4, 2021

Copy link
Copy Markdown

In my opinion a flag would not resolve it.
if i simply run gh repo fork I would prefer it assumed if something is named origin it should rename it to upstream as in the previous behavior.
now if you run gh repo fork --remote-name than no renaming needs to happen so why add the rename flag?

Perhaps rename flag could be added if the remote found was unexpected as in not origin
Maybe I am different as I preferred the magic instead of being prompted and I also prefer not to have to remember each incantation of flags.

If anything though gh repo fork still lacks the ability to rename the repo before forking it which is a bigger issue than what your remote is called locally but I digress.
Fine --rename would work, I'll just end up wrapping the command 😅

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.

Unwanted remote naming after gh repo fork

6 participants