Re: [Qemu-devel] [PATCH 1/4] Clean up wrong usage of FALSE and TRUE in places that use bool from stdbool.h

[Eric Blake]

On 3/22/19 7:05 AM, JafarAbdi wrote:
> Signed-off-by: JafarAbdi <address@hidden>
> ---

Thank you for your submission - it looks like your first contribution to
qemu.  However, I have some suggestions that will make it easier to get
your patches applied:

- Use a cover letter. Since you sent a series, including a 0/4 cover
letter makes it easier to reply to the series as a whole, and easier for
our continuous integration tools to run tests on your patches (for a
single patch, a cover letter is optional).

- Use distinct subject lines. Reusing the same subject line for four
different patches doesn't help anyone distinguish which patch to
backport in isolation (if only one of the four needs to be applied to a
downstream port).

- Use shorter subject lines. Try to stick to around 60 characters, and
using a 'topic: Description' layout (you can view git log of the file
you are touching to see topics that have been chosen in the past for
that file; for example, THIS patch would be better titled
  authz: fix usage of bool
then use the body to explain how TRUE/FALSE are for glib's (lame)
'gboolean' type only, and everywhere else we prefer <stdbool's> spelling.

- The subject line should say "what" (which you vaguely did, other than
missing the topic part), but the commit body should say "why" (what
problems are you fixing, why should a maintainer include your patch,
etc). You had no commit body other than your Sign-off.

- Your Sign off should be a legal name. I'm not one to argue whether
'JafarAbdi' is (one of) your legal name(s), but without a space it looks
suspicious as if it might be your login name instead, so I'm wondering
if you need to tweak your git settings so that you show up as 'Jafar
Abdi' or whatever other spelling you prefer to go by on legal documents.

>  authz/listfile.c |     2 +-
>  qemu.config      |     2 +
>  qemu.creator     |     1 +
>  qemu.files       | 28238 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu.includes    |  2568 +++++
>  5 files changed, 30810 insertions(+), 1 deletion(-)
>  create mode 100644 qemu.config
>  create mode 100644 qemu.creator
>  create mode 100644 qemu.files
>  create mode 100644 qemu.includes

- Your diffstat is highly unusual. (Thankfully, your email did not
actually include 28238 bogus lines of changes to qemu.files, or it would
have been a lot larger than 6k). Use 'git rebase -i' to polish your
patches before sending, so that you don't have to touch up your emails
to drop cruft that shouldn't be in the commit after all.

- You failed to cc: the maintainer for the file that your patch touches
(for authz, scripts/get_maintainer.pl reports that Dan Berrange should
be listed in cc; which I've done). Git can be configured to auto-cc
maintainers without you having to think about it.

- More patch submission tips at:
https://wiki.qemu.org/Contribute/SubmitAPatch

Good luck!

> 
> diff --git a/authz/listfile.c b/authz/listfile.c
> index d457976..03eaf46 100644
> --- a/authz/listfile.c
> +++ b/authz/listfile.c
> @@ -238,7 +238,7 @@ qauthz_list_file_init(Object *obj)
>  
>      authz->file_watch = -1;
>  #ifdef CONFIG_INOTIFY1
> -    authz->refresh = TRUE;
> +    authz->refresh = true;
>  #endif
>  }
>  
> 

At any rate, this code change is correct, so if you submit a v2 with an
improved commit message, you can add:

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature