Re: [JGIT PATCH v2 05/24] Added WorkTree class which can be constructed over Repository.

From: Shawn O. Pearce <spearce@spearce.org>
Date: May 13, 2008, 00:04 UTC
Message-ID: <20080513000405.GZ29038@spearce.org>
In-Reply-To: <1210623222-24908-6-git-send-email-florianskarten@web.de>

Florian Koeberle <florianskarten@web.de> wrote:

> +	public static WorkTree createWorkTree(File workTreeDirectory)

I'm only going to make this remark once for your series: In general I prefer to mark any method argument, instance field or local variable that I do not plan on changing within the method or class final. This has saved me more than once from stupidly changing a parameter when I meant to change something else.

Its not a hard and fast rule in either jgit or egit; you don't need to go back and fix every single site. Just something I have found useful over the years and now do almost everywhere.

Show 6 quoted lines

> +			throws IOException {
> +		final File gitDirectory = new File(workTreeDirectory,
> +				Constants.REPOSITORY_DIRECTORY_NAME);
> +		if (gitDirectory.exists()) {
> +			throw new IllegalArgumentException(
> +					"Repository exists in given project directory.");

When we throw an exception like this we should include the path we are complaining about already existing as part of the message (see Repository.create() for example). This way if the message makes it as far as the end-user (which it probably should, and then there's translation issues are currently ignoring in jgit) the user can at least see what path we were trying to operate on. It may help them to realize they are in the wrong directory, or made a typo on an input, etc.

Another thing about this method is we cannot supply the WindowCache that the Repository should use for object data access, which means this Repository will have its own WindowCache. Under Eclipse (or any other larger GUI/IDE) we really want only a single WindowCache instance for the entire JVM as the heap must be shared across all Repository instances.

This is actually making me reconsider how the WindowCache is handled. Maybe we should have a single WindowCache instance down inside of jgit, but allow the higher level UI (like Eclipse) to tune it on the fly. Then we don't have to worry about passing it down to utility routines like this anytime we open a Repository.

Hmmmm.

-- 
Shawn.