Re: [JGIT PATCH v2 07/24] Added findWorkTree method to Repository class.

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

Florian Koeberle <florianskarten@web.de> wrote:

Show 18 quoted lines

> @@ -1242,4 +1247,89 @@ public class Repository {
>  		}
>  	}
>  
> +	/**
> +	 * Find the git repository for the current working directory.
> +	 * 
> +	 * @return a {@link Repository}.
> +	 * @throws IOException
> +	 *             if the system property user.dir isn't set or if it is
> +	 *             invalid.
> +	 */
> +	public static WorkTree findWorkTree() throws IOException {
> +		final String workingDirectoryPath = System.getProperty("user.dir");
> +		if (workingDirectoryPath == null) {
> +			throw new IOException("unable to get working directory");
> +		}
> +		final File workingDirectoryFile = new File(workingDirectoryPath);

Isn't `final File workingDirectoryFile = new File(".")` easier to write?

And if we are going to throw a checked exception because we cannot find a repository in ".", maybe that should be a new checked exception, like NoGitRepositoryFoundException?

> +	/**
> +	 * Checks if a path is a valid git repository. Works similar like the method
> +	 * is_git_directory from the original setup.c file.

References to C Git aren't common in jgit source code. We aren't a reference implementation, but that hasn't stopped people from finding out code a sane description of "how things work". I'd rather not describe our code in terms of C Git's code; especially what happens if C Git refactors one day, this reference might no longer make sense. Anyone coming back here to improve jgit or use this method will be confused about what we are doing.

Show 9 quoted lines

> +	 * 
> +	 * @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.

Show 18 quoted lines

> +	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 often do write infinite loops with "for (;;) {", but usually its because the loop condition is based on an iterator-style method returning null at the end and I want to store the result in a local variable scoped to only the body of the loop:

	for (;;) {
		final RevCommit next = walk.next();
		if (next == null)
			break;

		// use next somehow
	}

Otherwise I try to avoid infinite loops in my final version of something. Though on a recent job interview the interviewer noticed I tend to start writing code with an infinite loop, then go back and correct the loop condition before saying "done". ;-)

-- 
Shawn.