Re: [PATCH v7 0/7] convert: add support for different encodings

From: Jeff King <peff@peff.net>
Date: Feb 27, 2018, 21:25 UTC
Message-ID: <20180227212537.GA6899@sigill.intra.peff.net>
In-Reply-To: <20180227210517.GA17555@tor.lan>

On Tue, Feb 27, 2018 at 10:05:17PM +0100, Torsten Bögershausen wrote:

> The other question is:
> Would this help showing diffs of UTF-16 encoded files on a "git hoster",
> github/bitbucket/.... ?

Almost. There's probably one more thing needed. We don't currently read in-tree .gitattributes when doing a diff in a bare repository. And most hosting sites will store bare repositories.

And of course it would require the users to actually set the attributes themselves.

> Or would the auto-magic UTF-16 avoid binary patch that I send out be more helpful ?
> Or both ?
> Or the w-t-e encoding ?

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.

-Peff