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.
Greg
