Re: [PATCH v6 1/1] config: add conditional include

[Philip Oakley]

From: "Duy Nguyen" 
On Sat, Feb 25, 2017 at 5:08 AM, Philip Oakley  
wrote:

+Conditional includes
+
+
+You can include one config file from another conditionally by setting



On first reading I thought this implied you can only have one `includeIf`
within the config file.
I think it is meant to mean that each `includeIf`could include one other
file, and that users can have multiple `includeIf` lines.


Yes. Not sure how to put it better though (I basically copied the
first paragraph from the unconditional include section above, which
shares the same confusion). Perhaps just write "the variable can be
specified multiple times"? Or "multiple variables include multiple
times, the last variable does not override the previous ones"?
--


My attempt, based on updating the `Includes` section would be something 
like:


`You can include a config file from another by setting the special 
`include.path` variable to the name of the file to be included. The variable 
takes a pathname as its value, and is subject to tilde expansion. 
`include.path` supports multiple key values.`


The subtle change was to s/one/a/ at the start, and then add the final short 
sentence that states that the section's variables can have multiple key 
values.


I copied the 'multiple key values' phrase from the man page intro for 
consitency, though 'multivalued' could just as easily be used as it is the 
term used by the 'Configuration File' section that this is part of 
https://git-scm.com/docs/git-config#_configuration_file.


Even shorter may be:
`You can include a config file from another by setting the special 
`include.path` variable to the name of the file to be included. The variable 
(can be multivalued) takes a pathname as its value, and is subject to tilde 
expansion.`



The Conditional Includes would follow suit.

Philip

Re: [PATCH v6 1/1] config: add conditional include

[Jeff King]

On Fri, Feb 24, 2017 at 08:14:25PM +0700, Nguyễn Thái Ngọc Duy wrote:

> +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:", , _len))
> + return include_by_gitdir(cond, cond_len, 0);
> + else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", , _len))
> + return include_by_gitdir(cond, cond_len, 1);
> +
> + error(_("unrecognized include condition: %.*s"), (int)cond_len, cond);
> + /* unknown conditionals are always false */
> + return 0;
> +}

This "error()" is a nice warning to people who have misspelled the
condition (or are surprised that an old version does not respect their
condition). But it seems out of place with the rest of the compatibility
strategy, which is to quietly ignore include types we don't understand.

For example, if your patch ships in v2.13 and we add the "foo:"
condition in v2.14, then with config like:

  [includeif "foo:bar"]
  path = whatever

you get:

  v2.12: silently ignore
  v2.13: loudly ignore
  v2.14: works

which seems odd.

If we do keep it, it should probably be warning(). I would expect
"error:" to indicate that some requested operation could not be
completed, but this is just "by the way, I ignored this garbage".

-Peff

PS I know we try to avoid dashes in the section names, but "includeIf"
   and "includeif" just look really ugly to me. Maybe it is just me,
   though.

Re: [PATCH v6 1/1] config: add conditional include

[Duy Nguyen]

On Sat, Feb 25, 2017 at 5:08 AM, Philip Oakley  wrote:
>> +Conditional includes
>> +
>> +
>> +You can include one config file from another conditionally by setting
>
>
> On first reading I thought this implied you can only have one `includeIf`
> within the config file.
> I think it is meant to mean that each `includeIf`could include one other
> file, and that users can have multiple `includeIf` lines.

Yes. Not sure how to put it better though (I basically copied the
first paragraph from the unconditional include section above, which
shares the same confusion). Perhaps just write "the variable can be
specified multiple times"? Or "multiple variables include multiple
times, the last variable does not override the previous ones"?
-- 
Duy

Re: [PATCH v6 1/1] config: add conditional include

[Philip Oakley]

From: "Nguyễn Thái Ngọc Duy" 

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.

Setting $GIT_DIR/.config works, but if the penalty of forgetting to
update $GIT_DIR/.config is high (especially when you end up cloning
often), it may not be the best way to go. Having the settings in
~/.gitconfig, which would work for just one set of repositories, would
not well in such a situation. Having separate ${HOME}s may add more
problems than it solves.

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 includeIf..path to make it clearer that a condition is
required. The new config has the same backward compatibility approach as
include.path: older git versions that don't understand includeIf will
simply ignore them.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
Documentation/config.txt  | 61 +
config.c  | 97 
+++

t/t1305-config-include.sh | 56 +++
3 files changed, 214 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 015346c417..6c0cd2a273 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -91,6 +91,56 @@ found at the location of the include directive. If the 
value of the

relative to the configuration file in which the include directive was
found.  See below for examples.

+Conditional includes
+
+
+You can include one config file from another conditionally by setting


On first reading I thought this implied you can only have one `includeIf` 
within the config file.
I think it is meant to mean that each `includeIf`could include one other 
file, and that users can have multiple `includeIf` lines.




+a `includeIf..path` variable to the name of the file to be
+included. The variable's value is treated the same way as `include.path`.
+


--
Philip 

Re: [PATCH v6 1/1] config: add conditional include

