How to submit patches and patchsets via grub-devel

How to submit patches and patchsets via grub-devel

[Hans Ulrich Niedermann]

Hello,

as I am continuing to flood this mailing list with patches, I am
realizing that I am missing some general rules for how things work on
grub-devel. Sorry for the inconvenience caused by that.

Anyway, here are a few questions I am beginning realize I should know
the answers to before posting patches to grub-devel:

  * What to put into the cover letter (git send-email --compose)?

  * How to format commit messages?

  * What about those special lines within commit messages?

      * Signed-off-by:
      * Reviewed-by:

    Who adds these special lines and when? If Daniel replies to me
    posting a patch with "Reviewed-by: Daniel", does that mean I should
    include that in my next iteration of that patch? Does it mean
    Daniel approves of the patch and plans to merge it to master
    himself with or without adding the Reviewed-by by himself?

    Are there other special lines I need to be aware of?

  * How are reviews done? Is there a formal definition of preconditions
    which must be satisfied before something is actually committed to
    the master branch?

I presume a good place to write those down for grub would be
docs/grub-dev.texi. I had not intended to touch grub-dev.texi much
before 2.06, but apparently this needs to be done first so I can
properly do the other things for 2.06. Am I wrong in this presumption
and just missing the place where the general rules on how to contribute
patches are written down?

Anyway, when I am finding out what those rules are I can start
writing them down without much additional effort, starting with what
Daniel replied to me in
<20200423100837.jllsb57rjitmd2xl@tomti.i.net-space.pl>.

Please add to this list if you notice something missing from it, so I
can collect all that, write it up for docs/grub-dev.texi and post a
patch updating docs/grub-dev.texi in a week or two.

So, here are the "how to submit patches via grub-devel" rules so far:

  * If you post more than one patch, create a cover email
    (git send-email --compose ...).

  * Create a new email thread for a new patchset. Do not attach it
    to existing email threads.

  * If you need to repost a patchset, repost the whole series of
    patches as a new email thread instead of just reposting some
    individual patches.

Thanks for your help.

Uli

Re: How to submit patches and patchsets via grub-devel

[Eli Schwartz]

[-- Attachment #1.1: Type: text/plain, Size: 4478 bytes --]

On 4/23/20 10:20 AM, Hans Ulrich Niedermann wrote:
> Hello,
> 
> as I am continuing to flood this mailing list with patches, I am
> realizing that I am missing some general rules for how things work on
> grub-devel. Sorry for the inconvenience caused by that.
> 
> Anyway, here are a few questions I am beginning realize I should know
> the answers to before posting patches to grub-devel:
> 
>   * What to put into the cover letter (git send-email --compose)?
> 
>   * How to format commit messages?
> 
>   * What about those special lines within commit messages?
> 
>       * Signed-off-by:
>       * Reviewed-by:
> 
>     Who adds these special lines and when? If Daniel replies to me
>     posting a patch with "Reviewed-by: Daniel", does that mean I should
>     include that in my next iteration of that patch? Does it mean
>     Daniel approves of the patch and plans to merge it to master
>     himself with or without adding the Reviewed-by by himself?
> 
>     Are there other special lines I need to be aware of?

https://git.wiki.kernel.org/index.php/CommitMessageConventions#Trailers

Different projects mean different things, but Signed-off-by is a common
one:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

Reviewed-by is another convention originating in the kernel -- it
indicates Daniel thinks the patch is good, and the person (in this case
himself) who takes that patch and commits it to the project (or to the
kernel subsystem for later merging into torvalds/linux.git) will include
that line when committing -- there's no need to submit a v2/v3/etc patch
with just this modification to the commit message.

>   * How are reviews done? Is there a formal definition of preconditions
>     which must be satisfied before something is actually committed to
>     the master branch?

I'm not entirely sure what you're asking, but I'd guess "one or more
project maintainers will reply to the patch, either approving the patch
or asking for changes". But of course they're unlikely to approve a
non-documentation patch without giving it a trial run and seeing it work.

Is your question about what type of testing is done?

> I presume a good place to write those down for grub would be
> docs/grub-dev.texi. I had not intended to touch grub-dev.texi much
> before 2.06, but apparently this needs to be done first so I can
> properly do the other things for 2.06. Am I wrong in this presumption
> and just missing the place where the general rules on how to contribute
> patches are written down?

I think it's more or less:

- Some of these rules are passed along by word of mouth due to being de
  facto rules, and, while preferred, aren't hard requirements.

- Some of these rules are commonly used in various projects, so people
  automatically assume everyone knows them, even though they don't. Then
  when someone asks "but what are the rules, anyway" there's a mad
  scramble to hunt down a description in someone else's documentation,
  often from kernel.org

- Often, documentation doesn't get written down because it's obvious
  to the people who write documentation, and the people who need the
  docs don't know it. And in the case of procedural rules this is even
  more egregious, because you don't have code to point at to prove that
  it isn't documented. Then eventually someone who was unfamiliar with
  the process says "how about we document this" and everyone else is
  like "oh uh, yeah, that makes sense, we probably should have done this
  a long time ago".

It's always nice to find a documentation file such as
'doc/submitting-patches.asciidoc' (or reStructuredText or texinfo or
whatever the project uses) documenting the conventions, as well as any
project-specific guidance. It will also usually tell you the email
address for the appropriate mailing list. One example can be found here:

https://www.archlinux.org/pacman/submitting-patches.html

source:
https://git.archlinux.org/pacman.git/tree/doc/submitting-patches.asciidoc

Currently the grub-dev manual talks a lot about the coding style
(similar in spirit to my HACKING.asciidoc/HACKING.html) and very little
about how to submit patches. This information is definitely missing and
I believe a patch to update it would be very appropriate.

-- 
Eli Schwartz
Arch Linux Bug Wrangler and Trusted User


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 1601 bytes --]

Re: How to submit patches and patchsets via grub-devel

[Daniel Axtens]

Hi Hans,

> Hello,
>
> as I am continuing to flood this mailing list with patches, I am
> realizing that I am missing some general rules for how things work on
> grub-devel. Sorry for the inconvenience caused by that.
>
> Anyway, here are a few questions I am beginning realize I should know
> the answers to before posting patches to grub-devel:
>
>   * What to put into the cover letter (git send-email --compose)?
>
>   * How to format commit messages?
>
>   * What about those special lines within commit messages?
>
>       * Signed-off-by:
>       * Reviewed-by:
>
>     Who adds these special lines and when? If Daniel replies to me
>     posting a patch with "Reviewed-by: Daniel", does that mean I should
>     include that in my next iteration of that patch? Does it mean
>     Daniel approves of the patch and plans to merge it to master
>     himself with or without adding the Reviewed-by by himself?
>
>     Are there other special lines I need to be aware of?

I don't know about grub specifically but these lines are extremely
common in the linux kernel community, where they're documented at:
(picking v4.17 as it came up first in the google search results and it's
still pretty current)

https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html

Specifically:

https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

I generally follow the kernel patch guidelines across any project I
contribute to where appropriate - obviously in this case there are
things like the GNU coding style rather than the linux coding style, so
it's not all applicable! But I think the kernel guidance on patch format
is extremely good and generally applicable - that might answer your
first two questions.

>
>   * How are reviews done? Is there a formal definition of preconditions
>     which must be satisfied before something is actually committed to
>     the master branch?

Again, just in terms of kernel practice, see
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

Projects (and indeed different subsystems within the kernel) have
different rules about what's formally required (e.g. is a Reviewed-by
required, does the maintainer apply a reviewed-by tag or a signed-off-by
tag) so I won't speak for Daniel K on this.

> I presume a good place to write those down for grub would be
> docs/grub-dev.texi. I had not intended to touch grub-dev.texi much
> before 2.06, but apparently this needs to be done first so I can
> properly do the other things for 2.06. Am I wrong in this presumption
> and just missing the place where the general rules on how to contribute
> patches are written down?
>
> Anyway, when I am finding out what those rules are I can start
> writing them down without much additional effort, starting with what
> Daniel replied to me in
> <20200423100837.jllsb57rjitmd2xl@tomti.i.net-space.pl>.
>
> Please add to this list if you notice something missing from it, so I
> can collect all that, write it up for docs/grub-dev.texi and post a
> patch updating docs/grub-dev.texi in a week or two.
>
> So, here are the "how to submit patches via grub-devel" rules so far:
>
>   * If you post more than one patch, create a cover email
>     (git send-email --compose ...).

I tend to use git format-patch --cover-letter and then look over
everything first - but most of my colleagues do it the way you've
suggested.
>
>   * Create a new email thread for a new patchset. Do not attach it
>     to existing email threads.
>
>   * If you need to repost a patchset, repost the whole series of
>     patches as a new email thread instead of just reposting some
>     individual patches.
>

My personal opinion is that these are all excellent rules for all
involved.

Regards,
Daniel


> Thanks for your help.
>
> Uli
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: How to submit patches and patchsets via grub-devel

[Daniel Kiper]

Daniel and Eli, thank you for nice explanations. I would like to add
just a thew things...

On Thu, Apr 23, 2020 at 11:17:40AM -0400, Eli Schwartz wrote:
> On 4/23/20 10:20 AM, Hans Ulrich Niedermann wrote:
> > Hello,
> >
> > as I am continuing to flood this mailing list with patches, I am
> > realizing that I am missing some general rules for how things work on
> > grub-devel. Sorry for the inconvenience caused by that.
> >
> > Anyway, here are a few questions I am beginning realize I should know
> > the answers to before posting patches to grub-devel:
> >
> >   * What to put into the cover letter (git send-email --compose)?
> >
> >   * How to format commit messages?
> >
> >   * What about those special lines within commit messages?
> >
> >       * Signed-off-by:
> >       * Reviewed-by:
> >
> >     Who adds these special lines and when? If Daniel replies to me
> >     posting a patch with "Reviewed-by: Daniel", does that mean I should
> >     include that in my next iteration of that patch? Does it mean
> >     Daniel approves of the patch and plans to merge it to master
> >     himself with or without adding the Reviewed-by by himself?
> >
> >     Are there other special lines I need to be aware of?
>
> https://git.wiki.kernel.org/index.php/CommitMessageConventions#Trailers
>
> Different projects mean different things, but Signed-off-by is a common
> one:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
>
> Reviewed-by is another convention originating in the kernel -- it
> indicates Daniel thinks the patch is good, and the person (in this case
> himself) who takes that patch and commits it to the project (or to the
> kernel subsystem for later merging into torvalds/linux.git) will include
> that line when committing -- there's no need to submit a v2/v3/etc patch
> with just this modification to the commit message.

Exactly! I would just add that I usually do not commit the patches
immediately. I do it to leave some time for others to take a look and
chime in if they find any issue. I usually commit a bunch of patches
once per week. I try to not delay patches which fix grave bugs.

> >   * How are reviews done? Is there a formal definition of preconditions
> >     which must be satisfied before something is actually committed to
> >     the master branch?
>
> I'm not entirely sure what you're asking, but I'd guess "one or more
> project maintainers will reply to the patch, either approving the patch
> or asking for changes". But of course they're unlikely to approve a

Everybody who feels that a given area is in his/her expertise can
review/test patches. However, final word belongs to the maintainers,
in this case me, Vladimir and Alex.

> non-documentation patch without giving it a trial run and seeing it work.

I reject all patches witch breaks anything in the GRUB. Usually I tell
what is broken...

> Is your question about what type of testing is done?
>
> > I presume a good place to write those down for grub would be
> > docs/grub-dev.texi. I had not intended to touch grub-dev.texi much
> > before 2.06, but apparently this needs to be done first so I can
> > properly do the other things for 2.06. Am I wrong in this presumption
> > and just missing the place where the general rules on how to contribute
> > patches are written down?
>
> I think it's more or less:
>
> - Some of these rules are passed along by word of mouth due to being de
>   facto rules, and, while preferred, aren't hard requirements.
>
> - Some of these rules are commonly used in various projects, so people
>   automatically assume everyone knows them, even though they don't. Then
>   when someone asks "but what are the rules, anyway" there's a mad
>   scramble to hunt down a description in someone else's documentation,
>   often from kernel.org
>
> - Often, documentation doesn't get written down because it's obvious
>   to the people who write documentation, and the people who need the
>   docs don't know it. And in the case of procedural rules this is even
>   more egregious, because you don't have code to point at to prove that
>   it isn't documented. Then eventually someone who was unfamiliar with
>   the process says "how about we document this" and everyone else is
>   like "oh uh, yeah, that makes sense, we probably should have done this
>   a long time ago".
>
> It's always nice to find a documentation file such as
> 'doc/submitting-patches.asciidoc' (or reStructuredText or texinfo or
> whatever the project uses) documenting the conventions, as well as any
> project-specific guidance. It will also usually tell you the email
> address for the appropriate mailing list. One example can be found here:
>
> https://www.archlinux.org/pacman/submitting-patches.html
>
> source:
> https://git.archlinux.org/pacman.git/tree/doc/submitting-patches.asciidoc
>
> Currently the grub-dev manual talks a lot about the coding style
> (similar in spirit to my HACKING.asciidoc/HACKING.html) and very little
> about how to submit patches. This information is definitely missing and
> I believe a patch to update it would be very appropriate.

Yeah, sadly that is missing. We have to improve that thing for sure.

Daniel