Thanks for your reply, I think you've raised some good points. I will attempt to address them below, in full.
I suppose I could have been more clear in defining what "review" means, but this confusion seems to (in my mind) largely be the result of not having a patch review culture or mindset within the project. Patch review does not always require spending hours poring over every line of code: it can, and frequently should, begin with someone briefly checking the patch to see if it's even something beneficial to the project. If someone submits a patch to remove all authors from the AUTHORS file, I would not need to review the content of the patch in order to reject it. Similarly, if someone submits a large patch implementing a much-requested feature then I would know that the eventual result of the patch should be worthwhile even without reviewing its content; the first version may not be acceptable upon in-depth review, but giving an initial response at least allows the author to know that their work is both seen and appreciated. To restate and completely define every term used: my proposal is that every patch submitted during a release cycle must be reviewed prior to the release deployment. Definitions of terms used: PATCH: any change to the efl repo submitted for review in the project's patch review system (currently phabricator) SUBMITTED DURING: the date on the post of the phabricator patch is during the release cycle RELEASE CYCLE: any major release development time period including both the feature and feature freeze sections of the period, excluding the time which begins 48h prior to the scheduled "final" release time, where the 'scheduled "final" release time' is defined as 'the time set as of the deployment of the last pre-release prior to the "final"' MUST: this is a requirement for the release to be deployed REVIEWED: someone in the commiters project group ( https://phab.enlightenment.org/tag/committers/) must comment on a patch, either rejecting it (entirely or because it requires changes), accepting it (stating intent to merge during the release cycle, intent to merge after the release cycle, or as a conceptual approval of a patch which needs subsequent and more in-depth review by someone else), or simply commenting on it to indicate whether it is a beneficial change which should eventually be accepted PRIOR: any point in time before the release is deployed RELEASE DEPLOYMENT: the point at which mails are sent for the announcements of the tarballs I've added to this proposal an explicit 48h gap prior to the release in order to ensure that the release is not unnecessarily delayed by individuals submitting huge numbers of patches immediately prior to the scheduled release time. Patches submitted after the defined 'release cycle' time period are considered as submitted during the next release cycle. The delay of a release (e.g., due to a newly-discovered showstopper bug) after the last pre-release would not affect the 48h time point as it applies to the time as scheduled at the time of the last pre-release deployment; once this point has been reached, all patches submitted are considered part of the next release cycle and are not required to be reviewed prior to deploying the release. A simple way to check the open patch submissions is to use a query like this: https://phab.enlightenment.org/differential/query/gIFEVr3jmk.q/#R Going forward, all committers group members on phabricator are automatically added to all patch submisisons for the efl repo which should further increase patch visibility. If any part of this needs further elaboration, please let me know. On Thu, May 24, 2018 at 10:05 AM Stefan Schmidt <[email protected]> wrote: > Hello. > > On 24.05.2018 15:37, Mike Blumenkrantz wrote: > > I think this argument of not having enough time to review before a > release > > is a bit questionable. If we lack the resources to perform at least a > > cursory review of incoming patches before a release, how can we possibly > > have the capability as a project to even execute a quality release? Is > the > > claim then that we are instead using these resources to review patches > > which have reached the repo without review? > > > > Getting into the habit of, at minimum, glancing at patches and > determining > > if it's something worthwhile for the current release cycle (e.g., a bug > > fix) seems like it would be beneficial overall. > > That is very much different from what you wrote in your first mail. Now > you are asking for "cursory review" and glancing at patches to see if > they should be in the release. > > In your first mail this sounded very different "I think we should make > it a policy going forward that the stabilization period cannot begin > until all pending patch submissions have been reviewed." > > If I take this literally the way you wrote it, something you requested > before to do with your mails, this means freeze on hold until the patch > queue is empty. > > You never wrote there that the "review" you mean in this case was > glancing over the patches and determine if they are bug fixes that > should go into a release. > > The last one is reasonable and is something I support. In contrast to > the first statement where I have to put everything on hold until the > whole patch queue is fully reviewed. Its that part I find unrealistic. > > regards > Stefan Schmidt > > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > enlightenment-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ enlightenment-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
