Community
Participate
Working Groups
Build Identifier: 4.2 feature request In our product, the copyright information is captured in the source code via a "public static final" field. The problem occurs when trying to detect changes between branches of the product. Often times, every single class is shown as changed because of the copyrights being updated. This makes the compare pretty much useless to detect *real* changes. If the synchronization view would allow me to select a difference, then choose to ignore all occurrences of that particular change, then I could see what has really changed. I would want the filtering to be on the outline tree on the left, not necessarily the text compare viewer for the file. Reproducible: Always
(In reply to comment #0) > In our product, the copyright information is captured in the source code via a > "public static final" field. How is it done for other file types (e.g. a properties file)? Do you also need support for those? > I would want the > filtering to be on the outline tree on the left, not necessarily the text > compare viewer for the file. You mean you'd want to filter the Synchronize view, right? Do you only need this for CVS or also other team providers? See also bug 26621. Maybe it would make sense to provide an extension point which allows to plug-in custom filters.
We use comments for properties files. Basically any file that requires copyright protection is affected. We use public static finals in the Java source files so that the .class files have the copyright information embedded. That said, the problem being solved is primarily about .java files. If the feature can cover comments in .properties files, that would be a bonus. We don't have a lot of properties files (so manually inspecting wouldn't be such a chore), and I have yet to encounter a situation where I cared about the difference between properties files between branches. I could see this being useful though; I personally haven't hit it though. Yes, by "outline" view, I really meant the Synchronize view. Sorry, that wording was not Eclipse friendly:) I was trying to differentiate between the higher-level Synchronize view and the low-level Java text compare viewer. I don't think the latter is necessary to have the filtering, and it may not make sense to do it there. We need it for CVS, as that is our source repository. From the discussions about this feature, I've been given the impression that this feature must be implemented per repository client. If it could be written in a more abstract way, that would be great too. Other areas of our product area use git.
This is certainly doable. Whether this is going to be implemented as a preference in Team, an extension point or something else, could be a matter to discuss. I just wonder if defining filters for files rather than actual changes is acceptable? In other words, are you fine with a filter like "exclude _files_ containing <your regex here>"? Which is slightly different then "exclude files with _changes_ containing ...". A filter like "exclude files with names matching <regex or pattern>" seems to be obvious.If the former is ok with you, what do you think about defining filters with contentTypes[1]? Another question is: are those filters meant to be global (workspace scope) or do you want to be able to specify which filters should be applied to specific projects? Filtering in the Compare Editor sounds like a separate bug to me. [1] http://help.eclipse.org/indigo/topic/org.eclipse.platform.doc.isv/reference/extension-points/org_eclipse_core_contenttype_contentTypes.html
(In reply to comment #3) > This is certainly doable. Whether this is going to be implemented as a > preference in Team, an extension point or something else, could be a matter to > discuss. I just wonder if defining filters for files rather than actual changes > is acceptable? In other words, are you fine with a filter like "exclude _files_ > containing <your regex here>"? Which is slightly different then "exclude files > with _changes_ containing ...". A filter like "exclude files with names > matching <regex or pattern>" seems to be obvious.If the former is ok with you, > what do you think about defining filters with contentTypes[1]? I don't think this helps in this case, but Grant can better clarify. > Filtering in the Compare Editor sounds like a separate bug to me. Right, this is not part of this bug.
Filtering based on file names or content types would not be useful in our case. We really want to filter .java files that have *only* the delta we identify. For example, let's say a base class is written as: package com.grant.taylor; public class MyClass { private static final String copyright = "Licensed Material - Property of Grant Taylor (C) Copyright Grant Taylor Corporation 2005, 2010 - All Rights Reserved. "; public boolean myMethod(String parm) { if(parm==null) return false; return true; } } If this copy of the file exists in a different branch, we want it to be filtered in the synchronize view because only the "copyright" field is different ("2010" is changed to "2011"): package com.grant.taylor; public class MyClass { private static final String copyright = "Licensed Material - Property of Grant Taylor (C) Copyright Grant Taylor Corporation 2005, 2011 - All Rights Reserved. "; public boolean myMethod(String parm) { if(parm==null) return false; return true; } } However, if the class were changed to this, then we would NOT want it filtered ("2010" is changed to "2011" and the body of myMethod is changed): package com.grant.taylor; public class MyClass { private static final String copyright = "Licensed Material - Property of Grant Taylor (C) Copyright Grant Taylor Corporation 2005, 2011 - All Rights Reserved. "; public boolean myMethod(String parm) { if("false".equals(parm)) return false; return true; } }
(In reply to comment #5) > Filtering based on file names or content types would not be useful in our case. > We really want to filter .java files that have *only* the delta we identify. Right, asking about filtering on file content only was rather silly. This means that we would need to calculate diffs for all files with a SyncInfo different than IN_SYNC. This is going to affect sync performance. What about being able to apply a filter to a subset of projects not the whole workspace. Is that needed?
> This means > that we would need to calculate diffs for all files with a SyncInfo different > than IN_SYNC. This is going to affect sync performance. Not necessarily: you could delegate this to the filter. The filter knows better on which files to do it and also knows whether it at all needs to check other versions on disk. Of course there would need to be some sort of API that lets the filter easily get the other version do the check/comparison.
I should have been more precise, I agree that diff calculation should be done in the filter (if necessary). This is what I meant in my comment, I just wanted to underline that the filter won't be consulted if the SyncInfo says "in sync".
(In reply to comment #8) > I just wanted > to underline that the filter won't be consulted if the SyncInfo says "in sync". Right, in that case the resource currently doesn't show up in the Sync view already.
> What about being able to apply a filter to a subset of projects not the whole > workspace. Is that needed? For our use case, the ability to apply the filtering per project isn't needed. We don't care about the copyright field changes regardless of where they exist. I can see the usefulness in having the ability to scope the filtering though.
(In reply to comment #10) > > What about being able to apply a filter to a subset of projects not the whole > > workspace. Is that needed? > For our use case, the ability to apply the filtering per project isn't needed. If we specified filters per project, the filters could be shareable. So I think it would be useful to choose the scope like we do for many other prefs e.g. in JDT. Important is whether we can come up with a solution that does not require changes in team providers. If we want to add this item to the plan, we should know how much work JGit/EGit guys need to do to consume the new feature. If I understand it well, the new feature working on CVS and Git is the minimum. Moreover as Tomek already mentioned, the performance of sync will be affected if we are going to consult the content. We already talked about some optimizations, but it will not reduce the perf impact to 0. We don't know how big the impact would be though.
I think Grant only needs it for CVS at the moment. I don't think the content should be inspected by Team. Rather it would delegate it to the contributed and eneabled filters. This would mean that out of the box there is no impact and only when someone enables a filter, then that filter would burn the cycles - as chosen by the user.
(In reply to comment #12) > I think Grant only needs it for CVS at the moment. It is not so obvious at the moment. I would wait for Grant. If he says only CVS, we will do just CVS. If he is not sure, we need to consider both. > I don't think the content should be inspected by Team. Rather it would delegate > it to the contributed and eneabled filters. This would mean that out of the box > there is no impact and only when someone enables a filter, then that filter > would burn the cycles - as chosen by the user. AFAIK during syncing somewhere, someone has to consult the filter. The question was if we are going to run this code only in Team or team providers should run the code that consults filters. In both cases the work will be done by filters. Of course that the performance will be affected only if the filtering is enabled, but that's obious, isn't it?
> Of course that the performance will be affected only if the filtering is > enabled, but that's obious, isn't it? It depends on how you craft the API :-)
If you asked me a week ago, I would say "only CVS". Now, it looks like we are moving to RTC/Jazz. We'll still be using CVS for our older releases, so we still need that covered. RTC/Jazz is a new requirement.
Created attachment 202509 [details] Fix v01 Here is an early bird version of the fix. The patch adds a preference for defining regex pattern to be used in synchronizations to exclude specific changes. In this particular case, pattern like ^private static final String copyright = "Licensed Material [– \w©]+ [\d]{4}, [\d]{4} – All Rights Reserved. ";$ should do the job. This is a functional fix, so any comments are welcome. If you spot anything you don't like or you're missing something and it is not mentioned below please let me know. Known drawbacks (work in progress): * changing pattern on the pref page should immediately update the synchronization * add missing doc, adjust wording/layout on the pref page * make the filter work for non-model synchronizations * fix all TODOs scattered around the code * add extension point for defining custom filter types, different then the regex filter <-- separate bug * allow multiple filters to be applied at the same time, add UI to turn selected filters on/off <-- separate bug * add scope filtering i.e. the ability to apply the filtering per project <-- separate bug Grant, I would be grateful if you could give it a try and share your opinion about it. I guess the most annoying part is the first bullet from the list above. Do you agree that last three points can be separate bugs? I haven't tried with EGit or any SCM other than CVS yet.
Created attachment 202510 [details] mylyn/context/zip
So far I've spent couple of days on this; moving to M3 to buy some time to address bullets from comment 16 and allow Grant to try the proposed fix.
Created attachment 204523 [details] Fix v02 (In reply to comment #16) > Known drawbacks (work in progress): > * changing pattern on the pref page should immediately update the sync > * make the filter work for non-model synchronizations Addressed by the fix. Still working on the others.
(In reply to comment #19) > Created attachment 204523 [details] > Fix v02 > > (In reply to comment #16) > > Known drawbacks (work in progress): > > * changing pattern on the pref page should immediately update the sync > > * make the filter work for non-model synchronizations > > Addressed by the fix. Still working on the others. Grant, could you verify that the fix attached to the bug is what you need?
Created attachment 205429 [details] Fix v02 applied and exported as jars 1/3 (In reply to comment #20) > Grant, could you verify that the fix attached to the bug is what you need? Normally, I would recomend self-hosting to check the patch, but Grant asked me to prepare set of jars. As for now, 3 projects are modified: team.core, team.cvs.ui and team.ui. The newer build you use to test them the better. I couldn't send it as a single zip, since there is 2MB limit for attachments, sorry.
Created attachment 205430 [details] Fix v02 applied and exported as jars 2/3
Created attachment 205431 [details] Fix v02 applied and exported as jars 3/3
Grant, we still can't release the fix to the repo due to lack of your review. Could you look at it so we can continue and release early in M4, please?
Sorry, I was planning to do this today... I swear :)
Created attachment 207138 [details] Fix v03 I had an offline chat with Grant and here is the result of some of his suggestions applied: * The regex is now being honored not only in synchronizations but also comparisons. So it will work for "Team > Synchronize", "Compare with > Latest from HEAD" and other "Compare with"s. * The filter is *not* applied, the diff is *shown*, if there is at least one difference (result of content comparison) for which neither left nor right side matches the regex. In other words, the diff is hidden if the regex matches left *or* right side for all of its differences. * To disable the filter, simply clear the regex. * The preference can be accessed with the Sync view's drop-down menu. Open the menu and select "Preferences...". To be honest, I haven't done anythng with that, just making sure we all know this shortcut. * Grant suggested an action like "ignore changes like this" in the Sync view which sounds like a great idea, but I definetly won't make it for M4 (a seperate bug, enhancement?) * I moved the regex pref to the Team > CVS > Compare/Synchronize page * Since the fix bounds the regex and the "Only use timestamps" prefs, in way the former is only enabled when the latter is unchecked, contents are considered now in all synchornizations/comparisons. This is what bug 315747 was about, but at that time we decided to ignore the pref in synchronizations. This doesn't make much sense now, since we fetch the content anyway to find diffs and match them with the regex pattern. * I will open a separate bug for updateing the doc and schedule it for M7. * Last, but not least, the filter is not applied when restoring synchronizations. Synchronizations, in contrast to comparisons, are persistable but the prefs are not re-applied. This sounds like a separate bug to me.
Created attachment 207139 [details] mylyn/context/zip
(In reply to comment #26) > * I will open a separate bug for updateing the doc and schedule it for M7. +1 > * Last, but not least, the filter is not applied when restoring > synchronizations. Synchronizations, in contrast to comparisons, are persistable > but the prefs are not re-applied. This sounds like a separate bug to me. +1 I'm also suggesting to indent the label for the regex. Other than that I think it is fine to release the patch and address the issues above as separate bugs.
Created attachment 207213 [details] Fix v04 Added the indent and Pattern.DOTALL flag, so matching against multiple lines got easier.
(In reply to comment #26) > * I will open a separate bug for updating the doc and schedule it for M7. => bug 364142 > * Last, but not least, the filter is not applied when restoring > synchronizations. Synchronizations, in contrast to comparisons, are persistable > but the prefs are not re-applied. => bug 364143
Fixed with 951477fe0d9d1aa4bbfcabd936d0d7641c6f712a. Available in builds >= N20111118-2000. Tip: for the case from comment 5 you can now use a regex like this: private static final String copyright =.*[0-9]{4}, [0-9]{4}.*
