Re: [PATCH 3/4] diff: introduce diff.<driver>.binary

[Jeff King <peff@peff.net>]

On Sun, Oct 12, 2008 at 09:00:50PM -0700, Junio C Hamano wrote:

> exchange workflow?  There needs either one or both of the following:
> 
>  - A command line option to Porcelains to override textconv so that an
>    applicable binary diff can be obtained upon request (or format-patch
>    always disables textconv); and/or

format-patch should always disable textconv. I admit that I didn't test
it, but assumed it was covered under the same code path as external
diff (i.e., just not reading in the config). But it doesn't seem to be.

So that is definitely broken in my patch series. But even once that is
fixed, I agree there should be a porcelain switch for turning this off.
I sometimes do "git diff >patch" and read the result into my MUA.
Obviously I would not want textconv'ing there.

>  - You teach git-apply to use a reverse transformation of textconv, so
>    that it does, upon reception of a textconv diff:
> 
>    (1) pass existing preimage through textconv;
>    (2) apply the patch;
>    (3) convert the result back to binary.

The problem with this approach is that it requires that the textconv be
a reversible mapping. And the two motivating examples (dumping exif tags
and converting word processor documents to text) are not; they are lossy
conversions.

It's possible that one could, given the binary preimage and the two
lossy textconv'd versions, produce a custom binary merge that would just
munge the tags, or just munge the text, or whatever. But that is an
order of magnitude more work than writing a textconv, which is usually
as simple as "/path/to/existing/conversion-script".

And the whole point of this exercise was to make it simple to set this
conversion up.

> I'd say that format-patch should always disable textconv so that we won't
> have to worry about it for now.

Agreed, if you remove "for now". I had never intended for these to be
anything but a human-readable display (and while I am generally OK with
generalizing functionality when we can, I think there is real value in
the simplicity here).

If somebody really wants to send patches with converted text for
reference, I would suggest producing a patch with the textconv'd output
as a comment, and including the full binary patch to actually be
applied (and yes, obviously they a malicious attacker could make them
not match, but given the binary patch, we can trivially regenerate the
textconv'd version and confirm it).

-Peff