Re: [PATCH] Patch mode, take 2

[Piotr Piastucki <leech miranda gmail com>]

On Sat, Mar 31, 2012 at 11:37 PM, Kai Willadsen <kai willadsen gmail com> 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.

A few issues I noticed when testing it:

 - The previous/next change shortcuts and buttons don't work. Like
file comparison, we should support Alt+Up/Down and Ctrl+E/D and make
it skip between chunks.

 - If I untoggle Flatten, the tree display breaks, maybe because I
wasn't applying it to the current directory? The tree ended up looking
like "0002-Enable-patch-mode.patch" -> ".." -> "meld-test" -> "" ->
"meld" -> "meldwindow.py". I think some path canonicalisation is in
order?

 - Similar thing in the Location column; there are double directory
separators showing.

 - I don't really see the point of the 'Show matched chunks' button.
Could you explain why I'd care to see that detail?

 - I can change the patch file name on top, but it doesn't do anything
except change the tree top level name. I think we should just not
support changing the patch file from there, and remove the entry;
change it to a label or something. Better yet, if we've got a patch
with a commit message, display that.

- Scrolling seems to becomes permanently unlocked if the files are the same.

- Our command line interface is already too overloaded, so I'd like to
see this mode require an explicit --patch option, rather than guessing
what to do when we see a .patch file.

First of all, thanks for the comments !
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.
 

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 :)
2) impact on the existing functionality; there is almost no impact, so at least the new code won't break anything.
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. 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 :)
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.

Regards,
Piotr