Re: [PATCH] Patch mode, take 2

[Kai Willadsen <kai willadsen gmail com>]

On 4 April 2012 21:48, Piotr Piastucki <leech miranda gmail com> wrote:
> 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.
>
> 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