Re: [PATCH/RFC v2 1/4] Add "core.eolStyle" variable to control end-of-line conversion

[Eyvind Bernhardsen <eyvind.bernhardsen@gmail.com>]

I guess I should nail my flag to the mast: Here's what I would have done, with the benefit of plenty of hindsight, had we not had core.autocrlf, and also what I think we should do to approach that ideal.

Please don't get hung up too much on the names, they were chosen to not match anything suggested so far so that I can refer back to them unambiguously.

My user interface would have been:

- an attribute "eolconv" that enables or disables line ending conversion
- a config variable "core.eolconv" that sets "eolconv" for all files where it is unset
- a config variable "core.localeol" that decides whether LF or CRLF is preferred

This provides the means to enable normalization on a per-project ("eolconv") or per-repository ("core.eolconv") basis, and allows the user to override the platform native line ending when normalization is in effect.


Now, how does that compare to Git's current implementation of autocrlf?

- The config variable "core.autocrlf" enables or disables line ending conversion and decides if conversion occurs in "both" directions ("autocrlf=true") or just towards the repository ("autocrlf=input").

- The attribute "crlf" allows the automatic detection of binary files to be overridden, and forcing one-way conversion even if "core.autocrlf" is true ("crlf=input").  It only has an effect when "core.autocrlf" is enabled.

I think I've stated my case against these settings before, but just to be clear, I think the biggest problem is that there's no way to tell if a repository is normalized from the contents of the repository, and it's not safe to enable autocrlf if the repository isn't normalized.

The second biggest problem is that "true" and "input" are bad names for "normalize and put CRLFs in my working directory" and "normalize and put LFs in my working directory", respectively.


So how to progress from here?

As Junio observed, the "crlf" attribute can act just like my hypothetical "eolconv" if you squeeze its semantics a bit and add a new setting.  Disregarding "crlf=input" makes it exactly equivalent to "eolconv".  The name is a bit off, but it's not monstrous.

"core.localeol" has no equivalent in the current implementation.  Using "core.autocrlf" as a stand in would not work, since that setting is expected to actually enable normalization, so if we want a moral equivalent of "eolconv", we need a new setting.  "core.crlf" has been suggested to match the "crlf" attribute, but that causes confusion: why doesn't setting "core.crlf=auto" mean the same thing as setting "crlf=auto" on all files?  I think a new variable is needed.

"core.autocrlf" is problematic because it mixes up two things: you use it to turn on normalization _and_ to decide which line endings you prefer, and to maintain backwards compatibility its semantics can't be changed too much.  A new setting "core.autocrlf=auto" (meaning the same as "eolconv=auto" on all files where "eolconv" isn't explicitly set) is a possibility, if kind of ugly.  Another alternative is to make "core.autocrlf=true" respect "core.localeol", but that would be a v1.8.0 kind of change.


My current thinking on how to change my series now runs along these lines:

- keep the current "crlf=auto" change
- rename "core.eolStyle" to "core.localcrlf"
- add a "core.crlf" that sets the "crlf" attribute on paths where it isn't explicitly configured
- keep "core.autocrlf" for backwards compatibility, but make "core.autocrlf=input" and "core.autocrlf=true" complain if they are in conflict with the other config settings.



An apology: I know I've said a lot of nasty things about autocrlf, but I haven't had to touch a thing about its implementation, which works very well.  My only beef is with its user interface (which probably made sense when it was implemented); the reason I don't think I'm just whining about the colour of the bike shed is that a lot of other people trip over that interface, to the point where the standard recommendation is simply to disable the feature, and I'd like to make it both more functional and more understandable.

In short, I'm burning autocrlf to save it :)
-- 
Eyvind