Uwe Kleine-König
2023-Jul-12 13:38 UTC
[Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
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. With me having spend ~3h on this series' changes, it's maybe understandable that I did it the way I did. 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. 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.> > So I still like the split version better, but I'm open to a more > > verbose reasoning from your side. > > You're doing only one thing here, really: you change the name of a > structure field. If it was shared between multiple maintainers, then > sure, splitting that up is easier for everyone, but this will go through > drm-misc, so I can't see the benefit it brings.I see your argument, but I think mine weights more. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | https://www.pengutronix.de/ | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: not available URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20230712/659ff89d/attachment-0001.sig>
Maxime Ripard
2023-Jul-12 13:53 UTC
[Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
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 -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 228 bytes Desc: not available URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20230712/da788ee6/attachment-0001.sig>
Christian König
2023-Jul-12 13:53 UTC
[PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Am 12.07.23 um 15:38 schrieb Uwe Kleine-K?nig:> 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.Yeah, but for a maintainer the size of the patches doesn't matter. That's only interesting if you need to manually review the patch, which you hopefully doesn't do in case of something auto-generated. In other words if the patch is auto-generated re-applying it completely is less work than fixing things up individually.> As the one who gets that TODO, I prefer the latter.Yeah, but your personal preferences are not a technical relevant argument to a maintainer. At the end of the day Dave or Daniel need to decide, because they need to live with it. Regards, Christian.> > 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. > > With me having spend ~3h on this series' changes, it's maybe > understandable that I did it the way I did. > > 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. > > 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. > >>> So I still like the split version better, but I'm open to a more >>> verbose reasoning from your side. >> You're doing only one thing here, really: you change the name of a >> structure field. If it was shared between multiple maintainers, then >> sure, splitting that up is easier for everyone, but this will go through >> drm-misc, so I can't see the benefit it brings. > I see your argument, but I think mine weights more. > > Best regards > Uwe >
Luben Tuikov
2023-Jul-13 00:06 UTC
[Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On 2023-07-12 09:53, Christian K?nig wrote:> Am 12.07.23 um 15:38 schrieb Uwe Kleine-K?nig: >> 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. > > Yeah, but for a maintainer the size of the patches doesn't matter. > That's only interesting if you need to manually review the patch, which > you hopefully doesn't do in case of something auto-generated. > > In other words if the patch is auto-generated re-applying it completely > is less work than fixing things up individually. > >> As the one who gets that TODO, I prefer the latter. > > Yeah, but your personal preferences are not a technical relevant > argument to a maintainer. > > At the end of the day Dave or Daniel need to decide, because they need > to live with it. > > Regards, > Christian. > >> >> 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. >> >> With me having spend ~3h on this series' changes, it's maybe >> understandable that I did it the way I did. >> >> 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. >> >> 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. >> >>>> So I still like the split version better, but I'm open to a more >>>> verbose reasoning from your side. >>> You're doing only one thing here, really: you change the name of a >>> structure field. If it was shared between multiple maintainers, then >>> sure, splitting that up is easier for everyone, but this will go through >>> drm-misc, so I can't see the benefit it brings. >> I see your argument, but I think mine weights more.I'm with Maxime and Christian on this--a single action necessitates a single patch. One single movement. As Maxime said "either 0 or 100." As to the name, perhaps "drm_dev" is more descriptive than just "drm". What is "drm"? Ah it's a "dev", as in "drm dev"... Then why not rename it to "drm_dev"? You are renaming it from "dev" to something more descriptive after all. "dev" --> "drm" is no better, but "dev" --> "drm_dev" is just right. -- Regards, Luben