Re: [JGIT PATCH v2 14/24] Added the class IgnoreRuleListFactory.

From: Shawn O. Pearce <spearce@spearce.org>
Date: May 13, 2008, 01:08 UTC
Message-ID: <20080513010844.GE29038@spearce.org>
In-Reply-To: <1210623222-24908-15-git-send-email-florianskarten@web.de>

Florian Koeberle <florianskarten@web.de> wrote:

Show 37 quoted lines

> +/**
> + * This class can be used to create lists of {@link Rule} objects from lines of
> + * .gitignore like files.
> + * 
> + */
> +class IgnoreRuleListFactory {
> +
> +	List<Rule> createIgnoreRuleList(Iterable<String> lineIterable) {
> +		LinkedList<Rule> rules = new LinkedList<Rule>();
> +		for (String line : lineIterable) {
> +			final String trimmedLine = line.trim();
> +			if (trimmedLine.startsWith("#")) {
> +				continue;
> +			}
> +			if (trimmedLine.length() == 0) {
> +				continue;
> +			}
> +			rules.add(0, createRule(trimmedLine));
> +		}
> +		return rules;
> +	}
> +
> +	List<Rule> createIgnoreRuleList(List<File> files)
> +			throws FileNotFoundException {
> +		final List<String> lines = new ArrayList<String>();
> +		for (File file : files) {
> +			Scanner scanner = new Scanner(file);
> +			try {
> +				while (scanner.hasNextLine()) {
> +					lines.add(scanner.nextLine());
> +				}
> +			} finally {
> +				scanner.close();
> +			}
> +		}
> +		return createIgnoreRuleList(lines);
> +	}

Why go through all this work to buffer the lines we don't care about (starting with # or are blank) when we could just discard them in the inside of createIgnoreRuleList and then create the rule right away?

Show 10 quoted lines

> +	private Rule createRule(String trimmedLine) {
> +		final boolean exclude;
> +		String patternString;
> +		if (trimmedLine.startsWith("!")) {
> +			exclude = false;
> +			patternString = trimmedLine.substring(1);
> +		} else {
> +			exclude = true;
> +			patternString = trimmedLine;
> +		}

I suspect this code would be easier to follow if you just accepted changing the method parameter, such as:

	private Rule createRule(String pattern) {
		boolean exclude = true;
		if (pattern.startsWith("!)) {
			pattern = pattern.substring(1);
			exclude = false;
		}

Show 14 quoted lines

> +		final boolean matchDirectoriesOnly;
> +		if (patternString.endsWith("/")) {
> +			matchDirectoriesOnly = true;
> +			patternString = patternString.substring(0,
> +					patternString.length() - 1);
> +		} else {
> +			matchDirectoriesOnly = false;
> +		}
> +
> +		final FilePattern pattern;
> +		if (patternString.contains("/")) {
> +			if (patternString.startsWith("/")) {
> +				patternString = patternString.substring(1);
> +			}

"foo/bar" will always end up in this code-path and will not match in all levels of the tree if I follow your code correctly.

An ignore rule in the top level of "foo/bar" should ignore any entry named "bar" within a directory "foo" at any level of the tree, even if it is 35 directories down from the root. Isn't ComplexFilePattern about the absolute (starts with "/") cases only?

Show 7 quoted lines

> +			final StringTokenizer stringTokenizer = new StringTokenizer(
> +					patternString, "/");
> +			final List<String> patternList = new ArrayList<String>();
> +			while (stringTokenizer.hasMoreTokens()) {
> +				final String token = stringTokenizer.nextToken();
> +				patternList.add(token);
> +			}

StringTokenizer is more-or-less replaced by String.split("/"), with the split method being the more preferred method of doing this.

-- 
Shawn.