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

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

[Carlos O'Donell]

Executive Summary
=================

I would like to propose that the "Contribution Checklist"
tell first time developers to send a 'git format-patch'
file or inlined text to the list for patch review instead
of a split patch and distinct ChangeLog.

Details
=======

I find it much easier to review patches which are effectively
an inlined 'git format-patch' file, or an attached 
'git format-patch' file which I can apply right away.

I use the the standard ChangeLog merge-driver for git and
I've literally had zero problems with this. So splitting
out the ChangeLog as the "Contribution Checklist" recommends
is something many of us have stopped doing.

It's simply so much easier to pile on a bunch of commits in
your local tree while you work, use git rebase -i to fix
things and edit, and then git format-patch to get a series
of patches ready to go.

Having the ChangeLog split out makes work for me, and
is annoying.

Having a 'git format-patch' file shows clearly what
will be committed as part of the commit message for
which we often want careful review to record the intent
of the change in a logical way.

Can we get consensus to change the "Contribution Checklist"
to suggest a 'git format-patch' patch?

-- 
Cheers,
Carlos.

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

[Paul Eggert]

On 2/13/19 1:33 PM, Carlos O'Donell wrote:
> Can we get consensus to change the "Contribution Checklist"
> to suggest a 'git format-patch' patch?

I like the idea; it'd save me work too.

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

[Joseph Myers]

On Wed, 13 Feb 2019, Carlos O'Donell wrote:

> Executive Summary
> =================
> 
> I would like to propose that the "Contribution Checklist"
> tell first time developers to send a 'git format-patch'
> file or inlined text to the list for patch review instead
> of a split patch and distinct ChangeLog.

We should be looking at what the instructions should be once the 
ChangeLog-generation script is ready for use and ChangeLog entries don't 
need writing manually in any form (for which something that works with 
"git am" seems a good idea).  I don't think instructions for ChangeLog 
entries are a particularly fruitful area in which to discuss changes other 
than eliminating the manually-written ChangeLog entries.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Carlos O'Donell]

On 2/13/19 6:00 PM, Joseph Myers wrote:
> On Wed, 13 Feb 2019, Carlos O'Donell wrote:
> 
>> Executive Summary
>> =================
>>
>> I would like to propose that the "Contribution Checklist"
>> tell first time developers to send a 'git format-patch'
>> file or inlined text to the list for patch review instead
>> of a split patch and distinct ChangeLog.
> 
> We should be looking at what the instructions should be once the 
> ChangeLog-generation script is ready for use and ChangeLog entries don't 
> need writing manually in any form (for which something that works with 
> "git am" seems a good idea).  I don't think instructions for ChangeLog 
> entries are a particularly fruitful area in which to discuss changes other 
> than eliminating the manually-written ChangeLog entries.
 
I agree, and I think this cleanup is part of a process to get
towards that goal.

What I want to do is the following:

(a) We get consensus that we want a patch that is in the form
    generated by git format-patch.

(b) I rewrite the Contribution Checklist a bit to organize the
    information into a flow that looks like:
    - A list of things to do.
      - Has *one* central section on "ChangeLog" writing
        (ready to be deleted).
    - Commit, write commit message (ready to have new instructions added).
    - git-format patch.
    - Post.

(c) Cleanup the list to mention only things relevant to master.
    All legacy notes are removed to a legacy branches page.

(d) When the new ChangeLog script is read I will delete
    the "Write a Changelog" part of the list of things to do,
    and adjust the "write commit message" with newer instructions.

Does this seem sensible to you (even if it isn't the exact set of
steps you might take)?

-- 
Cheers,
Carlos.

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

[Joseph Myers]

On Wed, 13 Feb 2019, Carlos O'Donell wrote:

> (a) We get consensus that we want a patch that is in the form
>     generated by git format-patch.

I think that more specifically we want a patch in the form that works with 
"git am" (and the formatting details are only really relevant anyway for a 
contributor without write access, save insofar as it's unclear what is 
actually proposed to be committed).  For example, that includes the "---" 
convention for information in the patch submission that is not intended to 
be part of the commit message, such as information about changes relative 
to the previous version of that patch.

In practice commit messages from inexperienced contributors need 
significant editing before use (and ChangeLog entries even more so), so 
exactly how they arrange the ChangeLog part of the submission doesn't 
matter much.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Florian Weimer]

* Carlos O'Donell:

> I would like to propose that the "Contribution Checklist"
> tell first time developers to send a 'git format-patch'
> file or inlined text to the list for patch review instead
> of a split patch and distinct ChangeLog.

I would like to see the actual wording.

Should we move the contribution checklist to Git, so that it we can
maintain it more easily?

Perhaps not into the manual, where it become outdated after
installation, but somewhere else in the tree?

Thanks,
Florian

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

[Joseph Myers]

On Thu, 14 Feb 2019, Florian Weimer wrote:

> Should we move the contribution checklist to Git, so that it we can
> maintain it more easily?

