On 4 April 2012 21:48, Piotr Piastucki <[email protected]> wrote: > On Sat, Mar 31, 2012 at 11:37 PM, Kai Willadsen <[email protected]> > wrote: >> >> This looks nice, and I think it may well be a good idea. I'd like to >> hear what other people think, whether they would find it useful, etc. > > First of all, thanks for the comments !
As a side note, I had a Master Plan *ages* ago that involved hooking up Meld + an Epiphany/Firefox plugin + bugzilla/whatever so that you could click a patch in bugzilla and have it download and launch Meld's patch mode on a local repository copy. I'm not sure that this is worth automating, but it's an example of how I wanted to use Meld to review patches. Allowing similar patch review from gitorious/github/etc. might also be awesome. > There are even more issues I am aware of :) Most notably, .rej files are not > saved yet. > > I based the options on Apply Patch dialog in Eclipse (see > http://piastucki.bdl.pl/meld/eclipse_apply_patch_dialog.png). Show matched > hunks is useful to review the whole patch hunk by hunk, I am not sure how > often it is used though. Personally, I wouldn't base any UI on what Eclipse does. To me, the matched hunks thing would only be useful if you didn't already have Meld's side-by-side view that shows changes in context. Maybe other people can chime in here, but I just don't see the need. >> However, these are mostly minor issues. The main thing that would stop >> me from merging this patch at this stage is the ~550 lines of patch >> parsing and handling code, which makes me nervous. I'll have to think >> about this. While I hate shelling out to patch, looking at other >> solutions makes me think that this isn't really all *that* stupid. >> Maybe you've thought about this already, but I'd like to know whether >> shelling out is an option, and if it's not, how robust is the patch >> parsing? what kinds of patches does it handle, etc. > > > I can fully understand your point of view as having additional 550 lines of > code to maintain is a considerable burden and I am far from pushing this > feature. Here are some additional informations: > 1) number of users; If there are just 2 people looking for this feature, > there is no point considering any merge at all. I have sent the patch to the > mailing list to get any input and see if there is anyone interested. Not too > many responses so far :) There are seldom many responses on this list, so I wouldn't let that put you off. :) I think a well-supported patch mode would probably be used by a fair number of people. > 2) impact on the existing functionality; there is almost no impact, so at > least the new code won't break anything. Yeah, the impact is relatively low. I don't love that it would introduce yet another type of comparison that we'd have to provide a way to launch, but that's not a huge problem. > 3) shelling out to patch; I did not consider it an option as I am not quite > sure if it is possible to get all the info needed to show patch details this > way. E.g. if parsing .rej file is the only way to get all rejected hunks, > the resulted code will contain both patch invoking code and patch file > parser. Honestly, my response to the .rej file issue would be that we should just refuse to handle any patches that didn't apply properly, which would circumvent having to do any patch parsing ourselves. Handling .rej files would be a nice addition of course. > If there are other ways to get all needed info from patch without > writing 200 lines of patch-specific hacks it will be great, otherwise, I > prefer to write twice as much clean and human-readable (and easily > portable!) code. I will take a look at what output can be retrieved from > patch as my experience with patch is limited to patching only :) I'm not suggesting that this is a good approach. It's what we do for reverse-applying VC diffs and I hate it. I just wanted to see whether you'd explored the option. > 4) supported patch formats; the code is supposed to handle unified patches. > I have not used other formats for years and IMHO unified format support is a > must have while support for other formats is a nice to have, but I may be > wrong. Unified patch is definitely the must-have option, and would cover many use cases. I think as long as we can apply the default patches generated by Git, Bazaar, Mercurial and SVN, we're probably okay. cheers, Kai _______________________________________________ meld-list mailing list [email protected] http://mail.gnome.org/mailman/listinfo/meld-list
