End-of-line normalization, take 2 (now only slightly scary)

Okay, here's a second shot at my end-of-line conversion sanification series. The main change is that instead of introducing a new attribute, I've added a new mode to the "crlf" attribute: "crlf=auto".

The semantics of "crlf" have also been modified slightly in that the attribute now enables end-of-line normalization whenever it is set, instead of simply amending what happens when "core.autocrlf" is enabled as it did before.

I think this version is a step in a direction that everyone can agree on. The new config variable seems to be a point of contention, but while I'm not married to the name "core.eolStyle" by any means, I don't think "core.crlf" explains what it is, and I don't think "input" and "true" are good ways of describing what it does.

I agree with Linus that "crlf=input" seems crazy, and I agree with Dmitry that "crlf" is a bad name for the attribute to begin with. This isn't bikeshedding: git has lost a lot of its sharp edges recently, but this one remains, and is quite an important one for Windows users.

For now, though, I'd like the focus for this patch series to be creating a mechanism that allows a project to enforce line ending normalization for all contributors to the repository, while letting the user decide what line endings he likes to see in his working directory. If we end up not shooting anybody in the head, all the better.

Thanks for all your inputs. I was worried that nobody would even notice this series :) I've expanded the Cc list a bit, hope that's okay.

Eyvind Bernhardsen (4):
Add "core.eolStyle" variable to control end-of-line conversion Add tests for per-repository eol normalization
Pass eol conv mode as an argument instead of using global auto_crlf Add per-repository eol normalization

 Documentation/config.txt        |   11 ++-
 Documentation/gitattributes.txt |   71 +++++++++++-----
 Makefile                        |    3 +
 cache.h                         |   19 ++++
 config.c                        |   16 +++-
 convert.c                       |   52 +++++++++---
 environment.c                   |    1 +
 t/t0025-crlf-auto.sh            |  180 +++++++++++++++++++++++++++++++++++++++
 8 files changed, 317 insertions(+), 36 deletions(-)
 create mode 100755 t/t0025-crlf-auto.sh

Introduce a new configuration variable, "core.eolStyle", that allows the user to set which line endings to use for end-of-line-normalized files in the working directory. It defaults to "native", which means CRLF on Windows and LF everywhere else.

---
 Documentation/config.txt |    7 +++++++
 Makefile                 |    3 +++
 cache.h                  |   19 +++++++++++++++++++
 config.c                 |   16 +++++++++++++++-
 environment.c            |    1 +
 5 files changed, 45 insertions(+), 1 deletions(-)

index 92f851e..3956ff7 100644
@@ -207,6 +207,13 @@ core.autocrlf::
 	the file's `crlf` attribute, or if `crlf` is unspecified,
 	based on the file's contents.  See linkgit:gitattributes[5].
 
+core.eolStyle::
+	Sets the line ending type to use for text files in the working
+	directory when the `auto-eol` property is set.  Alternatives are
+	'lf', 'crlf', 'native' and 'false'.  'native', the default, uses
+	the platform's native line ending.  'false' disables `auto-eol`
+	line ending conversion.  See linkgit:gitattributes[5].
+
 core.safecrlf::
 	If true, makes git check if converting `CRLF` as controlled by
 	`core.autocrlf` is reversible.  Git will verify if a command

index 910f471..419532e 100644
@@ -224,6 +224,8 @@ all::
 #
 # Define CHECK_HEADER_DEPENDENCIES to check for problems in the hard-coded
 # dependency rules.
+#
+# Define NATIVE_CRLF if your platform uses CRLF for line endings.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN

@@ -989,6 +991,7 @@ ifeq ($(uname_S),Windows)
 	NO_CURL = YesPlease
 	NO_PYTHON = YesPlease
 	BLK_SHA1 = YesPlease
+	NATIVE_CRLF = YesPlease
 
 	CC = compat/vcbuild/scripts/clink.pl
 	AR = compat/vcbuild/scripts/lib.pl

