[yocto] [oe] [OE-core] Git commit process question.

[Jon Mason]

On Wed, Apr 3, 2019 at 7:45 AM Tom Rini <trini at konsulko.com> wrote:
>
> On Tue, Apr 02, 2019 at 02:24:51PM -0700, akuster808 wrote:
> >
> >
> > On 4/2/19 12:47 PM, Tom Rini wrote:
> > > On Tue, Apr 02, 2019 at 04:45:16AM +0000, Jon Mason wrote:
> > >> On Tue, Apr 2, 2019 at 6:41 AM Mark Hatle <mark.hatle at windriver.com> wrote:
> > >>> On 4/1/19 6:20 PM, akuster808 wrote:
> > >>>>
> > >>>> On 4/1/19 4:02 PM, Richard Purdie wrote:
> > >>>>> On Mon, 2019-04-01 at 15:33 -0700, akuster808 wrote:
> > >>>>>> Hello,
> > >>>>>>
> > >>>>>> I have noticed a large number of git commits with no header
> > >>>>>> information being accepted.
> > >>>>> Can you be more specific about what "no header information" means? You
> > >>>>> mean a shortlog and no full log message?
> > >>>> Commits with just a "subject" and signoff. No additional information
> > >>> If you can convey the reason for the change in just the subject, that is
> > >>> acceptable.. but there is -always- supposed to be a signed-off-by line according
> > >>> to our guidelines.
> > >>>
> > >>> So if you see this, I think we need to step back and figure out where and why
> > >>> it's happening and get it resolved in the future.
> > >>>
> > >>> (Places I've seen in the past were one-off mistakes and clearly that -- so it
> > >>> wasn't anything that we needed to work on a correction.)
> > >>>
> > >>> --Mark
> > >>>
> > >>>> We tend to reference back to how the kernel does things.
> > >>>>
> > >>>> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> > >>>> These two sections in particular.
> > >>>>
> > >>>>
> > >>>>     2) Describe your changes
> > >>>>
> > >>>> Describe your problem. Whether your patch is a one-line bug fix or 5000 lines of
> > >>>> a new feature, there must be an underlying problem that motivated you to do this
> > >>>> work. Convince the reviewer that there is a problem worth fixing and that it
> > >>>> makes sense for them to read past the first paragraph.
> > >>>>
> > >>>>
> > >>>> along with this section.
> > >>>>
> > >>>>
> > >>>>     14) The canonical patch format
> > >>>>
> > >>>> This section describes how the patch itself should be formatted. Note that, if
> > >>>> you have your patches stored in a |git| repository, proper patch formatting can
> > >>>> be had with |git format-patch|. The tools cannot create the necessary text,
> > >>>> though, so read the instructions below anyway.
> > >>>>
> > >>>> The canonical patch subject line is:
> > >>>>
> > >>>> Subject: [PATCH 001/123] subsystem: summary phrase
> > >>>>
> > >>>> The canonical patch message body contains the following:
> > >>>>
> > >>>>       * A |from| line specifying the patch author, followed by an empty line
> > >>>>         (only needed if the person sending the patch is not the author).
> > >>>>       * The body of the explanation, line wrapped at 75 columns, which will be
> > >>>>         copied to the permanent changelog to describe this patch.
> > >>>>       * An empty line.
> > >>>>       * The |Signed-off-by:| lines, described above, which will also go in the
> > >>>>         changelog.
> > >>>>       * A marker line containing simply |---|.
> > >>>>       * Any additional comments not suitable for the changelog.
> > >>>>       * The actual patch (|diff| output).
> > >>>>
> > >>>>
> > >>>>     - Armin
> > >> There are existing git hooks that can be used to detect and fail to
> > >> merge patches like this.  For Linux, I have the following in
> > >> .git/hooks/pre-commit
> > >> #!/bin/sh
> > >> exec git diff --cached | scripts/checkpatch.pl -
> > > FWIW, over in U-Boot land I do:
> > > ./scripts/checkpatch.pl -q --git origin/master..
> > > as part of checking things prior to pushing to master.
> > Having hooks is fine but after the fact. It puts the burden back on the
> > Layer maintainer to resolve. If we think more info is better, it needs
> > to happen at time of change submittal.
>
> Note that I'm not talking about a hook, I'm talking about what's part of
> my CI process.  And when something pops up there is when I grab the
> patch in question and push back on the submitter.

Exactly!  I do the same things for the Linux stuff I own.  I'll do a
git-am, then redir the output and publicly shame them.  Public shaming
is a vastly underrated method of behavior modification.

Thanks,
Jon

>
> --
> Tom