policy of no merges from master into PRs?

Programmers discuss here anything related to FreeOrion programming. Primarily for the developers to discuss.

Moderator: Committer

Message
Author
User avatar
Dilvish
AI Lead, Programmer
Posts: 4712
Joined: Sat Sep 22, 2012 6:25 pm

policy of no merges from master into PRs?

#1 Post by Dilvish » Wed Jul 08, 2015 7:06 am

when a pending PR gets into conflict with master, then github can no longer auto-merge it and it must have the conflicts be manually resolved either during a manual merge process or during a manual rebasing process. We've seen a number of cases now where someone who submitted a PR and then merged master into their pending branch made mistakes with the merge, sometimes they have been easy to spot but other times it has taken a while and been a pain. Especially if there were a lot of files involved in the merge, it can be nearly impossible for anyone who didn't actually do the merge to notice that it had conflicts & to check if they got resolved correctly, because the changes would be buried under all the other proper changes.

So, I wanted to at least start (or reopen) a discussion of whether to have a policy of no merges like that in PRs, that if conflicts arise with current master then we ask the person to make a new branch rebased to current master and submit a new PR from that. They would still have to do the conflict resolution process, but at least it wouldn't wind up being buried under a ton of unrelated changes like it can be when you merge master into a branch. So it seems like it would be a lot more reliable for us.
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
adrian_broher
Programmer
Posts: 1072
Joined: Fri Mar 01, 2013 9:52 am
Location: Germany

Re: policy of no merges from master into PRs?

#2 Post by adrian_broher » Wed Jul 08, 2015 8:13 am

My stance is still: Always rebase to avoid pain, even if there is no merge conflict.

And merging master into the topic branch is a reason for immediate rejection of the PR for me, regardless of contents.
Resident code gremlin
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz

User avatar
Vezzra
Release Manager, Design
Posts: 4664
Joined: Wed Nov 16, 2011 12:56 pm
Location: Sol III

Re: policy of no merges from master into PRs?

#3 Post by Vezzra » Wed Jul 08, 2015 8:40 am

Sounds very reasonable, so, like Marcel, I agree to enforcing the proposed policy.

User avatar
MatGB
Creative Contributor
Posts: 3310
Joined: Fri Jun 28, 2013 11:45 pm

Re: policy of no merges from master into PRs?

#4 Post by MatGB » Wed Jul 08, 2015 1:27 pm

Reasonable, yes, but worth observing we have some users who're learning how to contribute who're even more clueless about the process than me, and Git has a really steep learning curve. I think we need a very simple "how to do this" guide for us, not linking to generic git help articles that frequently have a lot of stuff that's irrelevent.

I'm happy to help writing that sort of thing, but some of it, merging and especially manually resolving conflicts is something I'm still stuck on—when I last had an issue I rebased a new branch and copy/pasted in the code from the two conflicting versions, I'm certain there's a better way to do that (SVN had it) but I was so frustrated I gave up looking.

Having strict rules about keeping Master clean is good, but if they're enforced in such a way that it chases away new contributors it might be counterproductive, we need artists and scripters just as much as we need experienced github users.
Mat Bowles

Any code or patches in anything posted here is released under the CC and GPL licences in use for the FO project.

User avatar
Dilvish
AI Lead, Programmer
Posts: 4712
Joined: Sat Sep 22, 2012 6:25 pm

Re: policy of no merges from master into PRs?

#5 Post by Dilvish » Wed Jul 08, 2015 3:19 pm

MatGB wrote:Reasonable, yes, but worth observing we have some users who're learning how to contribute who're even more clueless about the process than me, and Git has a really steep learning curve.... Having strict rules about keeping Master clean is good, but if they're enforced in such a way that it chases away new contributors it might be counterproductive, we need artists and scripters just as much as we need experienced github users.
Unfortunately, Matt, if we can't rely them to manage to rebase or cherrypick, I don't think we can rely on them to be responsible for a Merge (which are not always trivial)-- that's the crux of it. If one of those contributors really can't take on a rebase or cherrypick, then one of us can decide if we want to handle the rebase or cherrypick, but that's not a reason to let the inexperienced person manage a conflicted merge.

As you noted, the real problem stems from people simply being unfamiliar with git-- we probably all messed up a bit of git conflict resolution during our learning curve, but hopefully we caught those. People can get experience in git conflict resolution during the rebase or cherrypicking process in a way that is much much easier to oversee than it is trying to review some big merge they've done (partly because merges can be quite big and bury things, and partly because merges have two different parent commits, making comparisons more difficult).
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
MatGB
Creative Contributor
Posts: 3310
Joined: Fri Jun 28, 2013 11:45 pm

