Community
Participate
Working Groups
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.
Created attachment 188801 [details] Removes redundant files and folders
Created attachment 188802 [details] Enhanced test case for verifying the bug fix Test case
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
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.
(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?
I've added the review flag for Pascal.
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).
Pascal, Henrik, thank you for this quick resolution.
