Dariusz on Software

During a merge from branch on the same repository as current copy:

svn merge -r17007:17249 http://hostname/svn/path

I got the error: svn: Malformed URL for repository. The fix is to add user name in SVN URL:

svn merge -r17007:17249 http://dcieslak@hostname/svn/path

Is this a Subversion bug? Probably the problem is that my username placed in SVN config file has form: domainname\username. Maybe SVN merge cannot handle that format?

I've been using many version control systems in software development projects. I'm aware Subversion is not the best version control system out there, but it's most popular (at least among enterprise projects I've been working on).

I'm describing here Subversion usage best practices that come from my experience with Subversion (or other version control systems) collected on many projects. Some suggestions are related strictly to Subversion, some are general.

Commit complete, logical changesets

Always commit complete patch. "Complete" means the commit:

• shouldn't have compilation errors
• shouldn't lower successful test cases counter
• should have precise purpose (one fixed bug, implemented feature, as so on...)

Why complete changesets are so important? You can easily then:

• reapply only selected bugfixes on another branch (sometimes it's called cherry-picking)
• undo change that made system unstable by simple reverted merge

Sometimes you have many different changes (example: two bugfixes, one feature added) in your workspace and it seems the easiest way is to commit them in one big commit. DON'T. You can use "svn diff", "patch" commands to split your local changeset into few parts and commit them separately. How? It's simple (but not obvious at first glance):

1. svn diff > ../changeset.diff
2. $EDITOR ../changeset.diff
3. Leave only changes to be removed from working copy, delete remaining changes
4. patch -p0 -R <  ../changeset.diff
5. Now you have yor workspace without changes to be removed, retest, commit
6. patch -p0 <  ../changeset.diff
7. Now you have only changed that previously were skipped, retest, commit

Special tool have been implemented that can automate above flow: PMS (Patch Management System).

Do not commit workspace partially

If you commit only subset of files from your working copy you risk breaking the build. The proper way for splitting working copy into separate commits is to reverse patch your changeset (see previous practice examples).

Similar problem appears when you forget to add to versioned files new file (and it appears as "?" on "svn status" listing). Use "svn status" frequently and properly set svn:ignored where necessary.

Continuous integration simplified: commit often, update very often

If you postpone your changes more that two days it's very likely someone will use obsolete API that you're refactoring and someone will get compilation errors. Try to make atomic but small commit to minimize such risk.

When you develop a feature it's very likely you will use obsolete API that some other developer is refactoring now. Update very often to see fresh system state.

Have you noticed above paragraphs are very similar? You have to make fast information flow. Making long-living branches will postpone new code from being integrated. Frequent merges and commits are implementation of continuous integration. Every integration problem (as API change mentioned in this section) will be visible as soon as possible (when it is stable enough to be committed).

Someone can point out that frequent commits can destroy trunk stability - see next section for answer.

Shared branches must be stable, Really

When you are working on isolated topic branch you don't need to ensure it's stable enough to be committed. It's not needed to compiler, either. But if anyone will have access with you to this branch/trunk you must ensure it will remain stable. Stable branch encourage frequent updates and it's base to Continuous integration.

How can we define "stable" criteria for local changeset to be committed:

• Code compiles with no errors
• No additional failing tests (no regression!)
• Ensure no accidental whole-file reformatting by viewing diffs (svn di)
• Ensure no forgotten unversioned files (svn status = ?) in your repository (it may break compilation for someone else)

Use meaningful commit comments

A typical error (yes, error) made by novice programmers is empty or "asdf"-style commit comments. The information what was committed is hidden in commit changeset only. It will make code inspection harder. I suggest to enter at least in commit comment:

• Issue tracker ID if preset (to link commit to issue fixed)
• short one-line description (can be copied from issue tracker)

Then anyone can look at fresh commits:

svn log -l 10

and see what's going on now in the project.

Care about merge and annotate - DO NOT reformat whole file

Sometimes when I want to trace who are responsible for particular change in file I'm discovering by annotate that whole file have been changed recently by one person. Why? This programmer reformatted whole file and committed big changeset. Now file looks better but at what price? Original committer information is lost.

Similar problem occurs when you merge such changeset (from topic branch to trunk for instance). There are many conflicts because merge changeset overlap with changesets committed previously on trunk.

Subversion like many other popular version control systems loses commit information on merge (merged person is visible instead of original authors of merged changesets). If you expect to preserve such information use GIT. (Thanks Michal to pointing this out: Subversion >= 1.5 tracks merges).

Setup auto properties correctly

In mixed systems environment there are many formats of storing files. EOL (end of line) standards differs between operating systems. Subversion allows to convert-on-fly EOL style. In order to make use of such feature you have to set "svn:mime-type: text/plain".

Setting such properties on every added files can be boring, so auto properties have been introduced in Subversion. You can map (locally) properties to extensions in your ~/.subversion/config:

[auto-props]
*.csv = svn:mime-type=text/plain
*.java = svn:mime-type=text/plain
*.sql = svn:mime-type=text/plain

Setup svn:ignore in repository

Some files are required to live in project directory but shouldn't be committed into SVN (*.class, *.ear, ...). General "ignore" mechanism marks such files (or file masks) to be invisible for SVN (not reported as unknown files "?"). Typically CVS used .cvsignore file, Bazaar reads .bzrignore, SVN reads svn:ignore property AND local user preferences stored in ~/.subversion/config.

Do not depend on local developer ignore settings in ~/.subversion/config. Attach svn:ignore tag to proper directories to be present on every working copy. It will make to add new filters easier (by commiting new properties once to Subversion).

Correcting comments of existing commits is easy if you have enough privileges on subversion repository:

svn propedit -r <revision-number-here> --revprop svn:log "<new log message>" <url>

That's all. Pretty simple.

Aplikacja.info believes in continuous integration and frequent reases, so uses so-called stable trunk policy. That means: at any time any developer can make installation from trunk to production systems. How stable trunk can be achieved?

• any commit done to trunk is protected by set of automated tests that should run 100% clean (more critical project will use framework for a continuous build process like Cruise Control or PQM).
• "risky" tasks are done using peer code review process based on version control software

What to review?

We believe the most efficient method of reviewing code is to look at changesets (unified diffs). Then reviewer can see exact changes done by another developer to meet task criteria and codebase to review is minimized to really important parts of code (changes with some lines of context). That's why we believe forming proper changesets is very important requirement.

Commit methods

There are mainly three methods of inserting new commits into trunk (specified in our implementation by special "target" field in bug tracker):

• Direct commit (target=master) without code review: used mainly for simplest functionality without much risk involved. Minimal administration burden, minimal merge conflict.
• Patch Based Review (target=patch) using PMS functionality for one-way code review: in this scenario only one developer is supposed to modify patch, another one is only reviewing. At least two developers are involved, merge conflict minimized by basing on trunk (patch is always re-based on trunk during development).
• Shared branch (target=branch) using VCS: Development is branched on separate branch and reviewved on this branch. Many developers can commit on this branch/review before merge. There is higher merge conflict risk for long development on branch.

A convention used by us: patch / branch name should contain noun from task summary and task id separated by a dash:

noun-task_id

for example: invoice-2351. This way we can easily find topic branches and precisely point to taks specification (by a task id).

Peer Code Review

Where's peer code review here? Three modes of commit, three review configurations follows:

• Direct commit: changeset is reviewed after commit on trunk (as I said lower risk encourages us to be flexible)
• Patch-based (read-only) changeset view: patch is sent to another team member for review by email or shared patches repository
• Classical "topic branch": read/write review: team member switches to topic branch and checks latest commits, he may also place comments in source code

Old, Good TODOs

What is a result of code review? The most efficient way to track problems found is to insert comments inside source code (in comments, near problematic code location). We prepared a convention of well-known TODO tags in source code: TODO:nick description. "nick" is abbreviation of comment recipient, description should be short and valuable. Here's the example:

/*
TODO:DCI VAT report summary should show taxes grouped by rates,
not the case now
*/

Recipient then scans all source code for such TODOs and fix problems found (thus removes TODO in the same commit). This way:

• we can connect problem report with problem fix in one changeset
• we can track who entered comment and who fixed it (from SVN history)

Summary

Code inspections were found to be very effective method of raising quality of software (the IBM story). I believe informal implementation called peer review plays very well with other agile tools we are using for developing better software.

You probably already know that Subversion stores some kind of metadata for all files added to repository. It's called "properties" in Subversion vocabulary. This key-value map is responsible for registering ingored files masks, file attributes, internal file content type etc.

The property I'm going to present today is "mime-type". It describes file content in similar way to HTTP header "Content-type" telling svn client how to handle the file. Typical values are: "text/plain", "application/octet-stream". Especially first part of mime-type is important:

• text/*: line-by-line merges are used, diffs are generated
• any other prefix: no text merges prepared

If you do not set this properly right you may end up with messed binary file (end-of-line conversions) or non-mergable changes in text file (that is marked as binary by mistake).

Of course during adding a file to workspace one can forget to set those properties correctly. Here auto-props comes with help. Auto-props are applied when "svn add" command (from command line or from GUI) is issued. Configuration is placed inside "~/.subversion/config" file. Here's my config fragment from one of projects.

[auto-props]
*.csv = svn:mime-type=text/plain
*.java = svn:mime-type=text/plain
*.sql = svn:mime-type=text/plain
*.sql = svn:keywords=Author Date Id Revision URL
*.jar = svn:mime-type=application/octet-stream

Besides mime-type svn:keywords is being set in the example. It controls which keywords are expanded in source files.

Let's say you tracked someone broke HEAD of trunk and you want to reverse that single commit from main branch. Subversion makes very handy syntax for reverse merges:

svn merge -c -19203 https://REPO_URL

In example above you do reverse merge of 19203 revision (note "-" sign before revision number). After that merge:

• Inspect if workspace compiles without errors
• If it's OK: commit local changeset
• Notify 19203 committer about the change

In order to fix broken commit original author should do the opposite:

svn merge -c 19203 https://REPO_URL

(note there's no "-" before revision number). Then:

• Correct changeset and test if workspace is not broken
• If it's OK: commit local changeset (it will be nice to show original revision number in a comment)