I think documentation belongs in git *when it is tied to a particular 
version of the source tree* (and so people working with an older source 
tree version should be using the corresponding version of the 
documentation).  So documentation of the source tree structure belongs in 
git (much of maint.texi is unfortunately rather out of date), but 
descriptions of process for contributing do not.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Florian Weimer]

* Joseph Myers:

> On Thu, 14 Feb 2019, Florian Weimer wrote:
>
>> Should we move the contribution checklist to Git, so that it we can
>> maintain it more easily?
>
> I think documentation belongs in git *when it is tied to a particular 
> version of the source tree* (and so people working with an older source 
> tree version should be using the corresponding version of the 
> documentation).  So documentation of the source tree structure belongs in 
> git (much of maint.texi is unfortunately rather out of date), but 
> descriptions of process for contributing do not.

I dont't think we have a good review process for official wiki changes.
Not much wiki content benefits from such a review, I suppose, but there
are some exceptions.

Thanks,
Florian

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

[Carlos O'Donell]

On 2/14/19 4:58 AM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> I would like to propose that the "Contribution Checklist"
>> tell first time developers to send a 'git format-patch'
>> file or inlined text to the list for patch review instead
>> of a split patch and distinct ChangeLog.
> 
> I would like to see the actual wording.

OK, I just went ahead and made *all* the changes I wanted to
make and put them into the streamlined v2 version of the
contribution checklist. I expect that when we fix ChangeLog
generation we can easily edit just "Properly Formatted GNU ChangeLog"
which is the only place that references ChangeLogs.

Current:
https://sourceware.org/glibc/wiki/Contribution%20checklist

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.

Created:
https://sourceware.org/glibc/wiki/Legacy%20Contributions

Reviewed and updated:
https://sourceware.org/glibc/wiki/Committer%20checklist
https://sourceware.org/glibc/wiki/GlibcGit

-- 
Cheers,
Carlos.

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

[Florian Weimer]

* 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).

Please capitalize “Bugzilla” consistently.

* 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.

* 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.)

* Contribution Email Subject Line

This is not followed at all, so I think we can substantially consolidate
this section.

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).

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)`
: * `Add fall-through comments.`

* Contribution Email Body

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

* `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.

* 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.

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

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.

Thanks,
Florian

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

[Joseph Myers]

On Fri, 15 Feb 2019, Florian Weimer wrote:

> I don't think we should suggest that scripts/build-many-glibcs.py
> improves a contribution.  Not everyone will be able to run it.

It's also only relevant to certain kinds of changes that are likely to 
involve architecture-specific issues.  Most normal changes don't involve 
anything architecture-specific, or are architecture-specific for a single 
architecture, and so testing on a single architecture is fine.

> : "Contributed by" statements *in source code comments* are no longer
> : required or desired in glibc source files (though existing statements
> : will remain).

I think we should remove the existing "Contributed by" statements (that 
avoids them appearing in new files people copy from old ones) - but before 
removing such a statement we need to make sure the names in question are 
properly credited in contrib.texi.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Florian Weimer]

* Joseph Myers:

> On Fri, 15 Feb 2019, Florian Weimer wrote:
>
>> I don't think we should suggest that scripts/build-many-glibcs.py
>> improves a contribution.  Not everyone will be able to run it.
>
> It's also only relevant to certain kinds of changes that are likely to 
> involve architecture-specific issues.  Most normal changes don't involve 
> anything architecture-specific, or are architecture-specific for a single 
> architecture, and so testing on a single architecture is fine.

On the other hand, I find extremely difficult to spot whether a change
is architecture-specific.  Consider the reason IP_RECVERR breakage.  Not
having done any Darwin or Hurd development work, I had no idea that the
feature was missing there.

>> : "Contributed by" statements *in source code comments* are no longer
>> : required or desired in glibc source files (though existing statements
>> : will remain).
>
> I think we should remove the existing "Contributed by" statements (that 
> avoids them appearing in new files people copy from old ones) - but before 
> removing such a statement we need to make sure the names in question are 
> properly credited in contrib.texi.

I don't want to make such changes because as far as I know, for some
contributors at least, we cannot legally remove them unilaterally
because these authors have a right to be named as such, something that
the FSF copyright assignment did not (and could not) take away from
them.

Thanks,
Florian

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

[Joseph Myers]

On Wed, 13 Mar 2019, Florian Weimer wrote:

> > I think we should remove the existing "Contributed by" statements (that 
> > avoids them appearing in new files people copy from old ones) - but before 
> > removing such a statement we need to make sure the names in question are 
> > properly credited in contrib.texi.
> 
> I don't want to make such changes because as far as I know, for some
> contributors at least, we cannot legally remove them unilaterally
> because these authors have a right to be named as such, something that

What's the basis for such a right to be named in the source file (as 
opposed to in contrib.texi, ChangeLogs, etc.)?

(In UK law, the right to be identified as author of a work does not apply 
to a computer program anyway.)

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Carlos O'Donell]