index 5eb0573..690511e 100644
@@ -561,6 +561,25 @@ enum safe_crlf {
 
 extern enum safe_crlf safe_crlf;
 
+enum auto_crlf {
+	AUTO_CRLF_FALSE = 0,
+	AUTO_CRLF_TRUE = 1,
+	AUTO_CRLF_INPUT = -1,
+};
+
+enum eol_style {
+	EOL_STYLE_FALSE = AUTO_CRLF_FALSE,
+	EOL_STYLE_CRLF = AUTO_CRLF_TRUE,
+	EOL_STYLE_LF = AUTO_CRLF_INPUT,
+#ifdef NATIVE_CRLF
+	EOL_STYLE_NATIVE = EOL_STYLE_CRLF,
+#else
+	EOL_STYLE_NATIVE = EOL_STYLE_LF,
+#endif
+};
+
+extern enum eol_style eol_style;
+
 enum branch_track {
 	BRANCH_TRACK_UNSPECIFIED = -1,
 	BRANCH_TRACK_NEVER = 0,

index 6963fbe..8a11052 100644
@@ -461,7 +461,7 @@ static int git_default_core_config(const char *var, const char *value)
 
 	if (!strcmp(var, "core.autocrlf")) {
 		if (value && !strcasecmp(value, "input")) {
-			auto_crlf = -1;
+			auto_crlf = AUTO_CRLF_INPUT;
 			return 0;
 		}
 		auto_crlf = git_config_bool(var, value);

@@ -477,6 +477,20 @@ static int git_default_core_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.eolstyle")) {
+		if (value && !strcasecmp(value, "lf"))
+			eol_style = EOL_STYLE_LF;
+		else if (value && !strcasecmp(value, "crlf"))
+			eol_style = EOL_STYLE_CRLF;
+		else if (value && !strcasecmp(value, "native"))
+			eol_style = EOL_STYLE_NATIVE;
+		else if (! git_config_bool(var, value))
+			eol_style = EOL_STYLE_FALSE;
+		else
+			return error("Malformed value for %s", var);
+		return 0;
+	}
+
 	if (!strcmp(var, "core.notesref")) {
 		notes_ref_name = xstrdup(value);
 		return 0;

index 876c5e5..05cd1d5 100644
@@ -40,6 +40,7 @@ const char *editor_program;
 const char *excludes_file;
 int auto_crlf = 0;	/* 1: both ways, -1: only when adding git objects */
 int read_replace_refs = 1;
+enum eol_style eol_style = EOL_STYLE_NATIVE;
 enum safe_crlf safe_crlf = SAFE_CRLF_WARN;
 unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;

1.7.1.3.gb95c9

So I at least agree with the semantics now, but I think that we really would be better off just calling it "core.crlf". I don't _care_ whether people think it's a bad name - much worse than a bad name is to use a name that is not consistent.

Having two config variables named "core.autocrlf" and "core.crlf" at least is consistent. Having "autocrlf" and "eolStyle" is just messed up.

Linus

I see your point, but "crlf=lf" makes no sense and "crlf=input" is hideous. It's bad enough that autocrlf uses "true" and "input"; this is a chance to reduce the insanity, not perpetuate it.

I'll try to think of a better name.

Eyvind

Heh. How about "localcrlf={true,false,native}"?

It breaks down a bit if we ever decide to support old-school-Mac-style CR line endings, but at that point you're approaching the borders of madness anyway.

Eyvind

IMHO, the 'local' prefix certainly does not improve anything. Also, I would rather call default as "default" instead of "native". So, why not use "core.crlf={true, false, default}"?

Though crlf is not my preferable name, I think consistency is important, and we should use the same name here as in git attributes.

Dmitry

But the attribute does something different! The attribute turns eol conversion on and off, the configuration variable decides which line endings to use when conversion is on. They should be related, but making them identical doesn't make any kind of sense.

To be consistent, I would expect a variable called "core.crlf" to do the same thing as the attribute, so "core.crlf=auto" would replace "core.autocrlf={true,input}", allowing you to turn on line ending conversion without having to modify the repository.

If we added this consistently named "core.crlf", we'd still need a separate config variable to decide between LFs and CRLFs in the working directory, so what should that variable be called?

"localcrlf" at least conveys the idea that this is a local setting, so it's not too much of a stretch to guess that it controls the working directory.

Eyvind

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

After some meditation, I think the following:

The most important thing is how we can fix git on Windows to operate well by default. This is what we really should try to fix.

The way git on Windows would work best, while at the same time cause the minimum amount of damage, would IMHO be:

- Convert LF-only text files to CRLF on checkout
- Convert those same files (but only those!) back to LF on commit
- Optionally: Convert new CRLF text files to LF on commit

And all this should happen without setting any configuration, and not setting any attributes.

So maybe, just maybe, we can make everything sufficiently good by repairing "core.autocrlf = {input,true}" so that git will not convert a CRLF already in the repo. This would make autocrlf = true a safe default value (and probably input too, but you'd have to "do something" to get a new text file with CRLF into the repo then).

Then we go from a situation that didn't work, to a working situation, and break nothing that used to work. You no longer have to normalise your repos to work from Windows.

- Finn Arne

First of autocrlf is safe as it is implemented now. Second, to do something to get a new with CRLF into the repo is really stupid. The whole point of autocrlf is being automatic and do not have the user to worry about CRLF when he adds a new file.

The only real problem I am aware of is that some repository are not compatible with autocrlf conversion, because they store text files with different endings and do not have appropriate .gitatributes to describle what text files should and should not be converted. So, each user has to disable autocrlf in them manually and then re-checkout all files using (rm -rf * && git checkout -f), which is confusing for many users. In fact, you do not have to disable autocrlf, you can add a few lines to .git/info/attributes to make it autocrlf compatible, but again many users even not aware about this file, let alone what needs to be added. Further, the problem amplified by the fact that you have to do the same procedure every time when you do cloning, and though cloning is not most common operation, it happens often enough to annoy many users.

I believe that the right solution is to be able to enable autocrlf but only for those repositories that are marked as autocrlf compatible by upstream.

Dmitry

No, it isn't. autocrlf as it is implemented now is destructive for any file that contains CRLF in the repo (it also gives dirty files after checkout and so on).

Yes absolutely. But how do you tell autocrlf that the repository is compatible with it? This is what is causing all the problems.

Now, I propose to change autocrlf in such a way that it will work as before for all repositories that are "compatible with it", but _also_ so that it works reasonably with those that aren't.

- Finn Arne

It may give dirty files in your working tree, but you do not lose any information silently (as it happened with CVS), so it is safe in this respect.

I suppose the original design assumption of autocrlf was that nearly all repo should be compatible, because autocrlf is very good on detecting text files. The problem with that is that many people want to store text files with different endings, but they cannot be bothered to add a few lines to .gitattributes. And while it is possible now to disable autocrlf in any repo just by adding one line to .gitattributes:
* -crlf
but it is impossible to say the opposite.

So, I believe we should add "crlf=auto" as Eyvind proposed, as well as to do eol conversion for files marked as "crlf" even if autocrlf is not set as Linus suggested earlier. (Certainly, "eolconv" would be a better name than "crlf", but maybe it is not the right time to replace it right now).

No, you proposed something different. You said that conversion for new files would become optional. I don't know what exactly you mean by optional, but it sounds incompatible with what we have now. In fact, what I really like about autocrlf is that I do not need to think about when I add a new file.

Moreover, you said "Convert LF-only text files to CRLF on checkout", which raises two questions:

1. Where should information about what file was and what was not converted be stored? (Storying it in the index can make the index incompatible with old versions of Git).

2. How does it solve the problem with new files? (Just saying that this conversation will be optional, does not it mean that it will be right for this particular repo.)

Dmitry

I am puzzled about this second item; what is its type and what is its purpose? If it is to allow project-wide default to be specified, then isn't having "* eolconv=true" in .gitattributes a much better option and is already supported by the first item?

The way I understood it core.eolconv has exactly the same possible values as the "eolconv" attribute, and serves as a default value for "eolconv" if it isn't set. This would (mostly, I guess) be for Windows users who would like to check out a project that is primarily developed on Unix and didn't bother to set any eolconv attributes, and still get CRLF line endings. core.eolconv has some of the drawbacks of autocrlf, so shouldn't really be used if you can convince projects to add eolconv attributes instead.

Are you thinking we could live completely without it? Most other popular vcs-systems have eol-conversion/normalisation on by default, while git has it disabled by default. The config variable can change the default behaviour, but is isn't as helpful as it should have been perhaps.

To do a better job with old un-normalised repos, we could for each file remember what their eol-style was (CR, LF, CRLF, mixed), and then even if doing output conversion on checkout, convert back to the same eol style on commit. This would perhaps break down a bit for renames, but would be lovely if it worked for merges for example...

- Finn Arne

Yes, and your description makes it sound like it doesn't buy us anything that an entry '* eolconv=true" in .git/info/attributes wouldn't.

I'm not sure I even knew about .git/info/attributes, but it makes a lot of sense. The config variable would allow a user to turn on normalization globally, I guess, but that's probably not worth it.

Eyvind

Yes, but if the repository maintainers don't want to make that change (for whatever reason), it might be nice for the user to be able to enable normalization locally. It would be a boolean, and setting it would mean the equivalent of "crlf=auto" on all files where "crlf" is not explicitly set.

Eyvind

[...]

So, the meanings of these would become...

core.crlf [ auto | input | false ] : 'auto' means to enable bidirectional normalization, and 'false' would mean do not normalization, and 'input' would mean normalize on input only, otherwise output lf. Is this true?
core.localcrlf [ crlf | lf ] : this is obvious, and use-friendly

For the above case have you considered using 'core.crlflocal' instead? Usability-wise the related properties start with the same name prefix.

what you (see "my user interface would have been") specified, slight adjustment to the names only:

core.eolconv [ true | false ] - whether or not to turn on conversions core.eoltype [ lf | crlf ] - by default what to convert to for text files

I like this purely because, from the users standpoint, saying something like "localcrlf crlf" is strange; meaning the term "crlf" is on both sides of the assignment. I do prefer "eol... crlf", where eol refers to the applicability of the property and crlf is only one such value.

No, "auto" means to enable normalization for files git doesn't identify as text files, "true" means to always normalize, and "false" means never normalize. I probably wouldn't implement "input" unless there was a lot of demand. The idea is to make it act exactly like the "crlf" attribute, even though "core.crlf=true/false" would probably be used very rarely... I'm having second thoughts, actually.

Well, yes. I was thinking "true|false" (ie "I want crlf" or "I don't want crlf"), but I'm having second thoughts about that, too.

I didn't think too much about the name, so a completely different name might be even better.

Because of this and my second thoughts, I'm going to wait a few days before I make any more changes to allow good ideas to appear and give them time to sink in.

Agreed, but I think getting this feature included is more important than getting the user interface exactly right. A compromise between backwards compatibility and user friendliness is okay.

Yes, my "localcrlf" would be true/false instead of crlf/lf. It's definitely a compromise :)

Eyvind

I probably missed something. The part that confuses me in this statement is that you said "for files git doesn't identify as text files". The convert.c source is the heart of this, and if a file is not identified as text it is presumed to be binary. The statement made seems to imply you'd auto-convert PDF files? I know you did not mean that, but it could have been read that way.

What specifically happens in the three modes? Would it be precise to say the following?

"Files subject to EOL conversion are those that are explicitly
identified through attributes to be text files, or those algorithmically determined to be text files which happen to not bear the "text" file attribute. Otherwise the default value, "false", applies and no EOL conversions occur."

-Bob

Doh! I meant to write "files git _does_ identify as text files". Sorry for the confusion.

Very close, but my thinko threw you off. The "algorithmic determination" of text files is only performed when crlf=auto, either by the attribute or the config variable being set that way.

The point of the "core.crlf" config variable would be to provide a default value for the "crlf" attribute.

Eyvind

Okay, so that makes sense...

"Files subject to EOL conversion are those that are explicitly
identified through attributes to be text files, or provided core.crlf is
set to auto, those files algorithmically determined to be text files which happen to not bear
the "text" file attribute. Otherwise the default value, "false",
applies and no EOL conversions occur. When conversions occur the EOL character changes from the internal LF format to the format specified by core.crlftype."

This would work out well if file type maps were ever introduced. Type maps would not short circuit the explicit attributes identified by the first clause, then for those files without attributes you'd check the file-type maps, then fall back to the algorithmic if auto is enabled.

I like that.

Today I am writing up some documentation related to Git use at the company, and planning to check in all the data from Git vs Hg testing, and all on-boarding/first-day documentation related to Git use. So within the directory containing all these files I init-ed the repository and got this when I attempted to add multiple files. Mind you, this is on Linux!

"warning: LF will be replaced by CRLF"

This _really_ of scares me being that this was on Linux. Is this one of the problem spots that the topic of this thread attempts to resolve?

Bob

It looks like you set core.autocrlf to true on Linux.

The problem is core.autocrlf says two things -- whether you want to enable automatic eol conversion for text files and whether you have CRLF for text files. Only if the answer is "yes" to both then you should set it to true. In other words, you should never set it to true on Linux.

You may want use core.autocrlf=input on Linux to avoid text files being accidentally commit with a wrong encoding, when they were copied from Windows directly.

Yes, we want to have a separate setting, whihc will tell what is EOL on your system, and it should have sensible default, i.e. LF on Linux and CRLF on Windows.

Dmitry

Bah. I think relegating the old names to "deprecated, for compatibility" is absolutely the right thing to do. Is there a use case where the existing crlf setup is preferable? If not, why not just mark them as deprecated in the documentation and say "see ..." pointing to the new functionality and use the new names as you suggest.

$0.02. :-)

j.

One more point, which I'm sure will be inflammatory. After all, it has nothing to do with anything, but let's look at how svn deals with EOL issues:

1) A property for whether a file is binary or text
2) A property for the EOL style which applies only to text files:

http://svnbook.red-bean.com/en/1.5/svn.advanced.props.file-portability.html#svn.advanced.props.special.eol-style

And Mercurial's plan to deal with it:

http://mercurial.selenic.com/wiki/EOLTranslationPlan

:-)

