config: separate file and snapshot backends#5186
Merged
pks-t merged 7 commits intolibgit2:masterfrom Aug 1, 2019
Merged
Conversation
tiennou
approved these changes
Jul 25, 2019
Contributor
tiennou
left a comment
tiennou
left a comment
There was a problem hiding this comment.
LGTM, a few nitpicks, as ever 😉.
src/config_file.c
Outdated
| return error; | ||
| } | ||
|
|
||
| static int config_iterator_new_readonly( |
| if (res < 0 || (res = config_read(b->header.entries, repo, &b->file, level, 0)) < 0) { | ||
| git_config_entries_free(b->header.entries); | ||
| b->header.entries = NULL; | ||
| if (res < 0 || (res = config_read(b->entries, repo, &b->file, level, 0)) < 0) { |
Contributor
There was a problem hiding this comment.
I don't think res can be negative here, as l126 would have handled it.
Member
Author
There was a problem hiding this comment.
True. I've tried to refrain from updating existing code though to more clearly highlight what changed across the moves.
src/config_entries.c
Outdated
| int error; | ||
|
|
||
| dup = git__calloc(1, sizeof(git_config_entry)); | ||
| GIT_ERROR_CHECK_ALLOC(dup); |
Contributor
There was a problem hiding this comment.
Good catch. I'm surprised Coverity didn't complain, and noticed that it doesn't seem to be receiving updates ("May", it says)…
Member
Author
There was a problem hiding this comment.
Maybe this is related to our move to Azure Pipelines?
Member
There was a problem hiding this comment.
🙁 It was definitely working at one point, then coverity went offline. I'll take a look when I have a moment.
When duplicating a configuration entry, we allocate a new entry but do not verify that we get a valid pointer back. As we're dereferencing the pointer afterwards, we might thus run into a segfault in out-of-memory situations. Extract a new function `git_config_entries_dup_entry` that handles the complete entry duplication. Fix the error by using `GIT_ERROR_CHECK_ALLOC`.
pks-t
force-pushed
the
pks/config-snapshot-separation
branch
from
65c0577 to
3af7e5c
Compare
The `config_readonly_open` function currently receives as input a diskfile backend and will copy its entries to a new snapshot. This is rather intimate, as we need to assume that the source config backend is in fact a diskfile entry. We can do better than this though by using generic methods to copy contents of the provided backend, e.g. by using a config iterator. This also allows us to decouple the read-only backend from the read-write backend.
While most functions of the readonly configuration backend are implemented separately from the writeable configuration backend, the two functions `config_iterator_new` and `config_get` are shared between both. This sharing makes it necessary to have some shared data structures, which is the `diskfile_header` structure. Unfortunately, this makes the backends harder to grasp than necessary due to all the casting between structs and also quite error prone. Reimplement those functions for the readonly backends. As readonly backends cannot be refreshed anyway, we can remove the calls to `config_refresh` in there.
The `diskfile_header` structure is shared between both `diskfile_backend` and `diskfile_readonly_backend`. The separation and resulting casting is confusing at times and a source for programming errors. Remove the shared structure and inline them directly.
In `backend_readonly_free`, the passed in config backend is being cast to a `diskfile_backend` instead of to a `diskfile_readonly_backend`. While this works out just fine because we only access its header values, which were shared between both backends, it is undefined behaviour. Use the correct type to fix this.
To further distinguish the file writeable and readonly backends, separate the readonly backend into its own "config_snapshot.c" implementation. The snapshot backend can be generically used to snapshot any type of backend.
The internal backend structures are kind-of legacy and do not really speak for themselves. Rename them accordingly to make them easier to understand.
pks-t
force-pushed
the
pks/config-snapshot-separation
branch
from
3af7e5c to
37ebe9a
Compare
Member
Author
|
Fixed all your comments except the one issue that was pre-existing, thanks a lof for your review! I've also fixed a memory leak that I've introduced |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This completely tears apart the shared code between config file (
diskfile_backend) and config snapshots (diskfile_readonly_backend). The previous code was hard to understand with thediskfile_headerstructure, error prone and didn't have proper separation of concerns. With this, we now have a completely backend-agnostic snapshotting backend that can create a read-only snapshot of any other config backend.This PR doesn't yet go the extra step of making use of that extra functionality, but only does refactorings that should keep all semantics in place. But hopefully with less bugs and more enlightenment when one reads the code.
/cc @csware