On 3/13/19 5:35 PM, Joseph Myers wrote:
> On Wed, 13 Mar 2019, Florian Weimer wrote:
> 
>>> I think we should remove the existing "Contributed by" statements (that 
>>> avoids them appearing in new files people copy from old ones) - but before 
>>> removing such a statement we need to make sure the names in question are 
>>> properly credited in contrib.texi.
>>
>> I don't want to make such changes because as far as I know, for some
>> contributors at least, we cannot legally remove them unilaterally
>> because these authors have a right to be named as such, something that
> 
> What's the basis for such a right to be named in the source file (as 
> opposed to in contrib.texi, ChangeLogs, etc.)?
> 
> (In UK law, the right to be identified as author of a work does not apply 
> to a computer program anyway.)
 
I wondered this also and I spoke with DJ about this with regards to
US law. The best we could figure is that there has to be some country
where copyright assignment and authorship are two distinct items.
You can assign copyright, but authorship remains yours, and cannot
be removed. Removing a name might be seen as the equivalent of removing
authorship. I don't know if this is true, the ChangeLog and VCS all
contain authorship information for the file anyway, so I think we might
be able to remove "Contributed by" statements to avoid confusion.

IANAL. I think we might need legal advice here.

-- 
Cheers,
Carlos.

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

[Florian Weimer]

* Carlos O'Donell:

> I wondered this also and I spoke with DJ about this with regards to
> US law. The best we could figure is that there has to be some country
> where copyright assignment and authorship are two distinct items.
> You can assign copyright, but authorship remains yours, and cannot
> be removed. Removing a name might be seen as the equivalent of removing
> authorship. I don't know if this is true, the ChangeLog and VCS all
> contain authorship information for the file anyway, so I think we might
> be able to remove "Contributed by" statements to avoid confusion.
>
> IANAL. I think we might need legal advice here.

Agreed.

If you want to read up on this, key phrases are “moral rights” and
“droit d'auteur”.

Thanks,
Florian

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

[Gabriel F. T. Gomes]

On Wed, Mar 13 2019, Florian Weimer wrote:
> * Carlos O'Donell:
> 
> > I wondered this also and I spoke with DJ about this with regards to
> > US law. The best we could figure is that there has to be some country
> > where copyright assignment and authorship are two distinct items.
> > You can assign copyright, but authorship remains yours, and cannot
> > be removed. Removing a name might be seen as the equivalent of removing
> > authorship. I don't know if this is true, the ChangeLog and VCS all
> > contain authorship information for the file anyway, so I think we might
> > be able to remove "Contributed by" statements to avoid confusion.
> >
> > IANAL. I think we might need legal advice here.
> 
> Agreed.
> 
> If you want to read up on this, key phrases are “moral rights” and
> “droit d'auteur”.

I am also not a lawyer, but if you are looking for one such country
where copyright assignment and authorship are distinct items, I know
that, in Brazil, these two things are treated differently, i.e.: we have
what I would translate to "moral rights" and "property rights".  One
cannot give up or transfer their "moral rights", per Chapter II,
Article 27 [1].

[1] Reference (in Brazilian Portuguese):
http://www.planalto.gov.br/ccivil_03/leis/L9610.htm

There is another law [2] that distinguishes computer programs from other
types of intellectual works, which states that "moral rights" do *not*
apply to computer programs, except for the fact that the author has the
right to claim authorship (i.e. precisely what's been discussed here),
per Chapter II, Article 2, Paragraph 1.

[2] Reference (in Brazilian Portuguese):
http://www.planalto.gov.br/ccivil_03/leis/L9609.htm

Anyhow, IANAL. :/

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

[Florian Weimer]

* Joseph Myers:

> On Wed, 13 Mar 2019, Florian Weimer wrote:
>
>> > I think we should remove the existing "Contributed by" statements (that 
>> > avoids them appearing in new files people copy from old ones) - but before 
>> > removing such a statement we need to make sure the names in question are 
>> > properly credited in contrib.texi.
>> 
>> I don't want to make such changes because as far as I know, for some
>> contributors at least, we cannot legally remove them unilaterally
>> because these authors have a right to be named as such, something that
>
> What's the basis for such a right to be named in the source file (as 
> opposed to in contrib.texi, ChangeLogs, etc.)?
>
> (In UK law, the right to be identified as author of a work does not apply 
> to a computer program anyway.)

As far I know, the relevant German law makes no distinction based on the
kind of work.

Acknowledgement in the manual would not be sufficient for downstreams
that do not distribute it over licensing concerns.  With ChangeLog
files, I think the only gap would be installed header files, which are
sometimes distributed without ChangeLogs.  (Such distributions probably
do not meet the requirements of the GPL and other applicable licenses,
either, so maybe that does not matter.)

All this is just an uneducated guess.  I see that my comment quoted
above might be misleading.  What I intended to convey was that, I,
personally, will not make such changes.  I do not have a strong opinion
whether the project should keep them.

Thanks,
Florian

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

[Carlos O'Donell]

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.