j.

I don't know if it would get accepted, but I'll add a commit that does that to the next iteration :)

Eyvind

default and native have completely different connotations. default makes me think "one of true or false, which ever happens to be the default". native is a better fit here.

but indeed core.crlf=default means that it is either true or false whatever happened the system default, as well as "default" means the same as if it would be if it is not specified at all. If it were eol=native, it would be okay, but crlf=native looks strange to me.

Dmitry

It seems that autocrlf is currently so troublesome that most people end up disabling it, and handling it in other ways (we certainly do).

Instead of keeping the names and trying to graft on something that is almost backwards compatible, and forever live with names that we agree are bad, what about:

Deprecate core.autocrlf and the crlf attribute.

Make these instead:

Configs:
core.eol = lf, crlf, native [default=native] core.eolconversion = true, false, auto [default=unset]

Attribute:
eolconversion = true, false, auto [default=core.eolconversion]

- Finn Arne

I really don't understand that. What would it even mean?

An you _do_ realize that like it or not, we already have "crlf=input" as the syntax in our .gitattributes files? So that exact syntax already exists in one place.

As mentioned, I really can understand people not liking the name, but we already _have_ that name, and that syntax. I think it makes more sense to try to have a unified syntax than have two different strings for the same thing.

So I think we'd be better off with good documentation with a couple of real examples (and easily findable), so that the naming is at least something people can look up and see the semantics for. The "eol" vs "crlf" thing is just bike shedding, and we already ended up with "crlf". In contrast, making docs understandable is more than bikeshedding.

