Conversation
mislav
left a comment
mislav
left a comment
There was a problem hiding this comment.
This is shaping up great! I'd like us to figure out the git remote swapping behavior (in case of fork) before this lands though.
| "fmt" | ||
| "time" | ||
|
|
||
| "github.com/AlecAivazis/survey/v2" |
There was a problem hiding this comment.
CI is failing because this dependency doesn't seem to be vendored
There was a problem hiding this comment.
I have proposed that we instead stop requiring vendored dependencies in CI (and in development, for that matter, although they were never strictly required there) #31
|
|
||
| // Fail because the user has permission to push but `git push` failed for some other reason | ||
| if hasPermission { | ||
| return err |
There was a problem hiding this comment.
Interesting approach with trying a regular push and forking + retrying on errors!
Have you considered querying hasPermision before the push and thus avoiding the potential error due to lack of permissions? We do only 1 git operation that way, but the user experiences a small overhead of an API request before their push.
There was a problem hiding this comment.
Have you considered querying hasPermision before the push and thus avoiding the potential error due to lack of permissions?
This is what I originally had but thought it was overkill to check for permissions on every push when only a small minority of the pushes will fail because of permissions errors. So instead of the extra overhead being on the majority of pushes, it will be on the minority of pushes that fail.
Does that make sense or am I thinking about this wrong?
command/push.go
Outdated
|
|
||
| fmt.Printf("All your future changes will be pushed to your fork at %s.\n", url) | ||
|
|
||
| err = git.Run("remote", "set-url", "origin", cloneURL) |
There was a problem hiding this comment.
This assumes that the current remote was named "origin", but it might have been "upstream" or "github" (see BaseRepo). Furthermore, I think that changing the repo to which an existing git remote points might be confusing to the user.
How about creating a new git remote for the person's fork, and setting the upstream configuration for this branch to push to the new remote by default?
There was a problem hiding this comment.
This does assume that origin is the default branch. It should probably ask context what the default remote name is.
I think we need to think about when the user would hit this error. I think it almost always happen when you're trying to push to a public repo you don't have access to. In that case, if I want to clone it I would want origin to everywhere because I can't write anything to the original origin. This matches up with the user test we ran today where the user wasn't sure what would happen after forking the repo with gh push but he hoped it would update the origin.
I could be totally wrong about this though so it's worth asking more users about it!
We started vendoring dependencies because this was a practice that the Go community had for a while now to: 1. Speed up builds - no need to fetch dependencies every time; 2. Guard against 3rd-party downtime - CI passes even if hosts such as `gopkg.in` are down, or if someone deletes their GitHub repo/account hosting a particular module. With Go 1.13 and GitHub Actions, however, we have these problems solved for free: - The built-in goproxy caches dependencies and speeds up downloads; - Octofactory ensures that dependencies are cached on our own infrastructure, guarding us from 3rd-party downtime. With all this in mind, I feel that we don't have to require vendoring dependencies anymore.
probablycorey
changed the title
gh pushgh push
|
Closing because it is a prototype branch. |
Typo in the examples tools header referencing the wrong repo
2 participants
This PR implements
gh push. It was build to solve a problem that people run into with forking.The Problem
Cloning a repo you don’t have write access too
This is probably most common when making a change to an open source repository. It is fine unless you make changes you want to push up. At that point you’ll need to for the repo, update your remotes and then push your changes.
git clone https://github.com/evil-probablycorey/party-all-the-time.gitcode party-all-the-timegit commit -am’Look at this cool code I made’git pushThe
gh pushsolutionThese replace step 5 from “The Problem”
gh pushoriginis set to the forked repo,upstreamis set to the original repo, and the code is pushed without any problems.Notes
Why do you modify origin and the .git/config file
Once the user decides to fork with
gh pushI want the repo to be based on the fork, not the upstream repo. To do this I updated the origin remote, created an upstream remote and made sure existing branches are based on the new origin remote. It's a little magical, but I think it will feel like "it just works" for users.Why didn’t I ask which org the user wants to fork the repo into
This felt unnecessary since I assume most people who get into this flow accidentally forgot to fork the repo into their personal “org”. If they wanted to fork this into another fork they can always use the website.
Why didn’t I choose to solve this with
gh cloneorgh commitI didn’t prefer these solutions because they feel like a premature optimization. We don’t know if the user wants to change the repo at the clone or commit step, they may just be playing around. It is only when they push when they make their intentions clear.