On Fri, 19 Jan 2018, Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> wrote: > + Jani > > On Wed, 2018-01-10 at 17:32 -0800, Rodrigo Vivi wrote: >> On Tue, Jan 09, 2018 at 11:23:09PM +0000, Paulo Zanoni wrote: >> > Hello >> > >> > This is the first series of patches for the Icelake platform. Unlike the other >> > series that introduced new platforms, this one is very small and only contains >> > patches for very basic enabling, interrupts and some GEM code. No patches for >> > display or other subsystems yet and GEM is not complete either. I'm hoping that >> > by splitting Icelake enabling into many small series progress will be better >> > tracked and people only interested in one area of the code will be able to >> > ignore everything else more easily. In addition, except for the first very few >> > patches of this series, many of the sub-series that will follow are independent >> > from each other and can be merged in any order. And on top of everything, >> > tracking down any possible issues identified by the CI system will be easier if >> > the problem is in a series with 20 patches instead of 160 patches. >> >> good idea. >> >> > >> > Another point worth mentioning is that some patches already have Reviewed-by >> > tags. It is important to remind everybody that those tags were often given to >> > some early versions of those patches, and rebasing happened since then due to >> > the fast development pacing of our driver. Reworks may have landed on the >> > upstream driver that we missed while rebasing, so it is likely that some reworks >> > need to be applied to these patches now. I considered just removing the R-B tags >> > before submitting the patches here, but I think it's probably better if we give >> > credit to people who already spent time trying to check for problems in earlier >> > versions of the patches. So, those patches that already have R-B tags need to be >> > re-reviewed now, and special consideration should be given to any rebasing >> > problems. I'd love to see some "R-b tag still stands" emails. >> >> I'm glad you didn't removed the rv-b tags. The review process that >> happened so far was very productive. Let's keep the right credits in place and >> take extra care when merging to dinq. Let's only merge what we are confident >> that review is still valid or ask for re-reviews and extra acks. >> >> One idea that I heard this morning was to use on internal some custom tag >> like "Internally-Reviewed-by:" but I don't like this idea of adding custom >> tags and I trust our commiters to differentiate between valid internal reviews >> and risky ones. Agree? >> >> Thoughts? > > I've been all favour of converting R-b's to Cc:s and embedding any > meaningful changelog entries into the commit text. Because it'll be the > first revision sent to public, you can't trace any of the previous > review comments back by searching mailing lists. It'll only add > confusion. > > I don't see the value added by leaving just the changelog entries to > the commit messages. Quite contrary, they are a potentialcause of > confusion when somebody tries to track down non-existent review history > in public. > > And sending pre-reviewed patches to community mailing lists also > doesn't feel quite right. Even for IRC review the BKM is to respond to > the mailing list and note that the patch received a R-b in IRC, for > documentation purposes. > > And when you add the fact that there is high chance of not invalidating > the reviews when they should be (due to the urgency and amount of > patches there's related to new product development), I see it more as a > problem maker than a solver. > > It has little to give but the trade has much to lose. I don't want to devalue internal review, it's good stuff for the most part. However, there are a few issues. Most patches have seen a bunch of rebases and fixups since the review. Sometimes the review means, good enough for merging internally. And finally, I don't want us to give the impression of internal rubber stamp review. Basically I want every internal Reviewed-by confirmed on the public list. I think this pretty much aligns with what Paulo said in the cover letter. It's a matter of taste whether you require the confirmation of reviews or change them to Cc's and ask for the same. For the changelogs, I agree we should start scrubbing them for v1 posted on the public list. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
