Various fixes for diffs and file diff rendering#44805
Conversation
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits cb21493 and ca2c501 or learn more. Open explanation
|
| """ | ||
| Specifies which format/highlighting technique to use. | ||
| """ | ||
| format: HighlightResponseFormat = HTML_HIGHLIGHT |
There was a problem hiding this comment.
This argument is usually allowed for highlighting but was missing for diff hunks.
This can be used to do a fast-pass render without highlighting first for faster diff loads.
| // This is unparseable by go-diff. Once it isn't anymore, the test should fail, reminding | ||
| // us of the TODO comment in repository_comparison to reenable it. |
There was a problem hiding this comment.
Fixed that in https://github.com/sourcegraph/sourcegraph/pull/44771!
| {kind: "UNCHANGED", html: "func AddFeature(key string, isEnabled bool) {"}, | ||
| {kind: "UNCHANGED", html: "features[strings.ToLower(key)] = isEnabled"}, | ||
| {kind: "UNCHANGED", html: "}"}, | ||
| {kind: "UNCHANGED", html: ""}, | ||
| {kind: "DELETED", html: ""}, |
There was a problem hiding this comment.
I don't know what was going on here before, but that assertion was very broken.
| "func IsEnabled(key string) bool {", | ||
| "return features[strings.ToLower(key)]", | ||
| "}", | ||
| "", |
There was a problem hiding this comment.
Since we pass KeepFinalNewline to the highlighter, a newline at the end is expected.
| "func AddFeature(key string, isEnabled bool) {", | ||
| "features[strings.ToLower(key)] = isEnabled", | ||
| "}", | ||
| "", |
There was a problem hiding this comment.
Since we pass KeepFinalNewline to the highlighter, a newline at the end is expected.
| "## Installation", | ||
| "Wowza!", | ||
| "See [the main README](https://github.com/dominikh/go-tools#installation) for installation instructions.", | ||
| "", |
There was a problem hiding this comment.
Since we pass KeepFinalNewline to the highlighter, a newline at the end is expected.
| "## Installation", | ||
| "", | ||
| "See [the main README](https://github.com/dominikh/go-tools#installation) for installation instructions.", | ||
| "", |
There was a problem hiding this comment.
Since we pass KeepFinalNewline to the highlighter, a newline at the end is expected.
| "<div><span class=\"hl-text hl-html hl-markdown\">\n</span></div>", | ||
| "<div><span class=\"hl-text hl-html hl-markdown\">\n</span></div>", | ||
| "<div><span class=\"hl-text hl-html hl-markdown\">(c) Copyright Sourcegraph 2013-2021.</span></div>", | ||
| "<div><span class=\"hl-text hl-html hl-markdown\">\n</span></div>", |
There was a problem hiding this comment.
Since we pass KeepFinalNewline to the highlighter, a newline at the end is expected.
| "<div><span class=\"hl-text hl-html hl-markdown\">\n</span></div>", | ||
| "<div><span class=\"hl-text hl-html hl-markdown\">Detailed documentation can be found on\n</span></div>", | ||
| "<div><span class=\"hl-text hl-html hl-markdown\">[staticcheck.io](https://staticcheck.io/docs/).\n</span></div>", | ||
| "<div><span class=\"hl-text hl-html hl-markdown\">\n</span></div>", |
There was a problem hiding this comment.
Since we pass KeepFinalNewline to the highlighter, a newline at the end is expected.
| type highlightedDiffHunkLineKind int | ||
|
|
||
| const ( | ||
| highlightedDiffHunkLineKindUnchanged highlightedDiffHunkLineKind = iota | ||
| highlightedDiffHunkLineKindAdded | ||
| highlightedDiffHunkLineKindDeleted | ||
| ) |
There was a problem hiding this comment.
Made this int so we don't have to hoard so much memory for larger files.
|
|
||
| const viewFilesCommitElement = node.tree && ( | ||
| <div className="d-flex justify-content-between"> | ||
| <div className="d-flex justify-content-between align-items-start"> |
| size={small ? 'sm' : undefined} | ||
| variant="secondary" | ||
| outline={diffMode !== 'unified'} | ||
| className="mb-0" |
| size={small ? 'sm' : undefined} | ||
| variant="secondary" | ||
| outline={diffMode !== 'split'} | ||
| className="mb-0" |
| async event => { | ||
| event.preventDefault() | ||
|
|
||
| if (!state) { |
There was a problem hiding this comment.
Fixes that the button is not clickable again after an error
| fragment ChangesetDiffFields on ExternalChangeset { | ||
| currentSpec { | ||
| description { | ||
| __typename |
There was a problem hiding this comment.
The download patch button was broken because a typename assertion didn't hold. Sadly our type generator always includes typename but the backend doesn't 🙃
| } | ||
|
|
||
| func applyPatch(fileContent string, fileDiff *diff.FileDiff) string { | ||
| // applyPatch takes the contents of a file and a file diff to apply to it. It |
There was a problem hiding this comment.
Addressed issues in this method around patches without newlines in either base or head.
| paths: string[] | null | ||
| }): Observable<RepositoryComparisonDiff['comparison']['fileDiffs']> { | ||
| return queryGraphQL( | ||
| return requestGraphQL<RepositoryComparisonDiffResult, RepositoryComparisonDiffVariables>( |
There was a problem hiding this comment.
was deprecated, moved to new method.
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff a24d341...cb21493.
|
LawnGnome
left a comment
There was a problem hiding this comment.
This is a big set of cleanups!
The applyPatch changes are the kind of thing that I generally find mind melting, so good job there in particular. I do have a question around test coverage, but nothing that would stop us from merging this.
| // Note: Has no newline at the end. | ||
| wantFile: `No newline after this | ||
| Also no newline after this`, | ||
| }, |
There was a problem hiding this comment.
This testing coverage is great, so please don't hurt me for asking, but do you think we need test cases for some of those hunk-related special cases as well?
There was a problem hiding this comment.
sure, will add one, thanks!
There was a problem hiding this comment.
oh wait, I thought this was pointing to a different test - what exactly do you want to see covered here?
There was a problem hiding this comment.
There are a few branches we're not really exercising right now. (Which, to be clear, I realise you inherited in some cases, rather than being new code, but this feels like a good opportunity to make our test coverage a bit more complete here.)
I think the one that caught my eye was adding the original lines if the last hunk isn't at the end of the file — I remember writing the equivalent code for the CVS importer and not quite getting the nuances right the first time, so based on a sample size of 1 case of my own incompetence, I think we might want a test or two to cover that. 😄
For the record, I just threw gobco at this and it spat out the following branches we're not exercising:
cmd/frontend/graphqlbackend/preview_repository_comparison.go:150:5: condition "diffPathOrNull(fileDiff.NewName) == nil" was once false but never true
cmd/frontend/graphqlbackend/preview_repository_comparison.go:161:5: condition "len(fileContent) > 0" was once true but never false
cmd/frontend/graphqlbackend/preview_repository_comparison.go:175:10: condition "i == len(fileDiff.Hunks)-1" was 3 times true but never false
cmd/frontend/graphqlbackend/preview_repository_comparison.go:181:6: condition "hunk.OrigStartLine != 0 && hunk.OrigStartLine != currentLine" was once true but never false
cmd/frontend/graphqlbackend/preview_repository_comparison.go:181:6: condition "hunk.OrigStartLine != 0" was once true but never false
cmd/frontend/graphqlbackend/preview_repository_comparison.go:181:33: condition "hunk.OrigStartLine != currentLine" was once true but never false
cmd/frontend/graphqlbackend/preview_repository_comparison.go:198:9: condition "strings.HasPrefix(line, \"+\")" was 3 times true but never false
cmd/frontend/graphqlbackend/preview_repository_comparison.go:199:8: condition "isLastHunk(i) && hunkHasFinalNewline" was 3 times true but never false
cmd/frontend/graphqlbackend/preview_repository_comparison.go:199:8: condition "isLastHunk(i)" was 3 times true but never false
cmd/frontend/graphqlbackend/preview_repository_comparison.go:199:25: condition "hunkHasFinalNewline" was 3 times true but never false
cmd/frontend/graphqlbackend/preview_repository_comparison.go:215:44: condition "origLines > 0 && origLines != currentLine-1" was once false but never true
cmd/frontend/graphqlbackend/preview_repository_comparison.go:215:44: condition "origLines > 0" was once true but never false
cmd/frontend/graphqlbackend/preview_repository_comparison.go:215:61: condition "origLines != currentLine-1" was once false but never true
cmd/frontend/graphqlbackend/preview_repository_comparison.go:218:6: condition "origHasFinalNewline" was never evaluated
cmd/frontend/graphqlbackend/preview_repository_comparison.go:231:5: condition "lastHunkHadNewlineInLastAddition" was once true but never false
To be clear, I do not think we should add tests for all of these. For example, the first one probably has no value in being tested. But I do think it's worth glancing over each of these and ensuring we're comfortable not having test coverage for them, or adding a test as needed.
There was a problem hiding this comment.
ah, thanks for the detailed outline (and showing gobco, very cool), I'll address that :)
Trust me, that was a hard one 😀 |
| // Note: Has no newline at the end. | ||
| wantFile: `No newline after this | ||
| Also no newline after this`, | ||
| }, |
Started into an investigation after this thread: https://sourcegraph.slack.com/archives/CMMTWQQ49/p1669287139450499
Found quite a few things on the way and fixed them. See inline comments.
Before:
After:
Test plan
Added and adjusted test suites.