Re: [PATCH] Respect crlf attribute in "git add" even if core.autocrlf has not been set

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 23, 2008, 09:02 UTC
Message-ID: <alpine.DEB.1.00.0807231051200.2830@eeepc-johanness>
In-Reply-To: <719E03C0-E8C3-4C35-AE9C-9BD5A7BCDF03@zib.de>

Hi,

On Wed, 23 Jul 2008, Steffen Prohaska wrote:

Show 8 quoted lines

> On Jul 23, 2008, at 3:31 AM, Johannes Schindelin wrote:
> 
> >When a file's crlf attribute is explicitely set, it does not make sense 
> >to ignore it when adding the file, just because the config variable 
> >core.autocrlf has not been set.
> 
> Your patch is not about core.autocrlf unset, but about 
> core.autocrlf=false.

No, no, no!

It is about autocrlf _unset_, i.e. when the user was _not_ explicit!

Show 5 quoted lines

> >The alternative would be risking to get CR/LF files into the repository 
> >just because one user forgot to set core.autocrlf.
> 
> Git could help the user *and* the maintainer of the repository if we
> chose core.autocrlf=input as the default on Unix.

NO!

The machinery behind core.autocrlf _does_ slow down Git, even if only by a few percent. But with patch queues as maintainers of busy projects encounter frequently, this makes a difference.

So no, I am _totally_ opposed to punishing Unix users for Windows' faults. Totally.

Show 16 quoted lines

> >diff --git a/convert.c b/convert.c
> >index 78efed8..d038d2f 100644
> >--- a/convert.c
> >+++ b/convert.c
> >@@ -126,7 +126,7 @@ static int crlf_to_git(const char *path, const char *src,
> >size_t len,
> > struct text_stat stats;
> > char *dst;
> >
> >-	if ((action == CRLF_BINARY) || !auto_crlf || !len)
> >+	if ((action == CRLF_BINARY) || (!auto_crlf && action < 0) || !len)
> 
> I think we should strictly follow the documentation, so this should read
> 
> +       if ((action == CRLF_BINARY) || (!auto_crlf && action != CRLF_INPUT) ||
> !len)

NO!

Your version would break action == CRLF_TEXT.

Show 17 quoted lines

> >diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
> >index 1be7446..0bb3e6f 100755
> >--- a/t/t0020-crlf.sh
> >+++ b/t/t0020-crlf.sh
> >@@ -436,4 +436,14 @@ test_expect_success 'invalid .gitattributes (must not
> >crash)' '
> >
> >'
> >
> >+test_expect_success 'attribute crlf is heeded even without core.autocrlf' '
> >+
> >+	echo "allcrlf crlf=input" > .gitattributes &&
> >+	git config --unset core.autocrlf &&
> 
> You should set core.autocrlf explicitly to false:
> 
>   git config core.autocrlf false

But this is _not_ what I want! I want it to be _unset_. Yes, at the moment, auto_crlf is initialized to 0. But I do not _care_ what it is set. I want this test to succeed when the user did _not_ set autocrlf.

> Otherwise the test would pick up the user's global default.

It will not. We went to great pains not to pick up global defaults in the test suite.

> ... and we should verify that we only treat crlf=input specially, but not
> "crlf".

Umm... Why? Why should a file marked "crlf=true" be allowed to be checked in _with_ CRs?

Ciao, Dscho