[BUG] attribute "eol" with "crlf"

> There's a bug in git-1.7.8 if you use the attribute "eol" with "crlf".
>
> Steps to reproduce:
> - add and commit a text file which uses 0d0a for line breaks
> 7465 7374 0d0a 0d0a 7465 7374 0d0a       test....test..
> - add ".gitattributes" with "*.txt eol=crlf"
> - change a line in the file
> - execute "git checkout [file]"
>
> The result is:
> 7465 7374 0d0d 0a0d 0d0a 7465 7374 0d0d  test......test..

> Ralf Thielow <ralf.thielow@googlemail.com> writes:
>
>> There's a bug in git-1.7.8 if you use the attribute "eol" with "crlf".
>>
>> Steps to reproduce:
>> - add and commit a text file which uses 0d0a for line breaks
>> 7465 7374 0d0a 0d0a 7465 7374 0d0a       test....test..
>> - add ".gitattributes" with "*.txt eol=crlf"
>> - change a line in the file
>> - execute "git checkout [file]"
>>
>> The result is:
>> 7465 7374 0d0d 0a0d 0d0a 7465 7374 0d0d  test......test..
>
> It seems to me to be the expected behavior. You committed a file whose
> line endings are not normalized to LF in the repository, and asked for a
> conversion LF -> CRLF on checkout, which Git did.
>
> Git can't know exactly the moment when you edit .gitattributes, so it
> can't do the conversion at the time you add the eol=crlf attribute. It
> does it on checkout.
>
>> 0d0a was replaced by 0d0d0a.
>
> I'd say 0a (LF) was replaced by 0d0a (CRLF).
>
> What behavior would you have expected?

> The sequence adds "test\r\n" file without .gitattributes to have the
> repository record that exact byte sequence for the file. But then later
> goes around and says "This file wants to express the end of line with CRLF
> on the filesystem, so please replace LF in the repository representation
> to CRLF when checking out, and replace CRLF in the working tree to LF when
> checking in".
>
> So it is not surprising that "\r\n" coming from the repository is replaced
> to "\r\r\n" when checked out. As far as the repository data is concerned,
> that line has a funny byte with value "\r" at the end, immediately before
> the line terminator "\n".
>
> What you said is _technically_ correct in that sense.
>
> However, I think the CRLF filter used to have a hack to strip "\r" if the
> repository data records "\r" at the end of line. This was intended to help
> people who checked in such a broken text file (if it is a text file, then
> raw ascii CR does not have a place in it in the repository representation)
> and it was a useful hack to help people recover from such mistakes to
> start the project from DOS-only world (with CRLF in the repository data)
> and migrate to cross platform world (with LF in the repository data, CRLF
> in the DOS working tree).  I suspect that the streaming filter conversion
> may not have the same hack in it.

>> The sequence adds "test\r\n" file without .gitattributes to have the
>> repository record that exact byte sequence for the file. But then later
>> goes around and says "This file wants to express the end of line with CRLF
>> on the filesystem, so please replace LF in the repository representation
>> to CRLF when checking out, and replace CRLF in the working tree to LF when
>> checking in".
>>
>> So it is not surprising that "\r\n" coming from the repository is replaced
>> to "\r\r\n" when checked out. As far as the repository data is concerned,
>> that line has a funny byte with value "\r" at the end, immediately before
>> the line terminator "\n".
>>
>> What you said is _technically_ correct in that sense.
>>
>> However, I think the CRLF filter used to have a hack to strip "\r" if the
>> repository data records "\r" at the end of line. This was intended to help
>> people who checked in such a broken text file (if it is a text file, then
>> raw ascii CR does not have a place in it in the repository representation)
>> and it was a useful hack to help people recover from such mistakes to
>> start the project from DOS-only world (with CRLF in the repository data)
>> and migrate to cross platform world (with LF in the repository data, CRLF
>> in the DOS working tree).  I suspect that the streaming filter conversion
>> may not have the same hack in it.

