Community
Participate
Working Groups
Observed: When Hunk.doPatch applies a deletion, it looks for a subsequence of lines within the fuzz range that match the before state of the hunk. Expected: Hunk.doPatch looks for a set of lines within the fuzz range that match the before state exactly. Consequence: If a hunk starts with an easily-matched line such as the deletion of a closing brace and the user is using a big fuzz factor, then the patcher could end up deleting any closing brace in the file - not necessarily the particular closing brace that is part of the intended hunk. I will attach a patch and some files that demonstrate the problem. Marking as critical due to user data loss.
Created attachment 73978 [details] File to be patched
Created attachment 73980 [details] Patch to apply
Created attachment 73981 [details] Expected result
Created attachment 73982 [details] Actual result
To reproduce: Copy "stuff.txt" into your workspace. Use Apply Patch... to apply "stuff_patch.txt" to "stuff.txt". The only hunk is off by one line (this is necessary to reproduce - the problem only occurs when using a fuzz factor). Click guess. Look at the preview pane. If everything works correctly, the patch should replace the lines "this,is,only,a,test" with the lines "this,might,be,a,monkey,test". Observed: The preview pane shows contents that look like "stuff_actual_result.txt" (attached). Notice that the first two lines of the hunk ("this is") are deleted from the start of the file, even though they appear again later contiguously with the rest of the deletions. Expected: The preview pane shows contents that look like "stuff_expected_result.txt" (attached). If there is an exact contiguous match for the before state of the hunk, the hunk should be applied there.
It would appear that Apply Patch is confusing fuzz and shift. For the *nix patch tool, shifting is done automatically whereas the maximim fuzz factor indicates the number of context lines to ignore (the default is 2). However, the algorithm tries to match using a fuzz of 0 first before attempting to match with a 1 (and so on). At each stage, shifting is tried as well before increasing the fuzz factor.
Regarding comment 6: That sounds like a perfect algorithm. I'd suggest reimplementing Apply Patch to work as you've just described, and remove the fuzz factor setting from the UI (the default of 2 should be good enough).
Please do not remove the fuzz factor setting. For patches with a lot of context it is required.
I agree that there is no need to remove the fuzz factor from the UI. We just need to implement it properly.
Actually... it would be useful if, rather than specifying the maximum number of context lines to remove, we could specify the minimum number of context lines to keep. That way people could generate patches with an excessive number of context lines, which the hunk editor could display in order to help people understand the purpose of the hunk being merged.
Targeting for M1 but since M1 is next week, I don't think we'll have it fixed for then. But we don;t have an M2 milestone yet.
Stefan, that's twice now that you have removed he target milestone when modifying the bug. If you don't want us to fix it in 3.4, you shouldn't have entered the bug in the first place ;-)
Created attachment 75849 [details] A testcase based on attachments from Stefan I added a test case based on files attached by Stefan. Actually, what I did is I created a test method which scans all subfolders in the "patchdata" folder and looks for files which can be used to run a test. This way, creating a new test case for the patcher is reduced to creating a folder with bunch of files named in a specified way. I hope you like the idea. If yes we could consider converting actual test methods to act this way (create a folder for each test method) in the future. But this is just an option.
Created attachment 75852 [details] Fix After I read all comments for the bug and took a deeper look at the patching mechanism I realized that this bug is only the tip of the iceberg. Nevertheless I managed to create a patch which fixes the reported bug. When shifting a hunk it makes sure that all lines considered for deletion are removed one by one, as one group. But still there is much more to be done, and I will log separate bugs for other issues related to the patching mechanism.
Does your fix implement the algorithm described in comment 6, or does it just address the issue of contiguous insertions / deletions?
The patch fixes only the problem from bug's description. I opened bug 199846 in order to address other issues related to the patching.
Thanks, Tomasz. I haven't tried the patch yet, but it sounds good from your description.
Patch released to HEAD
Reopening to consider for 3.3.1.
Patch released to 3.3.1
Verified in M20070913-0832
