Changing encoding of a file : What should happen to CRLF in file ?

> Hello
> 
> I have a cross platform project. I have a utf-16 file in it.
> I changed its encoding to urf-8 and committed. When i pulled the file
> in Linux, it shows that file is modified. This means that the commit
> which changed the encoding does not convert crlf to lf, when new
> format is text based (utf-8).
> 
> Steps to reproduce:
> 
>    In windows :
> 
>     Change encoding of file from utf-16 to utf-8.
>     Commit the change.
> 
>    In linux:
> 
>     Pull your branch.
>     You will see the issue of file being shown as modified even though
> you have not used it.
> 
> 
> If i change the file encoding in linux and commit. Then if i do git
> pull in windows, i don't see the issue.
> In linux, during committing i get warning : warning: CRLF will be
> replaced by LF in …file_name..
> 
> Here are my configuration :
> 
> 
>> git config --global --get core.autocrlf
> 
> false
> 
> 
>> git config  --get core.autocrlf
> 
> false
> 
> 
> 
>> cat E:\work\WindowsFabric\.gitattributes
> 
> 
> # Set the default behavior, in case people don't have core.autocrlf set.
> 
> * text=auto
> *.vcxproj eol=crlf
> *.sh      eol=lf
> 
> # Denote all files that are truly binary and should not be modified.
> *.exe binary
> *.dll binary
> *.pdb binary
> *.ico binary
> *.png binary
> *.jpg binary
> 
> 
>> git --version
> git version 2.14.2.windows.2
> 
> 
> I played around with core.autocrlf values like true and native, but
> that didn't help.
> 
> The behavior is inconsistent across platforms, and windows version is
> giving me problem.
> 
> Can someone suggest right settings for this ? Or is this a bug in Git.
> 

> (Back to the beginning)
>
> You have a file ApplicationManifest.xml
> It is encoded in UTF-16 (and has CRLF)
>
> You convert it into UTF-8
> The file has still CRLF (in the worktree)
>
> Now you add it and make a commit.
> Under both Linux and Windows you have "text=auto".
>
> I assume that you have efficiently core.eol=lf under Linux
> and core.eol=crlf on Windows.
>
> (That is the default, when you don't change anything)
>
> Now, what happens to the CRLF?
> If you commit the file, it will be stored with LF in the index,
> on both systems.
> On checkout, Windows will convert them into CRLF, but Linux will not.
>
> That why you see
>>On linux, during committing i get warning : warning: CRLF will be
>>replaced by LF in …file_name..
>
> All in all there is nothing wrong, at least as I see it.
>
> The question remains:
> Do you need CRLF in Linux ?
> Probably not, but if yes, plase add a line
>
> *.xml text eol=crlf
>
> to your
> .gitattributes
>
> Otherwise your .gitconfig looks good to  me.
>
>
>
>
>
>

> > If you commit the file, it will be stored with LF in the index,
> This is what i believe is not happening.
> 
> Lets do this with a public repository and steps which are reproducible.
> I have created a repo : https://github.com/ashishnegi/git_encoding
> 
> If you clone this repo in linux and run `git status`, you will find
> that file is modified.

> On Wed, Nov 15, 2017 at 01:41:42PM +0530, Ashish Negi wrote:
>> > If you commit the file, it will be stored with LF in the index,
>> This is what i believe is not happening.
>>
>> Lets do this with a public repository and steps which are reproducible.
>> I have created a repo : https://github.com/ashishnegi/git_encoding
>>
>> If you clone this repo in linux and run `git status`, you will find
>> that file is modified.
>
> This is what what I get:
>
>  git ls-files --eol
> i/lf    w/lf    attr/text=auto          .gitattributes
> i/crlf  w/crlf  attr/text=auto          file_name.txt
>
> (And you get the same, I think)
> Newer versions of Git (>=2.10) will -not- treat file_name.txt as changed,
> older versions do.
> What does "git --version" say on your Linux machine ?
>
> However, if you want to fix it, you want to either end up with
>
> A)
> git ls-files --eol
> i/lf    w/lf    attr/text=auto          .gitattributes
> i/lf    w/crlf  attr/text=auto          file_name.txt
>
> or
> B)
> git ls-files --eol
> i/lf    w/lf    attr/text=auto          .gitattributes
> i/crlf  w/crlf  attr/-text              file_name.txt
>
> (The "attr/-text" means "don't change the line endings")
>
> Both A) or B) will work, typically A) is preferred.
>
> You should be able to achive A) :
>  git rm --cached file_name.txt  && git add file_name.txt
> rm 'file_name.txt'
> warning: CRLF will be replaced by LF in file_name.txt.
> The file will have its original line endings in your working directory.
>
> git ls-files --eol
> i/lf    w/lf    attr/text=auto          .gitattributes
> i/lf    w/crlf  attr/text=auto          file_name.txt
>
> [snip the rest]

