Re: [Bug] [Subversion 1.7] svn blame doesn't work for locally modified files

On Fri, Oct 14, 2011 at 6:49 AM, Daniel Shahaf <d.s_at_daniel.shahaf.name> wrote:
> [ switching list, please drop users@ from further replies ]
>
> Konstantin Kolinko wrote on Fri, Oct 14, 2011 at 11:43:14 +0400:
>> 2011/10/14 Andrey Paramonov <paramon_at_acdlabs.ru>:
>> > On 13.10.2011 22:31, Ryan Schmidt wrote:
>> >>
>> >> On Oct 13, 2011, at 06:51, Andrey Paramonov wrote:
>> >>
>> >>>>> I believe Subversion can automagically translate line ending sequences
>> >>>>> when transferring data to and from server. I use Windows, so I have CRLF
>> >>>>> sequences at the ends of the lines in my working copy. The question is: what
>> >>>>> is the EOL sequence on the server?
>> >>
>> >>
>> >> On Oct 13, 2011, at 07:06, Andrey Paramonov wrote:
>> >>>
>> >>> On 13.10.2011 15:54, Daniel Shahaf wrote:
>> >>>>
>> >>>> What properties are set on your file?
>> >>>
>> >>> svn proplist returns nothing.
>> >>
>> >> If the svn:eol-style property is not set on the file, then Subversion does
>> >> not modify the file in any way as it's stored in the repository. The EOL
>> >> sequence (and every other byte in the file) is the same on the server as it
>> >> is on the client.
>> >>
>> >> If, on the other hand, svn:eol-style is set to any valid value, then
>> >> Subversion stores the file in the repository with LF line endings, and on
>> >> checkout or export, the client translates the line endings to the style
>> >> requested in the svn:eol-style property.
>> >>
>> >
>> > By looking through the code in libsvn_client/blame.c I see that in
>> > svn_client_blame5 working copy version of the file is unconditionally
>> > normalized to have "\n" EOLs (by call to svn_subst_stream_translated).
>> > However, it seems that svn_ra_get_file_revs2 doesn't do EOL normalization,
>> > at least in my case (absent svn:eol-style).
>>
>> Confirming that the issue reproduces for me on Windows using a file
>> that does not have svn:eol-style property.
>>
>> The svn blame filename_at_BASE, as mentioned earlier, works correctly,
>> blaming unmodified base version of the file.
>>
>>
>> If a file has svn:eol-style=native the issue does not reproduce
>> and blame works correctly.
>>
>>
>
> @Both, thanks for the analysis. Could someone file a bug report and/or
> send a patch, too? If you have a build environment, does the patch
> below fix the problem?

Hi All,

Mike Pilato is tying off loose ends for the upcoming 1.8 release and
asked me to check this patch for issue #4034 on my Windows machine.

The patch doesn't appear to make any difference:

With three files, with three different eols as per their names:

trunk_at_1403813>svn st -v
                 9 9 pburba .
                 9 9 pburba file-with-CR-eols
                 9 9 pburba file-with-CRLF-eols
                 9 9 pburba file-with-LF-eols

All three files have the same contents but for eols:

trunk_at_1403813>type file-with-CRLF-eols
A
B
C
D
E
F

Blame works fine with no local mods as expected:

trunk_at_1403813>svn blame file-with-CR-eols
     9 pburba A
     9 pburba B
     9 pburba C
     9 pburba D
     9 pburba E
     9 pburba F

trunk_at_1403813>svn blame file-with-CRLF-eols
     9 pburba A
     9 pburba B
     9 pburba C
     9 pburba D
     9 pburba E
     9 pburba F

trunk_at_1403813>svn blame file-with-LF-eols
     9 pburba A
     9 pburba B
     9 pburba C
     9 pburba D
     9 pburba E
     9 pburba F

I edited line #3 in each file like this:

trunk_at_1403813>svn diff file-with-CRLF-eols

Index: file-with-CRLF-eols
===================================================================
--- file-with-CRLF-eols (revision 9)
+++ file-with-CRLF-eols (working copy)
@@ -1,6 +1,6 @@
 A
 B
-C
+C-mod
 D
 E
 F

Without Daniels patch we see the bug described previously in the thread:

trunk_at_1403813>svn blame file-with-CR-eols
     - - A
     - - B
     - - C-mod
     - - D
     - - E
     - - F

trunk_at_1403813>svn blame file-with-CRLF-eols
     - - A
     - - B
     - - C-mod
     - - D
     - - E
     - - F

trunk_at_1403813>svn blame file-with-LF-eols
     9 pburba A
     9 pburba B
     - - C-mod
     9 pburba D
     9 pburba E
     9 pburba F

But with Daniel's patch I see no difference in the blame output:

trunk_at_1403813>svn blame file-with-CR-eols
     - - A
     - - B
     - - C-mod
     - - D
     - - E
     - - F

trunk_at_1403813>svn blame file-with-CRLF-eols
     - - A
     - - B
     - - C-mod
     - - D
     - - E
     - - F

trunk_at_1403813>svn blame file-with-LF-eols
     9 pburba A
     9 pburba B
     - - C-mod
     9 pburba D
     9 pburba E
     9 pburba F

-- 
Paul T. Burba
CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
Skype: ptburba
> [[[
> Index: subversion/libsvn_client/blame.c
> ===================================================================
> --- subversion/libsvn_client/blame.c    (revision 1182771)
> +++ subversion/libsvn_client/blame.c    (working copy)
> @@ -473,6 +473,9 @@
>                                   svn_io_file_del_on_pool_cleanup,
>                                   filepool, filepool));
>
> +  cur_stream = svn_subst_stream_translated(cur_stream, "\n", TRUE, NULL,
> +                                           FALSE, filepool);
> +
>    /* Get window handler for applying delta. */
>    svn_txdelta_apply(last_stream, cur_stream, NULL, NULL,
>                      frb->currpool,
> ]]]
>
>> >
>> > This explains why it works on *nix regardless of whether svn:eol-style is
>> > set. On *nix, the data has "\n" EOLs in the repository. I believe you'll be
>> > able to reproduce the problem on *nix too if you commit a file with CRLF
>> > line endings. Also, I see that svn blame works correctly for modified files
>> > on my Windows system for files having LF line endings.
>> >
>> > I'm not sure what's the best way to resolve the issue. For me, just removing
>> > the call to svn_subst_stream_translated would fix the problem, I think. But
>> > it wouldn't always be TRT in presence of svn:eol-style.
>> >
>> > Anyway, please consider reverting svn blame behavior without explicit
>> > @REVISION as the last resort for 1.7.1.
>> >
>>
>> Best regards,
>> Konstantin Kolinko