Is there interest in reading ~/.gitconfig.d/* and /etc/gitconfig.d/*?

Is there interest in reading ~/.gitconfig.d/* and /etc/gitconfig.d/*?

[Ævar Arnfjörð Bjarmason]

I've patched Git to read ~/.gitconfig.d/* and /etc/gitconfig.d/* in
addition to reading ~/.gitconfig and /etc/gitconfig where it would
normally do so. the gitconfig.d/* files are read in numeric sort order
as is the custom with such files and read after the main .gitconfig
file so they override it.

This is very useful for me because I maintain my .gitconfig in a
public git repository but I don't want anyone to know my github.token.
Because of that I have to do a little song & dance every time I commit
to the file (if the token is part of the diff) or pull to the file (to
not encounter a merge conflict).

This would also facilitate neat stuff like easily using bits of other
people's gitconfig libraries by cloning their repository somewhere and
symlinking their .gitconfig to ~/.gitconfig.d/99-some-library.

I'm not attaching my patch because it would be inhumane to subject
anyone to it in its current state. However, I'd like to ask if there's
any interest in getting a proper & documented patch for this. If so I
can submit a proper patch, if not I can just continue to patch my Git
build with my hack.

Apologies if this has been brought up before. I tried to search for
prior art but ".gitconfig.d" makes for a really bad search query in
all the search engines I could find that index this list.

Re: Is there interest in reading ~/.gitconfig.d/* and /etc/gitconfig.d/*?

[Heiko Voigt]

On Thu, Apr 01, 2010 at 09:20:46PM +0000, Ævar Arnfjörð Bjarmason wrote:
> I've patched Git to read ~/.gitconfig.d/* and /etc/gitconfig.d/* in
> addition to reading ~/.gitconfig and /etc/gitconfig where it would
> normally do so. the gitconfig.d/* files are read in numeric sort order
> as is the custom with such files and read after the main .gitconfig
> file so they override it.

I like the idea and it would be great if we could extend it to hooks as
well. I know that you can implement hooks that themself read such a
structure but its not that neat as git handling this itself.

That would be a great way to distribute configurations to users as you
could say: "Put this file into your gitconfig.d/hooks.d folder and you
get this and that kind of configuration."

cheers Heiko

Re: Is there interest in reading ~/.gitconfig.d/* and /etc/gitconfig.d/*?

[Peter Krefting]

Ævar Arnfjörð Bjarmason:

> However, I'd like to ask if there's any interest in getting a proper & 
> documented patch for this. If so I can submit a proper patch, if not I can 
> just continue to patch my Git build with my hack.

I'd be interested. I also maintain my .gitconfig in a version control system 
(sadly not Git itself, because I don't have it available on all hosts I have 
the files checked out on), and would like to be able to store the 
host-specific settings out of the way (specifically for me that is 
user.email).

-- 
\\// Peter - http://www.softwolves.pp.se/

Re: Is there interest in reading ~/.gitconfig.d/* and /etc/gitconfig.d/*?

[Eli Barzilay]

On Apr  4, Peter Krefting wrote:
> Ævar Arnfjörð Bjarmason:
> 
> > However, I'd like to ask if there's any interest in getting a proper & 
> > documented patch for this. If so I can submit a proper patch, if not I can 
> > just continue to patch my Git build with my hack.
> 
> I'd be interested. I also maintain my .gitconfig in a version control system 
> (sadly not Git itself, because I don't have it available on all hosts I have 
> the files checked out on), and would like to be able to store the 
> host-specific settings out of the way (specifically for me that is 
> user.email).

Isn't it better to have a way to include files instead?

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

Re: Is there interest in reading ~/.gitconfig.d/* and /etc/gitconfig.d/*?

[Ævar Arnfjörð Bjarmason]

On Sun, Apr 4, 2010 at 07:50, Eli Barzilay <eli@barzilay.org> wrote:
> Isn't it better to have a way to include files instead?

Probably yes. Programs like Apache HTTPD, rsyslog and others just use
${foo}conf.d by convention by supporting config inclusion.

What would be the ideal config syntax for that? AFAICT git-config
doesn't (yet) support top-level keys so maybe this:

    [INCLUDE]
        path = ~/.gitconfig.d/*

Or if top-level support was added:

    INCLUDE = ~/.gitconfig.d/*

Re: Is there interest in reading ~/.gitconfig.d/* and /etc/gitconfig.d/*?

[Jakub Narebski]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> On Sun, Apr 4, 2010 at 07:50, Eli Barzilay <eli@barzilay.org> wrote:

> > Isn't it better to have a way to include files instead?
> 
> Probably yes. Programs like Apache HTTPD, rsyslog and others just use
> ${foo}conf.d by convention by supporting config inclusion.
> 
> What would be the ideal config syntax for that? AFAICT git-config
> doesn't (yet) support top-level keys so maybe this:
> 
>     [INCLUDE]
>         path = ~/.gitconfig.d/*
> 
> Or if top-level support was added:
> 
>     INCLUDE = ~/.gitconfig.d/*

<bikeshedding mode on>

I think that it would be better to use

        include ~/.gitconfig.d/*

<bikeshedding mode off>
-- 
Jakub Narebski
Poland
ShadeHawk on #git

[PATCH/RFC] Hacky version of a glob() driven config include

[Ævar Arnfjörð Bjarmason]

This is not ready for inclusion in anything. Commiting for RFC on
whether this way of doing it is sane in theory.

Known bugs:

  * Breaks the model of being able to *set* config values. That
    doesn't work for the included files. Maybe not a bug.

  * Errors in the git_config_from_file() call in glob_include_config()
    aren't passed upwards.

  * It relies on the GNU GLOB_TILDE extension with no
    alternative. That can be done by calling getenv("HOME") and
    s/~/$home/.

  * The whole bit with saving/restoring global state for config
    inclusion is evil, but then again so is the global state.

  * We don't check for recursion. But Git gives up eventually after
    after spewing a *lot* of duplicate entry errors. Not sure how to
    do this sanely w/symlinks.

Not-signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

> On Sun, Apr 4, 2010 at 07:50, Eli Barzilay <eli@barzilay.org> wrote:
> > Isn't it better to have a way to include files instead?
>
> Probably yes. Programs like Apache HTTPD, rsyslog and others just use
> ${foo}conf.d by convention by supporting config inclusion.

Here's an evil implementation of this. I know the code is horrid &
buggy (see above). But is the general idea sane. I thought it would be
better to submit this for comments before I went further with it.

 config.c               |   55 +++++++++++++++++++++++++++++++++++++++++++++++-
 t/t1300-repo-config.sh |   43 +++++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+), 1 deletions(-)

diff --git a/config.c b/config.c
index 6963fbe..e7581b4 100644
--- a/config.c
+++ b/config.c
@@ -7,6 +7,7 @@
  */
 #include "cache.h"
 #include "exec_cmd.h"
+#include <glob.h>
 
 #define MAXNAME (256)
 
@@ -111,6 +112,52 @@ static inline int iskeychar(int c)
 	return isalnum(c) || c == '-';
 }
 
+static void glob_include_config(config_fn_t fn, void *data, char *pattern)
+{
+	glob_t globber;
+	int glob_ret;
+	int conf_ret;
+	size_t i = 0;
+	char *cfile = NULL;
+	FILE *saved_config_file;
+	char *saved_config_file_name;
+	int saved_config_linenr;
+	int saved_config_file_eof;
+#ifdef GLOB_TILDE
+	int glob_flags = GLOB_TILDE;
+#else
+	/* XXX: Non-GNU support for ~ expansion */
+	int glob_flags = 0;
+#endif
+
+	glob_ret = glob(pattern, glob_flags, NULL, &globber);
+
+	if (glob_ret == GLOB_NOSPACE || glob_ret == GLOB_ABORTED) {
+		globfree(&globber);
+		die("Unable to include config with pattern %s", pattern);
+	}
+
+	for (i = 0; i < globber.gl_pathc; i++) {
+		cfile = globber.gl_pathv[i];
+
+		/* Save away global state for including another file */
+		saved_config_file = config_file;
+		saved_config_file_name = config_file_name;
+		saved_config_linenr = config_linenr;
+		saved_config_file_eof = config_file_eof;
+
+		conf_ret = git_config_from_file(fn, cfile, data);
+
+		/* Restore it again */
+		config_file = saved_config_file;
+		config_file_name = saved_config_file_name;
+		config_linenr = saved_config_linenr;
+		config_file_eof = saved_config_file_eof;
+	}
+
+	globfree(&globber);
+}
+
 static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)
 {
 	int c;
@@ -139,7 +186,13 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)
 		if (!value)
 			return -1;
 	}
-	return fn(name, value, data);
+
+	if (!strcmp(name, "voodoo.include")) {
+		glob_include_config(fn, data, value);
+		return 0;
+	} else {
+		return fn(name, value, data);
+	}
 }
 
 static int get_extended_base_var(char *name, int baselen, int c)
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index f11f98c..4df6658 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -824,4 +824,47 @@ test_expect_success 'check split_cmdline return' "
 	test_must_fail git merge master
 	"
 
+cat > .git/config << EOF
+[some]
+	variable = blah
+[voodoo]
+	include = .git/more_config_*
+EOF
+
+cat > .git/more_config_1 << EOF
+[another]
+	variable = blah blah
+EOF
+
+cat > .git/more_config_2 << EOF
+[evenmore]
+	variable = blah bluh 
+EOF
+
+test_expect_success 'The voodoo include variable is hidden from us' \
+    'test_must_fail git config --get voodoo.include'
+test_expect_success 'get some included variable' \
+    'git config --get some.variable'
+test_expect_success 'get another included variable' \
+    'git config --get another.variable'
+test_expect_success 'get evenmore included variable' \
+    'git config --get evenmore.variable'
+
+rm .git/more_config*
+
+cat > .git/config << EOF
+[voodoo]
+	include = .git/more_config_*
+EOF
+
+cat > .git/more_config_1 << EOF
+[foo]
+    bar = zar
+[voodoo]
+	include = .git/more_config_*
+EOF
+
+test_expect_success 'circular config inclusion' \
+    'test_must_fail git config --get foo.bar'
+
 test_done
-- 
1.7.1.dirty

Re: [PATCH/RFC] Hacky version of a glob() driven config include

[Bert Wesarg]

On Thu, May 6, 2010 at 23:14, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> Not-signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

So you don't agree to the Developer's Certificate of Origin, don't you?

Bert

Re: [PATCH/RFC] Hacky version of a glob() driven config include

[Ævar Arnfjörð Bjarmason]

On Fri, May 7, 2010 at 06:00, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> On Thu, May 6, 2010 at 23:14, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>> Not-signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> So you don't agree to the Developer's Certificate of Origin, don't you?

Signed-off-by is for "if you want your work included in git.git"
(according to Documentation/SubmittingPatches). I don't think this
patch is ready for inclusion as-is, but I wanted to solicit comments
on the general approach.

Re: [PATCH/RFC] Hacky version of a glob() driven config include

[Bert Wesarg]

On Fri, May 7, 2010 at 18:56, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Fri, May 7, 2010 at 06:00, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>> On Thu, May 6, 2010 at 23:14, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>>
>>> Not-signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>
>> So you don't agree to the Developer's Certificate of Origin, don't you?
>
> Signed-off-by is for "if you want your work included in git.git"
> (according to Documentation/SubmittingPatches). I don't think this
> patch is ready for inclusion as-is, but I wanted to solicit comments
> on the general approach.
>

Can you please quote SubmittingPatches for your argumentation.

I think you mix technical and legal aspects of submitting patches.

Thank you and please note I'm not a lawyer.

Bert

Re: [PATCH/RFC] Hacky version of a glob() driven config include

[Ævar Arnfjörð Bjarmason]

On Fri, May 7, 2010 at 18:29, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> On Fri, May 7, 2010 at 18:56, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> On Fri, May 7, 2010 at 06:00, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>>> On Thu, May 6, 2010 at 23:14, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>>>
>>>> Not-signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>>
>>> So you don't agree to the Developer's Certificate of Origin, don't you?
>>
>> Signed-off-by is for "if you want your work included in git.git"
>> (according to Documentation/SubmittingPatches). I don't think this
>> patch is ready for inclusion as-is, but I wanted to solicit comments
>> on the general approach.
>>
>
> Can you please quote SubmittingPatches for your argumentation.

I already did, but here's the full paragraph I quoted from, for
reference:

	- if you want your work included in git.git, add a
	  "Signed-off-by: Your Name <you@example.com>" line to the
	  commit message (or just use the option "-s" when
	  committing) to confirm that you agree to the Developer's
	  Certificate of Origin

I'm not seeking to include this work as-is in Git, so I added a
Not-signed-off-by line to make that clear (as if all the bugs didn't
do that already).

I do agree to the Developer's Certificate of Origin, but just read the
"Not-signed-off-by" as "you really don't want to apply this in its
current state". I'm asking for comments so that I can produce an
appliable patch, that one will have a Signed-off-by line.

Re: [PATCH/RFC] Hacky version of a glob() driven config include

[Jacob Helwig]

On Fri, May 7, 2010 at 11:58, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Fri, May 7, 2010 at 18:29, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>> On Fri, May 7, 2010 at 18:56, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>> On Fri, May 7, 2010 at 06:00, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>>>> On Thu, May 6, 2010 at 23:14, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>>>>
>>>>> Not-signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>>>
>>>> So you don't agree to the Developer's Certificate of Origin, don't you?
>>>
>>> Signed-off-by is for "if you want your work included in git.git"
>>> (according to Documentation/SubmittingPatches). I don't think this
>>> patch is ready for inclusion as-is, but I wanted to solicit comments
>>> on the general approach.
>>>
>>
>> Can you please quote SubmittingPatches for your argumentation.
>
> I already did, but here's the full paragraph I quoted from, for
> reference:
>
>        - if you want your work included in git.git, add a
>          "Signed-off-by: Your Name <you@example.com>" line to the
>          commit message (or just use the option "-s" when
>          committing) to confirm that you agree to the Developer's
>          Certificate of Origin
>
> I'm not seeking to include this work as-is in Git, so I added a
> Not-signed-off-by line to make that clear (as if all the bugs didn't
> do that already).
>
> I do agree to the Developer's Certificate of Origin, but just read the
> "Not-signed-off-by" as "you really don't want to apply this in its
> current state". I'm asking for comments so that I can produce an
> appliable patch, that one will have a Signed-off-by line.
>

Having "[RFC]" instead of "[PATCH]" as the subject prefix is typically
considered sufficient to indicate "this isn't actually intended for
inclusion".

Re: [PATCH/RFC] Hacky version of a glob() driven config include

