Skip to content

Conversation

@crockeo
Copy link

@crockeo crockeo commented Oct 19, 2023

As far as I know github.CommitFile structures patches such that:

  • Each commit is its own patch, but all patches are part of one large "meta-patch."
  • Each of these patches has its own top-to-bottom ordering, so each diff in a patch appears in ascending order of line number within a patch.
  • But each commit's patch is ordered sequentially. E.g. all diffs in patch 1 sit before diffs in patch 2, even if diffs in patch 2 affect earlier lines.

See: multi_stage.patch below for an example.

GitHub processes these multi-commit patch files intelligently and renders a PR diff so that each added line appears at the place it will be after merging. But less-advanced-security makes lineBounds from each line number that appears in the diff, and doesn't shift them around based on subsequent lineBounds.

All of this means that we can render annotations on lines which do not appear to be in a PR diff because that line number happened to appear in some commit's patch before it got shifted around by subsequent commits' edits.

@crockeo crockeo changed the title handle case where diff ranges are handle github.CommitFile multi-commit PR patch format Oct 19, 2023
Comment on lines +173 to +181
rowDiff := newRows - oldRows
for i, bound := range lineBounds {
if oldStart < bound.start {
bound.start += rowDiff
bound.end += rowDiff
} else if oldStart < bound.end {
bound.end += rowDiff
}

lineBounds = append(lineBounds, lineBound{start: start, end: start + endOffset - 1})
lineBounds[i] = bound
Copy link
Author

@crockeo crockeo Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the asymptotic complexity of this little bit right here. Let me know if you expect this to be a problem and I can come back and do it right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant