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

[Jonathan Tan <jonathantanmy@google.com>]

Glen Choo <chooglen@google.com> writes:
> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > +`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.

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