Re: policy of no merges from master into PRs?

#6 Post by MatGB » Wed Jul 08, 2015 4:05 pm

Dilvish wrote:
MatGB wrote:Reasonable, yes, but worth observing we have some users who're learning how to contribute who're even more clueless about the process than me, and Git has a really steep learning curve.... Having strict rules about keeping Master clean is good, but if they're enforced in such a way that it chases away new contributors it might be counterproductive, we need artists and scripters just as much as we need experienced github users.
Unfortunately, Matt, if we can't rely them to manage to rebase or cherrypick, I don't think we can rely on them to be responsible for a Merge (which are not always trivial)-- that's the crux of it. If one of those contributors really can't take on a rebase or cherrypick, then one of us can decide if we want to handle the rebase or cherrypick, but that's not a reason to let the inexperienced person manage a conflicted merge.
Oh, agreed, and it's going to be a problem going forward that we need to deal with.

So, no PRs that contain a merge should be accepted, but we need a Wiki page or a Forum thread (I prefer the former linked to the latter) that we link to in comment whenever we close/reject a PR that should explain the basics of what's needed to be doing?

Cherry Picking is a dead easy thing to do when you've seen the visual thing and you've got your head around branches, but I still have problems creating branches and using them properly at times, etc.

I suspect we basically need to go through the Git howto/questions thread and summarise it into a Wiki page.

(I mentioned this to Dilvish privately, but I have a browser pluging called CoLT (Copy Link Text) that allows me to right click on any page and copy a link to it in any format, Markdown and BBcode are built in, dead useful for this sort of thing).

Basically, saying "please do this" for some is so offputting they never actually do it, our regular contributors would know to come here but the big advantage of Git is that anyone can help, and for some it could be their first contact with us, "please do this, here's a link explaining how" and have that link go through the *(very) basics, plus have coding standards linked to, etc (I nearly submitted a chunk of tabs earlier today, reconfiguring Kate took a bit of effort, etc).

Git allows us to grow the project, and it allows much easier code review than SVN did, those are both really good things. But if we want to grow it more, then we need to be, well, helpful and welcoming, especially to people who're probably embarrassed they've messed things up ;-)
Mat Bowles

Any code or patches in anything posted here is released under the CC and GPL licences in use for the FO project.

User avatar
Cjkjvfnby
AI Contributor
Posts: 452
Joined: Tue Jun 24, 2014 9:55 pm

Re: policy of no merges from master into PRs?

#7 Post by Cjkjvfnby » Wed Jul 15, 2015 10:09 pm

adrian_broher wrote:My stance is still: Always rebase to avoid pain, even if there is no merge conflict.

And merging master into the topic branch is a reason for immediate rejection of the PR for me, regardless of contents.
Why you don't rebase changes instead of merge pull requests?

All documents about rebase say don't f*#kup public branches. (for example https://www.atlassian.com/git/tutorials ... -rebasing/)

Rebasing just before merging feature to master looks ok. (Before merge not 3 times during discussion)

Rebasing and cherry-picking can take more time then merges. (It is not productive and pleasant time)
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
Dilvish
AI Lead, Programmer
Posts: 4712
Joined: Sat Sep 22, 2012 6:25 pm

Re: policy of no merges from master into PRs?

#8 Post by Dilvish » Wed Jul 15, 2015 11:51 pm

Cjkjvfnby wrote:Why you don't rebase changes instead of merge pull requests?
When there are no conflicts and Github can automatically merge a Pull Request, that is a reliable process and so is fine. If people have done a merge in their pending Pull Request, we don't know if it had any conflicts or was done automatically. If there hadn't been any conflicts, then they could have done an automatic rebase as easily as an automatic merge. If there had been conflicts, it seems that people do a less reliable job of managing that conflict resolution via the merge process than they do via cherrypicking. Also, if they merge master into their branch then it is much more difficult for us to review than if they have done either a rebase or a new branch with cherrypicking.
Rebasing and cherry-picking can take more time then merges. (It is not productive and pleasant time)
I'm pretty doubtful about conflict resolution during merge being faster than it is during rebase-- how would that be so? As far as the new branch with cherrypicking goes, it might take a slight bit more time upfront, but really, the extra time selecting the commits is pretty trivial compared to the time it takes to actually resolve conflicts, and makes for a much more reliable product that saves everyone else a great deal of time reviewing it. It's a bit like commenting your code, you invest the time into it primarily for a more reliable product that others can work with more easily.