[Ramsay Jones]

On 24/02/17 13:14, Nguyễn Thái Ngọc Duy wrote:
[snip] 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  Documentation/config.txt  | 61 +
>  config.c  | 97 
> +++
>  t/t1305-config-include.sh | 56 +++
>  3 files changed, 214 insertions(+)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 015346c417..6c0cd2a273 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -91,6 +91,56 @@ found at the location of the include directive. If the 
> value of the
>  relative to the configuration file in which the include directive was
>  found.  See below for examples.
>  
> +Conditional includes
> +
> +
> +You can include one config file from another conditionally by setting
> +a `includeIf..path` variable to the name of the file to be
> +included. The variable's value is treated the same way as `include.path`.
> +
> +The condition starts with a keyword followed by a colon and some data
> +whose format and meaning depends on the keyword. Supported keywords
> +are:
> +
> +`gitdir`::
> +
> + The data that follows the keyword `gitdir:` is used as a glob
> + pattern. If the location of the .git directory match the
> + pattern, the include condition is met.
> ++
> +The .git location which may be auto-discovered, or come from
> +`$GIT_DIR` environment variable. If the repository auto discovered via
> +a .git file (e.g. from submodules, or a linked worktree), the .git
> +location would be the final location, not where the .git file is.
> ++
> +The pattern can contain standard globbing wildcards and two additional
> +ones, `**/` and `/**`, that can match multiple path components. Please
> +refer to linkgit:gitignore[5] for details. For convenience:
> +
> + * If the pattern starts with `~/`, `~` will be substituted with the
> +   content of the environment variable `HOME`.
> +
> + * If the pattern starts with `./`, it is replaced with the directory
> +   containing the current config file.
> +
> + * If the pattern does not start with either `~/`, `./` or `/`, `**/`
> +   will be automatically prepended. For example, the pattern `foo/bar`
> +   becomes `**/foo/bar` and would match `/any/path/to/foo/bar`.
> +
> + * 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 with `gitdir` and `gitdir/i`:
> +
> + * Symlinks in `$GIT_DIR` are not resolved before matching.
> +
> + * Note that "../" is not special and will match literally, which is
> +   unlikely what you want.
>  
>  Example
>  ~~~
> @@ -119,6 +169,17 @@ Example
>   path = foo ; expand "foo" relative to the current file
>   path = ~/foo ; expand "foo" in your `$HOME` directory
>  
> + ; include if $GIT_DIR is /path/to/foo/.git
> + [include-if "gitdir:/path/to/foo/.git"]

s/include-if/includeIf/

> + path = /path/to/foo.inc
> +
> + ; include for all repositories inside /path/to/group
> + [include-if "gitdir:/path/to/group/"]

ditto

> + path = /path/to/foo.inc
> +
> + ; include for all repositories inside $HOME/to/group
> + [include-if "gitdir:~/to/group/"]

ditto

ATB,
Ramsay Jones

Re: [PATCH v6 1/1] config: add conditional include

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> +Conditional includes
> +
> +
> +You can include one config file from another conditionally by setting
> +a `includeIf..path` variable to the name of the file to be
> +included. The variable's value is treated the same way as `include.path`.
> +
> +The condition starts with a keyword followed by a colon and some data
> +whose format and meaning depends on the keyword. Supported keywords
> +are:
> +
> +`gitdir`::
> +
> + The data that follows the keyword `gitdir:` is used as a glob
> + pattern. If the location of the .git directory match the
> + pattern, the include condition is met.

s/match//, I think.

> ++
> +The .git location which may be auto-discovered, or come from

s/ which//?

> +`$GIT_DIR` environment variable. If the repository auto discovered via

s/If the /In a/?

> +a .git file (e.g. from submodules, or a linked worktree), the .git
> +location would be the final location, not where the .git file is.

OK.

> @@ -170,9 +171,99 @@ static int handle_path_include(const char *path, struct 
> config_include_data *inc
>   return ret;
>  }
>  
> +static int prepare_include_condition_pattern(struct strbuf *pat)
> +{
> + ...
> + /* TODO: escape wildcards */
> + strbuf_add_absolute_path(, cf->path);

Is this still TODO?  As this one returns the prefix length (which
probably wants to be commented before the function) and this codepath
computes the prefix to cover the path to here, doesn't caller already
do the right thing?

> +static int include_condition_is_true(const char *cond, size_t cond_len)
> +{
> + /* no condition (i.e., "include.path") is always true */

Does this want to say "includeIf.path" instead?  "include.path" is
done by handle_path_include() without involving a call to this
function.

> + if (!cond)
> + return 1;
> +
> + if (skip_prefix_mem(cond, cond_len, "gitdir:", , _len))
> + return include_by_gitdir(cond, cond_len, 0);
> + else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", , _len))
> + return include_by_gitdir(cond, cond_len, 1);
> +
> + error(_("unrecognized include condition: %.*s"), (int)cond_len, cond);
> + /* unknown conditionals are always false */
> + return 0;
> +}

Thanks.