Re: [PATCH v5 1/1] config: add conditional include
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>> There was some discussion after v4. I think the open issues are:
>>
>> - the commit message is rather terse (it should describe motivation,
>> and can refer to the docs for the "how")
> This allows some more flexibility in managing configuration across
> repositories.
That is not an ideal opening to describe motivation without people knowing what "this" is ;-) Of course, the person who wrote the sentence know it already after writing the patch and the subject, but others don't.
Sometimes a set of repositories want to share configuration
settings among themselves that are distinct from other such
sets of repositories. A user may work on two projects, each
of which have multiple repositories, and use one user.email
for one project while using another for the other. Having
the settings in ~/.gitconfig, which would work for just one
set of repositories, would not well in such a situation.
Extend the include.path mechanism that lets a config file
include another config file, so that the inclusion can be
done only when some conditions hold. Then ~/.gitconfig can
say "include config-project-A only when working on project-A"
for each project A the user works on. In this patch, the only supported grouping is based on
$GIT_DIR (in absolute path), so you would need to group
repositories by directory, or something like that to take
advantage of it. We already have include.path for unconditional
includes. This patch goes with include-if.xxx.path to make
it clearer that a condition is required. Similar to include.path, older git versions that don't
understand include-if will simply ignore them.or something along that line?
Show 7 quoted lines
> +Conditional includes
> +~~~~~~~~~~~~~~~~~~~~
> +
> +You can include one config file from another conditionally by setting
> +a special `include-if.<condition>.path` variable to the name of the
> +file to be included. The variable is treated the same way as
> +`include.path`.
Drop "special", as all configuration variables are "special" in their own sense, it does not add any useful information.
I think we avoid '-' in Git-native variable and section names, so "include-if" would become an odd-man-out.
The variable is obviously not treated the same way as include.path ;-)
When includeIf.<condition>.path variable is set in a
configuration file, the configuration file named by that
variable is included (in a way similar to how include.path
works) only if the <condition> holds true.> +The condition starts with a keyword, followed by a colon and a
> +pattern. The interpretation of the pattern depends on the keyword.
"a pattern"? I think it is "some data whose format and meaning depends on the keyword".
Hence...
Show 5 quoted lines
> +Supported keywords are:
> +
> +`gitdir`::
> + The environment variable `GIT_DIR` must match the following
> + pattern for files to be included. The pattern can contain
The data that follows the keyword `gitdir:` is used as a
glob pattern. If the location of the .git directory (which
may be auto-discovered, or come from `GIT_DIR` environment
variable) match the pattern, the `<condition>` becomes true.
Show 10 quoted lines
> + ...
> + * If the pattern ends with `/`, `**` will be automatically added. For
> + example, the pattern `foo/` becomes `foo/**`. In other words, it
> + matches "foo" and everything inside, recursively.
> +
> +`gitdir/i`::
> + This is the same as `gitdir` except that matching is done
> + case-insensitively (e.g. on case-insensitive file sytems)
> +
> +A few more notes on matching:
As future <keywords> may not be about path or matching at all, this belongs to `gitdir`:: section (and it would be obvious that that applies to `gitdir/i`:: because we say "this is the same as...").
> + * Symlinks in `$GIT_DIR` are not resolved before matching.
> +
> + * Note that "../" is not special and will match literally, which is
> + unlikely what you want.
Show 11 quoted lines
> +static int prepare_include_condition_pattern(struct strbuf *pat)
> +{
> + struct strbuf path = STRBUF_INIT;
> + int prefix = 0;
> +
> + /* TODO: maybe support ~user/ too */
> + if (pat->buf[0] == '~' && is_dir_sep(pat->buf[1])) {
> + const char *home = getenv("HOME");
> +
> + if (!home)
> + return error(_("$HOME is not defined"));Instead of half-duplicating it here yourself, can't we let expand_user_path() do its thing?
Show 10 quoted lines
> +static int include_condition_is_true(const char *cond, size_t cond_len)
> +{
> + /* no condition (i.e., "include.path") is always true */
> + if (!cond)
> + return 1;
> +
> + if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len))
> + return include_by_gitdir(cond, cond_len, 0);
> + else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
> + return include_by_gitdir(cond, cond_len, 1);This may be OK for now, but it should be trivial to start from a table with two entries, i.e.
struct include_cond {
const char *keyword;
int (*fn)(const char *, size_t);
};and will show a better way to do things to those who follow your footsteps.
Nguyễn Thái Ngọc Duy
