Discussion:
The Trunk: Traits-ar.278.mcz
(too old to reply)
commits at source.squeak.org ()
2012-01-28 12:31:06 UTC
Permalink
Andreas Raab uploaded a new version of Traits to project The Trunk:
http://source.squeak.org/trunk/Traits-ar.278.mcz

==================== Summary ====================

Name: Traits-ar.278
Author: ar
Time: 9 January 2010, 5:57:39.959 pm
UUID: 55a356f8-f654-df4d-a991-dd6ccc464fbe
Ancestors: Traits-nice.277

Adds a class comment to TraitOrganizer (really this is to test the updated server image).

=============== Diff against Traits-nice.277 ===============

Item was changed:
ClassOrganizer subclass: #TraitOrganizer

instanceVariableNames: 'traitComposition'

classVariableNames: ''

poolDictionaries: ''

category: 'Traits-Kernel'!

+

+ !TraitOrganizer commentStamp: 'ar 1/9/2010 17:56' prior: 0!

+ A class organizer containing state for traits.!
Levente Uzonyi
2012-01-28 12:31:06 UTC
Permalink
Post by commits at source.squeak.org ()
http://source.squeak.org/trunk/Traits-ar.278.mcz
==================== Summary ====================
Name: Traits-ar.278
Author: ar
Time: 9 January 2010, 5:57:39.959 pm
UUID: 55a356f8-f654-df4d-a991-dd6ccc464fbe
Ancestors: Traits-nice.277
Adds a class comment to TraitOrganizer (really this is to test the updated server image).
I tried to log in, but something went wrong:
Request handling aborted; reload to retry


Levente
Post by commits at source.squeak.org ()
=============== Diff against Traits-nice.277 ===============
ClassOrganizer subclass: #TraitOrganizer
instanceVariableNames: 'traitComposition'
classVariableNames: ''
poolDictionaries: ''
category: 'Traits-Kernel'!
+
+ !TraitOrganizer commentStamp: 'ar 1/9/2010 17:56' prior: 0!
+ A class organizer containing state for traits.!
Andreas Raab
2012-01-28 12:31:06 UTC
Permalink
Post by Levente Uzonyi
Request handling aborted; reload to retry
I was trying to understand what goes wrong but how do you debug code
that relies intrinsically on continuations? I'm really at a loss here;
this works when I run it on my windows box but fails on
source.squeak.org. Any ideas what to try next? If not, I'll have to
revert to the old version.

Cheers,
-Andreas
Levente Uzonyi
2012-01-28 12:31:06 UTC
Permalink
Post by Levente Uzonyi
Request handling aborted; reload to retry
I was trying to understand what goes wrong but how do you debug code that
relies intrinsically on continuations? I'm really at a loss here; this works
when I run it on my windows box but fails on source.squeak.org. Any ideas
what to try next? If not, I'll have to revert to the old version.
What's the curret ErrorHandler class?
WADebugErrorHandler (opens a debugger) or SSMailErrorHandler (sends a
mail to the superuser) can help tracking the issue.


Levente
Cheers,
-Andreas
Andreas Raab
2012-01-28 12:31:06 UTC
Permalink
Post by Levente Uzonyi
What's the curret ErrorHandler class?
WADebugErrorHandler (opens a debugger) or SSMailErrorHandler (sends a
mail to the superuser) can help tracking the issue.
These classes aren't even present in our current version.
See http://source.squeak.org/ss.html

Cheers,
- Andreas
Levente Uzonyi
2012-01-28 12:31:06 UTC
Permalink
Post by Andreas Raab
Post by Levente Uzonyi
What's the curret ErrorHandler class?
WADebugErrorHandler (opens a debugger) or SSMailErrorHandler (sends a mail
to the superuser) can help tracking the issue.
These classes aren't even present in our current version.
See http://source.squeak.org/ss.html
I see, it's too old. There's WAEmailErrorPage.


Levente
Post by Andreas Raab
Cheers,
- Andreas
Andreas Raab
2012-01-28 12:31:06 UTC
Permalink
Post by Levente Uzonyi
Post by Andreas Raab
Post by Levente Uzonyi
What's the curret ErrorHandler class?
WADebugErrorHandler (opens a debugger) or SSMailErrorHandler (sends a
mail to the superuser) can help tracking the issue.
These classes aren't even present in our current version.
See http://source.squeak.org/ss.html
I see, it's too old. There's WAEmailErrorPage.
Should be okay now. It looks like there was some problem with a seaside
extension method that was somehow lost. The good news is we're now
running current code (update 8824) and no longer the ancient 3.7 based
version.

Cheers,
- Andreas
Bert Freudenberg
2012-01-28 12:31:06 UTC
Permalink
The good news is we're now running current code (update 8824) and no longer the ancient 3.7 based version.
Cheers,
- Andreas
Rock!

Let's watch if that fixes the next Multilingual package upload ...

- Bert -
Andreas Raab
2012-01-28 12:31:07 UTC
Permalink
Post by Bert Freudenberg
Rock!
Let's watch if that fixes the next Multilingual package upload ...
Works all right so far, but I think we still got two problems:

1) Does anyone have an idea where the double line ends come from when
mailing out the diffs? It's really annoying. Any pointers to where to
start looking at would be greatly welcome.

2) Does anyone feel as if we're getting a higher number of connection
failures than usual? It could be an issue on my end but I somehow doubt
that. I seem to getting connection failures about 20% of the time when
trying to update. It's quite noticable.

Cheers,
- Andreas
Levente Uzonyi
2012-01-28 12:31:07 UTC
Permalink
Post by Bert Freudenberg
Rock!
Let's watch if that fixes the next Multilingual package upload ...
1) Does anyone have an idea where the double line ends come from when mailing
out the diffs? It's really annoying. Any pointers to where to start looking
at would be greatly welcome.
I guess it's because TextDiffBuilder now keeps the original line endings,
so lines with different line endings will not match. If someone fixes line
endings (removing lf's) the diffs can reflect that.
2) Does anyone feel as if we're getting a higher number of connection
failures than usual? It could be an issue on my end but I somehow doubt that.
I seem to getting connection failures about 20% of the time when trying to
update. It's quite noticable.
I didn't get any, so far.


Levente
Cheers,
- Andreas
Levente Uzonyi
2012-01-28 12:31:09 UTC
Permalink
Post by Bert Freudenberg
Rock!
Let's watch if that fixes the next Multilingual package upload ...
1) Does anyone have an idea where the double line ends come from when mailing
out the diffs? It's really annoying. Any pointers to where to start looking
at would be greatly welcome.
2) Does anyone feel as if we're getting a higher number of connection
failures than usual? It could be an issue on my end but I somehow doubt that.
I seem to getting connection failures about 20% of the time when trying to
update. It's quite noticable.
I did a bit of stress testing, by uploading a few packages one after
another. There was no problem during the uploads, but when all uploads
were finished it took 20-30 seconds for the server to respond for the next
few 1-2 minutes. I guess it's related to the gc settings, so tweaking them
may help. For a reference, here are the "seaside defaults":

SmalltalkImage current
vmParameterAt: 5 put: 100000;
vmParameterAt: 6 put: 35000;
vmParameterAt: 24 put: 16 * 1024 * 1024;
vmParameterAt: 25 put: 8 * 1024 * 1024.
Smalltalk
setGCBiasToGrowGCLimit: 16 * 1024 * 1024;
setGCBiasToGrow: 1

The uploaded changes were produced with the code critics feature of
OmniBrowser. I think we should use it regularly, because it can point out
issues which could be hard to find otherwise and there are plenty of
issues to be fixed. They are bit broken at the moment, so running them on
the whole image needs a few fixes.
Some issues are easy to fix, others require deep understanding of the
system.

Some categories which should be fixed (number of possible issues):
Messages sent but not implemented (571)
Sends unknown message to global (36)
Subclass responsibility not defined (82)
Instance variable capitalization (4)
Temporary variable capitalization (5)
Inconsistent method classification (324)
Non-blocks in special messages (24)
Unnecessary assignment or return in block (45)
Uses "(a and: [b]) and: [c]" instead of "a and: [b and: [c]]" (78)
Uses (to:)do: instead of to:do: (23)
Variable is only assigned a single literal value (44)
Instance variable overridden by temporary variable (21)
Possible missing "; yourself" (490)
Returns a boolean and non boolean (42)
Subclass of collection that has instance variable but doesn't define copyEmpty (1)
Menus missing translations (75)
Method source contains linefeeds (123)
Assignment has no effect (22)
Check for same statements at end of ifTrue:ifFalse: blocks (43)
Instance variables not read AND written (209)
Method just sends super message (17)
Variable referenced in only one method and always assigned first (69)
Variables not referenced (266)

I hope we can decrease these numbers soon.


Levente
Cheers,
- Andreas
Mariano Martinez Peck
2012-01-28 12:31:13 UTC
Permalink
Post by Levente Uzonyi
Post by Andreas Raab
Post by Bert Freudenberg
Rock!
Let's watch if that fixes the next Multilingual package upload ...
1) Does anyone have an idea where the double line ends come from when
mailing out the diffs? It's really annoying. Any pointers to where to start
looking at would be greatly welcome.
2) Does anyone feel as if we're getting a higher number of connection
failures than usual? It could be an issue on my end but I somehow doubt
that. I seem to getting connection failures about 20% of the time when
trying to update. It's quite noticable.
I did a bit of stress testing, by uploading a few packages one after
another. There was no problem during the uploads, but when all uploads were
finished it took 20-30 seconds for the server to respond for the next few
1-2 minutes. I guess it's related to the gc settings, so tweaking them may
SmalltalkImage current
vmParameterAt: 5 put: 100000;
vmParameterAt: 6 put: 35000;
vmParameterAt: 24 put: 16 * 1024 * 1024;
vmParameterAt: 25 put: 8 * 1024 * 1024.
Levente: were did you get this values?? What is "seaside default" ?

I see another values in my "seaside default" image.

Thanks

Mariano
Post by Levente Uzonyi
Smalltalk
setGCBiasToGrowGCLimit: 16 * 1024 * 1024;
setGCBiasToGrow: 1
The uploaded changes were produced with the code critics feature of
OmniBrowser. I think we should use it regularly, because it can point out
issues which could be hard to find otherwise and there are plenty of issues
to be fixed. They are bit broken at the moment, so running them on the whole
image needs a few fixes.
Some issues are easy to fix, others require deep understanding of the
system.
Messages sent but not implemented (571)
Sends unknown message to global (36)
Subclass responsibility not defined (82)
Instance variable capitalization (4)
Temporary variable capitalization (5)
Inconsistent method classification (324)
Non-blocks in special messages (24)
Unnecessary assignment or return in block (45)
Uses "(a and: [b]) and: [c]" instead of "a and: [b and: [c]]" (78)
Uses (to:)do: instead of to:do: (23)
Variable is only assigned a single literal value (44)
Instance variable overridden by temporary variable (21)
Possible missing "; yourself" (490)
Returns a boolean and non boolean (42)
Subclass of collection that has instance variable but doesn't define copyEmpty (1)
Menus missing translations (75)
Method source contains linefeeds (123)
Assignment has no effect (22)
Check for same statements at end of ifTrue:ifFalse: blocks (43)
Instance variables not read AND written (209)
Method just sends super message (17)
Variable referenced in only one method and always assigned first (69)
Variables not referenced (266)
I hope we can decrease these numbers soon.
Levente
Cheers,
Post by Andreas Raab
- Andreas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20100113/5ed7ac79/attachment.htm
Levente Uzonyi
2012-01-28 12:31:13 UTC
Permalink
Post by Mariano Martinez Peck
Post by Levente Uzonyi
SmalltalkImage current
vmParameterAt: 5 put: 100000;
vmParameterAt: 6 put: 35000;
vmParameterAt: 24 put: 16 * 1024 * 1024;
vmParameterAt: 25 put: 8 * 1024 * 1024.
Levente: were did you get this values?? What is "seaside default" ?
I see another values in my "seaside default" image.
From SmalltalkImage >> #initializeMemorySettingsProfileSeaSide.
What are your defaults?


Levente
Post by Mariano Martinez Peck
Thanks
Mariano
Mariano Martinez Peck
2012-01-28 12:31:13 UTC
Permalink
Post by Mariano Martinez Peck
Post by Levente Uzonyi
SmalltalkImage current
Post by Levente Uzonyi
vmParameterAt: 5 put: 100000;
vmParameterAt: 6 put: 35000;
vmParameterAt: 24 put: 16 * 1024 * 1024;
vmParameterAt: 25 put: 8 * 1024 * 1024.
Levente: were did you get this values?? What is "seaside default" ?
I see another values in my "seaside default" image.
From SmalltalkImage >> #initializeMemorySettingsProfileSeaSide.
What are your defaults?
I don't see that method neither in 2.8 or 3 ... wierd
Post by Mariano Martinez Peck
Levente
Thanks
Post by Levente Uzonyi
Mariano
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20100113/0ca619fd/attachment.htm
Levente Uzonyi
2012-01-28 12:31:07 UTC
Permalink
Post by Bert Freudenberg
The good news is we're now running current code (update 8824) and no longer the ancient 3.7 based version.
Cheers,
- Andreas
Rock!
Let's watch if that fixes the next Multilingual package upload ...
It works, but there's a new (minor) issue: the diff contains double line
endings. And the old green image was better than the current blue on the
web interface, because other style elements are for the green image.


Levente
Post by Bert Freudenberg
- Bert -
Andreas Raab
2012-01-28 12:31:07 UTC
Permalink
Post by Levente Uzonyi
It works, but there's a new (minor) issue: the diff contains double line
endings. And the old green image was better than the current blue on the
web interface, because other style elements are for the green image.
Both should be fixed now.

Cheers,
- Andreas
Levente Uzonyi
2012-01-28 12:31:07 UTC
Permalink
Post by Andreas Raab
Post by Levente Uzonyi
It works, but there's a new (minor) issue: the diff contains double line
endings. And the old green image was better than the current blue on the
web interface, because other style elements are for the green image.
Both should be fixed now.
There are (at least) three ways to fix the extra empty lines issue:
1. restore the original behavior in TextDiffBuilder >> #split: by using
endWithoutSeparators instead of end.
2. update TextDiffBuilder >> #printTextPatchSequence:on: to add cr if and
only if the line doesn't end with cr or crlf.
3. create a subclass of TextDiffBuilder that overrides #split: and throws
away line endings. and use that from MCDiffyTextWriter. The extension
methods could be moved there too.


Levente
Post by Andreas Raab
Cheers,
- Andreas
Levente Uzonyi
2012-01-28 12:31:08 UTC
Permalink
Post by Levente Uzonyi
Post by Andreas Raab
Post by Levente Uzonyi
It works, but there's a new (minor) issue: the diff contains double line
endings. And the old green image was better than the current blue on the
web interface, because other style elements are for the green image.
Both should be fixed now.
1. restore the original behavior in TextDiffBuilder >> #split: by using
endWithoutSeparators instead of end.
2. update TextDiffBuilder >> #printTextPatchSequence:on: to add cr if and
only if the line doesn't end with cr or crlf.
3. create a subclass of TextDiffBuilder that overrides #split: and throws
away line endings. and use that from MCDiffyTextWriter. The extension
methods could be moved there too.
I finally fixed it in a fourth way by making #buildPatchSequence
more backwards compatible by ignoring crs (System-ul.230). A fifth
solution would be to change #buildTextPatch to use
#patchSequenceDoIfMatch:ifInsert:ifRemove: directly, like:

