feat: add fork remote command

[karolzwolak]

PR Description

The command allows you to quickly add a fork remote by replacing the owner in the origin URL and optionally check out a branch from new remote.

Examples:

• given url: https://github.com/jesseduffield/lazygit.git
  and username: karolzwolak

  adds a new remote with url: https://github.com/karolzwolak/lazygit.git

• given url: https://github.com/jesseduffield/lazygit.git
  and username: karolzwolak:add-fork-remote-command

  adds a new remote with url: https://github.com/karolzwolak/lazygit.git
  and then checks out it's branch add-fork-remote-command

Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go generate ./...)
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
• Text is internationalised (see here)
• If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
• Docs have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[stefanhaller]

Thanks for the PR, and sorry for the long delay in looking at it.

This is a useful feature, I need it myself frequently. Before I review in detail, a few general thoughts:

• it would be even more useful if it could take a repo:branch specification and create the remote and check out the branch in one go. For example, for this very PR I would click the copy-to-clipboard icon here:
  , invoke the command, and paste. Any thoughts on this?
• I didn't look at the code in detail yet, I only noticed from a cursory look that there are many lines of code related to parsing URIs. Is this really needed? Intuitively I would have thought that the implementation would consist of basically a single line of code doing a regex replace. I may well be missing something here, and I should probably look at the code in detail before asking questions like this.
• How much of this is github specific? For me personally, github is the only hosting service that I have experience with, but lazygit supports a bunch of others (gitlab, azure devops, gitea, bitbucket). I wonder if we need to hook into hosting_service.go somehow to make it work for those, too.

[karolzwolak]

Thanks for the PR, and sorry for the long delay in looking at it.

This is a useful feature, I need it myself frequently. Before I review in detail, a few general thoughts:

• it would be even more useful if it could take a repo:branch specification and create the remote and check out the branch in one go. For example, for this very PR I would click the copy-to-clipboard icon here:
  , invoke the command, and paste. Any thoughts on this?

Yes, that's a great follow-up. It should be easy to do, too. I will implement it.

• I didn't look at the code in detail yet, I only noticed from a cursory look that there are many lines of code related to parsing URIs. Is this really needed? Intuitively I would have thought that the implementation would consist of basically a single line of code doing a regex replace. I may well be missing something here, and I should probably look at the code in detail before asking questions like this.

Yeah, I might have gone overboard with the url parsing. I didn't know we had regex support. Regex would work well for ssh urls, but it's a bit more nuanced for http/https urls, but I'll try to simplify it.

• How much of this is github specific? For me personally, github is the only hosting service that I have experience with, but lazygit supports a bunch of others (gitlab, azure devops, gitea, bitbucket). I wonder if we need to hook into hosting_service.go somehow to make it work for those, too.

I also don't have experience besides github, but I think most other hosting services follow the same owner/repo url pattern as github does, so it should work fine without any further modifications. I might be wrong on this, though.

I'm away from home for about 2 weeks, so I won't be active on this right now.

[karolzwolak]

I've refactored the replacement to just use regex replace. I think your suggestion is great follow up, but I think it should be supported not just in this new add fork feature, but also when adding a general remote like somerepo.git:dev. I'll submit separate PR that will implement specifying the branch for all ways of adding a new remote.

@stefanhaller friendly ping

[karolzwolak]

Nevermind, I added the ability to checkout PRs with user:branch. I'm not sure how to name this feature now. Since the branch is optional I kept the old name (add fork remote).

[stefanhaller]

Still didn't look at the code very much, but tested the feature, and it works nicely.

One thing I'm wondering is whether we need the second prompt that asks for a remote name; in my own use, I always use the same name as the forkUsername, doesn't everybody?

And then, I was also wondering if this even needs to be a separate command at all. We could include the functionality in the normal "add new remote" command, where the first prompt would say New remote name (use remotename:branch to check out a branch):, and the second prompt would say New remote url (leave empty to use same pattern as origin):. Any thoughts about that?

[karolzwolak]

