Shawn O. Pearce wrote: > 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. > ack. > 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). ok > > Style nit: Don't put /* Constructors */, /* Methods */ or > / * Public Methods */ comments in code, e.g. > IgnoreProjectCache l.52-54 or GitIgnoreData l.58-61. > ok > 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. > ok > 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. > ok > 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. > when we convert the code to be in jgit we will not use this api. this will disappear. > 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. > ok > > 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. > ok. but I need much more input than that from you on how to go about moving the code into jgit. If you have ideas on what the rough architecture should be of the ignore processing in jgit then please let me know, I don't know that code at all and also don't know the approach you'd like to take there with its architecture. I've been thinking on and off about how to free the ignore processing from eclipse specific apis ever since the egit/jgit issue came up. I think it's not that hard to do, depending on the approach you'd like to take. you talk about TreeWalk. but what is it's prupose, in what context is it called? etc. You can probably much faster explain that to me than I can find out from the code (the egit code is much easier to place in context than the jgit code I think) > 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. > the way it works now is more efficient than git itself (most of the time), since we have a cache of the ignore patterns. I compared traces of git itself and the plugin against eachother. > > OK, that's about all I have for now. Its reasonable, but still an > early series. > yep. please let's discuss moving it into jgit asap. so that I can complete the feature. BTW In the meantime I've also added ignore preferences to the plugin on which the user can specify whether the Eclipse Team Ignored Resources should be taken into account. It defaults to yes. When set to no the ignore processing complies strict to git behaviour. looking forward to your input! Ferry -- 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