TextDiffBuilder >> #buildTextPatch

^String streamContents: [ :stream |
self
patchSequenceDoIfMatch: [ :string |
stream space: 2.
self print: string withAttributes: nil on: stream ]
ifInsert: [ :string |
stream nextPutAll: '+ '.
self print: string withAttributes: nil on: stream ]
ifRemove: [ :string |
stream nextPutAll: '- '.
self print: string withAttributes: nil on: stream ] ]

This would also allow us to remove #stringForAttributes: and
#printTextPatchSequence:on:.


Levente
Post by Levente Uzonyi
Levente
Post by Andreas Raab
Cheers,
- Andreas
Andreas Raab
2012-01-28 12:31:08 UTC
Permalink
Post by Levente Uzonyi
Post by Andreas Raab
Post by Levente Uzonyi
It works, but there's a new (minor) issue: the diff contains double
line endings. And the old green image was better than the current
blue on the web interface, because other style elements are for the
green image.
Both should be fixed now.
1. restore the original behavior in TextDiffBuilder >> #split: by using
endWithoutSeparators instead of end.
2. update TextDiffBuilder >> #printTextPatchSequence:on: to add cr if and
only if the line doesn't end with cr or crlf.
3. create a subclass of TextDiffBuilder that overrides #split: and throws
away line endings. and use that from MCDiffyTextWriter. The extension
methods could be moved there too.
I finally fixed it in a fourth way by making #buildPatchSequence more
backwards compatible by ignoring crs (System-ul.230). A fifth solution
would be to change #buildTextPatch to use
TextDiffBuilder >> #buildTextPatch
^String streamContents: [ :stream |
self
patchSequenceDoIfMatch: [ :string |
stream space: 2.
self print: string withAttributes: nil on: stream ]
ifInsert: [ :string |
stream nextPutAll: '+ '.
self print: string withAttributes: nil on: stream ]
ifRemove: [ :string |
stream nextPutAll: '- '.
self print: string withAttributes: nil on: stream ] ]
This would also allow us to remove #stringForAttributes: and
#printTextPatchSequence:on:.
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.

Cheers,
- Andreas
Levente Uzonyi
2012-01-28 12:31:08 UTC
Permalink
Post by Levente Uzonyi
Post by Andreas Raab
Post by Levente Uzonyi
It works, but there's a new (minor) issue: the diff contains double line
endings. And the old green image was better than the current blue on the
web interface, because other style elements are for the green image.
Both should be fixed now.
1. restore the original behavior in TextDiffBuilder >> #split: by using
endWithoutSeparators instead of end.
2. update TextDiffBuilder >> #printTextPatchSequence:on: to add cr if and
only if the line doesn't end with cr or crlf.
3. create a subclass of TextDiffBuilder that overrides #split: and throws
away line endings. and use that from MCDiffyTextWriter. The extension
methods could be moved there too.
I finally fixed it in a fourth way by making #buildPatchSequence more
backwards compatible by ignoring crs (System-ul.230). A fifth solution
would be to change #buildTextPatch to use
TextDiffBuilder >> #buildTextPatch
^String streamContents: [ :stream |
self
patchSequenceDoIfMatch: [ :string |
stream space: 2.
self print: string withAttributes: nil on: stream ]
ifInsert: [ :string |
stream nextPutAll: '+ '.
self print: string withAttributes: nil on: stream ]
ifRemove: [ :string |
stream nextPutAll: '- '.
self print: string withAttributes: nil on: stream ] ]
This would also allow us to remove #stringForAttributes: and
#printTextPatchSequence:on:.
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.


