Skip to content

Conversation

@eseliger
Copy link
Member

@eseliger eseliger commented Nov 24, 2022

Patches can contain non UTF-8 characters (who knew). Since we JSON-encode the patch as a string field, we lose that encoding over the wire to Sourcegraph. This PR adds support for a byte slice encoded (so base64 over the wire) mode from Sourcegraph 4.4 onwards, which retains original encoding.

Test plan

Validated this src-cli version still works with older Sourcegraph backends (then without this fix), and with a current Sourcegraph backend where this fixes patches with non UTF-8. Tested with src-cli execution and SSBC.

@@ -1,3 +1,3 @@
golang 1.18.1
golang 1.19.3
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to upgrade, Sourcegraph now requires it.

@eseliger eseliger changed the title Binary diffs Support binary diffs Nov 24, 2022
@eseliger eseliger marked this pull request as ready for review November 24, 2022 19:19
@eseliger eseliger requested a review from a team November 24, 2022 19:19
Copy link
Contributor

@LawnGnome LawnGnome left a comment

Choose a reason for hiding this comment

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

I haven't tested this yet — I'll do that after I've reviewed the backend PR — but this LGTM. I'm sort of looking at the []byte() conversions that are getting removed here and there and kicking myself that we didn't do this (much) sooner.

Client: opts.client,
})

ffs, err := svc.DetermineFeatureFlags(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

ffs also accurately reflects my response to the original Slack thread.

@eseliger eseliger enabled auto-merge (squash) November 29, 2022 02:25
@eseliger eseliger disabled auto-merge November 29, 2022 02:25
@eseliger eseliger enabled auto-merge (squash) November 29, 2022 02:28
@eseliger eseliger disabled auto-merge November 29, 2022 02:29
@eseliger eseliger merged commit e765697 into main Nov 29, 2022
@eseliger eseliger deleted the es/binary-diffs branch November 29, 2022 02:33
scjohns pushed a commit that referenced this pull request Apr 24, 2023
Patches can contain non UTF-8 characters (who knew). Since we JSON-encode the patch as a string field, we lose that encoding over the wire to Sourcegraph. This PR adds support for a byte slice encoded (so base64 over the wire) mode from Sourcegraph 4.4 onwards, which retains original encoding.
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.

3 participants