Removal of p_fallocate
#5114
Merged
Removal of p_fallocate
#5114
Conversation
This file contains 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
tiennou
approved these changes
Jun 14, 2019
tests/object/tree/read.c
Outdated
| @@ -79,46 +79,34 @@ void test_object_tree_read__two(void) | |||
|
|
|||
| void test_object_tree_read__largefile(void) | |||
| { | |||
| git_reference *ref; | |||
| const git_tree_entry *entry; | |||
| git_index_entry ie = { 0 }; | |||
The `p_fallocate` platform is currently in use in our tests, only, but it proved to be quite burdensome to get it implemented in a cross-platform way. The only "real" user is the test object::tree::read::largefile, where it's used to allocate a large file in the filesystem only to commit it to the repo and read its object back again. We can simplify this quite a bit by just using an in-memory buffer of 4GB. Sure, this cannot be used on platforms with low resources. But creating 4GB files is not any better, and we already skip the test if the environment variable "GITTEST_INVASIVE_FS_SIZE" is not set. So we're arguably not worse off than before.
By now, we have repeatedly failed to provide a nice cross-platform implementation of `p_fallocate`. Recent tries to do that escalated quite fast to a set of different CMake checks, implementations, fallbacks, etc., which started to look real awkward to maintain. In fact, `p_fallocate` had only been introduced in commit 4e3949b (tests: test that largefiles can be read through the tree API, 2019-01-30) to support a test with large files, but given the maintenance costs it just seems not to be worht it. As we have removed the sole user of `p_fallocate` in the previous commit, let's drop it altogether.
pks-t
force-pushed
the
pks/bigfile-refactoring
branch
from
e510c8e
to
2d85c7e
Compare
|
On Fri, Jun 14, 2019 at 05:51:27AM -0700, Etienne Samson wrote:
CI [NAKed](https://dev.azure.com/libgit2/libgit2/_build/results?buildId=2095&view=logs&jobId=7351e2b3-4d1a-54ac-0ff4-44bd85f4818d&taskId=49622b32-88bd-5294-a7a5-08280f44966c&lineStart=602&lineEnd=603&colStart=1&colEnd=1) this.
I hit this issue so many times already. I always wonder why it is
that CI catches those things while my own system doesn't. I mean
we're both using GCC -- are they setting up extra CFLAGS?
Anyway, should be fixed now, thanks for catching this.
|
Huh. Maybe? More likely same CFLAGS but different compiler versions? |
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.
The
p_fallocateplatform is currently in use in our tests,only, but it proved to be quite burdensome to get it implemented
in a cross-platform way. The only "real" user is the test
object::tree::read::largefile, where it's used to allocate a
large file in the filesystem only to commit it to the repo and
read its object back again. We can simplify this quite a bit by
just using an in-memory buffer of 4GB. Sure, this cannot be used
on platforms with low resources. But creating 4GB files is not
any better, and we already skip the test if the environment
variable "GITTEST_INVASIVE_FS_SIZE" is not set. So we're arguably
not worse off than before.
Supersedes #5087