> On windows :
> > git --version
> git version 2.14.2.windows.2
> 
> On linux :
> > git --version
> git version 2.7.4
> 
> I would like to understand the solution :
> If i understood it correctly : it removes file_name.txt from index, so
> git forgets about it.
> we then add the file again after changing encoding. This time, git
> takes it as utf-8 file and converts crlf to lf when storing it
> internally.
> Right ?

> On Thu, Nov 16, 2017 at 12:35:33AM +0530, Ashish Negi wrote:
>> On windows :
>> > git --version
>> git version 2.14.2.windows.2
>>
>> On linux :
>> > git --version
>> git version 2.7.4
>>
>> I would like to understand the solution :
>> If i understood it correctly : it removes file_name.txt from index, so
>> git forgets about it.
>> we then add the file again after changing encoding. This time, git
>> takes it as utf-8 file and converts crlf to lf when storing it
>> internally.
>> Right ?
>
> Yes, exactly.
> (In a coming release of Git there will be a "git add --renormalize <pathspec>" )
>
>>
>> Thank you for the support.
>>
>
> Thanks for a clean bug report.
> Actually it is a bug, I put it on my to do list
>
>

> On Thu, Nov 23, 2017 at 10:01:40PM +0530, Ashish Negi wrote:
>> Thanks for confirming.
>> Is it possible to track this via a bug number ?
>> It will help me to try out the fix when its available.
>>
>
> No worry, the fix is nearly complete and will come out in a couple of days.

> Running the command gives me :
> 
>       git ls-files --eol file_name
>       i/-text w/-text attr/text=auto          file_name
> 

