Calebotomy

Submitting Patches is better than “Patches Welcome”

So I received my first patch for Perl code a week ago. Amazingly I didn’t have to tell the person patches were welcome, they must have assumed so because my project was licensed under an OSI approved license. Also amazingly I’ve applied the patch even though it didn’t meet my quality standards. I figure that’s my fault though because no where did I document what those were.

Back when I forked Regen2 from Funtoo one of the first things I did (and a reason for the fork) was create a policy for patch submission. The policy for Funtoo was that all patches were welcome, and I recall 2 patches by 1 user specifically that should have been 10 patches and 2 (or more) of those 10 patches should have been dropped, because they were doing things they shouldn’t have. In order to create this policy as easily as possible, I stole Git’s SubmittingPatches policy and modified it to work with Regen2.

Once again I’ve been faced with patches that just aren’t up to par. So once again I’m using Git’s policy as a baseline for contributions to my projects. Here’s the beginning of the documentation

Checklist (and a short version for the impatient):

Commits:

- make commits of logical units
- check for unnecessary whitespace with “git diff –check”
before committing
- do not check in commented out code or unneeded files
- the first line of the commit message should be a short
description and should skip the full stop
- the body should provide a meaningful commit message, which:
- uses the imperative, present tense: “change”,
not “changed” or “changes”.
- includes motivation for the change, and contrasts
its implementation with previous behaviour
- if you want your work included in the main repository, add a
“Signed-off-by: Your Name you@example.com” line to the
commit message (or just use the option “-s” when
committing) to confirm that you agree to the Developer’s
Certificate of Origin
- make sure that you have tests for the bug you are fixing
- make sure that the test suite passes after your commit

Patch:

- use “git format-patch -M” to create the patch
- do not PGP sign your patch
- be careful doing cut & paste, not to corrupt whitespaces.
- provide additional information (which is unsuitable for
the commit message) between the “—” and the diffstat
- if you change, add, or remove any features or
make some other user interface change, the associated
documentation should be updated as well.
- if your name is not writable in ASCII, make sure that
you send the patch in the correct encoding.

Long version:


The only part I added was to add documentation for features. Obviously some parts under patch need modification as I’m not using a mailing list.

Obviously part of the reason I don’t like “Patches welcome” is I’ve seen that policy lead to the acceptance of really bad patches. Now obviously I make mistakes, everyone makes mistakes, but patches can be improved before they are committed. Even good patches can be flat out rejected for other reasons (I’ve had this happen. The consensus was not that the patch was bad, but the entire script need to be rewritten).

So what was wrong with this patch if I accepted it? well essentially it wasn’t a git patch and it was applied to the wrong directory (because I had the original source in src/ and the generated a / I’m not using CommitBuild to another branch) and, it also broke my test suite (even though it was a patch for a test. The breakage was a bug in another test).

In any event, I believe that having your project be “Open Source” and having a patch submission policy does more to say “Patches Welcome” than “Patches Welcome”. You don’t have to agree with that, but regardless of whether you do, or not, I think having a policy is a good thing.

P.S. Trolling posts will be deleted. Be constructive or talk elsewhere.


Share

comments powered by Disqus