Skip to content

Support binary diffs#887

Merged
eseliger merged 10 commits into
mainfrom
es/binary-diffs
Nov 29, 2022
Merged

Support binary diffs#887
eseliger merged 10 commits into
mainfrom
es/binary-diffs

Conversation

@eseliger

@eseliger eseliger commented Nov 24, 2022

Copy link
Copy Markdown
Member

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.

Comment thread .tool-versions
@@ -1,3 +1,3 @@
golang 1.18.1
golang 1.19.3

Copy link
Copy Markdown
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.

Comment thread go.mod Outdated
Comment thread internal/batches/features.go Outdated
@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

@LawnGnome LawnGnome left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread cmd/src/batch_common.go
Client: opts.client,
})

ffs, err := svc.DetermineFeatureFlags(ctx)

Copy link
Copy Markdown
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.

Comment thread internal/batches/features.go Outdated
@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.

2 participants