undefined | Better HN

0 pointsboomlinde3y ago0 comments

Where I work, development is trunk based and without pull requests, and all code is reviewed. When I want to submit code, I push it to a staging area that tracks master. This causes the commits to appear in Gerrit where (conflict-free) rebasing can be performed with a single button and the code can be reviewed. During the staging I can change anything about the commits to my hearts content. Once everyone is satisfied, someone with the authority to approve the change approves it and I submit it, upon which the patches are applied as-is on the branch I staged it for.

Creating a separate branch, pushing this to your public copy of the repository and then asking someone to pull from that branch into their master branch seems absurd to me, especially if it's just 1-2 commits, and the idea of reviewing code (which I think is extremely important at team scale) should not be conflated with the concept of pull requests.

0 comments

doix3y ago

What you are describing, is pretty much exactly how it works at every company I've worked at that used PRs. You create a new branch from master, make your changes, push the branch to github/bitbucket/gitlab, make a PR. While the PR is open, you can make any changes you want to the commits in that branch (since it's just a branch).

People look at the PR (which shows the diff) and approve it if they are happy, or request changes. If there are no conflicts, you can merge with a single button. Otherwise you need to resolve conflicts somehow. I normally just rebase the branch off updated master and force push.

> Creating a separate branch, pushing this to your public copy of the repository and then asking someone to pull from that branch into their master branch seems absurd to me.

That's how it works for open source projects because people do not have permissions to make branches in the main repo, so they must fork and have the changes in a different repo. I've never seen this done at a company, I presume everyone here is talking about creating branches in the main repo and requesting review before merging the branch to master.

kibwen3y ago

> I presume everyone here is talking about creating branches in the main repo and requesting review before merging the branch to master.

Regardless of whether someone is pushing to a branch in the upstream repo or pushing to a branch in a fork, the workflow is the same either way. At worst, it just means adding a remote if you want to check out someone's code locally.

boomlindeOP3y ago

I've used that workflow before, but it was called "merge requests", presumably because you're not actually requesting anyone to pull from a remote, but requesting someone to merge one branch into another.

doix3y ago

Yeah, it's one of those things where the term everyone uses strayed from the etymology. Probably because in the GitHub UI it still calls this a pull request despite not pulling from a different remote.

Either way, if you read the thread, the folks are arguing that giving someone the power to block code getting into the mainline is bad. Judging by what you described, where someone has to approve before it can be merged, we both agree to disagree with them. We are just arguing the semantics of how that is implemented.

andrewflnr3y ago

That process sounds isomorphic to a PR.

zo13y ago

I don't understand at all what you're saying. Sounds like you push your code to a branch called master that tracks master but sits on a separate repo and then you apply your commits after approval via patches? Seems like you're recreating the concept of branches using repos and then using patches instead of merging.

Either way, just so you know, this kind of attitude and that you think it's a-ok to use such a convoluted process for what is effectively the same thing would 100% mean me not hiring you. That may not mean much on some random forum on the net, but you will encounter it a lot as the industry is definitely not aligned to this odd flow you described.

boomlindeOP3y ago

I'm not sure how you read what you're suggesting into my description. I make a commit.

    git commit
    # ...
    git commit # or two

I push them to the staging area for a given branch, in this case master

    git push HEAD:refs/for/master

This isn't a branch in the traditional sense but git conveniently sees it that way.

Now they're immediately available for review. That looks something like this (this is not from my workplace, but the Go team's Gerrit instance): https://go-review.googlesource.com/c/go/+/361916

> Either way, just so you know, this kind of attitude and that you think it's a-ok to use such a convoluted process for what is effectively the same thing would 100% mean me not hiring you.

That's fine. There are plenty of companies that manage to hire based on competence, experience and references so I'm not exactly aching to get hired at a place that would deny me for having used Gerrit.

j / k navigate · click thread line to collapse

0 comments

doix3y ago

> Creating a separate branch, pushing this to your public copy of the repository and then asking someone to pull from that branch into their master branch seems absurd to me.

kibwen3y ago

> I presume everyone here is talking about creating branches in the main repo and requesting review before merging the branch to master.

boomlindeOP3y ago

doix3y ago

andrewflnr3y ago

That process sounds isomorphic to a PR.

zo13y ago

boomlindeOP3y ago

I'm not sure how you read what you're suggesting into my description. I make a commit.

    git commit
    # ...
    git commit # or two

I push them to the staging area for a given branch, in this case master

    git push HEAD:refs/for/master

This isn't a branch in the traditional sense but git conveniently sees it that way.

Now they're immediately available for review. That looks something like this (this is not from my workplace, but the Go team's Gerrit instance): https://go-review.googlesource.com/c/go/+/361916

> Either way, just so you know, this kind of attitude and that you think it's a-ok to use such a convoluted process for what is effectively the same thing would 100% mean me not hiring you.

j / k navigate · click thread line to collapse