Regarding reliability, so far the worst mistake I have seen anyone do in conflict resolution via rebasing or cherry picking was the inclusion of an extra blank line, and it was trivially easy to notice, which would not at all have been the case if they had merged master into their branch. We have gottten far, far worse problems from people doing manual merges.
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
Cjkjvfnby
AI Contributor
Posts: 452
Joined: Tue Jun 24, 2014 9:55 pm

Re: policy of no merges from master into PRs?

#9 Post by Cjkjvfnby » Thu Jul 16, 2015 7:53 am

Dilvish wrote:I'm pretty doubtful about conflict resolution during merge being faster than it is during rebase-- how would that be so?
I change same line 3 times in separate commits.
I got changes in master in that line, but not related to my changes, I want to preserve them in my result.

How many times I should merge if I cherry-pick my commits to new branch created from master?
How many times I should merge if I rebase my commits if I rebase master to my branch?
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
adrian_broher
Programmer
Posts: 1072
Joined: Fri Mar 01, 2013 9:52 am
Location: Germany

Re: policy of no merges from master into PRs?

#10 Post by adrian_broher » Thu Jul 16, 2015 8:29 am

Cjkjvfnby wrote:All documents about rebase say don't f*#kup public branches. (for example https://www.atlassian.com/git/tutorials ... -rebasing/)
There is only one set of 'public branches' for this project: the branches in the freeorion/freeorion repository on github. Everything else is not intended for public use and I don't care about the policies of repositories, may it be a fork of FO or not. However I care about reviewable and understandable code, even more so when reviewing code history, and merge commits are the complete opposite of it. Lumping together topic code and unrelated code just makes it harder or even completely impossible to understand whats going on.
Cjkjvfnby wrote:Rebasing just before merging feature to master looks ok. (Before merge not 3 times during discussion)
If you're able to fix all issues in a PR immediately you can go away with one rebase. If not, you will need to rebase as often as the PR code is still faulty and you have fixed anything.
Cjkjvfnby wrote:Rebasing and cherry-picking can take more time then merges.
I call Bullshit. If there is no conflicts it makes no difference. If you need to resolve conflicts it takes the same amount of time to resolve the conflict as you would have to resolve the conflict inside the merge commit. But when rebasing you change your to code to resolve the conflict while inside the merge you have the conflict resolve and all the code change mashed together one big and unreadable diff.
Cjkjvfnby wrote:(It is not productive and pleasant time)
It's not a productive and please time to decipher merge commits.
Cjkjvfnby wrote:I change same line 3 times in separate commits.
Then you probably you have to ask yourself:

Why didn't I squash and cleanup the topic branch properly beforehand to avoid problems like this and make reviewing easier?
Is the topic branch I'm working on too big or are those in fact multiple topics?

Also my experience shows me that rebase doesn't conflict as often as one would expect. I have local branches sitting here for month that I rebase on a regular (time) base. Still they never conflicted with anything.
Cjkjvfnby wrote:How many times I should merge if I rebase my commits if I rebase master to my branch?
As often as you have reviewable code that can be proposed as a PR. If it's one time: great for you. If it's 10 times: not so great for you; you should seriously consider reviewing the way you work.
Resident code gremlin
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz

User avatar
Dilvish
AI Lead, Programmer
Posts: 4712
Joined: Sat Sep 22, 2012 6:25 pm

Re: policy of no merges from master into PRs?

#11 Post by Dilvish » Thu Jul 16, 2015 4:28 pm

Cjkjvfnby wrote:I change same line 3 times in separate commits.
I got changes in master in that line, but not related to my changes, I want to preserve them in my result.
This kind of conflict just came up for me. Apparently while reviewing a recent PR of mine, Geoff got it in his head to do a tiny bit of low value formatting grooming to one of the lines changed in my PR, making a conflict for my PR. (BTW, Geoff, I submit that to create code conflicts with such low value grooming changes (whether knowingly or carelessly) is both rude and a *Bad Idea* from a process reliability point of view, even if it is fairly straightforward for me to resolve).

