On Tue, 12 Jan 2010, Igor Stasenko wrote:

> 2010/1/12 Levente Uzonyi <leves at elte.hu>:
>> On Tue, 12 Jan 2010, Igor Stasenko wrote:
>>
>>> 2010/1/12 Levente Uzonyi <leves at elte.hu>:
>>>>
>>>> On Mon, 11 Jan 2010, Nicolas Cellier wrote:
>>>>
>>>>> Hi Levente,
>>>>> what about completely ignoring line endings in diffs ?
>>>>>
>>>>
>>>> I intentionally added this feature. Do you think it's wrong?
>>>>
>>>>
>>> IMO empty lines (and white space in general), is not an informal part
>>> of source code,
>>> so diffing them makes not much sense.
>>
>> Imagine that you removed lf characters from the code or you accidentally
>> added some linefeeds while pasting code from and external source. The diff
>> shows no changes. Is that OK?
>>
>
> If the new lines is informal part of source code, i.e. belong to the
> string literal:
>
> foo := '1
> 2
> 3
>
> 4
> '.
>
> Then we should care. Otherwise not.

I care and we should. Newlines make a difference. I don't want to decode 
one liners like:

at: key put: anObject | index assoc | index := self scanFor: key. assoc := array at: index. assoc ifNil: [self atNewIndex: index put: (Association key: key value: anObject)] 	ifNotNil: [assoc value: anObject]. ^anObject

It's much easier to read this one:

at: key put: anObject

    | index assoc |
    index := self scanFor: key.
    assoc := array at: index.
    assoc
       ifNil: [ self atNewIndex: index put: (Association key: key value: anObject) ]
       ifNotNil: [ assoc value: anObject ].
    ^anObject

For me, every whitespace counts in the above method, but the original 
question was not this, but:
Should the diff algorithm care about the line endings or not?
Are these three lines identical or not (in a diff):
foo bar<cr><lf>
foo bar<cr>
foo bar<lf>


Levente

>
>
>>
>> Levente
>>
>>>
>>>> Levente
>>>>
>>>>> split: aString
>>>>>        "I return an Array of strings which are the lines extracted from
>>>>> aString. All lines contain the line separator characters"
>>>>>
>>>>>        ^Array streamContents: [ :stream |
>>>>>                aString lineIndicesDo: [ :start :endWithoutSeparators
>>>>> :end
>>>>> |
>>>>>                        stream nextPut: (aString copyFrom: start to:
>>>>> endWithoutSeparators) ] ]
>>>>>
>>>>> or simply
>>>>>
>>>>>
>>>>> split: aString
>>>>>        "I return an Array of strings which are the lines extracted from
>>>>> aString. All lines contain the line separator characters"
>>>>>
>>>>>        ^Array streamContents: [ :stream |
>>>>>                aString linesDo: [ :aLineWithoutEnding |
>>>>>                        stream nextPut: aLineWithoutEnding ] ]
>>>>>
>>>>> Nicolas
>>>>>
>>>>> 2010/1/11 Andreas Raab <andreas.raab at gmx.de>:
>>>>>>
>>>>>> Levente Uzonyi wrote:
>>>>>>>
>>>>>>> On Sun, 10 Jan 2010, Andreas Raab wrote:
>>>>>>>>
>>>>>>>> I think I'll leave that decision to you, you seem to have a good
>>>>>>>> handle
>>>>>>>> on this part of the system. FWIW, I had implemented option #2 for our
>>>>>>>> SqueakSource installation realizing that it would be robust even if
>>>>>>>> we
>>>>>>>> decided to leave out the CRs from the diff.
>>>>>>>
>>>>>>> With the latest TextDiffBuilder changes everything should work fine
>>>>>>> with
>>>>>>> all versions of SqueakSource. #buildTextPatch is SqueakSource's
>>>>>>> extension
>>>>>>> method, changing that would break backwards compatibility.
>>>>>>
>>>>>> Yup, agreed. It was a quick localized fix for our installation. I
>>>>>> didn't
>>>>>> feel like messing around with TextDiffBuilder itself - making the
>>>>>> change
>>>>>> in
>>>>>> the one extension method felt safer for our purposes. Thanks for fixing
>>>>>> the
>>>>>> issue at the root!
>>>>>>
>>>>>> Cheers,
>>>>>>  - Andreas
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>> --
>>> Best regards,
>>> Igor Stasenko AKA sig.
>>>
>>
>>
>>
>>
>
>
>
> -- 
> Best regards,
> Igor Stasenko AKA sig.
>
>