Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

A better pull request

developer.atlassian.com

Post reply on HN
Print view View original

161–167 of 167 posts

Re: A better pull request

#161

Post by aragot »

efuquen wrote: post #68
rubiquity wrote: post #33

Solution: Don't ever merge branches using a web GUI. Do the merge locally and then push to master. For the past couple of years I've only used GitHub's web-based PR tool for code discussion/peer review. Don't ever click that "merge" button. Another reason I hate the merge button: It creates an extra commit solely for the merge.

That does not solve what I consider the biggest problem, the clean merge that actually results in a logical bug. Without being able to review the diff between the tip of master that will happen with your solution regardless. EDIT : currently I always rebase our feature branches before submitting the PR to try and mitigate this and make sure review happens quickly after that. it doesn't fully solve the problem but it'…

> the clean merge that actually results in a logical bug

Theoretically possible, Probability < 3%. Besides, if master has tests and your branch has a test for the branch's feature, and you still have this problem, then maybe the 2 developers are overlapping so much that they should pair.

Re: A better pull request

#162

Post by lloeki »

sytse wrote: post #135

GitLab B.V. CEO here. The proposed diff is interesting and I love the way they explained the idea. During the development of GitLab (over 10.000 commits) we never experienced the problem that was mentioned. Maybe it is because our codebase contains many tests. One related idea that a GitLab user proposed is not testing the feature branch but testing the merge of the feature branch into master. This would require rete…

I hear GitLab EE has a 'rebase before merge' checkbox next to the 'merge' button. What I'd like to see is have it enforced, that is, the UI won't allow a merge if the MR has not been rebased onto master's tip. Enforcing this policy has saved us a few headaches, including the article's case which happened tree times on a project counting ~2000 commits.

(BTW, we love GitLab, congrats on the new UI layout, which has rave reviews coming back to me mere hours after the upgrade)

Re: A better pull request

#163

Post by [deleted] »

sytse wrote: post #150
z-e-r-o wrote: post #82

Earlier quoted context omitted.

In my case, OS X has the latest git, while Windows is stuck on 1.9, and my git folders get corrupted if I do a push on OS X and simply do a status on Windows. I guess something has changed between 1.x and 2.x but this corruption is really not something which should ever happen in a VCS. My solution is to use Gitlab instead of Dropbox only local repos.

GitLab B.V. CEO here, thanks for using us!

[deleted]

Re: A better pull request

#164

Post by forgottenpass »

snowwrestler wrote: post #95

I'm surprised anyone would be willing to consider merging a branch into master that has not itself pulled the latest head from master. It puts the the person running the merge into the position of having to test someone else's code. It would seem to me that if a branch diffs against an older point in master, then the PR should be rejected as not properly tested. Merging into master should never create a merge conflic…

Or what am I missing?

The linux kernel averages >5 patches per hour. Even with multi-teired maintanership, good luck getting everyone to base their patch sets on the latest upstream head.

I know nobody other than the kernel actually uses git as a distributed system, but there are use cases where your ideal workflow can't actually exist.

Re: A better pull request

#165

Post by ska »

Why hasn't the alice/master branch brought over all of the changes in master since branching, and tested them, before review and (eventual) merge?

Re: A better pull request

#166

Post by erikb »

jjwiseman wrote: post #101
erikb wrote: post #89

I can't really say why, but many things I've read from Atlassian/Bitbucket in recent months always had the feeling of B class (in contrast to A class). Even at the end of the article I still don't get the real problem that is solved. Commiting Mergeconflicts is not an advantage. And that logical error thingy I have never heard or seen before. Maybe because the people (who's opinion I care about) review commit diffs i…

Atlassian's bug tracker (Jira), wiki (Confluence) and CI system (Bamboo) all feel that way to me; B team, or even C. "Atlassian--we make software that you can eventually get to work in some fashion but will never please you in any way." Jira is almost OK, but still has that enterprise stank of trying to do too much and inserting itself too much in your workflow, instead of helping you and getting out of your way. Som…

There are so many very small things. Like Bitbucket adds a web-editor like Github, but the editor automatically replaces all new lines with \r\n and has no field for a commit message (corresponding bug not treated until today and only considered minor priority, maybe because most people use Windows anyway or what). Or the markup language that has the ability to highlight code but only in 3 or so languages. You don't even have the choice to switch to others. The wiki I have only used two or three times. Can't get warm with it either.

Re: A better pull request

#167

Post by rndstr »

latkin wrote: post #156
cpeterso wrote: post #146

Earlier quoted context omitted.

Another option, used by large teams at Facebook, is to only use fast-forwarding but squashes all branch commits. You can still roll-back a merge, but your master's revision history is still linear (for what that's worth).

This is how I prefer to handle PRs, too. `git merge --squash pr-branch`. Prevents the history from turning into an indecipherable tangle of branches, and prevents tons of "fixing typo," "code review feedback," "more code review feedback," "adding back file" kinds of commits from taking over the history. We are on GH, it's quite annoying that they provide 0 support for rebase/squash style PR application. http://stacko…

You can rebase commits in your feature branch as well and clean up the history. Having separate and small commits also helps in blaming and looking at the history do figure out why a certain change is in there.

If you squash all the commits you probably should summarize all the commits into that single commit message which is also work if done properly.

In the end it's preference and how your developers create commits and documment them.

Post reply on HN