[18/95] lib/list_kunit: follow new file name convention for KUnit tests

Commit Message

Andrew Morton Dec. 16, 2020, 4:43 a.m. UTC

From: Andy Shevchenko <[e-mail]>
Subject: lib/list_kunit: follow new file name convention for KUnit tests

Follow new file name convention for the KUnit tests.  Since we have
lib/*test*.c in a few variations, use 'kunit' suffix to distinguish usual
test cases with KUnit-based ones.

Link: https://lkml.kernel.org/r/[e-mail]
Signed-off-by: Andy Shevchenko <[e-mail]>
Reviewed-by: David Gow <[e-mail]>
Acked-by: Brendan Higgins <[e-mail]>
Cc: Mark Brown <[e-mail]>
Cc: Matti Vaittinen <[e-mail]>
Cc: Shuah Khan <[e-mail]>
Cc: Vitor Massaru Iha <[e-mail]>
Signed-off-by: Andrew Morton <[e-mail]>
---

 MAINTAINERS  |    2 +-
 lib/Makefile |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Linus Torvalds Dec. 16, 2020, 6:02 a.m. UTC | #1

This is complete garbage, Andrew.

In this patch, you change the name in the Makefile:

On Tue, Dec 15, 2020 at 8:43 PM Andrew Morton <[e-mail]> wrote:
>
> From: Andy Shevchenko <[e-mail]>
> Subject: lib/list_kunit: follow new file name convention for KUnit tests
>
> Follow new file name convention for the KUnit tests.  Since we have
> lib/*test*.c in a few variations, use 'kunit' suffix to distinguish usual
> test cases with KUnit-based ones.
...
> --- a/lib/Makefile~lib-list_kunit-follow-new-file-name-convention-for-kunit-tests
> +++ a/lib/Makefile
> @@ -350,6 +350,6 @@ obj-$(CONFIG_PLDMFW) += pldmfw/
>
>  # KUnit tests
>  obj-$(CONFIG_BITFIELD_KUNIT) += bitfield_kunit.o
> -obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
> +obj-$(CONFIG_LIST_KUNIT_TEST) += list_kunit.o
>  obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
>  obj-$(CONFIG_BITS_TEST) += test_bits.o

but the actual *file* isn't changed.

So there is no way in hell this will build.

The file is _actually_ renamed in patch [20/95], which claims to do
the lib/bits_kunit stuff, but actually does both the bits _and_ the
list_kunit stuff.

Making things worse, your use of substandard tools means that this
garbage shows up as a patch that is over sixteen *hundred* lines long,
when proper tooling would hav eactually shown it as a rename.

It _should_ have been about 10 lines total.  Not 1600 lines that hide
the problem and make it really nasty to see.

That 1600 lines of noise is ignoring patch [19/95], which does the
"linear_ranges_kunit" renaming, adding another ~500 lines of illegible
garbage to my mailbox.

At least that one got the Makefile right, although it was really hard
to actually see that, considering how nasty and illegible the patch
was due to renaming.

Basically, I'm going to throw all these rename patches away. Not only
were they were completely buggy, but they were also illegible because
of your inferior tools.

Don't send me any more rename patches until your tools can actually do renames.

            Linus

David Gow Dec. 16, 2020, 6:53 a.m. UTC | #2

Hmm... sorry about this: it definitely shouldn't've happened.

I know the original patchset did have an issue (with one of the other
patches) that Andrew fixed while merging, so maybe it snuck through
while that was happening.

On Wed, Dec 16, 2020 at 2:02 PM Linus Torvalds
<[e-mail]> wrote:
>
> This is complete garbage, Andrew.
>
> In this patch, you change the name in the Makefile:
>
> On Tue, Dec 15, 2020 at 8:43 PM Andrew Morton <[e-mail]> wrote:
> >
> > From: Andy Shevchenko <[e-mail]>
> > Subject: lib/list_kunit: follow new file name convention for KUnit tests
> >
> > Follow new file name convention for the KUnit tests.  Since we have
> > lib/*test*.c in a few variations, use 'kunit' suffix to distinguish usual
> > test cases with KUnit-based ones.
> ...
> > --- a/lib/Makefile~lib-list_kunit-follow-new-file-name-convention-for-kunit-tests
> > +++ a/lib/Makefile
> > @@ -350,6 +350,6 @@ obj-$(CONFIG_PLDMFW) += pldmfw/
> >
> >  # KUnit tests
> >  obj-$(CONFIG_BITFIELD_KUNIT) += bitfield_kunit.o
> > -obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
> > +obj-$(CONFIG_LIST_KUNIT_TEST) += list_kunit.o
> >  obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
> >  obj-$(CONFIG_BITS_TEST) += test_bits.o
>
> but the actual *file* isn't changed.
>
> So there is no way in hell this will build.

This is interesting, as the original patch did rename it here:
https://lore.kernel.org/linux-kselftest/[e-mail]/

So something's definitely more muddled than just the renames being
expanded out...

>
> The file is _actually_ renamed in patch [20/95], which claims to do
> the lib/bits_kunit stuff, but actually does both the bits _and_ the
> list_kunit stuff.

And again, that rename is not in the bits_kunit patch in the original thread:
https://lore.kernel.org/linux-kselftest/[e-mail]/

>
> Making things worse, your use of substandard tools means that this
> garbage shows up as a patch that is over sixteen *hundred* lines long,
> when proper tooling would hav eactually shown it as a rename.
>
> It _should_ have been about 10 lines total.  Not 1600 lines that hide
> the problem and make it really nasty to see.
>
> That 1600 lines of noise is ignoring patch [19/95], which does the
> "linear_ranges_kunit" renaming, adding another ~500 lines of illegible
> garbage to my mailbox.
>
> At least that one got the Makefile right, although it was really hard
> to actually see that, considering how nasty and illegible the patch
> was due to renaming.
>
> Basically, I'm going to throw all these rename patches away. Not only
> were they were completely buggy, but they were also illegible because
> of your inferior tools.

Note that the three rename patches (list_kunit, linear_ranges_kunit,
and bits_kunit) were part of the same patchset as the following three
lib/cmdline{,_kunit} changes. Those don't depend on the earlier
patches, don't have renames, and seem to be fine from a cursory
glance, but the last one ([23/95]) did have some issues earlier (which
are now fixed).

>
> Don't send me any more rename patches until your tools can actually do renames.
>
>             Linus

My other thought is that this sort of patchset really makes more sense
to push through the kselftest/kunit branch anyway, as all of the
changes were really more KUnit related than anything else. Does it
make sense to re-submit this that way?

Cheers,
-- David

Linus Torvalds Dec. 16, 2020, 7:01 a.m. UTC | #3

On Tue, Dec 15, 2020 at 10:53 PM David Gow <[e-mail]> wrote:
>
> I know the original patchset did have an issue (with one of the other
> patches) that Andrew fixed while merging, so maybe it snuck through
> while that was happening.

Yeah, I suspect it happened when Andrew was shuffling things around.

And it's probably partly due to the fact that the patches themselves
are _really_ hard to actually read, because all the real changes are
hidden by the huge patches to remove and add files.

When you have 1600 lines of patch, it's really easy to get "patch
blind" and miss the small patch fragments that change the Makefile or
Kconfig file etc.

Back in the days when we worked almost exclusively with patches (ie
before BK and git), we had very high barriers for renaming files
partly for this exact reason.  Renames as patches are just _so_ hard
to read. It's almost completely impossible to see if it's a pure
rename of if something else also changed, when you have one big hunk
that completely removes one file, and another big hunk that completely
adds a new one.

Of course, patch conflicts when there are changes to the files also
then make renames a huge pain in that situation.

With git, we've been *much* more open to file renames, because git
itself handles at least the usual simple cases of merge conflicts
automatically for us, and follows renames etc. And the git diff
extension to actually show renames as renames in a diff make it a
*lot* easier to see what a rename patch actually does.

But as long as renames are treated as patches, I'm going to go back to
the old rules that were "we never rename a file unless there is some
absolutely massive critical reason to do so".

                Linus

Andy Shevchenko Dec. 16, 2020, 10:41 a.m. UTC | #4

On Wed, Dec 16, 2020 at 02:53:10PM +0800, David Gow wrote:
> On Wed, Dec 16, 2020 at 2:02 PM Linus Torvalds
> <[e-mail]> wrote:

...

> > Don't send me any more rename patches until your tools can actually do renames.

> My other thought is that this sort of patchset really makes more sense
> to push through the kselftest/kunit branch anyway, as all of the
> changes were really more KUnit related than anything else. Does it
> make sense to re-submit this that way?

I think it makes sense because this is driven by rules set up by kselftest/kunit.
My main concern here is to have cmdline_kunit in the tree (it is a new file).
Renaming is up to you, I just wanted to be consistent with names and KUnit
documentation.

David Gow Dec. 17, 2020, 9:21 a.m. UTC | #5

On Wed, Dec 16, 2020 at 6:41 PM Andy Shevchenko
<[e-mail]> wrote:
>
> On Wed, Dec 16, 2020 at 02:53:10PM +0800, David Gow wrote:
> > On Wed, Dec 16, 2020 at 2:02 PM Linus Torvalds
> > <[e-mail]> wrote:
>
> ...
>
> > > Don't send me any more rename patches until your tools can actually do renames.
>
> > My other thought is that this sort of patchset really makes more sense
> > to push through the kselftest/kunit branch anyway, as all of the
> > changes were really more KUnit related than anything else. Does it
> > make sense to re-submit this that way?
>
> I think it makes sense because this is driven by rules set up by kselftest/kunit.
> My main concern here is to have cmdline_kunit in the tree (it is a new file).
> Renaming is up to you, I just wanted to be consistent with names and KUnit
> documentation.

It looks like the cmdline_kunit changes have been merged now, so it's
a relief that those weren't held up: I agree that they're the more
important changes.

I do think renaming things to match the new convention is a good idea
(so thanks again for doing that), but it's not exactly an urgent fix.
My preference is that these get added to one of the kunit branches in
the kselftest repo, so they can be picked up when convenient. This
should just be the first three patches in this series:
https://lore.kernel.org/linux-kselftest/[e-mail]/

I'd expect those to still apply pretty cleanly, but I haven't actually
checked yet.

Cheers,
-- David

Andy Shevchenko Dec. 17, 2020, 12:02 p.m. UTC | #6

On Thu, Dec 17, 2020 at 05:21:00PM +0800, David Gow wrote:
> On Wed, Dec 16, 2020 at 6:41 PM Andy Shevchenko
> <[e-mail]> wrote:
> > On Wed, Dec 16, 2020 at 02:53:10PM +0800, David Gow wrote:
> > > On Wed, Dec 16, 2020 at 2:02 PM Linus Torvalds
> > > <[e-mail]> wrote:

...

> > > > Don't send me any more rename patches until your tools can actually do renames.
> >
> > > My other thought is that this sort of patchset really makes more sense
> > > to push through the kselftest/kunit branch anyway, as all of the
> > > changes were really more KUnit related than anything else. Does it
> > > make sense to re-submit this that way?
> >
> > I think it makes sense because this is driven by rules set up by kselftest/kunit.
> > My main concern here is to have cmdline_kunit in the tree (it is a new file).
> > Renaming is up to you, I just wanted to be consistent with names and KUnit
> > documentation.
> 
> It looks like the cmdline_kunit changes have been merged now, so it's
> a relief that those weren't held up: I agree that they're the more
> important changes.

Yes, they are in, thanks Linus!

> I do think renaming things to match the new convention is a good idea
> (so thanks again for doing that), but it's not exactly an urgent fix.
> My preference is that these get added to one of the kunit branches in
> the kselftest repo, so they can be picked up when convenient. This
> should just be the first three patches in this series:
> https://lore.kernel.org/linux-kselftest/[e-mail]/
> 
> I'd expect those to still apply pretty cleanly, but I haven't actually
> checked yet.

They won't apply after v5.11-rc1 due to cmdline_kunit entry. I can rebase after
rc1 and send it to KUnit/kselftest mailing lists.

Andrew, please drop these patches from your quilt and thanks for carrying them.

Patch

--- a/lib/Makefile~lib-list_kunit-follow-new-file-name-convention-for-kunit-tests
+++ a/lib/Makefile
@@ -350,6 +350,6 @@  obj-$(CONFIG_PLDMFW) += pldmfw/
 
 # KUnit tests
 obj-$(CONFIG_BITFIELD_KUNIT) += bitfield_kunit.o
-obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
+obj-$(CONFIG_LIST_KUNIT_TEST) += list_kunit.o
 obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
 obj-$(CONFIG_BITS_TEST) += test_bits.o
--- a/MAINTAINERS~lib-list_kunit-follow-new-file-name-convention-for-kunit-tests
+++ a/MAINTAINERS
@@ -10263,7 +10263,7 @@  M:	David Gow <[e-mail]>
 L:	[e-mail]
 L:	[e-mail]
 S:	Maintained
-F:	lib/list-test.c
+F:	lib/list_kunit.c
 
 LIVE PATCHING
 M:	Josh Poimboeuf <[e-mail]>