Skip to content

Conditional includes#4332

Merged
carlosmn merged 11 commits into
libgit2:masterfrom
pks-t:pks/conditional-includes
Nov 4, 2017
Merged

Conditional includes#4332
carlosmn merged 11 commits into
libgit2:masterfrom
pks-t:pks/conditional-includes

Conversation

@pks-t
Copy link
Copy Markdown
Member

@pks-t pks-t commented Aug 25, 2017

I've accidentally closed #4244 as I have deleted my branch. Re-opening here.

@pks-t pks-t force-pushed the pks/conditional-includes branch from a857842 to 7089a9b Compare August 26, 2017 09:51
@pks-t
Copy link
Copy Markdown
Member Author

pks-t commented Aug 26, 2017

Rebased to fix conflicts.

pks-t added 6 commits October 9, 2017 11:19
This function has previously been implemented in Windows-specific path
handling code as `path__is_absolute`. As we will need this functionality
in other parts, extract the logic into "path.h" alongside with a
non-Windows implementation.
This function has previously been implemented in Windows-specific path
handling code as `path__is_dirsep`. As we will need this functionality
in other parts, extract the logic into "path.h" alongside with a
non-Windows implementation.
Several functions to retrieve variables from a repository only return
immutable values, which allows us to actually constify the passed-in
repository parameter. Do so to help a later patch, which will only have
access to a constant repository.
Our current configuration logic is completely oblivious of any
repository, but only cares for actual file paths. Unfortunately, we are
forced to break this assumption by the introduction of conditional
includes, which are evaluated in the context of a repository. Right now,
only one conditional exists with "gitdir:" -- it will only include the
configuration if the current repository's git directory matches the
value passed to "gitdir:".

To support these conditionals, we have to break our API and make the
repository available when opening a configuration file. This commit
extends the `open` call of configuration backends to include another
repository and adjusts existing code to have it available. This includes
the user-visible functions `git_config_add_file_ondisk` and
`git_config_add_backend`.
The logic inside this function will be required later on, when
implementing conditional includes. Extract it into its own function to
ease the implementation.
The reader machinery will be extended to handle conditional includes.
The only conditions that currently exist all match the against the git
directory of the repository the config file belongs to. As such, we need
to have access to the repository when reading configuration files to
properly handle these conditions.

One specialty of thes conditional includes is that the actual pattern
may also be a relative pattern starting with "./". In this case, we have
to match the pattern against the path relative to the config file which
is currently being parsed. So besides the repository, we also have to
pass down the path to the current config file that is being parsed.
@pks-t pks-t force-pushed the pks/conditional-includes branch 2 times, most recently from fc9c473 to 3ace427 Compare October 9, 2017 09:55
pks-t added 2 commits October 9, 2017 12:48
Upstream git.git has implemented the ability to include other
configuration files based on conditions. Right now, this only includes
the ability to include a file based on the gitdir-location of the
repository the currently parsed configuration file belongs to. This
commit implements handling these conditional includes for the
case-sensitive "gitdir" condition.
Next to the "gitdir" conditional for including other configuration
files, there's also a "gitdir/i" conditional. In contrast to the former
one, path matching with "gitdir/i" is done case-insensitively. This
commit implements the case-insensitive condition.
@pks-t pks-t force-pushed the pks/conditional-includes branch from 3ace427 to f7d837c Compare October 9, 2017 10:48
@pks-t
Copy link
Copy Markdown
Member Author

pks-t commented Oct 9, 2017

I've fixed the issue on Windows now, but tests on macOS currently fail. I have no idea why that is and am unfortunately unable to debug into that issue due to not having a working macOS setup right now. :(

@carlosmn
Copy link
Copy Markdown
Member

The error on macOS comes from a peculiarity of how it handles temporary paths. The clar_sandbox_path() is (for one example)

/var/folders/q8/y3fc7wsx7f5cl7pdft67g_f40000gn/T/libgit2_tests_KbEJqZ/

whereas the git_repoistory_path() is

/private/var/folders/q8/y3fc7wsx7f5cl7pdft67g_f40000gn/T/libgit2_tests_KbEJqZ/empty_standard_repo/.git/

The reason we get different paths is that we perform a whole bunch of realpath(3) calls when we open the repository which translate the "virtual" path into a more real one.

We put our repository in the temporary directory which makes macOS map the path
into a virtual path. `realpath(3)` can resolve it and we do so during repository
opening, but that makes its path have a different prefix from the sandbox path
clar thinks we have.

Resolve the sandbox path before putting it into the test config files so the
paths match as expected.
Comment thread src/config_file.c Outdated
} diskfile_readonly_backend;

static int config_read(git_strmap *values, struct config_file *file, git_config_level_t level, int depth);
static int config_read(const git_repository *repo, git_strmap *values, struct config_file *file, git_config_level_t level, int depth);
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.

The values map is the output parameter, isn't it? The repository should come later in the arg list.

Comment thread src/config_file.c
return 0;

condition = git__substrdup(section + strlen("includeIf."),
strlen(section) - strlen("includeIf.") - strlen(".path"));
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.

This might just be me, but I prefer the method used in the normalisation which searches for the first and last '.' to figure out how long the middle part is without making us count hard-coded strings.

@carlosmn carlosmn merged commit 0d723f3 into libgit2:master Nov 4, 2017
@carlosmn
Copy link
Copy Markdown
Member

carlosmn commented Nov 4, 2017

I went ahead and did the arg-order change. Yay for features.

@pks-t
Copy link
Copy Markdown
Member Author

pks-t commented Nov 6, 2017

The parameter reordering makes sense. Thanks for improving and merging!

@csware
Copy link
Copy Markdown
Contributor

csware commented Nov 6, 2017

Why pass a repo object into git_config_functions and not just a path? This would allow to construct git_config objects w/o intantiating a repo first.

@pks-t pks-t deleted the pks/conditional-includes branch November 11, 2017 20:18
@carlosmn
Copy link
Copy Markdown
Member

Because we don't know what else git is going to come up with and adding a path would mean that we're going to have to constantly change the interface.

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