From: Jonathan Tan <hidden> Date: 2021-10-12 22:57:32
Previously [1], I sent a patch set for remote-suggested configs that are
transmitted when fetching, but there were some security concerns. Here
is another way that remote repo administators can provide recommended
configs - through conditionally included files based on the configured
remote. Git itself neither transmits nor prompts for these files, which
hopefully reduces people's concerns.
More information is in the commit message of patch 2.
[1] https://lore.kernel.org/git/cover.1623881977.git.jonathantanmy@google.com/
Jonathan Tan (2):
config: make git_config_include() static
config: include file if remote URL matches a glob
config.c | 80 ++++++++++++++++++++++++++++++++++++++++++-----
config.h | 37 +++-------------------
t/t1300-config.sh | 27 ++++++++++++++++
3 files changed, 103 insertions(+), 41 deletions(-)
--
2.33.0.882.g93a45727a2-goog
From: Jonathan Tan <hidden> Date: 2021-10-12 22:57:38
It is not used from outside the file in which it is declared.
Signed-off-by: Jonathan Tan <redacted>
---
config.c | 12 +++++++++++-
config.h | 37 ++++---------------------------------
2 files changed, 15 insertions(+), 34 deletions(-)
From: Jonathan Tan <hidden> Date: 2021-10-12 22:57:39
This is a feature that supports config file inclusion conditional on
whether the repo has a remote with a URL that matches a glob.
Similar to my previous work on remote-suggested hooks [1], the main
motivation is to allow remote repo administrators to provide recommended
configs in a way that can be consumed more easily (e.g. through a
package installable by a package manager). But this can also be used by,
say, an individual that wants certain configs to apply to a certain set
of local repos but not others.
I marked this as RFC because there are some design points that need to
be resolved:
- The existing "include" and "includeIf" instructions are executed
immediately, whereas in order to be useful, the execution of
"includeIf hasremoteurl" needs to be delayed until all config files
are read. Are there better ways to do this?
- Is the conditionally-included file allowed to have its own
"include{,If}" instructions? I'm thinking that we should forbid it
because, for example, if we had 4 files as follows: A includes B and
C includes D, and we include A and C in our main config (in that
order), it wouldn't be clear whether B (because A was first included)
or C (because we should execute everything at the same depth first)
should be executed first. (In this patch, I didn't do anything about
includes.)
- A small one: the exact format of the glob. I probably will treat the
URL like a path.
[1] https://lore.kernel.org/git/cover.1623881977.git.jonathantanmy@google.com/
Signed-off-by: Jonathan Tan <redacted>
---
config.c | 70 +++++++++++++++++++++++++++++++++++++++++------
t/t1300-config.sh | 27 ++++++++++++++++++
2 files changed, 89 insertions(+), 8 deletions(-)
From: Jeff King <hidden> Date: 2021-10-12 23:30:09
On Tue, Oct 12, 2021 at 03:57:23PM -0700, Jonathan Tan wrote:
This is a feature that supports config file inclusion conditional on
whether the repo has a remote with a URL that matches a glob.
Similar to my previous work on remote-suggested hooks [1], the main
motivation is to allow remote repo administrators to provide recommended
configs in a way that can be consumed more easily (e.g. through a
package installable by a package manager). But this can also be used by,
say, an individual that wants certain configs to apply to a certain set
of local repos but not others.
OK. I was a little wary after reading the subject that this would be
"when we are using such a URL", which is full of all kinds of odd corner
cases. But if it is "a remote is defined with a matching URL" that makes
it a property of the repository, not the operation.
I think in general this kind of feature is currently served by just
correlating filesystem paths with their function. So with your patch I
could do:
[includeIf "hasremoteurl:https://myjob.example.com"]
path = foo.conf
But in general, I'd imagine most people put their repository in ~/work
or similar, and just do:
[includeIf "gitdir:~/work"]
path = foo.conf
(and of course you can imagine more subdivisions as necessary). So I
find the use-case only sort-of compelling. In general, I'm in favor of
adding new includeIf directions even if they're only moderately
convenient. But this one is rather sticky, because it is dependent on
other config keys being defined. So it introduces a new and complicated
ordering issue. Is it worth it? Maybe I'm not being imaginative enough
in seeing the use cases.
I marked this as RFC because there are some design points that need to
be resolved:
- The existing "include" and "includeIf" instructions are executed
immediately, whereas in order to be useful, the execution of
"includeIf hasremoteurl" needs to be delayed until all config files
are read. Are there better ways to do this?
Note that this violates the "as if they had been found at the location
of the include directive" rule which we advertise to users. I'd imagine
that most of the time it doesn't matter, but this is a pretty big
exception we'll need to document.
Just brainstorming some alternatives:
- We could stop the world while we are parsing and do a _new_ parse
that just looks at the remote config (in fact, this is the natural
thing if you were consulting the regular remote.c code for the list
of remotes, because it does its own config parse).
That does mean that the remote-conditional includes cannot
themselves define new remotes. But I think that is already the case
with your patch (and violating that gets you into weird circular
problems).
- We could simply document that if you want to depend on conditional
includes based on a particular remote.*.url existing, then that
remote config must appear earlier in the sequence.
This is a bit ugly, because I'm sure it will bite somebody
eventually. But at the same time, it resolves all of the weird
timing issues, and does so in a way that will be easy to match if we
have any other config dependencies.
- Is the conditionally-included file allowed to have its own
"include{,If}" instructions? I'm thinking that we should forbid it
because, for example, if we had 4 files as follows: A includes B and
C includes D, and we include A and C in our main config (in that
order), it wouldn't be clear whether B (because A was first included)
or C (because we should execute everything at the same depth first)
should be executed first. (In this patch, I didn't do anything about
includes.)
I'd say that A would expand B at the moment it is parsed, by the usual
as-if rule. If it has a recursive includeIf on remotes, then my head may
explode. I'd argue that we should refuse to do recursive remote-ifs in
that case (though all of this is a consequence of the after-the-fact
parsing; I'd much prefer one of the alternatives I gave earlier).
- A small one: the exact format of the glob. I probably will treat the
URL like a path.
You might want to use the matcher from urlmatch.[ch], which understands
things like wildcards. Of course remote "URLs" are not always real
syntactically valid URLs, which may make that awkward.
Barring that the usual fnmatch glob is probably our best bet.
So we make a copy of every remote name on the off chance that somebody
has an includeIf which looks at it. That feels wasteful, though in
practice it's probably not that big a deal.
By doing the config parsing ourselves here we're missing out on any
other forms of remote, like .git/remotes. Those are old and not widely
used, and I'd be OK with skipping them. But we should clearly document
that this is matching remote.*.url, not any of the other mechanisms.
[...]
I only lightly read the rest of the patch. I didn't see anything
obviously wrong, but I think the goal at this point is figuring out the
design.
-Peff
From: brian m. carlson <hidden> Date: 2021-10-13 00:47:27
On 2021-10-12 at 22:57:21, Jonathan Tan wrote:
Previously [1], I sent a patch set for remote-suggested configs that are
transmitted when fetching, but there were some security concerns. Here
is another way that remote repo administators can provide recommended
configs - through conditionally included files based on the configured
remote. Git itself neither transmits nor prompts for these files, which
hopefully reduces people's concerns.
More information is in the commit message of patch 2.
I won't go into the details of the patches, since I'm a little low on
time at the moment, but I think from what I've seen of the cover letter
and the commit messages, this approach is much better from a security
perspective and, provided we can get the kinks mentioned downthread
ironed out, I'd be happy to see this merged.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
It is not used from outside the file in which it is declared.
Signed-off-by: Jonathan Tan <redacted>
---
config.c | 12 +++++++++++-
config.h | 37 ++++---------------------------------
2 files changed, 15 insertions(+), 34 deletions(-)
...and avoid the forward declaration?
I've seen that in a few places, making the diff smaller here doesn't
seem worth it v.s. maintaining the definition in two places.
From: Jonathan Tan <hidden> Date: 2021-10-13 18:18:29
I won't go into the details of the patches, since I'm a little low on
time at the moment, but I think from what I've seen of the cover letter
and the commit messages, this approach is much better from a security
perspective and, provided we can get the kinks mentioned downthread
ironed out, I'd be happy to see this merged.
Thanks - I really appreciate this note. Thanks also for all your
thoughts up to now about the security perspective.
From: Jonathan Tan <hidden> Date: 2021-10-13 18:33:59
OK. I was a little wary after reading the subject that this would be
"when we are using such a URL", which is full of all kinds of odd corner
cases. But if it is "a remote is defined with a matching URL" that makes
it a property of the repository, not the operation.
I think in general this kind of feature is currently served by just
correlating filesystem paths with their function. So with your patch I
could do:
[includeIf "hasremoteurl:https://myjob.example.com"]
path = foo.conf
But in general, I'd imagine most people put their repository in ~/work
or similar, and just do:
[includeIf "gitdir:~/work"]
path = foo.conf
(and of course you can imagine more subdivisions as necessary). So I
find the use-case only sort-of compelling. In general, I'm in favor of
adding new includeIf directions even if they're only moderately
convenient. But this one is rather sticky, because it is dependent on
other config keys being defined. So it introduces a new and complicated
ordering issue. Is it worth it? Maybe I'm not being imaginative enough
in seeing the use cases.
My main use case is for a remote repo administrator to offer a
recommended config to anyone who clones that repo. For this, I don't
think we can prescribe a local directory structure (e.g. "~/work")
without being too restrictive or broad (that is, if the user ends up
creating a repo that so happens to match our glob but did not intend the
config to apply to it).
I did bring up the idea that an individual could use this to have config
in one place that affects a subset of remotes, but you're right that
they could just do this by putting repositories at different places in
the filesystem.
quoted
I marked this as RFC because there are some design points that need to
be resolved:
- The existing "include" and "includeIf" instructions are executed
immediately, whereas in order to be useful, the execution of
"includeIf hasremoteurl" needs to be delayed until all config files
are read. Are there better ways to do this?
Note that this violates the "as if they had been found at the location
of the include directive" rule which we advertise to users. I'd imagine
that most of the time it doesn't matter, but this is a pretty big
exception we'll need to document.
Yes, that's true. Another thing I just thought of is to add a new
"deferIncludeIf" which makes clear the different semantics (deferred
include, and perhaps not allow recursive includes).
Just brainstorming some alternatives:
- We could stop the world while we are parsing and do a _new_ parse
that just looks at the remote config (in fact, this is the natural
thing if you were consulting the regular remote.c code for the list
of remotes, because it does its own config parse).
That does mean that the remote-conditional includes cannot
themselves define new remotes. But I think that is already the case
with your patch (and violating that gets you into weird circular
problems).
Hmm...yes, having a special-case rule that such an included file cannot
define new remotes would be complex.
- We could simply document that if you want to depend on conditional
includes based on a particular remote.*.url existing, then that
remote config must appear earlier in the sequence.
This is a bit ugly, because I'm sure it will bite somebody
eventually. But at the same time, it resolves all of the weird
timing issues, and does so in a way that will be easy to match if we
have any other config dependencies.
My main issue with this is that different config files are read at
different times, and the repo config (that usually contains the remote)
is read last.
quoted
- Is the conditionally-included file allowed to have its own
"include{,If}" instructions? I'm thinking that we should forbid it
because, for example, if we had 4 files as follows: A includes B and
C includes D, and we include A and C in our main config (in that
order), it wouldn't be clear whether B (because A was first included)
or C (because we should execute everything at the same depth first)
should be executed first. (In this patch, I didn't do anything about
includes.)
I'd say that A would expand B at the moment it is parsed, by the usual
as-if rule. If it has a recursive includeIf on remotes, then my head may
explode. I'd argue that we should refuse to do recursive remote-ifs in
that case (though all of this is a consequence of the after-the-fact
parsing; I'd much prefer one of the alternatives I gave earlier).
If we can't expand in place, I would say that any recursive includes
should be refused. But as you said, we could still think about whether
in-place expansion can be done before addressing this question.
quoted
- A small one: the exact format of the glob. I probably will treat the
URL like a path.
You might want to use the matcher from urlmatch.[ch], which understands
things like wildcards. Of course remote "URLs" are not always real
syntactically valid URLs, which may make that awkward.
Barring that the usual fnmatch glob is probably our best bet.
OK.
So we make a copy of every remote name on the off chance that somebody
has an includeIf which looks at it. That feels wasteful, though in
practice it's probably not that big a deal.
By doing the config parsing ourselves here we're missing out on any
other forms of remote, like .git/remotes. Those are old and not widely
used, and I'd be OK with skipping them. But we should clearly document
that this is matching remote.*.url, not any of the other mechanisms.
Sounds good.
I only lightly read the rest of the patch. I didn't see anything
obviously wrong, but I think the goal at this point is figuring out the
design.
From: Jonathan Tan <hidden> Date: 2021-10-18 20:48:12
After some in-office discussion, here are the alternatives as I see it:
(1) Introduce a "includeAfterIf" (or "deferIncludeIf", or some other
name) command that is executed after all config files are read. (If
there are multiple, they are executed in order of appearance.)
Files included by this mechanism cannot directly or indirectly
contain another "includeAfterIf". This is the same as what was
introduced in this patch set, except for the name of the directive.
(2) Leave the name as "includeIf", and when it is encountered with a
remote-URL condition: continue parsing the config files, skipping
all "includeIf hasRemoteUrl", only looking for remote.*.url. After
that, resume the reading of config files at the first "includeIf
hasRemoteUrl", using the prior remote.*.url information gathered to
determine which files to include when "includeIf hasRemoteUrl" is
encountered. Files included by this mechanism cannot contain any
"remote.*.url" variables.
In all cases, the include is executed if at least one remote URL
matches.
There are other ideas including:
(3) remote.*.url must appear before a "includeIf hasRemoteUrl" that
wants to match it. (But this doesn't fit our use case, in which a
repo config has the URL but a system or user config has the
include.)
(4) "includeIf hasRemoteUrl" triggers a search of the repo config just
for remote.*.url. (I think this out-of-order config search is more
complicated than (2), though.)
For (2), I think that prohibiting "remote.*.url" from any "includeIf
hasRemoteUrl" files sidesteps questions like "what happens when an
included file overrides the URL that made us include this file in the
first place" or "what happens if an included file includes a
remote.*.url that validates or invalidates a prior or subsequent file",
because now that cannot happen at all. My main concern with this
prohibition was that if we were to introduce another similar condition
(say, one based on remote names), what would happen? But I think this is
solvable - make the prohibitions based only on all the conditions that
the actually used, so if the user only uses conditions on remote URLs,
then the user can still set refspecs (for example), even after the
remote-name-condition feature is introduced in Git.
For (1), it is simpler in concept (and also in implementation, I think).
The user just needs to know that certain includes are on-the-spot and
certain includes (the ones with "after" in the name) are deferred - in
particular, if a config variable isn't the value they expect, they'll
need to check that it wasn't introduced in an includeAfterIf file. (And
the user also needs to figure out that if they want to override such a
variable, they'll need to make their own includeAfterIf with an
always-true condition.)
From the prior replies, I think that people will be more interested in
(2) as it preserves the "last config wins" rule, and I'm inclined to go
for (2) too. I'll see if others have any other opinions, and if not I'll
see how the implementation of (2) will look like.
On Mon, Oct 18, 2021 at 01:48:03PM -0700, Jonathan Tan wrote:
After some in-office discussion, here are the alternatives as I see it:
(1) Introduce a "includeAfterIf" (or "deferIncludeIf", or some other
name) command that is executed after all config files are read. (If
there are multiple, they are executed in order of appearance.)
Files included by this mechanism cannot directly or indirectly
contain another "includeAfterIf". This is the same as what was
introduced in this patch set, except for the name of the directive.
(2) Leave the name as "includeIf", and when it is encountered with a
remote-URL condition: continue parsing the config files, skipping
all "includeIf hasRemoteUrl", only looking for remote.*.url. After
that, resume the reading of config files at the first "includeIf
hasRemoteUrl", using the prior remote.*.url information gathered to
determine which files to include when "includeIf hasRemoteUrl" is
encountered. Files included by this mechanism cannot contain any
"remote.*.url" variables.
In all cases, the include is executed if at least one remote URL
matches.
There are other ideas including:
(3) remote.*.url must appear before a "includeIf hasRemoteUrl" that
wants to match it. (But this doesn't fit our use case, in which a
repo config has the URL but a system or user config has the
include.)
(4) "includeIf hasRemoteUrl" triggers a search of the repo config just
for remote.*.url. (I think this out-of-order config search is more
complicated than (2), though.)
For (2), I think that prohibiting "remote.*.url" from any "includeIf
hasRemoteUrl" files sidesteps questions like "what happens when an
included file overrides the URL that made us include this file in the
first place" or "what happens if an included file includes a
remote.*.url that validates or invalidates a prior or subsequent file",
because now that cannot happen at all. My main concern with this
prohibition was that if we were to introduce another similar condition
(say, one based on remote names), what would happen? But I think this is
solvable - make the prohibitions based only on all the conditions that
the actually used, so if the user only uses conditions on remote URLs,
then the user can still set refspecs (for example), even after the
remote-name-condition feature is introduced in Git.
For (1), it is simpler in concept (and also in implementation, I think).
The user just needs to know that certain includes are on-the-spot and
certain includes (the ones with "after" in the name) are deferred - in
particular, if a config variable isn't the value they expect, they'll
need to check that it wasn't introduced in an includeAfterIf file. (And
the user also needs to figure out that if they want to override such a
variable, they'll need to make their own includeAfterIf with an
always-true condition.)
From the prior replies, I think that people will be more interested in
(2) as it preserves the "last config wins" rule, and I'm inclined to go
for (2) too. I'll see if others have any other opinions, and if not I'll
see how the implementation of (2) will look like.
Another concern which came up for me in a private conversation today -
How difficult will it be for users to override this include directive if
it is set somewhere outside of their control? For example:
/etc/gitconfig:
[includeIf hasRemoteUrl.https://example.com/example.git] // or whatever
path = /etc/some-special-config
Will it be possible for a user to "un-include" /etc/some-special-config
themselves?
I don't think this should change your patch much - if my understanding
is correct, we also don't have a way to "un-include" existing include or
includeIf directives made outside of the user's control. But I wonder if
it'd be useful to think about some way to do that. Maybe we can teach
the config parse how to include a config file in reverse? Maybe we need
a "neverInclude" directive? Food for thought, anyway.
Sorry, but I won't have time to take a look at the rest of this series
til next week.
- Emily
On Tue, Oct 12 2021, Jonathan Tan wrote:
I tried sending the below (sans some last minute spellchecking now)
around October 19th, but for some reason it didn't make it
on-list. Trying again now, apologies for [near-]duplicates, if any (I
elaborated a bit at the end just now).
Previously [1], I sent a patch set for remote-suggested configs that are
transmitted when fetching, but there were some security concerns. Here
is another way that remote repo administators can provide recommended
configs - through conditionally included files based on the configured
remote. Git itself neither transmits nor prompts for these files, which
hopefully reduces people's concerns.
I had some concerns about the specifics of the implementation/what
seemed to be tailoring it a bit too closely to one use-case[1][2], not
inherently with the idea (although I think e.g. for brian that more
closely reflects his thoughts).
Anyway, just saying that aside from this RFC I don't think we were at
the point of really fleshing out what this would look like, and there
being some hard "no", so I think that idea could still be pursued.
On this proposal: this also applies globally to all history, but I don't
have the same concern with that as the 1=1 mapping of remote-suggested
hooks, our path includes work that way, after all.
I think it would be nice if you could think about if/how this and the
"onbranch" include would work together though to serve the general case
better.
Also if you have a repo with N remotes each where "origin" tracks URLs
at git.example.com, and you add a "dev" tracking dev.example.com, will
the config apply if you're say on a branch tracking the "live" server,
if you've said "include this for repos matching dev.example.com?
Arguably that's what you want, but perhaps something that those more
used to the centralized workflows wouldn't consider as being unintuitive
for users who might want to add this config only for their main "origin"
remote. We don't really have a way of marking that special-ness though,
except maybe checkout.defaultRemote.
I'm also still somewhat mystified at how this would better serve your
userbase than the path-based included, i.e. the selling point of the
remote-suggested configuration was that it would Just Work.
But for this the users would either need to setup the config themselves
for your remote, but that would be easier than pro-actively cloning in
"work" or whatever? I guess, just wondering if I'm missing something.
Or if it's a partly-automated system where some automation is dropping
in a /etc/gitconfig.d/google-remote-config-include I wonder if this
whole thing wouldn't be better for users with such special-needs if we
just supported an "early config hook".
i.e. similar to how we read trace2 config from /etc/gitconfig early, we
could start picking up a hook that just so happens to conform to the
config schema Emily's config-based hooks use.
So the /etc/gitconfig would have say:
hook.ourConfigThingy.command=/usr/bin/googly-git-config
hook.ourConfigThingy.event=include-config
That hook would just produce a config snippet to be included on STDOUT.
Since it's an arbitrary external command it would nicely get around any
chicken and egg problems in git itself, it could run "git remote -v",
inspect the equivalent of an "onbranch" etc. etc, then just dynamically
produce config-to-be-included.
Please don't take this as some objection to your current proposal, just
a thought on something that might entirely bypass odd edge cases and
arbitrary limitations associated with doing this all in the "main"
process on-the-fly.
The special-ness with that one would need to be that we'd say it
wouldn't have the normal "last set wins" semantics, or maybe we could do
that and just note that we saw it, and execute the "include" when we
detect the end of the full config parsing (I'm not familiar enough with
those bits to say where that is).
Both of those seem easier than dealing with any chicken & egg problems
in parsing the config stream itself, since such a hook could just invoke
"git remote -v" and the like itself, after e.g. setting some environment
variable of its own to guard against its own recursion (or we'd do it
for it for such hooks...).
1. https://lore.kernel.org/git/87k0mn2dd3.fsf@evledraar.gmail.com/
2. https://lore.kernel.org/git/87o8awvglr.fsf@evledraar.gmail.com/
From: Jonathan Tan <hidden> Date: 2021-10-25 18:54:02
I had some concerns about the specifics of the implementation/what
seemed to be tailoring it a bit too closely to one use-case[1][2], not
inherently with the idea (although I think e.g. for brian that more
closely reflects his thoughts).
Anyway, just saying that aside from this RFC I don't think we were at
the point of really fleshing out what this would look like, and there
being some hard "no", so I think that idea could still be pursued.
Which idea specifically do you think could still be pursued?
On this proposal: this also applies globally to all history, but I don't
have the same concern with that as the 1=1 mapping of remote-suggested
hooks, our path includes work that way, after all.
I think it would be nice if you could think about if/how this and the
"onbranch" include would work together though to serve the general case
better.
Also if you have a repo with N remotes each where "origin" tracks URLs
at git.example.com, and you add a "dev" tracking dev.example.com, will
the config apply if you're say on a branch tracking the "live" server,
if you've said "include this for repos matching dev.example.com?
Right now, the feature is only dependent on remote URLs configured
through remote.?.url. It wouldn't work with "onbranch" because there's
no way to combine conditions (and I have no plans to do that). I think
that if you have something that you want depending on which branch
you're on, you can just use the existing "onbranch" feature.
Arguably that's what you want, but perhaps something that those more
used to the centralized workflows wouldn't consider as being unintuitive
for users who might want to add this config only for their main "origin"
remote. We don't really have a way of marking that special-ness though,
except maybe checkout.defaultRemote.
What do you mean by adding a config for a specific remote?
I'm also still somewhat mystified at how this would better serve your
userbase than the path-based included, i.e. the selling point of the
remote-suggested configuration was that it would Just Work.
But for this the users would either need to setup the config themselves
for your remote, but that would be easier than pro-actively cloning in
"work" or whatever? I guess, just wondering if I'm missing something.
Or if it's a partly-automated system where some automation is dropping
in a /etc/gitconfig.d/google-remote-config-include
Yes, the config is meant to be handled e.g. through a package manager
like apt. We don't want to prescribe directory structures like "work",
which is why the include is conditional upon the remote URL.
Even if the user pro-actively clones into "work", the user still needs
to set up the conditional config, so I don't see how that is a net
benefit.
I wonder if this
whole thing wouldn't be better for users with such special-needs if we
just supported an "early config hook".
i.e. similar to how we read trace2 config from /etc/gitconfig early, we
could start picking up a hook that just so happens to conform to the
config schema Emily's config-based hooks use.
So the /etc/gitconfig would have say:
hook.ourConfigThingy.command=/usr/bin/googly-git-config
hook.ourConfigThingy.event=include-config
That hook would just produce a config snippet to be included on STDOUT.
Since it's an arbitrary external command it would nicely get around any
chicken and egg problems in git itself, it could run "git remote -v",
inspect the equivalent of an "onbranch" etc. etc, then just dynamically
produce config-to-be-included.
I see that later on, you suggest an environment variable to guard
against recursion.
One thing is that if there are multiple such hooks, each one won't be
able to see what the other hooks have produced.
If the feature you described already existed in Git, I think I could use
that, but if we're deciding between implementing the config hook you
describe versus something with more constraints, I think the one I
proposed is better for now. Some design points that have already been
discussed are whether setting a config during processing of an included
file would then invalidate the include and also the order of operations,
both of which would be much more difficult to control with config hooks.
Please don't take this as some objection to your current proposal, just
a thought on something that might entirely bypass odd edge cases and
arbitrary limitations associated with doing this all in the "main"
process on-the-fly.
The special-ness with that one would need to be that we'd say it
wouldn't have the normal "last set wins" semantics, or maybe we could do
that and just note that we saw it, and execute the "include" when we
detect the end of the full config parsing (I'm not familiar enough with
those bits to say where that is).
The "last set" would be those set by the hooks, so yes, a user would
need to know to make their own hook in order to override anything set by
the hooks. The end of the full config parsing is in
config_with_options().
I had some concerns about the specifics of the implementation/what
seemed to be tailoring it a bit too closely to one use-case[1][2], not
inherently with the idea (although I think e.g. for brian that more
closely reflects his thoughts).
Anyway, just saying that aside from this RFC I don't think we were at
the point of really fleshing out what this would look like, and there
being some hard "no", so I think that idea could still be pursued.
Which idea specifically do you think could still be pursued?
I meant the whole in-repo .gitconfig. I.e. to the extent that you're
submitting this as an alternative to that because of the negative
feedback on that RFC.
quoted
On this proposal: this also applies globally to all history, but I don't
have the same concern with that as the 1=1 mapping of remote-suggested
hooks, our path includes work that way, after all.
I think it would be nice if you could think about if/how this and the
"onbranch" include would work together though to serve the general case
better.
Also if you have a repo with N remotes each where "origin" tracks URLs
at git.example.com, and you add a "dev" tracking dev.example.com, will
the config apply if you're say on a branch tracking the "live" server,
if you've said "include this for repos matching dev.example.com?
Right now, the feature is only dependent on remote URLs configured
through remote.?.url. It wouldn't work with "onbranch" because there's
no way to combine conditions (and I have no plans to do that). I think
that if you have something that you want depending on which branch
you're on, you can just use the existing "onbranch" feature.
I mean with this and the below...
quoted
Arguably that's what you want, but perhaps something that those more
used to the centralized workflows wouldn't consider as being unintuitive
for users who might want to add this config only for their main "origin"
remote. We don't really have a way of marking that special-ness though,
except maybe checkout.defaultRemote.
What do you mean by adding a config for a specific remote?
...what happens if you add a google.com remote for a repository that
"lives" on github.com. I.e. are the semantics "match any remote", or
"match the 'primary' remote (origin?" etc.
quoted
I'm also still somewhat mystified at how this would better serve your
userbase than the path-based included, i.e. the selling point of the
remote-suggested configuration was that it would Just Work.
But for this the users would either need to setup the config themselves
for your remote, but that would be easier than pro-actively cloning in
"work" or whatever? I guess, just wondering if I'm missing something.
Or if it's a partly-automated system where some automation is dropping
in a /etc/gitconfig.d/google-remote-config-include
Yes, the config is meant to be handled e.g. through a package manager
like apt. We don't want to prescribe directory structures like "work",
which is why the include is conditional upon the remote URL.
Even if the user pro-actively clones into "work", the user still needs
to set up the conditional config, so I don't see how that is a net
benefit.
Ah, that explains it. I assumed both cases would be ones where the user
would need to manually enable the 'configuration' (or cloning to a given
subdir).
quoted
I wonder if this
whole thing wouldn't be better for users with such special-needs if we
just supported an "early config hook".
i.e. similar to how we read trace2 config from /etc/gitconfig early, we
could start picking up a hook that just so happens to conform to the
config schema Emily's config-based hooks use.
So the /etc/gitconfig would have say:
hook.ourConfigThingy.command=/usr/bin/googly-git-config
hook.ourConfigThingy.event=include-config
That hook would just produce a config snippet to be included on STDOUT.
Since it's an arbitrary external command it would nicely get around any
chicken and egg problems in git itself, it could run "git remote -v",
inspect the equivalent of an "onbranch" etc. etc, then just dynamically
produce config-to-be-included.
I see that later on, you suggest an environment variable to guard
against recursion.
One thing is that if there are multiple such hooks, each one won't be
able to see what the other hooks have produced.
Yes, although aside from this hook that's a general caveat with the
proposed config-based hooks, I think if you need a hook that does that
(whether it's this, or pre-receive etc.) our answer is "put it in your
own wrapper".
If the feature you described already existed in Git, I think I could use
that, but if we're deciding between implementing the config hook you
describe versus something with more constraints, I think the one I
proposed is better for now. Some design points that have already been
discussed are whether setting a config during processing of an included
file would then invalidate the include and also the order of operations,
both of which would be much more difficult to control with config hooks.
I suggested it because maybe it would be a lot simpler, i.e. we don't
need such a feature to be aware of remote config at all, or having to
"read forward" to find it, maybe it would be more complex. I haven't
tried to implement it.
quoted
Please don't take this as some objection to your current proposal, just
a thought on something that might entirely bypass odd edge cases and
arbitrary limitations associated with doing this all in the "main"
process on-the-fly.
The special-ness with that one would need to be that we'd say it
wouldn't have the normal "last set wins" semantics, or maybe we could do
that and just note that we saw it, and execute the "include" when we
detect the end of the full config parsing (I'm not familiar enough with
those bits to say where that is).
The "last set" would be those set by the hooks, so yes, a user would
need to know to make their own hook in order to override anything set by
the hooks. The end of the full config parsing is in
config_with_options().
On the "user would need to know" that's the same if it's config? I.e. in
either case it would be in /etc/gitconfig or whatever shipped by the
*.deb package.
Anyway, I really just meant this as a suggestion, and one that might
make things simpler. If you don't think it makes sense...
From: Jeff King <hidden> Date: 2021-10-27 11:40:14
On Wed, Oct 13, 2021 at 11:33:41AM -0700, Jonathan Tan wrote:
quoted
But in general, I'd imagine most people put their repository in ~/work
or similar, and just do:
[includeIf "gitdir:~/work"]
path = foo.conf
(and of course you can imagine more subdivisions as necessary). So I
find the use-case only sort-of compelling. In general, I'm in favor of
adding new includeIf directions even if they're only moderately
convenient. But this one is rather sticky, because it is dependent on
other config keys being defined. So it introduces a new and complicated
ordering issue. Is it worth it? Maybe I'm not being imaginative enough
in seeing the use cases.
My main use case is for a remote repo administrator to offer a
recommended config to anyone who clones that repo. For this, I don't
think we can prescribe a local directory structure (e.g. "~/work")
without being too restrictive or broad (that is, if the user ends up
creating a repo that so happens to match our glob but did not intend the
config to apply to it).
Yeah, I agree that it's not quite as turnkey if you have to assume
something about the user's directory structure. On the other hand, they
have to decide to put the included config file somewhere, too, so it
seems like you need to give the user "do something like this"
instructions rather than purely something they can copy and paste.
I dunno. I guess you can assume they'll put it in ~/.gitconfig-foo or
similar, and come up with copy-and-pastable directions from that.
I agree that the "match the remote" rule makes things _more_ convenient.
Mostly I was just wondering if it changed things enough to merit the
complications it introduces. I'm not sure I have an answer, and clearly
it's pretty subjective.
quoted
Just brainstorming some alternatives:
- We could stop the world while we are parsing and do a _new_ parse
that just looks at the remote config (in fact, this is the natural
thing if you were consulting the regular remote.c code for the list
of remotes, because it does its own config parse).
That does mean that the remote-conditional includes cannot
themselves define new remotes. But I think that is already the case
with your patch (and violating that gets you into weird circular
problems).
Hmm...yes, having a special-case rule that such an included file cannot
define new remotes would be complex.
I think that's mostly true of your "defer" system, too, unless you keep
applying it recursively. The rule is slightly different there: it's not
"you can't define new remotes", but rather "you can't do a
remote-conditional include based on a remote included by
remote-conditional".
quoted
- We could simply document that if you want to depend on conditional
includes based on a particular remote.*.url existing, then that
remote config must appear earlier in the sequence.
This is a bit ugly, because I'm sure it will bite somebody
eventually. But at the same time, it resolves all of the weird
timing issues, and does so in a way that will be easy to match if we
have any other config dependencies.
My main issue with this is that different config files are read at
different times, and the repo config (that usually contains the remote)
is read last.
Ah, right. I was thinking of the definitions within a single file, but
you're right that the common case would be having the include in
~/.gitconfig, and the remotes defined in $GIT_DIR/config. So yeah, any
ordering constraint like that is a non-starter, I'd think.
-Peff
From: Jeff King <hidden> Date: 2021-10-27 11:56:09
On Mon, Oct 18, 2021 at 01:48:03PM -0700, Jonathan Tan wrote:
(1) Introduce a "includeAfterIf" (or "deferIncludeIf", or some other
name) command that is executed after all config files are read. (If
there are multiple, they are executed in order of appearance.)
Files included by this mechanism cannot directly or indirectly
contain another "includeAfterIf". This is the same as what was
introduced in this patch set, except for the name of the directive.
I think this works in terms of having self-consistent rules that make
sense. But deferring things does introduce new complications in terms of
overrides, because we rely on last-one-wins. Emily asked elsewhere about
overriding the inclusion of a file. We don't have a way to do that now,
and I think it would be tricky to add. But what about overriding a
single variable?
Right now this works:
git config --global foo.bar true
git config --local foo.bar false
to give you "false". But imagining there was a world of deferred config,
then:
git config --file ~/.gitconfig-foo foo.bar true
git config --global deferInclude.path .gitconfig-foo
git config --local foo.bar false
gives "true". We'd read .gitconfig-foo after everything else, overriding
the repo-level config.
If the deferred includes were processed at the end of each individual
file, that would solve that. You're still left with the slight oddness
that a deferred include may override options within the same file that
come after it, but that's inherent to the "defer" concept, and the
answer is probably "don't do that". It's only when it crosses file
boundaries (which are explicitly ordered by priority) that it really
hurts.
(2) Leave the name as "includeIf", and when it is encountered with a
remote-URL condition: continue parsing the config files, skipping
all "includeIf hasRemoteUrl", only looking for remote.*.url. After
that, resume the reading of config files at the first "includeIf
hasRemoteUrl", using the prior remote.*.url information gathered to
determine which files to include when "includeIf hasRemoteUrl" is
encountered. Files included by this mechanism cannot contain any
"remote.*.url" variables.
I think doing this as "continue parsing" and "resume" is hard to do.
Because you can't look at other non-remote.*.url entries here (otherwise
you'd see them out of order). So you have to either:
- complete the parse, stashing all the other variables away, and then
resolve the include, and then look at all the stashed variables as
if you were parsing them anew.
- teach our config parser how to save and restore state, including
both intra-file state and the progress across the set of files
I think it's much easier if you think of it as "start a new config parse
that does not respect hasRemoteURL". And the easiest way to do that is
to just let remote.c's existing git_config() start that parse (probably
by calling git_config_with_options() and telling it "don't respect
hasRemoteURL includes"). You may also need to teach the config parser to
be reentrant. We did some work on that a while ago, pushing the state
int config_source which functions as a stack, but I don't offhand know
if you can call git_config() from within a config callback.
There are other ideas including:
(3) remote.*.url must appear before a "includeIf hasRemoteUrl" that
wants to match it. (But this doesn't fit our use case, in which a
repo config has the URL but a system or user config has the
include.)
Yeah, I agree this won't work.
(4) "includeIf hasRemoteUrl" triggers a search of the repo config just
for remote.*.url. (I think this out-of-order config search is more
complicated than (2), though.)
I think this is what I described above, and actually is less
complicated. ;)
-Peff
From: Jonathan Tan <hidden> Date: 2021-10-27 17:24:05
Yeah, I agree that it's not quite as turnkey if you have to assume
something about the user's directory structure. On the other hand, they
have to decide to put the included config file somewhere, too, so it
seems like you need to give the user "do something like this"
instructions rather than purely something they can copy and paste.
They can copy and paste instructions to add a package repository (e.g.
by editing /etc/apt/sources.list) and then install a package.
I dunno. I guess you can assume they'll put it in ~/.gitconfig-foo or
similar, and come up with copy-and-pastable directions from that.
I agree that the "match the remote" rule makes things _more_ convenient.
Mostly I was just wondering if it changed things enough to merit the
complications it introduces. I'm not sure I have an answer, and clearly
it's pretty subjective.
I am almost done with the implementation, so maybe the community could
look at it and concretely see the extent of the complication.
quoted
quoted
Just brainstorming some alternatives:
- We could stop the world while we are parsing and do a _new_ parse
that just looks at the remote config (in fact, this is the natural
thing if you were consulting the regular remote.c code for the list
of remotes, because it does its own config parse).
That does mean that the remote-conditional includes cannot
themselves define new remotes. But I think that is already the case
with your patch (and violating that gets you into weird circular
problems).
Hmm...yes, having a special-case rule that such an included file cannot
define new remotes would be complex.
I think that's mostly true of your "defer" system, too, unless you keep
applying it recursively. The rule is slightly different there: it's not
"you can't define new remotes", but rather "you can't do a
remote-conditional include based on a remote included by
remote-conditional".
I was thinking that deferred includes cannot themselves have other
deferred includes and that which deferred includes are included would be
computed only once, and those would be the only rules. (But I guess this
is moot now - we're not doing this approach.)
quoted
quoted
- We could simply document that if you want to depend on conditional
includes based on a particular remote.*.url existing, then that
remote config must appear earlier in the sequence.
This is a bit ugly, because I'm sure it will bite somebody
eventually. But at the same time, it resolves all of the weird
timing issues, and does so in a way that will be easy to match if we
have any other config dependencies.
My main issue with this is that different config files are read at
different times, and the repo config (that usually contains the remote)
is read last.
Ah, right. I was thinking of the definitions within a single file, but
you're right that the common case would be having the include in
~/.gitconfig, and the remotes defined in $GIT_DIR/config. So yeah, any
ordering constraint like that is a non-starter, I'd think.
-Peff
Yeah. Thanks for continuing to take a look at this.
From: Jonathan Tan <hidden> Date: 2021-10-27 17:53:04
On Mon, Oct 18, 2021 at 01:48:03PM -0700, Jonathan Tan wrote:
quoted
(1) Introduce a "includeAfterIf" (or "deferIncludeIf", or some other
name) command that is executed after all config files are read. (If
there are multiple, they are executed in order of appearance.)
Files included by this mechanism cannot directly or indirectly
contain another "includeAfterIf". This is the same as what was
introduced in this patch set, except for the name of the directive.
I think this works in terms of having self-consistent rules that make
sense. But deferring things does introduce new complications in terms of
overrides, because we rely on last-one-wins. Emily asked elsewhere about
overriding the inclusion of a file. We don't have a way to do that now,
and I think it would be tricky to add. But what about overriding a
single variable?
Right now this works:
git config --global foo.bar true
git config --local foo.bar false
to give you "false". But imagining there was a world of deferred config,
then:
git config --file ~/.gitconfig-foo foo.bar true
git config --global deferInclude.path .gitconfig-foo
git config --local foo.bar false
gives "true". We'd read .gitconfig-foo after everything else, overriding
the repo-level config.
If the deferred includes were processed at the end of each individual
file, that would solve that. You're still left with the slight oddness
that a deferred include may override options within the same file that
come after it, but that's inherent to the "defer" concept, and the
answer is probably "don't do that". It's only when it crosses file
boundaries (which are explicitly ordered by priority) that it really
hurts.
This would indeed solve the issue of the user needing to know the trick
to override variables set by deferred includes. But this wouldn't solve
our primary use case in which a system-level config defines a
conditional include but the repo config defines the URL, I think.
quoted
(2) Leave the name as "includeIf", and when it is encountered with a
remote-URL condition: continue parsing the config files, skipping
all "includeIf hasRemoteUrl", only looking for remote.*.url. After
that, resume the reading of config files at the first "includeIf
hasRemoteUrl", using the prior remote.*.url information gathered to
determine which files to include when "includeIf hasRemoteUrl" is
encountered. Files included by this mechanism cannot contain any
"remote.*.url" variables.
I think doing this as "continue parsing" and "resume" is hard to do.
Because you can't look at other non-remote.*.url entries here (otherwise
you'd see them out of order). So you have to either:
- complete the parse, stashing all the other variables away, and then
resolve the include, and then look at all the stashed variables as
if you were parsing them anew.
- teach our config parser how to save and restore state, including
both intra-file state and the progress across the set of files
I am implementing something similar to your first approach (stashing
things). It's almost done so hopefully we'll have something concrete to
discuss soon.
I think it's much easier if you think of it as "start a new config parse
that does not respect hasRemoteURL". And the easiest way to do that is
to just let remote.c's existing git_config() start that parse (probably
by calling git_config_with_options() and telling it "don't respect
hasRemoteURL includes"). You may also need to teach the config parser to
be reentrant. We did some work on that a while ago, pushing the state
int config_source which functions as a stack, but I don't offhand know
if you can call git_config() from within a config callback.
Besides the reentrancy (which may be difficult, as there are some global
variables, but from a glance, some code seems to take care to save and
restore them, so it may already be reentrant or not too difficult to
make reentrant), we would have to bubble down the config (struct
git_config_source and struct config_options) into all the places that
could potentially start the parse and also have a place to store the
URLs we get. If we're already going to stash URLs, it may be easier to
stash the variables instead.
quoted
There are other ideas including:
(3) remote.*.url must appear before a "includeIf hasRemoteUrl" that
wants to match it. (But this doesn't fit our use case, in which a
repo config has the URL but a system or user config has the
include.)
Yeah, I agree this won't work.
quoted
(4) "includeIf hasRemoteUrl" triggers a search of the repo config just
for remote.*.url. (I think this out-of-order config search is more
complicated than (2), though.)
I think this is what I described above, and actually is less
complicated. ;)
-Peff
From: Jeff King <hidden> Date: 2021-10-27 20:32:58
On Wed, Oct 27, 2021 at 10:52:59AM -0700, Jonathan Tan wrote:
quoted
If the deferred includes were processed at the end of each individual
file, that would solve that. You're still left with the slight oddness
that a deferred include may override options within the same file that
come after it, but that's inherent to the "defer" concept, and the
answer is probably "don't do that". It's only when it crosses file
boundaries (which are explicitly ordered by priority) that it really
hurts.
This would indeed solve the issue of the user needing to know the trick
to override variables set by deferred includes. But this wouldn't solve
our primary use case in which a system-level config defines a
conditional include but the repo config defines the URL, I think.
Doh, of course. I forgot that was the whole point of the defer. ;)
I am implementing something similar to your first approach (stashing
things). It's almost done so hopefully we'll have something concrete to
discuss soon.
From: Jonathan Tan <hidden> Date: 2021-10-29 17:31:16
Here's a version that implements the URL lookahead. Compared to an
approach in which we rerun config in order to just get the remote URLs,
I think that the main benefit of this is that we don't read from disk
twice. One inelegant thing is that we now have essentially two caches of
config variables - the one in struct repository (struct config_set
*config) and the one newly introduced by this patch set. (Although I
think that many commands don't use the cache in struct repository,
instead just reading the config in one pass.)
(Of course, there is also the possibility that we should have the remote
repo administrator provide the config in another way.)
I have marked this WIP. This patch set is mostly done, except for the
following:
- Prohibiting remote.?.url from any files included directly or
indirectly by a URL-conditional include.
- Checking that memory everywhere is freed when no longer needed.
- Documentation (as mentioned in the NEEDSWORK comment in patch 2).
- Tests that check what the glob matches and doesn't match.
No range-diff included because this version is substantially different.
Jonathan Tan (2):
config: make git_config_include() static
config: include file if remote URL matches a glob
config.c | 142 ++++++++++++++++++++++++++++++++++++++++++----
config.h | 37 ++----------
t/t1300-config.sh | 60 ++++++++++++++++++++
3 files changed, 194 insertions(+), 45 deletions(-)
--
2.33.1.1089.g2158813163f-goog
From: Jonathan Tan <hidden> Date: 2021-10-29 17:31:18
It is not used from outside the file in which it is declared.
Signed-off-by: Jonathan Tan <redacted>
---
config.c | 12 +++++++++++-
config.h | 37 ++++---------------------------------
2 files changed, 15 insertions(+), 34 deletions(-)
From: Jonathan Tan <hidden> Date: 2021-10-29 17:31:25
This is a feature that supports config file inclusion conditional on
whether the repo has a remote with a URL that matches a glob.
Similar to my previous work on remote-suggested hooks [1], the main
motivation is to allow remote repo administrators to provide recommended
configs in a way that can be consumed more easily (e.g. through a
package installable by a package manager).
NEEDSWORK: The way this works is that if we see such an include, we
shunt all subsequent configs into a stash (while looking for URLs), then
process the stash. In particular, this means that more memory is needed,
and the nature of error reporting changes (currently, if a callback
returns nonzero for a variable, processing halts immediately, but with
this patch, all the config might be read from disk before the callback
even sees the variable). I'll need to expand on this and write a
documentation section.
One alternative is to rerun the config parsing mechanism upon noticing
the first URL-conditional include in order to find all URLs. This would
require the config files to be read from disk twice, though.
[1] https://lore.kernel.org/git/cover.1623881977.git.jonathantanmy@google.com/
Signed-off-by: Jonathan Tan <redacted>
---
config.c | 132 +++++++++++++++++++++++++++++++++++++++++-----
t/t1300-config.sh | 60 +++++++++++++++++++++
2 files changed, 180 insertions(+), 12 deletions(-)
On Fri, Oct 29, 2021 at 10:31:09AM -0700, Jonathan Tan wrote:
quoted hunk
It is not used from outside the file in which it is declared.
Signed-off-by: Jonathan Tan <redacted>
---
config.c | 12 +++++++++++-
config.h | 37 ++++---------------------------------
2 files changed, 15 insertions(+), 34 deletions(-)
I wondered why we even had this here, if we were only calling
'git_config_include()' from config.c. The last time this definition was
touched was when it was moved out of cache.h (e67a57fc518) and the time
before that was when it was introduced in 2012 (9b25a0b52e0). At the
time of its introduction it was only called in config.c, anyways. So I
guess it is just a matter of history.
It's still a WIP so I won't leave a reviewed-by line, but this patch
looks fine.
- Emily
-
/*
* Match and parse a config key of the form:
*
--
2.33.1.1089.g2158813163f-goog
On Fri, Oct 29, 2021 at 10:31:10AM -0700, Jonathan Tan wrote:
This is a feature that supports config file inclusion conditional on
whether the repo has a remote with a URL that matches a glob.
Similar to my previous work on remote-suggested hooks [1], the main
motivation is to allow remote repo administrators to provide recommended
configs in a way that can be consumed more easily (e.g. through a
package installable by a package manager).
To expand a little more on this:
At Google we ship /etc/gitconfig, as well as /usr/share/git-core/. Our
/etc/gitconfig looks basically like:
[include]
path = /usr/share/git-core/gitconfig
path = /usr/share/git-core/some-specific-config
path = /usr/share/git-core/other-specific-config
Jonathan's WIP allows us to append lines to /etc/gitconfig sort of like
[includeIf "hasRemoteUrl:https://internal-google/big-project"]
path = /usr/share/big-project/gitconfig
That's approach #1 to shipping a config, which we might use for a
project that makes up a significant portion of our userbase. We ship
(and own) the /etc/gitconfig; BigProject team ships and owns their own
gitconfig; everybody internally who works on BigProject, whether it's
just once to fix a small thing or every day as their main job, gets the
relevant configs for BigProject.
Approach #2 I think is also still a useful one, and maybe more
interesting outside of Google:
When I run 'sudo apt install big-oss-project-devkit', a few things
happen:
1. /usr/share/big-oss-project/gitconfig appears
2. `git config --global \
'includeIf.hasRemoteUrl:https://github/big-oss-project/*' \
'/usr/share/big-oss-project/gitconfig'` is run
3. whatever other special tools, scripts, etc. are installed
That way regardless of which project I'm working on -
big-oss-project/translation, big-oss-project/docs,
big-oss-project/big-oss-project - I still get configs and style checkers
and whatever else.
With this approach #2, it's still possible for someone to do a drive-by
contribution without ever running 'apt install big-oss-project-devkit',
so it's not quite as strong a recommendation as the former
"remote-suggested-hooks" topic. User would still want to take a look at
the README for big-oss-project to learn they're supposed to be
installing that package ahead of time. But it's still a oneshot setup
for nice things like partial clone filters, maybe sparsity filters,
maybe config-based hooks, etc., especially if big-oss-project already
was shipping some project-specific tooling (like maybe a special
debugger or a docker image or I don't know).
The nice thing about 'hasRemoteUrl' in this case is that we don't need
to know the location of the user's big-oss-project/ checkout on disk. We
can set that config globally and they can checkout big-oss-project as
many times and as many places as they wish. It wouldn't be possible to
ship configs via a package manager or other automated script without it.
NEEDSWORK: The way this works is that if we see such an include, we
shunt all subsequent configs into a stash (while looking for URLs), then
process the stash. In particular, this means that more memory is needed,
and the nature of error reporting changes (currently, if a callback
returns nonzero for a variable, processing halts immediately, but with
this patch, all the config might be read from disk before the callback
even sees the variable). I'll need to expand on this and write a
documentation section.
Hm. I'm not so sure about making another structure for storing config
into memory, because we already do that during the regular config parse
(to make things like git_config_get_string() fast). Can we not re-walk
the in-memory config at the end of the normal parse, rather than reading
from disk twice?
I think git_config()/repo_config() callback even does that for you for free...?
2304 void repo_config(struct repository *repo, config_fn_t fn, void
*data)
2305 {
2306 git_config_check_init(repo);
2307 configset_iter(repo->config, fn, data);
2308 }
One alternative is to rerun the config parsing mechanism upon noticing
the first URL-conditional include in order to find all URLs. This would
require the config files to be read from disk twice, though.
What's the easiest way to "try it and see", to add tooling and find out
whether the config files would be reopened during the second parse?
Because I suspect that we won't actually reopen those files, due to the
config cache.
So couldn't we do something like....
pass #1:
if (include)
if (not hasRemoteUrl)
open up path & parse
put config into in-memory cache normally
pass #2: (and this pass would need to be added to repo_config() probably)
if (include)
if (hasRemoteUrl)
open up path & parse
insert in-order into in-memory cache
don't touch existing configs otherwise
I think it's in practice similar to the approach you're using (getting
around the weird ordering with a cache in memory), but we could reuse
the existing config cache rather than creating a new and different one.
- Emily
On Fri, Oct 29, 2021 at 10:31:10AM -0700, Jonathan Tan wrote:
[...]
quoted
One alternative is to rerun the config parsing mechanism upon noticing
the first URL-conditional include in order to find all URLs. This would
require the config files to be read from disk twice, though.
What's the easiest way to "try it and see", to add tooling and find out
whether the config files would be reopened during the second parse?
Because I suspect that we won't actually reopen those files, due to the
config cache.
strace -f?
So couldn't we do something like....
pass #1:
if (include)
if (not hasRemoteUrl)
open up path & parse
put config into in-memory cache normally
pass #2: (and this pass would need to be added to repo_config() probably)
if (include)
if (hasRemoteUrl)
open up path & parse
insert in-order into in-memory cache
don't touch existing configs otherwise
I think it's in practice similar to the approach you're using (getting
around the weird ordering with a cache in memory), but we could reuse
the existing config cache rather than creating a new and different one.
I don't know enough to say if this two-step approach is better (although
I'm slightly biased in that direction, since it seems simpler), but this
just seems like premature optimization.
I.e. let's just read the files twice, they'll be in the OS's FS cache,
which is unlikely to be a bottleneck for the amount of files involved.
That being said we do have exactly this cache already. See [1] and
3c8687a73ee (add `config_set` API for caching config-like files,
2014-07-28).
But I think that was added due to *very* frequent re-parsing of the
entire config every time someone needed a config variable, not due to
the I/O overhead (but I may be wrong).
So if we've got 100 config variables we need and 10 config files then
10*100 is probably starting to hurt, but if for whatever reason we
needed 2*10 here that's probably no big deal, and in any case would only
happen if this new include mechanism was in play.
1. https://lore.kernel.org/git/1404631162-18556-1-git-send-email-tanayabh@gmail.com/
From: Jonathan Tan <hidden> Date: 2021-11-09 00:22:59
To expand a little more on this:
[snip]
The nice thing about 'hasRemoteUrl' in this case is that we don't need
to know the location of the user's big-oss-project/ checkout on disk. We
can set that config globally and they can checkout big-oss-project as
many times and as many places as they wish. It wouldn't be possible to
ship configs via a package manager or other automated script without it.
Ah, thanks for the elaboration!
quoted
NEEDSWORK: The way this works is that if we see such an include, we
shunt all subsequent configs into a stash (while looking for URLs), then
process the stash. In particular, this means that more memory is needed,
and the nature of error reporting changes (currently, if a callback
returns nonzero for a variable, processing halts immediately, but with
this patch, all the config might be read from disk before the callback
even sees the variable). I'll need to expand on this and write a
documentation section.
Hm. I'm not so sure about making another structure for storing config
into memory, because we already do that during the regular config parse
(to make things like git_config_get_string() fast). Can we not re-walk
the in-memory config at the end of the normal parse, rather than reading
from disk twice?
I think git_config()/repo_config() callback even does that for you for free...?
The main thing is that we wouldn't know if an entry would have been
overridden by a value from an includeif.hasremoteurl or not.
What's the easiest way to "try it and see", to add tooling and find out
whether the config files would be reopened during the second parse?
Because I suspect that we won't actually reopen those files, due to the
config cache.
So couldn't we do something like....
pass #1:
if (include)
if (not hasRemoteUrl)
open up path & parse
put config into in-memory cache normally
pass #2: (and this pass would need to be added to repo_config() probably)
if (include)
if (hasRemoteUrl)
open up path & parse
insert in-order into in-memory cache
don't touch existing configs otherwise
I think it's in practice similar to the approach you're using (getting
around the weird ordering with a cache in memory), but we could reuse
the existing config cache rather than creating a new and different one.
What do you mean by "insert in-order"? If you mean figuring out which
variables would be overridden (and for multi-valued variables, what
order to put all the values in), I think that's the hard part.
Another thing is that at the point where we read the config
(config_with_options()), we have a callback, so we would need to make
sure that we're writing to the in-memory cache in the first place (as
opposed to passing a callback that does something else). That might be
doable by changing the API, but in ay case, I'll try the recursive
config parse first.
From: Jonathan Tan <hidden> Date: 2021-11-09 00:25:40
quoted
What's the easiest way to "try it and see", to add tooling and find out
whether the config files would be reopened during the second parse?
Because I suspect that we won't actually reopen those files, due to the
config cache.
strace -f?
Thanks - this might work.
quoted
So couldn't we do something like....
pass #1:
if (include)
if (not hasRemoteUrl)
open up path & parse
put config into in-memory cache normally
pass #2: (and this pass would need to be added to repo_config() probably)
if (include)
if (hasRemoteUrl)
open up path & parse
insert in-order into in-memory cache
don't touch existing configs otherwise
I think it's in practice similar to the approach you're using (getting
around the weird ordering with a cache in memory), but we could reuse
the existing config cache rather than creating a new and different one.
I don't know enough to say if this two-step approach is better (although
I'm slightly biased in that direction, since it seems simpler), but this
just seems like premature optimization.
I.e. let's just read the files twice, they'll be in the OS's FS cache,
which is unlikely to be a bottleneck for the amount of files involved.
OK - let me try this.
That being said we do have exactly this cache already. See [1] and
3c8687a73ee (add `config_set` API for caching config-like files,
2014-07-28).
But I think that was added due to *very* frequent re-parsing of the
entire config every time someone needed a config variable, not due to
the I/O overhead (but I may be wrong).
So if we've got 100 config variables we need and 10 config files then
10*100 is probably starting to hurt, but if for whatever reason we
needed 2*10 here that's probably no big deal, and in any case would only
happen if this new include mechanism was in play.
1. https://lore.kernel.org/git/1404631162-18556-1-git-send-email-tanayabh@gmail.com/
From: Jonathan Tan <hidden> Date: 2021-11-16 03:41:38
Here's a version that starts a second traversal of the config file read
when encountering the first remote URL. No user-visible changes from v2,
but hopefully the algorithm is simpler.
I've also added the user-facing documentation and a test of what the
glob pattern should match.
Some people have suggested avoiding the forward declaration in patch 1,
but I found that there are 2 functions that call each other, so the
forward declaration cannot be avoided.
Jonathan Tan (2):
config: make git_config_include() static
config: include file if remote URL matches a glob
Documentation/config.txt | 11 ++++
config.c | 133 ++++++++++++++++++++++++++++++++++++---
config.h | 44 ++++---------
t/t1300-config.sh | 100 +++++++++++++++++++++++++++++
4 files changed, 246 insertions(+), 42 deletions(-)
--
2.34.0.rc1.387.gb447b232ab-goog
From: Jonathan Tan <hidden> Date: 2021-11-16 03:41:42
It is not used from outside the file in which it is declared.
Signed-off-by: Jonathan Tan <redacted>
---
config.c | 12 +++++++++++-
config.h | 37 ++++---------------------------------
2 files changed, 15 insertions(+), 34 deletions(-)
From: Jonathan Tan <hidden> Date: 2021-11-16 03:41:44
This is a feature that supports config file inclusion conditional on
whether the repo has a remote with a URL that matches a glob.
Similar to my previous work on remote-suggested hooks [1], the main
motivation is to allow remote repo administrators to provide recommended
configs in a way that can be consumed more easily (e.g. through a
package installable by a package manager - it could, for example,
contain a file to be included conditionally and a post-install script
that adds the include directive to the system-wide config file).
In order to do this, Git reruns the config parsing mechanism upon
noticing the first URL-conditional include in order to find all remote
URLs, and these remote URLs are then used to determine if that first and
all subsequent includes are executed. Remote URLs are not allowed to be
configued in any URL-conditionally-included file.
[1] https://lore.kernel.org/git/cover.1623881977.git.jonathantanmy@google.com/
Signed-off-by: Jonathan Tan <redacted>
---
Documentation/config.txt | 11 ++++
config.c | 121 ++++++++++++++++++++++++++++++++++++---
config.h | 7 +++
t/t1300-config.sh | 100 ++++++++++++++++++++++++++++++++
4 files changed, 231 insertions(+), 8 deletions(-)
@@ -159,6 +159,17 @@ all branches that begin with `foo/`. This is useful if your branches are organized hierarchically and you would like to apply a configuration to all the branches in that hierarchy.+`hasremoteurl`::+ The data that follows the keyword `hasremoteurl:` is taken to+ be a pattern with standard globbing wildcards and two+ additional ones, `**/` and `/**`, that can match multiple+ components. The rest of the config files will be scanned for+ remote URLs, and then if there at least one remote URL that+ matches this pattern, the include condition is met.+++Files included by this option (directly or indirectly) are not allowed+to contain remote URLs.+ A few more notes on matching via `gitdir` and `gitdir/i`: * Symlinks in `$GIT_DIR` are not resolved before matching.
@@ -316,12 +322,83 @@ static int include_condition_is_true(const struct config_options *opts,return0;}+staticintadd_remote_url(constchar*var,constchar*value,void*data)+{+structstring_list*remote_urls=data;+constchar*remote_name;+size_tremote_name_len;+constchar*key;++if(!parse_config_key(var,"remote",&remote_name,&remote_name_len,+&key)&&+remote_name&&+!strcmp(key,"url"))+string_list_append(remote_urls,value);+return0;+}++staticvoidpopulate_remote_urls(structconfig_include_data*inc)+{+structconfig_optionsopts;++structconfig_source*store_cf=cf;+structkey_value_info*store_kvi=current_config_kvi;+enumconfig_scopestore_scope=current_parsing_scope;++opts=*inc->opts;+opts.unconditional_remote_url=1;++cf=NULL;+current_config_kvi=NULL;+current_parsing_scope=0;++inc->remote_urls=xmalloc(sizeof(*inc->remote_urls));+string_list_init_dup(inc->remote_urls);+config_with_options(add_remote_url,inc->remote_urls,inc->config_source,&opts);++cf=store_cf;+current_config_kvi=store_kvi;+current_parsing_scope=store_scope;+}++staticintforbid_remote_url(constchar*var,constchar*value,void*data)+{+constchar*remote_name;+size_tremote_name_len;+constchar*key;++if(!parse_config_key(var,"remote",&remote_name,&remote_name_len,+&key)&&+remote_name&&+!strcmp(key,"url"))+die(_("remote URLs cannot be configured in file directly or indirectly included by includeIf.hasremoteurl"));+return0;+}++staticintat_least_one_url_matches_glob(constchar*glob,intglob_len,+structstring_list*remote_urls)+{+structstrbufpattern=STRBUF_INIT;+structstring_list_item*url_item;+intfound=0;++strbuf_add(&pattern,glob,glob_len);+for_each_string_list_item(url_item,remote_urls){+if(!wildmatch(pattern.buf,url_item->string,WM_PATHNAME)){+found=1;+break;+}+}+strbuf_release(&pattern);+returnfound;+}+staticintgit_config_include(constchar*var,constchar*value,void*data){structconfig_include_data*inc=data;constchar*cond,*key;size_tcond_len;-intret;+intret=0;/**Passalongallvalues,including"include"directives;thismakesit
+`hasremoteurl`::+ The data that follows the keyword `hasremoteurl:` is taken to+ be a pattern with standard globbing wildcards and two+ additional ones, `**/` and `/**`, that can match multiple+ components. The rest of the config files will be scanned for+ remote URLs, and then if there at least one remote URL that
if there {is,exists}* at least one remote URL that
quoted hunk
+ matches this pattern, the include condition is met.+++Files included by this option (directly or indirectly) are not allowed+to contain remote URLs.
As Jeff mentioned earlier in this thread, this "last-config-wins" is a
pretty big exception to the existing semantics, as
Documentation/config.txt reads:
The contents of the included file are inserted immediately, as if they
had been found at the location of the include directive.
At minimum, I think we should call out this exception in
Documentation/config.txt and the commit message, but calling out *just*
hasremoteurl makes this exception seem like a strange anomaly at first
glance, even though we actually have a good idea of when and why we are
doing this (which is that it simplifies includes that rely on config
values).
I was a big fan of your includeIfDeferred proposal, and I still think
that it's easier for users to understand if we explicitly require
"includeIfDeferred" instead of counting on them to remember when
"includeIf" behaves as it always did vs this new 'deferred' behavior.
That said, I doubt most users actually rely on the inclusion order, and
I am ok with this approach as long as we document the different
inclusion order.
When unconditional_remote_url is true, we forbid remote urls in the
included files as expected, but...
quoted hunk
+ ret = handle_path_include(value, inc);+ inc->fn = old_fn;+ } else {+ if (!inc->remote_urls)+ populate_remote_urls(inc);+ if (at_least_one_url_matches_glob(+ url, url_len, inc->remote_urls))+ ret = handle_path_include(value, inc);+ }+ } else if (include_condition_is_true(inc->opts, cond, cond_len)) {+ ret = handle_path_include(value, inc);+ }+ } return ret; }
It's not clear to me whether we are forbidding the remote urls correctly
when uncondition_remote_url is false. I would be convinced if we had
tests that convered this behavior, but I did not find any such test
cases.
As mentioned above, I would have expected to find test cases that test
whether or not we forbid the remote urls correctly, but the tests are
pretty clear.
+`hasremoteurl`::
+ The data that follows the keyword `hasremoteurl:` is taken to
Both here..
+ die(_("remote URLs cannot be configured in file directly or indirectly included by includeIf.hasremoteurl"));
..and here...
+ if (skip_prefix_mem(cond, cond_len, "hasremoteurl:", &url,
...but not here (C code)..
+ * For internal use. Include all includeif.hasremoteurl paths without
..but here..
+test_expect_success 'includeIf.hasremoteurl' '
..and also here etc., let's consistently camelCase config keys whenever
we're not using them for lookups in the C
code.
I.e. "includeIf.hasRemoteUrl" (possibly "includeIf.hasRemoteURL"?). It
makes them a lot easier to read, and makes the end-user documentation &
messaging more consistent.
From: Jonathan Tan <hidden> Date: 2021-11-29 22:30:22
On Mon, Nov 15 2021, Jonathan Tan wrote:
quoted
+`hasremoteurl`::
+ The data that follows the keyword `hasremoteurl:` is taken to
Both here..
quoted
+ die(_("remote URLs cannot be configured in file directly or indirectly included by includeIf.hasremoteurl"));
..and here...
quoted
+ if (skip_prefix_mem(cond, cond_len, "hasremoteurl:", &url,
...but not here (C code)..
quoted
+ * For internal use. Include all includeif.hasremoteurl paths without
..but here..
quoted
+test_expect_success 'includeIf.hasremoteurl' '
..and also here etc., let's consistently camelCase config keys whenever
we're not using them for lookups in the C
code.
I.e. "includeIf.hasRemoteUrl" (possibly "includeIf.hasRemoteURL"?). It
makes them a lot easier to read, and makes the end-user documentation &
messaging more consistent.
The middle part is not case-insensitive, though - I tried changing it in
the test and the test now fails. (Unless you mean that we should also
change the code to make it case-insensitive - but I would think that
it's better for the URL to be case-sensitive, and by extension, the
"hasremoteurl:" part connected to it.)
From: Jonathan Tan <hidden> Date: 2021-11-29 22:33:38
It is not used from outside the file in which it is declared.
Signed-off-by: Jonathan Tan <redacted>
---
config.c | 12 +++++++++++-
config.h | 37 ++++---------------------------------
2 files changed, 15 insertions(+), 34 deletions(-)
From: Jonathan Tan <hidden> Date: 2021-11-29 22:44:57
Glen Choo [off-list ref] writes:
Jonathan Tan [off-list ref] writes:
quoted
+`hasremoteurl`::+ The data that follows the keyword `hasremoteurl:` is taken to+ be a pattern with standard globbing wildcards and two+ additional ones, `**/` and `/**`, that can match multiple+ components. The rest of the config files will be scanned for+ remote URLs, and then if there at least one remote URL that
if there {is,exists}* at least one remote URL that
Ah, good catch.
quoted
+ matches this pattern, the include condition is met.+++Files included by this option (directly or indirectly) are not allowed+to contain remote URLs.
As Jeff mentioned earlier in this thread, this "last-config-wins" is a
pretty big exception to the existing semantics, as
Documentation/config.txt reads:
The contents of the included file are inserted immediately, as if they
had been found at the location of the include directive.
At minimum, I think we should call out this exception in
Documentation/config.txt and the commit message, but calling out *just*
hasremoteurl makes this exception seem like a strange anomaly at first
glance, even though we actually have a good idea of when and why we are
doing this (which is that it simplifies includes that rely on config
values).
I've switched it to expand-in-place semantics. The scanning for remote
URLs does not mean that those configs are applied before the include.
I'll add a note to the documentation about that, but if you can think of
a better way to explain that, that would be great.
The patch includes a test "includeIf.hasremoteurl respects
last-config-wins". Take a look and see if it matches your expected
behavior, and let me know if it could be clearer.
I was a big fan of your includeIfDeferred proposal, and I still think
that it's easier for users to understand if we explicitly require
"includeIfDeferred" instead of counting on them to remember when
"includeIf" behaves as it always did vs this new 'deferred' behavior.
That said, I doubt most users actually rely on the inclusion order, and
I am ok with this approach as long as we document the different
inclusion order.
The user still needs to know that config variables in the future can
affect the behavior of the include, but perhaps that will be easier than
remembering that certain configs are deferred.
It's not clear to me whether we are forbidding the remote urls correctly
when uncondition_remote_url is false. I would be convinced if we had
tests that convered this behavior, but I did not find any such test
cases.
[snip]
As mentioned above, I would have expected to find test cases that test
whether or not we forbid the remote urls correctly, but the tests are
pretty clear.
Ah yes, I should include a test for this. I'll include it in the next
reroll.
From: Jonathan Tan <hidden> Date: 2021-11-29 22:46:14
Thanks everyone for your comments. Here's an update.
Jonathan Tan (2):
config: make git_config_include() static
config: include file if remote URL matches a glob
Documentation/config.txt | 12 ++++
config.c | 133 ++++++++++++++++++++++++++++++++++++---
config.h | 44 ++++---------
t/t1300-config.sh | 118 ++++++++++++++++++++++++++++++++++
4 files changed, 265 insertions(+), 42 deletions(-)
Range-diff against v3:
1: b2dcae03ed = 1: b2dcae03ed config: make git_config_include() static
2: 1c1a07a0b6 ! 2: 3b3af0da98 config: include file if remote URL matches a glob
@@ Documentation/config.txt: all branches that begin with `foo/`. This is useful if
+ The data that follows the keyword `hasremoteurl:` is taken to
+ be a pattern with standard globbing wildcards and two
+ additional ones, `**/` and `/**`, that can match multiple
-+ components. The rest of the config files will be scanned for
-+ remote URLs, and then if there at least one remote URL that
-+ matches this pattern, the include condition is met.
++ components. The first time this keyword is seen, the rest of
++ the config files will be scanned for remote URLs (without
++ applying any values). If there exists at least one remote URL
++ that matches this pattern, the include condition is met.
++
+Files included by this option (directly or indirectly) are not allowed
+to contain remote URLs.
@@ t/t1300-config.sh: test_expect_success '--get and --get-all with --fixed-value'
+ git -C hasremoteurlTest config --get user.ssm &&
+ test_must_fail git -C hasremoteurlTest config --get user.no
+'
++
++test_expect_success 'includeIf.hasremoteurl forbids remote url in such included files' '
++ git init hasremoteurlTest &&
++ test_when_finished "rm -rf hasremoteurlTest" &&
++
++ cat >"$(pwd)"/include-with-url <<-\EOF &&
++ [remote "bar"]
++ url = bar
++ EOF
++ cat >>hasremoteurlTest/.git/config <<-EOF &&
++ [includeIf "hasremoteurl:foo"]
++ path = "$(pwd)/include-with-url"
++ EOF
++
++ # test with any Git command
++ test_must_fail git -C hasremoteurlTest status 2>err &&
++ grep "fatal: remote URLs cannot be configured in file directly or indirectly included by includeIf.hasremoteurl" err
++'
+
test_done
--
2.34.0.rc2.393.gf8c9666880-goog
+`hasremoteurl`::
+ The data that follows the keyword `hasremoteurl:` is taken to
Both here..
quoted
+ die(_("remote URLs cannot be configured in file directly or indirectly included by includeIf.hasremoteurl"));
..and here...
quoted
+ if (skip_prefix_mem(cond, cond_len, "hasremoteurl:", &url,
...but not here (C code)..
quoted
+ * For internal use. Include all includeif.hasremoteurl paths without
..but here..
quoted
+test_expect_success 'includeIf.hasremoteurl' '
..and also here etc., let's consistently camelCase config keys whenever
we're not using them for lookups in the C
code.
I.e. "includeIf.hasRemoteUrl" (possibly "includeIf.hasRemoteURL"?). It
makes them a lot easier to read, and makes the end-user documentation &
messaging more consistent.
The middle part is not case-insensitive, though - I tried changing it in
the test and the test now fails. (Unless you mean that we should also
change the code to make it case-insensitive - but I would think that
it's better for the URL to be case-sensitive, and by extension, the
"hasremoteurl:" part connected to it.)
Ah, I forgot about that edge case. sorry. And sent [1] without having
seen this as a reminder on v4. Makes sense.
(I seem to be getting really slow delivery from kernel.org to GMail
these days, sometimes I can see things on lore.kernel.org hours or half
a day before it pops up in my mail...)
1. https://lore.kernel.org/git/211129.864k7ug02c.gmgdl@evledraar.gmail.com/
From: Jonathan Tan <hidden> Date: 2021-11-29 22:53:25
This is a feature that supports config file inclusion conditional on
whether the repo has a remote with a URL that matches a glob.
Similar to my previous work on remote-suggested hooks [1], the main
motivation is to allow remote repo administrators to provide recommended
configs in a way that can be consumed more easily (e.g. through a
package installable by a package manager - it could, for example,
contain a file to be included conditionally and a post-install script
that adds the include directive to the system-wide config file).
In order to do this, Git reruns the config parsing mechanism upon
noticing the first URL-conditional include in order to find all remote
URLs, and these remote URLs are then used to determine if that first and
all subsequent includes are executed. Remote URLs are not allowed to be
configued in any URL-conditionally-included file.
[1] https://lore.kernel.org/git/cover.1623881977.git.jonathantanmy@google.com/
Signed-off-by: Jonathan Tan <redacted>
---
Documentation/config.txt | 12 ++++
config.c | 121 ++++++++++++++++++++++++++++++++++++---
config.h | 7 +++
t/t1300-config.sh | 118 ++++++++++++++++++++++++++++++++++++++
4 files changed, 250 insertions(+), 8 deletions(-)
@@ -159,6 +159,18 @@ all branches that begin with `foo/`. This is useful if your branches are organized hierarchically and you would like to apply a configuration to all the branches in that hierarchy.+`hasremoteurl`::+ The data that follows the keyword `hasremoteurl:` is taken to+ be a pattern with standard globbing wildcards and two+ additional ones, `**/` and `/**`, that can match multiple+ components. The first time this keyword is seen, the rest of+ the config files will be scanned for remote URLs (without+ applying any values). If there exists at least one remote URL+ that matches this pattern, the include condition is met.+++Files included by this option (directly or indirectly) are not allowed+to contain remote URLs.+ A few more notes on matching via `gitdir` and `gitdir/i`: * Symlinks in `$GIT_DIR` are not resolved before matching.
@@ -316,12 +322,83 @@ static int include_condition_is_true(const struct config_options *opts,return0;}+staticintadd_remote_url(constchar*var,constchar*value,void*data)+{+structstring_list*remote_urls=data;+constchar*remote_name;+size_tremote_name_len;+constchar*key;++if(!parse_config_key(var,"remote",&remote_name,&remote_name_len,+&key)&&+remote_name&&+!strcmp(key,"url"))+string_list_append(remote_urls,value);+return0;+}++staticvoidpopulate_remote_urls(structconfig_include_data*inc)+{+structconfig_optionsopts;++structconfig_source*store_cf=cf;+structkey_value_info*store_kvi=current_config_kvi;+enumconfig_scopestore_scope=current_parsing_scope;++opts=*inc->opts;+opts.unconditional_remote_url=1;++cf=NULL;+current_config_kvi=NULL;+current_parsing_scope=0;++inc->remote_urls=xmalloc(sizeof(*inc->remote_urls));+string_list_init_dup(inc->remote_urls);+config_with_options(add_remote_url,inc->remote_urls,inc->config_source,&opts);++cf=store_cf;+current_config_kvi=store_kvi;+current_parsing_scope=store_scope;+}++staticintforbid_remote_url(constchar*var,constchar*value,void*data)+{+constchar*remote_name;+size_tremote_name_len;+constchar*key;++if(!parse_config_key(var,"remote",&remote_name,&remote_name_len,+&key)&&+remote_name&&+!strcmp(key,"url"))+die(_("remote URLs cannot be configured in file directly or indirectly included by includeIf.hasremoteurl"));+return0;+}++staticintat_least_one_url_matches_glob(constchar*glob,intglob_len,+structstring_list*remote_urls)+{+structstrbufpattern=STRBUF_INIT;+structstring_list_item*url_item;+intfound=0;++strbuf_add(&pattern,glob,glob_len);+for_each_string_list_item(url_item,remote_urls){+if(!wildmatch(pattern.buf,url_item->string,WM_PATHNAME)){+found=1;+break;+}+}+strbuf_release(&pattern);+returnfound;+}+staticintgit_config_include(constchar*var,constchar*value,void*data){structconfig_include_data*inc=data;constchar*cond,*key;size_tcond_len;-intret;+intret=0;/**Passalongallvalues,including"include"directives;thismakesit
@@ -2387,4 +2387,122 @@ test_expect_success '--get and --get-all with --fixed-value' 'test_must_failgitconfig--file=config--get-regexp--fixed-valuefixed+non-existent'+test_expect_success'includeIf.hasremoteurl''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++cat>"$(pwd)"/include-this<<-\EOF&&+[user]+this=this-is-included+EOF+cat>"$(pwd)"/dont-include-that<<-\EOF&&+[user]+that=that-is-not-included+EOF+cat>>hasremoteurlTest/.git/config<<-EOF&&+[includeIf"hasremoteurl:foo"]+path="$(pwd)/include-this"+[includeIf"hasremoteurl:bar"]+path="$(pwd)/dont-include-that"+[remote"foo"]+url=foo+EOF++echothis-is-included>expect-this&&+git-ChasremoteurlTestconfig--getuser.this>actual-this&&+test_cmpexpect-thisactual-this&&++test_must_failgit-ChasremoteurlTestconfig--getuser.that+'++test_expect_success'includeIf.hasremoteurl respects last-config-wins''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++cat>"$(pwd)"/include-two-three<<-\EOF&&+[user]+two=included-config+three=included-config+EOF+cat>>hasremoteurlTest/.git/config<<-EOF&&+[remote"foo"]+url=foo+[user]+one=main-config+two=main-config+[includeIf"hasremoteurl:foo"]+path="$(pwd)/include-two-three"+[user]+three=main-config+EOF++echomain-config>expect-main-config&&+echoincluded-config>expect-included-config&&++git-ChasremoteurlTestconfig--getuser.one>actual&&+test_cmpexpect-main-configactual&&++git-ChasremoteurlTestconfig--getuser.two>actual&&+test_cmpexpect-included-configactual&&++git-ChasremoteurlTestconfig--getuser.three>actual&&+test_cmpexpect-main-configactual+'++test_expect_success'includeIf.hasremoteurl globs''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++printf"[user]\ndss = yes\n">"$(pwd)/double-star-start"&&+printf"[user]\ndse = yes\n">"$(pwd)/double-star-end"&&+printf"[user]\ndsm = yes\n">"$(pwd)/double-star-middle"&&+printf"[user]\nssm = yes\n">"$(pwd)/single-star-middle"&&+printf"[user]\nno = no\n">"$(pwd)/no"&&++cat>>hasremoteurlTest/.git/config<<-EOF&&+[remote"foo"]+url=https://foo/bar/baz+[includeIf"hasremoteurl:**/baz"]+path="$(pwd)/double-star-start"+[includeIf"hasremoteurl:**/nomatch"]+path="$(pwd)/no"+[includeIf"hasremoteurl:https:/**"]+path="$(pwd)/double-star-end"+[includeIf"hasremoteurl:nomatch:/**"]+path="$(pwd)/no"+[includeIf"hasremoteurl:https:/**/baz"]+path="$(pwd)/double-star-middle"+[includeIf"hasremoteurl:https:/**/nomatch"]+path="$(pwd)/no"+[includeIf"hasremoteurl:https://*/bar/baz"]+path="$(pwd)/single-star-middle"+[includeIf"hasremoteurl:https://*/baz"]+path="$(pwd)/no"+EOF++git-ChasremoteurlTestconfig--getuser.dss&&+git-ChasremoteurlTestconfig--getuser.dse&&+git-ChasremoteurlTestconfig--getuser.dsm&&+git-ChasremoteurlTestconfig--getuser.ssm&&+test_must_failgit-ChasremoteurlTestconfig--getuser.no+'++test_expect_success'includeIf.hasremoteurl forbids remote url in such included files''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++cat>"$(pwd)"/include-with-url<<-\EOF&&+[remote"bar"]+url=bar+EOF+cat>>hasremoteurlTest/.git/config<<-EOF&&+[includeIf"hasremoteurl:foo"]+path="$(pwd)/include-with-url"+EOF++# test with any Git command+test_must_failgit-ChasremoteurlTeststatus2>err&&+grep"fatal: remote URLs cannot be configured in file directly or indirectly included by includeIf.hasremoteurl"err+'+ test_done
From: Jonathan Tan <hidden> Date: 2021-12-02 23:31:47
It is not used from outside the file in which it is declared.
Signed-off-by: Jonathan Tan <redacted>
---
config.c | 12 +++++++++++-
config.h | 37 ++++---------------------------------
2 files changed, 15 insertions(+), 34 deletions(-)
From: Jonathan Tan <hidden> Date: 2021-12-02 23:31:48
This is a feature that supports config file inclusion conditional on
whether the repo has a remote with a URL that matches a glob.
Similar to my previous work on remote-suggested hooks [1], the main
motivation is to allow remote repo administrators to provide recommended
configs in a way that can be consumed more easily (e.g. through a
package installable by a package manager - it could, for example,
contain a file to be included conditionally and a post-install script
that adds the include directive to the system-wide config file).
In order to do this, Git reruns the config parsing mechanism upon
noticing the first URL-conditional include in order to find all remote
URLs, and these remote URLs are then used to determine if that first and
all subsequent includes are executed. Remote URLs are not allowed to be
configued in any URL-conditionally-included file.
[1] https://lore.kernel.org/git/cover.1623881977.git.jonathantanmy@google.com/
Signed-off-by: Jonathan Tan <redacted>
---
Documentation/config.txt | 16 +++++
config.c | 122 +++++++++++++++++++++++++++++++++++----
config.h | 9 +++
t/t1300-config.sh | 118 +++++++++++++++++++++++++++++++++++++
4 files changed, 255 insertions(+), 10 deletions(-)
@@ -159,6 +159,22 @@ all branches that begin with `foo/`. This is useful if your branches are organized hierarchically and you would like to apply a configuration to all the branches in that hierarchy.+`hasconfig:remote.*.url:`::+ The data that follows this keyword is taken to+ be a pattern with standard globbing wildcards and two+ additional ones, `**/` and `/**`, that can match multiple+ components. The first time this keyword is seen, the rest of+ the config files will be scanned for remote URLs (without+ applying any values). If there exists at least one remote URL+ that matches this pattern, the include condition is met.+++Files included by this option (directly or indirectly) are not allowed+to contain remote URLs.+++This keyword is designed to be forwards compatible with a naming+scheme that supports more variable-based include conditions, but+currently Git only supports the exact keyword described above.+ A few more notes on matching via `gitdir` and `gitdir/i`: * Symlinks in `$GIT_DIR` are not resolved before matching.
@@ -301,16 +307,97 @@ static int include_by_branch(const char *cond, size_t cond_len)returnret;}-staticintinclude_condition_is_true(conststructconfig_options*opts,+staticintadd_remote_url(constchar*var,constchar*value,void*data)+{+structstring_list*remote_urls=data;+constchar*remote_name;+size_tremote_name_len;+constchar*key;++if(!parse_config_key(var,"remote",&remote_name,&remote_name_len,+&key)&&+remote_name&&+!strcmp(key,"url"))+string_list_append(remote_urls,value);+return0;+}++staticvoidpopulate_remote_urls(structconfig_include_data*inc)+{+structconfig_optionsopts;++structconfig_source*store_cf=cf;+structkey_value_info*store_kvi=current_config_kvi;+enumconfig_scopestore_scope=current_parsing_scope;++opts=*inc->opts;+opts.unconditional_remote_url=1;++cf=NULL;+current_config_kvi=NULL;+current_parsing_scope=0;++inc->remote_urls=xmalloc(sizeof(*inc->remote_urls));+string_list_init_dup(inc->remote_urls);+config_with_options(add_remote_url,inc->remote_urls,inc->config_source,&opts);++cf=store_cf;+current_config_kvi=store_kvi;+current_parsing_scope=store_scope;+}++staticintforbid_remote_url(constchar*var,constchar*value,void*data)+{+constchar*remote_name;+size_tremote_name_len;+constchar*key;++if(!parse_config_key(var,"remote",&remote_name,&remote_name_len,+&key)&&+remote_name&&+!strcmp(key,"url"))+die(_("remote URLs cannot be configured in file directly or indirectly included by includeIf.hasconfig:remote.*.url"));+return0;+}++staticintat_least_one_url_matches_glob(constchar*glob,intglob_len,+structstring_list*remote_urls)+{+structstrbufpattern=STRBUF_INIT;+structstring_list_item*url_item;+intfound=0;++strbuf_add(&pattern,glob,glob_len);+for_each_string_list_item(url_item,remote_urls){+if(!wildmatch(pattern.buf,url_item->string,WM_PATHNAME)){+found=1;+break;+}+}+strbuf_release(&pattern);+returnfound;+}++staticintinclude_condition_is_true(structconfig_include_data*inc,constchar*cond,size_tcond_len){+conststructconfig_options*opts=inc->opts;-if(skip_prefix_mem(cond,cond_len,"gitdir:",&cond,&cond_len))+if(skip_prefix_mem(cond,cond_len,"gitdir:",&cond,&cond_len)){returninclude_by_gitdir(opts,cond,cond_len,0);-elseif(skip_prefix_mem(cond,cond_len,"gitdir/i:",&cond,&cond_len))+}elseif(skip_prefix_mem(cond,cond_len,"gitdir/i:",&cond,&cond_len)){returninclude_by_gitdir(opts,cond,cond_len,1);-elseif(skip_prefix_mem(cond,cond_len,"onbranch:",&cond,&cond_len))+}elseif(skip_prefix_mem(cond,cond_len,"onbranch:",&cond,&cond_len)){returninclude_by_branch(cond,cond_len);+}elseif(skip_prefix_mem(cond,cond_len,"hasconfig:remote.*.url:",&cond,+&cond_len)){+if(inc->opts->unconditional_remote_url)+return1;+if(!inc->remote_urls)+populate_remote_urls(inc);+returnat_least_one_url_matches_glob(cond,cond_len,+inc->remote_urls);+}/* unknown conditionals are always false */return0;
@@ -2387,4 +2387,122 @@ test_expect_success '--get and --get-all with --fixed-value' 'test_must_failgitconfig--file=config--get-regexp--fixed-valuefixed+non-existent'+test_expect_success'includeIf.hasconfig:remote.*.url''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++cat>"$(pwd)"/include-this<<-\EOF&&+[user]+this=this-is-included+EOF+cat>"$(pwd)"/dont-include-that<<-\EOF&&+[user]+that=that-is-not-included+EOF+cat>>hasremoteurlTest/.git/config<<-EOF&&+[includeIf"hasconfig:remote.*.url:foo"]+path="$(pwd)/include-this"+[includeIf"hasconfig:remote.*.url:bar"]+path="$(pwd)/dont-include-that"+[remote"foo"]+url=foo+EOF++echothis-is-included>expect-this&&+git-ChasremoteurlTestconfig--getuser.this>actual-this&&+test_cmpexpect-thisactual-this&&++test_must_failgit-ChasremoteurlTestconfig--getuser.that+'++test_expect_success'includeIf.hasconfig:remote.*.url respects last-config-wins''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++cat>"$(pwd)"/include-two-three<<-\EOF&&+[user]+two=included-config+three=included-config+EOF+cat>>hasremoteurlTest/.git/config<<-EOF&&+[remote"foo"]+url=foo+[user]+one=main-config+two=main-config+[includeIf"hasconfig:remote.*.url:foo"]+path="$(pwd)/include-two-three"+[user]+three=main-config+EOF++echomain-config>expect-main-config&&+echoincluded-config>expect-included-config&&++git-ChasremoteurlTestconfig--getuser.one>actual&&+test_cmpexpect-main-configactual&&++git-ChasremoteurlTestconfig--getuser.two>actual&&+test_cmpexpect-included-configactual&&++git-ChasremoteurlTestconfig--getuser.three>actual&&+test_cmpexpect-main-configactual+'++test_expect_success'includeIf.hasconfig:remote.*.url globs''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++printf"[user]\ndss = yes\n">"$(pwd)/double-star-start"&&+printf"[user]\ndse = yes\n">"$(pwd)/double-star-end"&&+printf"[user]\ndsm = yes\n">"$(pwd)/double-star-middle"&&+printf"[user]\nssm = yes\n">"$(pwd)/single-star-middle"&&+printf"[user]\nno = no\n">"$(pwd)/no"&&++cat>>hasremoteurlTest/.git/config<<-EOF&&+[remote"foo"]+url=https://foo/bar/baz+[includeIf"hasconfig:remote.*.url:**/baz"]+path="$(pwd)/double-star-start"+[includeIf"hasconfig:remote.*.url:**/nomatch"]+path="$(pwd)/no"+[includeIf"hasconfig:remote.*.url:https:/**"]+path="$(pwd)/double-star-end"+[includeIf"hasconfig:remote.*.url:nomatch:/**"]+path="$(pwd)/no"+[includeIf"hasconfig:remote.*.url:https:/**/baz"]+path="$(pwd)/double-star-middle"+[includeIf"hasconfig:remote.*.url:https:/**/nomatch"]+path="$(pwd)/no"+[includeIf"hasconfig:remote.*.url:https://*/bar/baz"]+path="$(pwd)/single-star-middle"+[includeIf"hasconfig:remote.*.url:https://*/baz"]+path="$(pwd)/no"+EOF++git-ChasremoteurlTestconfig--getuser.dss&&+git-ChasremoteurlTestconfig--getuser.dse&&+git-ChasremoteurlTestconfig--getuser.dsm&&+git-ChasremoteurlTestconfig--getuser.ssm&&+test_must_failgit-ChasremoteurlTestconfig--getuser.no+'++test_expect_success'includeIf.hasconfig:remote.*.url forbids remote url in such included files''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++cat>"$(pwd)"/include-with-url<<-\EOF&&+[remote"bar"]+url=bar+EOF+cat>>hasremoteurlTest/.git/config<<-EOF&&+[includeIf"hasconfig:remote.*.url:foo"]+path="$(pwd)/include-with-url"+EOF++# test with any Git command+test_must_failgit-ChasremoteurlTeststatus2>err&&+grep"fatal: remote URLs cannot be configured in file directly or indirectly included by includeIf.hasconfig:remote.*.url"err+'+ test_done
Thanks, Junio, for your comments. I think the code is more clearly laid
out now.
The main changes from v4 are that I've maintained the existing code
structure more, and changed the keyword used to something that hopefully
will be more forwards compatible. I've also updated the documentation to
explain the forwards compatibility idea.
I read through this and came up with the below as a proposed squash-in
just while reading through it. These may or may not help. Changes:
* There was some needless "$(pwd)" in the tests
* Inlining the "remote_urls" in the struct makes its management easier;
and the free/NULL checks just check .nr now, and string_list_clear() can be
unconditional.
* Created a include_by_remote_url() function. Makes the overall diff smaller
since you don't need to add braces to everything in include_condition_is_true()
Other comments (not related to the below):
* It would be nice if e.g. the "includeIf.hasconfig:remote.*.url globs" test
were split up by condition, but maybe that's a hassle (would need a small helper).
Just something that would have helped while hacking on this, i.e. now most of it
was an all-or-nothing failure & peek at the trace output
* Your last test appears to entirely forbid recursion. I.e. we die if you include config
which in turn tries to use this include mechanism, right?
That's probably wise, and it is explicitly documented.
But as far as the documentation about this being a forward-compatible facility, do we
think that this limitation would apply to any future config key? I.e. if I include based
on "user.email" nothing in that to-be-included can set user.email?
That's probably OK, just wondering. In any case it can always be expanded later on.
@@ -159,6 +159,22 @@ all branches that begin with `foo/`. This is useful if your branches are organized hierarchically and you would like to apply a configuration to all the branches in that hierarchy.+`hasconfig:remote.*.url:`::+ The data that follows this keyword is taken to+ be a pattern with standard globbing wildcards and two+ additional ones, `**/` and `/**`, that can match multiple+ components. The first time this keyword is seen, the rest of+ the config files will be scanned for remote URLs (without+ applying any values). If there exists at least one remote URL+ that matches this pattern, the include condition is met.+++Files included by this option (directly or indirectly) are not allowed+to contain remote URLs.+++This keyword is designed to be forwards compatible with a naming+scheme that supports more variable-based include conditions, but+currently Git only supports the exact keyword described above.+
A reader of this description doesn't have any reason to think that
`hasconfig:remote.*.url` wouldn't respect in-place semantics, so my
concern in [1] is addressed.
`hasconfig:foo.*.bar` seems reasonable from a forwards-compatibility
perspective. Ideally, it would be nice to see a generic implementation
that actually handles config values beyond `remote.*.url`, but unless we
take a closer look at all config values and the conditions we would like
to support, a generic implementation seems like a premature
optimization that won't age well.
So OK to having a forward-compatible name without a forward compatible
implementation.
quoted hunk
+static int include_condition_is_true(struct config_include_data *inc, const char *cond, size_t cond_len) {+ const struct config_options *opts = inc->opts;- if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len))+ if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len)) { return include_by_gitdir(opts, cond, cond_len, 0);- else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))+ } else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len)) { return include_by_gitdir(opts, cond, cond_len, 1);- else if (skip_prefix_mem(cond, cond_len, "onbranch:", &cond, &cond_len))+ } else if (skip_prefix_mem(cond, cond_len, "onbranch:", &cond, &cond_len)) { return include_by_branch(cond, cond_len);+ } else if (skip_prefix_mem(cond, cond_len, "hasconfig:remote.*.url:", &cond,+ &cond_len)) {+ if (inc->opts->unconditional_remote_url)+ return 1;+ if (!inc->remote_urls)+ populate_remote_urls(inc);+ return at_least_one_url_matches_glob(cond, cond_len,+ inc->remote_urls);+ } /* unknown conditionals are always false */ return 0;
Nit: I have a preference for Ævar's version [2], which looks more
consistent with the rest of the function i.e. handling the match using a
helper function.
+test_expect_success 'includeIf.hasconfig:remote.*.url forbids remote url in such included files' '
+ git init hasremoteurlTest &&
+ test_when_finished "rm -rf hasremoteurlTest" &&
+
+ cat >"$(pwd)"/include-with-url <<-\EOF &&
+ [remote "bar"]
+ url = bar
+ EOF
+ cat >>hasremoteurlTest/.git/config <<-EOF &&
+ [includeIf "hasconfig:remote.*.url:foo"]
+ path = "$(pwd)/include-with-url"
+ EOF
+
+ # test with any Git command
+ test_must_fail git -C hasremoteurlTest status 2>err &&
+ grep "fatal: remote URLs cannot be configured in file directly or indirectly included by includeIf.hasconfig:remote.*.url" err
+'
+
test_done
--
2.34.1.400.ga245620fadb-goog
From: Jonathan Tan <hidden> Date: 2021-12-07 17:46:23
I read through this and came up with the below as a proposed squash-in
just while reading through it. These may or may not help. Changes:
* There was some needless "$(pwd)" in the tests
Ah, thanks for catching that.
* Inlining the "remote_urls" in the struct makes its management easier;
and the free/NULL checks just check .nr now, and string_list_clear() can be
unconditional.
I don't think we can do this - nr might still be 0 after a scan if we
don't have remote URLs for some reason, so we still need to distinguish
between not-scanned and scanned-with-zero-URLs.
* Created a include_by_remote_url() function. Makes the overall diff smaller
since you don't need to add braces to everything in include_condition_is_true()
Ah, good idea. I'll do this.
Other comments (not related to the below):
* It would be nice if e.g. the "includeIf.hasconfig:remote.*.url globs" test
were split up by condition, but maybe that's a hassle (would need a small helper).
Just something that would have helped while hacking on this, i.e. now most of it
was an all-or-nothing failure & peek at the trace output
What do you mean by condition? There seems to only be one condition
(whether the URL is there or not), unless you were thinking of smaller
subdivisions.
* Your last test appears to entirely forbid recursion. I.e. we die if you include config
which in turn tries to use this include mechanism, right?
That's probably wise, and it is explicitly documented.
But as far as the documentation about this being a forward-compatible facility, do we
think that this limitation would apply to any future config key? I.e. if I include based
on "user.email" nothing in that to-be-included can set user.email?
That's probably OK, just wondering. In any case it can always be expanded later on.
We can decide later what the future facility will be, but I envision
that we will not allow included files to set config that can affect any
include directives in use. So, for example, if I have a user.email-based
include, none of my config-conditionally included files can set user.email.
From: Jonathan Tan <hidden> Date: 2021-12-07 17:53:06
Glen Choo [off-list ref] writes:
A reader of this description doesn't have any reason to think that
`hasconfig:remote.*.url` wouldn't respect in-place semantics, so my
concern in [1] is addressed.
`hasconfig:foo.*.bar` seems reasonable from a forwards-compatibility
perspective. Ideally, it would be nice to see a generic implementation
that actually handles config values beyond `remote.*.url`, but unless we
take a closer look at all config values and the conditions we would like
to support, a generic implementation seems like a premature
optimization that won't age well.
So OK to having a forward-compatible name without a forward compatible
implementation.
Thanks for taking a look at this.
Nit: I have a preference for Ævar's version [2], which looks more
consistent with the rest of the function i.e. handling the match using a
helper function.
I read through this and came up with the below as a proposed squash-in
just while reading through it. These may or may not help. Changes:
* There was some needless "$(pwd)" in the tests
Ah, thanks for catching that.
quoted
* Inlining the "remote_urls" in the struct makes its management easier;
and the free/NULL checks just check .nr now, and string_list_clear() can be
unconditional.
I don't think we can do this - nr might still be 0 after a scan if we
don't have remote URLs for some reason, so we still need to distinguish
between not-scanned and scanned-with-zero-URLs.
You mean so that we don't double-free? The way string_list_clear()
protects against that, but maybe there's something else.
Whatever it is (if there's anything) it could use test coverage then :)
quoted
* Created a include_by_remote_url() function. Makes the overall diff smaller
since you don't need to add braces to everything in include_condition_is_true()
Ah, good idea. I'll do this.
quoted
Other comments (not related to the below):
* It would be nice if e.g. the "includeIf.hasconfig:remote.*.url globs" test
were split up by condition, but maybe that's a hassle (would need a small helper).
Just something that would have helped while hacking on this, i.e. now most of it
was an all-or-nothing failure & peek at the trace output
What do you mean by condition? There seems to only be one condition
(whether the URL is there or not), unless you were thinking of smaller
subdivisions.
Maybe I'm just misunderstanding the intent here, but aren't you trying
to guard against the case of having a ~/.gitconfig that includes
~/.gitconfig.d/for-this-url, and *that* file in turns changes the
remote's "url" in its config, followed by another "include if url
matches" condition therein?
I.e. I read (more like skimmed) the documentation & test at the end as
forbidding that, but maybe that's OK?
quoted
* Your last test appears to entirely forbid recursion. I.e. we die if you include config
which in turn tries to use this include mechanism, right?
That's probably wise, and it is explicitly documented.
But as far as the documentation about this being a forward-compatible facility, do we
think that this limitation would apply to any future config key? I.e. if I include based
on "user.email" nothing in that to-be-included can set user.email?
That's probably OK, just wondering. In any case it can always be expanded later on.
We can decide later what the future facility will be, but I envision
that we will not allow included files to set config that can affect any
include directives in use. So, for example, if I have a user.email-based
include, none of my config-conditionally included files can set user.email.
I didn't look deeply at the implementation at all, but why would this be
a problem?
You parse ~/.gitconfig, it has user.name=foo, then right after in that
file we do:
[includeIf "hasconfig:user.name:*foo*"]
path = ~/.gitconfig.d/foo
Now the top of ~/.gitconfig.d/foo we have:
[user]
name = bar
[includeIf "hasconfig:user.name:*bar*"]
path = ~/.gitconfig.d/bar
Why would it matter that we included on user.name=foo before?
Doesn't that only matter *while* we process that first "path" line? Once
we move past it we update our configset to user.name=bar once we hit the
"name" line of the included file.
Then when we get another "hasconfig:user.name" we just match it to our
current user.name=*bar*.
No?
Anyway, I think it's fine to punt on it for now or whatever, just
curious...
From: Jonathan Tan <hidden> Date: 2021-12-07 18:53:00
quoted
quoted
* Inlining the "remote_urls" in the struct makes its management easier;
and the free/NULL checks just check .nr now, and string_list_clear() can be
unconditional.
I don't think we can do this - nr might still be 0 after a scan if we
don't have remote URLs for some reason, so we still need to distinguish
between not-scanned and scanned-with-zero-URLs.
You mean so that we don't double-free? The way string_list_clear()
protects against that, but maybe there's something else.
Whatever it is (if there's anything) it could use test coverage then :)
No - we only want to do one scan per config read. If we scan and there
are no remote URLs, with your scheme, next time we encounter another
includeIf.hasconfig, we would need to scan again (because nr is still
0). With my scheme, we can see that the pointer is non-NULL, so we know
that we have already scanned.
quoted
quoted
* It would be nice if e.g. the "includeIf.hasconfig:remote.*.url globs" test
were split up by condition, but maybe that's a hassle (would need a small helper).
Just something that would have helped while hacking on this, i.e. now most of it
was an all-or-nothing failure & peek at the trace output
What do you mean by condition? There seems to only be one condition
(whether the URL is there or not), unless you were thinking of smaller
subdivisions.
Maybe I'm just misunderstanding the intent here, but aren't you trying
to guard against the case of having a ~/.gitconfig that includes
~/.gitconfig.d/for-this-url, and *that* file in turns changes the
remote's "url" in its config, followed by another "include if url
matches" condition therein?
I.e. I read (more like skimmed) the documentation & test at the end as
forbidding that, but maybe that's OK?
If we're including "~/.gitconfig.d/for-this-url" by includeIf.hasconfig,
then yes, I'm guarding against that and other similar conditions.
quoted
We can decide later what the future facility will be, but I envision
that we will not allow included files to set config that can affect any
include directives in use. So, for example, if I have a user.email-based
include, none of my config-conditionally included files can set user.email.
I didn't look deeply at the implementation at all, but why would this be
a problem?
You parse ~/.gitconfig, it has user.name=foo, then right after in that
file we do:
[includeIf "hasconfig:user.name:*foo*"]
path = ~/.gitconfig.d/foo
Now the top of ~/.gitconfig.d/foo we have:
[user]
name = bar
[includeIf "hasconfig:user.name:*bar*"]
path = ~/.gitconfig.d/bar
Why would it matter that we included on user.name=foo before?
Doesn't that only matter *while* we process that first "path" line? Once
we move past it we update our configset to user.name=bar once we hit the
"name" line of the included file.
Then when we get another "hasconfig:user.name" we just match it to our
current user.name=*bar*.
No?
Anyway, I think it's fine to punt on it for now or whatever, just
curious...
Well, we can't punt on it because what you describe also applies to
remote URL :-)
So what you're saying is that once we have decided to include a file, we
always include it in its entirety regardless of whether the condition
changes during the file's include. That's reasonable, but other people
could have differing opinions. In this case, I think it's fine just to
prohibit it entirely. In the future, we may look into relaxing this
condition.
From: Jonathan Tan <hidden> Date: 2021-12-07 23:24:00
It is not used from outside the file in which it is declared.
Signed-off-by: Jonathan Tan <redacted>
---
config.c | 12 +++++++++++-
config.h | 37 ++++---------------------------------
2 files changed, 15 insertions(+), 34 deletions(-)
From: Jonathan Tan <hidden> Date: 2021-12-07 23:24:04
This is a feature that supports config file inclusion conditional on
whether the repo has a remote with a URL that matches a glob.
Similar to my previous work on remote-suggested hooks [1], the main
motivation is to allow remote repo administrators to provide recommended
configs in a way that can be consumed more easily (e.g. through a
package installable by a package manager - it could, for example,
contain a file to be included conditionally and a post-install script
that adds the include directive to the system-wide config file).
In order to do this, Git reruns the config parsing mechanism upon
noticing the first URL-conditional include in order to find all remote
URLs, and these remote URLs are then used to determine if that first and
all subsequent includes are executed. Remote URLs are not allowed to be
configued in any URL-conditionally-included file.
[1] https://lore.kernel.org/git/cover.1623881977.git.jonathantanmy@google.com/
Signed-off-by: Jonathan Tan <redacted>
---
Documentation/config.txt | 16 ++++++
config.c | 121 ++++++++++++++++++++++++++++++++++++---
config.h | 9 +++
t/t1300-config.sh | 118 ++++++++++++++++++++++++++++++++++++++
4 files changed, 257 insertions(+), 7 deletions(-)
@@ -159,6 +159,22 @@ all branches that begin with `foo/`. This is useful if your branches are organized hierarchically and you would like to apply a configuration to all the branches in that hierarchy.+`hasconfig:remote.*.url:`::+ The data that follows this keyword is taken to+ be a pattern with standard globbing wildcards and two+ additional ones, `**/` and `/**`, that can match multiple+ components. The first time this keyword is seen, the rest of+ the config files will be scanned for remote URLs (without+ applying any values). If there exists at least one remote URL+ that matches this pattern, the include condition is met.+++Files included by this option (directly or indirectly) are not allowed+to contain remote URLs.+++This keyword is designed to be forwards compatible with a naming+scheme that supports more variable-based include conditions, but+currently Git only supports the exact keyword described above.+ A few more notes on matching via `gitdir` and `gitdir/i`: * Symlinks in `$GIT_DIR` are not resolved before matching.
@@ -301,9 +307,92 @@ static int include_by_branch(const char *cond, size_t cond_len)returnret;}-staticintinclude_condition_is_true(conststructconfig_options*opts,+staticintadd_remote_url(constchar*var,constchar*value,void*data)+{+structstring_list*remote_urls=data;+constchar*remote_name;+size_tremote_name_len;+constchar*key;++if(!parse_config_key(var,"remote",&remote_name,&remote_name_len,+&key)&&+remote_name&&+!strcmp(key,"url"))+string_list_append(remote_urls,value);+return0;+}++staticvoidpopulate_remote_urls(structconfig_include_data*inc)+{+structconfig_optionsopts;++structconfig_source*store_cf=cf;+structkey_value_info*store_kvi=current_config_kvi;+enumconfig_scopestore_scope=current_parsing_scope;++opts=*inc->opts;+opts.unconditional_remote_url=1;++cf=NULL;+current_config_kvi=NULL;+current_parsing_scope=0;++inc->remote_urls=xmalloc(sizeof(*inc->remote_urls));+string_list_init_dup(inc->remote_urls);+config_with_options(add_remote_url,inc->remote_urls,inc->config_source,&opts);++cf=store_cf;+current_config_kvi=store_kvi;+current_parsing_scope=store_scope;+}++staticintforbid_remote_url(constchar*var,constchar*value,void*data)+{+constchar*remote_name;+size_tremote_name_len;+constchar*key;++if(!parse_config_key(var,"remote",&remote_name,&remote_name_len,+&key)&&+remote_name&&+!strcmp(key,"url"))+die(_("remote URLs cannot be configured in file directly or indirectly included by includeIf.hasconfig:remote.*.url"));+return0;+}++staticintat_least_one_url_matches_glob(constchar*glob,intglob_len,+structstring_list*remote_urls)+{+structstrbufpattern=STRBUF_INIT;+structstring_list_item*url_item;+intfound=0;++strbuf_add(&pattern,glob,glob_len);+for_each_string_list_item(url_item,remote_urls){+if(!wildmatch(pattern.buf,url_item->string,WM_PATHNAME)){+found=1;+break;+}+}+strbuf_release(&pattern);+returnfound;+}++staticintinclude_by_remote_url(structconfig_include_data*inc,+constchar*cond,size_tcond_len)+{+if(inc->opts->unconditional_remote_url)+return1;+if(!inc->remote_urls)+populate_remote_urls(inc);+returnat_least_one_url_matches_glob(cond,cond_len,+inc->remote_urls);+}++staticintinclude_condition_is_true(structconfig_include_data*inc,constchar*cond,size_tcond_len){+conststructconfig_options*opts=inc->opts;if(skip_prefix_mem(cond,cond_len,"gitdir:",&cond,&cond_len))returninclude_by_gitdir(opts,cond,cond_len,0);
@@ -311,6 +400,9 @@ static int include_condition_is_true(const struct config_options *opts,returninclude_by_gitdir(opts,cond,cond_len,1);elseif(skip_prefix_mem(cond,cond_len,"onbranch:",&cond,&cond_len))returninclude_by_branch(cond,cond_len);+elseif(skip_prefix_mem(cond,cond_len,"hasconfig:remote.*.url:",&cond,+&cond_len))+returninclude_by_remote_url(inc,cond,cond_len);/* unknown conditionals are always false */return0;
@@ -2387,4 +2387,122 @@ test_expect_success '--get and --get-all with --fixed-value' 'test_must_failgitconfig--file=config--get-regexp--fixed-valuefixed+non-existent'+test_expect_success'includeIf.hasconfig:remote.*.url''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++cat>include-this<<-\EOF&&+[user]+this=this-is-included+EOF+cat>dont-include-that<<-\EOF&&+[user]+that=that-is-not-included+EOF+cat>>hasremoteurlTest/.git/config<<-EOF&&+[includeIf"hasconfig:remote.*.url:foo"]+path="$(pwd)/include-this"+[includeIf"hasconfig:remote.*.url:bar"]+path="$(pwd)/dont-include-that"+[remote"foo"]+url=foo+EOF++echothis-is-included>expect-this&&+git-ChasremoteurlTestconfig--getuser.this>actual-this&&+test_cmpexpect-thisactual-this&&++test_must_failgit-ChasremoteurlTestconfig--getuser.that+'++test_expect_success'includeIf.hasconfig:remote.*.url respects last-config-wins''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++cat>include-two-three<<-\EOF&&+[user]+two=included-config+three=included-config+EOF+cat>>hasremoteurlTest/.git/config<<-EOF&&+[remote"foo"]+url=foo+[user]+one=main-config+two=main-config+[includeIf"hasconfig:remote.*.url:foo"]+path="$(pwd)/include-two-three"+[user]+three=main-config+EOF++echomain-config>expect-main-config&&+echoincluded-config>expect-included-config&&++git-ChasremoteurlTestconfig--getuser.one>actual&&+test_cmpexpect-main-configactual&&++git-ChasremoteurlTestconfig--getuser.two>actual&&+test_cmpexpect-included-configactual&&++git-ChasremoteurlTestconfig--getuser.three>actual&&+test_cmpexpect-main-configactual+'++test_expect_success'includeIf.hasconfig:remote.*.url globs''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++printf"[user]\ndss = yes\n">double-star-start&&+printf"[user]\ndse = yes\n">double-star-end&&+printf"[user]\ndsm = yes\n">double-star-middle&&+printf"[user]\nssm = yes\n">single-star-middle&&+printf"[user]\nno = no\n">no&&++cat>>hasremoteurlTest/.git/config<<-EOF&&+[remote"foo"]+url=https://foo/bar/baz+[includeIf"hasconfig:remote.*.url:**/baz"]+path="$(pwd)/double-star-start"+[includeIf"hasconfig:remote.*.url:**/nomatch"]+path="$(pwd)/no"+[includeIf"hasconfig:remote.*.url:https:/**"]+path="$(pwd)/double-star-end"+[includeIf"hasconfig:remote.*.url:nomatch:/**"]+path="$(pwd)/no"+[includeIf"hasconfig:remote.*.url:https:/**/baz"]+path="$(pwd)/double-star-middle"+[includeIf"hasconfig:remote.*.url:https:/**/nomatch"]+path="$(pwd)/no"+[includeIf"hasconfig:remote.*.url:https://*/bar/baz"]+path="$(pwd)/single-star-middle"+[includeIf"hasconfig:remote.*.url:https://*/baz"]+path="$(pwd)/no"+EOF++git-ChasremoteurlTestconfig--getuser.dss&&+git-ChasremoteurlTestconfig--getuser.dse&&+git-ChasremoteurlTestconfig--getuser.dsm&&+git-ChasremoteurlTestconfig--getuser.ssm&&+test_must_failgit-ChasremoteurlTestconfig--getuser.no+'++test_expect_success'includeIf.hasconfig:remote.*.url forbids remote url in such included files''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++cat>include-with-url<<-\EOF&&+[remote"bar"]+url=bar+EOF+cat>>hasremoteurlTest/.git/config<<-EOF&&+[includeIf"hasconfig:remote.*.url:foo"]+path="$(pwd)/include-with-url"+EOF++# test with any Git command+test_must_failgit-ChasremoteurlTeststatus2>err&&+grep"fatal: remote URLs cannot be configured in file directly or indirectly included by includeIf.hasconfig:remote.*.url"err+'+ test_done
Minor nit: it looks like we don't need to restore inc->fn conditionally,
so instead of:
if (inc->opts->unconditional_remote_url)
inc->fn = old_fn;
we could just have:
inc->fn = old_fn;
which (purely as a matter of personal taste) looks a bit more consistent
with the unconditional assignment of:
config_fn_t old_fn = inc->fn;
No comments on the rest of the patch; it looks clean and
easy-to-understand :)
@@ -159,6 +159,22 @@ all branches that begin with `foo/`. This is useful if your branches are organized hierarchically and you would like to apply a configuration to all the branches in that hierarchy.+`hasconfig:remote.*.url:`::+ The data that follows this keyword is taken to+ be a pattern with standard globbing wildcards and two+ additional ones, `**/` and `/**`, that can match multiple+ components. The first time this keyword is seen, the rest of+ the config files will be scanned for remote URLs (without+ applying any values). If there exists at least one remote URL+ that matches this pattern, the include condition is met.+++Files included by this option (directly or indirectly) are not allowed+to contain remote URLs.
Wondering out loud.. Reading this and Ævar's comment [1], I wonder if we
should make it clear to users *why* we choose to forbid remote URLs.
Since this series is setting a precedent for future "hasconfig:"
conditions (files included by "hasconfig:foo.*.bar" cannot contain any
"foo.*.bar" values), it would be useful to git developers to explain
*why* we chose to do this. And if we're documenting it for ourselves,
we might as well write it in the public docs. That way, users would know
that this is more of a guardrail (because it's simpler to understand
this way) than a hard limitation.
[1] https://lore.kernel.org/git/211207.86k0ggnvfo.gmgdl@evledraar.gmail.com
From: Jonathan Tan <hidden> Date: 2021-12-09 22:16:29
Glen Choo [off-list ref] writes:
Minor nit: it looks like we don't need to restore inc->fn conditionally,
so instead of:
if (inc->opts->unconditional_remote_url)
inc->fn = old_fn;
we could just have:
inc->fn = old_fn;
which (purely as a matter of personal taste) looks a bit more consistent
with the unconditional assignment of:
config_fn_t old_fn = inc->fn;
No comments on the rest of the patch; it looks clean and
easy-to-understand :)
Thanks for taking a look. This is a good suggestion - I'll use it.
From: Jonathan Tan <hidden> Date: 2021-12-09 22:39:23
Glen Choo [off-list ref] writes:
quoted
+`hasconfig:remote.*.url:`::+ The data that follows this keyword is taken to+ be a pattern with standard globbing wildcards and two+ additional ones, `**/` and `/**`, that can match multiple+ components. The first time this keyword is seen, the rest of+ the config files will be scanned for remote URLs (without+ applying any values). If there exists at least one remote URL+ that matches this pattern, the include condition is met.+++Files included by this option (directly or indirectly) are not allowed+to contain remote URLs.
Wondering out loud.. Reading this and Ævar's comment [1], I wonder if we
should make it clear to users *why* we choose to forbid remote URLs.
Since this series is setting a precedent for future "hasconfig:"
conditions (files included by "hasconfig:foo.*.bar" cannot contain any
"foo.*.bar" values), it would be useful to git developers to explain
*why* we chose to do this. And if we're documenting it for ourselves,
we might as well write it in the public docs. That way, users would know
that this is more of a guardrail (because it's simpler to understand
this way) than a hard limitation.
[1] https://lore.kernel.org/git/211207.86k0ggnvfo.gmgdl@evledraar.gmail.com
The explanation is rather long, though. It goes something like this:
If the main config is:
[remote a]
url = bar
[includeif hasconfig:remote.*.url:foo]
path = foo
[includeif hasconfig:remote.*.url:bar]
path = bar
and "bar" contains:
[remote b]
url = foo
Should "foo" be included? For now, we avoid these situations
completely by prohibiting URLs from being configured in "includeif
hasconfig".
If you can think of a concise explanation, maybe we can include it.
+`hasconfig:remote.*.url:`::+ The data that follows this keyword is taken to+ be a pattern with standard globbing wildcards and two+ additional ones, `**/` and `/**`, that can match multiple+ components. The first time this keyword is seen, the rest of+ the config files will be scanned for remote URLs (without+ applying any values). If there exists at least one remote URL+ that matches this pattern, the include condition is met.+++Files included by this option (directly or indirectly) are not allowed+to contain remote URLs.
Wondering out loud.. Reading this and Ævar's comment [1], I wonder if we
should make it clear to users *why* we choose to forbid remote URLs.
Since this series is setting a precedent for future "hasconfig:"
conditions (files included by "hasconfig:foo.*.bar" cannot contain any
"foo.*.bar" values), it would be useful to git developers to explain
*why* we chose to do this. And if we're documenting it for ourselves,
we might as well write it in the public docs. That way, users would know
that this is more of a guardrail (because it's simpler to understand
this way) than a hard limitation.
[1] https://lore.kernel.org/git/211207.86k0ggnvfo.gmgdl@evledraar.gmail.com
The explanation is rather long, though. It goes something like this:
If the main config is:
[remote a]
url = bar
[includeif hasconfig:remote.*.url:foo]
path = foo
[includeif hasconfig:remote.*.url:bar]
path = bar
and "bar" contains:
[remote b]
url = foo
Should "foo" be included? For now, we avoid these situations
completely by prohibiting URLs from being configured in "includeif
hasconfig".
If you can think of a concise explanation, maybe we can include it.
Yeah, I can't think of a concise-yet-clear way to convey this to users
(if I had thought of one, I wouldn't have prefaced my original comment
with "Wondering out loud").
Spitballing here...
`hasconfig:remote.*.url:`::
The data that follows this keyword is taken to
be a pattern with standard globbing wildcards and two
additional ones, `**/` and `/**`, that can match multiple
components. The first time this keyword is seen, the rest of
the config files will be scanned for remote URLs (without
applying any values). If there exists at least one remote URL
that matches this pattern, the include condition is met.
- Files included by this option (directly or indirectly) are not allowed
- to contain remote URLs.
+ Because new remote URLs might affect the correctness of the include
+ condition, files included by this option (directly or indirectly) are
+ not allowed to contain remote URLs.
Although, upon further reflection, I wonder if this approach of banning
config variables really gives us the safety we want after all. Reworking
your example, say we expand "hasconfig" to include
"hasconfig:branch.*.merge" then we can have this in the main config:
[remote a]
url = baz
[branch c]
merge = bar
[includeif hasconfig:remote.*.url:foo]
path = foo
[includeif hasconfig:branch.*.merge:bar]
path = bar
and "bar" contains:
[remote b]
url = foo
we end up with the exact same question of "Should "foo" be included?".
This shows that the rule isn't actually "files included by
hasconfig:remote.*.url cannot include remote.*.url", but the much more
restrictive "files included by hasconfig:<anything> cannot include any
config values that can appear in hasconfig". This sounds pretty unusable
to me..
But I think that with the semantics you've defined, we don't really need
to forbid config variables. This section describes:
The first time this keyword is seen, the rest of the config files will
be scanned for remote URLs (without applying any values). If there
exists at least one remote URL that matches this pattern, the include
condition is met.
which, to me, gives us a pass to say "the first time we see a hasconfig,
we will do an additional scan without applying values". That doesn't
sound _too_ confusing to me, but I don't know how it looks to someone
with fresh eyes.
Forgive me if this exact suggestion came up before on-list (I know we've
discussed this exact approach off-list).
From: Jonathan Tan <hidden> Date: 2021-12-13 23:35:50
Glen Choo [off-list ref] writes:
Yeah, I can't think of a concise-yet-clear way to convey this to users
(if I had thought of one, I wouldn't have prefaced my original comment
with "Wondering out loud").
Spitballing here...
`hasconfig:remote.*.url:`::
The data that follows this keyword is taken to
be a pattern with standard globbing wildcards and two
additional ones, `**/` and `/**`, that can match multiple
components. The first time this keyword is seen, the rest of
the config files will be scanned for remote URLs (without
applying any values). If there exists at least one remote URL
that matches this pattern, the include condition is met.
- Files included by this option (directly or indirectly) are not allowed
- to contain remote URLs.
+ Because new remote URLs might affect the correctness of the include
+ condition, files included by this option (directly or indirectly) are
+ not allowed to contain remote URLs.
Although, upon further reflection, I wonder if this approach of banning
config variables really gives us the safety we want after all. Reworking
your example, say we expand "hasconfig" to include
"hasconfig:branch.*.merge" then we can have this in the main config:
[remote a]
url = baz
[branch c]
merge = bar
[includeif hasconfig:remote.*.url:foo]
path = foo
[includeif hasconfig:branch.*.merge:bar]
path = bar
and "bar" contains:
[remote b]
url = foo
we end up with the exact same question of "Should "foo" be included?".
This shows that the rule isn't actually "files included by
hasconfig:remote.*.url cannot include remote.*.url", but the much more
restrictive "files included by hasconfig:<anything> cannot include any
config values that can appear in hasconfig". This sounds pretty unusable
to me..
This was my original idea actually (using any config variable anywhere
bans you from that config variable in all "includeif hasconfig"). I
think it would still be usable - you just have to be careful in which
config variables you use. But we don't have plans to include other
variables now anyway.
But I think that with the semantics you've defined, we don't really need
to forbid config variables. This section describes:
The first time this keyword is seen, the rest of the config files will
be scanned for remote URLs (without applying any values). If there
exists at least one remote URL that matches this pattern, the include
condition is met.
which, to me, gives us a pass to say "the first time we see a hasconfig,
we will do an additional scan without applying values". That doesn't
sound _too_ confusing to me, but I don't know how it looks to someone
with fresh eyes.
Forgive me if this exact suggestion came up before on-list (I know we've
discussed this exact approach off-list).
This "additional scan without applying values" is not very well-defined,
though. In the scenario I described in [2], should "foo" be included?
"Yes" because it is referenced (even though at that time, nobody has
ever head of the URL "foo") or "no" because at that point in time in the
scan, nobody has ever heard of the URL "foo"?
[2] https://lore.kernel.org/git/20211209223919.513113-1-jonathantanmy@google.com/
From: Jonathan Tan <hidden> Date: 2021-12-14 21:31:57
Thanks, everyone, for your comments. I've followed Glen's code
suggestion and Junio's documentation suggestion, as you can see in the
range-diff.
Jonathan Tan (2):
config: make git_config_include() static
config: include file if remote URL matches a glob
Documentation/config.txt | 27 ++++++++
config.c | 132 ++++++++++++++++++++++++++++++++++++---
config.h | 46 ++++----------
t/t1300-config.sh | 118 ++++++++++++++++++++++++++++++++++
4 files changed, 282 insertions(+), 41 deletions(-)
Range-diff against v6:
1: b2dcae03ed = 1: b2dcae03ed config: make git_config_include() static
2: de2be06818 ! 2: 7c70089074 config: include file if remote URL matches a glob
@@ Documentation/config.txt: all branches that begin with `foo/`. This is useful if
+Files included by this option (directly or indirectly) are not allowed
+to contain remote URLs.
++
-+This keyword is designed to be forwards compatible with a naming
-+scheme that supports more variable-based include conditions, but
-+currently Git only supports the exact keyword described above.
++Note that unlike other includeIf conditions, resolving this condition
++relies on information that is not yet known at the point of reading the
++condition. A typical use case is this option being present as a
++system-level or global-level config, and the remote URL being in a
++local-level config; hence the need to scan ahead when resolving this
++condition. In order to avoid the chicken-and-egg problem in which
++potentially-included files can affect whether such files are potentially
++included, Git breaks the cycle by prohibiting these files from affecting
++the resolution of these conditions (thus, prohibiting them from
++declaring remote URLs).
+++
++As for the naming of this keyword, it is for forwards compatibiliy with
++a naming scheme that supports more variable-based include conditions,
++but currently Git only supports the exact keyword described above.
+
A few more notes on matching via `gitdir` and `gitdir/i`:
@@ config.c: static int git_config_include(const char *var, const char *value, void
+ if (inc->opts->unconditional_remote_url)
+ inc->fn = forbid_remote_url;
ret = handle_path_include(value, inc);
-+ if (inc->opts->unconditional_remote_url)
-+ inc->fn = old_fn;
++ inc->fn = old_fn;
+ }
return ret;
--
2.34.1.173.g76aa8bc2d0-goog
From: Jonathan Tan <hidden> Date: 2021-12-14 21:31:59
It is not used from outside the file in which it is declared.
Signed-off-by: Jonathan Tan <redacted>
---
config.c | 12 +++++++++++-
config.h | 37 ++++---------------------------------
2 files changed, 15 insertions(+), 34 deletions(-)
From: Jonathan Tan <hidden> Date: 2021-12-14 21:32:02
This is a feature that supports config file inclusion conditional on
whether the repo has a remote with a URL that matches a glob.
Similar to my previous work on remote-suggested hooks [1], the main
motivation is to allow remote repo administrators to provide recommended
configs in a way that can be consumed more easily (e.g. through a
package installable by a package manager - it could, for example,
contain a file to be included conditionally and a post-install script
that adds the include directive to the system-wide config file).
In order to do this, Git reruns the config parsing mechanism upon
noticing the first URL-conditional include in order to find all remote
URLs, and these remote URLs are then used to determine if that first and
all subsequent includes are executed. Remote URLs are not allowed to be
configued in any URL-conditionally-included file.
[1] https://lore.kernel.org/git/cover.1623881977.git.jonathantanmy@google.com/
Signed-off-by: Jonathan Tan <redacted>
---
Documentation/config.txt | 27 +++++++++
config.c | 120 ++++++++++++++++++++++++++++++++++++---
config.h | 9 +++
t/t1300-config.sh | 118 ++++++++++++++++++++++++++++++++++++++
4 files changed, 267 insertions(+), 7 deletions(-)
@@ -159,6 +159,33 @@ all branches that begin with `foo/`. This is useful if your branches are organized hierarchically and you would like to apply a configuration to all the branches in that hierarchy.+`hasconfig:remote.*.url:`::+ The data that follows this keyword is taken to+ be a pattern with standard globbing wildcards and two+ additional ones, `**/` and `/**`, that can match multiple+ components. The first time this keyword is seen, the rest of+ the config files will be scanned for remote URLs (without+ applying any values). If there exists at least one remote URL+ that matches this pattern, the include condition is met.+++Files included by this option (directly or indirectly) are not allowed+to contain remote URLs.+++Note that unlike other includeIf conditions, resolving this condition+relies on information that is not yet known at the point of reading the+condition. A typical use case is this option being present as a+system-level or global-level config, and the remote URL being in a+local-level config; hence the need to scan ahead when resolving this+condition. In order to avoid the chicken-and-egg problem in which+potentially-included files can affect whether such files are potentially+included, Git breaks the cycle by prohibiting these files from affecting+the resolution of these conditions (thus, prohibiting them from+declaring remote URLs).+++As for the naming of this keyword, it is for forwards compatibiliy with+a naming scheme that supports more variable-based include conditions,+but currently Git only supports the exact keyword described above.+ A few more notes on matching via `gitdir` and `gitdir/i`: * Symlinks in `$GIT_DIR` are not resolved before matching.
@@ -301,9 +307,92 @@ static int include_by_branch(const char *cond, size_t cond_len)returnret;}-staticintinclude_condition_is_true(conststructconfig_options*opts,+staticintadd_remote_url(constchar*var,constchar*value,void*data)+{+structstring_list*remote_urls=data;+constchar*remote_name;+size_tremote_name_len;+constchar*key;++if(!parse_config_key(var,"remote",&remote_name,&remote_name_len,+&key)&&+remote_name&&+!strcmp(key,"url"))+string_list_append(remote_urls,value);+return0;+}++staticvoidpopulate_remote_urls(structconfig_include_data*inc)+{+structconfig_optionsopts;++structconfig_source*store_cf=cf;+structkey_value_info*store_kvi=current_config_kvi;+enumconfig_scopestore_scope=current_parsing_scope;++opts=*inc->opts;+opts.unconditional_remote_url=1;++cf=NULL;+current_config_kvi=NULL;+current_parsing_scope=0;++inc->remote_urls=xmalloc(sizeof(*inc->remote_urls));+string_list_init_dup(inc->remote_urls);+config_with_options(add_remote_url,inc->remote_urls,inc->config_source,&opts);++cf=store_cf;+current_config_kvi=store_kvi;+current_parsing_scope=store_scope;+}++staticintforbid_remote_url(constchar*var,constchar*value,void*data)+{+constchar*remote_name;+size_tremote_name_len;+constchar*key;++if(!parse_config_key(var,"remote",&remote_name,&remote_name_len,+&key)&&+remote_name&&+!strcmp(key,"url"))+die(_("remote URLs cannot be configured in file directly or indirectly included by includeIf.hasconfig:remote.*.url"));+return0;+}++staticintat_least_one_url_matches_glob(constchar*glob,intglob_len,+structstring_list*remote_urls)+{+structstrbufpattern=STRBUF_INIT;+structstring_list_item*url_item;+intfound=0;++strbuf_add(&pattern,glob,glob_len);+for_each_string_list_item(url_item,remote_urls){+if(!wildmatch(pattern.buf,url_item->string,WM_PATHNAME)){+found=1;+break;+}+}+strbuf_release(&pattern);+returnfound;+}++staticintinclude_by_remote_url(structconfig_include_data*inc,+constchar*cond,size_tcond_len)+{+if(inc->opts->unconditional_remote_url)+return1;+if(!inc->remote_urls)+populate_remote_urls(inc);+returnat_least_one_url_matches_glob(cond,cond_len,+inc->remote_urls);+}++staticintinclude_condition_is_true(structconfig_include_data*inc,constchar*cond,size_tcond_len){+conststructconfig_options*opts=inc->opts;if(skip_prefix_mem(cond,cond_len,"gitdir:",&cond,&cond_len))returninclude_by_gitdir(opts,cond,cond_len,0);
@@ -311,6 +400,9 @@ static int include_condition_is_true(const struct config_options *opts,returninclude_by_gitdir(opts,cond,cond_len,1);elseif(skip_prefix_mem(cond,cond_len,"onbranch:",&cond,&cond_len))returninclude_by_branch(cond,cond_len);+elseif(skip_prefix_mem(cond,cond_len,"hasconfig:remote.*.url:",&cond,+&cond_len))+returninclude_by_remote_url(inc,cond,cond_len);/* unknown conditionals are always false */return0;
@@ -2387,4 +2387,122 @@ test_expect_success '--get and --get-all with --fixed-value' 'test_must_failgitconfig--file=config--get-regexp--fixed-valuefixed+non-existent'+test_expect_success'includeIf.hasconfig:remote.*.url''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++cat>include-this<<-\EOF&&+[user]+this=this-is-included+EOF+cat>dont-include-that<<-\EOF&&+[user]+that=that-is-not-included+EOF+cat>>hasremoteurlTest/.git/config<<-EOF&&+[includeIf"hasconfig:remote.*.url:foo"]+path="$(pwd)/include-this"+[includeIf"hasconfig:remote.*.url:bar"]+path="$(pwd)/dont-include-that"+[remote"foo"]+url=foo+EOF++echothis-is-included>expect-this&&+git-ChasremoteurlTestconfig--getuser.this>actual-this&&+test_cmpexpect-thisactual-this&&++test_must_failgit-ChasremoteurlTestconfig--getuser.that+'++test_expect_success'includeIf.hasconfig:remote.*.url respects last-config-wins''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++cat>include-two-three<<-\EOF&&+[user]+two=included-config+three=included-config+EOF+cat>>hasremoteurlTest/.git/config<<-EOF&&+[remote"foo"]+url=foo+[user]+one=main-config+two=main-config+[includeIf"hasconfig:remote.*.url:foo"]+path="$(pwd)/include-two-three"+[user]+three=main-config+EOF++echomain-config>expect-main-config&&+echoincluded-config>expect-included-config&&++git-ChasremoteurlTestconfig--getuser.one>actual&&+test_cmpexpect-main-configactual&&++git-ChasremoteurlTestconfig--getuser.two>actual&&+test_cmpexpect-included-configactual&&++git-ChasremoteurlTestconfig--getuser.three>actual&&+test_cmpexpect-main-configactual+'++test_expect_success'includeIf.hasconfig:remote.*.url globs''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++printf"[user]\ndss = yes\n">double-star-start&&+printf"[user]\ndse = yes\n">double-star-end&&+printf"[user]\ndsm = yes\n">double-star-middle&&+printf"[user]\nssm = yes\n">single-star-middle&&+printf"[user]\nno = no\n">no&&++cat>>hasremoteurlTest/.git/config<<-EOF&&+[remote"foo"]+url=https://foo/bar/baz+[includeIf"hasconfig:remote.*.url:**/baz"]+path="$(pwd)/double-star-start"+[includeIf"hasconfig:remote.*.url:**/nomatch"]+path="$(pwd)/no"+[includeIf"hasconfig:remote.*.url:https:/**"]+path="$(pwd)/double-star-end"+[includeIf"hasconfig:remote.*.url:nomatch:/**"]+path="$(pwd)/no"+[includeIf"hasconfig:remote.*.url:https:/**/baz"]+path="$(pwd)/double-star-middle"+[includeIf"hasconfig:remote.*.url:https:/**/nomatch"]+path="$(pwd)/no"+[includeIf"hasconfig:remote.*.url:https://*/bar/baz"]+path="$(pwd)/single-star-middle"+[includeIf"hasconfig:remote.*.url:https://*/baz"]+path="$(pwd)/no"+EOF++git-ChasremoteurlTestconfig--getuser.dss&&+git-ChasremoteurlTestconfig--getuser.dse&&+git-ChasremoteurlTestconfig--getuser.dsm&&+git-ChasremoteurlTestconfig--getuser.ssm&&+test_must_failgit-ChasremoteurlTestconfig--getuser.no+'++test_expect_success'includeIf.hasconfig:remote.*.url forbids remote url in such included files''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++cat>include-with-url<<-\EOF&&+[remote"bar"]+url=bar+EOF+cat>>hasremoteurlTest/.git/config<<-EOF&&+[includeIf"hasconfig:remote.*.url:foo"]+path="$(pwd)/include-with-url"+EOF++# test with any Git command+test_must_failgit-ChasremoteurlTeststatus2>err&&+grep"fatal: remote URLs cannot be configured in file directly or indirectly included by includeIf.hasconfig:remote.*.url"err+'+ test_done
@@ -159,6 +159,33 @@ all branches that begin with `foo/`. This is useful if your branches are organized hierarchically and you would like to apply a configuration to all the branches in that hierarchy.+`hasconfig:remote.*.url:`::+ The data that follows this keyword is taken to+ be a pattern with standard globbing wildcards and two+ additional ones, `**/` and `/**`, that can match multiple+ components. The first time this keyword is seen, the rest of+ the config files will be scanned for remote URLs (without+ applying any values). If there exists at least one remote URL+ that matches this pattern, the include condition is met.+++Files included by this option (directly or indirectly) are not allowed+to contain remote URLs.+++Note that unlike other includeIf conditions, resolving this condition+relies on information that is not yet known at the point of reading the+condition. A typical use case is this option being present as a+system-level or global-level config, and the remote URL being in a+local-level config; hence the need to scan ahead when resolving this+condition. In order to avoid the chicken-and-egg problem in which+potentially-included files can affect whether such files are potentially+included, Git breaks the cycle by prohibiting these files from affecting+the resolution of these conditions (thus, prohibiting them from+declaring remote URLs).
Putting myself in the shoes of someone who is unfamiliar with the
implementation, I think that this becomes clear if you read it enough
times (but also, I'm not a good reader), so this is ok.
It would be nice for this to be reviewed by someone who is _actually_
unfamiliar, though.
Thanks, everyone, for your comments. I've followed Glen's code
suggestion and Junio's documentation suggestion, as you can see in the
range-diff.
Jonathan Tan (2):
config: make git_config_include() static
config: include file if remote URL matches a glob
Documentation/config.txt | 27 ++++++++
config.c | 132 ++++++++++++++++++++++++++++++++++++---
config.h | 46 ++++----------
t/t1300-config.sh | 118 ++++++++++++++++++++++++++++++++++
4 files changed, 282 insertions(+), 41 deletions(-)
Range-diff against v6:
1: b2dcae03ed = 1: b2dcae03ed config: make git_config_include() static
2: de2be06818 ! 2: 7c70089074 config: include file if remote URL matches a glob
@@ Documentation/config.txt: all branches that begin with `foo/`. This is useful if
+Files included by this option (directly or indirectly) are not allowed
+to contain remote URLs.
++
-+This keyword is designed to be forwards compatible with a naming
-+scheme that supports more variable-based include conditions, but
-+currently Git only supports the exact keyword described above.
++Note that unlike other includeIf conditions, resolving this condition
++relies on information that is not yet known at the point of reading the
++condition. A typical use case is this option being present as a
++system-level or global-level config, and the remote URL being in a
++local-level config; hence the need to scan ahead when resolving this
++condition. In order to avoid the chicken-and-egg problem in which
++potentially-included files can affect whether such files are potentially
++included, Git breaks the cycle by prohibiting these files from affecting
++the resolution of these conditions (thus, prohibiting them from
++declaring remote URLs).
+++
++As for the naming of this keyword, it is for forwards compatibiliy with
++a naming scheme that supports more variable-based include conditions,
++but currently Git only supports the exact keyword described above.
+
A few more notes on matching via `gitdir` and `gitdir/i`:
@@ config.c: static int git_config_include(const char *var, const char *value, void
+ if (inc->opts->unconditional_remote_url)
+ inc->fn = forbid_remote_url;
ret = handle_path_include(value, inc);
-+ if (inc->opts->unconditional_remote_url)
-+ inc->fn = old_fn;
++ inc->fn = old_fn;
+ }
return ret;
--
2.34.1.173.g76aa8bc2d0-goog
The implementation looks good, and I think that the precedent we are
setting with "hasconfig:" is pretty well captured on this thread.
This looks good to me, though I'm not an expert in this area, so it
would be good for others to chime in.
Reviewed-by: Glen Choo <redacted>
On Wed, Dec 15, 2021 at 7:01 AM Jonathan Tan [off-list ref] wrote:
quoted hunk
This is a feature that supports config file inclusion conditional on
whether the repo has a remote with a URL that matches a glob.
Similar to my previous work on remote-suggested hooks [1], the main
motivation is to allow remote repo administrators to provide recommended
configs in a way that can be consumed more easily (e.g. through a
package installable by a package manager - it could, for example,
contain a file to be included conditionally and a post-install script
that adds the include directive to the system-wide config file).
In order to do this, Git reruns the config parsing mechanism upon
noticing the first URL-conditional include in order to find all remote
URLs, and these remote URLs are then used to determine if that first and
all subsequent includes are executed. Remote URLs are not allowed to be
configued in any URL-conditionally-included file.
[1] https://lore.kernel.org/git/cover.1623881977.git.jonathantanmy@google.com/
Signed-off-by: Jonathan Tan <redacted>
---
Documentation/config.txt | 27 +++++++++
config.c | 120 ++++++++++++++++++++++++++++++++++++---
config.h | 9 +++
t/t1300-config.sh | 118 ++++++++++++++++++++++++++++++++++++++
4 files changed, 267 insertions(+), 7 deletions(-)
@@ -159,6 +159,33 @@ all branches that begin with `foo/`. This is useful if your branches are organized hierarchically and you would like to apply a configuration to all the branches in that hierarchy.+`hasconfig:remote.*.url:`::+ The data that follows this keyword is taken to+ be a pattern with standard globbing wildcards and two+ additional ones, `**/` and `/**`, that can match multiple+ components. The first time this keyword is seen, the rest of+ the config files will be scanned for remote URLs (without+ applying any values). If there exists at least one remote URL+ that matches this pattern, the include condition is met.+++Files included by this option (directly or indirectly) are not allowed+to contain remote URLs.+++Note that unlike other includeIf conditions, resolving this condition+relies on information that is not yet known at the point of reading the+condition. A typical use case is this option being present as a+system-level or global-level config, and the remote URL being in a+local-level config; hence the need to scan ahead when resolving this+condition. In order to avoid the chicken-and-egg problem in which+potentially-included files can affect whether such files are potentially+included, Git breaks the cycle by prohibiting these files from affecting+the resolution of these conditions (thus, prohibiting them from+declaring remote URLs).+++As for the naming of this keyword, it is for forwards compatibiliy with+a naming scheme that supports more variable-based include conditions,+but currently Git only supports the exact keyword described above.+ A few more notes on matching via `gitdir` and `gitdir/i`: * Symlinks in `$GIT_DIR` are not resolved before matching.
@@ -301,9 +307,92 @@ static int include_by_branch(const char *cond, size_t cond_len)returnret;}-staticintinclude_condition_is_true(conststructconfig_options*opts,+staticintadd_remote_url(constchar*var,constchar*value,void*data)+{+structstring_list*remote_urls=data;+constchar*remote_name;+size_tremote_name_len;+constchar*key;++if(!parse_config_key(var,"remote",&remote_name,&remote_name_len,+&key)&&+remote_name&&+!strcmp(key,"url"))+string_list_append(remote_urls,value);+return0;+}++staticvoidpopulate_remote_urls(structconfig_include_data*inc)+{+structconfig_optionsopts;++structconfig_source*store_cf=cf;+structkey_value_info*store_kvi=current_config_kvi;+enumconfig_scopestore_scope=current_parsing_scope;++opts=*inc->opts;+opts.unconditional_remote_url=1;++cf=NULL;+current_config_kvi=NULL;+current_parsing_scope=0;++inc->remote_urls=xmalloc(sizeof(*inc->remote_urls));+string_list_init_dup(inc->remote_urls);+config_with_options(add_remote_url,inc->remote_urls,inc->config_source,&opts);++cf=store_cf;+current_config_kvi=store_kvi;+current_parsing_scope=store_scope;+}++staticintforbid_remote_url(constchar*var,constchar*value,void*data)+{+constchar*remote_name;+size_tremote_name_len;+constchar*key;++if(!parse_config_key(var,"remote",&remote_name,&remote_name_len,+&key)&&+remote_name&&+!strcmp(key,"url"))+die(_("remote URLs cannot be configured in file directly or indirectly included by includeIf.hasconfig:remote.*.url"));+return0;+}++staticintat_least_one_url_matches_glob(constchar*glob,intglob_len,+structstring_list*remote_urls)+{+structstrbufpattern=STRBUF_INIT;+structstring_list_item*url_item;+intfound=0;++strbuf_add(&pattern,glob,glob_len);+for_each_string_list_item(url_item,remote_urls){+if(!wildmatch(pattern.buf,url_item->string,WM_PATHNAME)){+found=1;+break;+}+}+strbuf_release(&pattern);+returnfound;+}++staticintinclude_by_remote_url(structconfig_include_data*inc,+constchar*cond,size_tcond_len)+{+if(inc->opts->unconditional_remote_url)+return1;+if(!inc->remote_urls)+populate_remote_urls(inc);+returnat_least_one_url_matches_glob(cond,cond_len,+inc->remote_urls);+}++staticintinclude_condition_is_true(structconfig_include_data*inc,constchar*cond,size_tcond_len){+conststructconfig_options*opts=inc->opts;if(skip_prefix_mem(cond,cond_len,"gitdir:",&cond,&cond_len))returninclude_by_gitdir(opts,cond,cond_len,0);
@@ -311,6 +400,9 @@ static int include_condition_is_true(const struct config_options *opts,returninclude_by_gitdir(opts,cond,cond_len,1);elseif(skip_prefix_mem(cond,cond_len,"onbranch:",&cond,&cond_len))returninclude_by_branch(cond,cond_len);+elseif(skip_prefix_mem(cond,cond_len,"hasconfig:remote.*.url:",&cond,+&cond_len))+returninclude_by_remote_url(inc,cond,cond_len);/* unknown conditionals are always false */return0;
@@ -2387,4 +2387,122 @@ test_expect_success '--get and --get-all with --fixed-value' 'test_must_failgitconfig--file=config--get-regexp--fixed-valuefixed+non-existent'+test_expect_success'includeIf.hasconfig:remote.*.url''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++cat>include-this<<-\EOF&&+[user]+this=this-is-included+EOF+cat>dont-include-that<<-\EOF&&+[user]+that=that-is-not-included+EOF+cat>>hasremoteurlTest/.git/config<<-EOF&&+[includeIf"hasconfig:remote.*.url:foo"]+path="$(pwd)/include-this"+[includeIf"hasconfig:remote.*.url:bar"]+path="$(pwd)/dont-include-that"+[remote"foo"]+url=foo
Which "foo" is relevant here? The remote name, or the url value?
Could they be given different values so that the testcase is a bit
easier to read and understand?
The testcases are very helpful. I found myself confused when reading
just the documentation about how it would be used. Perhaps an example
or two should be added to the documentation?
On Wed, Dec 15, 2021 at 7:25 AM Jonathan Tan [off-list ref] wrote:
Thanks, everyone, for your comments. I've followed Glen's code
suggestion and Junio's documentation suggestion, as you can see in the
range-diff.
So, the basic idea is, in a setting like Google's, you can have users
install additional files on their system out-of-band, and have the
users specify a simple line in their configuration to make use of
those additional files -- or portions thereof. It's a way of easily
providing potentially large blocks of pre-vetted configuration for
users.
Seems to make sense. (and I've read over the code lightly, so feel
free to take this as an Acked-by.)
But can I back up and comment on a bigger picture item?
This mechanism requires somehow getting additional files to the user
separately; projects that span companies (git.git, linux.git, etc.)
won't likely be able to make use of this.
Scalar also has a mechanism for providing potentially large blocks of
pre-vetted configuration for users. It does so as part of a new
top-level command. And it does so with a very opinionated set of
values that are not configurable. Thus, while I'd like to use it,
they use a configuration option that would break things badly at my
$DAYJOB. (Too many gradle plugins using jgit, which doesn't
understand index.version=4 and will blow up with a very suboptimal
error message when they see it.) And, it's very specific to scalar;
we probably don't want to add a new toplevel command everytime someone
wants common configuration to be easily grabbed by some user.
It would be nice if we could find some more generic solution.
Granted, I can't think of any, and I don't think this comment should
block this particular series (nor the scalar one), but I am worrying a
little bit that we're getting multiple completely different solutions
for the same general problem, and each brings caveats big enough to
preclude many (most?) potential users. I don't know what to do about
that, especially since configuration that is too easy to propagate
comes with big security problems, but I wanted to at least raise the
issue and hope others have good ideas. If nothing else, I want to
raise awareness to avoid proliferation of similar
pre-vetted-configuration-deployment mechanisms. I'm CC'ing a couple
scalar folks as well for that point.
But can I back up and comment on a bigger picture item?
This mechanism requires somehow getting additional files to the user
separately; projects that span companies (git.git, linux.git, etc.)
won't likely be able to make use of this.
Scalar also has a mechanism for providing potentially large blocks of
pre-vetted configuration for users. It does so as part of a new
top-level command. And it does so with a very opinionated set of
values that are not configurable. Thus, while I'd like to use it,
they use a configuration option that would break things badly at my
$DAYJOB. (Too many gradle plugins using jgit, which doesn't
understand index.version=4 and will blow up with a very suboptimal
error message when they see it.) And, it's very specific to scalar;
we probably don't want to add a new toplevel command everytime someone
wants common configuration to be easily grabbed by some user.
It would be nice if we could find some more generic solution.
Granted, I can't think of any, and I don't think this comment should
block this particular series (nor the scalar one), but I am worrying a
little bit that we're getting multiple completely different solutions
for the same general problem, and each brings caveats big enough to
preclude many (most?) potential users. I don't know what to do about
that, especially since configuration that is too easy to propagate
comes with big security problems, but I wanted to at least raise the
issue and hope others have good ideas. If nothing else, I want to
raise awareness to avoid proliferation of similar
pre-vetted-configuration-deployment mechanisms. I'm CC'ing a couple
scalar folks as well for that point.
Yes, that's an accurate description. To reiterate what Jonathan said in
his first cover letter [1], the primary motivation is that we want to be
able to 'suggest' hooks to users. There was an RFC for this
'remote-suggested hooks feature' (docs [2], RFC implementation [3]) but
it ultimately stalled due to security concerns I believe (this was
before I joined the team, so I'm not the most familiar with this).
It might be worth re-reading those threads since they tread on pretty
much the same ground of shipping pre-vetted config (this is directed at
me too, since I haven't read through those in detail). I've also been
told that we're (aka Google) still looking for feedback on [2], so feel
free to share any thoughts on that thread too.
[1] https://lore.kernel.org/git/cover.1634077795.git.jonathantanmy@google.com
[2] https://lore.kernel.org/git/pull.908.v4.git.1620241892929.gitgitgadget@gmail.com/
[3] https://lore.kernel.org/git/cover.1623881977.git.jonathantanmy@google.com/
Which "foo" is relevant here? The remote name, or the url value?
Could they be given different values so that the testcase is a bit
easier to read and understand?
Thanks for taking a look. Sorry for the late reply - I just got back
from vacation.
This is a good point - I'll change one of them.
The testcases are very helpful. I found myself confused when reading
just the documentation about how it would be used. Perhaps an example
or two should be added to the documentation?
Will do. I notice that there is a section with examples - I'll add it
there.
From: Jonathan Tan <hidden> Date: 2022-01-10 19:22:52
Elijah Newren [off-list ref] writes:
On Wed, Dec 15, 2021 at 7:25 AM Jonathan Tan [off-list ref] wrote:
quoted
Thanks, everyone, for your comments. I've followed Glen's code
suggestion and Junio's documentation suggestion, as you can see in the
range-diff.
So, the basic idea is, in a setting like Google's, you can have users
install additional files on their system out-of-band, and have the
users specify a simple line in their configuration to make use of
those additional files -- or portions thereof. It's a way of easily
providing potentially large blocks of pre-vetted configuration for
users.
Seems to make sense. (and I've read over the code lightly, so feel
free to take this as an Acked-by.)
Thanks.
But can I back up and comment on a bigger picture item?
This mechanism requires somehow getting additional files to the user
separately; projects that span companies (git.git, linux.git, etc.)
won't likely be able to make use of this.
Yes, they would also need to use a separate mechanism in addition to
Git.
Scalar also has a mechanism for providing potentially large blocks of
pre-vetted configuration for users. It does so as part of a new
top-level command. And it does so with a very opinionated set of
values that are not configurable. Thus, while I'd like to use it,
they use a configuration option that would break things badly at my
$DAYJOB. (Too many gradle plugins using jgit, which doesn't
understand index.version=4 and will blow up with a very suboptimal
error message when they see it.) And, it's very specific to scalar;
we probably don't want to add a new toplevel command everytime someone
wants common configuration to be easily grabbed by some user.
Do you have more information on this? The closest thing I've seen is
"Scalar Config" under "Modifying Configuration Values" in [1], which
seems to be more about bundling additional tools (which may change
config, of course).
Unless you're referring to the config bundled in the Scalar tool itself,
in which case this patch set seems orthogonal and potentially
complementary - I was envisioning config being provided by a package
manager package, but Scalar could provide some too for users to use at
their own discretion.
[1] https://github.com/microsoft/git/blob/7a514b4c2d5df7fdd2f66f048010d8ddcb412d0b/contrib/scalar/docs/troubleshooting.md
It would be nice if we could find some more generic solution.
Granted, I can't think of any, and I don't think this comment should
block this particular series (nor the scalar one), but I am worrying a
little bit that we're getting multiple completely different solutions
for the same general problem, and each brings caveats big enough to
preclude many (most?) potential users. I don't know what to do about
that, especially since configuration that is too easy to propagate
comes with big security problems, but I wanted to at least raise the
issue and hope others have good ideas. If nothing else, I want to
raise awareness to avoid proliferation of similar
pre-vetted-configuration-deployment mechanisms. I'm CC'ing a couple
scalar folks as well for that point.
That's a good point. As Glen said [2], it seems like transmitting config
itself (or, at least, hooks) through Git is something that we (the Git
project) don't want to do, so I have been working from the basis that
Git should just make use of config/hooks delivered through a non-Git
mechanism, and not deliver the config/hooks itself.
[2] https://lore.kernel.org/git/kl6lee5w5nng.fsf@chooglen-macbookpro.roam.corp.google.com/
On Mon, Jan 10, 2022 at 11:22 AM Jonathan Tan [off-list ref] wrote:
Elijah Newren [off-list ref] writes:
quoted
On Wed, Dec 15, 2021 at 7:25 AM Jonathan Tan [off-list ref] wrote:
quoted
Thanks, everyone, for your comments. I've followed Glen's code
suggestion and Junio's documentation suggestion, as you can see in the
range-diff.
So, the basic idea is, in a setting like Google's, you can have users
install additional files on their system out-of-band, and have the
users specify a simple line in their configuration to make use of
those additional files -- or portions thereof. It's a way of easily
providing potentially large blocks of pre-vetted configuration for
users.
Seems to make sense. (and I've read over the code lightly, so feel
free to take this as an Acked-by.)
Thanks.
quoted
But can I back up and comment on a bigger picture item?
This mechanism requires somehow getting additional files to the user
separately; projects that span companies (git.git, linux.git, etc.)
won't likely be able to make use of this.
Yes, they would also need to use a separate mechanism in addition to
Git.
quoted
Scalar also has a mechanism for providing potentially large blocks of
pre-vetted configuration for users. It does so as part of a new
top-level command. And it does so with a very opinionated set of
values that are not configurable. Thus, while I'd like to use it,
they use a configuration option that would break things badly at my
$DAYJOB. (Too many gradle plugins using jgit, which doesn't
understand index.version=4 and will blow up with a very suboptimal
error message when they see it.) And, it's very specific to scalar;
we probably don't want to add a new toplevel command everytime someone
wants common configuration to be easily grabbed by some user.
Do you have more information on this? The closest thing I've seen is
"Scalar Config" under "Modifying Configuration Values" in [1], which
seems to be more about bundling additional tools (which may change
config, of course).
Unless you're referring to the config bundled in the Scalar tool itself,
in which case this patch set seems orthogonal and potentially
complementary - I was envisioning config being provided by a package
manager package, but Scalar could provide some too for users to use at
their own discretion.
[1] https://github.com/microsoft/git/blob/7a514b4c2d5df7fdd2f66f048010d8ddcb412d0b/contrib/scalar/docs/troubleshooting.md
Yes, I was referring to the config hardcoded in the Scalar tool itself
(see set_recommended_config() in
https://lore.kernel.org/git/4439ab4de0bc3f48a6bdcf4b5165b16fad792ebd.1638538470.git.gitgitgadget@gmail.com/).
I agree they are different solutions to "help others setup config in a
pre-vetted way", that the two don't seem to conflict, and one can't be
implemented in terms of the other. It might even be possible for
someone somewhere to simultaneously take advantage of both (not sure
if anyone would try, but I don't forsee problems in doing so, except
in the narrow case that both schemes try to set the same config and
there are worries about which one "wins", which might boil down to
whether the include directive came first in the config file or the
specific config value that scalar set).
quoted
It would be nice if we could find some more generic solution.
Granted, I can't think of any, and I don't think this comment should
block this particular series (nor the scalar one), but I am worrying a
little bit that we're getting multiple completely different solutions
for the same general problem, and each brings caveats big enough to
preclude many (most?) potential users. I don't know what to do about
that, especially since configuration that is too easy to propagate
comes with big security problems, but I wanted to at least raise the
issue and hope others have good ideas. If nothing else, I want to
raise awareness to avoid proliferation of similar
pre-vetted-configuration-deployment mechanisms. I'm CC'ing a couple
scalar folks as well for that point.
That's a good point. As Glen said [2], it seems like transmitting config
itself (or, at least, hooks) through Git is something that we (the Git
project) don't want to do, so I have been working from the basis that
Git should just make use of config/hooks delivered through a non-Git
mechanism, and not deliver the config/hooks itself.
[2] https://lore.kernel.org/git/kl6lee5w5nng.fsf@chooglen-macbookpro.roam.corp.google.com/
Yeah, makes sense. And I don't know any better solutions. I guess
all I'm really saying is that if a third narrowly targetted way to
provide pre-vetted configuration shows up on the list, it may be time
to ask folks to step back and try to find a more generic solution.
From: Jonathan Tan <hidden> Date: 2022-01-18 17:47:46
Thanks everyone for your review. v8 follows Elijah Newren's comments
about making URLs more identifiable in tests, and also including an
example in documentation.
Jonathan Tan (2):
config: make git_config_include() static
config: include file if remote URL matches a glob
Documentation/config.txt | 35 +++++++++++
config.c | 132 ++++++++++++++++++++++++++++++++++++---
config.h | 46 ++++----------
t/t1300-config.sh | 118 ++++++++++++++++++++++++++++++++++
4 files changed, 290 insertions(+), 41 deletions(-)
Range-diff against v7:
1: b2dcae03ed = 1: b2dcae03ed config: make git_config_include() static
2: 7c70089074 ! 2: 6691e39c82 config: include file if remote URL matches a glob
@@ Documentation/config.txt: all branches that begin with `foo/`. This is useful if
A few more notes on matching via `gitdir` and `gitdir/i`:
* Symlinks in `$GIT_DIR` are not resolved before matching.
+@@ Documentation/config.txt: Example
+ ; currently checked out
+ [includeIf "onbranch:foo-branch"]
+ path = foo.inc
++
++; include only if a remote with the given URL exists (note
++; that such a URL may be provided later in a file or in a
++; file read after this file is read, as seen in this example)
++[includeIf "hasconfig:remote.*.url:https://example.com/**"]
++ path = foo.inc
++[remote "origin"]
++ url = https://example.com/git
+ ----
+
+ Values
## config.c ##
@@ config.c: struct config_include_data {
@@ t/t1300-config.sh: test_expect_success '--get and --get-all with --fixed-value'
+ that = that-is-not-included
+ EOF
+ cat >>hasremoteurlTest/.git/config <<-EOF &&
-+ [includeIf "hasconfig:remote.*.url:foo"]
++ [includeIf "hasconfig:remote.*.url:foourl"]
+ path = "$(pwd)/include-this"
-+ [includeIf "hasconfig:remote.*.url:bar"]
++ [includeIf "hasconfig:remote.*.url:barurl"]
+ path = "$(pwd)/dont-include-that"
+ [remote "foo"]
-+ url = foo
++ url = foourl
+ EOF
+
+ echo this-is-included >expect-this &&
@@ t/t1300-config.sh: test_expect_success '--get and --get-all with --fixed-value'
+ EOF
+ cat >>hasremoteurlTest/.git/config <<-EOF &&
+ [remote "foo"]
-+ url = foo
++ url = foourl
+ [user]
+ one = main-config
+ two = main-config
-+ [includeIf "hasconfig:remote.*.url:foo"]
++ [includeIf "hasconfig:remote.*.url:foourl"]
+ path = "$(pwd)/include-two-three"
+ [user]
+ three = main-config
@@ t/t1300-config.sh: test_expect_success '--get and --get-all with --fixed-value'
+
+ cat >include-with-url <<-\EOF &&
+ [remote "bar"]
-+ url = bar
++ url = barurl
+ EOF
+ cat >>hasremoteurlTest/.git/config <<-EOF &&
-+ [includeIf "hasconfig:remote.*.url:foo"]
++ [includeIf "hasconfig:remote.*.url:foourl"]
+ path = "$(pwd)/include-with-url"
+ EOF
+
--
2.34.1.703.g22d0c6ccf7-goog
From: Jonathan Tan <hidden> Date: 2022-01-18 17:47:48
It is not used from outside the file in which it is declared.
Signed-off-by: Jonathan Tan <redacted>
---
config.c | 12 +++++++++++-
config.h | 37 ++++---------------------------------
2 files changed, 15 insertions(+), 34 deletions(-)
From: Jonathan Tan <hidden> Date: 2022-01-18 17:47:50
This is a feature that supports config file inclusion conditional on
whether the repo has a remote with a URL that matches a glob.
Similar to my previous work on remote-suggested hooks [1], the main
motivation is to allow remote repo administrators to provide recommended
configs in a way that can be consumed more easily (e.g. through a
package installable by a package manager - it could, for example,
contain a file to be included conditionally and a post-install script
that adds the include directive to the system-wide config file).
In order to do this, Git reruns the config parsing mechanism upon
noticing the first URL-conditional include in order to find all remote
URLs, and these remote URLs are then used to determine if that first and
all subsequent includes are executed. Remote URLs are not allowed to be
configued in any URL-conditionally-included file.
[1] https://lore.kernel.org/git/cover.1623881977.git.jonathantanmy@google.com/
Signed-off-by: Jonathan Tan <redacted>
---
Documentation/config.txt | 35 ++++++++++++
config.c | 120 ++++++++++++++++++++++++++++++++++++---
config.h | 9 +++
t/t1300-config.sh | 118 ++++++++++++++++++++++++++++++++++++++
4 files changed, 275 insertions(+), 7 deletions(-)
@@ -159,6 +159,33 @@ all branches that begin with `foo/`. This is useful if your branches are organized hierarchically and you would like to apply a configuration to all the branches in that hierarchy.+`hasconfig:remote.*.url:`::+ The data that follows this keyword is taken to+ be a pattern with standard globbing wildcards and two+ additional ones, `**/` and `/**`, that can match multiple+ components. The first time this keyword is seen, the rest of+ the config files will be scanned for remote URLs (without+ applying any values). If there exists at least one remote URL+ that matches this pattern, the include condition is met.+++Files included by this option (directly or indirectly) are not allowed+to contain remote URLs.+++Note that unlike other includeIf conditions, resolving this condition+relies on information that is not yet known at the point of reading the+condition. A typical use case is this option being present as a+system-level or global-level config, and the remote URL being in a+local-level config; hence the need to scan ahead when resolving this+condition. In order to avoid the chicken-and-egg problem in which+potentially-included files can affect whether such files are potentially+included, Git breaks the cycle by prohibiting these files from affecting+the resolution of these conditions (thus, prohibiting them from+declaring remote URLs).+++As for the naming of this keyword, it is for forwards compatibiliy with+a naming scheme that supports more variable-based include conditions,+but currently Git only supports the exact keyword described above.+ A few more notes on matching via `gitdir` and `gitdir/i`: * Symlinks in `$GIT_DIR` are not resolved before matching.
@@ -226,6 +253,14 @@ Example ; currently checked out [includeIf "onbranch:foo-branch"] path = foo.inc++; include only if a remote with the given URL exists (note+; that such a URL may be provided later in a file or in a+; file read after this file is read, as seen in this example)+[includeIf "hasconfig:remote.*.url:https://example.com/**"]+ path = foo.inc+[remote "origin"]+ url = https://example.com/git ---- Values
@@ -301,9 +307,92 @@ static int include_by_branch(const char *cond, size_t cond_len)returnret;}-staticintinclude_condition_is_true(conststructconfig_options*opts,+staticintadd_remote_url(constchar*var,constchar*value,void*data)+{+structstring_list*remote_urls=data;+constchar*remote_name;+size_tremote_name_len;+constchar*key;++if(!parse_config_key(var,"remote",&remote_name,&remote_name_len,+&key)&&+remote_name&&+!strcmp(key,"url"))+string_list_append(remote_urls,value);+return0;+}++staticvoidpopulate_remote_urls(structconfig_include_data*inc)+{+structconfig_optionsopts;++structconfig_source*store_cf=cf;+structkey_value_info*store_kvi=current_config_kvi;+enumconfig_scopestore_scope=current_parsing_scope;++opts=*inc->opts;+opts.unconditional_remote_url=1;++cf=NULL;+current_config_kvi=NULL;+current_parsing_scope=0;++inc->remote_urls=xmalloc(sizeof(*inc->remote_urls));+string_list_init_dup(inc->remote_urls);+config_with_options(add_remote_url,inc->remote_urls,inc->config_source,&opts);++cf=store_cf;+current_config_kvi=store_kvi;+current_parsing_scope=store_scope;+}++staticintforbid_remote_url(constchar*var,constchar*value,void*data)+{+constchar*remote_name;+size_tremote_name_len;+constchar*key;++if(!parse_config_key(var,"remote",&remote_name,&remote_name_len,+&key)&&+remote_name&&+!strcmp(key,"url"))+die(_("remote URLs cannot be configured in file directly or indirectly included by includeIf.hasconfig:remote.*.url"));+return0;+}++staticintat_least_one_url_matches_glob(constchar*glob,intglob_len,+structstring_list*remote_urls)+{+structstrbufpattern=STRBUF_INIT;+structstring_list_item*url_item;+intfound=0;++strbuf_add(&pattern,glob,glob_len);+for_each_string_list_item(url_item,remote_urls){+if(!wildmatch(pattern.buf,url_item->string,WM_PATHNAME)){+found=1;+break;+}+}+strbuf_release(&pattern);+returnfound;+}++staticintinclude_by_remote_url(structconfig_include_data*inc,+constchar*cond,size_tcond_len)+{+if(inc->opts->unconditional_remote_url)+return1;+if(!inc->remote_urls)+populate_remote_urls(inc);+returnat_least_one_url_matches_glob(cond,cond_len,+inc->remote_urls);+}++staticintinclude_condition_is_true(structconfig_include_data*inc,constchar*cond,size_tcond_len){+conststructconfig_options*opts=inc->opts;if(skip_prefix_mem(cond,cond_len,"gitdir:",&cond,&cond_len))returninclude_by_gitdir(opts,cond,cond_len,0);
@@ -311,6 +400,9 @@ static int include_condition_is_true(const struct config_options *opts,returninclude_by_gitdir(opts,cond,cond_len,1);elseif(skip_prefix_mem(cond,cond_len,"onbranch:",&cond,&cond_len))returninclude_by_branch(cond,cond_len);+elseif(skip_prefix_mem(cond,cond_len,"hasconfig:remote.*.url:",&cond,+&cond_len))+returninclude_by_remote_url(inc,cond,cond_len);/* unknown conditionals are always false */return0;
@@ -2387,4 +2387,122 @@ test_expect_success '--get and --get-all with --fixed-value' 'test_must_failgitconfig--file=config--get-regexp--fixed-valuefixed+non-existent'+test_expect_success'includeIf.hasconfig:remote.*.url''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++cat>include-this<<-\EOF&&+[user]+this=this-is-included+EOF+cat>dont-include-that<<-\EOF&&+[user]+that=that-is-not-included+EOF+cat>>hasremoteurlTest/.git/config<<-EOF&&+[includeIf"hasconfig:remote.*.url:foourl"]+path="$(pwd)/include-this"+[includeIf"hasconfig:remote.*.url:barurl"]+path="$(pwd)/dont-include-that"+[remote"foo"]+url=foourl+EOF++echothis-is-included>expect-this&&+git-ChasremoteurlTestconfig--getuser.this>actual-this&&+test_cmpexpect-thisactual-this&&++test_must_failgit-ChasremoteurlTestconfig--getuser.that+'++test_expect_success'includeIf.hasconfig:remote.*.url respects last-config-wins''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++cat>include-two-three<<-\EOF&&+[user]+two=included-config+three=included-config+EOF+cat>>hasremoteurlTest/.git/config<<-EOF&&+[remote"foo"]+url=foourl+[user]+one=main-config+two=main-config+[includeIf"hasconfig:remote.*.url:foourl"]+path="$(pwd)/include-two-three"+[user]+three=main-config+EOF++echomain-config>expect-main-config&&+echoincluded-config>expect-included-config&&++git-ChasremoteurlTestconfig--getuser.one>actual&&+test_cmpexpect-main-configactual&&++git-ChasremoteurlTestconfig--getuser.two>actual&&+test_cmpexpect-included-configactual&&++git-ChasremoteurlTestconfig--getuser.three>actual&&+test_cmpexpect-main-configactual+'++test_expect_success'includeIf.hasconfig:remote.*.url globs''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++printf"[user]\ndss = yes\n">double-star-start&&+printf"[user]\ndse = yes\n">double-star-end&&+printf"[user]\ndsm = yes\n">double-star-middle&&+printf"[user]\nssm = yes\n">single-star-middle&&+printf"[user]\nno = no\n">no&&++cat>>hasremoteurlTest/.git/config<<-EOF&&+[remote"foo"]+url=https://foo/bar/baz+[includeIf"hasconfig:remote.*.url:**/baz"]+path="$(pwd)/double-star-start"+[includeIf"hasconfig:remote.*.url:**/nomatch"]+path="$(pwd)/no"+[includeIf"hasconfig:remote.*.url:https:/**"]+path="$(pwd)/double-star-end"+[includeIf"hasconfig:remote.*.url:nomatch:/**"]+path="$(pwd)/no"+[includeIf"hasconfig:remote.*.url:https:/**/baz"]+path="$(pwd)/double-star-middle"+[includeIf"hasconfig:remote.*.url:https:/**/nomatch"]+path="$(pwd)/no"+[includeIf"hasconfig:remote.*.url:https://*/bar/baz"]+path="$(pwd)/single-star-middle"+[includeIf"hasconfig:remote.*.url:https://*/baz"]+path="$(pwd)/no"+EOF++git-ChasremoteurlTestconfig--getuser.dss&&+git-ChasremoteurlTestconfig--getuser.dse&&+git-ChasremoteurlTestconfig--getuser.dsm&&+git-ChasremoteurlTestconfig--getuser.ssm&&+test_must_failgit-ChasremoteurlTestconfig--getuser.no+'++test_expect_success'includeIf.hasconfig:remote.*.url forbids remote url in such included files''+gitinithasremoteurlTest&&+test_when_finished"rm -rf hasremoteurlTest"&&++cat>include-with-url<<-\EOF&&+[remote"bar"]+url=barurl+EOF+cat>>hasremoteurlTest/.git/config<<-EOF&&+[includeIf"hasconfig:remote.*.url:foourl"]+path="$(pwd)/include-with-url"+EOF++# test with any Git command+test_must_failgit-ChasremoteurlTeststatus2>err&&+grep"fatal: remote URLs cannot be configured in file directly or indirectly included by includeIf.hasconfig:remote.*.url"err+'+ test_done
On Tue, Jan 18, 2022 at 9:47 AM Jonathan Tan [off-list ref] wrote:
Thanks everyone for your review. v8 follows Elijah Newren's comments
about making URLs more identifiable in tests, and also including an
example in documentation.
Thanks for that; this version looks good to me.
Jonathan Tan (2):
config: make git_config_include() static
config: include file if remote URL matches a glob
Documentation/config.txt | 35 +++++++++++
config.c | 132 ++++++++++++++++++++++++++++++++++++---
config.h | 46 ++++----------
t/t1300-config.sh | 118 ++++++++++++++++++++++++++++++++++
4 files changed, 290 insertions(+), 41 deletions(-)
Range-diff against v7:
1: b2dcae03ed = 1: b2dcae03ed config: make git_config_include() static
2: 7c70089074 ! 2: 6691e39c82 config: include file if remote URL matches a glob
@@ Documentation/config.txt: all branches that begin with `foo/`. This is useful if
A few more notes on matching via `gitdir` and `gitdir/i`:
* Symlinks in `$GIT_DIR` are not resolved before matching.
+@@ Documentation/config.txt: Example
+ ; currently checked out
+ [includeIf "onbranch:foo-branch"]
+ path = foo.inc
++
++; include only if a remote with the given URL exists (note
++; that such a URL may be provided later in a file or in a
++; file read after this file is read, as seen in this example)
++[includeIf "hasconfig:remote.*.url:https://example.com/**"]
++ path = foo.inc
++[remote "origin"]
++ url = https://example.com/git
+ ----
+
+ Values
## config.c ##
@@ config.c: struct config_include_data {
@@ t/t1300-config.sh: test_expect_success '--get and --get-all with --fixed-value'
+ that = that-is-not-included
+ EOF
+ cat >>hasremoteurlTest/.git/config <<-EOF &&
-+ [includeIf "hasconfig:remote.*.url:foo"]
++ [includeIf "hasconfig:remote.*.url:foourl"]
+ path = "$(pwd)/include-this"
-+ [includeIf "hasconfig:remote.*.url:bar"]
++ [includeIf "hasconfig:remote.*.url:barurl"]
+ path = "$(pwd)/dont-include-that"
+ [remote "foo"]
-+ url = foo
++ url = foourl
+ EOF
+
+ echo this-is-included >expect-this &&
@@ t/t1300-config.sh: test_expect_success '--get and --get-all with --fixed-value'
+ EOF
+ cat >>hasremoteurlTest/.git/config <<-EOF &&
+ [remote "foo"]
-+ url = foo
++ url = foourl
+ [user]
+ one = main-config
+ two = main-config
-+ [includeIf "hasconfig:remote.*.url:foo"]
++ [includeIf "hasconfig:remote.*.url:foourl"]
+ path = "$(pwd)/include-two-three"
+ [user]
+ three = main-config
@@ t/t1300-config.sh: test_expect_success '--get and --get-all with --fixed-value'
+
+ cat >include-with-url <<-\EOF &&
+ [remote "bar"]
-+ url = bar
++ url = barurl
+ EOF
+ cat >>hasremoteurlTest/.git/config <<-EOF &&
-+ [includeIf "hasconfig:remote.*.url:foo"]
++ [includeIf "hasconfig:remote.*.url:foourl"]
+ path = "$(pwd)/include-with-url"
+ EOF
+
--
2.34.1.703.g22d0c6ccf7-goog
From: Johannes Schindelin <hidden> Date: 2022-01-25 13:29:04
Hi Elijah,
On Mon, 10 Jan 2022, Elijah Newren wrote:
On Mon, Jan 10, 2022 at 11:22 AM Jonathan Tan [off-list ref] wrote:
quoted
Elijah Newren [off-list ref] writes:
quoted
Scalar also has a mechanism for providing potentially large blocks
of pre-vetted configuration for users. It does so as part of a new
top-level command. And it does so with a very opinionated set of
values that are not configurable. Thus, while I'd like to use it,
they use a configuration option that would break things badly at my
$DAYJOB. (Too many gradle plugins using jgit, which doesn't
understand index.version=4 and will blow up with a very suboptimal
error message when they see it.) And, it's very specific to scalar;
we probably don't want to add a new toplevel command everytime
someone wants common configuration to be easily grabbed by some
user.
Do you have more information on this? The closest thing I've seen is
"Scalar Config" under "Modifying Configuration Values" in [1], which
seems to be more about bundling additional tools (which may change
config, of course).
Unless you're referring to the config bundled in the Scalar tool itself,
in which case this patch set seems orthogonal and potentially
complementary - I was envisioning config being provided by a package
manager package, but Scalar could provide some too for users to use at
their own discretion.
[1] https://github.com/microsoft/git/blob/7a514b4c2d5df7fdd2f66f048010d8ddcb412d0b/contrib/scalar/docs/troubleshooting.md
I was kind of thinking that such problems might be solved via introducing
e.g. `scalar.ensureJGitCompatibility = true` (which should be a relatively
trivial patch to write).
What do you think?
Ciao,
Dscho