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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Shawn O. Pearce wrote:
Florian Koeberle <florianskarten@xxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux