Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make Diff::LCS::Hunk Robust #43

Closed
junaruga opened this issue Aug 27, 2018 · 2 comments · Fixed by #66
Closed

Make Diff::LCS::Hunk Robust #43

junaruga opened this issue Aug 27, 2018 · 2 comments · Fixed by #66
Assignees
@halostatue

Comments

@junaruga
Copy link

junaruga commented Aug 27, 2018 •
edited

Seeing below lines.
https://github.com/halostatue/diff-lcs/blob/v1.3/lib/diff/lcs/hunk.rb#L42-L45

@start_old = a1 || (b1 - before)
@start_new = b1 || (a1 + before)
@end_old   = a2 || (b2 - after)
@end_new = b2 || (a2 + after)

When both a1 and b1 are nil or both a2 and b2 are nil, NameError is raised unintentionally.
Maybe we can add a code to check it in advance?

$ cat diff.rb 
a1 = nil
b1 = nil
start_old = a1 || (b1 - before)

$ ruby diff.rb
Traceback (most recent call last):
diff.rb:3:in `<main>': undefined local variable or method `before' for main:Object (NameError)
@halostatue halostatue self-assigned this Feb 3, 2019
@halostatue
Copy link
Owner

Do you have an example that causes this in the real library? I can see that this could happen, but it would only be a case where there’s a hunk that has neither remove or insert operations. An actual example allows me to write useful tests and decide the correct sort of behaviour.

@junaruga
Copy link
Author

Hi @halostatue . Thanks for checking the code.
I do not have the real library. This was detected by a static code analysis tool
Possibly it is Coverity Scan.
https://scan.coverity.com/

halostatue added a commit that referenced this issue Jul 1, 2020
@halostatue
Resolve #43
17bdff5
@halostatue halostatue linked a pull request Jul 1, 2020 that will close this issue
diff-lcs 1.4.4 - Fix broken formatting #66
Merged
halostatue added a commit that referenced this issue Jul 1, 2020
@halostatue
# This is a combination of 9 commits.
60613d2 
# 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
@halostatue
diff-lcs 1.4.4 - Fix broken formatting
b75e0bd 
- 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.
@halostatue halostatue mentioned this issue Jul 1, 2020
Merged
@dependabot dependabot bot mentioned this issue Mar 14, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@halostatue halostatue

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

diff-lcs 1.4.4 - Fix broken formatting halostatue/diff-lcs
2 participants
@halostatue @junaruga