Re: [PATCH] Maintainers.txt: Add Giri as 2nd maintainer

[Jordan Justen <jordan.l.justen@intel.com>]

On 2016-09-07 01:00:46, Yao, Jiewen wrote:
>    Jordan
> 
>    I have a quick check:
> 
>    Here is my observation:
> 
>    1)      From
>    https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format,
>    it mentions:
> 
>      o The length of 'Pkg-Module: Brief-single-line-summary' should not
>        exceed 70 characters
> 
>    2)      In the PatchCheck.py, we have below code:
> 
>           if count >= 1 and len(lines[0]) > 76:
> 
>                self.error('First line of commit message (subject line) ' +
> 
>                           'is too long.')
> 

I think the general idea of the subject line is described well here as
the "summary phrase":

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=bca47613#n493

Regarding subject line length, I think that it should work well with
git log --oneline to produce output less than 80 characters. Based on
the standard prefix in git log --oneline, this would indicate that we
should try to use less than 72 characters. I guess the kernel uses
70-75 characters for guidance.

> 
>    3)      You recommendation
>    “Maintainers.txt: Add Giri as IntelFsp2*Pkg, IntelSiliconPkg maintainer”,
>    it is 71 char.
> 
>    However, if we add “[edk2] [PATCH]”, it becomes 85 char.
> 

Hmm, I don't think "[edk2] [PATCH]" should be factored into the
length. The length after applying in git is what should matter. Is
this is a bug in PatchCheck.py?

I think maybe the subject line length could be a warning from
PatchCheck.py, but it is almost always possible to reword the subject
line fairly easily. If it is difficult to make a good short subject
line, then it might be a sign that the patch should be split. For
example, you could split this patch in 2. One for IntelFsp2*Pkg, and
another for IntelSiliconPkg. (After all, they are 2 separate package
types, so it is reasonable to change them separately.)

-Jordan

> 
>    I am confused on the rule. So I have 2 suggestion:
> 
>    1)      wiki mentions *70* char, but tool checks *76* char. It seems
>    mismatch. We had better fix that. Can we make them consistent?
> 
>    2)      In most cases, the patch subject is start with *[edk2] [PATCH]*.
>    These 14 additional char had better be excluded in the total 70 or 76 char
>    (I am not sure what is right number), when the subject is calculated.
> 
>    I have no problem to fix this patch manually.
> 
>    I do hope we can have consistent rule and tool to help us do the check.
> 
>    Thank you
> 
>    Yao Jiewen
> 
>    From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>    Yao, Jiewen
>    Sent: Wednesday, September 7, 2016 3:21 PM
>    To: Justen, Jordan L <jordan.l.justen@intel.com>; edk2-devel@lists.01.org
>    Cc: Chan, Amy <amy.chan@intel.com>
>    Subject: Re: [edk2] [PATCH] Maintainers.txt: Add Giri as 2nd maintainer
> 
>     
> 
>    Jordan
>    I got error from BaseTools\Scripts\PatchCheck.py, if I add it.
> 
>    Can you fix the tool to remove such limitation?
> 
>    =======================
>    Checking patch file: C:\home\EdkIITransition\AllComboGit\edk2\0001-Maintainers.t
>    xt-Add-Giri-as-2nd-maintainer-to-IntelF.patch
>    The commit message format is not valid:
>    * First line of commit message (subject line) is too long.
>    https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format
>    The code passed all checks.
>    =======================
> 
>    From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jordan Justen
>    Sent: Wednesday, September 7, 2016 3:09 PM
>    To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
>    Cc: Chan, Amy <amy.chan@intel.com>
>    Subject: Re: [edk2] [PATCH] Maintainers.txt: Add Giri as 2nd maintainer
> 
>    What about this patch subject instead?
> 
>    Maintainers.txt: Add Giri as IntelFsp2*Pkg, IntelSiliconPkg maintainer
> 
>    This way we can see the affected packages from the subject line.
> 
>    -Jordan
> 
>    On 2016-09-06 18:21:10, Jiewen Yao wrote:
>    > Add Giri as 2nd maintainer to IntelFsp2*Pkg and IntelSiliconPkg.
>    >
>    > Cc: Giri P Mudusuru <giri.p.mudusuru@intel.com<mailto:giri.p.mudusuru@intel.com>>
>    > Cc: Amy Chan <amy.chan@intel.com<mailto:amy.chan@intel.com>>
>    > Contributed-under: TianoCore Contribution Agreement 1.0
>    > Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>    > ---
>    >  Maintainers.txt | 3 +++
>    >  1 file changed, 3 insertions(+)
>    >
>    > diff --git a/Maintainers.txt b/Maintainers.txt
>    > index b2e679d..6ac7085 100644
>    > --- a/Maintainers.txt
>    > +++ b/Maintainers.txt
>    > @@ -130,10 +130,12 @@ M: Jeff Fan <jeff.fan@intel.com<mailto:jeff.fan@intel.com>>
>    >  IntelFsp2Pkg
>    >  W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2Pkg
>    >  M: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>    > +M: Giri P Mudusuru <giri.p.mudusuru@intel.com<mailto:giri.p.mudusuru@intel.com>>
>    >
>    >  IntelFsp2WrapperPkg
>    >  W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFsp2WrapperPkg
>    >  M: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>    > +M: Giri P Mudusuru <giri.p.mudusuru@intel.com<mailto:giri.p.mudusuru@intel.com>>
>    >
>    >  IntelFspPkg
>    >  W: https://github.com/tianocore/tianocore.github.io/wiki/IntelFspPkg
>    > @@ -146,6 +148,7 @@ M: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>    >  IntelSiliconPkg
>    >  W: https://github.com/tianocore/tianocore.github.io/wiki/IntelSiliconPkg
>    >  M: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>    > +M: Giri P Mudusuru <giri.p.mudusuru@intel.com<mailto:giri.p.mudusuru@intel.com>>
>    >
>    >  MdeModulePkg
>    >  W: https://github.com/tianocore/tianocore.github.io/wiki/MdeModulePkg
>    > --
>    > 2.7.4.windows.1
>    >
>    > _______________________________________________
>    > edk2-devel mailing list
>    > edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>    > https://lists.01.org/mailman/listinfo/edk2-devel
>    _______________________________________________
>    edk2-devel mailing list
>    edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
>    https://lists.01.org/mailman/listinfo/edk2-devel
>    _______________________________________________
>    edk2-devel mailing list
>    edk2-devel@lists.01.org
>    https://lists.01.org/mailman/listinfo/edk2-devel