The following issues are fixed:
- WARNING: Missing or malformed SPDX-License-Identifier tag
- WARNING: Prefer __noreturn over __attribute__((noreturn))
- ERROR: trailing statements should be on next line
- WARNING: braces {} are not necessary for single statement blocks
- ERROR: space required before the open parenthesis '('
- ERROR: code indent should use tabs where possible
- WARNING: please, no spaces at the start of a line
- WARNING: Missing a blank line after declarations
Signed-off-by: Franziska Naepelt <[email protected]>
---
certs/extract-cert.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/certs/extract-cert.c b/certs/extract-cert.c
index 70e9ec89d87d..dd76fb0f7f8d 100644
--- a/certs/extract-cert.c
+++ b/certs/extract-cert.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: LGPL-2.1
/* Extract X.509 certificate in DER form from PKCS#11 or PEM.
*
* Copyright © 2014-2015 Red Hat, Inc. All Rights Reserved.
@@ -32,7 +33,7 @@
#define PKEY_ID_PKCS7 2
-static __attribute__((noreturn))
+static __noreturn
void format(void)
{
fprintf(stderr,
@@ -63,7 +64,8 @@ static void drain_openssl_errors(void)
if (ERR_peek_error() == 0)
return;
- while (ERR_get_error_line(&file, &line)) {}
+ while (ERR_get_error_line(&file, &line))
+ ;
}
#define ERR(cond, fmt, ...) \
@@ -73,7 +75,7 @@ static void drain_openssl_errors(void)
if (__cond) { \
err(1, fmt, ## __VA_ARGS__); \
} \
- } while(0)
+ } while (0)
static const char *key_pass;
static BIO *wb;
@@ -107,7 +109,7 @@ int main(int argc, char **argv)
if (verbose_env && strchr(verbose_env, '1'))
verbose = true;
- key_pass = getenv("KBUILD_SIGN_PIN");
+ key_pass = getenv("KBUILD_SIGN_PIN");
if (argc != 3)
format();
@@ -118,6 +120,7 @@ int main(int argc, char **argv)
if (!cert_src[0]) {
/* Invoked with no input; create empty file */
FILE *f = fopen(cert_dst, "wb");
+
ERR(!f, "%s", cert_dst);
fclose(f);
exit(0);
@@ -155,6 +158,7 @@ int main(int argc, char **argv)
x509 = PEM_read_bio_X509(b, NULL, NULL, NULL);
if (wb && !x509) {
unsigned long err = ERR_peek_last_error();
+
if (ERR_GET_LIB(err) == ERR_LIB_PEM &&
ERR_GET_REASON(err) == PEM_R_NO_START_LINE) {
ERR_clear_error();
base-commit: 7877cb91f1081754a1487c144d85dc0d2e2e7fc4
--
2.39.2 (Apple Git-143)
Hi Franziska,
kernel test robot noticed the following build errors:
[auto build test ERROR on 7877cb91f1081754a1487c144d85dc0d2e2e7fc4]
url: https://github.com/intel-lab-lkp/linux/commits/Franziska-Naepelt/certs-extract-cert-Fix-checkpatch-issues/20230602-030657
base: 7877cb91f1081754a1487c144d85dc0d2e2e7fc4
patch link: https://lore.kernel.org/r/20230601190508.56610-1-franziska.naepelt%40gmail.com
patch subject: [PATCH] certs/extract-cert: Fix checkpatch issues
config: mips-buildonly-randconfig-r002-20230602 (https://download.01.org/0day-ci/archive/20230602/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 4faf3aaf28226a4e950c103a14f6fc1d1fdabb1b)
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install mips cross compiling tool for clang build
# apt-get install binutils-mipsel-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/7fdfaec7a3c9f58676a5892e679d8bca319abd8a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Franziska-Naepelt/certs-extract-cert-Fix-checkpatch-issues/20230602-030657
git checkout 7fdfaec7a3c9f58676a5892e679d8bca319abd8a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=mips olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> certs/extract-cert.c:36:8: error: unknown type name '__noreturn'
static __noreturn
^
1 error generated.
vim +/__noreturn +36 certs/extract-cert.c
35
> 36 static __noreturn
37 void format(void)
38 {
39 fprintf(stderr,
40 "Usage: extract-cert <source> <dest>\n");
41 exit(2);
42 }
43
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Franziska,
kernel test robot noticed the following build errors:
[auto build test ERROR on 7877cb91f1081754a1487c144d85dc0d2e2e7fc4]
url: https://github.com/intel-lab-lkp/linux/commits/Franziska-Naepelt/certs-extract-cert-Fix-checkpatch-issues/20230602-030657
base: 7877cb91f1081754a1487c144d85dc0d2e2e7fc4
patch link: https://lore.kernel.org/r/20230601190508.56610-1-franziska.naepelt%40gmail.com
patch subject: [PATCH] certs/extract-cert: Fix checkpatch issues
config: arm-randconfig-r046-20230531 (https://download.01.org/0day-ci/archive/20230602/[email protected]/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/7fdfaec7a3c9f58676a5892e679d8bca319abd8a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Franziska-Naepelt/certs-extract-cert-Fix-checkpatch-issues/20230602-030657
git checkout 7fdfaec7a3c9f58676a5892e679d8bca319abd8a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arm olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All error/warnings (new ones prefixed by >>):
>> certs/extract-cert.c:36:18: error: expected ';' before 'void'
36 | static __noreturn
| ^
| ;
37 | void format(void)
| ~~~~
>> certs/extract-cert.c:37:6: warning: no previous prototype for 'format' [-Wmissing-prototypes]
37 | void format(void)
| ^~~~~~
vim +36 certs/extract-cert.c
35
> 36 static __noreturn
> 37 void format(void)
38 {
39 fprintf(stderr,
40 "Usage: extract-cert <source> <dest>\n");
41 exit(2);
42 }
43
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
The following issues are fixed:
- WARNING: Missing or malformed SPDX-License-Identifier tag
- ERROR: trailing statements should be on next line
- WARNING: braces {} are not necessary for single statement blocks
- ERROR: space required before the open parenthesis '('
- ERROR: code indent should use tabs where possible
- WARNING: please, no spaces at the start of a line
- WARNING: Missing a blank line after declarations
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Franziska Naepelt <[email protected]>
---
v2:
- revert noreturn changes to fix build issues
---
certs/extract-cert.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/certs/extract-cert.c b/certs/extract-cert.c
index 70e9ec89d87d..96c0728bf4d1 100644
--- a/certs/extract-cert.c
+++ b/certs/extract-cert.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: LGPL-2.1
/* Extract X.509 certificate in DER form from PKCS#11 or PEM.
*
* Copyright © 2014-2015 Red Hat, Inc. All Rights Reserved.
@@ -63,7 +64,8 @@ static void drain_openssl_errors(void)
if (ERR_peek_error() == 0)
return;
- while (ERR_get_error_line(&file, &line)) {}
+ while (ERR_get_error_line(&file, &line))
+ ;
}
#define ERR(cond, fmt, ...) \
@@ -73,7 +75,7 @@ static void drain_openssl_errors(void)
if (__cond) { \
err(1, fmt, ## __VA_ARGS__); \
} \
- } while(0)
+ } while (0)
static const char *key_pass;
static BIO *wb;
@@ -107,7 +109,7 @@ int main(int argc, char **argv)
if (verbose_env && strchr(verbose_env, '1'))
verbose = true;
- key_pass = getenv("KBUILD_SIGN_PIN");
+ key_pass = getenv("KBUILD_SIGN_PIN");
if (argc != 3)
format();
@@ -118,6 +120,7 @@ int main(int argc, char **argv)
if (!cert_src[0]) {
/* Invoked with no input; create empty file */
FILE *f = fopen(cert_dst, "wb");
+
ERR(!f, "%s", cert_dst);
fclose(f);
exit(0);
@@ -155,6 +158,7 @@ int main(int argc, char **argv)
x509 = PEM_read_bio_X509(b, NULL, NULL, NULL);
if (wb && !x509) {
unsigned long err = ERR_peek_last_error();
+
if (ERR_GET_LIB(err) == ERR_LIB_PEM &&
ERR_GET_REASON(err) == PEM_R_NO_START_LINE) {
ERR_clear_error();
base-commit: 7877cb91f1081754a1487c144d85dc0d2e2e7fc4
--
2.39.2 (Apple Git-143)
On Fri, Jun 02, 2023 at 10:59:02AM +0200, Franziska Naepelt wrote:
> The following issues are fixed:
> - WARNING: Missing or malformed SPDX-License-Identifier tag
> - ERROR: trailing statements should be on next line
> - WARNING: braces {} are not necessary for single statement blocks
> - ERROR: space required before the open parenthesis '('
> - ERROR: code indent should use tabs where possible
> - WARNING: please, no spaces at the start of a line
> - WARNING: Missing a blank line after declarations
Again, write the patch description in imperative mood (e.g. "Do foo").
> +// SPDX-License-Identifier: LGPL-2.1
> /* Extract X.509 certificate in DER form from PKCS#11 or PEM.
> *
> * Copyright © 2014-2015 Red Hat, Inc. All Rights Reserved.
Nope.
The license boilerplate says LGPL 2.1 or any later version, so the
corresponding SPDX tag should have been:
```
// SPDX-License-Identifier: LGPL-2.1-or-later
```
And please also delete the boilerplate and separate this SPDX conversion
into its own patch.
Thanks.
--
An old man doll... just what I always wanted! - Clara
On Fri Jun 2, 2023 at 11:59 AM EEST, Franziska Naepelt wrote:
> The following issues are fixed:
> - WARNING: Missing or malformed SPDX-License-Identifier tag
> - ERROR: trailing statements should be on next line
> - WARNING: braces {} are not necessary for single statement blocks
> - ERROR: space required before the open parenthesis '('
> - ERROR: code indent should use tabs where possible
> - WARNING: please, no spaces at the start of a line
> - WARNING: Missing a blank line after declarations
>
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
Remove the empty line.
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Franziska Naepelt <[email protected]>
Fixes tag?
> ---
> v2:
> - revert noreturn changes to fix build issues
> ---
> certs/extract-cert.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/certs/extract-cert.c b/certs/extract-cert.c
> index 70e9ec89d87d..96c0728bf4d1 100644
> --- a/certs/extract-cert.c
> +++ b/certs/extract-cert.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: LGPL-2.1
> /* Extract X.509 certificate in DER form from PKCS#11 or PEM.
> *
> * Copyright © 2014-2015 Red Hat, Inc. All Rights Reserved.
> @@ -63,7 +64,8 @@ static void drain_openssl_errors(void)
>
> if (ERR_peek_error() == 0)
> return;
> - while (ERR_get_error_line(&file, &line)) {}
> + while (ERR_get_error_line(&file, &line))
> + ;
> }
>
> #define ERR(cond, fmt, ...) \
> @@ -73,7 +75,7 @@ static void drain_openssl_errors(void)
> if (__cond) { \
> err(1, fmt, ## __VA_ARGS__); \
> } \
> - } while(0)
> + } while (0)
>
> static const char *key_pass;
> static BIO *wb;
> @@ -107,7 +109,7 @@ int main(int argc, char **argv)
> if (verbose_env && strchr(verbose_env, '1'))
> verbose = true;
>
> - key_pass = getenv("KBUILD_SIGN_PIN");
> + key_pass = getenv("KBUILD_SIGN_PIN");
>
> if (argc != 3)
> format();
> @@ -118,6 +120,7 @@ int main(int argc, char **argv)
> if (!cert_src[0]) {
> /* Invoked with no input; create empty file */
> FILE *f = fopen(cert_dst, "wb");
> +
> ERR(!f, "%s", cert_dst);
> fclose(f);
> exit(0);
> @@ -155,6 +158,7 @@ int main(int argc, char **argv)
> x509 = PEM_read_bio_X509(b, NULL, NULL, NULL);
> if (wb && !x509) {
> unsigned long err = ERR_peek_last_error();
> +
> if (ERR_GET_LIB(err) == ERR_LIB_PEM &&
> ERR_GET_REASON(err) == PEM_R_NO_START_LINE) {
> ERR_clear_error();
>
> base-commit: 7877cb91f1081754a1487c144d85dc0d2e2e7fc4
> --
> 2.39.2 (Apple Git-143)
IMHO should be split to separate commits with fixes tags for
trackability sake.
My guess is that fixes tag is missing because this commit is
bundling a pile of stuff.
BR, Jarkko
On Tue Jun 6, 2023 at 3:44 PM EEST, Jarkko Sakkinen wrote:
> On Tue Jun 6, 2023 at 3:38 PM EEST, Jarkko Sakkinen wrote:
> > On Fri Jun 2, 2023 at 11:59 AM EEST, Franziska Naepelt wrote:
> > > The following issues are fixed:
> > > - WARNING: Missing or malformed SPDX-License-Identifier tag
> > > - ERROR: trailing statements should be on next line
> > > - WARNING: braces {} are not necessary for single statement blocks
> > > - ERROR: space required before the open parenthesis '('
> > > - ERROR: code indent should use tabs where possible
> > > - WARNING: please, no spaces at the start of a line
> > > - WARNING: Missing a blank line after declarations
> > >
> > > Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > > Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > >
> >
> > Remove the empty line.
> >
> > > Reported-by: kernel test robot <[email protected]>
> > > Signed-off-by: Franziska Naepelt <[email protected]>
> >
> > Fixes tag?
> >
> > > ---
> > > v2:
> > > - revert noreturn changes to fix build issues
> > > ---
> > > certs/extract-cert.c | 10 +++++++---
> > > 1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/certs/extract-cert.c b/certs/extract-cert.c
> > > index 70e9ec89d87d..96c0728bf4d1 100644
> > > --- a/certs/extract-cert.c
> > > +++ b/certs/extract-cert.c
> > > @@ -1,3 +1,4 @@
> > > +// SPDX-License-Identifier: LGPL-2.1
> > > /* Extract X.509 certificate in DER form from PKCS#11 or PEM.
> > > *
> > > * Copyright © 2014-2015 Red Hat, Inc. All Rights Reserved.
> > > @@ -63,7 +64,8 @@ static void drain_openssl_errors(void)
> > >
> > > if (ERR_peek_error() == 0)
> > > return;
> > > - while (ERR_get_error_line(&file, &line)) {}
> > > + while (ERR_get_error_line(&file, &line))
> > > + ;
> > > }
> > >
> > > #define ERR(cond, fmt, ...) \
> > > @@ -73,7 +75,7 @@ static void drain_openssl_errors(void)
> > > if (__cond) { \
> > > err(1, fmt, ## __VA_ARGS__); \
> > > } \
> > > - } while(0)
> > > + } while (0)
> > >
> > > static const char *key_pass;
> > > static BIO *wb;
> > > @@ -107,7 +109,7 @@ int main(int argc, char **argv)
> > > if (verbose_env && strchr(verbose_env, '1'))
> > > verbose = true;
> > >
> > > - key_pass = getenv("KBUILD_SIGN_PIN");
> > > + key_pass = getenv("KBUILD_SIGN_PIN");
> > >
> > > if (argc != 3)
> > > format();
> > > @@ -118,6 +120,7 @@ int main(int argc, char **argv)
> > > if (!cert_src[0]) {
> > > /* Invoked with no input; create empty file */
> > > FILE *f = fopen(cert_dst, "wb");
> > > +
> > > ERR(!f, "%s", cert_dst);
> > > fclose(f);
> > > exit(0);
> > > @@ -155,6 +158,7 @@ int main(int argc, char **argv)
> > > x509 = PEM_read_bio_X509(b, NULL, NULL, NULL);
> > > if (wb && !x509) {
> > > unsigned long err = ERR_peek_last_error();
> > > +
> > > if (ERR_GET_LIB(err) == ERR_LIB_PEM &&
> > > ERR_GET_REASON(err) == PEM_R_NO_START_LINE) {
> > > ERR_clear_error();
> > >
> > > base-commit: 7877cb91f1081754a1487c144d85dc0d2e2e7fc4
> > > --
> > > 2.39.2 (Apple Git-143)
> >
> > IMHO should be split to separate commits with fixes tags for
> > trackability sake.
> >
> > My guess is that fixes tag is missing because this commit is
> > bundling a pile of stuff.
>
> Why? I mean I do get it might sound cutting hairs, so here's a
> big longer explanation.
>
> When you look up for a victim commit for a bug that actually screws up
> run-time behaviour in a way or another, exactly these "random selection
> of fixes" really can make you use an inappropriate vocabulary, and you
> *really* have to meditate not to spill that garbage online :-)
>
> Exactly because of this carefully localized fixes are very important.
> If you don't do it, your fix is counter-productive for the codebase
> IMHO.
... not to mention downstream distribution kernels. We maintain upstream
and do not obviously proactively support downstream *but* at the same
time we probably do not want to make backporting bug fixes more
difficult than it is necessary.
BR, Jarkko
On Tue Jun 6, 2023 at 3:38 PM EEST, Jarkko Sakkinen wrote:
> On Fri Jun 2, 2023 at 11:59 AM EEST, Franziska Naepelt wrote:
> > The following issues are fixed:
> > - WARNING: Missing or malformed SPDX-License-Identifier tag
> > - ERROR: trailing statements should be on next line
> > - WARNING: braces {} are not necessary for single statement blocks
> > - ERROR: space required before the open parenthesis '('
> > - ERROR: code indent should use tabs where possible
> > - WARNING: please, no spaces at the start of a line
> > - WARNING: Missing a blank line after declarations
> >
> > Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> > Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
> >
>
> Remove the empty line.
>
> > Reported-by: kernel test robot <[email protected]>
> > Signed-off-by: Franziska Naepelt <[email protected]>
>
> Fixes tag?
>
> > ---
> > v2:
> > - revert noreturn changes to fix build issues
> > ---
> > certs/extract-cert.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/certs/extract-cert.c b/certs/extract-cert.c
> > index 70e9ec89d87d..96c0728bf4d1 100644
> > --- a/certs/extract-cert.c
> > +++ b/certs/extract-cert.c
> > @@ -1,3 +1,4 @@
> > +// SPDX-License-Identifier: LGPL-2.1
> > /* Extract X.509 certificate in DER form from PKCS#11 or PEM.
> > *
> > * Copyright © 2014-2015 Red Hat, Inc. All Rights Reserved.
> > @@ -63,7 +64,8 @@ static void drain_openssl_errors(void)
> >
> > if (ERR_peek_error() == 0)
> > return;
> > - while (ERR_get_error_line(&file, &line)) {}
> > + while (ERR_get_error_line(&file, &line))
> > + ;
> > }
> >
> > #define ERR(cond, fmt, ...) \
> > @@ -73,7 +75,7 @@ static void drain_openssl_errors(void)
> > if (__cond) { \
> > err(1, fmt, ## __VA_ARGS__); \
> > } \
> > - } while(0)
> > + } while (0)
> >
> > static const char *key_pass;
> > static BIO *wb;
> > @@ -107,7 +109,7 @@ int main(int argc, char **argv)
> > if (verbose_env && strchr(verbose_env, '1'))
> > verbose = true;
> >
> > - key_pass = getenv("KBUILD_SIGN_PIN");
> > + key_pass = getenv("KBUILD_SIGN_PIN");
> >
> > if (argc != 3)
> > format();
> > @@ -118,6 +120,7 @@ int main(int argc, char **argv)
> > if (!cert_src[0]) {
> > /* Invoked with no input; create empty file */
> > FILE *f = fopen(cert_dst, "wb");
> > +
> > ERR(!f, "%s", cert_dst);
> > fclose(f);
> > exit(0);
> > @@ -155,6 +158,7 @@ int main(int argc, char **argv)
> > x509 = PEM_read_bio_X509(b, NULL, NULL, NULL);
> > if (wb && !x509) {
> > unsigned long err = ERR_peek_last_error();
> > +
> > if (ERR_GET_LIB(err) == ERR_LIB_PEM &&
> > ERR_GET_REASON(err) == PEM_R_NO_START_LINE) {
> > ERR_clear_error();
> >
> > base-commit: 7877cb91f1081754a1487c144d85dc0d2e2e7fc4
> > --
> > 2.39.2 (Apple Git-143)
>
> IMHO should be split to separate commits with fixes tags for
> trackability sake.
>
> My guess is that fixes tag is missing because this commit is
> bundling a pile of stuff.
Why? I mean I do get it might sound cutting hairs, so here's a
big longer explanation.
When you look up for a victim commit for a bug that actually screws up
run-time behaviour in a way or another, exactly these "random selection
of fixes" really can make you use an inappropriate vocabulary, and you
*really* have to meditate not to spill that garbage online :-)
Exactly because of this carefully localized fixes are very important.
If you don't do it, your fix is counter-productive for the codebase
IMHO.
BR, Jarkko
On Tue, Jun 06, 2023 at 07:28:52PM +0700, Bagas Sanjaya wrote:
> On Fri, Jun 02, 2023 at 10:59:02AM +0200, Franziska Naepelt wrote:
> > The following issues are fixed:
> > - WARNING: Missing or malformed SPDX-License-Identifier tag
> > - ERROR: trailing statements should be on next line
> > - WARNING: braces {} are not necessary for single statement blocks
> > - ERROR: space required before the open parenthesis '('
> > - ERROR: code indent should use tabs where possible
> > - WARNING: please, no spaces at the start of a line
> > - WARNING: Missing a blank line after declarations
>
> Again, write the patch description in imperative mood (e.g. "Do foo").
>
Why do you care about imperative tense? Imperative tense doesn't
matter. What matters is that you can understand the issue and how it
looks like to the user. I was working with a group of foreign students
and it was painful to see the contortions that they went through to make
a commit message imperative. It's like saying "Bake a cake", "Ok, now
bake it while juggling." The cake ends up worse. And the commit
message end up worse when we force nonsense rules like this.
That said the rest of your comments are correct.
Franziska, I kind of feel like you should start in drivers/staging/
because you're sending beginner patches to kind of core parts of the
kernel where people are busy. Most likely your going to have to send a
bunch of iterations of the patch. In drivers/staging/ we don't mind
reviewing newbie patches.
regards,
dan carpenter
Jarkko Sakkinen <[email protected]> wrote:
> Fixes tag?
That's not really necessary in this case. It's not fixing any bugs so much as
keeping checkpatch happy, so there's not much point backporting the patches.
> IMHO should be split to separate commits with fixes tags for
> trackability sake.
I think a single patch is fine for what it's doing. Let's not add a bunch of
individual one-liner keeping-checkpatch-happy patches.
David
On Tue, Jun 06, 2023 at 05:51:09PM +0300, Jarkko Sakkinen wrote:
> On Tue Jun 6, 2023 at 4:38 PM EEST, Dan Carpenter wrote:
> > On Tue, Jun 06, 2023 at 07:28:52PM +0700, Bagas Sanjaya wrote:
> > > On Fri, Jun 02, 2023 at 10:59:02AM +0200, Franziska Naepelt wrote:
> > > > The following issues are fixed:
> > > > - WARNING: Missing or malformed SPDX-License-Identifier tag
> > > > - ERROR: trailing statements should be on next line
> > > > - WARNING: braces {} are not necessary for single statement blocks
> > > > - ERROR: space required before the open parenthesis '('
> > > > - ERROR: code indent should use tabs where possible
> > > > - WARNING: please, no spaces at the start of a line
> > > > - WARNING: Missing a blank line after declarations
> > >
> > > Again, write the patch description in imperative mood (e.g. "Do foo").
> > >
> >
> > Why do you care about imperative tense? Imperative tense doesn't
> > matter. What matters is that you can understand the issue and how it
> > looks like to the user. I was working with a group of foreign students
> > and it was painful to see the contortions that they went through to make
> > a commit message imperative. It's like saying "Bake a cake", "Ok, now
> > bake it while juggling." The cake ends up worse. And the commit
> > message end up worse when we force nonsense rules like this.
>
> How about a simple and stupid reason?
>
> Usually I write commit message without caring about this. Then I rewrite
> the commit message and 9/10 it gets shorter. Based on empirical
> experience, imperative form has minimum amount of extra words.
>
I'm looking through the git log to see if it's true the imperative tense
commit message are shorter and better and neither one of those things is
obvious to me.
This patch had an imperative subject already so it was already kind of
imperative. Does every sentence have to be imperative or can you just
add a "Fix it." to the end?
I don't want to belittle the challenges you face around the English
language but I think students were less fluent than you are. So maybe
imperative tense works for you but it definitely made their commit
messages far worse.
regards,
dan carpenter
On Tue Jun 6, 2023 at 4:38 PM EEST, Dan Carpenter wrote:
> On Tue, Jun 06, 2023 at 07:28:52PM +0700, Bagas Sanjaya wrote:
> > On Fri, Jun 02, 2023 at 10:59:02AM +0200, Franziska Naepelt wrote:
> > > The following issues are fixed:
> > > - WARNING: Missing or malformed SPDX-License-Identifier tag
> > > - ERROR: trailing statements should be on next line
> > > - WARNING: braces {} are not necessary for single statement blocks
> > > - ERROR: space required before the open parenthesis '('
> > > - ERROR: code indent should use tabs where possible
> > > - WARNING: please, no spaces at the start of a line
> > > - WARNING: Missing a blank line after declarations
> >
> > Again, write the patch description in imperative mood (e.g. "Do foo").
> >
>
> Why do you care about imperative tense? Imperative tense doesn't
> matter. What matters is that you can understand the issue and how it
> looks like to the user. I was working with a group of foreign students
> and it was painful to see the contortions that they went through to make
> a commit message imperative. It's like saying "Bake a cake", "Ok, now
> bake it while juggling." The cake ends up worse. And the commit
> message end up worse when we force nonsense rules like this.
How about a simple and stupid reason?
Usually I write commit message without caring about this. Then I rewrite
the commit message and 9/10 it gets shorter. Based on empirical
experience, imperative form has minimum amount of extra words.
I came up with this convention first when submitting patches to x86, and
over time it has started to make sense to me.
Especially for multi patch sets too verbose language tends to be super
tiring for non-native English speaker. One should optimize the language
in those: say *exactly* what is needed, and not more. If this not the
case, I tend to move these patch sets very last in my queue.
It's not a "punishment". It's more like that I really have to take the
time to read the prose...
BR, Jarkko
On Tue Jun 6, 2023 at 6:25 PM EEST, Dan Carpenter wrote:
> On Tue, Jun 06, 2023 at 05:51:09PM +0300, Jarkko Sakkinen wrote:
> > On Tue Jun 6, 2023 at 4:38 PM EEST, Dan Carpenter wrote:
> > > On Tue, Jun 06, 2023 at 07:28:52PM +0700, Bagas Sanjaya wrote:
> > > > On Fri, Jun 02, 2023 at 10:59:02AM +0200, Franziska Naepelt wrote:
> > > > > The following issues are fixed:
> > > > > - WARNING: Missing or malformed SPDX-License-Identifier tag
> > > > > - ERROR: trailing statements should be on next line
> > > > > - WARNING: braces {} are not necessary for single statement blocks
> > > > > - ERROR: space required before the open parenthesis '('
> > > > > - ERROR: code indent should use tabs where possible
> > > > > - WARNING: please, no spaces at the start of a line
> > > > > - WARNING: Missing a blank line after declarations
> > > >
> > > > Again, write the patch description in imperative mood (e.g. "Do foo").
> > > >
> > >
> > > Why do you care about imperative tense? Imperative tense doesn't
> > > matter. What matters is that you can understand the issue and how it
> > > looks like to the user. I was working with a group of foreign students
> > > and it was painful to see the contortions that they went through to make
> > > a commit message imperative. It's like saying "Bake a cake", "Ok, now
> > > bake it while juggling." The cake ends up worse. And the commit
> > > message end up worse when we force nonsense rules like this.
> >
> > How about a simple and stupid reason?
> >
> > Usually I write commit message without caring about this. Then I rewrite
> > the commit message and 9/10 it gets shorter. Based on empirical
> > experience, imperative form has minimum amount of extra words.
> >
>
> I'm looking through the git log to see if it's true the imperative tense
> commit message are shorter and better and neither one of those things is
> obvious to me.
>
> This patch had an imperative subject already so it was already kind of
> imperative. Does every sentence have to be imperative or can you just
> add a "Fix it." to the end?
>
> I don't want to belittle the challenges you face around the English
> language but I think students were less fluent than you are. So maybe
> imperative tense works for you but it definitely made their commit
> messages far worse.
Yeah, I was not trying to oppose, just reasoning why I like it more.
For a single patch, this does not really matter anyway :-)
BR, Jarkko
Am Di., 6. Juni 2023 um 18:03 Uhr schrieb Jarkko Sakkinen <[email protected]>:
> On Tue Jun 6, 2023 at 6:25 PM EEST, Dan Carpenter wrote:
> > On Tue, Jun 06, 2023 at 05:51:09PM +0300, Jarkko Sakkinen wrote:
> > > On Tue Jun 6, 2023 at 4:38 PM EEST, Dan Carpenter wrote:
> > > > On Tue, Jun 06, 2023 at 07:28:52PM +0700, Bagas Sanjaya wrote:
> > > > > On Fri, Jun 02, 2023 at 10:59:02AM +0200, Franziska Naepelt wrote:
> > > > > > The following issues are fixed:
> > > > > > - WARNING: Missing or malformed SPDX-License-Identifier tag
> > > > > > - ERROR: trailing statements should be on next line
> > > > > > - WARNING: braces {} are not necessary for single statement blocks
> > > > > > - ERROR: space required before the open parenthesis '('
> > > > > > - ERROR: code indent should use tabs where possible
> > > > > > - WARNING: please, no spaces at the start of a line
> > > > > > - WARNING: Missing a blank line after declarations
> > > > >
> > > > > Again, write the patch description in imperative mood (e.g. "Do foo").
> > > > >
> > > >
> > > > Why do you care about imperative tense? Imperative tense doesn't
> > > > matter. What matters is that you can understand the issue and how it
> > > > looks like to the user. I was working with a group of foreign students
> > > > and it was painful to see the contortions that they went through to make
> > > > a commit message imperative. It's like saying "Bake a cake", "Ok, now
> > > > bake it while juggling." The cake ends up worse. And the commit
> > > > message end up worse when we force nonsense rules like this.
> > >
> > > How about a simple and stupid reason?
> > >
> > > Usually I write commit message without caring about this. Then I rewrite
> > > the commit message and 9/10 it gets shorter. Based on empirical
> > > experience, imperative form has minimum amount of extra words.
> > >
> >
> > I'm looking through the git log to see if it's true the imperative tense
> > commit message are shorter and better and neither one of those things is
> > obvious to me.
> >
> > This patch had an imperative subject already so it was already kind of
> > imperative. Does every sentence have to be imperative or can you just
> > add a "Fix it." to the end?
> >
> > I don't want to belittle the challenges you face around the English
> > language but I think students were less fluent than you are. So maybe
> > imperative tense works for you but it definitely made their commit
> > messages far worse.
>
> Yeah, I was not trying to oppose, just reasoning why I like it more.
>
> For a single patch, this does not really matter anyway :-)
>
> BR, Jarkko
I'm a bit puzzled now since there are different opinions on my patch.
I'm struggling to draw a conclusion whether to split the patch into smaller
single line patches or not.
I'd propose to split it into two patches:
* One for SPDX license tag fix
* One for spacing, tab, blank line, unnecessary braces etc.
And fix the remarks related to SPDX license tag and the use of imperative.
If you agree I'm happy to provide two new patches.
Anyway, as per Dan's proposal I'll continue to work in drivers/staging.
Thanks,
Franziska
On Tue, Jun 06, 2023 at 07:59:02PM +0200, Franziska N?pelt wrote:
> Am Di., 6. Juni 2023 um 18:03 Uhr schrieb Jarkko Sakkinen <[email protected]>:
> > On Tue Jun 6, 2023 at 6:25 PM EEST, Dan Carpenter wrote:
> > > On Tue, Jun 06, 2023 at 05:51:09PM +0300, Jarkko Sakkinen wrote:
> > > > On Tue Jun 6, 2023 at 4:38 PM EEST, Dan Carpenter wrote:
> > > > > On Tue, Jun 06, 2023 at 07:28:52PM +0700, Bagas Sanjaya wrote:
> > > > > > On Fri, Jun 02, 2023 at 10:59:02AM +0200, Franziska Naepelt wrote:
> > > > > > > The following issues are fixed:
> > > > > > > - WARNING: Missing or malformed SPDX-License-Identifier tag
> > > > > > > - ERROR: trailing statements should be on next line
> > > > > > > - WARNING: braces {} are not necessary for single statement blocks
> > > > > > > - ERROR: space required before the open parenthesis '('
> > > > > > > - ERROR: code indent should use tabs where possible
> > > > > > > - WARNING: please, no spaces at the start of a line
> > > > > > > - WARNING: Missing a blank line after declarations
> > > > > >
[ snip ]
>
> I'm a bit puzzled now since there are different opinions on my patch.
> I'm struggling to draw a conclusion whether to split the patch into smaller
> single line patches or not.
>
> I'd propose to split it into two patches:
> * One for SPDX license tag fix
> * One for spacing, tab, blank line, unnecessary braces etc.
You should definitely pull the SPDX change into its own patch because
it's sightly controversial and important.
In drivers/staging/ we would say pull each type of checkpatch warning
into its own patch so it would be something like 6 patches. But I don't
know how it's done in this subsystem. I feel like Greg maybe goes
overboard on splitting patches up, but the advantage of Greg's system is
that it's easy to explain the rules to newbies. There is a lot about
staging/ which designed around newbies.
If I'm totally honest, in a lot of subsystems the policy is just leave
it alone. Don't bother cleaning up checkpatch stuff because it just
creates more work and makes the git log noisier.
regards,
dan carpenter
On Tue, Jun 06, 2023 at 18:25:24 +0300, Dan Carpenter wrote:
> I'm looking through the git log to see if it's true the imperative tense
> commit message are shorter and better and neither one of those things is
> obvious to me.
>
> This patch had an imperative subject already so it was already kind of
> imperative. Does every sentence have to be imperative or can you just
> add a "Fix it." to the end?
I don't know about the length argument, but it feels like it reads
better when skimming summaries with the imperative mood. The way I think
about it is that the subject should complete the phrase:
When applied, this patch will…
The body then gives more context and description as necessary. I don't
really worry so much about the mood/tense/whatever in the body except
that I try to use the present tense for anything the patch is doing and
past for any historical context. I understand that kernel maintainers
may care a lot more about it though.
Basically, a patch, on its own, does nothing (just like a recipe). It is
only when it is applied that anything actually happens. I read it as
"`git apply`, please $summary".
--Ben
On Wed Jun 7, 2023 at 12:43 AM EEST, Ben Boeckel wrote:
> On Tue, Jun 06, 2023 at 18:25:24 +0300, Dan Carpenter wrote:
> > I'm looking through the git log to see if it's true the imperative tense
> > commit message are shorter and better and neither one of those things is
> > obvious to me.
> >
> > This patch had an imperative subject already so it was already kind of
> > imperative. Does every sentence have to be imperative or can you just
> > add a "Fix it." to the end?
>
> I don't know about the length argument, but it feels like it reads
> better when skimming summaries with the imperative mood. The way I think
> about it is that the subject should complete the phrase:
>
> When applied, this patch will…
>
> The body then gives more context and description as necessary. I don't
> really worry so much about the mood/tense/whatever in the body except
> that I try to use the present tense for anything the patch is doing and
> past for any historical context. I understand that kernel maintainers
> may care a lot more about it though.
>
> Basically, a patch, on its own, does nothing (just like a recipe). It is
> only when it is applied that anything actually happens. I read it as
> "`git apply`, please $summary".
>
> --Ben
+1
BR, Jarkko
On Tue, Jun 06, 2023 at 05:51:09PM +0300, Jarkko Sakkinen wrote:
> It's not a "punishment". It's more like that I really have to take the
> time to read the prose...
The thing about imperative tense is that it was used as a punishment on
me once five years ago. I wrote a quite bad commit message and a senior
maintainer told me to re-write it properly and I realized that it was
true. My commit message was bad. So I wrote a proper commit message.
And then he yelled at me, "Can't you follow simple directions and write
it in imperative tense like the documentation says? Are you a
shithead?"
So then I swore I would never talk to him again or to anyone who
enforced the imperative tense rule. That has only happened once in the
intervening years. I told the maintainer, "Fine. Re-write the commit
message however you like and give me Reported-by credit." This was a
cheeky response and it made the maintainer enraged. I guess he thought
that my boss would force me to fix the bug or something? I felt bad for
the Intel developer who had to fix my bug instead because I knew that
the maintainer was going to be super angry if he gave me reported-by
credit so I had put him in a bind. I almost re-wrote the commit message
so that he wouldn't have to deal with that. Maybe this is how mothers
feel when they try to take abuse from an angry husband instead of
letting their kids suffer. But I am a bad mother and I left.
My boss would never have forced me to deal with that. When he left for
a different company he said, "Dan, I'm transitioning and XXX is taking
over me and I have told him all your weirdness so he is prepared." And
it was a huge comfort to me because I know what my weakness are.
You people on this thread all seem super nice. And you're right that we
should always try to be improve every aspect of our craft.
When Jarkko talked about people who write too long commit messages, I
thought about one developer in particular who writes too long commit
messages. He writes in imperative tense. He takes everything so
seriously and he's never seen a rule without following it. His patches
are always right. People have told him that his commit messages are bad
and too long and those people are right. But they need to shut up. The
good things that he does and the bad things that he does are all part of
the same package. He can't change and I don't want him to feel anything
but welcome.
It's hard to be a good kernel developer without being at least slightly
obsessive. Both developers and maintainers are that way. And I deal
with a lot of people and accomodating maintainers you disagree with is
part of the job.
So long as everyone is kind to each other. That's the main thing.
regards,
dan carpenter
On Fri Jun 9, 2023 at 5:01 PM EEST, Dan Carpenter wrote:
> On Tue, Jun 06, 2023 at 05:51:09PM +0300, Jarkko Sakkinen wrote:
> > It's not a "punishment". It's more like that I really have to take the
> > time to read the prose...
>
> The thing about imperative tense is that it was used as a punishment on
> me once five years ago. I wrote a quite bad commit message and a senior
> maintainer told me to re-write it properly and I realized that it was
> true. My commit message was bad. So I wrote a proper commit message.
> And then he yelled at me, "Can't you follow simple directions and write
> it in imperative tense like the documentation says? Are you a
> shithead?"
Wow :-O I'm totally against name calling or any sort of shittiness like
that, and all for co-operation. Just told my personal thoughts on the
matter. I'm sorry that this happened to you.
>
> So then I swore I would never talk to him again or to anyone who
> enforced the imperative tense rule. That has only happened once in the
> intervening years. I told the maintainer, "Fine. Re-write the commit
> message however you like and give me Reported-by credit." This was a
> cheeky response and it made the maintainer enraged. I guess he thought
> that my boss would force me to fix the bug or something? I felt bad for
> the Intel developer who had to fix my bug instead because I knew that
> the maintainer was going to be super angry if he gave me reported-by
> credit so I had put him in a bind. I almost re-wrote the commit message
> so that he wouldn't have to deal with that. Maybe this is how mothers
> feel when they try to take abuse from an angry husband instead of
> letting their kids suffer. But I am a bad mother and I left.
>
> My boss would never have forced me to deal with that. When he left for
> a different company he said, "Dan, I'm transitioning and XXX is taking
> over me and I have told him all your weirdness so he is prepared." And
> it was a huge comfort to me because I know what my weakness are.
>
> You people on this thread all seem super nice. And you're right that we
> should always try to be improve every aspect of our craft.
>
> When Jarkko talked about people who write too long commit messages, I
> thought about one developer in particular who writes too long commit
> messages. He writes in imperative tense. He takes everything so
> seriously and he's never seen a rule without following it. His patches
> are always right. People have told him that his commit messages are bad
> and too long and those people are right. But they need to shut up. The
> good things that he does and the bad things that he does are all part of
> the same package. He can't change and I don't want him to feel anything
> but welcome.
>
> It's hard to be a good kernel developer without being at least slightly
> obsessive. Both developers and maintainers are that way. And I deal
> with a lot of people and accomodating maintainers you disagree with is
> part of the job.
>
> So long as everyone is kind to each other. That's the main thing.
I 110% agree with this. I even bookmarked this response :-)
> regards,
> dan carpenter
BR, Jarkko
On Fri Jun 9, 2023 at 6:37 PM EEST, Jarkko Sakkinen wrote:
> On Fri Jun 9, 2023 at 5:01 PM EEST, Dan Carpenter wrote:
> > On Tue, Jun 06, 2023 at 05:51:09PM +0300, Jarkko Sakkinen wrote:
> > > It's not a "punishment". It's more like that I really have to take the
> > > time to read the prose...
> >
> > The thing about imperative tense is that it was used as a punishment on
> > me once five years ago. I wrote a quite bad commit message and a senior
> > maintainer told me to re-write it properly and I realized that it was
> > true. My commit message was bad. So I wrote a proper commit message.
> > And then he yelled at me, "Can't you follow simple directions and write
> > it in imperative tense like the documentation says? Are you a
> > shithead?"
>
> Wow :-O I'm totally against name calling or any sort of shittiness like
> that, and all for co-operation. Just told my personal thoughts on the
> matter. I'm sorry that this happened to you.
>
> >
> > So then I swore I would never talk to him again or to anyone who
> > enforced the imperative tense rule. That has only happened once in the
> > intervening years. I told the maintainer, "Fine. Re-write the commit
> > message however you like and give me Reported-by credit." This was a
> > cheeky response and it made the maintainer enraged. I guess he thought
> > that my boss would force me to fix the bug or something? I felt bad for
> > the Intel developer who had to fix my bug instead because I knew that
> > the maintainer was going to be super angry if he gave me reported-by
> > credit so I had put him in a bind. I almost re-wrote the commit message
> > so that he wouldn't have to deal with that. Maybe this is how mothers
> > feel when they try to take abuse from an angry husband instead of
> > letting their kids suffer. But I am a bad mother and I left.
> >
> > My boss would never have forced me to deal with that. When he left for
> > a different company he said, "Dan, I'm transitioning and XXX is taking
> > over me and I have told him all your weirdness so he is prepared." And
> > it was a huge comfort to me because I know what my weakness are.
> >
> > You people on this thread all seem super nice. And you're right that we
> > should always try to be improve every aspect of our craft.
> >
> > When Jarkko talked about people who write too long commit messages, I
> > thought about one developer in particular who writes too long commit
> > messages. He writes in imperative tense. He takes everything so
> > seriously and he's never seen a rule without following it. His patches
> > are always right. People have told him that his commit messages are bad
> > and too long and those people are right. But they need to shut up. The
> > good things that he does and the bad things that he does are all part of
> > the same package. He can't change and I don't want him to feel anything
> > but welcome.
> >
> > It's hard to be a good kernel developer without being at least slightly
> > obsessive. Both developers and maintainers are that way. And I deal
> > with a lot of people and accomodating maintainers you disagree with is
> > part of the job.
> >
> > So long as everyone is kind to each other. That's the main thing.
>
> I 110% agree with this. I even bookmarked this response :-)
To add: it is super hard and non-trivial issue to keep the balance
between "nice" and "obsessive". If you are too nice, the stuff does not
get fixed. If you are too obssesive, well... then you act like a jerk.
So you have to try to say the truth as clearly as possible, but still
keep the tone constructive. Not always easy to manage.
Personally, I keep bullshit threshold from other people. There are
better and worse days and that does affect your communication. So if
you get one nastier email from someone 9/10 it is better just ignore
the bullshit and focus on matter. Of course when it is a trend, then
it is better to vocally ask for better communication.
BR, Jarkko