Hi Greg and Nathan, On 12/31/19, Gregory Nutt <spudan...@gmail.com> wrote: > >> Would it make sense, then, to begin a transition period now? That is, >> start a gradual move from the current state where Greg is reviewing >> and merging all changes, toward the direction where other committers >> are reviewing/merging changes. Perhaps, for the next couple of weeks, >> Greg could continue reviewing and merging to the more finicky areas >> like the build system, while other committers start getting their feet >> wet by reviewing and merging changes to areas less likely to bring >> breakage? I think Greg should be mentoring us in the art of handling >> contributions, instead of doing all the drudge work himself. > > That sounds like a good idea. Let me summarize what I do. This is /not > /the official workflow! It is basically the legacy way I have been > doing things translated in to this new environment. It really only > specifically only addresses PRs, but patches would be similar: You > would commit the patch to master or a development branch instead of > merging the PR. Otherwise it is the same: > > * First review the change. If it looks risky, ask for other, > appropriate people to review the change too. > * If the change is a trivial change (like a typo fix) or an obvious > change (like correcting a bad definition), I just merge it directly > to master and we are done. > * Otherwise, review the change. Wait for other review comments if you > have requested them. You may need to ask the contributor to fix > certain things and force push an update. > * When you are satisfied, merge the change onto the 'dev' branch (or a > custom branch of your choosing). > > * First make sure that the dev branch is up-to-date > * cd incubator-nuttx > * git checkout dev > * git rebase master > * git push [--force] origin dev > * --force is a little dangerous. Don't use it if other people are > using the dev branch. This is a good argument for merging the > change onto a custom branch. > * Set the base branch of the PR to the dev branch per > > https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/changing-the-base-branch-of-a-pull-request > * Squash merge the change onto the dev branch. > > * Capture the .c and .h files that were added or modified on the 'dev' > branch. Run nxstyle on every such file. Modify the code as > necessary so that all .c and .h files pass the test (using good > judgement if you don't agree with nxstyle). > * Squash merge the 'dev' branch onto master. The change on master > will include both the original PR and your coding style fixes. > Again, this assumes you are the only user of dev and another reason > to consider using a custom temporary branch. > * Finally, refresh the dev branch (as above) or delete your custom, > temporary branch. > > Notice that there is no attempt to prove that the code builds correctly > and, hence, incorporating the change could easily cause breakage. That > is, in my opinion, more than you can expect from a purely manual > process. I used to run extensive build testing of over 408 ARM > configurations which detects build problems in many (but not all > cases). I have stopped doing that, however. > > So who wants to help? It is really kind of entertaining, provided that > you aren't swamped with more than you can do and provided it does not > take over your life (as it did mine). > >> .... Changes are only merged after an appropriate level of review. > > If someone puts a person down as a review of a PR, then we all need to > take responsibility to provide the review, comments, and approval. For > example, the nxstyle change that was haggled for several days had > reviews requested from 6 people (as I recall). I think I am the only > person that both reviewed, commented, and approved the change. >
Thank you for sharing this step-by-step process. I think it is fine, we will always push the commits the dev branch and only when everything is tested and stable it should be moved to the master. In the future we could have some automated test to run nxstyle and flag the PR as complaint before we review it. Projects using gerrit automatically validates whether the patch could be applied and whether it will break the build or not. Then the contributor needs to send a fine patch before it be reviewed. Happy new year! I wish we have a great Apache NuttX 2020 Year! BR, Alan