> Ralf Thielow <ralf.thielow@googlemail.com> writes:
>
>> So i have to commit ".gitattributes" and everything is fine for me after!?
>
> No.  Sorry if I was unclear, but I do not see which part was unclear in
> what I wrote, so...
>
>>> The sequence adds "test\r\n" file without .gitattributes to have the
>>> repository record that exact byte sequence for the file. But then later
>>> goes around and says "This file wants to express the end of line with CRLF
>>> on the filesystem, so please replace LF in the repository representation
>>> to CRLF when checking out, and replace CRLF in the working tree to LF when
>>> checking in".
>>>
>>> So it is not surprising that "\r\n" coming from the repository is replaced
>>> to "\r\r\n" when checked out. As far as the repository data is concerned,
>>> that line has a funny byte with value "\r" at the end, immediately before
>>> the line terminator "\n".
>>>
>>> What you said is _technically_ correct in that sense.
>>>
>>> However, I think the CRLF filter used to have a hack to strip "\r" if the
>>> repository data records "\r" at the end of line. This was intended to help
>>> people who checked in such a broken text file (if it is a text file, then
>>> raw ascii CR does not have a place in it in the repository representation)
>>> and it was a useful hack to help people recover from such mistakes to
>>> start the project from DOS-only world (with CRLF in the repository data)
>>> and migrate to cross platform world (with LF in the repository data, CRLF
>>> in the DOS working tree).  I suspect that the streaming filter conversion
>>> may not have the same hack in it.

> 2011/12/16 Junio C Hamano <gitster@pobox.com>:
>> Ralf Thielow <ralf.thielow@googlemail.com> writes:
>>
>>> So i have to commit ".gitattributes" and everything is fine for me after!?
>>
>> No.  Sorry if I was unclear, but I do not see which part was unclear in
>> what I wrote, so...
>>
>>>> The sequence adds "test\r\n" file without .gitattributes to have the
>>>> repository record that exact byte sequence for the file. But then later
>>>> goes around and says "This file wants to express the end of line with CRLF
>>>> on the filesystem, so please replace LF in the repository representation
>>>> to CRLF when checking out, and replace CRLF in the working tree to LF when
>>>> checking in".
>>>>
>>>> So it is not surprising that "\r\n" coming from the repository is replaced
>>>> to "\r\r\n" when checked out. As far as the repository data is concerned,
>>>> that line has a funny byte with value "\r" at the end, immediately before
>>>> the line terminator "\n".
>>>>
>>>> What you said is _technically_ correct in that sense.
>>>> ...

> ...
> What you said is _technically_ correct in that sense.
>
> However, I think the CRLF filter used to have a hack to strip "\r" if the
> repository data records "\r" at the end of line. This was intended to help
> people who checked in such a broken text file (if it is a text file, then
> raw ascii CR does not have a place in it in the repository representation)
> and it was a useful hack to help people recover from such mistakes to
> start the project from DOS-only world (with CRLF in the repository data)
> and migrate to cross platform world (with LF in the repository data, CRLF
> in the DOS working tree).  I suspect that the streaming filter conversion
> may not have the same hack in it.