Linus

Sorry, I should have explained it. I chided you for "crlf=input" being crazy, but then I realized that "crlf=true" really isn't too bad a config variable for saying that you want CRLFs. Just "crlf" seems a bit obscure still, so I came up with "localcrlf": if it's "true" it means you want CRLFs locally (in your working directory), if it's "false" you want LFs.

It helps to think about the situations where you're likely to change this. If you're on Linux but you want CRLFs in your working directory, set "localcrlf=true". If you're on Windows but don't want CRLFs, "localcrlf=false". Thinking about it, "input" could be an alias for "false", which would make it _exactly_ what you suggested except for a slight clarification in the name.

I'm sorry. Are you the same Linus Torvalds who wrote this:

? I agree with that sentiment. "crlf=input" is silly; if you have a file that always has to be LF no matter what, just set it to "-crlf" and make sure you don't accidentally introduce any CRs. That is what you have to do for a file that always has to be CRLF, after all. As always, I'm not suggesting getting rid of "crlf=input"; I would keep it for backwards compatibility, but not emphasize it in the documentation.

I agree with this! I just don't agree that the unified syntax should be married to the old, poor syntax, and "crlf=input" is a straw man. It's both ugly and unnecessary.

Making the docs understandable is important. I have changed the docs, and I will keep trying to improve them. But making the configuration understandable is _also_ important! It's not just the colour of the bike shed, it's how it _works_. This discussion is not a waste of time.

But if you insist, here's how it looks to me: I'm the one trying to build a new bike shed from the parts of the grotty one that nobody really likes to use, and you're the one saying I need to paint it in the same 70s brown-and-mustard colour scheme as the old one.

The frustrating thing is that you're also coming up with some interesting colour combinations elsewhere, but you seem resigned to the idea that we're stuck with the ugliness. I don't think we are.

Eyvind

Yes, it's dubious. But as with the kernel, we need to support backwards compatibility for things that have reasonably been used (and "input" has).

I really brought it up as an example of things that weren't necessarily all that well designed.

That said, it looks like people actually do want per-file line-ending settings, ie not just a global "I want CRLF vs LF". So it looks like crlf=input is actually useful in a .gitattributes files, if only because some people seem to want to mix CRLF and just LF in the same repository.

It also sounds like people actually want to have the reverse (ie not just "input", but have a mode where LF may be the default, but then some particular files must always be CRLF even if most files are normal text).

So I suspect we want to really have support for all four combinations _both_ in the .git/config file, _and_ in the .gitattributes file.

The four cases would be "none" ("binary" or "-crlf"), "lf" ("input" or "crlf=input"), "system default", and "force crlf".

Honestly, I would personally have preferred to have just a repo-wide "this is the line ending". Not some path-specific endings like "crlf=input" in the .gitattributes. But people do seem to want it.

Linus

Oh, absolutely. I'm not suggesting removing support for it, I just don't think it's a good reason to keep the old naming alive.

My plan was to "support" that by disabling conversion for those files; git wouldn't enforce the line endings, but at least it wouldn't break them.

I guess there's no reason not to have the option to enforce in there if there is a huge demand for that.

Well, "crlf=crlf" and "crlf=lf" would look silly, but no more than I could live with. .gitconfig is already covered in my series.

Eyvind

---
 t/t0025-crlf-auto.sh |  180 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 180 insertions(+), 0 deletions(-)
 create mode 100755 t/t0025-crlf-auto.sh

