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

["Shawn O. Pearce" <spearce@spearce.org>]

Florian Koeberle <florianskarten@web.de> wrote:
> @@ -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.

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

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