batches: fix trailing newline logic for preview diff rendering
Created by: courier-new
A customer (https://github.com/sourcegraph/accounts/issues/6859) ran into a strange issue this past week which I've traced back to the trailing newline logic of the repository comparison resolver.
To illustrate the erroneous behavior, the following diff:
diff --git a/fake.go b/fake.go
index 239d050..e581214 100644
--- a/fake.go
+++ b/fake.go
@@ -25,9 +25,9 @@ import (
"private/some-path"
)
-var configPath = flag.String("config", "", "Path to the config file to use.")
-
type config struct {
+ baseplate.Config `yaml:",inline"`
+
Authentication authenticationsvc.Config `yaml:"authentication"`
Sso ssosvc.Config `yaml:"sso"`
OAuth oauthsvc.Config `yaml:"oauth"`when applied from a batch spec with src batch preview, produces an improperly rendered diff from the preview UI:
where the line with baseplate.Config... appears before type config struct instead of after it. But once published, the diff is rendered correctly once again:
The RepositoryComparisonResolver is responsible for constructing the raw html diff hunks that the Batch Changes preview UI will render for every unpublished changeset.
The file diff highlighter logic from this resolver is implemented to trim trailing newlines at the end of a diff hunk to prevent differences in the number of lines between the hunk and the base file as they are represented at this point in the code. In https://github.com/sourcegraph/sourcegraph/pull/20673, additional logic was added to also trim any trailing "-" lines, by looking for the last "-" that is not followed by any unchanged lines. This was in response to observed out-of-bounds panics when these lines were not trimmed.
In the example from the above illustration, the "-" line in the diff directly after -var configPath... is mistakenly flagged as one of these trailing "-" lines and removed, such that subsequent lines from the diff that are added to the resolver's html are 1 line "ahead" of the corresponding lines in the base file.
I believe that the check for "not followed by any unchanged lines" is the culprit here as it is a bit overly aggressive: rather than resetting lastMinus if another unchanged line was detected after the last "-" line, it was only resetting lastMinus if another empty unchanged line was detected after it. Now, it will reset lastMinus for any unchanged line, so in the example above, the "-" line would not be trimmed.
Here is the new diff rendered from the preview UI after this change, which once again matches expectations:
This bug and the challenge of tracking it down/understanding it is perhaps good motivation to prioritize https://github.com/sourcegraph/sourcegraph/issues/20704.