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