new file mode 100755
index 0000000..367b459
@@ -0,0 +1,180 @@
+#!/bin/sh
+
+test_description='CRLF conversion'
+
+. ./test-lib.sh
+
+has_cr() {
+	tr '\015' Q <"$1" | grep Q >/dev/null
+}
+
+test_expect_success setup '
+
+	git config core.autocrlf false &&
+
+	for w in Hello world how are you; do echo $w; done >one &&
+	for w in I am very very fine thank you; do echo ${w}Q; done | q_to_cr >two &&
+	git add . &&
+
+	git commit -m initial &&
+
+	one=`git rev-parse HEAD:one` &&
+	two=`git rev-parse HEAD:two` &&
+
+	for w in Some extra lines here; do echo $w; done >>one &&
+	git diff >patch.file &&
+	patched=`git hash-object --stdin <one` &&
+	git read-tree --reset -u HEAD &&
+
+	echo happy.
+'
+
+test_expect_success 'default settings cause no changes' '
+
+	rm -f .gitattributes tmp one two &&
+	git read-tree --reset -u HEAD &&
+
+	if has_cr one || ! has_cr two
+	then
+		echo "Eh? $f"
+		false
+	fi &&
+	onediff=`git diff one` &&
+	twodiff=`git diff two` &&
+	test -z "$onediff" -a -z "$twodiff"
+'
+
+test_expect_success 'no crlf=auto, explicit eolstyle=native causes no changes' '
+
+	rm -f .gitattributes tmp one two &&
+	git config core.eolstyle native &&
+	git read-tree --reset -u HEAD &&
+
+	if has_cr one || ! has_cr two
+	then
+		echo "Eh? $f"
+		false
+	fi &&
+	onediff=`git diff one` &&
+	twodiff=`git diff two` &&
+	test -z "$onediff" -a -z "$twodiff"
+'
+
+test_expect_failure 'crlf=auto, eolStyle=crlf <=> autocrlf=true' '
+
+	rm -f .gitattributes tmp one two &&
+	git config core.autocrlf false &&
+	git config core.eolstyle crlf &&
+	echo "* crlf=auto" > .gitattributes &&
+	git read-tree --reset -u HEAD &&
+	unset missing_cr &&
+
+	for f in one two
+	do
+		if ! has_cr "$f"
+		then
+			echo "Eh? $f"
+			missing_cr=1
+			break
+		fi
+	done &&
+	test -z "$missing_cr"
+'
+
+test_expect_failure 'crlf=auto, eolStyle=lf <=> autocrlf=input' '
+
+	rm -f .gitattributes tmp one two &&
+	git config core.autocrlf false &&
+	git config core.eolstyle lf &&
+	echo "* crlf=auto" > .gitattributes &&
+	git read-tree --reset -u HEAD &&
+
+	if has_cr one || ! has_cr two
+	then
+		echo "Eh? $f"
+		false
+	fi &&
+	onediff=`git diff one` &&
+	twodiff=`git diff two` &&
+	test -z "$onediff" -a -n "$twodiff"
+'
+
+test_expect_success 'crlf=auto, eolStyle=false <=> autocrlf=false' '
+
+	rm -f .gitattributes tmp one two &&
+	git config core.autocrlf false &&
+	git config core.eolstyle false &&
+	echo "* crlf=auto" > .gitattributes &&
+	git read-tree --reset -u HEAD &&
+
+	if has_cr one || ! has_cr two
+	then
+		echo "Eh? $f"
+		false
+	fi
+	onediff=`git diff one` &&
+	twodiff=`git diff two` &&
+	test -z "$onediff" -a -z "$twodiff"
+'
+
+test_expect_success 'autocrlf=true overrides crlf=auto, eolStyle=lf' '
+
+	rm -f .gitattributes tmp one two &&
+	git config core.autocrlf true &&
+	git config core.eolstyle lf &&
+	echo "* crlf=auto" > .gitattributes &&
+	git read-tree --reset -u HEAD &&
+	unset missing_cr &&
+
+	for f in one two
+	do
+		if ! has_cr "$f"
+		then
+			echo "Eh? $f"
+			missing_cr=1
+			break
+		fi
+	done &&
+	test -z "$missing_cr"
+'
+
+test_expect_success 'autocrlf=input overrides crlf=auto, eolStyle=crlf' '
+
+	rm -f .gitattributes tmp one two &&
+	git config core.autocrlf input &&
+	git config core.eolstyle crlf &&
+	echo "* crlf=auto" > .gitattributes &&
+	git read-tree --reset -u HEAD &&
+
+	if has_cr one || ! has_cr two
+	then
+		echo "Eh? $f"
+		false
+	fi &&
+	onediff=`git diff one` &&
+	twodiff=`git diff two` &&
+	test -z "$onediff" -a -n "$twodiff"
+'
+
+test_expect_success 'autocrlf=true overrides crlf=auto, eolStyle=false' '
+
+	rm -f .gitattributes tmp one two &&
+	git config core.autocrlf true &&
+	git config core.eolstyle false &&
+	echo "* crlf=auto" > .gitattributes &&
+	git read-tree --reset -u HEAD &&
+	unset missing_cr &&
+
+	for f in one two
+	do
+		if ! has_cr "$f"
+		then
+			echo "Eh? $f"
+			missing_cr=1
+			break
+		fi
+	done &&
+	test -z "$missing_cr"
+'
+
+test_done

1.7.1.3.gb95c9

This patch has no semantic changes, but makes the next commit easier to review.

---
 convert.c |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

