On Thu, Jan 26, 2012 at 12:58:25PM -0800, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > Include directives are turned on for regular git config
> > parsing (i.e., when you call git_config()), as well as for
> > lookups via the "git config" program. They are not turned on
> > in other cases, including:
> >
> > 1. Parsing of other config-like files, like .gitmodules.
> > There isn't a real need, and I'd rather be conservative
> > and avoid unnecessary incompatibility or confusion.
>
> This is a good design decision, I think, but I am not sure where this "we
> do not let gitmodules to honor inclusion" is implemented in this patch. I
> would have expected a patch to "git-submodule.sh" that adds --no-includes
> where it invokes "git config"?
My goal was to leave git_config_from_file untouched, so that the code in
submodule.c:gitmodules_config would work without modification. And that
works, obviously.
However, I didn't think about the fact that git-submodule.sh would be
calling git-config separately, and that is accidentally changed by my
patch. Even if we changed git-submodule to use "git config
--no-includes" that would break any third-party scripts that use "git
config" to read git-like files.
But it would be nice for callers doing "git config foo.bar" to get the
includes by default. So maybe the right rule is:
1. In C:
a. git_config() respects includes automatically.
b. other callers do not do so automatically (e.g., gitmodules via
submodule.c).
(i.e., what is implemented by this patch)
2. Callers of git-config:
a. respect includes for lookup that checks all of the "normal"
config spots in sequence: .git/config, ~/.gitconfig, and
/etc/gitconfig. These are the shell equivalent of calling
git_config().
b. when we are looking in a specific file (via GIT_CONFIG or "git
config -f"), do not respect includes (but allow --includes if
the caller chooses). This specific file may be something like
.gitmodules. Or perhaps somebody is saying "no, I really just
want to know what is in _this_ file, not what the config
ecosystem tells me in general".
And then because of 1a and 2a, most programs should Just Work without
any changes, but because of 1b and 2b, any special uses will have to
decide manually whether they would want to allow includes.
Does that make sense?
> > +Includes
> > +~~~~~~~~
> > +
> > +You can include one config file from another by setting the special
> > +`include.path` variable to the name of the file to be included. The
> > +included file is expanded immediately, as if its contents had been
> > +found at the location of the include directive.
>
> I cannot offer better wording, but the first reaction I had to this was
> "Eh, if I include a file A that includes another file B, to which config
> file the '[include] path = B' directive found in file A relative to?"
>
> Logically it should be relative to A, and I think your implementation
> makes it so, but the above "as if its contents had been found at..."
> sounds as if it should be the same as writing '[include] path = B' in the
> original config file that included A.
Yes, it is relative to A. Anything else would be insane (because it
would mean "A" has to care about who is including it).
I had a similar thought while writing it, but hoped the sentence after
(that you snipped) would make it clear. But I see the "as if..." can be
ambiguous (because it's only mostly "as if"). I started trying to write
something like "with the exception of further includes, which..." but I
think that is just getting more confusing.
How about:
The included file is processed immediately, before any other
directives from the surrounding file.
What I wanted to make clear there is the ordering, which sometimes
matters.
> By the way, I was a bit surprised that you did not have to add the source
> stacking to git_config_from_file().
>
> It was added in 924aaf3 (config.c: Make git_config() work correctly when
> called recursively, 2011-06-16) to address the situation where
> git_config() ends up calling git_config() recursively.
Yeah. I remembered discussing that patch with Ramsay a few months ago,
and I was pleased that it just worked in this case.
> My gut feeling is that logically that issue shouldn't be limited to
> configuration that is coming from file, which in turn makes me suspect
> that the source stacking may be better in one level higher than
> git_config_from_file() so that the ones coming from parameters could
> also be treated as a different kind of source to read configuration
> from.
I do factor it out in the second patch. I also considered how
git_config_from_parameters doesn't share the same source marking. But it
follows a completely different code-path, as its syntax is totally
different. So there's really not much point in adding to the config_file
stack (if you had another source that could be included from a file, you
would want to mention it in the stack. But the command-line parameters
must always be the "root" of the stack, so it's sufficient to simply
have a NULL stack to mark that).
> But in practice, including from the command line (i.e. -c include.path=...)
> is somewhat crazy and we probably would not want to support it, so...
Actually, I made sure it works, and it's in the tests. It even complains
if you use a relative path (I thought about using $PWD, but that is too
insane, as git changes the working directory behind the scenes).
The one use I think is to bundle a bunch of related config options, and
then turn them on selectively. So you _could_ do:
cat >>~/.bashrc <<-\EOF
FOO_OPTIONS="-c foo.one=1 -c foo.two=2"
EOF
# without FOO
git blah
# with FOO
git $FOO_OPTIONS blah
but with this patch, you can do:
cat >~/.gitconfig.foo <<-\EOF
[foo]
one = 1
two = 2
EOF
git -c include.path=$HOME/.gitconfig.foo blah
which to my mind is a little more natural. I don't personally plan on
using it, but it was easy to implement (in fact, I'd have to write to
disallow it, since the include magic is in the callback handler).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html