Ferry Huberts <ferry.huberts@xxxxxxxxxx> wrote: > This is the first - early - code that adds ignore functionality to EGit. > Currently it reads in all ignore patterns upon workspace startup into an > ignore cache. From this cache the ignore state of a resource is evaluated > in the same fashion as git does. > > The code does not yet react to changes in ignore files but I'm planning to add > that soon and I can share a lot of code for that. > > I send this code to receive feedback and to give you insight into what I'm > doing with it. I'm new both to EGit programming and Eclipse programming so > there might be things that could be done more elegantly :-) Ok, I finally got a chance to review this series. We really want as much of the Git specific logic as we can in JGit under the BSD license. This has already been raised elsewhere in this thread. JGit and EGit are holding the line on Java 5 support; that means that String.isEmpty() must be spelled as String.length() == 0 (isEmpty was added in Java 6). Style nit: Don't put /* Constructors */, /* Methods */ or / * Public Methods */ comments in code, e.g. IgnoreProjectCache l.52-54 or GitIgnoreData l.58-61. Style nit: Don't assign fields to their default values. E.g. Exclude.java l.25,33,42,.. these are being set to the same value that the JRE sets the field to if the field is not explicitly initialized. We find it much easier to read code when the defaults are assumed. Style nit: Don't use "this." to refer to members. Your IDE should highlight field references differently than parameters, and a parameter should never shadow a field name, thus "this." is unnecessary and makes the code much more verbose to read. E.g. see Exclude.java 's constructor on l.87-108; I can't see the forest (the code) due to all the trees (this.) appearing. IgnoreFileOutside: Ugh, our own implementation of IFile ? I'm worried about the long-term stability of the IFile API. Is it really frozen enough that we can implement it ourselves? Of course, this may be moot if much of the code was moved back to JGit. IgnoreRepositoryCache: Why not put this into RepositoryMapping? Instead of caching it inside a static HashMap of GitIgnoreData, wouldn't it be better to put it into RepositoryMapping? The TrackOperation for example already has the RepositoryMapping handle in scope, saving a few lookup operations, and avoiding needing to manage this new additional static HashMap against leaks. I kind of wanted to tie exclude processing (and attribute processing) into a TreeWalk, so that we can do an n-way merge against trees and working directories by tossing all of their AbstractTreeIterators into a single walk, possibly apply a path filter, and let the walk handle the per-directory ignore rules as it goes. Most of your code seems to be built around the Eclipse IResource model, and the idea that it gets called for a single file path at a time, which may make it less efficient when we put it into a TreeWalk and apply the notion of entering and exiting a subdirectory. OK, that's about all I have for now. Its reasonable, but still an early series. -- Shawn. -- 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
