FreeOrion

Forums for the FreeOrion project
It is currently Mon Dec 11, 2017 1:48 am

All times are UTC




Post new topic Reply to topic  [ 20 posts ]  Go to page 1, 2  Next
Author Message
PostPosted: Wed Jul 08, 2015 7:06 am 
Offline
AI Lead, Programmer
User avatar

Joined: Sat Sep 22, 2012 6:25 pm
Posts: 4390
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


Top
 Profile  
 
PostPosted: Wed Jul 08, 2015 8:13 am 
Offline
Programmer
User avatar

Joined: Fri Mar 01, 2013 9:52 am
Posts: 1040
Location: Germany
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


Top
 Profile  
 
PostPosted: Wed Jul 08, 2015 8:40 am 
Offline
Release Manager, Design
User avatar

Joined: Wed Nov 16, 2011 12:56 pm
Posts: 4284
Location: Sol III
Sounds very reasonable, so, like Marcel, I agree to enforcing the proposed policy.


Top
 Profile  
 
PostPosted: Wed Jul 08, 2015 1:27 pm 
Offline
Creative Contributor
User avatar

Joined: Fri Jun 28, 2013 11:45 pm
Posts: 3286
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.


Top
 Profile  
 
PostPosted: Wed Jul 08, 2015 3:19 pm 
Offline
AI Lead, Programmer
User avatar

Joined: Sat Sep 22, 2012 6:25 pm
Posts: 4390
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


Top
 Profile  
 
PostPosted: Wed Jul 08, 2015 4:05 pm 
Offline
Creative Contributor
User avatar

Joined: Fri Jun 28, 2013 11:45 pm
Posts: 3286
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.


Top
 Profile  
 
PostPosted: Wed Jul 15, 2015 10:09 pm 
Offline
AI Contributor
User avatar

Joined: Tue Jun 24, 2014 9:55 pm
Posts: 444
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


Top
 Profile  
 
PostPosted: Wed Jul 15, 2015 11:51 pm 
Offline
AI Lead, Programmer
User avatar

Joined: Sat Sep 22, 2012 6:25 pm
Posts: 4390
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.

Quote:
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


Top
 Profile  
 
PostPosted: Thu Jul 16, 2015 7:53 am 
Offline
AI Contributor
User avatar

Joined: Tue Jun 24, 2014 9:55 pm
Posts: 444
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


Top
 Profile  
 
PostPosted: Thu Jul 16, 2015 8:29 am 
Offline
Programmer
User avatar

Joined: Fri Mar 01, 2013 9:52 am
Posts: 1040
Location: Germany
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


Top
 Profile  
 
PostPosted: Thu Jul 16, 2015 4:28 pm 
Offline
AI Lead, Programmer
User avatar

Joined: Sat Sep 22, 2012 6:25 pm
Posts: 4390
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


Top
 Profile  
 
PostPosted: Thu Jul 16, 2015 4:42 pm 
Offline
Programming, Design, Admin
User avatar

Joined: Wed Oct 08, 2003 1:33 am
Posts: 12039
Location: Munich
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...


Top
 Profile  
 
PostPosted: Thu Jul 16, 2015 8:11 pm 
Offline
AI Lead, Programmer
User avatar

Joined: Sat Sep 22, 2012 6:25 pm
Posts: 4390
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


Top
 Profile  
 
PostPosted: Thu Jul 16, 2015 9:00 pm 
Offline
AI Contributor
User avatar

Joined: Tue Jun 24, 2014 9:55 pm
Posts: 444
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 :)

Quote:
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.


Quote:
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


Top
 Profile  
 
PostPosted: Thu Sep 24, 2015 4:46 am 
Offline
AI Contributor
User avatar

Joined: Tue Jun 24, 2014 9:55 pm
Posts: 444
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


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 20 posts ]  Go to page 1, 2  Next

All times are UTC


Who is online

Users browsing this forum: No registered users and 1 guest


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  
cron
Powered by phpBB® Forum Software © phpBB Group