[Bert Wesarg]

On Fri, May 7, 2010 at 20:58, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Fri, May 7, 2010 at 18:29, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>> On Fri, May 7, 2010 at 18:56, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>> On Fri, May 7, 2010 at 06:00, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>>>> On Thu, May 6, 2010 at 23:14, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>>>>
>>>>> Not-signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>>>
>>>> So you don't agree to the Developer's Certificate of Origin, don't you?
>>>
>>> Signed-off-by is for "if you want your work included in git.git"
>>> (according to Documentation/SubmittingPatches). I don't think this
>>> patch is ready for inclusion as-is, but I wanted to solicit comments
>>> on the general approach.
>>>
>>
>> Can you please quote SubmittingPatches for your argumentation.
>
> I already did, but here's the full paragraph I quoted from, for
> reference:
>
>        - if you want your work included in git.git, add a
>          "Signed-off-by: Your Name <you@example.com>" line to the
>          commit message (or just use the option "-s" when
>          committing) to confirm that you agree to the Developer's
>          Certificate of Origin
>

But where does the Developer's Certificate of Origin talks about
non-legal aspects of patch submitting? E.g. correctness, quality, ...

I think the part "if you want your work included in git.git" is very
misleading in this paragraph, and I propose to remove it.

> I'm not seeking to include this work as-is in Git, so I added a
> Not-signed-off-by line to make that clear (as if all the bugs didn't
> do that already).
>
> I do agree to the Developer's Certificate of Origin, but just read the
> "Not-signed-off-by" as "you really don't want to apply this in its
> current state". I'm asking for comments so that I can produce an
> appliable patch, that one will have a Signed-off-by line.
>

And thats exactly where you mixed legal and technical aspects of patch
submitting, and others may not (especially me, obviously). The S-o-b
line has nothing to do with the technical aspect, or the quality, of
the patch. Adding "Not-signed-off-by" (or even worse: changing it
later to Signed-off-by) could actually mean that you stole the code
from someone else.

Bert

[PATCH/RFC v2] Hacky version of a glob() driven config include

[Ævar Arnfjörð Bjarmason]

Add support for a special `voodoo.include` config variable. Its
contents will be globbed, and the files it expands to included as
config files.

This enables support for keeping around a ~/.gitconfig.d/* directory,
or for distros to keep a /etc/gitconfig.d/* directory. Some use cases:

  * Keep secret portions of the Git config file separate,
    e.g. github.token. Useful if the ~/.gitconfig itself is kept in
    a public Git repository.

  * Ability to selectively include config libraries. You could include
    e.g. ~/.gitconfig.d/pretty-colors.ini or
    ~/.gitconfig.d/better-submodule-handling.ini from a remote source.

Known bugs:

  * Breaks the model of being able to *set* config values. That
    doesn't work for the included files, i.e. setting a value that
    might be considered derived from a value in an included file won't
    end up in the "right" place. Maybe not a bug.

  * Errors in the git_config_from_file() call in glob_include_config()
    aren't passed upwards.

  * It relies on the GNU GLOB_TILDE extension with no
    alternative. That can be done by calling getenv("HOME") and
    s/~/$home/.

  * The whole bit with saving/restoring global state for config
    inclusion is evil, but then again so is the global state.

  * We don't check for recursion. But Git gives up eventually after
    after spewing a *lot* of duplicate entry errors. Not sure how to
    do this sanely w/symlinks.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Fri, May 7, 2010 at 19:52, Bert Wesarg <bert.wesarg@googlemail.com> wrote:

> But where does the Developer's Certificate of Origin talks about
> non-legal aspects of patch submitting? E.g. correctness, quality, ...
>
> I think the part "if you want your work included in git.git" is very
> misleading in this paragraph, and I propose to remove it.

Fair enough. My reading of it was literal, i.e. `if (want_inclusion &&
DCO) { -s }`. Since I didn't want inclusion for the patch I didn't add a SOB.

Here's a version v2 of the patch with a Signed-off-by, and a better
commit message.

 config.c               |   55 +++++++++++++++++++++++++++++++++++++++++++++++-
 t/t1300-repo-config.sh |   43 +++++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+), 1 deletions(-)

diff --git a/config.c b/config.c
index 6963fbe..e7581b4 100644
--- a/config.c
+++ b/config.c
@@ -7,6 +7,7 @@
  */
 #include "cache.h"
 #include "exec_cmd.h"
+#include <glob.h>
 
 #define MAXNAME (256)
 
@@ -111,6 +112,52 @@ static inline int iskeychar(int c)
 	return isalnum(c) || c == '-';
 }
 
