Bugzilla – Full Text Bug Listing
| Summary: | [External Settings Provider] Settings lost after changing language IDs | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Christian Walther <walther> | ||||||||||||||
| Component: | cdt-core | Assignee: | Christian Walther <walther> | ||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Doug Schaefer <cdtdoug> | ||||||||||||||
| Severity: | normal | ||||||||||||||||
| Priority: | P3 | CC: | jamesblackburn+eclipse, jonah | ||||||||||||||
| Version: | 7.0.1 | ||||||||||||||||
| Target Milestone: | 9.6.0 | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| See Also: |
https://git.eclipse.org/r/133773 https://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=f2f92ab4045802da83d2efeae9548a4fd544becb |
||||||||||||||||
| Whiteboard: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
Created attachment 187543 [details]
extsettings-unittest.patch
Created attachment 187544 [details]
extsettings-fix.patch
Created attachment 187545 [details]
extsettings-deque.patch
It looks like you've just cat'd the patches together, so they don't apply in Eclipse. Can you provide a workspace patch? Also can you update the copyright headers of any files you touch? Hmm, I generated the patches with the following commands, according to the hint on http://wiki.eclipse.org/CDT/Developer/FAQ#Git : walthermac:~/X/projects/org.eclipse.cdt cwalther$ git diff --no-prefix --relative=all -b HEAD^^^ HEAD^^ -- > extsettings-unittest.patch walthermac:~/X/projects/org.eclipse.cdt cwalther$ git diff --no-prefix --relative=all -b HEAD^^ HEAD^ -- > extsettings-fix.patch walthermac:~/X/projects/org.eclipse.cdt cwalther$ git diff --no-prefix --relative=all -b HEAD^ HEAD -- > extsettings-deque.patch I'll try applying them in Eclipse to see what's wrong. Will do about the copyright header - I wasn’t sure if that’s the responsibility of the committer. Thanks for the quick reaction! Created attachment 187622 [details]
extsettings-unittest-2.patch
Alright, here's a new set of patches, with adjusted copyright headers. I chose to consider the added unit test a "significant contribution" and the bugfix not, but I leave the final decision on that to you.
I had no problems applying the original patches in Eclipse, to both cdt_7_0 and origin (right-click on any project, Team > Apply Patch, load patch from file, apply to workspace root), so I'm not sure what the problem is - what errors do you get? I'm not sure what you mean by "cat'd together", is it a problem that extsettings-unittest.patch contains changes to two files? I didn't "cat" anything manually, but it seems normal to me that a multi-file patch is the concatenation of the respective single-file patches.
My workspace setup is that the relevant projects "org.eclipse.cdt.core" and "org.eclipse.cdt.core.tests" in my workspace come from the "all" subfolder of the Git checkout (I have all projects from the "all" folder and none from any others), is anything wrong with that?
Is there any confusion about the order of the patches? -unittest and -fix are independent of each other and can be applied in any order (I recommend -unittest first so you can see the test failing), -deque (if desired) needs to come after -fix.
I tried to generate the new patches in Eclipse using "Create Patch" in the contextual menu of the commit in the History view, but that resulted in patches that omitted the project name at the beginning of paths (with "Export in git patch format" unchecked, otherwise they start with "a/all/"), which seems wrong to me if the patches are to be applied to the whole workspace. So, these patches are from "git diff" on the command line again (with the same options as above).
Created attachment 187623 [details]
extsettings-fix-2.patch
Created attachment 187624 [details]
extsettings-deque-2.patch
Couple points:
- Deque & ArrayDeque only appeared in Java 6. CDT builds against Java 5,
- Your name should be added to any files you touch, irrespective of the size of the contribution.
http://www.eclipse.org/legal/cpl2epl/cpl_copyrightandlicensenotice.php
- Don't worry too much about splitting simple changes into two patches -- it's pretty clear that a queue is a better data structure for pushing / popping from both ends.
Thanks again for the contribution and keep the patches and tests coming :)
*** cdt cvs genie on behalf of jblackburn *** Bug 335344 - [External Settings Provider] Settings lost after changing language IDs [*] CExternalSettinsDeltaCalculator.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.core/model/org/eclipse/cdt/internal/core/settings/model/CExternalSettinsDeltaCalculator.java?root=Tools_Project&r1=1.5&r2=1.6 [*] ExternalSettingsProviderTests.java 1.9 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.core.tests/model/org/eclipse/cdt/core/settings/model/ExternalSettingsProviderTests.java?root=Tools_Project&r1=1.8&r2=1.9 [*] TestExtSettingsProvider.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.core.tests/model/org/eclipse/cdt/core/settings/model/TestExtSettingsProvider.java?root=Tools_Project&r1=1.4&r2=1.5 Thanks! Good point about Java 5, I'll try to keep that in mind in the future. Regarding the contributor name/"significant contribution" idea, I got that from <http://wiki.eclipse.org/CDT/policy#Copyright>: "Normally contributors line is added for significant changes in the file." Perhaps that could be amended if it doesn't conform with current policy (or "significant" is too vague a term). I will keep them coming if you guys keep the bugs coming! ;) It turns out that my 8-years-old fix only fixed half of the problem, and I have now happened on the second half. I didn’t even remember that I had worked on this and was surprised to see my name at the top of the files… The fix worked for the case where a complete CExternalSetting was removed and replaced by a different one, but not in the case where individual entries from a CExternalSetting were moved to a different one, but others remained (and, in both cases, the two CExternalSettings applied to the same ICLanguageSetting). I am putting a new unit test that tests this condition as well as a new fix that replaces the old one on Gerrit. (The old fix could also be left in, but it is made redundant by the new one.) New Gerrit change created: https://git.eclipse.org/r/133773 (I'm not sure about reopening a very old bug vs submitting a new one.) I have set a target milestone of 9.6.0, which means *today*. I'll review it and if it is low risk and needs no changes I'll merge today, otherwise it will have to wait until 9.6.1 or 9.7.0. Thanks for the fix. Gerrit change https://git.eclipse.org/r/133773 was merged to [master]. Commit: http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=f2f92ab4045802da83d2efeae9548a4fd544becb Thanks Jonah, you are very quick! I didn’t mean to put any stress on you, if I had remembered that you were planning to branch today I would have waited until after that. I was on the fence about both of the things you mentioned myself. I chose to reopen the old bug since it was mine to begin with and was really about the same issue, so it seemed the easiest way of keeping all the discussion cross-referenced. And I added the contributor line mainly for consistency with the other files that already had it, which saved me the bother of looking up whether it was still required - thanks for the confirmation. |
Build Identifier: M20100909-0800 When a setting provided by an external settings provider changes the set of language IDs it applies to, the setting disappears from all languages in the intersection of the old and new set. Steps to reproduce: 1. Let an external settings provider provide a CExternalSetting with arbitrary content (e.g. one CMacroEntry) that applies to language ID "a" (e.g. "org.eclipse.cdt.core.assembly", "org.eclipse.cdt.core.gcc"). 2. Check in Project Properties > C/C++ General > Paths and Settings that the setting is indeed applied to language "a". 3. Change the external settings provider to provide the same setting, but now for languages {"a", "b"}, and update it. 4. Check again for which lanugages the setting appears in Paths and Settings. Expected result: The setting should appear for languages "a" and "b". Actual result: The setting appears for language "b" but has disappeared for "a". Attached patch "extsettings-unittest.patch" adds a unit test that performs the above steps, and therefore currently fails. The cause for this misbehavior is that this external setting change is modeled by the CExternalSettinsDeltaCalculator as removal of the setting for the old languages ({"a"}) and addition for the new languages ({"a", "b"}), but CExternalSettinsDeltaCalculator.getSettingChange() returns these in the order (addition, removal). CExternalSettingsDeltaProcessor.applyDelta(ICLanguageSetting, ExtSettingsDelta[], int) will then apply them in the same order and therefore add and immediately remove the setting again for lanugage "a", leaving it only on language "b". To fix this, CExternalSettinsDeltaCalculator.getSettingChange() should always return removals before additions. This is what the attached patch "extsettings-fix.patch" does. On top of that, "extsettings-deque.patch" might provide a small performance improvement by replacing the ArrayList that is now appended to from both ends by an ArrayDeque (I didn't measure it though). Patches are against revision b1c81d4b5b57826b17c2abf84d5b3dcdf1532dd0 of the cdt_7_0 branch of git://dev.eclipse.org/org.eclipse.cdt/org.eclipse.cdt.git, because I wanted to avoid the hassle of installing Eclipse 3.7, but should apply to HEAD (origin) as well. Reproducible: Always