Re: Changes to "Contribution Checklist" -- Format of the contribution.

[Carlos O'Donell <carlos at redhat dot com>]

On 2/15/19 7:41 AM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> Proposed:
>> https://sourceware.org/glibc/wiki/Contribution%20checklist%20v2
>> - Shorter. All version specific information moved out to
>>   "Legacy Contributions" page.
>> - Streamlined. Steps in order of relevance and patch creation
>>   process.
>> - Focus on getting full patches via `git format-patch` from
>>   developers, and thus making review easy.
>> - Suggest use of 8< to split discussion from patch
>>   (see git-mailinfo).
>> - Fixed up any old information.
> 
> Thanks for doing this.  Here are my comments.
> 
> * Bugzilla Entry
> 
> I find this confusing.  I think we call those “bugs”, not “entries”.
> 
> | If your contribution fixes a user-visible issue it must have a
> | bugzilla entry.
> 
> User-visible issue *in a released version*, I think.  It also makes
> sense to file bugs for issues discovered during
> 
> : If your contribution fixes a user-visible defect present in a released
> : glibc version (or has been discovered during a release freeze), it
> : must reference a bug in Bugzilla.  Please also reference any bug that
> : has been filed even if it is not strictly needed by the preceding rule
> : (e.g., if the change concerns an enhancement, not a defect).

Fixed. Thank you for the text.
 
> Please capitalize “Bugzilla” consistently.

Fixed.

> * General Patch Requirements
> 
> I think we are mixing what should be in the posted email with what
> should go into the actual commit.  E.g., should test reports be in the
> commit message or not?  There are also general development instructions
> (such as identifying related changes).
> 
> I think this would be improved by separating the three steps: patch
> development, testing, commit formatting, and mailing list posting.
> 
> I don't think we should suggest that scripts/build-many-glibcs.py
> improves a contribution.  Not everyone will be able to run it.

I like your suggestion to further break this into the steps that a normal
developer would follow.

> * Proper Formatted Unified diff of the Changes
> 
> Suggest addition:
> 
> : Make sure that your commit is on top of a commit which is already in
> : the public Git tree, so that your collaborators can apply it using
> : `git am -3`.
> (Otherwise the command will likely fail.)

Added.
 
> * Contribution Email Subject Line
> 
> This is not followed at all, so I think we can substantially consolidate
> this section.

Some of the text is OK, others have some variance.

> This section should be titled “Commit subject line”, which is what the
> developer will actually produce before posting to the list, and come
> earlier in the checklist (after development/testing).

Agreed, I called it "First line" under "Commit formatting"

> Suggest instead:
> 
> : The first line of the commit message should contain the following:
> : 
> : * Subsystem (`argp`, `malloc`, etc., generally a directory name in the
> :   source tree), kernel (`Linux` or `Hurd`), or architecture (e.g., `x86`
> :   for all three variants).  This can be omitted for generic changes
> :   that do not fit to a particular subsystem.
> : * A colon (`:`), if the first field is present.
> : * A brief summary of the change (~50 characters long).
> : * The referenced bug(s) in Bugzilla (separated by a space from the
> :   previous).
> : 
> : Here a some examples of commit subjects:
> :
> : * `nss: Add tst-nss-files-hosts-long test [BZ #21915]`
> : * `powerpc: Fix VSCR position in ucontext (bug 24088)`

I added an example for multiple bugs: (bug xxxx, bug yyyy).

> : * `Add fall-through comments.`

I added this.

> * Contribution Email Body
> 
> These conflicts with the `git format-patch` recommendation.  We should
> describe two ways of posting `git am`-consumable patches:

Yes, I agree, I fixed this.

> * `git format-patch` output with manual edits in the ignored places.
> * `git format-patch` output posted as an attachment.
> 
> I don't know if we can get `git am` to automatically detect which case
> is which.

OK, I've done this.
 
> * Attribution
> 
> I still think we should ban `Signed-off-by` because it seems to imply
> that the submission is *not* subject to the FSF copyright assignment.

I accept that this could be a problem.

I have removed it.

> We need to distinguish between authors and entities with copyright
> ownership.  Only the latter need to be identified.

Fixed.
 
> I also suggests this change:
> 
> : "Contributed by" statements *in source code comments* are no longer
> : required or desired in glibc source files (though existing statements
> : will remain).
> 
> This should probably go under the development section, though.

Moved.

Please review the new docs.

-- 
Cheers,
Carlos.