+static void glob_include_config(config_fn_t fn, void *data, char *pattern)
+{
+	glob_t globber;
+	int glob_ret;
+	int conf_ret;
+	size_t i = 0;
+	char *cfile = NULL;
+	FILE *saved_config_file;
+	char *saved_config_file_name;
+	int saved_config_linenr;
+	int saved_config_file_eof;
+#ifdef GLOB_TILDE
+	int glob_flags = GLOB_TILDE;
+#else
+	/* XXX: Non-GNU support for ~ expansion */
+	int glob_flags = 0;
+#endif
+
+	glob_ret = glob(pattern, glob_flags, NULL, &globber);
+
+	if (glob_ret == GLOB_NOSPACE || glob_ret == GLOB_ABORTED) {
+		globfree(&globber);
+		die("Unable to include config with pattern %s", pattern);
+	}
+
+	for (i = 0; i < globber.gl_pathc; i++) {
+		cfile = globber.gl_pathv[i];
+
+		/* Save away global state for including another file */
+		saved_config_file = config_file;
+		saved_config_file_name = config_file_name;
+		saved_config_linenr = config_linenr;
+		saved_config_file_eof = config_file_eof;
+
+		conf_ret = git_config_from_file(fn, cfile, data);
+
+		/* Restore it again */
+		config_file = saved_config_file;
+		config_file_name = saved_config_file_name;
+		config_linenr = saved_config_linenr;
+		config_file_eof = saved_config_file_eof;
+	}
+
+	globfree(&globber);
+}
+
 static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)
 {
 	int c;
@@ -139,7 +186,13 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)
 		if (!value)
 			return -1;
 	}
-	return fn(name, value, data);
+
+	if (!strcmp(name, "voodoo.include")) {
+		glob_include_config(fn, data, value);
+		return 0;
+	} else {
+		return fn(name, value, data);
+	}
 }
 
 static int get_extended_base_var(char *name, int baselen, int c)
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index f11f98c..4df6658 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -824,4 +824,47 @@ test_expect_success 'check split_cmdline return' "
 	test_must_fail git merge master
 	"
 
+cat > .git/config << EOF
+[some]
+	variable = blah
+[voodoo]
+	include = .git/more_config_*
+EOF
+
+cat > .git/more_config_1 << EOF
+[another]
+	variable = blah blah
+EOF
+
+cat > .git/more_config_2 << EOF
+[evenmore]
+	variable = blah bluh 
+EOF
+
+test_expect_success 'The voodoo include variable is hidden from us' \
+    'test_must_fail git config --get voodoo.include'
+test_expect_success 'get some included variable' \
+    'git config --get some.variable'
+test_expect_success 'get another included variable' \
+    'git config --get another.variable'
+test_expect_success 'get evenmore included variable' \
+    'git config --get evenmore.variable'
+
+rm .git/more_config*
+
+cat > .git/config << EOF
+[voodoo]
+	include = .git/more_config_*
+EOF
+
+cat > .git/more_config_1 << EOF
+[foo]
+    bar = zar
+[voodoo]
+	include = .git/more_config_*
+EOF
+
+test_expect_success 'circular config inclusion' \
+    'test_must_fail git config --get foo.bar'
+
 test_done
-- 
1.7.1.dirty

Re: [PATCH/RFC] Hacky version of a glob() driven config include

[Jakub Narebski]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> This is not ready for inclusion in anything. Commiting for RFC on
> whether this way of doing it is sane in theory.

I think this is a good idea at least in theory.
 
> Known bugs:
> 
>   * Breaks the model of being able to *set* config values. That
>     doesn't work for the included files. Maybe not a bug.

Errr... do I understand correctly that it simply means that you are
not able to set config values that came from included files, in
included files?

This is quite serious limitation.

> 
>   * Errors in the git_config_from_file() call in glob_include_config()
>     aren't passed upwards.

Hmmm...

> 
>   * It relies on the GNU GLOB_TILDE extension with no
>     alternative. That can be done by calling getenv("HOME") and
>     s/~/$home/.

"git config --path <variable>" expands leading '~' to $HOME, and ~user
to home directory of given user.  Why not use this?

> 
>   * The whole bit with saving/restoring global state for config
>     inclusion is evil, but then again so is the global state.

Why not encapsulate those global variables in a struct, passed to
appropriate functions, with a global variable holding an instance of
such struct (IIRC similarly to what is done for "the_index").

> 
>   * We don't check for recursion. But Git gives up eventually after
>     after spewing a *lot* of duplicate entry errors. Not sure how to
>     do this sanely w/symlinks.

The alternates mechanism has some depth limit; why not use it also for
config file inclusion?  The machanism is quite similar...

> 
> Not-signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

You can simply do not add Signed-off-by for an RFC patch...

> ---
> 
> > On Sun, Apr 4, 2010 at 07:50, Eli Barzilay <eli@barzilay.org> wrote:
> > > Isn't it better to have a way to include files instead?
> >
> > Probably yes. Programs like Apache HTTPD, rsyslog and others just use
> > ${foo}conf.d by convention by supporting config inclusion.
> 
> Here's an evil implementation of this. I know the code is horrid &
> buggy (see above). But is the general idea sane. I thought it would be
> better to submit this for comments before I went further with it.
> 
>  config.c               |   55 +++++++++++++++++++++++++++++++++++++++++++++++-
>  t/t1300-repo-config.sh |   43 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 97 insertions(+), 1 deletions(-)

