Return-Path: Delivered-To: apmail-subversion-commits-archive@minotaur.apache.org Received: (qmail 97203 invoked from network); 4 Feb 2010 08:56:43 -0000 Received: from hermes.apache.org (HELO mail.apache.org) (140.211.11.3) by minotaur.apache.org with SMTP; 4 Feb 2010 08:56:43 -0000 Received: (qmail 16618 invoked by uid 500); 4 Feb 2010 08:56:43 -0000 Delivered-To: apmail-subversion-commits-archive@subversion.apache.org Received: (qmail 16515 invoked by uid 500); 4 Feb 2010 08:56:43 -0000 Mailing-List: contact commits-help@subversion.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@subversion.apache.org, commits@subversion.apache.org Delivered-To: mailing list commits@subversion.apache.org Received: (qmail 16502 invoked by uid 99); 4 Feb 2010 08:56:43 -0000 Received: from nike.apache.org (HELO nike.apache.org) (192.87.106.230) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 04 Feb 2010 08:56:43 +0000 X-ASF-Spam-Status: No, hits=-2000.0 required=10.0 tests=ALL_TRUSTED X-Spam-Check-By: apache.org Received: from [140.211.11.4] (HELO eris.apache.org) (140.211.11.4) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 04 Feb 2010 08:56:28 +0000 Received: by eris.apache.org (Postfix, from userid 65534) id 077C6238890A; Thu, 4 Feb 2010 08:56:06 +0000 (UTC) Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: svn commit: r906405 [1/3] - /subversion/site/publish/docs/community-guide/ Date: Thu, 04 Feb 2010 08:56:05 -0000 To: commits@subversion.apache.org From: jerenkrantz@apache.org X-Mailer: svnmailer-1.0.8 Message-Id: <20100204085606.077C6238890A@eris.apache.org> X-Virus-Checked: Checked by ClamAV on apache.org Author: jerenkrantz Date: Thu Feb 4 08:56:04 2010 New Revision: 906405 URL: http://svn.apache.org/viewvc?rev=906405&view=rev Log: * docs/community-guide/*: Take a shot at refactoring community guide into sub-pages while also retaining single-page view for those who like Ctrl+F. (This is not linked up to as part of the site, so it's "hidden" for now.) Added: subversion/site/publish/docs/community-guide/building.html (with props) subversion/site/publish/docs/community-guide/building.part.html (with props) subversion/site/publish/docs/community-guide/building.toc.html (with props) subversion/site/publish/docs/community-guide/conventions.html (with props) subversion/site/publish/docs/community-guide/conventions.part.html (with props) subversion/site/publish/docs/community-guide/conventions.toc.html (with props) subversion/site/publish/docs/community-guide/debugging.html (with props) subversion/site/publish/docs/community-guide/debugging.part.html (with props) subversion/site/publish/docs/community-guide/debugging.toc.html (with props) subversion/site/publish/docs/community-guide/general.html (with props) subversion/site/publish/docs/community-guide/general.part.html (with props) subversion/site/publish/docs/community-guide/general.toc.html (with props) subversion/site/publish/docs/community-guide/guide-nav.html (with props) subversion/site/publish/docs/community-guide/guide.css (with props) subversion/site/publish/docs/community-guide/index-onepage.html (with props) subversion/site/publish/docs/community-guide/issues.html (with props) subversion/site/publish/docs/community-guide/issues.part.html (with props) subversion/site/publish/docs/community-guide/issues.toc.html (with props) subversion/site/publish/docs/community-guide/l10n.html (with props) subversion/site/publish/docs/community-guide/l10n.part.html (with props) subversion/site/publish/docs/community-guide/l10n.toc.html (with props) subversion/site/publish/docs/community-guide/mailing-lists.html (with props) subversion/site/publish/docs/community-guide/mailing-lists.part.html (with props) subversion/site/publish/docs/community-guide/mailing-lists.toc.html (with props) subversion/site/publish/docs/community-guide/releasing.html (with props) subversion/site/publish/docs/community-guide/releasing.part.html (with props) subversion/site/publish/docs/community-guide/releasing.toc.html (with props) subversion/site/publish/docs/community-guide/roles.html (with props) subversion/site/publish/docs/community-guide/roles.part.html (with props) subversion/site/publish/docs/community-guide/roles.toc.html (with props) Added: subversion/site/publish/docs/community-guide/building.html URL: http://svn.apache.org/viewvc/subversion/site/publish/docs/community-guide/building.html?rev=906405&view=auto ============================================================================== --- subversion/site/publish/docs/community-guide/building.html (added) +++ subversion/site/publish/docs/community-guide/building.html Thu Feb 4 08:56:04 2010 @@ -0,0 +1,21 @@ + + + +Apache Subversion - Community Guide - Building and Testing + + + + + + + +
+ + + + + Propchange: subversion/site/publish/docs/community-guide/building.html ------------------------------------------------------------------------------ svn:eol-style = native Propchange: subversion/site/publish/docs/community-guide/building.html ------------------------------------------------------------------------------ svn:executable = * Propchange: subversion/site/publish/docs/community-guide/building.html ------------------------------------------------------------------------------ svn:mime-type = text/html Added: subversion/site/publish/docs/community-guide/building.part.html URL: http://svn.apache.org/viewvc/subversion/site/publish/docs/community-guide/building.part.html?rev=906405&view=auto ============================================================================== --- subversion/site/publish/docs/community-guide/building.part.html (added) +++ subversion/site/publish/docs/community-guide/building.part.html Thu Feb 4 08:56:04 2010 @@ -0,0 +1,316 @@ +
+

Building and Testing

+ +
+

The configuration/build system under unix

+ +

Greg Stein wrote a custom build system for Subversion, which had +been using `automake' and recursive Makefiles. Now it uses a single, +top-level Makefile, generated from Makefile.in (which is kept under +revision control). `Makefile.in' in turn includes `build-outputs.mk', +which is automatically generated from `build.conf' by the +`gen-make.py' script. Thus, the latter two are under revision +control, but `build-outputs.mk' is not.

+ +

Here is Greg's original mail describing the system, followed by +some advice about hacking it:

+ +
+   From: Greg Stein <gstein@lyra.org>
+   Subject:  new build system (was: Re: CVS update: MODIFIED: ac-helpers ...)
+   To: dev@subversion.tigris.org
+   Date: Thu, 24 May 2001 07:20:55 -0700
+   Message-ID: <20010524072055.F5402@lyra.org>
+
+   On Thu, May 24, 2001 at 01:40:17PM -0000, gstein@tigris.org wrote:
+   >   User: gstein
+   >   Date: 01/05/24 06:40:17
+   >
+   >   Modified:    ac-helpers .cvsignore svn-apache.m4
+   >   Added:       .        Makefile.in
+   >   Log:
+   >   Switch over to the new non-recursive build system.
+   >...
+
+   Okay... this is it. We're now on the build system.
+
+       "It works on my machine."
+
+   I suspect there may be some tweaks to make on different OSs. I'd be
+   interested to hear if Ben can really build with normal BSD make. It
+   should be possible.
+
+   The code supports building, installation, checking, and
+   dependencies. It does *NOT* yet deal with the doc/ subdirectory. That
+   is next; I figured this could be rolled out and get the kinks worked
+   out while I do the doc/ stuff.  Oh, it doesn't build Neon or APR yet
+   either. I also saw a problem where libsvn_fs wasn't getting built
+   before linking one of the test proggies (see below).
+
+   Basic operation: same as before.
+
+   $ ./autogen.sh
+   $ ./configure OPTIONS
+   $ make
+   $ make check
+   $ make install
+
+   There are some "make check" scripts that need to be fixed up. That'll
+   happen RSN. Some of them create their own log, rather than spewing to
+   stdout (where the top-level make will place the output into
+   [TOP]/tests.log).
+
+   The old Makefile.am files are still around, but I'll be tossing those
+   along with a bunch of tweaks to all the .cvsignore files. There are a
+   few other cleanups, too. But that can happen as a step two.
+
+   [ $ cvs rm -f `find . -name Makefile.rm`
+
+     See the mistake in that line? I didn't when I typed it. The find
+     returned nothing, so cvs rm -f proceeded to delete my entire
+     tree. And the -f made sure to delete all my source files, too. Good
+     fugging thing that I had my mods in some Emacs buffers, or I'd be
+     bitching.
+
+     I am *so* glad that Ben coded SVN to *not* delete locally modified
+     files *and* that we have an "undel" command. I had to go and tweak a
+     bazillion Entries files to undo the delete...
+   ]
+
+   The top-level make has a number of shortcuts in it (well, actually in
+   build-outputs.mk):
+
+   $ make subversion/libsvn_fs/libsvn_fs.la
+
+   or
+
+   $ make libsvn_fs
+
+   The two are the same. So... when your test proggie fails to link
+   because libsvn_fs isn't around, just run "make libsvn_fs" to build it
+   immediately, then go back to the regular "make".
+
+   Note that the system still conditionally builds the FS stuff based
+   on whether DB (See 'Building on Unix' below) is available, and
+   mod_dav_svn if Apache is available.
+
+   Handy hint: if you don't like dependencies, then you can do:
+
+   $ ./autogen.sh -s
+
+   That will skip the dependency generation that goes into
+   build-outputs.mk. It makes the script run quite a bit faster (48 secs
+   vs 2 secs on my poor little Pentium 120).
+
+   Note that if you change build.conf, you can simply run:
+
+   $ ./gen-make.py build.conf
+
+   to regen build-outputs.mk. You don't have to go back through the whole
+   autogen.sh / configure process.
+
+   You should also note that autogen.sh and configure run much faster now
+   that we don't have the automake crap. Oh, and our makefiles never
+   re-run configure on you out of the blue (gawd, I hated when automake
+   did that to me).
+
+   Obviously, there are going to be some tweaky things going on. I also
+   think that the "shadow" builds or whatever they're called (different
+   source and build dirs) are totally broken. Something tweaky will have
+   to happen there.  But, thankfully, we only have one Makefile to deal
+   with.
+
+   Note that I arrange things so that we have one generated file
+   (build-outputs.mk), and one autoconf-generated file (Makefile from
+   .in).  I also tried to shove as much logic/rules into
+   Makefile.in. Keeping build-outputs.mk devoid of rules (thus, implying
+   gen-make.py devoid of rules in its output generation) manes that
+   tweaking rules in Makefile.in is much more approachable to people.
+
+   I think that is about it. Send problems to the dev@ list and/or feel
+   free to dig in and fix them yourself. My next steps are mostly
+   cleanup. After that, I'm going to toss out our use of libtool and rely
+   on APR's libtool setup (no need for us to replicate what APR already
+   did).
+
+   Cheers,
+   -g
+
+   --
+   Greg Stein, http://www.lyra.org/
+
+ +

