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

["Florian Köberle" <FloriansKarten@web.de>]

Shawn O. Pearce wrote:
> Florian Koeberle <florianskarten@web.de> wrote:
>> +/**
>> + * 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?
I did this to reduce complexity and to increase modularity.
The method createIgnoreRuleList(Iterable<String> lineIterable) can now 
be tested without the need to create files. Also it is so possible to 
read the patterns from different sources.

If you worry about memory usuage I could create some kind of 
RulesBuilder class:

public class RulesBuilder {
	private final List<Rule> rules;
	private RuleListToObjectConverter converter;

	// method used by tests:
	public void addIgnoreRule(String ignoreRuleLine) {
		
	}
  	public void addIgnoreRuleFile(File) {
		// reads file and calls addIgnoreRule
	}

	public void addIncludeRuleOfAddCommand(String pattern) {
	}

	public void addIgnoreGitDirectoryRule() {
	}

	public Rules buildRules() {
		return converter.convertToObject(rules);
	}
}

This would have the advantages:
* all Rule creating code at one place.
* There is no need to keep the original lines in memory.
* It's easy to create rules for testing purposes.
* It's easy to create Factories like AddRulesFactory.

> 
>> +	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;
> 		}
If I remember correctly Intellij IDEA marks per default cases where you 
change a parameter. So it looks like that there are style guides which 
are against the practice of changing the values of parameters.

That and the fact that you can't make exclude final now where the reason 
why I wrote it the otherway.
> 
>> +		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?
Yes, ComplexFilePattern is about that cases only.

I implemented it that way, becasue git behave the same way:
* create the follwing file: a/a/b/test.txt
* add the line "a/b" to .gitignore
* add a and notice that it adds the test.txt file.

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