No documentation.
 
[...]
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index f11f98c..4df6658 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -824,4 +824,47 @@ test_expect_success 'check split_cmdline return' "
>  	test_must_fail git merge master
>  	"
>  
> +cat > .git/config << EOF
> +[some]
> +	variable = blah
> +[voodoo]
> +	include = .git/more_config_*
> +EOF

I don't like this syntax.

First, it forces git-config to hide all 'include' keys.  I think there
might be some legitimate <section>.include config variables (perhaps
outside git-core); with this patch they are impossible.


Second, I guess that the section name has absolutely no meaning here.
If included config file has section.key config variable, i.e.:

  [section]
        key = value

the variable in master config file (visible by git-config) would not
be voodoo.section.key.


Third, what happens with the sections in master config file?  If I
have the following in .git/config

  [voodoo]
        var1 = val1
        include = .git/more_config
        var2 = val2

and the .git/more_config has

  [foo]
        bar = baz

would "git config --list" see 'voodoo.var2' (i.e. sections in included
file does not change parsing of master file), or would it see
'foo.var2'?


I would propose

  include .git/more_config_*

if not for the fast that it would trip older git.  Perhaps

  [include ".git/more_config_*"]

or

  [include .git/more_config_*]

or

  ## include ".git/more_config_*"

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH/RFC] Hacky version of a glob() driven config include

[Ævar Arnfjörð Bjarmason]

On Fri, May 7, 2010 at 20:46, Jakub Narebski <jnareb@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> This is not ready for inclusion in anything. Commiting for RFC on
>> whether this way of doing it is sane in theory.
>
> I think this is a good idea at least in theory.

Thanks, and thanks for all your comments below. They were very useful.

>> Known bugs:
>>
>>   * Breaks the model of being able to *set* config values. That
>>     doesn't work for the included files. Maybe not a bug.
>
> Errr... do I understand correctly that it simply means that you are
> not able to set config values that came from included files, in
> included files?
>
> This is quite serious limitation.

It is. And recap, you can now you can set Git's config in either
places .git/config, ~/.gitconfig and $prefix/etc/gitconfig.

With inclusion this is a bit more complex. If my ~/.gitconfig includes
a seekrt.key=foobar via an include in ~/.gitconfig/seekrt, what should
`git config --global seekrt.key newkey` do? How about `git config
--global seekrt.some_new_value blah`?

I think it's best to not try to get into that mess and just let the
user manage included files manually, or with `git config --file`.

>>   * Errors in the git_config_from_file() call in glob_include_config()
>>     aren't passed upwards.
>
> Hmmm...
>
>>
>>   * It relies on the GNU GLOB_TILDE extension with no
>>     alternative. That can be done by calling getenv("HOME") and
>>     s/~/$home/.
>
> "git config --path <variable>" expands leading '~' to $HOME, and ~user
> to home directory of given user.  Why not use this?

I didn't spot it. expand_user_path() does do everything I need,
yippie! I'll use that then.

>>   * The whole bit with saving/restoring global state for config
>>     inclusion is evil, but then again so is the global state.
>
> Why not encapsulate those global variables in a struct, passed to
> appropriate functions, with a global variable holding an instance of
> such struct (IIRC similarly to what is done for "the_index").

That's indeed the sane way to go. I'll do that (and look at
the_index).

>>   * We don't check for recursion. But Git gives up eventually after
>>     after spewing a *lot* of duplicate entry errors. Not sure how to
>>     do this sanely w/symlinks.
>
> The alternates mechanism has some depth limit; why not use it also for
> config file inclusion?  The machanism is quite similar...

Yet another example of prior art in git itself I have to check out,
thanks.

>> Not-signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> You can simply do not add Signed-off-by for an RFC patch...
>
>> ---
>>
>> > On Sun, Apr 4, 2010 at 07:50, Eli Barzilay <eli@barzilay.org> wrote:
>> > > Isn't it better to have a way to include files instead?
>> >
>> > Probably yes. Programs like Apache HTTPD, rsyslog and others just use
>> > ${foo}conf.d by convention by supporting config inclusion.
>>
>> Here's an evil implementation of this. I know the code is horrid &
>> buggy (see above). But is the general idea sane. I thought it would be
>> better to submit this for comments before I went further with it.
>>
>>  config.c               |   55 +++++++++++++++++++++++++++++++++++++++++++++++-
>>  t/t1300-repo-config.sh |   43 +++++++++++++++++++++++++++++++++++++
>>  2 files changed, 97 insertions(+), 1 deletions(-)
>
> No documentation.

A final patch will have that. Since some of the semantics are "munges
your .gitconfig" (a bug). I'll write dosc for it once it's behaving.

