Re: [RFC PATCH 2/2] config: include file if remote URL matches a glob

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