And here is some advice for those changing or testing the +configuration/build system:

+ +
+   From: Karl Fogel <kfogel@collab.net>
+   To: dev@subversion.tigris.org
+   Subject: when changing build/config stuff, always do this first
+   Date: Wed 28 Nov 2001
+
+   Yo everyone: if you change part of the configuration/build system,
+   please make sure to clean out any old installed Subversion libs
+   *before* you try building with your changes.  If you don't do this,
+   your changes may appear to work fine, when in fact they would fail if
+   run on a truly pristine system.
+
+   This script demonstrates what I mean by "clean out".  This is
+   `/usr/local/cleanup.sh' on my system.  It cleans out the Subversion
+   libs (and the installed httpd-2.0 libs, since I'm often reinstalling
+   that too):
+
+      #!/bin/sh
+
+      # Take care of libs
+      cd /usr/local/lib
+      rm -f APRVARS
+      rm -f libapr*
+      rm -f libexpat*
+      rm -f libneon*
+      rm -f libsvn*
+
+      # Take care of headers
+      cd /usr/local/include
+      rm -f apr*
+      rm -f svn*
+      rm -f neon/*
+
+      # Take care of headers
+      cd /usr/local/apache2/lib
+      rm -f *
+
+   When someone reports a configuration bug and you're trying to
+   reproduce it, run this first. :-)
+
+   The voice of experience,
+   -Karl
+
+ +

The build system is a vital tool for all developers working on trunk. +Sometimes, changes made to the build system work perfectly fine for +one developer, but inadvertently break the build system for another.

+ +

+To prevent loss of productivity, any committer (full or partial) can +immediately revert any build system change that breaks their ability to +effectively do development on their platform of choice, as a matter of +ordinary routing, without fear of accusations of an over-reaction. +The log message of the commit reverting the change should contain an +explanatory note saying why the change is being reverted, containing +sufficient detail to be suitable to start off a discussion of the +problem on dev@, should someone chose to reply to the commit mail.

+ +

However, care should be taken not go into "default revert mode". +If you can quickly fix the problem, then please do so. If not, then stop and +think about it for a minute. After you've thought about it, and you still have +no solution, go ahead and revert the change, and bring the discussion to the +list.

+ +

Once the change has been reverted, it is up to the original committer of +the reverted change to either recommit a fixed version of their original +change, if, based on the reverting committer's rationale, they feel very +certain that their new version definitely is fixed, or, to submit the +revised version for testing to the reverting committer before committing +it again.

+ +
+ + +
+

Automated tests

+ +

For a description of how to use and add tests to Subversion's +automated test framework, please read subversion/tests/README and subversion/tests/cmdline/README.

+ +

Various people have arranged for the automated test framework to +run at regular intervals on their own machines, sending the results to +the svn-breakage@subversion.tigris.org mailing list. The more +different platforms the tests run on, the more quickly we can detect +portability bugs in Subversion. If you'd like to send svn-breakage +messages too, use the svntest framework (start at the README).

+ +
+ + +
+

Build farm

+ +

Lieven Govaerts has set up a +BuildBot build/test +farm at http://buildbot.subversion.org/buildbot/, see this message:

+ +
+   http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=114212
+   (Thread URL: http://subversion.tigris.org/servlets/BrowseList?list=dev&by=thread&from=450110)
+   Message-ID: 20060326205918.F3C50708B0@adicia.telenet-ops.be
+   From: "Lieven Govaerts" <lgo@mobsol.be>
+   To: <dev@subversion.tigris.org>
+   Subject: Update: Subversion build and test farm with Buildbot.
+   Date: Sun, 26 Mar 2006 22:56:11 +0200
+
+ +

for more details. (BuildBot is a system for centrally managing multiple automated +testing environments; it's especially useful for portability testing, +including of uncommitted changes.)

+ +

Once the details of the build farm get finalized, +integrate those instructions here. Perhaps the information at +https://svn.apache.org/repos/asf/subversion/trunk/www/build-farm.html?p=867958 +can serve as a good starting point?

+ +
+ + +
+

Writing test cases before code

+ +
+From: Karl Fogel <kfogel@collab.net>
+Subject: writing test cases
+To: dev@subversion.tigris.org
+Date: Mon, 5 Mar 2001 15:58:46 -0600
+
+Many of us implementing the filesystem interface have now gotten into
+the habit of writing the test cases (see fs-test.c) *before* writing
+the actual code.  It's really helping us out a lot -- for one thing,
+it forces one to define the task precisely in advance, and also it
+speedily reveals the bugs in one's first try (and second, and
+third...).
+
+I'd like to recommend this practice to everyone.  If you're
+implementing an interface, or adding an entirely new feature, or even
+just fixing a bug, a test for it is a good idea.  And if you're going
+to write the test anyway, you might as well write it first. :-)
+
+Yoshiki Hayashi's been sending test cases with all his patches lately,
+which is what inspired me to write this mail to encourage everyone to
+do the same.  Having those test cases makes patches easier to examine,
+because they show the patch's purpose very clearly.  It's like having
+a second log message, one whose accuracy is verified at run-time.
+
+That said, I don't think we want a rigid policy about this, at least
+not yet.  If you encounter a bug somewhere in the code, but you only
+have time to write a patch with no test case, that's okay -- having
+the patch is still useful; someone else can write the test case.
+
+As Subversion gets more complex, though, the automated test suite gets
+more crucial, so let's all get in the habit of using it early.
+
+-K
+
+ +
+ +
Propchange: subversion/site/publish/docs/community-guide/building.part.html ------------------------------------------------------------------------------ svn:eol-style = native Propchange: subversion/site/publish/docs/community-guide/building.part.html ------------------------------------------------------------------------------ svn:executable = * Propchange: subversion/site/publish/docs/community-guide/building.part.html ------------------------------------------------------------------------------ svn:keywords = Date Propchange: subversion/site/publish/docs/community-guide/building.part.html ------------------------------------------------------------------------------ svn:mime-type = text/html Added: subversion/site/publish/docs/community-guide/building.toc.html URL: http://svn.apache.org/viewvc/subversion/site/publish/docs/community-guide/building.toc.html?rev=906405&view=auto ============================================================================== --- subversion/site/publish/docs/community-guide/building.toc.html (added) +++ subversion/site/publish/docs/community-guide/building.toc.html Thu Feb 4 08:56:04 2010 @@ -0,0 +1,8 @@ +
  • Building and Testing +
      +
    1. The configuration/build system under unix
      2. +
    2. Automated tests
      3. +
    3. Build farm
      4. +
    4. Writing test cases before code
      5. +
    +
    • Propchange: subversion/site/publish/docs/community-guide/building.toc.html ------------------------------------------------------------------------------ svn:eol-style = native Propchange: subversion/site/publish/docs/community-guide/building.toc.html ------------------------------------------------------------------------------ svn:executable = * Propchange: subversion/site/publish/docs/community-guide/building.toc.html ------------------------------------------------------------------------------ svn:keywords = Date Propchange: subversion/site/publish/docs/community-guide/building.toc.html ------------------------------------------------------------------------------ svn:mime-type = text/html Added: subversion/site/publish/docs/community-guide/conventions.html URL: http://svn.apache.org/viewvc/subversion/site/publish/docs/community-guide/conventions.html?rev=906405&view=auto ============================================================================== --- subversion/site/publish/docs/community-guide/conventions.html (added) +++ subversion/site/publish/docs/community-guide/conventions.html Thu Feb 4 08:56:04 2010 @@ -0,0 +1,21 @@ + + + +Apache Subversion - Community Guide - Coding and Commit Conventions + + + + + + + +
    + + + + + Propchange: subversion/site/publish/docs/community-guide/conventions.html ------------------------------------------------------------------------------ svn:eol-style = native Propchange: subversion/site/publish/docs/community-guide/conventions.html ------------------------------------------------------------------------------ svn:executable = * Propchange: subversion/site/publish/docs/community-guide/conventions.html ------------------------------------------------------------------------------ svn:mime-type = text/html Added: subversion/site/publish/docs/community-guide/conventions.part.html URL: http://svn.apache.org/viewvc/subversion/site/publish/docs/community-guide/conventions.part.html?rev=906405&view=auto ============================================================================== --- subversion/site/publish/docs/community-guide/conventions.part.html (added) +++ subversion/site/publish/docs/community-guide/conventions.part.html Thu Feb 4 08:56:04 2010 @@ -0,0 +1,1158 @@ +
    +

    Coding and Commit Conventions

    + +
    +

    Code modularity and interface visibility

    + +

    Subversion's code and headers files are segregated along a couple +of key lines: library-specific vs. inter-library; public vs. private. +This separation is done primarily because of our focus on proper +modularity and code organization, but also because of our promises as +providers and maintainers of a widely adopted public API. As you +write new functions in Subversion, you'll need to carefully consider +these things, asking some questions of yourself as you go along:

    + +

    "Are the consumers of my new code local to a particular source +code file in a single library?" If so, you probably want a static +function in that same source file.

    + +

    "Is my new function of the sort that other source code within +this library will need to use, but nothing *outside* the library?" +If so, you want to use a non-static, double-underscore-named function +(such as svn_foo__do_something), with its prototype in the +appropriate library-specific header file.

    + +

    "Will my code need to be accessed from a different +library?" Here you have some additional questions to answer, such +as "Should my code live in the original library I was going to put +it in, or should it live in a more generic utility library such as +libsvn_subr?" Either way, you're now looking at using an +inter-library header file. But see the next question before you decide +which one...

    + +

    "Is my code such that it has a clean, maintainable API that can +reasonably be maintained in perpetuity and brings value to the +Subversion public API offering?" If so, you'll be adding +prototypes to the public API, immediately +inside subversion/include/. If not, double-check your plans +-- maybe you haven't chosen the best way to abstract your +functionality. But sometimes it just happens that libraries need to +share some function that is arguably of no use to other software +besides Subversion itself. In those cases, use the private header +files in subversion/include/private/.

    + +
    + +
    +

    Coding style

    + +

    Subversion uses ANSI C, and follows the GNU coding standards, +except that we do not put a space between the name of a function and +the opening parenthesis of its parameter list. Emacs users can just +load svn-dev.el to get the right indentation behavior (most source +files here will load it automatically, if `enable-local-eval' is set +appropriately).

    + +

    Read http://www.gnu.org/prep/standards.html for a full description of +the GNU coding standards. Below is a short example demonstrating the +most important formatting guidelines, including our +no-space-before-param-list-paren exception:

    + +
    +   char *                                     /* func type on own line */