Levente
Cheers,
- Andreas
Andreas Raab
2012-01-28 12:31:08 UTC
Permalink
Post by Levente Uzonyi
Post by Andreas Raab
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
Nicolas Cellier
2012-01-28 12:31:09 UTC
Permalink
Hi Levente,
what about completely ignoring line endings in diffs ?

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
Post by Andreas Raab
Post by Levente Uzonyi
Post by Andreas Raab
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
Levente Uzonyi
2012-01-28 12:31:10 UTC
Permalink
Post by Nicolas Cellier
Hi Levente,
what about completely ignoring line endings in diffs ?
I intentionally added this feature. Do you think it's wrong?


Levente
Post by Nicolas Cellier
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
Post by Andreas Raab
Post by Levente Uzonyi
Post by Andreas Raab
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
Igor Stasenko
2012-01-28 12:31:10 UTC
Permalink
Post by Levente Uzonyi
Post by Nicolas Cellier
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.
Post by Levente Uzonyi
Levente
Post by Nicolas Cellier
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
|
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
Post by Andreas Raab
Post by Levente Uzonyi
Post by Andreas Raab
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.
Levente Uzonyi
2012-01-28 12:31:10 UTC
Permalink
Post by Igor Stasenko
Post by Levente Uzonyi
Post by Nicolas Cellier
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?


Levente
Post by Igor Stasenko
Post by Levente Uzonyi
Levente
Post by Nicolas Cellier
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
|
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
Post by Andreas Raab
Post by Levente Uzonyi
Post by Andreas Raab
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.
Nicolas Cellier
2012-01-28 12:31:10 UTC
Permalink
Post by Igor Stasenko
Post by Levente Uzonyi
Post by Nicolas Cellier
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?
To me, it's OK since different lineEnding are now rendered the same.
Unless the difference is important for exporting code?

Nicolas
Levente
Post by Igor Stasenko
Post by Levente Uzonyi
Levente
Post by Nicolas Cellier
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
|
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
Post by Andreas Raab
Post by Levente Uzonyi
Post by Andreas Raab
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.
Igor Stasenko
2012-01-28 12:31:11 UTC
Permalink
Post by Igor Stasenko
Post by Levente Uzonyi
Post by Nicolas Cellier
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.
Levente
Post by Igor Stasenko
Post by Levente Uzonyi
Levente
Post by Nicolas Cellier
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
|
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
Post by Andreas Raab
Post by Levente Uzonyi
Post by Andreas Raab
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.
Levente Uzonyi
2012-01-28 12:31:11 UTC
Permalink
Post by Igor Stasenko
Post by Igor Stasenko
Post by Levente Uzonyi
Post by Nicolas Cellier
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
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
Post by Igor Stasenko
Levente
Post by Igor Stasenko
Post by Levente Uzonyi
Levente
Post by Nicolas Cellier
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
|
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
Post by Andreas Raab
Post by Levente Uzonyi
Post by Andreas Raab
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.
Igor Stasenko
2012-01-28 12:31:11 UTC
Permalink
Post by Igor Stasenko
Post by Igor Stasenko
Post by Levente Uzonyi
Post by Nicolas Cellier
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
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
at: key put: anObject | index assoc | index := self scanFor: key. assoc :=
key value: anObject)] ? ifNotNil: [assoc value: anObject]. ^anObject
at: key put: anObject
? | index assoc |
? index := self scanFor: key.
? assoc := array at: index.
? assoc
anObject) ]
? ? ?ifNotNil: [ assoc value: anObject ].
? ^anObject
For me, every whitespace counts in the above method, but the original
Should the diff algorithm care about the line endings or not?
foo bar<cr><lf>
foo bar<cr>
foo bar<lf>
No. Please, not again. :) All of them should be treated equally. We're
talking here about source code,
not about binary files.
Levente
--
Best regards,
Igor Stasenko AKA sig.
Levente Uzonyi
2012-01-28 12:31:11 UTC
Permalink
Post by Igor Stasenko
Post by Levente Uzonyi
foo bar<cr><lf>
foo bar<cr>
foo bar<lf>
No. Please, not again. :) All of them should be treated equally. We're
talking here about source code,
not about binary files.
Again? I doubt this question was raised before. We are talking about
diffs (at least I am).

