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

Previous thread: [PATCH] Prompt for a username when an HTTP request 401s by Scott Chacon on Thursday, April 1, 2010 - 1:29 pm. (3 messages)

Next thread: [PATCH] Prompt for a username when an HTTP request 401s by Scott Chacon on Thursday, April 1, 2010 - 3:14 pm. (4 messages)
From: =?UTF-8?B?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?=
Date: Thursday, April 1, 2010 - 2:20 pm

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.
--

From: Heiko Voigt
Date: Thursday, April 1, 2010 - 3:03 pm

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
--

From: Peter Krefting
Date: Sunday, April 4, 2010 - 12:24 am

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/
--

From: Eli Barzilay
Date: Sunday, April 4, 2010 - 12:59 am

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!
--

From: =?UTF-8?B?w4Z2YXIgQXJuZmrDtnLDsCBCamFybWFzb24=?=
Date: Tuesday, April 6, 2010 - 1:15 am

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/*
--

From: Jakub Narebski
Date: Tuesday, April 6, 2010 - 2:02 am

<bikeshedding mode on>

I think that it would be better to use

        include ~/.gitconfig.d/*

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

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
Date: Thursday, May 6, 2010 - 2:14 pm

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>

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 = ...
From: Bert Wesarg
Date: Thursday, May 6, 2010 - 11:00 pm

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

Bert
--

From: Ævar Arnfjörð Bjarmason
Date: Friday, May 7, 2010 - 9:56 am

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.
--

From: Bert Wesarg
Date: Friday, May 7, 2010 - 11:29 am

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
--

From: Ævar Arnfjörð Bjarmason
Date: Friday, May 7, 2010 - 11:58 am

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.
--

From: Jacob Helwig
Date: Friday, May 7, 2010 - 12:02 pm

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

From: Bert Wesarg
Date: Friday, May 7, 2010 - 12:52 pm

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

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
--

From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
Date: Friday, May 7, 2010 - 1:11 pm

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>
---


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 ...
From: Jakub Narebski
Date: Friday, May 7, 2010 - 1:46 pm

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?



"git config --path <variable>" expands leading '~' to $HOME, and ~user

Why not encapsulate those global variables in a struct, passed to
appropriate functions, with a global variable holding an instance of

The alternates mechanism has some depth limit; why not use it also for


No documentation.
 

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
--

From: Ævar Arnfjörð Bjarmason
Date: Friday, May 7, 2010 - 3:15 pm

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

I didn't spot it. expand_user_path() does do everything I need,

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

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

A final patch will have that. Since some of the semantics are "munges


It's only hiding the full 'voodoo.include' key currently, you can

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

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

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


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.
--

From: Jakub Narebski
Date: Friday, May 7, 2010 - 4:43 pm

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).


Note that variable might not be called the_index...


Here I didn't notice that it has to be voodoo.include, and not any other

voodoo.include shows that black magic voodoo; include.file could be

Or perhaps


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 (#) 

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
--

From: Ping Yin
Date: Friday, May 7, 2010 - 7:30 pm

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.
--

From: Jakub Narebski
Date: Saturday, May 8, 2010 - 1:18 am

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
--

From: Ping Yin
Date: Saturday, May 8, 2010 - 2:03 am

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

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

From: Jeff King
Date: Friday, May 7, 2010 - 10:06 pm

We already have expand_user_path which handles ~ properly (it is used

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
--

Previous thread: [PATCH] Prompt for a username when an HTTP request 401s by Scott Chacon on Thursday, April 1, 2010 - 1:29 pm. (3 messages)

Next thread: [PATCH] Prompt for a username when an HTTP request 401s by Scott Chacon on Thursday, April 1, 2010 - 3:14 pm. (4 messages)