Re: [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

[Maxime Ripard <mripard@xxxxxxxxxx>]

On Wed, Jul 12, 2023 at 03:38:03PM +0200, Uwe Kleine-König wrote:
> Hello Maxime,
> 
> On Wed, Jul 12, 2023 at 02:52:38PM +0200, Maxime Ripard wrote:
> > On Wed, Jul 12, 2023 at 01:02:53PM +0200, Uwe Kleine-König wrote:
> > > > Background is that this makes merge conflicts easier to handle and 
> > > > detect.
> > > 
> > > Really?
> > 
> > FWIW, I agree with Christian here.
> > 
> > > Each file (apart from include/drm/drm_crtc.h) is only touched once. So
> > > unless I'm missing something you don't get less or easier conflicts by
> > > doing it all in a single patch. But you gain the freedom to drop a
> > > patch for one driver without having to drop the rest with it.
> > 
> > Not really, because the last patch removed the union anyway. So you have
> > to revert both the last patch, plus that driver one. And then you need
> > to add a TODO to remove that union eventually.
> 
> Yes, with a single patch you have only one revert (but 194 files changed,
> 1264 insertions(+), 1296 deletions(-)) instead of two (one of them: 1
> file changed, 9 insertions(+), 1 deletion(-); the other maybe a bit
> bigger). (And maybe you get away with just reverting the last patch.)
> 
> With a single patch the TODO after a revert is "redo it all again (and
> prepare for a different set of conflicts)" while with the split series
> it's only "fix that one driver that was forgotten/borked" + reapply that
> 10 line patch. As the one who gets that TODO, I prefer the latter.
> 
> So in sum: If your metric is "small count of reverted commits", you're
> right. If however your metric is: Better get 95% of this series' change
> in than maybe 0%, the split series is the way to do it.

I guess that's where we disagree: I don't see the point of having 95% of
it, either 0 or 100.

> With me having spend ~3h on this series' changes, it's maybe
> understandable that I did it the way I did.

I'm sorry, but that's never been an argument? I'm sure you and I both
have had series that took much longer dropped because it wasn't the
right approach.

> FTR: This series was created on top of v6.5-rc1. If you apply it to
> drm-misc-next you get a (trivial) conflict in patch #2. If I consider to
> be the responsible maintainer who applies this series, I like being able
> to just do git am --skip then. 

Or we can ask that the driver is based on drm-misc-next ...

> FTR#2: In drm-misc-next is a new driver
> (drivers/gpu/drm/loongson/lsdc_crtc.c) so skipping the last patch for
> now might indeed be a good idea.

... which is going to fix that one too.

Maxime

Attachment: signature.asc
Description: PGP signature