> [...]
>> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
>> index f11f98c..4df6658 100755
>> --- a/t/t1300-repo-config.sh
>> +++ b/t/t1300-repo-config.sh
>> @@ -824,4 +824,47 @@ test_expect_success 'check split_cmdline return' "
>>       test_must_fail git merge master
>>       "
>>
>> +cat > .git/config << EOF
>> +[some]
>> +     variable = blah
>> +[voodoo]
>> +     include = .git/more_config_*
>> +EOF
>
> I don't like this syntax.

Me neither.

> First, it forces git-config to hide all 'include' keys.  I think there
> might be some legitimate <section>.include config variables (perhaps
> outside git-core); with this patch they are impossible.

It's only hiding the full 'voodoo.include' key currently, you can
still have e.g. 'bleh.include'.

> Second, I guess that the section name has absolutely no meaning here.
> If included config file has section.key config variable, i.e.:
>
>  [section]
>        key = value
>
> the variable in master config file (visible by git-config) would not
> be voodoo.section.key.

No. All includes are done at the top level. The end result should be
equivalent to having used cat(1) to stitch a bunch of config files
together.

> Third, what happens with the sections in master config file?  If I
> have the following in .git/config
>
>  [voodoo]
>        var1 = val1
>        include = .git/more_config
>        var2 = val2
>
> and the .git/more_config has
>
>  [foo]
>        bar = baz
>
> would "git config --list" see 'voodoo.var2' (i.e. sections in included
> file does not change parsing of master file), or would it see
> 'foo.var2'?

Only voodoo.include should be hidden, nothing else. But let's move on
to..

> I would propose
>
>  include .git/more_config_*
>
> if not for the fast that it would trip older git.  Perhaps

>  ## include ".git/more_config_*"

Probably not a good idea to mix up comments & configuration like
that. Some (semi-broken) parsers of .gitconfig also use INI parsers to
parse it, which breaks on # comments. Those are already broken, but it
would be nice if a feature didn't require them.

>  [include .git/more_config_*]

Syntax error on older Gits.

>  [include ".git/more_config_*"]

I like this one the best. It's also easy to modify the parser (so it
doesn't think it's a section) to handle it. And it doesn't incur the
confusion of looking like a normal configuration variable.

Re: [PATCH/RFC] Hacky version of a glob() driven config include

[Jakub Narebski]

On Sat, 8 May 2010, Ævar Arnfjörð Bjarmason wrote:
> On Fri, May 7, 2010 at 20:46, Jakub Narebski <jnareb@gmail.com> wrote:
> > Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> > > Known bugs:
> > >
> > >   * Breaks the model of being able to *set* config values. That
> > >     doesn't work for the included files. Maybe not a bug.
> >
> > Errr... do I understand correctly that it simply means that you are
> > not able to set config values that came from included files, in
> > included files?
> >
> > This is quite serious limitation.

I was wrong there; this is not even a problem.

> It is. And recap, you can now you can set Git's config in either
> places .git/config, ~/.gitconfig and $prefix/etc/gitconfig.
> 
> With inclusion this is a bit more complex. If my ~/.gitconfig includes
> a seekrt.key=foobar via an include in ~/.gitconfig/seekrt, what should
> `git config --global seekrt.key newkey` do? How about `git config
> --global seekrt.some_new_value blah`?
> 
> I think it's best to not try to get into that mess and just let the
> user manage included files manually, or with `git config --file`.

This is not a problem: while "git config --get foo.bar" would search
through $GIT_DIR/config, ~/.gitconfig and /etc/gitconfig (and with
your addition also included files), "git config foo.bar baz" would set
foo.bar to baz always in per-repository config file (in absence of
--global / --system / --file=<file> option).

So this would be simply an extension of existing situation.  Not a bug.

> > >   * The whole bit with saving/restoring global state for config
> > >     inclusion is evil, but then again so is the global state.
> >
> > Why not encapsulate those global variables in a struct, passed to
> > appropriate functions, with a global variable holding an instance of
> > such struct (IIRC similarly to what is done for "the_index").
> 
> That's indeed the sane way to go. I'll do that (and look at
> the_index).

Note that variable might not be called the_index...

[...]
> > > +cat > .git/config << EOF
> > > +[some]
> > > +     variable = blah
> > > +[voodoo]
> > > +     include = .git/more_config_*
> > > +EOF
> >
> > I don't like this syntax.
> 
> Me neither.
> 
> > First, it forces git-config to hide all 'include' keys.  I think
> > there might be some legitimate <section>.include config variables
> > (perhaps outside git-core); with this patch they are impossible.

Here I didn't notice that it has to be voodoo.include, and not any other
fully qualified variable name, i.e. section name must be voodoo.

> It's only hiding the full 'voodoo.include' key currently, you can
> still have e.g. 'bleh.include'.

voodoo.include shows that black magic voodoo; include.file could be
a bit better.

> > I would propose
> >
> >  include .git/more_config_*
> >
> > if not for the fast that it would trip older git.  Perhaps
> >
> >  ## include ".git/more_config_*"

Or perhaps

  #include ".git/more_config_*"

:-)

