Sorry, I misunderstand that you suggest this will be the new workflow. I agree that before the new process is ready, your process should continue as before since you have most experience and insight how the whole thing work together. But the committer is growing and most of us don't have that capablity now, so the more strict workflow with automation tool can avoid the risk.
On Wed, Jan 1, 2020 at 1:36 PM Gregory Nutt <spudan...@gmail.com> wrote: > That list just documents how I have handled the me-only workflow in the > past. It is not the workflow that is being defined by Nathan and Brennan. > > I don't appreciate a lot of advice and criticism from people who > contribute nothing to this process. If everyone thinks I can going > continue handling all changes, with no help from any one but on criticism, > you are all wrong. I won't do that. That is totally unfair. > > Thanks > On 12/31/2019 11:25 PM, Xiang Xiao wrote: > > > > On Wed, Jan 1, 2020 at 5:09 AM 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. > > > > But I have some concern about this process(especially for style and > squash). > For example, https://github.com/apache/incubator-nuttx/pull/17 contain 5 > patches(change 52 files and 915 lines) to clean up the network stack, but > the final result on master have one huge patch. > The problem include: > 1.It's difficult to find all modification relate to a particular issue > 2.It's difficult to revert one patch if that patch make the regression > 3.The owner info will lost if the patchset is contrubted by multiple > authors > 4.The owner mayn't agree the change made by committer without any > discussion > 5.It's hard to master the real change if style patch squash into the > functionality change > And the most important is that the whole complex process is done manually > and directly merge into master, NO ANY TOOL has the chance to verify the > change. > Why we can't "Rebase and merge" PR without any modification after > reviewing to keep the full history? > [image: Annotation 2020-01-01 124839.jpg] > If there are any issues, the best way is: > 1.Add the comment in PR > 2.Owner fix the problem or explain the reason > 3.Update the PR again > And github can trigger our tool to check PR when the owner create or > update PR. > > > > 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. > > We need fix the problem: once PR is created, the build job should start > automatically to very the change doesn't break the build. > Actually, the committer don't need review the code before PR pass all > automation check. > > > > > 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 > > > > > >
