Write directly to the ODB when possible #724
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| [Theory] | ||
| [InlineData(16, 32)] | ||
| [InlineData(7854, 9785)] | ||
| public void CreatingABlobTooShortAStreamThrows(int contentSize, int numberOfBytesToConsume) |
There was a problem hiding this comment.
CreatingABlob From TooShortAStreamThrows?
|
Rebased. Ok to merge? |
| @@ -148,7 +155,8 @@ public int Provider(IntPtr content, int max_length, IntPtr data) | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// Inserts a <see cref="Blob"/> into the object database, created from the content of a data provider. | |||
| /// Inserts a <see cref="Blob"/> into the object database, created from the content of a stream. | |||
| /// <para>Optionally, git filters will be applied to the content prior it's stored.</para> | |||
There was a problem hiding this comment.
there's a word missing here. Either "prior to being stored" or "before storing it" or something work work.
ObjectDatabase.CreateBlob() accepts a number of bytes to read. It currently however treats this as a max, rather than a hard size, which seems ripe for introducing bugs. Assert that we should throw when asked to read too much from a Stream.
When given a size and no path, we know that we do not need to buffer the content or apply any filters, so we can create an write-stream into the object database and put in our content directly, avoiding the temporary file and callbacks altogether.
|
💥 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While trying to get rid of the uglyness that is
git_blob_create_fromchunks(), I realised we can do it bit by bit, so here's the first bit.If we're given no path (and thus cannot do any filtering) and a size, we know that we can stream the data directly into the database, so let's do that with a
git_odb_stream.This brought up the fact that we expect a short read to succeed, which is only possible because
fromchunkswaits for an EOF signal from us, which we can simply pass along fromStream.Read(). Trying to keep this behaviour would make it impossible to optimise anything, and it also seems like a great place to introduce subtle bugs, so I've changed the tests to reflect that we need to read that amount of bytes.This however is still the behaviour for the case when we have a path, since that needs filtering and thus must still rely on
fromchunks. This however means that the place where we would raise the exception is beyond a C wall, so that probably wouldn't work without storing the exception somewhere.