On Fri, Oct 30, 2015 at 2:38 PM, Uwe Bonnes < [email protected]> wrote:
> >>>>> "Andreas" == Andreas Fritiofson <[email protected]> > writes: > > Andreas> Someone tell me which patches are *truly* ready to merge and I > Andreas> will click the button. The clicking is not the hard part. > > Isn't the "hook" in the "CR" column what is supposed to point to the > patches > that are ready to merge? > Not necessarily. An approved patch might require other patches that are still pending review or have issues. Regardless, I no longer poll Gerrit regularly. I used to do that at least once a week, to review/commit some low-hanging fruit if nothing else, but since about a year I haven't had the time or interest to do even that. > Otherwise there are two different regions where patches touch openocd: > - central functionality everybody needs > - device/board/adapter specific things that maybe only few people have. > > The first category needs stringent code review, I agree. > > But for the second category I find using gerrit counterproductive! > I see what you mean and I kind of agree that the second category has relaxed requirements, however that has less to do with whether or not Gerrit is used. > > Otherwise gerrit also hides the history of a patch. What did happen between > the first appearance of the patch and final application will not get part > of > the git log. Nor should it. The final clean commit is what matters, the winding road there is pretty much irrelevant. We wouldn't want to have the git log cluttered by build failures, dead ends and rebase notices. If there is relevant information in the discussion on Gerrit that doesn't show in the git log, that's a failure on the part of the author of the commit message. The commit message should be self-sufficient and explain the whats, whys and hows of the patch. > Gerrit even offers nothing like a "git log" while the > patch is worked on. This is a substantial shortcoming of gerrit > i.m.h.o. Correct me if I didn't notice some feature. > Not sure what you mean. Everything that happens in Gerrit remains in the change's page for the lifetime of Gerrit. You can see previous incarnations of the change, read the discussion that led to the merge and... well, what else do you need? > > In short: Patches like e.g 3011: "niietcm4: support for NIIET's Cortex-M4 > microcontrollers" should get applied after the two week grace period > mentioned in http://openocd.org/doc/doxygen/html/patchguide.html even > with no > review if nobody objects and the patch merges. > As said, I agree that "category two" changes could have less strict requirements (and they do to some extent). However that doesn't mean that it's a good idea to submit without review. For example take a look at the recent contribution of a STM32F7 flash driver. It was a new flash driver and surely could have been submitted without risk of regressions. The code itself looked reasonable enough and people had actually even voted +1. But the patch was basically duplicating the entire existing STM32F2/4 driver for no good reason at all, a code duplication that would have increased the maintenance efforts required in the future (refactorings, bitrot...). The finally merged STM32F7 support was a few lines of changes to the existing driver, clearly much better. /Andreas
------------------------------------------------------------------------------
_______________________________________________ OpenOCD-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openocd-devel
