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

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.