Skip to content

Fix file-blob comparison - #7997

Merged
alexperovich merged 1 commit into
dotnet:mainfrom
alexperovich:fixFileBlobCompare
Oct 6, 2021
Merged

Fix file-blob comparison#7997
alexperovich merged 1 commit into
dotnet:mainfrom
alexperovich:fixFileBlobCompare

Conversation

@alexperovich

Copy link
Copy Markdown
Member

To double check:

@epananth

epananth commented Oct 6, 2021

Copy link
Copy Markdown
Member

Can we do a test against any of the already existing builds?

@MattGal

MattGal commented Oct 6, 2021

Copy link
Copy Markdown
Member

seems like it fixes some bug that repros when the exact file size is a multiple of 1 MB, but where is the bug this is fixing?

@epananth

epananth commented Oct 6, 2021

Copy link
Copy Markdown
Member

@MattGal MattGal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now that I've seen the failure being fixed I'm fairly convinced it'll fix it; please consider actually filing that bug for posterity's sake.

@epananth

epananth commented Oct 6, 2021

Copy link
Copy Markdown
Member

Created the issue -> #8004

@alexperovich

Copy link
Copy Markdown
Member Author

Can we do a test against any of the already existing builds?

I tested this against the failing build I had.

@alexperovich

Copy link
Copy Markdown
Member Author

seems like it fixes some bug that repros when the exact file size is a multiple of 1 MB, but where is the bug this is fixing?

This is a bug that always happens when this code path happens. The loop continues until the Read returns 0 indicating EOF, but the call to read 0 from the blob still happens and fails.

@alexperovich
alexperovich merged commit d319c25 into dotnet:main Oct 6, 2021
@alexperovich
alexperovich deleted the fixFileBlobCompare branch October 6, 2021 21:20
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.

4 participants