+   argblarg(char *arg1, int arg2)             /* func name on own line */
+   {                                          /* first brace on own line */
+     if ((some_very_long_condition && arg2)   /* indent 2 cols */
+         || remaining_condition)              /* new line before operator */
+       {                                      /* brace on own line, indent 2 */
+         arg1 = some_func(arg1, arg2);        /* NO SPACE BEFORE PAREN */
+       }                                      /* close brace on own line */
+     else
+       {
+         do                                   /* format do-while like this */
+           {
+             arg1 = another_func(arg1);
+           }
+         while (*arg1);
+       }
+   }
+
    + +

    In general, be generous with parentheses even when you're sure +about the operator precedence, and be willing to add spaces and +newlines to avoid "code crunch". Don't worry too much about vertical +density; it's more important to make code readable than to fit that +extra line on the screen.

    + +
    + + +
    +

    Using page breaks

    + +

    We're using page breaks (the Ctrl-L character, ASCII 12) for +section boundaries in both code and plaintext prose files. Each +section starts with a page break, and immediately after the page break +comes the title of the section.

    + +

    This helps out people who use the Emacs page commands, such as +`pages-directory' and `narrow-to-page'. Such people are not as scarce +as you might think, and if you'd like to become one of them, then add +(require 'page-ext) to your .emacs and type C-x C-p C-h sometime.

    + +
    + + +
    +

    Error message conventions

    + +

    For error messages the following conventions apply:

    + +
      + +

    • Provide specific error messages only when there is information + to add to the general error message found in + subversion/include/svn_error_codes.h.

      • + +

    • Messages start with a capital letter.

      • + +

    • Try keeping messages below 70 characters.

      • + +

    • Don't end the error message with a period (".").

      • + +

    • Don't include newline characters in error messages.

      • + +

    • Quoting information is done using single quotes (e.g. "'some info'").

      • + +

    • Don't include the name of the function where the error occurs + in the error message. If Subversion is compiled using the + '--enable-maintainer-mode' configure-flag, it will provide this + information by itself.

      • + +

    • When including path or filenames in the error string, be sure + to quote them (e.g. "Can't find '/path/to/repos/userfile'").

      • + +

    • When including path or filenames in the error string, be sure + to convert them using svn_path_local_style() before inclusion (since + paths passed to and from Subversion APIs are assumed to be + in canonical form).

      • + +

    • Don't use Subversion-specific abbreviations (e.g. use "repository" + instead of "repo", "working copy" instead of "wc").

      • + +

    • If you want to add an explanation to the error, report it + followed by a colon and the explanation like this:

      + 
      +       "Invalid " SVN_PROP_EXTERNALS " property on '%s': "
+       "target involves '.' or '..'".
+
      • + +

    • Suggestions or other additions can be added after a semi-colon, + like this:

      + 
      +       "Can't write to '%s': object of same name already exists; remove "
+       "before retrying".
+
      • + +

    • Try to stay within the boundaries of these conventions, so please avoid + separating different parts of error messages by other separators such + as '--' and others.

      • + +
    + +

    Also read about Localization.

    + +
    + + +
    +

    APR pool usage conventions

    + +

    (This assumes you already basically understand how APR pools work; +see apr_pools.h for details.)

    + +

    Applications using the Subversion libraries must call +apr_initialize() before calling any Subversion functions.

    + +

    Subversion's general pool usage strategy can be summed up in two +principles:

    + +
      +

    1. The call level that created a pool is the only place to clear or + destroy that pool.

      +
      2. +

    2. When iterating an unbounded number of times, create a subpool + before entering the iteration, use it inside the loop and clear + it at the start of each iteration, then destroy it after the loop + is done, like so:

      + 
      +         apr_pool_t *iterpool = svn_pool_create(scratch_pool);
+
+         for (i = 0; i < n; ++i)
+         {
+           svn_pool_clear(iterpool);
+           do_operation(..., iterpool);
+         }
+
+         svn_pool_destroy(iterpool);
+
      +
      3. +
    + +

    Supporting the above rules, we use the following pool names as conventions +to represent various pool lifetimes:

    +
      +

    • result_pool: The pool in which the output of a function +should be allocated. A result pool declaration should always +be found in a function argument list, and never inside a local block. (But +not all functions need or have result pools.) +

      • + +

    • scratch_pool: The pool in which all function-local data +should be allocated. This pool is also provided by the caller, who may +choose to clear this pool immediately when control returns to it. +

      • + +

    • iterpool: An iteration pool, used inside loops, +as per the above example.

      • +
    + +

    (Note: Some legacy code uses a single pool function argument, +which operates as both the result and scratch pools.)

    + +

    By using an iterpool for loop-bounded data, you ensure O(1) instead +of O(N) memory leakage should the function return abruptly from +within the loop (say, due to error). That's why you shouldn't make a +subpool for data which persists throughout a function, but instead +should use the pool passed in by the caller. That memory will be +reclaimed when the caller's pool is cleared or destroyed. If the +caller is invoking the callee in a loop, then trust the caller to take +care of clearing the pool on each iteration. The same logic +propagates all the way up the call stack.

    + +

    The pool you use also helps readers of the code understand object +lifetimes. Is a given object used only during one iteration of the +loop, or will it need to last beyond the end of the loop? For +example, pool choices indicate a lot about what's going on in this +code:

    + +
    +      apr_hash_t *persistent_objects = apr_hash_make(result_pool);
+      apr_pool_t *iterpool = svn_pool_create(scratch_pool);
+
+      for (i = 0; i < n; ++i)
+      {
+        const char *intermediate_result;
+        const char *key, *val;
+        
+        svn_pool_clear(iterpool);
+        SVN_ERR(do_something(&intermediate_result, ..., iterpool));
+        SVN_ERR(get_result(intermediate_result, &key, &val, ...,
+                           result_pool));
+        apr_hash_set(persistent_objects, key, APR_HASH_KEY_STRING, val);
+      }
+
+      svn_pool_destroy(iterpool);
+      return persistent_objects;
+
    + +

    Except for some legacy code, which was written before these +principles were fully understood, virtually all pool usage in +Subversion follows the above guidelines.

    + +

    One such legacy pattern is a tendency to allocate an object inside +a pool, store the pool in the object, and then free that pool (either +directly or through a close_foo() function) to destroy the object.

    + +

    For example:

    + +
    +   /*** Example of how NOT to use pools.  Don't be like this. ***/
+
+   static foo_t *
+   make_foo_object(arg1, arg2, apr_pool_t *pool)
+   {
+      apr_pool_t *subpool = svn_pool_create(pool);
+      foo_t *foo = apr_palloc(subpool, sizeof(*foo));
+
+      foo->field1 = arg1;
+      foo->field2 = arg2;
+      foo->pool   = subpool;
+   }
+
+   [...]
+
+   [Now some function calls make_foo_object() and returns, passing
+   back a new foo object.]
+
+   [...]
+
+   [Now someone, at some random call level, decides that the foo's
+   lifetime is over, and calls svn_pool_destroy(foo->pool).]
+
    + +

    This is tempting, but it defeats the point of using pools, which is +to not worry so much about individual allocations, but rather about +overall performance and lifetime groups. Instead, foo_t generally +should not have a `pool' field. Just allocate as many foo objects as +you need in the current pool — when that pool gets +cleared or destroyed, they will all go away simultaneously.

    + +

    See also the Exception handling +section, for details of how resources associated with a pool are +cleaned up when that pool is destroyed.

    + +

    In summary:

    + +
      + +

    • Objects should not have their own pools. An object is + allocated into a pool defined by the constructor's caller. The + caller knows the lifetime of the object and will manage it via + the pool.

      +
      • + +

    • Functions should not create/destroy pools for their operation; + they should use a pool provided by the caller. Again, the + caller knows more about how the function will be used, how + often, how many times, etc. thus, it should be in charge of the + function's memory usage.

      + +

      For example, the caller might know that the app will exit upon + the function's return. Thus, the function would create extra + work if it built/destroyed a pool. Instead, it should use the + passed-in pool, which the caller is going to be tossing as part + of app-exit anyway.

      +
      • + +

    • Whenever an unbounded iteration occurs, an iteration subpool + should be used.

      +
      • + +

    • Given all of the above, it is pretty well mandatory to pass a + pool to every function. Since objects are not recording pools + for themselves, and the caller is always supposed to be + managing memory, then each function needs a pool, rather than + relying on some hidden magic pool. In limited cases, objects + may record the pool used for their construction so that they + can construct sub-parts, but these cases should be examined + carefully.

      +
      • +
    + + +

    See also Tracking down memory +leaks for tips on diagnosing pool usage problems.

    + +
    + + +
    +

    APR status codes

    + +

    Always check for APR status codes (except APR_SUCCESS) with the +APR_STATUS_IS_...() macros, not by direct comparison. This is required +for portability to non-Unix platforms.

    + +
    + + +
    +

    Exception handling

    + +

    OK, here's how to use exceptions in Subversion.

    + +
      + +

    1. Exceptions are stored in svn_error_t structures:

      + +
      +typedef struct svn_error_t
+{
+  apr_status_t apr_err;      /* APR error value, possibly SVN_ custom err */
+  const char *message;       /* details from producer of error */
+  struct svn_error_t *child; /* ptr to the error we "wrap" */
+  apr_pool_t *pool;          /* place to generate message strings from */
+  const char *file;          /* Only used iff SVN_DEBUG */
+  long line;                 /* Only used iff SVN_DEBUG */
+} svn_error_t;
+
      + +
      2. + +

    2. If you are the original creator of an error, you would do + something like this:

      + + 
      +return svn_error_create(SVN_ERR_FOO, NULL, 
+                        "User not permitted to write file");
+
      + +

      NOTICE the NULL field... indicating that this error has no + child, i.e. it is the bottom-most error.

      + +

      See also the section on writing + error messages.

      + +

      Subversion internally uses UTF-8 to store its data. This also + applies to the 'message' string. APR is assumed to return its data + in the current locale, so any text returned by APR needs + conversion to UTF-8 before inclusion in the message string.

      +
      3. + +

    3. If you receive an error, you have three choices:

      + +
        +

      1. Handle the error yourself. Use either your own code, or + just call the primitive svn_handle_error(err). (This + routine unwinds the error stack and prints out messages + converting them from UTF-8 to the current locale.)

        + +

        When your routine receives an error which it intends to + ignore or handle itself, be sure to clean it up using + svn_error_clear(). Any time such an error is not cleared + constitutes a memory leak.

        +
        2. + +

      2. Throw the error upwards, unmodified:

        + + 
        +        error = some_routine(foo);
+        if (error)
+          return svn_error_return(error);
+
        + +

        Actually, a better way to do this would be with the + SVN_ERR() macro, which does the same thing:

        + 
        +        SVN_ERR(some_routine(foo));
+
        +
        3. + +

      3. Throw the error upwards, wrapping it in a new error + structure by including it as the "child" argument:

        + + 
        +        error = some_routine(foo);
+        if (error)
+          {
+           svn_error_t *wrapper = svn_error_create(SVN_ERR_FOO, error,
+                                                   "Authorization failed");
+           return wrapper;
+          }
+
        + +

        Of course, there's a convenience routine which creates a + wrapper error with the same fields as the child, except for + your custom message:

        + + 
        +        error = some_routine(foo);
+        if (error)
+          {
+           return svn_error_quick_wrap(error, 
+                                       "Authorization failed");
+          }
+
        + +

        The same can (and should) be done by using the SVN_ERR_W() + macro:

        + + 
        +          SVN_ERR_W(some_routine(foo), "Authorization failed");
+
        +
        4. +
      + +

      In cases (b) and (c) it is important to know that resources + allocated by your routine which are associated with a pool, are + automatically cleaned up when the pool is destroyed. This means + that there is no need to cleanup these resources before passing + the error. There is therefore no reason not to use the SVN_ERR() + and SVN_ERR_W() macros. Resources associated with pools are:

      + +
        + +

      • Memory

        • + +

      • Files

        + +

        All files opened with apr_file_open are closed at pool + cleanup. Subversion uses this function in its svn_io_file_* + api, which means that files opened with svn_io_file_* or + apr_file_open will be closed at pool cleanup.

        + +

        Some files (lock files for example) need to be removed when + an operation is finished. APR has the APR_DELONCLOSE flag for + this purpose. The following functions create files which are + removed on pool cleanup:

        + +
          +

        • apr_file_open and svn_io_file_open (when passed the + APR_DELONCLOSE flag)

          • +

        • svn_io_open_unique_file (when passed TRUE in its + delete_on_close)

          • +
        + +

        Locked files are unlocked if they were locked using + svn_io_file_lock.

        +
        • +
      +
      4. + +

    4. The SVN_ERR() macro will create a wrapped error when + SVN_ERR__TRACING is defined. This helps developers + determine what caused the error, and can be enabled with the + --enable-maintainer-mode option to configure. +

      +
      5. + +

    5. Sometimes, you just want to return whatever a called function + returns, usually at the end of your own function. Avoid the + temptation to directly return the result:

      + 
      +    /* Don't do this! */
+    return some_routine(foo);
      + +

      Instead, use the svn_error_return meta-function to return the value. + This ensures that stack tracing happens correctly when enabled.

      + 
      +    return svn_error_return(some_routine(foo));
      + +
      6. +
    + +
    + + +
    +

    Secure coding guidelines

    + +

    Just like almost any other programming language, C has undesirable +features which enables an attacker to make your program fail in +predictable ways, often to the attacker's benefit. The goal of these +guidelines is to make you aware of the pitfalls of C as they apply to +the Subversion project. You are encouraged to keep these pitfalls in +mind when reviewing code of your peers, as even the most skilled and +paranoid programmers make occasional mistakes.

    + +

    Input validation is the act of defining legal input and rejecting +everything else. The code must perform input validation on all +untrusted input.

    + +

    Security boundaries:

    + +

    A security boundary in the Subversion server code must be +identified as such as this enables auditors to quickly determine the +quality of the boundary. Security boundaries exist where the running +code has access to information the user does not or where the code +runs with privileges above those of the user making the +request. Typical examples of such is code that does access control or +an application with the SUID bit set.

    + +

    Functions which make calls to a security boundary must include +validation checks of the arguments passed. Functions which themselves +are security boundaries should audit the input received and alarm when +invoked with improper values.

    + +

    [### todo: need some examples from Subversion here...]

    + +

    String operations:

    + +

    Use the string functions provided in apr_strings.h instead of +standard C library functions that write to strings. The APR functions +are safer because they do bounds-checking and dest allocation +automatically. Although there may be circumstances where it's +theoretically safe to use plain C string functions (such as when you +already know the lengths of the source and dest), please use the APR +functions anyway, so the code is less brittle and more reviewable.

    + +

    Password storage:

    + +

    Help users keep their passwords secret: When the client reads or +writes password locally, it should ensure that the file is mode +0600. If the file is readable by other users, the client should exit +with a message that tells the user to change the filemode due to the +risk of exposure.

    + +
    + + +
    +

    Destruction of stacked resources

    + +

    Some resources need destruction to ensure correct functioning of the +application. Such resources include files, especially since open +files cannot be deleted on Windows.

    + +

    When writing an API which creates and returns a stream, in the +background this stream may be stacked on a file or other stream. To +ensure correct destruction of the resources the stream is built upon, +it must correctly call the destructors of the stream(s) it is built +upon (owns).

    + +

    At first in +http://svn.haxx.se/dev/archive-2005-12/0487.shtml +and later in +http://svn.haxx.se/dev/archive-2005-12/0633.shtml this +was discussed in more general terms for files, streams, editors and +window handlers.

    + +

    As Greg Hudson put it:

    + +
    +

    On consideration, here is where I would like us to be:

    + +
    • Streams which read from or write to an underlying object own that +object, i.e. closing the stream closes the underlying object, if +applicable.
      • + +
    • The layer (function or data type) which created a stream is +responsible for closing it, except when the above rule applies.
      • + +
    • Window handlers are thought of as an odd kind of stream, and passing +the final NULL window is considered closing the stream.
      • +
    + +

    If you think of apply_textdelta as creating a window handler, then I +don't think we're too far off. svn_stream_from_aprfile isn't owning its +subsidiary file, svn_txdelta_apply is erroneously taking responsibility +for closing the window stream it is passed, and there may be some other +deviations.

    +
    + +

    There is one exception to the rules above though. When a stream is passed +to a function as an argument (for example: the 'out' parameter of +svn_client_cat2()), that routine can't call the streams destructor, since +it did not create that resource.

    + +

    If svn_client_cat2() creates a stream, it must also call the destructor +for that stream. By the above model, that stream will call the destructor +for the 'out' parameter. This is however wrong, because the responsibility +to destruct the 'out' parameter lies elsewhere.

    + +

    To solve this problem, at least in the stream case, svn_stream_disown() +has been introduced. This function wraps a stream, making sure it's +not destroyed, even though any streams stacked upon it may try +to do so.

    + +
    + + +
    +

    Other coding conventions

    + +

    In addition to the GNU standards, Subversion uses these +conventions:

    + +
      +

    • When using a path or file name as input to most Subversion APIs, be + sure to convert them to Subversion's internal/canonical form + using the svn_path_internal_style() API. Alternately, when + receiving a path or file name as output from a Subversion API, + convert them into the expected form for your platform using the + svn_path_local_style() API.

      • + +

    • Use only spaces for indenting code, never tabs. Tab display + width is not standardized enough, and anyway it's easier to + manually adjust indentation that uses spaces.

      +
      • + +

    • Restrict lines to 79 columns, so that code will display well in a + minimal standard display window. (There can be exceptions, such + as when declaring a block of 80-column text with a few extra + columns taken up by indentation, quotes, etc., if splitting each + line in two would be unreasonably messy.)

      +
      • + +

    • All published functions, variables, and structures must be signified + with the corresponding library name - such as libsvn_wc's + svn_wc_adm_open. All library-internal declarations made in a + library-private header file (such as libsvn_wc/wc.h) must be signified + by two underscores after the library prefix (such as + svn_wc__ensure_directory). All declarations private to a single file + (such as the static function get_entry_url inside of + libsvn_wc/update_editor.c) do not require any + additional namespace decorations. Symbols that need to be used + outside a library, but still are not public are put in a shared + header file in the include/private/ directory, and use + the double underscore notation. Such symbols may be used by + Subversion core code only.

      + +

      To recap:

      + 
      +         /* Part of published API: subversion/include/svn_wc.h */
+         svn_wc_adm_open()            
+         #define SVN_WC_ADM_DIR_NAME ...
+         typedef enum svn_wc_schedule_t ...
+
+         /* For use within one library only: subversion/libsvn_wc/wc.h */
+         svn_wc__ensure_directory()   
+         #define SVN_WC__BASE_EXT ... 
+         typedef struct svn_wc__compat_notify_baton_t ...
+
+         /* For use within one file: subversion/libsvn_wc/update_editor.c */ 
+         get_entry_url()
+         struct handler_baton {
+
+         /* For internal use in svn core code only:
+            subversion/include/private/svn_wc_private.h */
+         svn_wc__entry_versioned()
+
      + +

      Pre-Subversion 1.5, private symbols which needed to be used + outside of a library were put into public header files, + using the double underscore notation. This practice has been + abandoned, and any such symbols are legacy, maintained for backwards compatibility.

      +
      • + +

    • In text strings that might be printed out (or otherwise made + available) to users, use only forward quotes around paths and + other quotable things. For example:

      + 
      +         $ svn revert foo
+         svn: warning: svn_wc_is_wc_root: 'foo' is not a versioned resource
+         $
+
      + +

      There used to be a lot of strings that used a backtick for + the first quote (`foo' instead of 'foo'), but that looked bad in + some fonts, and also messed up some people's auto-highlighting, + so we settled on the convention of always using forward + quotes.

      +
      • + +

    • If you use Emacs, put something like this in your .emacs file, + so you get svn-dev.el and svnbook.el when needed:

      + 
      +         ;;; Begin Subversion development section
+         (defun my-find-file-hook ()
+           (let ((svn-tree-path (expand-file-name "~/projects/subversion"))
+                 (book-tree-path (expand-file-name "~/projects/svnbook")))
+             (cond
+              ((string-match svn-tree-path buffer-file-name)
+               (load (concat svn-tree-path "/tools/dev/svn-dev")))
+              ((string-match book-tree-path buffer-file-name)
+               ;; Handle load exception for svnbook.el, because it tries to
+               ;; load psgml, and not everyone has that available.
+               (condition-case nil
+                   (load (concat book-tree-path "/src/tools/svnbook"))
+                 (error
+                  (message "(Ignored problem loading svnbook.el.)")))))))
+
+         (add-hook 'find-file-hooks 'my-find-file-hook)
+         ;;; End Subversion development section
+
      + +

      You'll need to customize the path for your setup, of course. + You can also make the regexp to string-match more selective; for + example, one developer says:

      + 
      +      > Here's the regexp I'm using:
+      > 
+      >     "src/svn/[^/]*/\\(subversion\\|tools\\|build\\)/"
+      >
+      > Two things to notice there: (1) I sometimes have several
+      > working copies checked out under ...src/svn, and I want the
+      > regexp to match all of them; (2) I want the hook to catch only
+      > in "our" directories within the working copy, so I match
+      > "subversion", "tools" and "build" explicitly; I don't want to
+      > use GNU style in the APR that's checked out into my repo. :-)
+
      +
      • + +

    • We have a tradition of not marking files with the names of + individual authors (i.e., we don't put lines like + "Author: foo" or "@author foo" in a special position + at the top of a source file). This is to discourage + territoriality — even when a file has only one + author, we want to make sure others feel free to make changes. + People might be unnecessarily hesitant if someone appears to + have staked a personal claim to the file.

      +
      • + +

    • Put two spaces between the end of one sentence and the start of + the next. This helps readability, and allows people to use + their editors' sentence-motion and -manipulation commands.

      +
      • + +

    • There are many other unspoken conventions maintained throughout + the code, that are only noticed when someone unintentionally + fails to follow them. Just try to have a sensitive eye for the + way things are done, and when in doubt, ask.

      +
      • +
    + +
    + + +
    +

    Writing log messages

    + +

    Every commit needs a log message.

    + +

    The intended audience for a log message is a developer who is +already familiar with Subversion, but not necessarily familiar with +this particular commit. Usually when someone goes back and reads a +change, he no longer has in his head all the context around that +change. This is true even if he is the author of the change! All the +discussions and mailing list threads and everything else may be +forgotten; the only clue to what the change is about comes from the +log message and the diff itself. People revisit changes with +surprising frequency, too: for example, it might be months after the +original commit and now the change is being ported to a maintenance +branch.

    + +

    The log message is the introduction to the change. Start it off +with one line indicating the general nature of the change, and follow +that with a descriptive paragraph if necessary. This not only helps +put developers in the right frame of mind for reading the rest of the +log message, but also plays well with the "CIA" bot that echoes the +first line of each commit to realtime forums like IRC. (For details, +see http://cia.vc/.) However, if the +commit is just one simple change to one file, then you can dispense +with the general description and simply go straight to the detailed +description, in the standard filename-then-symbol format shown +below.

    + +

    Throughout the log message, use full sentences, not sentence +fragments. Fragments are more often ambiguous, and it takes only a +few more seconds to write out what you mean. Certain fragments like +"Doc fix", "New file", or "New function" are acceptable because they +are standard idioms, and all further details should appear in the +source code.

    + +

    The log message should name every affected function, variable, +macro, makefile target, grammar rule, etc, including the names of +symbols that are being removed in this commit. This helps people +searching through the logs later. Don't hide names in wildcards, +because the globbed portion may be what someone searches for later. +For example, this is bad:

    + +
    +   * subversion/libsvn_ra_pigeons/twirl.c
+     (twirling_baton_*): Removed these obsolete structures.
+     (handle_parser_warning): Pass data directly to callees, instead
+      of storing in twirling_baton_*.
+
+   * subversion/libsvn_ra_pigeons/twirl.h: Fix indentation.
+
    + +

    Later on, when someone is trying to figure out what happened to +`twirling_baton_fast', they may not find it if they just search for +"_fast". A better entry would be:

    + +
    +   * subversion/libsvn_ra_pigeons/twirl.c
+     (twirling_baton_fast, twirling_baton_slow): Removed these
+      obsolete structures. 
+     (handle_parser_warning): Pass data directly to callees, instead
+      of storing in twirling_baton_*. 
+
+   * subversion/libsvn_ra_pigeons/twirl.h: Fix indentation.
+
    + +

    The wildcard is okay in the description for +`handle_parser_warning', but only because the two structures were +mentioned by full name elsewhere in the log entry.

    + +

    You should also include property changes in your log messages. +For example, if you were to modify the "svn:ignore" property on +the trunk, you might put something like this in your log:

    + +
    +   * trunk/ (svn:ignore): Ignore 'build'.
+
    + +

    The above only applies to properties you maintain, not those +maintained by subversion like "svn:mergeinfo".

    + +

    Note how each file gets its own entry prefixed with an "*", and the +changes within a file are grouped by symbol, with the symbols listed +in parentheses followed by a colon, followed by text describing the +change. Please adhere to this format, even when only one file is +changed — not only does consistency aid readability, +it also allows software to colorize log entries automatically.

    + +

    As an exception to the above, if you make exactly the same change +in several files, list all the changed files in one entry. For +example:

    + +
    +   * subversion/libsvn_ra_pigeons/twirl.c,
+     subversion/libsvn_ra_pigeons/roost.c:
+     Include svn_private_config.h.
+
    + +

    If all the changed files are deep inside the source tree, you can +shorten the file name entries by noting the common prefix before the +change entries:

    + +
    +   [in subversion/bindings/swig/birdsong]
+
+   * dialects/nightingale.c (get_base_pitch): Allow 3/4-tone
+     pitch variation to account for trait variability amongst
+     isolated populations Erithacus megarhynchos.
+
+   * dialects/gallus_domesticus.c: Remove. Unreliable due to
+     extremely low brain-to-body mass ratio.
+
    + +

    If your change is related to a specific issue in the issue tracker, +then include a string like "issue #N" in the log message, but make +sure you still summarize what the change is about. For example, if a +patch resolves issue #1729, then the log message might be:

    + +
    +   Fix issue #1729: Don't crash because of a missing file.
+
+   * subversion/libsvn_ra_ansible/get_editor.c
+     (frobnicate_file): Check that file exists before frobnicating.
+
    + +

    Try to put related changes together. For example, if you create +svn_ra_get_ansible2(), deprecating svn_ra_get_ansible(), then those +two things should be near each other in the log message:

    + +
    +   * subversion/include/svn_ra.h
+     (svn_ra_get_ansible2): New prototype, obsoletes svn_ra_get_ansible.
+     (svn_ra_get_ansible): Deprecate.
+
    + +

    For large changes or change groups, group the log entry into +paragraphs separated by blank lines. Each paragraph should be a set +of changes that accomplishes a single goal, and each group should +start with a sentence or two summarizing the change. Truly +independent changes should be made in separate commits, of course.

    + +

    See Crediting for how to give credit to +someone else if you are committing their patch, or committing a change +they suggested.

    + +

    One should never need the log entries to understand the current +code. If you find yourself writing a significant explanation in the +log, you should consider carefully whether your text doesn't actually +belong in a comment, alongside the code it explains. Here's an +example of doing it right:

    + +
    +   (consume_count): If `count' is unreasonable, return 0 and don't
+    advance input pointer.
+
    + +

    And then, in `consume_count' in `cplus-dem.c':

    + +
    +   while (isdigit((unsigned char)**type))
+     {
+       count *= 10;
+       count += **type - '0';
+       /* A sanity check.  Otherwise a symbol like
+         `_Utf390_1__1_9223372036854775807__9223372036854775'
+         can cause this function to return a negative value.
+         In this case we just consume until the end of the string.  */
+      if (count > strlen(*type))
+        {
+          *type = save;
+          return 0;
+        }
+
    + +

    This is why a new function, for example, needs only a log entry +saying "New Function" --- all the details should be in the source.

    + +

    You can make common-sense exceptions to the need to name everything +that was changed. For example, if you have made a change which +requires trivial changes throughout the rest of the program (e.g., +renaming a variable), you needn't name all the functions affected, you +can just say "All callers changed". When renaming any symbol, please +remember to mention both the old and new names, for traceability; see +r861020 for an example.

    + +

    In general, there is a tension between making entries easy to find +by searching for identifiers, and wasting time or producing unreadable +entries by being exhaustive. Use the above guidelines and your best +judgment, and be considerate of your fellow developers. (Also, use +"svn log" to see how others have been writing their log entries.)

    + +

    Log messages for documentation or translation have somewhat looser +guidelines. The requirement to name every symbol obviously does not +apply, and if the change is just one more increment in a continuous +process such as translation, it's not even necessary to name every +file. Just briefly summarize the change, for example: "More work on +Malagasy translation." Please write your log messages in English, so +everybody involved in the project can understand the changes you +made.

    + +
    + + +
    +

    Crediting

    + +

    It is very important to record code contributions in a consistent +and parseable way. This allows us to write scripts to figure out who +has been actively contributing — and what they have +contributed — so we can spot +potential new committers quickly. The Subversion project uses +human-readable but machine-parseable fields in log messages to +accomplish this.

    + +

    When committing a patch written by someone else, use +"Patch by: " at the beginning of a line to indicate the +author:

    + +
    +   Fix issue #1729: Don't crash because of a missing file.
+
+   * subversion/libsvn_ra_ansible/get_editor.c
+     (frobnicate_file): Check that file exists before frobnicating.
+
+   Patch by: J. Random <jrandom@example.com>
+
    + +

    If multiple individuals wrote the patch, list them each on a +separate line — making sure to start each continuation +line with whitespace. Non-committers should be listed by name, if +known, and e-mail. Full and partial committers should be listed by +their canonical usernames from COMMITTERS (the leftmost column in that file). Additionally, +"me" is an acceptable shorthand for the person actually committing the +change.

    + +
    +   Fix issue #1729: Don't crash because of a missing file.
+
+   * subversion/libsvn_ra_ansible/get_editor.c
+     (frobnicate_file): Check that file exists before frobnicating.
+
+   Patch by: J. Random <jrandom@example.com>
+             Enrico Caruso <codingtenor@codingtenor.com>
+             jcommitter
+             me
+
    + +

    If someone found the bug or pointed out the problem, but didn't +write the patch, indicate their contribution with +"Found by: ":

    + +
    +   Fix issue #1729: Don't crash because of a missing file.
+
+   * subversion/libsvn_ra_ansible/get_editor.c
+     (frobnicate_file): Check that file exists before frobnicating.
+
+   Found by: J. Random <jrandom@example.com>
+
    + +

    If someone suggested something useful, but didn't write the patch, +indicate their contribution with "Suggested by: ":

    + +
    +   Extend the Contribulyzer syntax to distinguish finds from ideas.
+
+   * www/hacking.html (crediting): Adjust accordingly.
+
+   Suggested by: dlr
+
    + +

    If someone reviewed the change, use "Review by: " +(or "Reviewed by: " if you prefer):

    + +
    +   Fix issue #1729: Don't crash because of a missing file.
+
+   * subversion/libsvn_ra_ansible/get_editor.c
+     (frobnicate_file): Check that file exists before frobnicating.
+
+   Review by: Eagle Eyes <eeyes@example.com>
+
    + +

    A field may have multiple lines, and a log message may contain any +combination of fields:

    + +
    +   Fix issue #1729: Don't crash because of a missing file.
+
+   * subversion/libsvn_ra_ansible/get_editor.c
+     (frobnicate_file): Check that file exists before frobnicating.
+
+   Patch by: J. Random <jrandom@example.com>
+             Enrico Caruso <codingtenor@codingtenor.com>
+             me
+   Found by: J. Random <jrandom@example.com>
+   Review by: Eagle Eyes <eeyes@example.com>
+              jcommitter
+
    + +

    Further details about a contribution should be listed in a +parenthetical aside immediately after the corresponding field. Such +an aside always applies to the field right above it; in the following +example, the fields have been spaced out for readability, but note +that the spacing is optional and not necessary for parseability:

    + +
    +   Fix issue #1729: Don't crash because of a missing file.
+
+   * subversion/libsvn_ra_ansible/get_editor.c
+     (frobnicate_file): Check that file exists before frobnicating.
+
+   Patch by: J. Random <jrandom@example.com>
+   (Tweaked by me.)
+
+   Review by: Eagle Eyes <eeyes@example.com>
+              jcommitter
+   (Eagle Eyes caught an off-by-one-error in the basename extraction.)
+
    + +

    Currently, these fields

    + +
    +   Patch by:
+   Suggested by:
+   Found by:
+   Review by:
+
    + +

    are the only officially-supported crediting fields (where +"supported" means scripts know to look for them), and they are widely +used in Subversion log messages. Future fields will probably be of +the form "VERB by: ", and from time to time someone may use +a field that sounds official but really is not — for +example, there are a few instances of "Reported by: ". +These are okay, but try to use an official field, or a parenthetical +aside, in preference to creating your own. Also, don't use +"Reported by: " when the reporter is already recorded in an +issue; instead, simply refer to the issue.

    + +

    Look over Subversion's existing log messages to see how to use +these fields in practice. This command from the top of your trunk +working copy will help:

    + +
    +svn log | contrib/client-side/search-svnlog.pl "(Patch|Review|Suggested) by: "
+
    + +

    Note: The "Approved by: " field seen in some +commit messages is totally unrelated to these crediting fields, and is +generally not parsed by scripts. It is simply the standard syntax for +indicating either who approved a partial committer's commit outside +their usual area, or (in the case of merges to release branches) who +voted for the change to be merged.

    + +
    + +
    Propchange: subversion/site/publish/docs/community-guide/conventions.part.html ------------------------------------------------------------------------------ svn:eol-style = native Propchange: subversion/site/publish/docs/community-guide/conventions.part.html ------------------------------------------------------------------------------ svn:executable = * Propchange: subversion/site/publish/docs/community-guide/conventions.part.html ------------------------------------------------------------------------------ svn:keywords = Date Propchange: subversion/site/publish/docs/community-guide/conventions.part.html ------------------------------------------------------------------------------ svn:mime-type = text/html Added: subversion/site/publish/docs/community-guide/conventions.toc.html URL: http://svn.apache.org/viewvc/subversion/site/publish/docs/community-guide/conventions.toc.html?rev=906405&view=auto ============================================================================== --- subversion/site/publish/docs/community-guide/conventions.toc.html (added) +++ subversion/site/publish/docs/community-guide/conventions.toc.html Thu Feb 4 08:56:04 2010 @@ -0,0 +1,16 @@ +
  • Coding and Commit Conventions +
      +
    1. Code modularity and interface visibility
      2. +
    2. Coding style
      3. +
    3. Using page breaks
      4. +
    4. Error message conventions
      5. +
    5. APR pool usage conventions
      6. +
    6. APR status codes
      7. +
    7. Exception handling
      8. +
    8. Secure coding guidelines
      9. +
    9. Destruction of stacked resources
      10. +
    10. Other coding conventions
      11. +
    11. Writing log messages
      12. +
    12. Crediting
      13. +
    +
    • Propchange: subversion/site/publish/docs/community-guide/conventions.toc.html ------------------------------------------------------------------------------ svn:eol-style = native Propchange: subversion/site/publish/docs/community-guide/conventions.toc.html ------------------------------------------------------------------------------ svn:executable = * Propchange: subversion/site/publish/docs/community-guide/conventions.toc.html ------------------------------------------------------------------------------ svn:keywords = Date Propchange: subversion/site/publish/docs/community-guide/conventions.toc.html ------------------------------------------------------------------------------ svn:mime-type = text/html Added: subversion/site/publish/docs/community-guide/debugging.html URL: http://svn.apache.org/viewvc/subversion/site/publish/docs/community-guide/debugging.html?rev=906405&view=auto ============================================================================== --- subversion/site/publish/docs/community-guide/debugging.html (added) +++ subversion/site/publish/docs/community-guide/debugging.html Thu Feb 4 08:56:04 2010 @@ -0,0 +1,21 @@ + + + +Apache Subversion - Community Guide - Debugging Subversion + + + + + + + +
    + + + + + Propchange: subversion/site/publish/docs/community-guide/debugging.html ------------------------------------------------------------------------------ svn:eol-style = native Propchange: subversion/site/publish/docs/community-guide/debugging.html ------------------------------------------------------------------------------ svn:executable = * Propchange: subversion/site/publish/docs/community-guide/debugging.html ------------------------------------------------------------------------------ svn:mime-type = text/html Added: subversion/site/publish/docs/community-guide/debugging.part.html URL: http://svn.apache.org/viewvc/subversion/site/publish/docs/community-guide/debugging.part.html?rev=906405&view=auto ============================================================================== --- subversion/site/publish/docs/community-guide/debugging.part.html (added) +++ subversion/site/publish/docs/community-guide/debugging.part.html Thu Feb 4 08:56:04 2010 @@ -0,0 +1,255 @@ +
    +

    Debugging Subversion

    + +
    +

    Debugging the server

    + +
    +

    Debugging the DAV server

    + +

    'mod_dav_svn.so' contains the main Subversion server logic; it runs +as a module within mod_dav, which runs as a module within httpd. +Since httpd is probably using dynamic shared modules, you normally +won't be able to set breakpoints in advance when you start Apache in a +debugger such as GDB. Instead, you'll need to start up, then +interrupt httpd, set your breakpoint, and continue:

    + +
    +   % gdb httpd
+   (gdb) run -X
+   ^C
+   (gdb) break some_func_in_mod_dav_svn
+   (gdb) continue
+
    + +

    The -X switch is equivalent to -DONE_PROCESS and -DNO_DETACH, which +ensure that httpd runs as a single thread and remains attached to the +tty, respectively. As soon as it starts, it sits and waits for +requests; that's when you hit control-C and set your breakpoint.

    + +

    You'll probably want to watch Apache's run-time logs

    + +
    +   /usr/local/apache2/logs/error_log
+   /usr/local/apache2/logs/access_log
+
    + +

    to help determine what might be going wrong and where to set +breakpoints.

    + +
    + +
    +

    Debugging the ra_svn client and server, on Unix

    + +

    Bugs in ra_svn usually manifest themselves with one of the +following cryptic error messages:

    + +
    +  svn: Malformed network data
+  svn: Connection closed unexpectedly
+
    + +

    (The first message can also mean the data stream was corrupted in +tunnel mode by user dotfiles or hook scripts; see +issue #1145.) The first message generally means you to have +to debug the client; the second message generally means you have to +debug the server.

    + +

    It is easiest to debug ra_svn using a build with --disable-shared +--enable-maintainer-mode. With the latter option, the error message +will tell you exactly what line to set a breakpoint at; otherwise, +look up the line number at the end of marshal.c:vparse_tuple() where +it returns the "Malformed network data" error.

    + +

    To debug the client, simply pull it up in gdb, set a breakpoint, +and run the offending command:

    + +
    +  % gdb svn
+  (gdb) break marshal.c:NNN
+  (gdb) run ARGS
+  Breakpoint 1, vparse_tuple (list=___, pool=___, fmt=___, 
+    ap=___) at subversion/libsvn_ra_svn/marshal.c:NNN
+  NNN                                 "Malformed network data");
+
    + +

    There are several bits of useful information:

    + +
      +

    • A backtrace will tell you exactly what protocol exchange is + failing.

      +
      • + +

    • "print *conn" will show you the connection buffer. read_buf, + read_ptr, and read_end represent the read buffer, which can + show + you the data the marshaller is looking at. (Since read_buf isn't + generally 0-terminated at read_end, be careful of falsely assuming + that there's garbage data in the buffer.)

      +
      • + +

    • The format string determines what the marshaller was expecting to + see.

      +
      • +
    + +

    To debug the server in daemon mode, pull it up in gdb, set a +breakpoint (usually a "Connection closed unexpectedly" error on the +client indicates a "Malformed network data" error on the server, +although it can also indicate a core dump), and run it with the "-X" +option to serve a single connection:

    + +
    +  % gdb svnserve
+  (gdb) break marshal.c:NNN
+  (gdb) run -X
+
    + +

    Then run the offending client command. From there, it's just like +debugging the client.

    + +

    Debugging the server in tunnel mode is more of a pain. You'll need +to stick something like "{ int x = 1; while (x); }" near the top of +svnserve's main() and put the resulting svnserve in the user path on +the server. Then start an operation, gdb attach the process on the +server, "set x = 0", and step through the code as desired.

    + +
    + +
    + + +
    +

    Tracing network traffic

    + +

    Use Wireshark (formerly +known as "Ethereal") to eavesdrop on the conversation.

    + +

    First, make sure that between captures within the same wireshark +session, you hit Clear, otherwise filters from one capture +(say, an HTTP capture) might interfere with others (say, an ra_svn +capture).

    + +

    Assuming you're cleared, then:

    + +
      +

    1. Pull down the Capture menu, and choose + Capture Filters.

      2. +

    2. If debugging the http:// (WebDAV) protocol, then in the window + that pops up, choose "HTTP TCP port (80)" + (which should result in the filter string + "tcp port http").

      +

      If debugging the svn:// (ra_svn) protocol, then choose New, + give the new filter a name (say, "ra_svn"), and type + "tcp port 3690" into the filter string box.

      +

      When done, click OK.

      3. +

    3. Again go to the Capture menu, this time choose + Interfaces, and click Options next to the + appropriate interface (probably you want interface "lo", for + "loopback", assuming the server will run on the same machine as + the client).

      4. +

    4. Turn off promiscuous mode by unchecking the appropriate + checkbox.

      5. +

    5. Click the Start button in the lower right to start the + capture.

      6. +

    6. Run your Subversion client.

      7. +

    7. Click the Stop icon (a red X over an ethernet interface card) when + the operation is finished (or Capture->Stop should + work). Now you have a capture. It looks like a huge list of + lines.

      8. +

    8. Click on the Protocol column to sort.

      9. +

    9. Then, click on the first relevant line to select it; usually this + is just the first line.

      10. +

    10. Right click, and choose Follow TCP Stream. You'll be + presented with the request/response pairs of the Subversion + client's HTTP conversion.

      11. +
    + +

    The above instructions are specific to the graphical version of +Wireshark (version 0.99.6), and don't apply to the command-line +version known as "tshark" (which corresponds to "tethereal", from back +when Wireshark was called Ethereal).

    + +

    Alternatively, you may set the neon-debug-mask parameter in your +servers configuration file to cause neon's debugging output +to appear when you run the svn client. The numeric value of +neon-debug-mask is a combination of the NE_DBG_... values +in the header file ne_utils.h. For current versions of neon, setting +neon-debug-mask to 130 (i.e. NE_DBG_HTTP+NE_DBG_HTTPBODY) +will cause the HTTP data to be shown.

    + +

    You may well want to disable compression when doing a network +trace — see the http-compression parameter in the +servers configuration file.

    + +

    Another alternative is to set up a logging proxy between the +Subversion client and server. A simple way to do this is to use the +socat program. For example, to log communication with an +svnserve instance, run the following command:

    + +

    socat -v TCP4-LISTEN:9630,reuseaddr,fork + TCP4:localhost:svn

    + +

    Then run your svn commands using an URL base of +svn://127.0.0.1:9630/; socat will forward the +traffic from port 9630 to the normal svnserve port (3690), and will +print all traffic in both directions to standard error, prefixing it +with < and > signs to show the direction of the traffic.

    + +
    + + +
    +

    Tracking down memory leaks

    + +

    Our use of APR pools makes it unusual for us to have memory leaks +in the strictest sense; all the memory we allocate will be cleaned up +eventually. But sometimes an operation takes more memory than it +should; for instance, a checkout of a large source tree should not use +much more memory than a checkout of a small source tree. When that +happens, it generally means we're allocating memory from a pool whose +lifetime is too long.

    + +

    If you have a favorite memory leak tracking tool, you can configure +with --enable-pool-debug (which will make every pool allocation use +its own malloc()), arrange to exit in the middle of the operation, and +go to it. If not, here's another way:

    + +
      + +

    • Configure with --enable-pool-debug=verbose-alloc. Make sure to + rebuild all of APR and Subversion so that every allocation gets + file-and-line information.

      +
      • + +

    • Run the operation, piping stderr to a file. Hopefully you have + lots of disk space.

      +
      • + +

    • In the file, you'll see lots of lines which look like:

      + + 
      +    POOL DEBUG: [5383/1024] PCALLOC (      2763/      2763/      5419) \
+    0x08102D48 "subversion/svn/main.c:612"                             \
+    <subversion/libsvn_subr/auth.c:122> (118/118/0)
+
      + +

      What you care about most is the tenth field (the one in + quotes), which gives you the file and line number where the + pool for this allocation was created. Go to that file and line + and determine the lifetime of the pool. In the example above, + main.c:612 indicates that this allocation was made in the + top-level pool of the svn client. If this were an allocation + which was repeated many times during the course of an + operation, that would indicate a source of a memory leak. The + eleventh field (the one in brackets) gives the file and line + number of the allocation itself.

      +
      • + +
    + +
    + +
    Propchange: subversion/site/publish/docs/community-guide/debugging.part.html ------------------------------------------------------------------------------ svn:eol-style = native Propchange: subversion/site/publish/docs/community-guide/debugging.part.html ------------------------------------------------------------------------------ svn:executable = * Propchange: subversion/site/publish/docs/community-guide/debugging.part.html ------------------------------------------------------------------------------ svn:keywords = Date Propchange: subversion/site/publish/docs/community-guide/debugging.part.html ------------------------------------------------------------------------------ svn:mime-type = text/html Added: subversion/site/publish/docs/community-guide/debugging.toc.html URL: http://svn.apache.org/viewvc/subversion/site/publish/docs/community-guide/debugging.toc.html?rev=906405&view=auto ============================================================================== --- subversion/site/publish/docs/community-guide/debugging.toc.html (added) +++ subversion/site/publish/docs/community-guide/debugging.toc.html Thu Feb 4 08:56:04 2010 @@ -0,0 +1,7 @@ +
  • Debugging Subversion +
      +
    1. Debugging the server
      2. +
    2. Tracing network traffic
      3. +
    3. Tracking down memory leaks
      4. +
    +
    • Propchange: subversion/site/publish/docs/community-guide/debugging.toc.html ------------------------------------------------------------------------------ svn:eol-style = native Propchange: subversion/site/publish/docs/community-guide/debugging.toc.html ------------------------------------------------------------------------------ svn:executable = * Propchange: subversion/site/publish/docs/community-guide/debugging.toc.html ------------------------------------------------------------------------------ svn:keywords = Date Propchange: subversion/site/publish/docs/community-guide/debugging.toc.html ------------------------------------------------------------------------------ svn:mime-type = text/html Added: subversion/site/publish/docs/community-guide/general.html URL: http://svn.apache.org/viewvc/subversion/site/publish/docs/community-guide/general.html?rev=906405&view=auto ============================================================================== --- subversion/site/publish/docs/community-guide/general.html (added) +++ subversion/site/publish/docs/community-guide/general.html Thu Feb 4 08:56:04 2010 @@ -0,0 +1,21 @@ + + + +Apache Subversion - Community Guide - General Overview + + + + + + + +
    + + + + + Propchange: subversion/site/publish/docs/community-guide/general.html ------------------------------------------------------------------------------ svn:eol-style = native Propchange: subversion/site/publish/docs/community-guide/general.html ------------------------------------------------------------------------------ svn:executable = * Propchange: subversion/site/publish/docs/community-guide/general.html ------------------------------------------------------------------------------ svn:mime-type = text/html