| Message ID | 20240809064222.3527881-1-aliceryhl@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rust: sort includes in bindings_helper.h | expand |
On Fri, Aug 9, 2024 at 8:42 AM Alice Ryhl <aliceryhl@google.com> wrote: > > Dash has ascii value 45 and underscore has ascii value 95, so to > correctly sort the includes, the underscore should be last. > > Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module") Looks good to me (`LC_ALL=C`), thanks! I can take it; otherwise: Acked-by: Miguel Ojeda <ojeda@kernel.org> I am not sure if this should count as a bug/fix (there is an recent/ongoing debate about the Fixes tag). (This kind of issues can be also opened as "good first issues", by the way, i.e. as a way to get contributors to set their email workflow.) Cheers, Miguel
On 8/9/24 8:42 AM, Alice Ryhl wrote: > Dash has ascii value 45 and underscore has ascii value 95, so to > correctly sort the includes, the underscore should be last. > > Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module") I don't think this patch needs a "Fixes" tag, it's usually for bugs only. But it still makes sense to mention the commit that introduced the include in the commit message. > Signed-off-by: Alice Ryhl <aliceryhl@google.com> Generally speaking, unless minor style issues cause compiler or linter warnings, I think it's better to leave them alone in favor of not messing with git-blame. In this case we're not hiding something relevant though, hence Acked-by: Danilo Krummrich <dakr@kernel.org> > --- > rust/bindings/bindings_helper.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index b940a5777330..ae82e9c941af 100644 > --- a/rust/bindings/bindings_helper.h > +++ b/rust/bindings/bindings_helper.h > @@ -7,8 +7,8 @@ > */ > > #include <kunit/test.h> > -#include <linux/blk_types.h> > #include <linux/blk-mq.h> > +#include <linux/blk_types.h> > #include <linux/blkdev.h> > #include <linux/errname.h> > #include <linux/ethtool.h>
On 8/9/24 12:42 AM, Alice Ryhl wrote: > Dash has ascii value 45 and underscore has ascii value 95, so to > correctly sort the includes, the underscore should be last. This commit message lacks an explanation for why the change is being done. Yes it states that it brings the headers in ascii sort order, but WHY?
On Fri, Aug 9, 2024 at 3:07 PM Jens Axboe <axboe@kernel.dk> wrote: > > On 8/9/24 12:42 AM, Alice Ryhl wrote: > > Dash has ascii value 45 and underscore has ascii value 95, so to > > correctly sort the includes, the underscore should be last. > > This commit message lacks an explanation for why the change is > being done. Yes it states that it brings the headers in ascii > sort order, but WHY? I can add the following to the commit message: The headers in this file are sorted alphabetically, which makes it easy to quickly resolve conflicts by selecting all of the headers and invoking :'<,'>sort to sort them. To keep this technique to resolve conflicts working, also apply sorting to symbols that are not letters. Alice
On Fri, Aug 9, 2024 at 3:01 PM Danilo Krummrich <dakr@kernel.org> wrote: > > On 8/9/24 8:42 AM, Alice Ryhl wrote: > > Dash has ascii value 45 and underscore has ascii value 95, so to > > correctly sort the includes, the underscore should be last. > > > > Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module") > > I don't think this patch needs a "Fixes" tag, it's usually for bugs only. > > But it still makes sense to mention the commit that introduced the include > in the commit message. Ok. I can make this change. Alice
On Fri, Aug 9, 2024 at 2:58 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Fri, Aug 9, 2024 at 8:42 AM Alice Ryhl <aliceryhl@google.com> wrote: > > > > Dash has ascii value 45 and underscore has ascii value 95, so to > > correctly sort the includes, the underscore should be last. > > > > Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module") > > Looks good to me (`LC_ALL=C`), thanks! > > I can take it; otherwise: > > Acked-by: Miguel Ojeda <ojeda@kernel.org> > > I am not sure if this should count as a bug/fix (there is an > recent/ongoing debate about the Fixes tag). I fix merge conflicts in this file almost daily, so I think there's a case to be made for taking it as a fix. I should have clarified this in my commit message. I sent a v2 with more info: https://lore.kernel.org/r/20240809132835.274603-1-aliceryhl@google.com > (This kind of issues can be also opened as "good first issues", by the > way, i.e. as a way to get contributors to set their email workflow.) I didn't think of that, but if I had I would probably still have submitted it myself for the above reason. Alice
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index b940a5777330..ae82e9c941af 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -7,8 +7,8 @@ */ #include <kunit/test.h> -#include <linux/blk_types.h> #include <linux/blk-mq.h> +#include <linux/blk_types.h> #include <linux/blkdev.h> #include <linux/errname.h> #include <linux/ethtool.h>
Dash has ascii value 45 and underscore has ascii value 95, so to correctly sort the includes, the underscore should be last. Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module") Signed-off-by: Alice Ryhl <aliceryhl@google.com> --- rust/bindings/bindings_helper.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)