Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 343538

Summary: Index not updated when complete folder deleted
Product: [Tools] CDT Reporter: Marko Tomljenovic <marko.tomljenovic>
Component: cdt-coreAssignee: Anton Leherbauer <aleherb+eclipse>
Status: RESOLVED FIXED QA Contact: Doug Schaefer <cdtdoug>
Severity: major    
Priority: P3 CC: aleherb+eclipse, yevshif
Version: 7.0.2Flags: aleherb+eclipse: review? (mschorn.eclipse)
Target Milestone: 8.0   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Attachments:
Description Flags
Plugin with context menu action that results in invalid index
none
Test project with src files to test contextmenuaction.zip with
none
Project indexer settings
none
DeltaAnalyzer to collect removed resources from resource delta
aleherb+eclipse: iplog-
Revised fix aleherb+eclipse: iplog-

Description Marko Tomljenovic CLA 2011-04-21 10:03:57 EDT 
Build Identifier: 7.0.2

When deleting a folder programmatically a little bit different (and not so efficient) than normal the index is not updated. I can still find the elements in the index (Open Element dlg) but after double clicking the file, cdt tells me that the file is not available any more.

Impl Hint: The deletion is done by using a workspace job that in sequence first forcibly deletes the files of the folder (recursively), then the folders are deleted forcibly. (see class NewAction.java)

Debug Hint:
The problem is that somehow that the CElementDelta is not properly created for the deleted folder. When debugging it tells me always that the removed folder has the default kind (changed) and not "removed".

Reproducible: Always

Steps to Reproduce:
1.Add plugin "Test" to your runtime eclipse
2.Import the given test project as is
3.Open (Resource) Navigator
4.Select folder "root"
5.Open context menu and execute "Delete files and folder"
6.Open "Open Element" dlg and search for "wort_buffer".
==> It is found in a .c file (a16.5.c) that has been deleted before
Comment 1 Marko Tomljenovic CLA 2011-04-21 10:04:47 EDT 
Created attachment 193825 [details]
Plugin with context menu action that results in invalid index
Comment 2 Marko Tomljenovic CLA 2011-04-21 10:05:17 EDT 
Created attachment 193827 [details]
Test project with src files to test contextmenuaction.zip with
Comment 3 Marko Tomljenovic CLA 2011-04-26 02:55:32 EDT 
Created attachment 194036 [details]
Project indexer settings
Comment 4 Marko Tomljenovic CLA 2011-04-26 02:57:37 EDT 
After some playing around it is simply enough to delete a folder that contains sources to reproduce the error.
Comment 5 Marko Tomljenovic CLA 2011-04-26 05:25:05 EDT 
The problem is that the DeltaAnalyzer is not handling removed CContainers at all which is the case when a complete folder is removed.
Comment 6 Marko Tomljenovic CLA 2011-04-26 05:59:40 EDT 
From what I understand till now there a two ways to solve this bug:

1. CElementDelta objects must be created for all files within the folder that have been deleted
2. Provide the resource delta for the deleted files within the folder so that the DeltaAnalyzer can determine PotentialTranslationUnits for the removed files
Comment 7 Marko Tomljenovic CLA 2011-04-26 09:30:09 EDT 
(In reply to comment #6)
> From what I understand till now there a two ways to solve this bug:
> 
> 1. CElementDelta objects must be created for all files within the folder that
> have been deleted
> 2. Provide the resource delta for the deleted files within the folder so that
> the DeltaAnalyzer can determine PotentialTranslationUnits for the removed files

I am following the first approach. The problem now could be located up to class 'CelementDelta' and method 'removed()' and 'addAffectedChild()'.

Why does method 'removed()' assume that a removed celement does not have any affected children (in case of removed folder it has affected children)

For removed files of a removed folder the method 'addAffectedChild()' does not add the c element delta of the removed file as an affected child of the c element delta of the removed folder.

Anybody an idea?
Comment 8 Anton Leherbauer CLA 2011-04-28 08:28:14 EDT 
Created attachment 194258 [details]
DeltaAnalyzer to collect removed resources from resource delta

I'd rather not change the way CModelDeltas are created.  Cutting off the deltas for removed and added resources is intentional and is the same in JDT world.  Changing that could cause regressions for existing clients.  Attached is a proposed fix for DeltaAnalyzer to always scan through resource deltas for removed resources.  This should not have a relevant impact on performance.
It would be great if you could contribute a JUnit test case for this bug!
Comment 9 Anton Leherbauer CLA 2011-04-28 09:24:49 EDT 
Created attachment 194267 [details]
Revised fix

I just noticed that the resource delta is not always part of the CElementDelta.  That was due to an omission in the DeltaProcesser.  This patch includes both fixes.
Comment 10 Marko Tomljenovic CLA 2011-04-29 08:55:56 EDT 
(In reply to comment #8)
> Created attachment 194258 [details]
> DeltaAnalyzer to collect removed resources from resource delta
> 
> I'd rather not change the way CModelDeltas are created.  Cutting off the deltas
> for removed and added resources is intentional and is the same in JDT world. 
> Changing that could cause regressions for existing clients.  Attached is a
> proposed fix for DeltaAnalyzer to always scan through resource deltas for
> removed resources.  This should not have a relevant impact on performance.
> It would be great if you could contribute a JUnit test case for this bug!

Are there any helper classes/plugins for writing cdt tests?
Comment 11 Marko Tomljenovic CLA 2011-04-29 09:33:15 EDT 
(In reply to comment #9)
> Created attachment 194267 [details]
> Revised fix
> 
> I just noticed that the resource delta is not always part of the CElementDelta.
>  That was due to an omission in the DeltaProcesser.  This patch includes both
> fixes.

Are you sure your patch is correct? After applying your changes on the 7.0.2 sources the bug still seem to exist!
Comment 12 Marko Tomljenovic CLA 2011-04-29 09:42:37 EDT 
(In reply to comment #11)
> (In reply to comment #9)
> > Created attachment 194267 [details] [details]
> > Revised fix
> > 
> > I just noticed that the resource delta is not always part of the CElementDelta.
> >  That was due to an omission in the DeltaProcesser.  This patch includes both
> > fixes.
> 
> Are you sure your patch is correct? After applying your changes on the 7.0.2
> sources the bug still seem to exist!

Does CElementDelta.addResourceDelta need not be changed too to allow resource deltas for removed c elements?
Comment 13 Anton Leherbauer CLA 2011-05-12 04:05:57 EDT 
(In reply to comment #10)
> Are there any helper classes/plugins for writing cdt tests?

Look into org.eclipse.cdt.core.tests.  There are numerous examples which you can use as boilerplate.

(In reply to comment #11)
> Are you sure your patch is correct? After applying your changes on the 7.0.2
> sources the bug still seem to exist!

I just tried with 7.0.2 and the patch works for me.  I followed exactly your steps to reproduce.

(In reply to comment #12)
> Does CElementDelta.addResourceDelta need not be changed too to allow resource
> deltas for removed c elements?

No, I don't think so.  There is still no resource delta for add or removed elements, but the parent container will have a resource delta.
Comment 14 Marko Tomljenovic CLA 2011-05-12 06:57:24 EDT 
(In reply to comment #13)
> (In reply to comment #10)
> > Are there any helper classes/plugins for writing cdt tests?
> 
> Look into org.eclipse.cdt.core.tests.  There are numerous examples which you
> can use as boilerplate.
> 
> (In reply to comment #11)
> > Are you sure your patch is correct? After applying your changes on the 7.0.2
> > sources the bug still seem to exist!
> 
> I just tried with 7.0.2 and the patch works for me.  I followed exactly your
> steps to reproduce.
> 
> (In reply to comment #12)
> > Does CElementDelta.addResourceDelta need not be changed too to allow resource
> > deltas for removed c elements?
> 
> No, I don't think so.  There is still no resource delta for add or removed
> elements, but the parent container will have a resource delta.

You are right, it works.
It will take some time to provide a test since I will be on leave for a month.
Comment 15 Anton Leherbauer CLA 2011-05-12 08:02:10 EDT 
(In reply to comment #14)
> You are right, it works.
> It will take some time to provide a test since I will be on leave for a month.

Thanks for the confirmation. I am going to create a test myself. A month from now is too close to release time.

Markus, would you review my changes in DeltaAnalyzer?
Comment 16 Markus Schorn CLA 2011-05-12 09:16:07 EDT 
(In reply to comment #15)
> (In reply to comment #14)
> > You are right, it works.
> > It will take some time to provide a test since I will be on leave for a month.
> Thanks for the confirmation. I am going to create a test myself. A month from
> now is too close to release time.
> Markus, would you review my changes in DeltaAnalyzer?

Looks good.
Comment 17 Anton Leherbauer CLA 2011-05-12 09:44:55 EDT 
Thanks Markus.
I have committed the fix together with test case IndexBugsTests.testUpdateOnFolderRemove_343538 to HEAD.