index 4f8fcb7..2eef2f6 100644
@@ -90,12 +90,13 @@ static int is_binary(unsigned long size, struct text_stat *stats)
 }
 
 static void check_safe_crlf(const char *path, int action,
-                            struct text_stat *stats, enum safe_crlf checksafe)
+			    struct text_stat *stats, enum safe_crlf checksafe,
+			    int eol_conversion)
 {
 	if (!checksafe)
 		return;
 
-	if (action == CRLF_INPUT || auto_crlf <= 0) {
+	if (action == CRLF_INPUT || eol_conversion <= 0) {
 		/*
 		 * CRLFs would not be restored by checkout:
 		 * check if we'd remove CRLFs

@@ -106,7 +107,7 @@ static void check_safe_crlf(const char *path, int action,
 			else /* i.e. SAFE_CRLF_FAIL */
 				die("CRLF would be replaced by LF in %s.", path);
 		}
-	} else if (auto_crlf > 0) {
+	} else if (eol_conversion > 0) {
 		/*
 		 * CRLFs would be added by checkout:
 		 * check if we have "naked" LFs

@@ -121,12 +122,13 @@ static void check_safe_crlf(const char *path, int action,
 }
 
 static int crlf_to_git(const char *path, const char *src, size_t len,
-                       struct strbuf *buf, int action, enum safe_crlf checksafe)
+		       struct strbuf *buf, int action, enum safe_crlf checksafe,
+		       int eol_conversion)
 {
 	struct text_stat stats;
 	char *dst;
 
-	if ((action == CRLF_BINARY) || !auto_crlf || !len)
+	if ((action == CRLF_BINARY) || !eol_conversion || !len)
 		return 0;
 
 	gather_stats(src, len, &stats);

@@ -147,7 +149,7 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 			return 0;
 	}
 
-	check_safe_crlf(path, action, &stats, checksafe);
+	check_safe_crlf(path, action, &stats, checksafe, eol_conversion);
 
 	/* Optimization: No CR? Nothing to convert, regardless. */
 	if (!stats.cr)

@@ -180,13 +182,13 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 }
 
 static int crlf_to_worktree(const char *path, const char *src, size_t len,
-                            struct strbuf *buf, int action)
+			    struct strbuf *buf, int action, int eol_conversion)
 {
 	char *to_free = NULL;
 	struct text_stat stats;
 
 	if ((action == CRLF_BINARY) || (action == CRLF_INPUT) ||
-	    auto_crlf <= 0)
+	    eol_conversion <= 0)
 		return 0;
 
 	if (!len)

@@ -591,7 +593,7 @@ int convert_to_git(const char *path, const char *src, size_t len,
 		src = dst->buf;
 		len = dst->len;
 	}
