Re: [PATCH] Patch mode, take 2

[Kai Willadsen <kai willadsen gmail com>]

On 30 March 2012 01:59, Piotr Piastucki <leech miranda gmail com> wrote:
> Hi,
>
> I sent a small patch that added a simple patch view mode to meld a couple of
> moths ago. Here is a somewhat improved version that makes it possible to
> review patches and apply them to existing source code folder. Some info
> about the usage can be found at http://piastucki.bdl.pl/meld/patch/. From
> the technical point of view it works more or less as follows:
> 1) patch file is parsed (patchparser.py)
> 2) changes are applied in memory (patcher.py)
> 3) new UI to review patches is provided (patchview.py, patchdiff.py)
>
> Any comment, questions and ideas for improvements are welcome.

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.

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.

Either way, it would be great to see this code and any successors
attached to bugzilla somewhere.

cheers,
Kai