I had changed that line in two of the commits, so would have had to resolve the conflict twice. Once I had the PR branch finished, I made a cleanup copy of the PR branch. I then squashed the 4 commits down to 2, where only one of them involved the conflicted line. I then compiled and ran to make sure I didn't goof the squash (no conflict resolution was done yet). I then again copied this branch to a new one, which I then rebased against master, only needing to do the conflict resolution one time instead of two. I again compiled & ran, and then pushed to github (for me, by checking out master, cherrypicked those two commits from that final cleanup branch, and pushing, but I could have just submitted the final cleanup branch as a new clean PR).
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 12268
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: policy of no merges from master into PRs?

#12 Post by Geoff the Medio » Thu Jul 16, 2015 4:42 pm

Dilvish wrote:I submit that to create code conflicts with such low value grooming changes (whether knowingly or carelessly) is both rude and a *Bad Idea*
Meaning trivial grooming commits should never be pushed, or that they can't be done while someone else is working on the same code? Making sure that nobody's working in conflicting locations seems a difficult to do reliably, though does seem like it would be preferable to creating unnecessary merge conflicts...

User avatar
Dilvish
AI Lead, Programmer
Posts: 4712
Joined: Sat Sep 22, 2012 6:25 pm

Re: policy of no merges from master into PRs?

#13 Post by Dilvish » Thu Jul 16, 2015 8:11 pm

Geoff the Medio wrote:
Dilvish wrote:I submit that to create code conflicts with such low value grooming changes (whether knowingly or carelessly) is both rude and a *Bad Idea*
Meaning trivial grooming commits should never be pushed, or that they can't be done while someone else is working on the same code? Making sure that nobody's working in conflicting locations seems a difficult to do reliably, though does seem like it would be preferable to creating unnecessary merge conflicts...
This wasn't a situation like "making sure no one else is working on the code" -- a few hours after commenting specifically on my changes to SitrepEntry.h, you make minor grooming changes to at least one of the same lines in that file. I was willing to grant that this could have been an 'oops, I forgot' situation (as unlikely as that seemed) but even that would still be careless enough that I wanted to point it out to you so that you would be at least a bit more careful in the future.
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
Cjkjvfnby
AI Contributor
Posts: 452
Joined: Tue Jun 24, 2014 9:55 pm

Re: policy of no merges from master into PRs?

#14 Post by Cjkjvfnby » Thu Jul 16, 2015 9:00 pm

adrian_broher wrote:There is only one set of 'public branches' for this project: the branches in the freeorion/freeorion repository on github. Everything else is not intended for public use and I don't care about the policies of repositories, may it be a fork of FO or not.
Good position :)
I call Bullshit. If there is no conflicts it makes no difference. If you need to resolve conflicts it takes the same amount of time to resolve the conflict as you would have to resolve the conflict inside the merge commit.
Same but not the same.

Then you probably you have to ask yourself:

Why didn't I squash and cleanup the topic branch properly beforehand to avoid problems like this and make reviewing easier?
Is the topic branch I'm working on too big or are those in fact multiple topics?
Oh, yes. Squashing and rebasing own commit is not same as merges. It can't produce any errors and take no time to do.

You mix code and history in your answer. It is not same for me. Clean history does not produce clean code, clean code can be with shitty history.
Dilvish wrote: I had changed that line in two of the commits, so would have had to resolve the conflict twice. Once I had the PR branch finished, I made a cleanup copy of the PR branch. I then squashed the 4 commits down to 2, where only one of them involved the conflicted line. I then compiled and ran to make sure I didn't goof the squash (no conflict resolution was done yet). I then again copied this branch to a new one, which I then rebased against master, only needing to do the conflict resolution one time instead of two. I again compiled & ran, and then pushed to github (for me, by checking out master, cherrypicked those two commits from that final cleanup branch, and pushing, but I could have just submitted the final cleanup branch as a new clean PR).
At least you know what I am talking about when say that rebasing can require more actions then merge.

Small branch is not good place to compare rebase vs merge. I will join this discussion when big branch with a lot of commits will be maintained by developers team.
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
Cjkjvfnby
AI Contributor
Posts: 452
Joined: Tue Jun 24, 2014 9:55 pm

Re: policy of no merges from master into PRs?

#15 Post by Cjkjvfnby » Thu Sep 24, 2015 4:46 am

It time to return to this thread.

We have long development branch. https://github.com/Dilvish-fo/freeorion ... esearch-ai
I have forked branch https://github.com/Cjkjvfnby/freeorion/tree/research_ai

Both branchws are not compatiable with curent test build.

What I want:
Master merged to Dilvish often (onece a week when new test build comes or when related changed in master done).
I want to fork this branch and merge my changes to it often too.

What you suggest me to do?
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

Post Reply