Updates to CONTRIBUTING and CONVENTIONS

The discussion about converting some of our foreach-style APIs to
use iterator objects got me wanting to make a list of good starter
projects.  I put it in CONTRIBUTING.md and then went crazy with
updates to that file and to CONVENTIONS.md.

1 files changed, 102 insertions, 44 deletions

diff --git a/CONVENTIONS.md b/CONVENTIONS.md
index 1e909a3e8..67b60f4b3 100644
--- a/CONVENTIONS.md
+++ b/CONVENTIONS.md
@@ -1,12 +1,39 @@
 # Libgit2 Conventions
 
-We like to keep the source consistent and readable.  Herein are some guidelines
-that should help with that.
+We like to keep the source consistent and readable.  Herein are some
+guidelines that should help with that.
 
+## Compatibility
+
+`libgit2` runs on many different platforms with many different compilers.
+It is written in [ANSI C](http://en.wikipedia.org/wiki/ANSI_C) (a.k.a. C89)
+with some specific standards for function and type naming, code formatting,
+and testing.
+
+We try to avoid more recent extensions to maximize portability.  We also, to
+the greatest extent possible, try to avoid lots of `#ifdef`s inside the core
+code base.  This is somewhat unavoidable, but since it can really hamper
+maintainability, we keep it to a minimum.
+
+## Match Surrounding Code
+
+If there is one rule to take away from this document, it is *new code should
+match the surrounding code in a way that makes it impossible to distinguish
+the new from the old.* Consistency is more important to us than anyone's
+personal opinion about where braces should be placed or spaces vs. tabs.
+
+If a section of code is being completely rewritten, it is okay to bring it
+in line with the standards that are laid out here, but we will not accept
+submissions that contain a large number of changes that are merely
+reformatting.
 
 ## Naming Things
 
-All types and functions start with `git_`, and all #define macros start with `GIT_`.
+All external types and functions start with `git_` and all `#define` macros
+start with `GIT_`.  The `libgit2` API is mostly broken into related
+functional modules each with a corresponding header.  All functions in a
+module should be named like `git_modulename_functioname()`
+(e.g. `git_repository_open()`).
 
 Functions with a single output parameter should name that parameter `out`.
 Multiple outputs should be named `foo_out`, `bar_out`, etc.
@@ -14,26 +41,27 @@ Multiple outputs should be named `foo_out`, `bar_out`, etc.
 Parameters of type `git_oid` should be named `id`, or `foo_id`.  Calls that
 return an OID should be named `git_foo_id`.
 
-Where there is a callback passed in, the `void *` that is passed into it should
-be named "payload".
+Where a callback function is used, the function should also include a
+user-supplied extra input that is a `void *` named "payload" that will be
+passed through to the callback at each invocation.
 
-## Typedef
+## Typedefs
 
-Wherever possible, use `typedef`.  If a structure is just a collection of
-function pointers, the pointer types don't need to be separately typedef'd, but
-loose function pointer types should be.
+Wherever possible, use `typedef`.  In some cases, if a structure is just a
+collection of function pointers, the pointer types don't need to be
+separately typedef'd, but loose function pointer types should be.
 
 ## Exports
 
 All exported functions must be declared as:
 
-```C
+```c
 GIT_EXTERN(result_type) git_modulename_functionname(arg_list);
 ```
 
 ## Internals
 
-Functions whose modulename is followed by two underscores,
+Functions whose *modulename* is followed by two underscores,
 for example `git_odb__read_packed`, are semi-private functions.
 They are primarily intended for use within the library itself,
 and may disappear or change their signature in a future release.
@@ -43,42 +71,46 @@ and may disappear or change their signature in a future release.
 Out parameters come first.
 
 Whenever possible, pass argument pointers as `const`.  Some structures (such
-as `git_repository` and `git_index`) have internal structure that prevents
-this.
+as `git_repository` and `git_index`) have mutable internal structure that
+prevents this.
 
 Callbacks should always take a `void *` payload as their last parameter.
-Callback pointers are grouped with their payloads, and come last when passed as
-arguments:
+Callback pointers are grouped with their payloads, and typically come last
+when passed as arguments:
 
-```C
-int foo(git_repository *repo, git_foo_cb callback, void *payload);
+```c
+int git_foo(git_repository *repo, git_foo_cb callback, void *payload);
 ```
 
-
 ## Memory Ownership
 
 Some APIs allocate memory which the caller is responsible for freeing; others
 return a pointer into a buffer that's owned by some other object.  Make this
 explicit in the documentation.
 
-
 ## Return codes
 
-Return an `int` when a public API can fail in multiple ways.  These may be
-transformed into exception types in some bindings, so returning a semantically
-appropriate error code is important.  Check
-[`errors.h`](https://github.com/libgit2/libgit2/blob/development/include/git2/errors.h)
+Most public APIs should return an `int` error code.  As is typical with most
+C library functions, a zero value indicates success and a negative value
+indicates failure.
+
+Some bindings will transform these returned error codes into exception
+types, so returning a semantically appropriate error code is important.
+Check
+[`include/git2/errors.h`](https://github.com/libgit2/libgit2/blob/development/include/git2/errors.h)
 for the return codes already defined.
 
-Use `giterr_set` to provide extended error information to callers.
+In your implementation, use `giterr_set()` to provide extended error
+information to callers.
 
-If an error is not to be propagated, use `giterr_clear` to prevent callers from
-getting the wrong error message later on.
+If a `libgit2` function internally invokes another function that reports an
+error, but the error is not propagated up, use `giterr_clear()` to prevent
+callers from getting the wrong error message later on.
 
 
-## Opaque Structs
+## Structs
 
-Most types should be opaque, e.g.:
+Most public types should be opaque, e.g.:
 
 ```C
 typedef struct git_odb git_odb;
@@ -95,10 +127,10 @@ append to the end of the structure.
 
 ## Option Structures
 
-If a function's parameter count is too high, it may be desirable to package up
-the options in a structure.  Make them transparent, include a version field,
-and provide an initializer constant or constructor.  Using these structures
-should be this easy:
+If a function's parameter count is too high, it may be desirable to package
+up the options in a structure.  Make them transparent, include a version
+field, and provide an initializer constant or constructor.  Using these
+structures should be this easy:
 
 ```C
 git_foo_options opts = GIT_FOO_OPTIONS_INIT;
@@ -108,30 +140,40 @@ git_foo(&opts);
 
 ## Enumerations
 
-Typedef all enumerated types.  If each option stands alone, use the enum type
-for passing them as parameters; if they are flags, pass them as `unsigned int`.
+Typedef all enumerated types.  If each option stands alone, use the enum
+type for passing them as parameters; if they are flags to be OR'ed together,
+pass them as `unsigned int` or `uint32_t` or some appropriate type.
 
 ## Code Layout
 
-Try to keep lines less than 80 characters long.  Use common sense to wrap most
-code lines; public function declarations should use this convention:
+Try to keep lines less than 80 characters long.  This is a loose
+requirement, but going significantly over 80 columns is not nice.
 
-```C
+Use common sense to wrap most code lines; public function declarations
+can use a couple of different styles:
+
+```c
+/** All on one line is okay if it fits */
+GIT_EXTERN(int) git_foo_simple(git_oid *id);
+
+/** Otherwise one argument per line is a good next step */
 GIT_EXTERN(int) git_foo_id(
-   git_oid **out,
-   int a,
-   int b);
+	git_oid **out,
+	int a,
+	int b);
 ```
 
-Indentation is done with tabs; set your editor's tab width to 3 for best effect.
+Indent with tabs; set your editor's tab width to 4 for best effect.
 
+Avoid trailing whitespace and only commit Unix-style newlines (i.e. no CRLF
+in the repository - just set `core.autocrlf` to true if you are writing code
+on a Windows machine).
 
 ## Documentation
 
 All comments should conform to Doxygen "javadoc" style conventions for
-formatting the public API documentation.  Try to document every parameter, and
-keep the comments up to date if you change the parameter list.
-
+formatting the public API documentation.  Try to document every parameter,
+and keep the comments up to date if you change the parameter list.
 
 ## Public Header Template
 
@@ -167,3 +209,19 @@ All inlined functions must be declared as:
 GIT_INLINE(result_type) git_modulename_functionname(arg_list);
 ```
 
+## Tests
+
+`libgit2` uses the (clar)[https://github.com/vmg/clar] testing framework.
+
+All PRs should have corresponding tests.
+
+* If the PR fixes an existing issue, the test should fail prior to applying
+  the PR and succeed after applying it.
+* If the PR is for new functionality, then the tests should exercise that
+  new functionality to a certain extent.  We don't require 100% coverage
+  right now (although we are getting stricter over time).
+
+When adding new tests, we prefer if you attempt to reuse existing test data
+(in `tests-clar/resources/`) if possible.  If you are going to add new test
+repositories, please try to strip them of unnecessary files (e.g. sample
+hooks, etc).