(About the source code: I don't care how the source code is handled if
formatting is preserved. And no, I don't like pretty-printed versions.)


Levente
Post by Igor Stasenko
Post by Levente Uzonyi
Levente
--
Best regards,
Igor Stasenko AKA sig.
Igor Stasenko
2012-01-28 12:31:11 UTC
Permalink
Post by Igor Stasenko
Post by Levente Uzonyi
foo bar<cr><lf>
foo bar<cr>
foo bar<lf>
No. Please, not again. :) All of them should be treated equally. We're
talking here about source code,
not about binary files.
Again? I doubt this question was raised before. We are talking about diffs
(at least I am).
There was a lengthly discussion about how to deal with different line
ending sequences in editor.
You luckily missed it :)
(About the source code: I don't care how the source code is handled if
formatting is preserved. And no, I don't like pretty-printed versions.)
Levente
Post by Igor Stasenko
Post by Levente Uzonyi
Levente
--
Best regards,
Igor Stasenko AKA sig.
--
Best regards,
Igor Stasenko AKA sig.
Levente Uzonyi
2012-01-28 12:31:11 UTC
Permalink
Post by Igor Stasenko
Post by Igor Stasenko
Post by Levente Uzonyi
foo bar<cr><lf>
foo bar<cr>
foo bar<lf>
No. Please, not again. :) All of them should be treated equally. We're
talking here about source code,
not about binary files.
Again? I doubt this question was raised before. We are talking about diffs
(at least I am).
There was a lengthly discussion about how to deal with different line
ending sequences in editor.
You luckily missed it :)
The context is different. This is about diffs, not editors.


Levente
Post by Igor Stasenko
(About the source code: I don't care how the source code is handled if
formatting is preserved. And no, I don't like pretty-printed versions.)
Levente
Post by Igor Stasenko
Post by Levente Uzonyi
Levente
--
Best regards,
Igor Stasenko AKA sig.
--
Best regards,
Igor Stasenko AKA sig.
Nicolas Cellier
2012-01-28 12:31:11 UTC
Permalink
Post by Igor Stasenko
Post by Igor Stasenko
Post by Levente Uzonyi
Post by Nicolas Cellier
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
foo := '1
2
3
4
'.
Then we should care. Otherwise not.
True, there is a "semantic" difference as long as cr lf cr-lf have
different semantic. But do they ?
Since invisible characters are not an explicit specification robust to
future editions, my expectations would be low on such code!
My interpretation would be this one: if developer did rely on specific
line-endings then she should care to explicitely specify line-endings.
If she uses invisible specifications, then it means she just want
whatever line-endings.

Thus, I would not event bother with line endings inside literals and
would tend to say: please use appropriate message (like
withSqueakLineEnding)

Nicolas
Post by Igor Stasenko
Levente
Post by Igor Stasenko
Post by Levente Uzonyi
Levente
Post by Nicolas Cellier
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
|
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
Post by Andreas Raab
Post by Levente Uzonyi
Post by Andreas Raab
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.
Igor Stasenko
2012-01-28 12:31:11 UTC
Permalink
Post by Nicolas Cellier
Post by Igor Stasenko
Post by Igor Stasenko
Post by Levente Uzonyi
Post by Nicolas Cellier
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
foo := '1
2
3
4
'.
Then we should care. Otherwise not.
True, there is a "semantic" difference as long as cr lf cr-lf have
different semantic. But do they ?
Since invisible characters are not an explicit specification robust to
future editions, my expectations would be low on such code!
My interpretation would be this one: if developer did rely on specific
line-endings then she should care to explicitely specify line-endings.
If she uses invisible specifications, then it means she just want
whatever line-endings.
Thus, I would not event bother with line endings inside literals and
would tend to say: please use appropriate message (like
withSqueakLineEnding)
+1 . A well-grounded argumentation.
Post by Nicolas Cellier
Nicolas
--
Best regards,
Igor Stasenko AKA sig.
Loading...