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

Diff scoping is imprecise #7

Open
domfarolino opened this issue Nov 12, 2022 · 0 comments
Open

Diff scoping is imprecise #7

domfarolino opened this issue Nov 12, 2022 · 0 comments
Labels
help wanted Extra attention is needed

Comments

@domfarolino
Copy link
Owner

In order to scope specfmt to the diff of the current spec branch, we:

  1. Read each line of the spec, marking each as ineligible for formatting
  2. Read the git diff
  3. "Apply" the diff to the lines of the spec (via apply_diff() function), marking each line in the spec that also appears in the git diff, as eligible for formatting

The issue with the above is that the git diff does not include line numbers, so we might actually mark lines of the spec (that may even predate the current branch) as eligible for formatting, if they are identical to lines that have been added in the current branch, and are therefore in the current git diff.

There are two tests that demonstrate this behavior:

As per the second test above, the reason you're unlikely to run into this problem in practice, is because the preexisting lines in the spec that might mistakenly get formatted as a result of this bug, must be after all preceding lines in the git diff. That's a bit confusing, so here's a more clear example. Imagine the spec:

+----------+
| My spec  |
|          |
|+ Intro   |
|+ My spec |
+----------+

So the original spec had the text "My spec", and the current branch of the spec adds the remaining lines. Given the description of the bug, since the text "My spec" appears in the git diff, it might sound like line 1 (which existed before the spec's current branch adds the other lines) would mistakenly get formatted (since its text also exists in the diff), but that's not the case. We'd only format lines matching "My spec" that come after lines matching "Intro", since "Intro" is the first line of the git diff.

That's why this bug is not quite as bad as "Oh no, specfmt formats every single line of a spec that matches any line in the diff". But it is still an annoying bug.


To fix this, we probably need line numbers for all the additions in the git diff, so that apply_diff() can be as simple as iterating through each Line of the spec, marking each line whose number appears in the diff as eligible for formatting.

@domfarolino domfarolino added the help wanted Extra attention is needed label Nov 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant