On 19/01/2018 11:45, Joonas Lahtinen 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 agree with some points but also think it is not desirable to just lose
any record of potentially significant review effort that went in before
first public posting.
The only idea I can think of at the moment, if we don't want to use a
separate tag, is to, as you say, squash meaningful change log entries
into the commit, convert the r-b to r-b # internal (similar to r-b # v1
notation), and add the reviewer as cc explicitly:
drm/i915: Some patch title
Some commit text.
Signed-of-by: A
Reviewed-by: B # internal
Cc: B
B then follows up with upgrading the r-b.
?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
