Diff::LCS::Hunk#diff(:unified) output changed between 1.3 and 1.4.x, part 2 #65
Assignees
Comments
|
This is going to take a little longer to fix, but you are correct. The problem is that the The changes in numbers (especially for single-line changes as in the first case) are intentional fixes. |
halostatue
added a commit
that referenced
this issue
Jun 30, 2020
halostatue
added a commit
that referenced
this issue
Jul 1, 2020
Resolve #65 - Also add even more tests for checking `ldiff` results against `diff` results. - Fix issues with diff/ldiff output highlighted by the above tests. - Add a parameter to indicate that the hunk being processed is the _last_ hunk; this results in correct counting of the hunk size. - The misplaced chunks were happening because of an improper `.abs` on `#diff_size`, when the `.abs` needed to be on the finding of the maximum diff size.
|
Can you test the code #66 to see if it resolves your issue? BTW, the test script you provided will still output incorrect values for the last chunk, compared to diff, unless you make the following change with what’s in that PR. - puts "#{previous_hunk.diff(:unified)}\n" unless previous_hunk.nil?
+ puts "#{previous_hunk.diff(:unified, true)}\n" unless previous_hunk.nil? |
|
The tests for |
|
TL;DR: the line number output changes are intentional and the output of v1.3 and earlier is incorrect. v1.4, with this upcoming patch version, is finally fully compatible with the output of @JonRowe did some testing for RSpec to work with both v1.3 and v1.4; the line number changes here are intentional because they’ve been wrong forever. In some ways, 1.4.4 is going to make the line number changes worse because I have a better handle on the special cases that need to be handled to produce output compatible with This change, in particular, is an example of what I mean: -@@ -1,2 +1,2 @@
+@@ -1 +1 @@
-cf4b8a020bdc1ba6914093a8a07a5514cce8a3a2979a967b1f32ea704a61785b
+304566f86a620aae85797a3c491a51fb8c6ecf996407e77b8063aa3ee59672c5If you did the same comparison with Also, you may see cases of
I don’t yet have test files with and without newlines for this, but I have run visual comparison tests and |
|
FWIW we (RSpec) have gone with a sprinkling of helpers and an additional env var to test both 1.3 and 1.4 outputs, the diff (pardon the the pun) is quite small and if we didn't care about supporting both versions we'd happily just take the 1.4 outputs. See rspec/rspec-support#421 and its attached issues on the other repositories. |
halostatue
added a commit
that referenced
this issue
Jul 1, 2020
# This is the 1st commit message: Fix improperly placed chunks Resolve #65 - Also add even more tests for checking `ldiff` results against `diff` results. - Fix issues with diff/ldiff output highlighted by the above tests. - Add a parameter to indicate that the hunk being processed is the _last_ hunk; this results in correct counting of the hunk size. - The misplaced chunks were happening because of an improper `.abs` on `#diff_size`, when the `.abs` needed to be on the finding of the maximum diff size. # This is the commit message #2: Ooops. Debugger # This is the commit message #3: Restore missing test - Fix some more format issues raised by the missing test. - Start fixing Rubocop formatting. # This is the commit message #4: Last RuboCop fixes # This is the commit message #5: Finalize diff-lcs 1.4 # This is the commit message #6: Fix #44 The problem here was the precedence of `or` vs `||`. Switching to `||` resulted in the expected behaviour. # This is the commit message #7: Resolve #43 # This is the commit message #8: Typo # This is the commit message #9: Resolve #35 with a comment
halostatue
added a commit
that referenced
this issue
Jul 1, 2020
- Resolve #65: Two different issues were reported: - Line numbers changed between 1.3 and 1.4. This change is intentional, but in comparing the output against `diff` for the new examples provided it became clear that for unified and context diffs, the last hunk was reporting incorrect hunk lengths (that is, if it was `12,4 11,8`, it should have been `12,3 11,7`. This has been resolved by providing `Diff::LCS::Hunk#diff` an additional parameter, `last` which defaults to `false`. - Net new lines were added in the middle of a unified diff hunk, which was incorrect. That is, we were getting: ```diff @@ -10,6 +11,10 @@ recipe[j::default] recipe[k::default] recipe[l::default] +recipe[o::new] +recipe[p::new] +recipe[q::new] +recipe[r::new] recipe[m::default] recipe[n::default] ``` instead of: ```diff @@ -12,3 +11,7 @@ recipe[l::default] recipe[m::default] recipe[n::default] +recipe[o::new] +recipe[p::new] +recipe[q::new] +recipe[r::new] ``` This has been resolved. The error was that `Diff::LCS::Block#diff_size` was applying `.abs` to its output, which is incorrect. We need to know the direction of the size for placing changes, but when determining the maximum block size for use in `Diff::LCS::Hunk#diff` calculations, we need to know the `.abs` size. - New tests were added to prevent these changes from regressing in the future, both as issue tests and as additional `ldiff` tests. These tests highlighted more issues with diff-lcs output as compared to `diff`, specifically the handling and reporting of missing newlines at the end of files. All of the issues highlighted were resolved and the structure of `ldiff` tests was changed so that it is easier to add new comparison files at any time. - Resolve #35: Indicate that when comparing against custom objects, `#eql?` must be implemented such that objects that _resolve_ to the same meaning are _treated_ as the same meaning. This is important because the basic LCS algorithm uses a hash for position matching. - Resolve #43: Provide a more meaningful error from `Diff::LCS::Hunk.new` if the `piece` provided does not create useful `Diff::LCS::Block`. It's extremely unlikely, this error will be more useful than `NoMethodError` being thrown. - Resolve #44: `ldiff` binary detection failed to work correctly. It had been `!old_text or !new_text`, but the precedence of `or` broke that. It is now `!old_text || !new_text`. Also: - Ran Rubocop again. Updated configuration definitions, fixed some code formatting.
mpraglowski
added a commit
to RailsEventStore/rails_event_store
that referenced
this issue
Jul 8, 2020
The latest changes in diff-lcs gem (updated to 1.4.4) have lead to fails in our tests due to wrong formatted output. The change has been introduced here: halostatue/diff-lcs#65
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Hi,
Similar to the case I mentioned in #60, there were failures in another Chef project, chef-cli, upon upgrading from diff-lcs 1.3 to 1.4.x. See here for the actual failures: https://buildkite.com/chef-oss/chef-chef-cli-master-verify/builds/227#aa286955-6862-4b7c-9d0b-c347c895a150
I made another test script that demonstrates the differences:
The output on diff-lcs 1.3:
The output on diff-cls 1.4.x:
BTW, thanks so much for your quick response on the last issue. ❤️
The text was updated successfully, but these errors were encountered: