Make comparison of newlines in text files more precise #2600

HannesWell · 2023-06-29T07:16:52Z

At the moment the TextComparator ignores simplify all kind of newline characters (\r and \n), which has the effect that, although a new line to a text file was added or removed, the comparator considers that file as unchanged.

This PR enhances the TextComparator to only ignore a difference in newline characters if there is is an exactly matching pair of \r or \r\n in baseline respectively reactor.

This PR also adds tests for the TextComparator and moves the showDiffDetails property to ComparisonData (in the first commit) to simplify testing.

laeubi · 2023-06-29T07:34:17Z

At the moment the TextComparator ignores simplify all kind of newline characters (\r and \n), which has the effect that, although a new line to a text file was added or removed, the comparator considers that file as unchanged.

If newlines do not matter for the delta should then additional ones also don't matter? Can you explain in what case an additional newline is relevant? In the end the user can always bump the version anyways if it is considered a relevant change.

laeubi · 2023-06-29T07:35:24Z

...rtifactcomparator/src/main/java/org/eclipse/tycho/zipcomparator/internal/TextComparator.java

-        boolean hasNext() {
-            skipNewLines();
-            return index < bytes.length;
+            // Both sliders are at either "\n" or "\r\n"


Please note that a single \r is a valid newline as well!

That's right (although AFAICT not widely used), but the important part is that a single \r is treated equally on Windows and Linux, especially by git.

Please have a look at the test-cases I created. If you think there is a case that is not covered, please let me know and the tests can be easily extended.

HannesWell · 2023-06-29T07:53:14Z

At the moment the TextComparator ignores simplify all kind of newline characters (\r and \n), which has the effect that, although a new line to a text file was added or removed, the comparator considers that file as unchanged.

If newlines do not matter for the delta should then additional ones also don't matter? Can you explain in what case an additional newline is relevant? In the end the user can always bump the version anyways if it is considered a relevant change.

They do matter. At least from my point of view, the new line skip was introduced for text files to deal with the different default line-ending on Windows and Linux and because it is very common to use git's auto-crlf feature where on Windows on checks in Linux-style and checks-out windows style line-endings.
So I think the goal should be to check if there is a content difference or if only the line-endings style changed.
And yes I would consider just a blank new line a content change, even if one for example just appends a new-line at the end of the file.

And considering added/removed newlines is for example relevant if you adjust a doc or license file. Of course one can bump the version, but if that is not necessary I believe nobody thinks about that in advance and will just wonder why the changed doc/license file is not in the build jars, while they are present in the IDE.

github-actions · 2023-06-29T09:19:16Z

Test Results

  558 files ±0   558 suites ±0 3h 51m 59s ⏱️ - 13m 49s
  357 tests ±0   351 ✔️ ±0   6 💤 ±0 0 ❌ ±0
1 071 runs ±0 1 052 ✔️ ±0 19 💤 ±0 0 ❌ ±0

Results for commit ccd6ce3. ± Comparison against base commit 865f130.

♻️ This comment has been updated with latest results.

laeubi · 2023-07-03T07:08:31Z

I believe nobody thinks about that in advance and will just wonder why the changed doc/license file is not in the build jars, while they are present in the IDE.

Maybe because its a strange license that is considered different because of more line endings ;-)

Anyways it seems the testcases do not pass (anymore) with that change on some platforms.

laeubi · 2023-07-03T07:11:02Z

...rtifactcomparator/src/main/java/org/eclipse/tycho/artifactcomparator/ArtifactComparator.java

