Blame showing all lines as NCY #260

Activity

1. frogonwheels

   self-assigned this

   on Oct 3, 2014
2. t-b commented on Oct 3, 2014

   

   t-b

   on Oct 3, 2014

   More actions

   Thanks for taking the task!

   I can't reprocude that here though:

   thomas@THOMAS-WIN7-X64 /git (master)
   $ git blame convert.h | head
   d1bf0e08 (Junio C Hamano 2011-05-20 12:59:01 -0700  1) /*
   d1bf0e08 (Junio C Hamano 2011-05-20 12:59:01 -0700  2)  * Copyright (c) 2011, Google Inc.
   d1bf0e08 (Junio C Hamano 2011-05-20 12:59:01 -0700  3)  */
   d1bf0e08 (Junio C Hamano 2011-05-20 12:59:01 -0700  4) #ifndef CONVERT_H
   d1bf0e08 (Junio C Hamano 2011-05-20 12:59:01 -0700  5) #define CONVERT_H
   d1bf0e08 (Junio C Hamano 2011-05-20 12:59:01 -0700  6)
   d1bf0e08 (Junio C Hamano 2011-05-20 12:59:01 -0700  7) enum safe_crlf {
   d1bf0e08 (Junio C Hamano 2011-05-20 12:59:01 -0700  8)  SAFE_CRLF_FALSE = 0,
   d1bf0e08 (Junio C Hamano 2011-05-20 12:59:01 -0700  9)  SAFE_CRLF_FAIL = 1,
   d1bf0e08 (Junio C Hamano 2011-05-20 12:59:01 -0700 10)  SAFE_CRLF_WARN = 2
   
   thomas@THOMAS-WIN7-X64 /git (master)
   $ git config core.autocrlf
   true
   
   thomas@THOMAS-WIN7-X64 /git (master)
   $ git config core.eol
   
   
3. frogonwheels commented on Nov 10, 2014

   

   frogonwheels

   on Nov 10, 2014

   Author

   More actions

   I've finally found some time to work on this:

   Try with
   $ git config core.autocrlf=false
$ git config core.eol=crlf

   I'm assuming you agree that this should work?

   This also has similar issues with diff. The weird thing is that I can see where diff does it's crlf translation - and blame is using the textconv code from diff, but doesn't seem to be using the crlf translation code! I'm probably missing something though. Anyway, I've got a couple of things on the way.
4. dscho commented on Nov 10, 2014

   

   dscho

   on Nov 10, 2014

   Member

   More actions

   This probably happens with Vagrant, too?
5. frogonwheels commented on Nov 10, 2014

   

   frogonwheels

   on Nov 10, 2014

   Author

   More actions

   I haven't used Vagrant, so can't comment.

   What I've found is that in git diff, the crlf_action is set to CRLF_GUESS and so, because_ autocrlf_ is False, the crlf conversion is skipped. I've fixed this by setting it from CRLF_GUESS to CRLF_CRLF in the case where core_eol is EOL_CRLF (convert_attrs). (*note I found a simpler fix than this)

   The next issue is that in blame, it appears that diff_populate_filespec() is not used, which means that convert_to_git() is not called (which does the crlf conversion in diff). This can be fixed by calling it in `textconv_object()in a similar way tofill_textconv`` which is used in diff.c
6. frogonwheels commented on Nov 10, 2014

   

   frogonwheels

   on Nov 10, 2014

   Author

   More actions

   Wow.. without looking at the history, I'm guessing there's been a restructure of all these functions in diff.c, forgetting that they are used in blame.c. It's assumed within diff code, that the lifetime of `diff_filespec *dfis longer than the lifetime of the buffer returned infill_textconv``. Moreover, this value returned in``*outbuf``can be static (const) or from``df``, which in turn can either be allocated memory or a memory-mapped file. The problem with using this memory in``blame.c`` is that all of these issues are ignored, and the assumption made that the returned memory is allocated and thus can be assigned to a stringbuf. In the case where there's a textconv, this is freed before being passed back anyway! Youch. Am I missing something?
7. frogonwheels commented on Nov 11, 2014

   

   frogonwheels

   on Nov 11, 2014

   Author

   More actions

   I've put changes up here:

   https://github.com/frogonwheels/git/tree/mrg/blame-crlf-v1

   What I seem to have missed is how blame worked at all. When constructing the '00000000' fake commit which is the current working file, if the textconv failed (ie there wasn't one), then it seemed to just stream the file straight to memory! This is what I was seeing. Maybe in those other cases the textconv was behaving differently somehow.
8. frogonwheels commented on Dec 23, 2014

   

   frogonwheels

   on Dec 23, 2014

   Author

   More actions

   There were still a few problems, so I've made a new version

   https://github.com/frogonwheels/git/tree/mrg/blame-crlf-v2

   Some feedback on the filespec blob commits would be good. It seems like there's some potential for quite bad behaviour there, as well as just missing out on the conversion. I guess this is also one for upstream.

   Now the only time I get issues is when I have binary nulls in my text file, and I'm happy to file that under expected behaviour.. though it's rather hard to diagnose (as in I didn't work it out until I was in the debugger and it triggered the 'is binary' heuristic).
9. dscho commented on Dec 23, 2014

   

   dscho

   on Dec 23, 2014

   Member

   More actions

   @frogonwheels I commented a bit on your v2. Please note, though, that it might be better to use GitHub to keep track of the current Pull Request version than to add -v1, -v2, etc to branch names (it is too easy to lose track).
10. dscho commented on Aug 24, 2015

    

    dscho

    on Aug 24, 2015

    Member

    More actions

    @frogonwheels would you terribly mind switching to Git for Windows 2.x' SDK, rebase your branch, and open a Pull Request? Even if you think there is more work to be done; in that case, at least others can see your current state and help out.
11. dscho

    closed this as completedon Aug 24, 2015