One thing I'm wondering is whether we need the second prompt that asks for a remote name; in my own use, I always use the same name as the forkUsername, doesn't everybody?

Not always. I often add my own fork as origin or the main repo as upstream. But if we ditched the name prompt, I guess I could just rename the remote after creating it.
I think the name prompt should stay. You'd just confirm the suggested name most of the time, but there would be an option to name it differently if there's a need.

And then, I was also wondering if this even needs to be a separate command at all. We could include the functionality in the normal "add new remote" command, where the first prompt would say New remote name (use remotename:branch to check out a branch):, and the second prompt would say New remote url (leave empty to use same pattern as origin):. Any thoughts about that?

Incorporating it into add remote sounds nice in theory, but I’m not sure it works as well in practice. You still end up with two prompts and some limits:

• always basing the URL on origin
• no way to rename immediately

It also gets tricky if there’s no origin or multiple remotes. I think incorporating it this way would be too clunky. Dedicated command feels clearer, more powerful, and would allow more flexibility. I'm open to other suggestions for improving this new feature.

[stefanhaller]

I have used this for a while in my local build now, and I quite like it. I'm happy to keep it as a separate command; after thinking about it more, my suggestion to incorporate it into the add remote command wasn't so good.

However, I still can't get used to the second prompt, and I still vote for getting rid of it. The only scenarios I can imagine where you would want a different remote name is either origin itself (but that's irrelevant here), or an "upstream" remote. For that, I'd imagine you would use the regular add remote command; or rename it afterwards. Since that's a one-time thing, I don't find that too bad. But in all other cases the additional prompt is simply confusing, it trips me up every time.

Apart from that, I was thinking if we should have any special handling of the case that a remote with that name already exists; currently you get an error popup, which I guess is appropriate, but I was thinking it might be nice if I didn't have to pay attention and it would just use the existing remote and only check out the branch. But maybe that's too much magic and we just keep it the way it is.

[karolzwolak]

I'm okay with removing the name prompt.
I think we should support 're-adding' remotes that already exist so that you can check out multiple PRs by copy pasting the user:branchname from github.
In my opinion it should work this way: if remote with user name already exists, and the url matches, then we just don't create new remote, otherwise we error that there's already remote with name user and it has different url.

[stefanhaller]

Yes, that's pretty much what I had in mind. We should probably only do that when "user:branch" was given though; otherwise, when only a user was given, there would be no feedback. I guess we should still show a "remote already exists" popup in that case.

[karolzwolak]

I'm not sure the behavior should change depending if only user was provided or not (in the case of remote already existing).
I implemented it so that it doesn't matter and it just fetches that remote and doesn't show any popup.
Let me know what you think of this iteration.

[stefanhaller]

I'm not sure the behavior should change depending if only user was provided or not (in the case of remote already existing). I implemented it so that it doesn't matter and it just fetches that remote and doesn't show any popup.

I was initially thinking that it should definitely show a popup in the case that no branch was given, because otherwise the command would silently do nothing and you wouldn't see any feedback, which would feel a bit broken. But I now realized that this is not the case: it will still select the remote and fetch it, so you do get visual feedback. Fine with me then.

Let me know what you think of this iteration.

This coming along really nicely now, great work. I squashed your commits into one, rebased onto master, and added a few fixups; see their commit messages for what they are about.

There are probably a few more minor nitpicks to sort out (for example, some error strings are not i18n'ed), but before I look more closely at those, there's one more fundamental thing to talk about: currently the function works with the selected remote and uses it as a base URL. I was surprised to see this in the code, and it contradicts what the tooltip says, which talks about the origin remote. I would have expected that it doesn't matter what remote is selected when I invoke F (just as with n), and that it always works with the "origin" remote (probably giving an error if I don't have a remote with that name).

Maybe it doesn't make a difference in practice since origin and all fork remotes tend to have the same URL pattern usually, but I'm not sure this has to be the case, and I just found it surprising. Any thoughts on that?

[karolzwolak]

