Skip to content

Configuration file fixes with includes#4250

Merged
ethomson merged 7 commits intolibgit2:masterfrom
pks-t:pks/config-file-iteration
Jul 19, 2017
Merged

Configuration file fixes with includes#4250
ethomson merged 7 commits intolibgit2:masterfrom
pks-t:pks/config-file-iteration

Conversation

@pks-t
Copy link
Copy Markdown
Member

@pks-t pks-t commented May 30, 2017

This fixes the issues causing #4247, but it does not yet fix additional issues with includes. One more issue I found was that upon refreshing a config file, we'll duplicate the includes inside the readers struct, if I'm not mistaken. I'll try to tackle this problem tomorrow, alongside with some tests for the broken behaviour.

@pks-t pks-t force-pushed the pks/config-file-iteration branch 4 times, most recently from 4ed47d1 to 926fe18 Compare May 31, 2017 21:27
@pks-t
Copy link
Copy Markdown
Member Author

pks-t commented May 31, 2017

As ever so often, this PR has grown quite a lot again as several other issues have popped up :P So I've started refactoring our configuration includes code to be easier to maintain and fix these bugs. I hope that these refactorings might ease further improvements alongside with include files,for example when modifying variables in the correct config file.

But yeah, I'm done with that code for now, not to keen on revisiting it too soon 🙂

@pks-t pks-t mentioned this pull request Jun 6, 2017
@pks-t pks-t force-pushed the pks/config-file-iteration branch 3 times, most recently from 9c3ea27 to 00d0697 Compare June 9, 2017 07:06
@csware
Copy link
Copy Markdown
Contributor

csware commented Jul 1, 2017

Any plans when this gets merged?

@pks-t
Copy link
Copy Markdown
Member Author

pks-t commented Jul 10, 2017

While I'd like to get it merged soon-ish, I won't merge without some thorough review as it touches quite some internals of how we handle config files. So doing reviews would definitly accelerate getting this PR ready.

pks-t added 7 commits July 15, 2017 14:14
Current code for configuration files uses the `reader` structure to
parse configuration files and store additional metadata like the file's
path and checksum. These structures are stored within an array in the
backend itself, which causes multiple problems.

First, it does not make sense to keep around the file's contents with
the backend itself. While this data is usually free'd before being added
to the backend, this brings along somewhat intricate lifecycle problems.
A better solution would be to store only the file paths as well as the
checksum of the currently parsed content only.

The second problem is that the `reader` structures are stored inside an
array. When re-parsing configuration files due to changed contents, we
may cause this array to be reallocated, requiring us to update pointers
hold by callers. Furthermore, we do not keep track of includes which
are already associated to a reader inside of this array. This causes us
to add readers multiple times to the backend, e.g. in the scenario of
refreshing configurations.

This commit fixes these shortcomings. We introduce a split between the
parsing data and the configuration file's metadata. The `reader` will
now only hold the file's contents and the parser state and the new
`config_file` structure holds the file's path and checksum. Furthermore,
the new structure is a recursive structure in that it will also hold
references to the files it directly includes. The diskfile is changed to
only store the top-level configuration file.

These changes allow us further refactorings and greatly simplify
understanding the code.
Previously, the callbacks passed to `config_parse` got the reader via a
pointer to a pointer. This allowed the callbacks to update the callers
`reader` variable when the array holding it has been reallocated. As the
array is no longer present, we can simply the code by making the reader
a simple pointer.
The backend passed to `config_read` is never actually used anymore, so
we can remove it from the function and the `parse_data` structure.
Currently, we only re-parse the top-level configuration file when it has
changed itself. This can cause problems when an include is changed, as
we were not updating all values correctly.

Instead of conditionally reparsing only refreshed files, the logic
becomes much clearer and easier to follow if we always re-parse the
top-level configuration file when either the file itself or one of its
included configuration files has changed on disk. This commit implements
this logic.

Note that this might impact performance in some cases, as we need to
re-read all configuration files whenever any of the included files
changed. It could increase performance to just re-parse include files
which have actually changed, but this would compromise maintainability
of the code without much gain. The only case where we will gain anything
is when we actually use includes and when only these includes are
updated, which will probably be quite an unusual scenario to actually be
worthwhile to optimize.
Right now, we have multiple call sites which initialize a `reader`
structure. As the structure is only actually used inside of
`config_read`, we can instead just move the reader inside of the
`config_read` function. Instead, we can just pass in the configuration
file into `config_read`, which eases code readability.
Modifying variables pulled in by an included file currently succeeds,
but it doesn't actually do what one would expect, as refreshing the
configuration will cause the values to reappear. As we are currently not
really able to support this use case, we will instead just return an
error for deleting and setting variables which were included via an
include.
@pks-t pks-t force-pushed the pks/config-file-iteration branch from 00d0697 to 1b32908 Compare July 15, 2017 12:17
@pks-t pks-t mentioned this pull request Jul 15, 2017
@ethomson ethomson merged commit e056862 into libgit2:master Jul 19, 2017
@ethomson
Copy link
Copy Markdown
Member

🎉

@pks-t pks-t deleted the pks/config-file-iteration branch July 21, 2017 09:24
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.

3 participants