Re: [PATCH v8 10/10] ce_compare_data() did not respect conversion

From: Junio C Hamano <gitster@pobox.com>
Date: May 2, 2016, 19:33 UTC
Message-ID: <xmqqpot4s1ap.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqtwigtjfg.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 18 quoted lines

> Torsten Bögershausen <tboegi@web.de> writes:
>
>> On 29.04.16 23:09, Junio C Hamano wrote:
>>
>>> Well, didn't I do exactly the above much earlier and discarded it
>>> because that breaks the definition of "diff"?  Or is this doing
>>> something differently?
>>
>> Yes, and I try to sneak it in anyway ;-)
>>
>> I spend some time debugging how to get t6038 passed, and need
>> some more time.
>>
>> If 10/10 is a no-go (and it probably should be),
>> does it make sense to keep 1/10..4/10 and discard 5..10 for the moment ?
>
> Earlier patches in the series certainly felt alright.  I do not
> remember noticing where it went in a strange direction to be honest.

Let's step back a bit and make sure we are on the same page. I think this "series" conflates a bit too many things into a single topic.

 * The comparison between the index and the working tree, i.e. "git
   diff", should compare result of convert_to_git() with what is in
   the index, and the world around it should be made consistent with
   that.  Your separate "git blame" fix to add missing knowledge
   that convert_to_git() would not do s/CRLF/LF/ for a path whose
   index entry already is contaminated with CR falls into this
   category and was a very good thing to do.

 * A convert_to_git() and convert_to_working_tree() pair that do not
   roundtrip would (by definition) leave contents in the working
   tree, that, when passed through convert_to_git(), will be
   different from the index, upon completion of "reset --hard".  We
   _should_ fix it so that "git diff" _reports_ differences.
   Currently, lstat(2) based optimization hides this in a racy way
   (when racy Git kicks in to reinspect the index and the working
   tree file actually matches, it finds out that they do not match),
   it is a bug that needs to be fixed, not 10/10 where it tries to
   hide the differences consistently and spreads the bug.  I haven't
   studied 8/10 carefully yet, but it seems to attempt the same.

 * I think the "text=auto eol=THIS" that did not mean "I do not care
   to specify which ones are text files.  Please detect the file
   type, and for those automatically detected, please make sure that
   the contents follwo THIS eol convention." was a bug, and what
   07/10 tries to do is a good thing.

By the way, lack of the cover letter of this series made it more painful to write a reply like this than necessary. A cover letter for a trivial 3-patch series might be overkill, but for anything with substance that spans more than 4-5 patches, a cover letter to describe the overall direction would really help.

Thanks.