This coming along really nicely now, great work. I squashed your commits into one, rebased onto master, and added a few fixups; see their commit messages for what they are about.

I'm happy to hear that!

There are probably a few more minor nitpicks to sort out (for example, some error strings are not i18n'ed), but before I look more closely at those, there's one more fundamental thing to talk about: currently the function works with the selected remote and uses it as a base URL. I was surprised to see this in the code, and it contradicts what the tooltip says, which talks about the origin remote. I would have expected that it doesn't matter what remote is selected when I invoke F (just as with n), and that it always works with the "origin" remote (probably giving an error if I don't have a remote with that name).

Maybe it doesn't make a difference in practice since origin and all fork remotes tend to have the same URL pattern usually, but I'm not sure this has to be the case, and I just found it surprising. Any thoughts on that?

I must have accidentally typed 'origin' instead of 'remote', that has been my intention from the start.
I'm not entirely convinced treating 'origin' as the base is better as you loose some flexibility if you'd want to mix https and ssh but I guess that's rare in practice. What I mean is that you're locked to using the scheme that your origin has for all new 'fork' remotes. If the url was based on the selected remote, then you can easily choose what scheme you want to use just by having at least 2 remotes, one with https and other ssh.

However, as I said I don't really see any usecase for using 2 schemes for different 'fork' remotes. Another argument is that it'd work fine without origin remote, but that's rare too.
So to summarize my ranting -- I think you're right and it should be based on the origin like you suggest.

[karolzwolak]

Hey, sorry for the delay.
I've rebased and made so that adding fork treats origin as the base (and it's disabled unless there's origin remote). I wasn't sure why you left the commits with 'fixup!', I just squashed them and renamed them so that the CI passes. I'm assuming we're gonna squash this into 1 commit anyway when merging.

I think the only thing left now is to translate the errors from the replaceForkUsername(). I wasn't sure how to achieve that in a good way -- it'd make testing annoying. My 1st instinct was to make the function return an error enum and in the tests verify the correct enum variant is returned when an error is expected. Alternatively we could perform empty string checking before calling the function (but that's not ideal), and then the only thing that could go wrong is the URL not matching the regex. I tried to see if any other controllers tested functions that return errors, but I couldn't find any. Let me know how'd structure testing here and if there are any other issues we have to address.

[stefanhaller]

I finally got around to looking at this again, sorry that it took so long. I pushed several fixup commits again, please review. I'm happy with the current state of the branch, and would be ready to merge it if you agree with my changes.

I wasn't sure why you left the commits with 'fixup!',

That's the standard way of making changes to a branch during review. It allows you to maintain a clean commit history from the beginning, while still making changes transparent. See https://github.com/jesseduffield/lazygit/blob/master/docs/Fixup_Commits.md.

... renamed them so that the CI passes.

It's normal for CI to fail because of fixup commits. This check exists to make sure we don't forget to squash them at the end.

I'm assuming we're gonna squash this into 1 commit anyway when merging.

While this may be true for this particular PR, it's not the default assumption; most of my PRs have more than one commit. I prefer to use fixup commits no matter what so that I don't have to think about it.

I think the only thing left now is to translate the errors from the replaceForkUsername(). I wasn't sure how to achieve that in a good way -- it'd make testing annoying.

Not at all, this wouldn't be an issue; tests always run against the english translation, so it's fine to hard-code translatable error messages in tests. However: I got rid of two of the errors (1550662), so the only one left is the one about an invalid origin URL. Since this should never happen in practice, I decided to not pollute the language file with it and leave it in english, I think that's good enough.

[karolzwolak]

I finally got around to looking at this again, sorry that it took so long. I pushed several fixup commits again, please review. I'm happy with the current state of the branch, and would be ready to merge it if you agree with my changes.

The changes make sense and everything's working nicely. Let's finally merge this.

As for the fixup 'workflow' — I wasn't familiar with it but I like it. I squashed the commits.

[karolzwolak]

I also updated the PR description and removed the demo clip as it was outdated.