@@ -19,22 +19,22 @@

 public interface ArtifactComparator {

-    public static class ComparisonData {
+    public static record ComparisonData(List<String> ignoredPattern, boolean writeDelta, boolean showDiffDetails) {


Maybe this change can be a separate PR.

Sure. Here we go: #2610

laeubi · 2023-07-03T07:12:04Z

...rtifactcomparator/src/main/java/org/eclipse/tycho/zipcomparator/internal/TextComparator.java

+    // Possible new lines:
+    // \n -- unix style
+    // \r\n -- windows style
+    // \r -- whatever style (ignored?!)


this is macos https://stackoverflow.com/questions/38688516/line-break-represented-as-r-on-mac

According to Wikipedia's Mac OS 9 article, the latest release of Mac OS 9 is 9.2.2 / December 5, 2001, which was 21 years ago. Therefore I think this is not really relevant anymore.

Nevertheless my intention was that this works as if one would use a BufferedReader to read the lines one by one using BufferedReader.readLine() and compare the line content, just more efficient (i.e. without creating Strings all the time).
And the javadoc of BufferedReader.readLine() says:

Reads a line of text. A line is considered to be terminated by any oneof a line feed ('\n'), a carriage return ('\r'), a carriage return followed immediately by a line feed, or by reaching the end-of-file(EOF).

So if equivalence is the goal, \r needs to be considered.

...rtifactcomparator/src/main/java/org/eclipse/tycho/zipcomparator/internal/TextComparator.java

laeubi · 2023-07-06T09:39:37Z

@HannesWell do you plan to work on this and possibly backport to Tycho 4? I currently plan to prepare a bugfix release so so we might want to include it there.

HannesWell · 2023-07-06T21:26:57Z

@HannesWell do you plan to work on this and possibly backport to Tycho 4? I currently plan to prepare a bugfix release so so we might want to include it there.

I adjusted the logic so that \r now should be treated as regular line-ending and in the latest run all tests passed unchanged.

#2610 should be merged before this

Don't ignore all kind of newlines when comparing text-files, only treat exactly matching \n and \r\n as equals.

github-actions · 2023-07-09T05:07:55Z

💚 All backports created successfully

Status	Branch	Result
✅	tycho-4.0.x

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

HannesWell requested a review from laeubi June 29, 2023 07:16

HannesWell force-pushed the enhancedNewlineComparison branch 2 times, most recently from bee5fc3 to bae1cf1 Compare June 29, 2023 07:26

laeubi reviewed Jun 29, 2023

View reviewed changes

laeubi reviewed Jul 3, 2023

View reviewed changes

...rtifactcomparator/src/main/java/org/eclipse/tycho/zipcomparator/internal/TextComparator.java Outdated Show resolved Hide resolved

HannesWell force-pushed the enhancedNewlineComparison branch 2 times, most recently from c73bc38 to d10aed2 Compare July 6, 2023 21:14

HannesWell force-pushed the enhancedNewlineComparison branch from d10aed2 to 5aef81f Compare July 7, 2023 07:48

Make comparison of newlines in text files more precise

ccd6ce3

Don't ignore all kind of newlines when comparing text-files, only treat exactly matching \n and \r\n as equals.

HannesWell force-pushed the enhancedNewlineComparison branch from 5aef81f to ccd6ce3 Compare July 8, 2023 06:36

HannesWell requested a review from laeubi July 8, 2023 08:16

laeubi merged commit 199647e into eclipse-tycho:master Jul 9, 2023

laeubi added the backport-to-tycho-4.0.x Can be added to a PR to trigger an automatic backport of the change label Jul 9, 2023

github-actions bot mentioned this pull request Jul 9, 2023

[tycho-4.0.x] Make comparison of newlines in text files more precise #2614

Merged

HannesWell deleted the enhancedNewlineComparison branch July 9, 2023 11:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make comparison of newlines in text files more precise #2600

Make comparison of newlines in text files more precise #2600

HannesWell commented Jun 29, 2023

laeubi commented Jun 29, 2023

laeubi Jun 29, 2023

HannesWell Jun 29, 2023

HannesWell commented Jun 29, 2023

github-actions bot commented Jun 29, 2023 •

edited

Loading

laeubi commented Jul 3, 2023

laeubi Jul 3, 2023

HannesWell Jul 6, 2023

laeubi Jul 3, 2023

HannesWell Jul 6, 2023

laeubi commented Jul 6, 2023

HannesWell commented Jul 6, 2023

github-actions bot commented Jul 9, 2023

Make comparison of newlines in text files more precise #2600

Make comparison of newlines in text files more precise #2600

Conversation

HannesWell commented Jun 29, 2023

laeubi commented Jun 29, 2023

laeubi Jun 29, 2023

Choose a reason for hiding this comment

HannesWell Jun 29, 2023

Choose a reason for hiding this comment

HannesWell commented Jun 29, 2023

github-actions bot commented Jun 29, 2023 • edited Loading

Test Results

laeubi commented Jul 3, 2023

laeubi Jul 3, 2023

Choose a reason for hiding this comment

HannesWell Jul 6, 2023

Choose a reason for hiding this comment

laeubi Jul 3, 2023

Choose a reason for hiding this comment

HannesWell Jul 6, 2023

Choose a reason for hiding this comment

laeubi commented Jul 6, 2023

HannesWell commented Jul 6, 2023

github-actions bot commented Jul 9, 2023

💚 All backports created successfully

Questions ?

github-actions bot commented Jun 29, 2023 •

edited

Loading