From: Florian Köberle <florianskarten@web.de>
Date: Fri, 23 May 2008 18:12:31 GMT
Subject: Re: [JGIT PATCH v2 07/24] Added findWorkTree method to Repository class.
Message-ID: <4837090F.4000307@web.de>
In-Reply-To: <20080513002409.GC29038@spearce.org>

>> +	 * 
>> +	 * @param directory
>> +	 *            the path which should be checked.
>> +	 * @return true if the path is a valid git repository.
>> +	 */
>> +	private static boolean isRepository(File directory) {
>> +		if (!directory.isDirectory()) {
>> +			return false;
>> +		}
> 
> We usually omit { and } on simple conditions like this.  Its a coding
> pattern we stole from C Git, which stole it from the Linux kernel.

I used this style once too, but was convinced that it dangerous to do so.

The following looks correct at the first look, but it's not:

	if (a)
		if (b)
			something;
	else
		something;

this can't happen if you use { and }:
	if (a) {
		if (b) {
			something0();
		}
	} else {
		something1();
	}


Also it is better extenable:

if (a) {
	something0();
}

if (a) {
	something0():
+	something1();
}

compared too:

-if (a)
+if (a) {
	something0();
+	something1();
+ }


it's even possible that someone does it wrong:
if (a)
	something0():
	something1();

I changed it at placed I edited to the short form, but if it is ok for 
you then I would write it the long way.

> 
>> +	private static WorkTree findWorkTree(File directory) throws IOException {
>> +		File currentDirectory = directory.getAbsoluteFile();
>> +		while (true) {
>> +			final File commonGitDirectory = new File(directory,
>> +					REPOSITORY_DIRECTORY_NAME);
>> +			if (isRepository(commonGitDirectory)) {
>> +				return new WorkTree(currentDirectory, new Repository(
>> +						commonGitDirectory));
>> +			}
>> +
>> +			if (isRepository(currentDirectory)) {
>> +				return new WorkTree(null, new Repository(currentDirectory));
>> +			}
>> +			currentDirectory = currentDirectory.getParentFile();
>> +			if (currentDirectory == null) {
>> +				throw new IOException("Can't find git repository");
>> +			}
>> +		}
> 
> Would this perhaps be a shorter, easier to follow variant of the
> same algorithm?
> 
> 	directory = directory.getAbsoluteFile();
> 	File workdir = directory;
> 	while (workdir != null) {
> 		final File gitdir = new File(workdir, REPOSITORY_DIRECTORY_NAME);
> 		if (isRepository(gitdir))
> 			return new WorkTree(workdir, new Repository(gitdir));
> 		if (isRepository(workdir))
> 			return new WorkTree(null, new Repository(workdir));
> 		workdir = workdir.getParentFile();
> 	}
> 	throw new NoGitRepositoryFoundException("No repository for " + directory);
> 

I did it this way now, but the line length limit makes it harder to read 
then my first version.