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

Bug 336986

Summary:

[native] Enable removal of redundant files and folders by RemoveAction

Product:

[Eclipse Project] Equinox

Reporter:

Katya Stoycheva <katya.stoycheva>

Component:

Assignee:

Henrik Lindberg <henrik.lindberg>

Status:

CLOSED FIXED

QA Contact:

Severity:

critical

Priority:

CC:

henrik.lindberg, kaloyan, pascal, s.yousouf

Version:

unspecified

Target Milestone:

3.7 M7

Hardware:

OS:

Windows XP

Whiteboard:

Attachments:

Description	Flags
Removes redundant files and folders	pascal: iplog+
Enhanced test case for verifying the bug fix	pascal: iplog+

Description Katya Stoycheva

2011-02-11 13:10:58 EST

Build Identifier: I20110127-2034

If executed several times over one and the same folder or file, RemoveAction fails poorly. For example, this can happen in productive scenarios during provisioning of  IUs which are published with specific touchpoint instructions. The instructions are to unzip some files to one and the same temporary folder, process them and remove the temporary folder.

The current implementation of removal relies on moving the files/folders to a backup location. Deletions are not performed if the files are found to be already backed up. As a result, bundles which unzip to and remove one and the same folder, lead to the following behavior:
 - Remove action for the first bundle is successful
 - Remove action for the second bundle leaves a dummy file in the folder for deletion but reports that the operation is successful
 - Remove action for the second bundle leaves a dummy file in the folder for deletion, tries to delete the folder, but fails because the folder is not empty.

The proposed patch modifies the backup store to take care to remove the files, no matter they have been previously backed up or not.

Reproducible: Always

Steps to Reproduce:
Run the attached RemoveActionTest (enhanced with new test methods). Only method "testExecuteUndo()" must pass successfully.

Comment 1 Katya Stoycheva

2011-02-11 13:11:58 EST

Created attachment 188801 [details]
Removes redundant files and folders

Comment 2 Katya Stoycheva

2011-02-11 13:13:02 EST

Created attachment 188802 [details]
Enhanced test case for verifying the bug fix

Test case

Comment 3 Kaloyan Raev

2011-04-08 12:43:45 EDT

Dear p2 committers, 

As our adopter's product grows in complexity, this bug has now a serious impact on it - it blocks a very important user scenario. 

Please, consider reviewing the attached patch soon. We need the fix in the Indigo code line. 

I am increasing the severity of the bug to Critical to reflect the impact on the adopter's product. 

Thanks,
Kaloyan

Comment 4 Henrik Lindberg

2011-04-08 17:14:17 EDT

Thanks for the patch and test cases Katya.

I wrote the original code, and I did (obviously :/)  not anticipate this use case.

I agree that the added functionality is important, and that the way this functionality is implemented also is ok - i.e do not write new things to backup if already backed up, but perform the delete.

I see no harm in accepting this change for those that are not deleting the same thing multiple times. 
I can't imagine that anyone else relies on the fact that it fails if trying to delete same file multiple times.

I have reviewed the diff and the changes look ok, but I have not built and tested the code.

I am sorry, but I am not in a position to do the work of testing and committing this as I am not currently working on p2 code.  Also, the the backup code in general is sensitive to differences in the underlying platform (although the changes in this patch should be ok), and since it is may be late in the Indigo cycle there may be reluctance to take on a change in this code. 

I am adding a review request for Pascal.

Comment 5 Henrik Lindberg

2011-04-08 17:17:34 EDT

(In reply to comment #4)
> 
> I am adding a review request for Pascal.
I tried adding "review ? pascal@sonatype.com" but that did not work, but I see you are cc:ed. Pascal, have you seen this?

Comment 6 Kaloyan Raev

2011-04-09 10:53:52 EDT

I've added the review flag for Pascal.

Comment 7 Pascal Rapicault

2011-04-11 19:27:05 EDT

I've released this patch. Thx for the contrib, sorry it took so long.
For further reference, and in hope to smoothen the process and minimize the frustration, adding me for review is pointless as I'm not checking my p2 related bugzilla mail everyday. You will have a more success emailing me directly, reaching out the p2-dev ML directly or mention the bug on the p2 / equinox calls (which Katya did).

Comment 8 Kaloyan Raev

2011-04-12 05:32:55 EDT

Pascal, Henrik, thank you for this quick resolution.