> When a text file had been commited with CRLF and the file is commited
> again, the CRLF are kept if .gitattributs has "text=auto".
> This is done by analyzing the content of the blob stored in the index:
> If a '\r' is found, Git assumes that the blob was commited with CRLF.
>
> The simple search for a '\r' does not always work as expected:
> A file is encoded in UTF-16 with CRLF and commited. Git treats it as binary.
> Now the content is converted into UTF-8. At the next commit Git treats the
> file as text, the CRLF should be converted into LF, but isn't.
>
> Solution:
> Replace has_cr_in_index() with has_crlf_in_index(). When no '\r' is found,
> 0 is returned directly, this is the most common case.
> If a '\r' is found, the content is analyzed more deeply.
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
> diff --git a/convert.c b/convert.c
> @@ -220,18 +220,27 @@ static void check_safe_crlf(const char *path, enum crlf_action crlf_action,
> -static int has_cr_in_index(const struct index_state *istate, const char *path)
> +static int has_crlf_in_index(const struct index_state *istate, const char *path)
>  {
>         unsigned long sz;
>         void *data;
> -       int has_cr;
> +       const char *crp;
> +       int has_crlf = 0;
>
>         data = read_blob_data_from_index(istate, path, &sz);
>         if (!data)
>                 return 0;
> -       has_cr = memchr(data, '\r', sz) != NULL;
> +
> +       crp = memchr(data, '\r', sz);
> +       if (crp && (crp[1] == '\n')) {

> +               unsigned int ret_stats;
> +               ret_stats = gather_convert_stats(data, sz);
> +               if (!(ret_stats & CONVERT_STAT_BITS_BIN) &&
> +                   (ret_stats & CONVERT_STAT_BITS_TXT_CRLF))
> +                       has_crlf = 1;
> +       }
>         free(data);
> -       return has_cr;
> +       return has_crlf;
>  }

> On Fri, Nov 24, 2017 at 11:14 AM,  <tboegi@web.de> wrote:
> > When a text file had been commited with CRLF and the file is commited
> > again, the CRLF are kept if .gitattributs has "text=auto".
> > This is done by analyzing the content of the blob stored in the index:
> > If a '\r' is found, Git assumes that the blob was commited with CRLF.
> >
> > The simple search for a '\r' does not always work as expected:
> > A file is encoded in UTF-16 with CRLF and commited. Git treats it as binary.
> > Now the content is converted into UTF-8. At the next commit Git treats the
> > file as text, the CRLF should be converted into LF, but isn't.
> >
> > Solution:
> > Replace has_cr_in_index() with has_crlf_in_index(). When no '\r' is found,
> > 0 is returned directly, this is the most common case.
> > If a '\r' is found, the content is analyzed more deeply.
> >
> > Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> > ---
> > diff --git a/convert.c b/convert.c
> > @@ -220,18 +220,27 @@ static void check_safe_crlf(const char *path, enum crlf_action crlf_action,
> > -static int has_cr_in_index(const struct index_state *istate, const char *path)
> > +static int has_crlf_in_index(const struct index_state *istate, const char *path)
> >  {
> >         unsigned long sz;
> >         void *data;
> > -       int has_cr;
> > +       const char *crp;
> > +       int has_crlf = 0;
> >
> >         data = read_blob_data_from_index(istate, path, &sz);
> >         if (!data)
> >                 return 0;
> > -       has_cr = memchr(data, '\r', sz) != NULL;
> > +
> > +       crp = memchr(data, '\r', sz);
> > +       if (crp && (crp[1] == '\n')) {
> 
> If I understand correctly, this isn't a NUL-terminated string and it
> might be a binary blob, so if the lone CR in a file resides at the end
> of the file, won't this try looking for LF out-of-bounds? I would have
> expected the conditional to be:
> 
>     if (crp && crp - data + 1 < sz && crp[1] == '\n') {
> 
> or any equivalent variation.
> 

> From: Torsten Bögershausen <tboegi@web.de>
>
> When a text file had been commited with CRLF and the file is commited
> again, the CRLF are kept if .gitattributs has "text=auto".
> This is done by analyzing the content of the blob stored in the index:
> If a '\r' is found, Git assumes that the blob was commited with CRLF.
>
> The simple search for a '\r' does not always work as expected:
> A file is encoded in UTF-16 with CRLF and commited. Git treats it as binary.
> Now the content is converted into UTF-8. At the next commit Git treats the
> file as text, the CRLF should be converted into LF, but isn't.
>
> Solution:

>
> Reported-By: Ashish Negi <ashishnegi33@gmail.com>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>  convert.c            | 19 +++++++++----
>  t/t0027-auto-crlf.sh | 76 ++++++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 85 insertions(+), 10 deletions(-)
>
> diff --git a/convert.c b/convert.c
> index 20d7ab67bd..63ef799239 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -220,18 +220,27 @@ static void check_safe_crlf(const char *path, enum crlf_action crlf_action,
>  	}
>  }
>  
> -static int has_cr_in_index(const struct index_state *istate, const char *path)
> +static int has_crlf_in_index(const struct index_state *istate, const char *path)
>  {
>  	unsigned long sz;
>  	void *data;
> -	int has_cr;
> +	const char *crp;
> +	int has_crlf = 0;
>  
>  	data = read_blob_data_from_index(istate, path, &sz);
>  	if (!data)
>  		return 0;
> -	has_cr = memchr(data, '\r', sz) != NULL;
> +
> +	crp = memchr(data, '\r', sz);
> +	if (crp && (crp[1] == '\n')) {
> +		unsigned int ret_stats;
> +		ret_stats = gather_convert_stats(data, sz);
> +		if (!(ret_stats & CONVERT_STAT_BITS_BIN) &&
> +		    (ret_stats & CONVERT_STAT_BITS_TXT_CRLF))
> +			has_crlf = 1;
> +	}
>  	free(data);
> -	return has_cr;
> +	return has_crlf;
>  }
>  
>  static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,
> @@ -290,7 +299,7 @@ static int crlf_to_git(const struct index_state *istate,
>  		 * cherry-pick.
>  		 */
>  		if ((checksafe != SAFE_CRLF_RENORMALIZE) &&
> -		    has_cr_in_index(istate, path))
> +		    has_crlf_in_index(istate, path))
>  			convert_crlf_into_lf = 0;
>  	}
>  	if ((checksafe == SAFE_CRLF_WARN ||
> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> index 68108d956a..0af35cfb1f 100755
> --- a/t/t0027-auto-crlf.sh
> +++ b/t/t0027-auto-crlf.sh
> @@ -43,19 +43,31 @@ create_gitattributes () {
>  	} >.gitattributes
>  }
>  
> -create_NNO_files () {
> +# Create 2 sets of files:
> +# The NNO files are "Not NOrmalized in the repo. We use CRLF_mix_LF and store
> +#   it under different names for the different test cases, see ${pfx}
> +#   Depending on .gitattributes they are normalized at the next commit (or not)
> +# The MIX files have different contents in the repo.
> +#   Depending on its contents, the "new safer autocrlf" may kick in.
> +create_NNO_MIX_files () {
>  	for crlf in false true input
>  	do
>  		for attr in "" auto text -text
>  		do
>  			for aeol in "" lf crlf
>  			do
> -				pfx=NNO_attr_${attr}_aeol_${aeol}_${crlf}
> +				pfx=NNO_attr_${attr}_aeol_${aeol}_${crlf} &&
>  				cp CRLF_mix_LF ${pfx}_LF.txt &&
>  				cp CRLF_mix_LF ${pfx}_CRLF.txt &&
>  				cp CRLF_mix_LF ${pfx}_CRLF_mix_LF.txt &&
>  				cp CRLF_mix_LF ${pfx}_LF_mix_CR.txt &&
> -				cp CRLF_mix_LF ${pfx}_CRLF_nul.txt
> +				cp CRLF_mix_LF ${pfx}_CRLF_nul.txt &&
> +				pfx=MIX_attr_${attr}_aeol_${aeol}_${crlf} &&
> +				cp LF          ${pfx}_LF.txt &&
> +				cp CRLF        ${pfx}_CRLF.txt &&
> +				cp CRLF_mix_LF ${pfx}_CRLF_mix_LF.txt &&
> +				cp LF_mix_CR   ${pfx}_LF_mix_CR.txt &&
> +				cp CRLF_nul    ${pfx}_CRLF_nul.txt
>  			done
>  		done
>  	done
> @@ -136,6 +148,49 @@ commit_chk_wrnNNO () {
>  	'
>  }
>  
> +# Commit a file with mixed line endings on top of different files
> +# in the index. Check for warnings
> +commit_MIX_chkwrn () {
> +	attr=$1 ; shift
> +	aeol=$1 ; shift
> +	crlf=$1 ; shift
> +	lfwarn=$1 ; shift
> +	crlfwarn=$1 ; shift
> +	lfmixcrlf=$1 ; shift
> +	lfmixcr=$1 ; shift
> +	crlfnul=$1 ; shift
> +	pfx=MIX_attr_${attr}_aeol_${aeol}_${crlf}
> +	#Commit file with CLRF_mix_LF on top of existing file
> +	create_gitattributes "$attr" $aeol &&
> +	for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul
> +	do
> +		fname=${pfx}_$f.txt &&
> +		cp CRLF_mix_LF $fname &&
> +		printf Z >>"$fname" &&
> +		git -c core.autocrlf=$crlf add $fname 2>"${pfx}_$f.err"
> +	done
> +
> +	test_expect_success "commit file with mixed EOL crlf=$crlf attr=$attr LF" '
> +		check_warning "$lfwarn" ${pfx}_LF.err
> +	'
> +	test_expect_success "commit file with mixed EOL attr=$attr aeol=$aeol crlf=$crlf CRLF" '
> +		check_warning "$crlfwarn" ${pfx}_CRLF.err
> +	'
> +
> +	test_expect_success "commit file with mixed EOL attr=$attr aeol=$aeol crlf=$crlf CRLF_mix_LF" '
> +		check_warning "$lfmixcrlf" ${pfx}_CRLF_mix_LF.err
> +	'
> +
> +	test_expect_success "commit file with mixed EOL attr=$attr aeol=$aeol crlf=$crlf LF_mix_cr" '
> +		check_warning "$lfmixcr" ${pfx}_LF_mix_CR.err
> +	'
> +
> +	test_expect_success "commit file with mixed EOL attr=$attr aeol=$aeol crlf=$crlf CRLF_nul" '
> +		check_warning "$crlfnul" ${pfx}_CRLF_nul.err
> +	'
> +}
> +
> +
>  stats_ascii () {
>  	case "$1" in
>  	LF)
> @@ -323,8 +378,8 @@ test_expect_success 'setup master' '
>  	printf "\$Id: 0000000000000000000000000000000000000000 \$\r\nLINEONE\r\nLINETWO\rLINETHREE"   >CRLF_mix_CR &&
>  	printf "\$Id: 0000000000000000000000000000000000000000 \$\r\nLINEONEQ\r\nLINETWO\r\nLINETHREE" | q_to_nul >CRLF_nul &&
>  	printf "\$Id: 0000000000000000000000000000000000000000 \$\nLINEONEQ\nLINETWO\nLINETHREE" | q_to_nul >LF_nul &&
> -	create_NNO_files CRLF_mix_LF CRLF_mix_LF CRLF_mix_LF CRLF_mix_LF CRLF_mix_LF &&
> -	git -c core.autocrlf=false add NNO_*.txt &&
> +	create_NNO_MIX_files CRLF_mix_LF CRLF_mix_LF CRLF_mix_LF CRLF_mix_LF CRLF_mix_LF &&
> +	git -c core.autocrlf=false add NNO_*.txt MIX_*.txt &&
>  	git commit -m "mixed line endings" &&
>  	test_tick
>  '
> @@ -385,6 +440,17 @@ test_expect_success 'commit files attr=crlf' '
>  	commit_check_warn input "crlf" "LF_CRLF" ""        "LF_CRLF" "LF_CRLF" ""
>  '
>  
> +# Commit "CRLFmixLF" on top of these files already in the repo:
> +# LF, CRLF, CRLFmixLF LF_mix_CR CRLFNULL
> +#                 attr                    LF        CRLF      CRLFmixLF   LF_mix_CR   CRLFNUL
> +commit_MIX_chkwrn ""      ""      false   ""        ""        ""          ""          ""
> +commit_MIX_chkwrn ""      ""      true    "LF_CRLF" ""        ""          "LF_CRLF"   "LF_CRLF"
> +commit_MIX_chkwrn ""      ""      input   "CRLF_LF" ""        ""          "CRLF_LF"   "CRLF_LF"
> +
> +commit_MIX_chkwrn "auto"  ""      false   "CRLF_LF" ""        ""          "CRLF_LF"   "CRLF_LF"
> +commit_MIX_chkwrn "auto"  ""      true    "LF_CRLF" ""        ""          "LF_CRLF"   "LF_CRLF"
> +commit_MIX_chkwrn "auto"  ""      input   "CRLF_LF" ""        ""          "CRLF_LF"   "CRLF_LF"
> +
>  #                 attr                    LF        CRLF      CRLFmixLF   LF_mix_CR   CRLFNUL
>  commit_chk_wrnNNO ""      ""      false   ""        ""        ""          ""          ""
>  commit_chk_wrnNNO ""      ""      true    LF_CRLF   ""        ""          ""          ""

> From: Torsten Bögershausen <tboegi@web.de>
>
> Replace `git commit -m "comment" ""` with `git commit -m "comment"` to
> remove the empty path spec.
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>  t/t0027-auto-crlf.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> index c2c208fdcd..97154f5c79 100755
> --- a/t/t0027-auto-crlf.sh
> +++ b/t/t0027-auto-crlf.sh
> @@ -370,7 +370,7 @@ test_expect_success 'setup master' '
>  	echo >.gitattributes &&
>  	git checkout -b master &&
>  	git add .gitattributes &&
> -	git commit -m "add .gitattributes" "" &&
> +	git commit -m "add .gitattributes" &&
>  	printf "\$Id: 0000000000000000000000000000000000000000 \$\nLINEONE\nLINETWO\nLINETHREE"     >LF &&
>  	printf "\$Id: 0000000000000000000000000000000000000000 \$\r\nLINEONE\r\nLINETWO\r\nLINETHREE" >CRLF &&
>  	printf "\$Id: 0000000000000000000000000000000000000000 \$\nLINEONE\r\nLINETWO\nLINETHREE"   >CRLF_mix_LF &&

> tboegi@web.de writes:
>
>> From: Torsten Bögershausen <tboegi@web.de>
>>
>> Replace `git commit -m "comment" ""` with `git commit -m "comment"` to
>> remove the empty path spec.
>>
>> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
>> ---
>>  t/t0027-auto-crlf.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> This looks a bit strange.  The intent seems to commit all changes
> made in the working tree, so I'd understand it if it replaced the
> empty string with a single dot.
>
> I also thought that we deprecated use of an empty string as "match
> all" pathspec recently, and expected that this to break.
>
>     ... goes and looks ...
>
> Indeed, 229a95aa ("t0027: do not use an empty string as a pathspec
> element", 2017-06-23) does exactly that.

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > tboegi@web.de writes:
> >
> >> From: Torsten Bögershausen <tboegi@web.de>
> >>
> >> Replace `git commit -m "comment" ""` with `git commit -m "comment"` to
> >> remove the empty path spec.
> >>
> >> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> >> ---
> >>  t/t0027-auto-crlf.sh | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > This looks a bit strange.  The intent seems to commit all changes
> > made in the working tree, so I'd understand it if it replaced the
> > empty string with a single dot.
> >
> > I also thought that we deprecated use of an empty string as "match
> > all" pathspec recently, and expected that this to break.
> >
> >     ... goes and looks ...
> >
> > Indeed, 229a95aa ("t0027: do not use an empty string as a pathspec
> > element", 2017-06-23) does exactly that.
> 
> OK, I think I can safely omit this patch, because
> 
>  (1) when 2/2 is queued on top of tb/check-crlf-for-safe-crlf,
>      because ex/deprecate-empty-pathspec-as-match-all is not yet in
>      the topic, an empty pathspec still means "match all" and
>      nothing breaks; and
> 
>  (2) when tb/check-crlf-for-safe-crlf plus 2/2 is merged to any
>      branch with ex/deprecate-empty-pathspec-as-match-all, the topic
>      already fixes what this 1/2 tries to
> 
> Thanks for being thorough, though.
>