Community
Participate
Working Groups
Enhancement request: Provide API to apply a set of IHunks to a file without going through IFilePatch.apply. Some usages of this would be: - I already have a UI that lets the user manipulate unmatched hunks. I'd like to write an action that lets them try re-applying a hunk. For example, they might use this if the file state has changed and they now believe the hunk will apply without problems. In this case, I would have already made an attempt to apply the hunk via IFilePatch.apply and I'm now trying to reapply the same hunk to different file contents. - I'd like my UI for resolving hunks to attempt to reapply hunks whenever the file changes on disk. For example, if the user merges in one hunk, this may supply the context lines that are needed to make another hunk apply successfully. I have no immediate plans for this, but it would be cool to be able to autodetect this sort of thing. - I want to write a preview pane that lets the user look at what is inside a patch before they apply it. I'd like that preview pane to have actions that let the user selectively apply the hunks. In this case, I want to do basically the same thing that IFilePatch.apply is doing, but I want to do it one hunk at a time. - Assuming we eventually get API for instantiating IHunks, I may have some programmatically-created hunks that I want to apply to a file.
Suggested API: /** * Attempts to apply the given hunks to the given input stream. */ public static IFilePatchResult applyPatch(IStorage contents, IHunk[] hunksToApply, IProgressMonitor progress);
Since it involves API changes, it will not be fixed for 3.4. Reassigning to 3.5 for further investigation.
(In reply to comment #0) > Enhancement request: > > Provide API to apply a set of IHunks to a file without going through > IFilePatch.apply. > > Some usages of this would be: > - I already have a UI that lets the user manipulate unmatched hunks. I'd like > to write an action that lets them try re-applying a hunk. For example, they > might use this if the file state has changed and they now believe the hunk will > apply without problems. In this case, I would have already made an attempt to > apply the hunk via IFilePatch.apply and I'm now trying to reapply the same hunk > to different file contents. > > - I'd like my UI for resolving hunks to attempt to reapply hunks whenever the > file changes on disk. For example, if the user merges in one hunk, this may > supply the context lines that are needed to make another hunk apply > successfully. I have no immediate plans for this, but it would be cool to be > able to autodetect this sort of thing. > I have to say I don't know when it could help automate the patch applying process. Of course one could create a hacked patch that could be applied in this sequential way. But with regular patches this seems impossible. All I can find is a situation where the hank context supplied by a previous hunk is accidentally the same as the original context (do we want to apply the hank in that case?). > - I want to write a preview pane that lets the user look at what is inside a > patch before they apply it. I'd like that preview pane to have actions that let > the user selectively apply the hunks. In this case, I want to do basically the > same thing that IFilePatch.apply is doing, but I want to do it one hunk at a > time. > > - Assuming we eventually get API for instantiating IHunks, I may have some > programmatically-created hunks that I want to apply to a file. We could add an API to let user browse hunks from a patch and decide which of them to apply. This way you could analyse and filter out changes that are irrelevant (like formatting changes in Java files). Also this would enable you to enable hunks one after another and patch a file in such a sequential way. Would that be enough? Adding Michael to CC list.
This is not about creating hacked patches. When I apply a patch using IFilePatch, some of the hunks may match while others are returned in the reject list of IFilePatchResult. The question then becomes how does a client attempt to apply only the rejects that have not yet been applied. The API mentioned in comment 1 would support this. In regards to your second paragraph in comment 3, I find it hard to determine exactly what you are proposing because you are mixing client terminology (e.g. API) with user terminology (e.g. browse). Perhaps you could try to restate your proposal in client terminology (e.g. perhaps pseudo code of how your proposed API would work).
I wasn't clear enough, sorry for that. What I meant is an API that looks as follows: 1. IHunk[] IFilePatch.getHunks(); 2A. void IFilePatch.setActiveHunks(IHunk[] ); or: 2B. void IFilePatch.setHunkFilter(IHunkFilter); where the filter has one method: boolean IHunkFilter.isHunkActive(IHunk); 3. IFilePatchResult IFilePatch.apply(IStorage contents, PatchConfiguration configuration, IProgressMonitor monitor); (already available) From IFilePatchResult you could obtain patched content as well as rejected hunks and pass the modified content through apply one more time (disabling matched hunks). Would that be API that satisfy your needs? Method applyPatch suggested in comment 1 (I guess in IFilePatch) would be static and ignoring all the hunks IFilePatch contains. I'm not in favour of that solution. Such an API would fix bug 183233 as well which is about returning hunks from IFilePatch.
The only remaining advantage of the API in comment 1 is that it would accommodate IHunks that were not associated with an IFilePath (i.e. if bug 183226 was address). However, one could argue that if bug 183226 was addressed an apply method could be added to IHunk itself. Given that, I think the API you are proposing would be adequate.
I'll prepare a patch with API in form suggested in comment 5. Adding static method to IFilePatch and ignoring hunks that the object contains (using only passed ones) seems inappropriate. If bug 183226 is fixed IHunk.apply could be added like Michael suggested in comment 6. As an alternative enabling clients to create IPatchFile from arbitrary IHunks would be possible. However this is a separate issue and will be tracked on bug 183226. Mike, I believe we have an agreement on this. Please correct me if I'm wrong.
We are in agreement.
Created attachment 125240 [details] Patch_v01 I added new API, additions are as follows. Methods: 1. IHunk[] IFilePatch.getHunks(); 2A. PatchConfiguration.addHunkFilter(IHunkFilter filter); 2B. PatchConfiguration.removeHunkFilter(IHunkFilter filter); 2C. IHunkFilter[] PatchConfiguration.getHunkFilters(); Interfaces: 3. IHunkFilter I changed the class that implements IHunk interface from HunkResult to Hunk. Some of the interface methods changed their behaviour a bit but previously they were incompatible with documentation. So, right now methods such as IHunk.getPatchedContents() don't take into consideration information about whether the patch is to be applied in a reversed way (API's client should take this into consideration since they provide a PatchConfiguration object). I'm not sure what to do with IHunk.getCharset() implementation which is now being moved into Hunk class. Hope one of the guys reviewing will suggest a solution.
Patch from comment 9 resolves also bug 183233.
> Method applyPatch suggested in comment 1 (I guess in IFilePatch) would > be static and ignoring all the hunks IFilePatch contains. This is not the case. If the caller wanted to apply all the hunks in an IFilePatch, they could have done this with the API from comment 1: IFilePatch originalPatch = ... applyPatch(contents, originalPatch.getHunks(), progress); The API proposed in comment 5 provides some filtering of hunks, but it still requires an IFilePatch. The reason I filed this bug originally is to address the case where you do not have an IFilePatch and need to apply an IHunk. Basically, the API from comment 5 is less general and more complicated than the API from comment 1. I don't see how this is an improvement.
(In reply to comment #11) > > Method applyPatch suggested in comment 1 (I guess in IFilePatch) would > > be static and ignoring all the hunks IFilePatch contains. > > This is not the case. If the caller wanted to apply all the hunks in an > IFilePatch, they could have done this with the API from comment 1: > > > IFilePatch originalPatch = ... > applyPatch(contents, originalPatch.getHunks(), progress); > Where would such applyPatch(...) method fit? I don't know whether you want to add it to IFilePatch or add a new class (I guess with no state since you pass IHunks to it) to handle the call. The former ignores the state of IFilePatch which doesn't look good for me. The latter introduces a new class that could be perceived as a form that operates on a set of IHunks (basically what IFilePatch does). Compare has a well known hierarchy of IHunks aggregated by IFilePatch aggregated by Patcher. I think it is better to allow creation of IFilePatch from a set of IHunks. This way we don't obscure current flow with a method enforcing a non-consistent one. If we add apply to IHunk like Mike suggested, we also gain ability to apply a single IHunk. So we could have three level hierarchy with an ability to perform apply on each level. Cleaner and more intuitive for me. Bug 183226 is about instantiating IHunks. I could add comment to let user create an IFilePatch instance out of a set of IHunks. > Basically, the API from comment 5 is less general and more complicated than the > API from comment 1. I don't see how this is an improvement. > I agree, it can be simplified. I could remove filtering facility and add IFilePatch.setEnabledHunks(IHunk []) to which you could pass a subset of IHunks returned from IFilePatch.getHunks(). That reduces the number of added methods from four to two and removes the need of introducing IHunkFilter. To summarize, I would go for introducing IFilePatch.setEnabledHunks(IHunk[]) and IFilePatch.getHunks() in this patch. Then add a method to instantiate IHunks and create from it a IFilePatch in bug 183226. This gives two new methods per bug and leaves the flow coherent. Guys, what do you think about it?
Created attachment 125371 [details] Patch_v02 (In reply to comment #9) > I'm not sure what to do with IHunk.getCharset() implementation which is now > being moved into Hunk class. Hope one of the guys reviewing will suggest a > solution. > Well, now this method behaves the way it used to. Charset is returned if available and in case of a call before the first apply attempt (when is impossible to obtain charset) it always returns null. I also deprecated it and added a javadoc link to use IFilePatchResult.getCharset() since it cannot be called before the first apply attempt.
Two comments: - I like the suggested solution and I'm sure that the filtering facility is better and more general than IFilePatch#setEnabledHunks(IHunk []). Summary of this bug should be changed to better suit Pawel's effort. - Move the discussion about applying arbitrary hunks to bug 183226. - Summary of bug 183226 should be changed to something like "Make API to apply arbitrary hunk" and the solution would include "API to instantiate IHunk".
Discussion about applying arbitrary hunks is moved to bug 183226.
Pawel, I like the patch. One comment is that Hunk and HunkResult could be merged in one class. I'm almost sure that we've discussed it, however I'm not sure what was the conclusion. Moreover, we need more tests :-) I think that I saw some in your workspace, please attach them too to the bug.
Hunk and HunkResult should not be combined. Hunk represents the state of a hunk independent of any file to which it could be applied. HunkResult represents an attempt to apply a hunk to a particular file. They were separated to make it easier to deal with failed attempts to apply a patch (i.e. if the attempt fails, you can throw away all your HunkResults and potentially attempt to apply the hunk to a different file).
(In reply to comment #16) > Pawel, I like the patch. One comment is that Hunk and HunkResult could be > merged in one class. I'm almost sure that we've discussed it, however I'm not > sure what was the conclusion. You're right, we've discussed it. One of the reasons to merge it was that IHunk interface is exposed via API in IHunk[] IFilePatchResult.getRejects() where HunkResult (the only class implementing IHunk) objects was returned. Since we wanted to return IHunks from IFilePatch, where only Hunks (no HunkResults) were present, one of the options was to merge the classes. However HunkResult delegated most of the calls from the interface to an aggregated Hunk object. So, I unattached the interface from HunkResult and attached it to Hunk. This way we can use the same interface in API no matter if results are present or not. To summarize, the only argument to merge the classes disappeared. > Moreover, we need more tests :-) I think that I saw some in your workspace, > please attach them too to the bug. > I'll address it in the next patch, thanks for pointing this out ;)
Created attachment 125770 [details] Patch_v03 Added unit test for new API.
Created attachment 125776 [details] Patch_v04 Patch updated to HEAD.
Created attachment 125789 [details] Patch_v04 with some minor changes
Released to HEAD. New API available in builds >N20090215-2000.