> Junio C Hamano <gitster@pobox.com> writes:
>
>> ...
>> What you said is _technically_ correct in that sense.
>>
>> However, I think the CRLF filter used to have a hack to strip "\r" if the
>> repository data records "\r" at the end of line. This was intended to help
>> people who checked in such a broken text file (if it is a text file, then
>> raw ascii CR does not have a place in it in the repository representation)
>> and it was a useful hack to help people recover from such mistakes to
>> start the project from DOS-only world (with CRLF in the repository data)
>> and migrate to cross platform world (with LF in the repository data, CRLF
>> in the DOS working tree).  I suspect that the streaming filter conversion
>> may not have the same hack in it.
>
> Perhaps something like this, but I do not use CRLF myself, so it probably
> needs to be checked by extra sets of eyes.
>
> Thanks.
>
> -- >8 --
> Subject: lf_to_crlf_filter(): resurrect CRLF->CRLF hack
>
> The non-streaming version of the filter counts CRLF and LF in the whole
> buffer, and returns without doing anything when they match (i.e. what is
> recorded in the object store already uses CRLF). This was done to help
> people who added files from the DOS world before realizing they want to go
> cross platform and adding .gitattributes to tell Git that they only want
> CRLF in their working tree.
>
> The streaming version of the filter does not want to read the whole thing
> before starting to work, as that defeats the whole point of streaming. So
> we instead check what byte follows CR whenever we see one, and add CR
> before LF only when the LF does not immediately follow CR already to keep
> CRLF as is.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  convert.c |   60 ++++++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 files changed, 50 insertions(+), 10 deletions(-)
>
> diff --git a/convert.c b/convert.c
> index c028275..8daf4e4 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -879,7 +879,8 @@ int is_null_stream_filter(struct stream_filter *filter)
>
>  struct lf_to_crlf_filter {
>        struct stream_filter filter;
> -       unsigned want_lf:1;
> +       unsigned has_held:1;
> +       char held;
>  };
>
>  static int lf_to_crlf_filter_fn(struct stream_filter *filter,
> @@ -889,10 +890,14 @@ static int lf_to_crlf_filter_fn(struct stream_filter *filter,
>        size_t count, o = 0;
>        struct lf_to_crlf_filter *lf_to_crlf = (struct lf_to_crlf_filter *)filter;
>
> -       /* Output a pending LF if we need to */
> -       if (lf_to_crlf->want_lf) {
> -               output[o++] = '\n';
> -               lf_to_crlf->want_lf = 0;
> +       /*
> +        * We may be holding onto the CR to see if it is followed by a
> +        * LF, in which case we would need to go to the main loop.
> +        * Otherwise, just emit it to the output stream.
> +        */
> +       if (lf_to_crlf->has_held && (lf_to_crlf->held != '\r' || !input)) {
> +               output[o++] = lf_to_crlf->held;
> +               lf_to_crlf->has_held = 0;
>        }
>
>        /* We are told to drain */
> @@ -902,22 +907,57 @@ static int lf_to_crlf_filter_fn(struct stream_filter *filter,
>        }
>
>        count = *isize_p;
> -       if (count) {
> +       if (count || lf_to_crlf->has_held) {
>                size_t i;
> +               int was_cr = 0;
> +
> +               if (lf_to_crlf->has_held) {
> +                       was_cr = 1;
> +                       lf_to_crlf->has_held = 0;
> +               }
> +
>                for (i = 0; o < *osize_p && i < count; i++) {
>                        char ch = input[i];
> +
>                        if (ch == '\n') {
>                                output[o++] = '\r';
> -                               if (o >= *osize_p) {
> -                                       lf_to_crlf->want_lf = 1;
> -                                       continue; /* We need to increase i */
> -                               }
> +                       } else if (was_cr) {
> +                               /*
> +                                * Previous round saw CR and it is not followed
> +                                * by a LF; emit the CR before processing the
> +                                * current character.
> +                                */
> +                               output[o++] = '\r';
>                        }
> +
> +                       /*
> +                        * We may have consumed the last output slot,
> +                        * in which case we need to break out of this
> +                        * loop; hold the current character before
> +                        * returning.
> +                        */
> +                       if (*osize_p <= o) {
> +                               lf_to_crlf->has_held = 1;
> +                               lf_to_crlf->held = ch;
> +                               continue; /* break but increment i */
> +                       }
> +
> +                       if (ch == '\r') {
> +                               was_cr = 1;
> +                               continue;
> +                       }
> +
> +                       was_cr = 0;
>                        output[o++] = ch;
>                }
>
>                *osize_p -= o;
>                *isize_p -= i;
> +
> +               if (!lf_to_crlf->has_held && was_cr) {
> +                       lf_to_crlf->has_held = 1;
> +                       lf_to_crlf->held = '\r';
> +               }
>        }
>        return 0;
>  }

>> Perhaps something like this, but I do not use CRLF myself, so it probably
>> needs to be checked by extra sets of eyes.
>>
>> Thanks.
>>
>
> Works fine for me. Thanks