> 
> Probably not a good idea to mix up comments & configuration like
> that. Some (semi-broken) parsers of .gitconfig also use INI parsers to
> parse it, which breaks on # comments. Those are already broken, but it
> would be nice if a feature didn't require them.

BTW IIRC ini-files format (at least one of them) allows for ';'-prefixed
line comments (comment must be on its own line); I wonder how it is with 
ini-like git config format.

But in some ini-formats definition we have that both the hash mark (#) 
and the semicolon (;) are comment characters.

> 
> >  [include .git/more_config_*]
> 
> Syntax error on older Gits.
> 
> >  [include ".git/more_config_*"]
> 
> I like this one the best. It's also easy to modify the parser (so it
> doesn't think it's a section) to handle it. And it doesn't incur the
> confusion of looking like a normal configuration variable.

What I don't like with this proposal is that one could write

  [include ".git/more_config_*"]
  	foo = bar

Which is confusing.

But perhaps we can break backwards compatibility here.  I don't know...

  <include .git/more_config_*>
  [[.git/more_config_*]]
  {{.git/more_config_*}}
  [=.git/more_config_*=]
  [@.git/more_config_*]
  %include ".git/more_config_*"
  @INCLUDE = .git/more_config_* 

-- 
Jakub Narebski
Poland

Re: [PATCH/RFC] Hacky version of a glob() driven config include

[Ping Yin]

>
> But perhaps we can break backwards compatibility here.  I don't know...
>
I think we can. Because config file is not in repository, so if your
older git doesn't support it, you should not use this new syntax.

Re: [PATCH/RFC] Hacky version of a glob() driven config include

[Jeff King]

On Thu, May 06, 2010 at 09:14:00PM +0000, Ævar Arnfjörð Bjarmason wrote:

> This is not ready for inclusion in anything. Commiting for RFC on
> whether this way of doing it is sane in theory.

I think the goal of having globbing include files is reasonable.

>   * It relies on the GNU GLOB_TILDE extension with no
>     alternative. That can be done by calling getenv("HOME") and
>     s/~/$home/.

We already have expand_user_path which handles ~ properly (it is used
for the values of some config entries). You could then glob the result.

> +cat > .git/config << EOF
> +[some]
> +	variable = blah
> +[voodoo]
> +	include = .git/more_config_*
> +EOF

My eyes! The goggles do nothing!

That syntax is horrid. Wouldn't it be much simpler to introduce some
top-level syntax for "include this file here", with some very simple
semantics:

  - the included file starts with no section. It should have its own
    section header.

  - after returning from the included file, we are in no section. You
    need a new section header.

Yes, there are some complex tricks those semantics won't allow, but they
are simple to read and understand, simple to use, and simple to
implement.

And really, what is the point in supporting crap like:

   $ cat .gitconfig
   [diff]
   #include foo

   $ cat foo
   rename = true

-Peff

Re: [PATCH/RFC] Hacky version of a glob() driven config include

[Jakub Narebski]

On Sat, 8 May 2010, Ping Yin wrote:
> >
> > But perhaps we can break backwards compatibility here.  I don't know...
> >
> I think we can. Because config file is not in repository, so if your
> older git doesn't support it, you should not use this new syntax.

Actually per-repository $GIT_DIR/config file *is* in repository... but
is not distributed (it is not transferred on clone / fetch).

The problem with breaking backwards compatibility is when repository is
on shared filesystem (be it networked filesystem such as NFS or
CIFS/Samba share, or portable USB (pen)drive), and can be accessed by
different versions of git.


From mentioned backward-incompatibile proposals, there is one that is
already used (at least in some Perl modules in CPAN), namely

  [@foo]

syntax, which is used by Dist::Zilla (where 'foo' is name of "bundle",
which roughly means set of pre-defined configuration variables).  
Although it does not support globbing...

The '@INCLUDE = db_config.ini' is taken from OpenInteract2::Config::Ini.
-- 
Jakub Narebski
Poland

Re: [PATCH/RFC] Hacky version of a glob() driven config include

[Ping Yin]

On Sat, May 8, 2010 at 4:18 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>
> On Sat, 8 May 2010, Ping Yin wrote:
> > >
> > > But perhaps we can break backwards compatibility here.  I don't know...
> > >
> > I think we can. Because config file is not in repository, so if your
> > older git doesn't support it, you should not use this new syntax.
>
> Actually per-repository $GIT_DIR/config file *is* in repository... but
> is not distributed (it is not transferred on clone / fetch).
>
> The problem with breaking backwards compatibility is when repository is
> on shared filesystem (be it networked filesystem such as NFS or
> CIFS/Samba share, or portable USB (pen)drive), and can be accessed by
> different versions of git.

You are right, i missed this case.
However, If NFS is used, the git version is very likely the same. So i
think this case is not worth the effort to keep the backwards
compatibility.

>
> The '@INCLUDE = db_config.ini' is taken from OpenInteract2::Config::Ini.

I prefer "include foo" which is used by apache, and "@include foo"
which doesn't fail to parse when foo doesn't exist.