Re: [JGIT PATCH v3 0/23] Implementation of a file tree iteration using ignore rules.

Florian Koeberle <florianskarten@web.de> wrote:

I finally found time while travelling around on planes this past week to review this series. Since I didn't have access to my email client I didn't organize my thoughts per-patch very well, if it all.

I agree with all of Robin's comments. He noticed a few things I didn't, but they all made sense once I read them.

Without further ado, here are my remarks on this current series:

---- Please consider using a cleanup commit to:

- Rename FileNameMatcher.extendStringToMatch to just append().
  The action used here is generally called "append" in Java, see
  StringBuffer for example.

- Rename FileNameMatcher.resetStringToMatch to just reset().  It
  is cleaning up the state and the only state it can cleanup is
  the string we are matching against.

- Rename FileNameMatcher.willExtendResultInNoMatch() to say
  canAppendMatch() with the negation in the callers of the
  current willExtendResultInNoMatch() method.  I think its
  easier to read as "!canAppendMatch()".

Ecipse can easily handle the refactoring and its probably easier to just do at the end of the series rather than fixing every commit along the way.

---- I'm not sure I see why we have FilePattern.isSameForSubdirectories(), or Rule.isSameForSubDirectories(). By the end of the series you do not call either method. These should be removed from the abstract definition and from all implementing classes. Again a cleanup commit to remove this would be OK.

---- Class Rule should not be public; none of its members are public.

FileNameMatcher's Head.getNextHeads and AbstractHead.setNewHeads should not be public either; the interfaces are visible only to this package.

In general please make method access reflect the access of the class the method is declared within. If the class is not a public class then the method isn't visible, even if it is marked public, so instead mark it package-level.

---- Since FilePattern is not public none of its members should be public; use default package access for the MATCH_ALWAYS and MATCH_NEVER constants and private access for the static inner classes that supply those two implementations.

---- In FileNameMatcher the symbol name invalidWildgetCharacter is used as a parameter to a number of methods/constructors. Its name does not mean anything to me as I don't know what "Wildget" means. In many cases this long name is causing Eclipse to wrap the code poorly. Rename it to notInWild perhaps?

In FileNameMatcher.extendStringToMatchByOneCharacter you don't need to use an int index into the List<Head> called "heads". Instead use the Java 5 syntax of "for (final Head head : heads)" and let the compiler generate the loop. If the memory allocation of the Iterator is too costly here then you probably need to rethink your storage anyway. You could have getNextHeads return you an Head[] instead of List<Head>. The compiler can loop over the array and an array takes up less memory than an ArrayList of the same size.

In FileNameMatcher.extractGroupPatternWithoutBrackets your string literals for the pattern argument of String.format() are way too long and cause them to overflow the line. Break the string into multiple literals and use + to append them together. Any decent Java compiler will combine them back into a single string in the class string pool. Even if the compiler sucks, this is for an error condition only. Who cares how long the StringBuilder is going to take at that point, the operation has failed.

---- Don't create a local variable just to return it. You do this at least in FileNameMatcher.extractGroupPatternWithoutBrackets:

	final String groupPatternWithOutBrackets = patternString.substring(
		start + 1, closingBracketIndex);
	return groupPatternWithOutBrackets;

and it looks more complex than is really needed here:

	return patternString.substring(start + 1, closingBracketIndex);

This is also true in createMatcherForSuffix. You can reduce that entire method to just:

	return new FileNameMatcher(new ArrayList<Head>(heads));

which is much easier to read than the 3 lines you have to create the copyOfHeads.

---- Please use a for loop where it makes sense. In the case of FileNameMatcher.extractGroupPatternWithoutBrackets you use a while loop with a variable called "start", with a post condition to decrement the variable. That's why we have the for(;;) loop construct in the language.

But this entire method is 30 lines of code to say this:

	final int ob = pattern.lastIndexOf('[', closingBracket - 1);
	final int cb = pattern.lastIndexOf(']', closingBracket - 1);
	if (ob < 0 || cb > ob)
		throw new InvalidPatternException("Unbalanced []", pattern);
	return pattern.substring(ob + 1, closingBracket);

---- In FileNameMatcher the isMatch method is _way_ more complex than it needs to be. Just use:

	public boolean isMatch() {
		return heads.contains(LastHead.INSTANCE);
	}

contains works perfectly well. From what I can tell of the implementation the size of heads should be fairly small, usually only 1 or 2 items at most. Testing via contains without using a backward traversing ListIterator is just fine here.

---- Please don't initialize fields to null. For example in the AbstractHead class of FileNameMatcher you set newHeads = null; the JVM does this for you automatically and that makes your own statement redundant.

---- I'm not sure I see the value in the Head interface within FileNameMatcher. Only two implementations exist; the one for AbstractHead and the one for LastHead. Since LastHead is already just a singleton you might as well just have it extend from the AbstractHead base class and "waste" the couple of words of static memory for an empty head list.

---- In FilePathPattern you use "new Character('/')". Instead use the flyweight one that comes with the JRE by "Character.valueOf('/')".

---- In general the implementation still feels overly complex to me. Lots of interfaces, factories and delegation for what should be a relatively simple thing of pruning names that should be skipped.

It also seems to be somewhat special cased around git-add sort of behavior. We also need to process ignore lists for merge, to make sure we can safely overwrite something or not if there is an untracked file in the working tree that conflicts with what we are merging the working directory to.