On 27 Feb 2018, at 22:25, Jeff King [off-list ref] wrote:
On Tue, Feb 27, 2018 at 10:05:17PM +0100, Torsten Bögershausen wrote:
Of the three solutions, I think the relative merits are something like
this:
1. baked-in textconv (my patch)
- reuses an existing diff feature, so minimal code and not likely to
break things
- requires people to add a .gitattributes entry
- needs work to make bare-repo .gitattributes work (though I think
this would be useful for other features, too)
- has a run-time cost at each diff to do the conversion
- may sometimes annoy people when it doesn't kick in (e.g.,
emailed patches from format-patch won't have a readable diff)
- doesn't combine with other custom-diff config (e.g., utf-16
storing C code should still use diff=c funcname rules, but
wouldn't with my patch)
2. auto-detect utf-16 (your patch)
- Just Works for existing repositories storing utf-16
- carries some risk of kicking in when people would like it not to
(e.g., when they really do want a binary patch that can be
applied).
I think it would probably be OK if this kicked in only when
ALLOW_TEXTCONV is set (the default for porcelain), and --binary
is not (i.e., when we would otherwise just say "binary
files differ").
- similar to (1), carries a run-time cost for each diff, and users
may sometimes still see binary diffs
3. w-t-e (Lars's patch)
- requires no server-side modifications; the diff is plain vanilla
- works everywhere you diff, plumbing and porcelain
- does require people to add a .gitattributes entry
- run-time cost is per-checkout, not per-diff
So I can see room for (3) to co-exist alongside the others. Between (1)
and (2), I think (2) is probably the better direction.
Thanks for the great summary! I agree they could co-exist and people
could use whatever works best for them.
I'll incorporate Eric's feedback and send a w-t-e v9 soonish.
- Lars