Hi,
I have recently made some changes to Proofread extension[1]. There are more in the review queue[2]. This extension needs refactoring and I have some free time to do it.
I prefer making small changes, so it is easy to review and test them. However due to not clearly determined order of merging changes I have no idea what should I choose as my baseline. I prefer to avoid solving merge conflicts with myself. This usually happens when changes got reviewed and merged in for example reverse order. It is clearly a waste of time.
I know gerrit can use dependencies, so I can make a chain of dependant changes: c1 <- c2 <- c3 <- c4. However if c2 and c3 got a positive review and c1 needs some rework, c2 and c3 need to be reviewed again after I submit c1. Sometimes another, unrelated change may be merged, so the whole chain needs to be rebased against master.
The most ideal approach would be to review changes as soon as they are submitted, but I am well aware, that there are not enough people to do the review.
Any thoughts?
[1] https://gerrit.wikimedia.org/r/#/q/status:closed+project:mediawiki/extension...
[2] https://gerrit.wikimedia.org/r/#/q/status:open+project:mediawiki/extensions/...
On 07/05/12 10:26, Beau wrote:
I prefer making small changes, so it is easy to review and test them. However due to not clearly determined order of merging changes I have no idea what should I choose as my baseline. I prefer to avoid solving merge conflicts with myself. This usually happens when changes got reviewed and merged in for example reverse order. It is clearly a waste of time.
I know gerrit can use dependencies, so I can make a chain of dependant changes: c1 <- c2 <- c3 <- c4. However if c2 and c3 got a positive review and c1 needs some rework, c2 and c3 need to be reviewed again after I submit c1.
Sometimes another, unrelated change may be merged, so the whole chain needs to be rebased against master.
No. That doesn't need a rebase. They will just be merged. And -assuming they don't conflict- that should not be a problem.
The most ideal approach would be to review changes as soon as they are submitted, but I am well aware, that there are not enough people to do the review.
Any thoughts?
No better idea than just making dependant changes. You can join them under the same topic, and maybe provide a hint in the changeset comments to note please review ancestors before.
Auto-rebase of non-merged descendants seems a good feature request.
Beau beau@adres.pl wrote:
I know gerrit can use dependencies, so I can make a chain of dependant changes: c1 <- c2 <- c3 <- c4. However if c2 and c3 got a positive review and c1 needs some rework, c2 and c3 need to be reviewed again after I submit c1. Sometimes another, unrelated change may be merged, so the whole chain needs to be rebased against master.
Refactoring with code review is hard... If you refactor code, whole merging magic is usually making more harm than good.
What I would suggest:
* Try to keep refactoring changes separate to functional changes * You can try to separate independent changes and have a common parent for them
Ideally, the commit tree could look like:
P ------ | \ \ | \ \ r1 r2 c1 |\ \ | \ \ c2 c3 c4
r1 - refactoring change 1 r2 - independent refactoring change 2 c1 - independent functional change c2,c3 - functional changes after refactoring 1 c4 - functional change after refactoring 2
You can achieve this by goint back to parent every time you are finished with some other change:
git checkout <parent>
or
git reset --soft <parent> (if you already have changes pending)
Make sure to have separate Change ID's for c2 and c3 otherwise they might end up as two patchsets of the same change.
//Saper
wikitech-l@lists.wikimedia.org