Commit 1631147c196ff32a82f8775e3979fdf42729de6b

Russell Belfer 2013-03-02T13:51:31

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.

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 8561794..04a537f 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -7,41 +7,94 @@ your help.
 
 We hang out in the #libgit2 channel on irc.freenode.net.
 
+Also, feel free to open an
+[Issue](https://github.com/libgit2/libgit2/issues/new) to start a discussion
+about any concerns you have.  We like to use Issues for that so there is an
+easily accessible permanent record of the conversation.
+
 ## Reporting Bugs
 
-First, know which version of libgit2 your problem is in.  Compile and test
-against the `development` branch to avoid re-reporting an issue that's already
-been fixed.
+First, know which version of libgit2 your problem is in and include it in
+your bug report.  This can either be a tag (e.g.
+[v0.17.0](https://github.com/libgit2/libgit2/tree/v0.17.0) ) or a commit
+SHA (e.g.
+[01be7863](https://github.com/libgit2/libgit2/commit/01be786319238fd6507a08316d1c265c1a89407f)
+).  Using [`git describe`](http://git-scm.com/docs/git-describe) is a great
+way to tell us what version you're working with.
+
+If you're not running against the latest `development` branch version,
+please compile and test against that to avoid re-reporting an issue that's
+already been fixed.
 
-It's *incredibly* helpful to be able to reproduce the problem.  Please include
-a bit of code and/or a zipped repository (if possible).  Note that some of the
-developers are employees of GitHub, so if your repository is private, find us
-on IRC and we'll figure out a way to help you.
+It's *incredibly* helpful to be able to reproduce the problem.  Please
+include a list of steps, a bit of code, and/or a zipped repository (if
+possible).  Note that some of the libgit2 developers are employees of
+GitHub, so if your repository is private, find us on IRC and we'll figure
+out a way to help you.
 
 ## Pull Requests
 
-Life will be a lot easier for you if you create a named branch for your
-contribution, rather than just using your fork's `development`.
+Our work flow is a typical GitHub flow, where contributors fork the
+[libgit2 repository](https://github.com/libgit2/libgit2), make their changes
+on branch, and submit a
+[Pull Request](https://help.github.com/articles/using-pull-requests)
+(a.k.a. "PR").
 
-It's helpful if you include a nice description of your change with your PR; if
-someone has to read the whole diff to figure out why you're contributing in the
-first place, you're less likely to get feedback and have your change merged in.
+Life will be a lot easier for you (and us) if you follow this pattern
+(i.e. fork, named branch, submit PR).  If you use your fork's `development`
+branch, things can get messy.
+
+Please include a nice description of your changes with your PR; if we have
+to read the whole diff to figure out why you're contributing in the first
+place, you're less likely to get feedback and have your change merged in.
 
 ## Porting Code From Other Open-Source Projects
 
-The most common case here is porting code from core Git.  Git is a GPL project,
-which means that in order to port code to this project, we need the explicit
-permission of the author.  Check the
+`libgit2` is licensed under the terms of the GPL v2 with a linking
+exception.  Any code brought in must be compatible with those terms.
+
+The most common case is porting code from core Git.  Git is a pure GPL
+project, which means that in order to port code to this project, we need the
+explicit permission of the author.  Check the
 [`git.git-authors`](https://github.com/libgit2/libgit2/blob/development/git.git-authors)
-file for authors who have already consented; feel free to add someone if you've
-obtained their consent.
+file for authors who have already consented; feel free to add someone if
+you've obtained their consent.
 
-Other licenses have other requirements; check the license of the library you're
-porting code *from* to see what you need to do.
+Other licenses have other requirements; check the license of the library
+you're porting code *from* to see what you need to do.  As a general rule,
+MIT and BSD (3-clause) licenses are typically no problem.  Apache 2.0
+license typically doesn't work due to GPL incompatibility.
 
-## Styleguide
+## Style Guide
 
-We like to keep the source code consistent and easy to read.  Maintaining this
-takes some discipline, but it's been more than worth it.  Take a look at the
+`libgit2` is written in [ANSI C](http://en.wikipedia.org/wiki/ANSI_C)
+(a.k.a. C89) with some specific conventions for function and type naming,
+code formatting, and testing.
+
+We like to keep the source code consistent and easy to read.  Maintaining
+this takes some discipline, but it's been more than worth it.  Take a look
+at the
 [conventions file](https://github.com/libgit2/libgit2/blob/development/CONVENTIONS.md).
 
+## Starter Projects
+
+So, you want to start helping out with `libgit2`? That's fantastic? We
+welcome contributions and we promise we'll try to be nice.
+
+If you want to jump in, you can look at our issues list to see if there
+are any unresolved issues to jump in on.  Also, here is a list of some
+smaller project ideas that could help you become familiar with the code
+base and make a nice first step:
+
+* Convert a `git_*modulename*_foreach()` callback-based iteration API
+  into a `git_*modulename*_iterator` object with a create/advance style
+  of API.  This helps folks writing language bindings and usually isn't
+  too complicated.
+* Write a new `examples/` program that mirrors a particular core git
+  command.  (See `examples/diff.c` for example.)  This lets you (and us)
+  easily exercise a particular facet of the API and measure compatability
+  and feature parity with core git.
+* Submit a PR to clarify documentation! While we do try to document all of
+  the APIs, your fresh eyes on the documentation will find areas that are
+  confusing much more easily.
+  
diff --git a/CONVENTIONS.md b/CONVENTIONS.md
index 1e909a3..67b60f4 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).