Date:   Fri, 8 Sep 2023 10:18:19 +0200
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     linux-mtd@...ts.infradead.org, Richard Weinberger <richard@....at>,
        Tudor Ambarus <Tudor.Ambarus@...aro.org>,
        Vignesh Raghavendra <vigneshr@...com>,
        Frieder Schrempf <frieder.schrempf@...tron.de>,
        Michael Walle <michael@...le.cc>,
        Pratyush Yadav <pratyush@...nel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [GIT PULL] mtd: Changes for v6.6-rc1

Hi Linus,

torvalds@...ux-foundation.org wrote on Mon, 4 Sep 2023 11:22:08 -0700:

> On Mon, 4 Sept 2023 at 01:28, Miquel Raynal <miquel.raynal@...tlin.com> wrote:
> >
> > Back in 2020, you complained about one of my pull requests with:
> >  
> > > You didn't even mention the stm32 controller change, which seems to be
> > > the biggest individual thing in here..  
> >
> > And indeed that was a mistake on my side, but I received that comment
> > as a request for a more detailed list of what had been touched. That's
> > likely an over interpretation, but it lead me to be more exhaustive so
> > the "you did not mention <this>" would no longer happen.  
> 
> You went from one extreme to another, and what I really would want is
> a nice middle ground: mention the important things, summarize the
> rest, and if something is subtle, please explain it.
> 
> Now, that "something is subtle" may not even happen most of the time -
> particularly in drivers - so that is probably almost never an issue.
> 
> > About your request today, I totally get why you would like something
> > more meaningful, but I don't know how to do it. Sometimes I get series
> > which have a goal and I could definitely try to capture that goal in
> > the summary rather than listing the patches.  
> 
> Exactly. If you have a series with a goal, please mention / explain
> the goal - not the details of the series.
> 
> But, as you say:
> 
> > But then, what about the
> > endless list of miscellaneous patches to fix the style, the W=1
> > warnings, various robot complains...  
> 
> Absolutely. That's generally the bulk of any subsystem, and then all
> you need to do is mention it as a kind of "this is what happened".
> 
> When I complained back in 2020, it was bvecause you didn't mention
> even the big changes. Although quite often "big" changes can also just
> be "a lot of cleanup", so mentioning it as such is also fine.
> 
> >  Because this is what I mostly get
> > currently, and I believe there is no way you'll prefer something like
> > this:
> > * Fix misc typos
> > * Fix misc style fixes
> > * Update to newer API's
> > Or maybe it is as long as the patches are trivial?  
> 
> Absolutely. That's _exactly_ what I want for trivial patches
> (including if it's a series of 15 trivial patches that all do the same
> thing to 15 different drivers).
> 
> Instead of mentioning the individual patches, just say exactly the
> above kind of "Update to new helper APIs", or "Fix warnings reported
> by syzbot".
> 
> Honestly, for pure "endpoint" drivers, that's kind of the expected
> explanation. Drivers themselves seldom have any conceptually big
> changes, and so the above kind of things is normal and expected.  So
> then you have exactly that kind of  "Fix W=1 warnings" comment without
> any more specificity.
>
> Then, occasionally you have one driver that gets a lot of work because
> somebody goes in and cleans up that driver in _particular_, and so if
> one particular driver stands out because a vendor (or an individual)
> just decided to do a lot of spring cleaning, then you might have a
> much more specific "Lots of work on cleaning up the XYZ driver"
> mention.

Clear.

> Just as an example, let's look at some of the recent driver merges I
> did, and take something like SCSI where not a lot of interesting stuff
> happened. The mention was just
> 
>      "Updates to the usual drivers (ufs, lpfc, qla2xxx, mpi3mr, libsas) and
>       the usual minor updates and bug fixes but no significant core changes"
> 
> and that's ok. It was a lot of small patches that didn't do anything
> that you'd really care about unless you had some specific interest in
> a driver.
> 
> But I mention that merge message because it is worth noting that I
> actually complained a bit to James about it, because that pull also
> did end up having one thing that stood out if you looked at the
> diffstat: it removed the UFS HPB support entirely. Nobody *really*
> cares about it (because it was never used), which was James' argument
> for not mentioning it, but it's the kind of thing that *does* stand
> out and might be worth mentioning even if it's just a curiosity. It's
> a _conceptually_ interesting part of the pull, even if it doesn't
> actually matter in the real world.
>
> So I give that merge message as an example of both a perfectly fine
> thing in general, but also as an example of "yeah, it could certainly
> have been better". Just to give you kind of an idea what I'm looking
> for.
> 
> And don't get me wrong: sometimes I really appreciate - and want - a
> lot more. *IF* there are major ABI changes, I not only want them
> mentioned, I really want them explained.

Duly noted.

> They *probably* don't actually happen for the MTD subsystem very much,
> if at all, but to give an example of somebody who does do that kind of
> "ABI changes that can affect a lot of other maintainers", we have
> things like the VFS layer that then affects multiple different
> filesystems, and then that shows up in the merge messages as big
> explanations. Or new system calls, or things like that, which affect
> not only the internal kernel ABI, but that actually exposes new
> user-space ABI. Then the explanations can be tens of lines of "this is
> what's going on".
> 
> So examples from the VFS layer on *those* kinds of changes:
> 
>     git show 475d4df82719
>     git show c0a572d9d32f
> 
> and no, I do not expect the MTD drivers to ever do that.
> 
> But to give a recent example of a nice middle road from a merge I did
> yesterday, look at the phy pull from yesterday, or the HID updates
> from Friday. They have slightly different approaches to the summary,
> but both of those make me feel like they are explaining what went on
> in some simple summary without bogging down in excessive detail:"
> 
>     git show db906f0ca6bb
>     git show 29aa98d0fe01

That one mentions the main authors, I believe there is no added value
doing this as it takes time to write and would be easily catch with a
git log.

But otherwise, I think I get what you expect.

> Anyway, all those examples are meant as just that - *examples*. It
> obviously depends entirely on what has been going on, and different
> subsystems can have very different rules. And often "core changes" to
> the subsystem are much more worthy of mention than some cleanup of
> individual drivers.
> 
> A merge message of just a single line of "Trivial cleanups to drivers"
> can be entirely acceptable. But then I do expect to just see pretty
> much completely mindless one- or few-liners.
> 
> Or a merge message might be 30+ lines of explanation, but then I do
> expect it to be some major new feature or re-organization that will
> affect end-users or other subsystems.
> 
> So there is no one single "correct" thing.

I believe I get the idea. I will try to match your expectations in my
next pull requests, please do not hesitate to share your thoughts if
you think it could be further improved in a specific way.

Thanks,
Miquèl

Powered by blists - more mailing lists