-	ret |= crlf_to_git(path, src, len, dst, crlf, checksafe);
+	ret |= crlf_to_git(path, src, len, dst, crlf, checksafe, auto_crlf);
 	if (ret) {
 		src = dst->buf;
 		len = dst->len;

@@ -621,7 +623,7 @@ int convert_to_working_tree(const char *path, const char *src, size_t len, struc
 		src = dst->buf;
 		len = dst->len;
 	}
-	ret |= crlf_to_worktree(path, src, len, dst, crlf);
+	ret |= crlf_to_worktree(path, src, len, dst, crlf, auto_crlf);
 	if (ret) {
 		src = dst->buf;
 		len = dst->len;

1.7.1.3.gb95c9

Change the semantics of the "crlf" attribute so that it enables end-of-line normalization when it is set. Add a new setting, "auto", which enables end-of-line conversion but does not override the automatic binary file detection.

The effect of this change is that a project can enable end-of-line normalization for all files. This is similar to the "core.autocrlf" configuration variable, but since the setting is part of the content, it is cloned when the project is cloned and can be changed if a previously un-normalized repository is normalized.

Since the line ending style to be used in the working directory is a user preference, the configuration variable "core.eolStyle" controls it.

"core.autocrlf" can still be used to enable conversion, and every effort has been taken to avoid backwards incompatibility. There is one small semantic change: if "crlf" is set on a path, that file will now have its line endings normalized. Previously, they would only be normalized if "core.autocrlf" was also set.

---
 Documentation/config.txt        |    4 +-
 Documentation/gitattributes.txt |   71 +++++++++++++++++++++++++++-----------
 convert.c                       |   34 ++++++++++++++++--
 t/t0025-crlf-auto.sh            |    4 +-
 4 files changed, 84 insertions(+), 29 deletions(-)

index 3956ff7..7bbf8a0 100644
@@ -215,8 +215,8 @@ core.eolStyle::
 	line ending conversion.  See linkgit:gitattributes[5].
 
 core.safecrlf::
-	If true, makes git check if converting `CRLF` as controlled by
-	`core.autocrlf` is reversible.  Git will verify if a command
+	If true, makes git check if converting `CRLF` is reversible when
+	end-of-line conversion is active.  Git will verify if a command
 	modifies a file in the work tree either directly or indirectly.
 	For example, committing a file followed by checking out the
 	same file should yield the original file in the work tree.  If

index 2eef2f6..6871896 100644
@@ -15,6 +15,7 @@
 #define CRLF_BINARY	0
 #define CRLF_TEXT	1
 #define CRLF_INPUT	2
+#define CRLF_AUTO	3
 
 struct text_stat {
 	/* NUL, CR, LF and CRLF counts */

@@ -392,6 +393,17 @@ static void setup_convert_check(struct git_attr_check *check)
 	check[2].attr = attr_filter;
 }
 
+static int choose_eol_conversion(int auto_eol)
+{
+	if (auto_crlf)
+		return auto_crlf;
+
+	if (auto_eol)
+		return eol_style;
+
+	return 0;
+}
+
 static int count_ident(const char *cp, unsigned long size)
 {
 	/*

@@ -546,6 +558,8 @@ static int git_path_check_crlf(const char *path, struct git_attr_check *check)
 		;
 	else if (!strcmp(value, "input"))
 		return CRLF_INPUT;
+	else if (!strcmp(value, "auto"))
+		return CRLF_AUTO;
 	return CRLF_GUESS;
 }
 

@@ -575,7 +589,7 @@ int convert_to_git(const char *path, const char *src, size_t len,
 {
 	struct git_attr_check check[3];
 	int crlf = CRLF_GUESS;
-	int ident = 0, ret = 0;
+	int ident = 0, ret = 0, auto_eol = 0;
 	const char *filter = NULL;
 
 	setup_convert_check(check);

@@ -586,6 +600,11 @@ int convert_to_git(const char *path, const char *src, size_t len,
 		drv = git_path_check_convert(path, check + 2);
 		if (drv && drv->clean)
 			filter = drv->clean;
+		if (crlf > 0) {
+			auto_eol = 1;
+			if (crlf == CRLF_AUTO)
+				crlf = CRLF_GUESS;
+		}
 	}
 
 	ret |= apply_filter(path, src, len, dst, filter);

@@ -593,7 +612,8 @@ int convert_to_git(const char *path, const char *src, size_t len,
 		src = dst->buf;
 		len = dst->len;
 	}
-	ret |= crlf_to_git(path, src, len, dst, crlf, checksafe, auto_crlf);
+	ret |= crlf_to_git(path, src, len, dst, crlf, checksafe,
+		choose_eol_conversion(auto_eol));
 	if (ret) {
 		src = dst->buf;
 		len = dst->len;

@@ -605,7 +625,7 @@ int convert_to_working_tree(const char *path, const char *src, size_t len, struc
 {
 	struct git_attr_check check[3];
 	int crlf = CRLF_GUESS;
-	int ident = 0, ret = 0;
+	int ident = 0, ret = 0, auto_eol = 0;
 	const char *filter = NULL;
 
 	setup_convert_check(check);

@@ -616,6 +636,11 @@ int convert_to_working_tree(const char *path, const char *src, size_t len, struc
 		drv = git_path_check_convert(path, check + 2);
 		if (drv && drv->smudge)
 			filter = drv->smudge;
+		if (crlf > 0) {
+			auto_eol = 1;
+			if (crlf == CRLF_AUTO)
+				crlf = CRLF_GUESS;
+		}
 	}
 
 	ret |= ident_to_worktree(path, src, len, dst, ident);

@@ -623,7 +648,8 @@ int convert_to_working_tree(const char *path, const char *src, size_t len, struc
 		src = dst->buf;
 		len = dst->len;
 	}
-	ret |= crlf_to_worktree(path, src, len, dst, crlf, auto_crlf);
+	ret |= crlf_to_worktree(path, src, len, dst, crlf,
+		choose_eol_conversion(auto_eol));
 	if (ret) {
 		src = dst->buf;
 		len = dst->len;

index 367b459..e8e76f5 100755
@@ -60,7 +60,7 @@ test_expect_success 'no crlf=auto, explicit eolstyle=native causes no changes' '
 	test -z "$onediff" -a -z "$twodiff"
 '
 
-test_expect_failure 'crlf=auto, eolStyle=crlf <=> autocrlf=true' '
+test_expect_success 'crlf=auto, eolStyle=crlf <=> autocrlf=true' '
 
 	rm -f .gitattributes tmp one two &&
 	git config core.autocrlf false &&

@@ -81,7 +81,7 @@ test_expect_failure 'crlf=auto, eolStyle=crlf <=> autocrlf=true' '
 	test -z "$missing_cr"
 '
 
-test_expect_failure 'crlf=auto, eolStyle=lf <=> autocrlf=input' '
+test_expect_success 'crlf=auto, eolStyle=lf <=> autocrlf=input' '
 
 	rm -f .gitattributes tmp one two &&
 	git config core.autocrlf false &&

1.7.1.3.gb95c9