git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: chooglen@google.com
Cc: jonathantanmy@google.com, git@vger.kernel.org, avarab@gmail.com
Subject: Re: [PATCH v6 2/2] config: include file if remote URL matches a glob
Date: Mon, 13 Dec 2021 15:35:46 -0800	[thread overview]
Message-ID: <20211213233546.773716-1-jonathantanmy@google.com> (raw)
In-Reply-To: <kl6l8rwt9ww2.fsf@chooglen-macbookpro.roam.corp.google.com>

Glen Choo <chooglen@google.com> 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.

Junio suggested another approach [1] - I'll try that and see what I come
up with.

[1] https://lore.kernel.org/git/xmqqmtl8m8wj.fsf@gitster.g/

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

  reply	other threads:[~2021-12-13 23:35 UTC|newest]

Thread overview: 87+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-12 22:57 [RFC PATCH 0/2] Conditional config includes based on remote URL Jonathan Tan
2021-10-12 22:57 ` [RFC PATCH 1/2] config: make git_config_include() static Jonathan Tan
2021-10-12 23:07   ` Jeff King
2021-10-12 23:26   ` Junio C Hamano
2021-10-13  8:26   ` Ævar Arnfjörð Bjarmason
2021-10-13 17:00     ` Junio C Hamano
2021-10-13 18:13       ` Jonathan Tan
2021-10-12 22:57 ` [RFC PATCH 2/2] config: include file if remote URL matches a glob Jonathan Tan
2021-10-12 23:30   ` Jeff King
2021-10-13 18:33     ` Jonathan Tan
2021-10-27 11:40       ` Jeff King
2021-10-27 17:23         ` Jonathan Tan
2021-10-12 23:48   ` Junio C Hamano
2021-10-13 19:52     ` Jonathan Tan
2021-10-13  0:46 ` [RFC PATCH 0/2] Conditional config includes based on remote URL brian m. carlson
2021-10-13 18:17   ` Jonathan Tan
2021-10-18 20:48 ` Jonathan Tan
2021-10-22  3:12   ` Emily Shaffer
2021-10-27 11:55   ` Jeff King
2021-10-27 17:52     ` Jonathan Tan
2021-10-27 20:32       ` Jeff King
2021-10-25 13:03 ` Ævar Arnfjörð Bjarmason
2021-10-25 18:53   ` Jonathan Tan
2021-10-26 10:12     ` Ævar Arnfjörð Bjarmason
2021-10-29 17:31 ` [WIP v2 " Jonathan Tan
2021-10-29 17:31   ` [WIP v2 1/2] config: make git_config_include() static Jonathan Tan
2021-11-05 19:45     ` Emily Shaffer
2021-10-29 17:31   ` [WIP v2 2/2] config: include file if remote URL matches a glob Jonathan Tan
2021-11-05 20:24     ` Emily Shaffer
2021-11-06  4:41       ` Ævar Arnfjörð Bjarmason
2021-11-09  0:25         ` Jonathan Tan
2021-11-09  0:22       ` Jonathan Tan
2021-11-16  0:00 ` [PATCH v3 0/2] Conditional config includes based on remote URL Jonathan Tan
2021-11-16  0:00   ` [PATCH v3 1/2] config: make git_config_include() static Jonathan Tan
2021-11-16  0:00   ` [PATCH v3 2/2] config: include file if remote URL matches a glob Jonathan Tan
2021-11-22 22:59     ` Glen Choo
2021-11-29 17:53       ` Jonathan Tan
2021-11-23  1:22     ` Junio C Hamano
2021-11-29 18:18       ` Jonathan Tan
2021-12-01 18:51         ` Junio C Hamano
2021-12-02 23:14           ` Jonathan Tan
2021-11-23  1:27     ` Ævar Arnfjörð Bjarmason
2021-11-29 18:33       ` Jonathan Tan
2021-11-29 20:50         ` Ævar Arnfjörð Bjarmason
2021-11-29 20:23 ` [PATCH v4 0/2] Conditional config includes based on remote URL Jonathan Tan
2021-11-29 20:23   ` [PATCH v4 1/2] config: make git_config_include() static Jonathan Tan
2021-11-29 20:23   ` [PATCH v4 2/2] config: include file if remote URL matches a glob Jonathan Tan
2021-12-02  6:57     ` Junio C Hamano
2021-12-02 17:41       ` Jonathan Tan
2021-11-29 20:48   ` [PATCH v4 0/2] Conditional config includes based on remote URL Ævar Arnfjörð Bjarmason
2021-11-30  7:51     ` Junio C Hamano
2021-12-02 23:31 ` [PATCH v5 " Jonathan Tan
2021-12-02 23:31   ` [PATCH v5 1/2] config: make git_config_include() static Jonathan Tan
2021-12-02 23:31   ` [PATCH v5 2/2] config: include file if remote URL matches a glob Jonathan Tan
2021-12-06 22:32     ` Glen Choo
2021-12-07 17:53       ` Jonathan Tan
2021-12-06 18:57   ` [PATCH v5 0/2] Conditional config includes based on remote URL Ævar Arnfjörð Bjarmason
2021-12-07 17:46     ` Jonathan Tan
2021-12-07 17:56       ` Ævar Arnfjörð Bjarmason
2021-12-07 18:52         ` Jonathan Tan
2021-12-07 23:23 ` [PATCH v6 " Jonathan Tan
2021-12-07 23:23   ` [PATCH v6 1/2] config: make git_config_include() static Jonathan Tan
2021-12-07 23:23   ` [PATCH v6 2/2] config: include file if remote URL matches a glob Jonathan Tan
2021-12-08 19:19     ` Glen Choo
2021-12-09 22:16       ` Jonathan Tan
2021-12-08 19:55     ` Glen Choo
2021-12-09 22:39       ` Jonathan Tan
2021-12-09 23:33         ` Glen Choo
2021-12-13 23:35           ` Jonathan Tan [this message]
2021-12-10 21:45         ` Junio C Hamano
2021-12-13 23:37           ` Jonathan Tan
2021-12-14 21:31 ` [PATCH v7 0/2] Conditional config includes based on remote URL Jonathan Tan
2021-12-14 21:31   ` [PATCH v7 1/2] config: make git_config_include() static Jonathan Tan
2021-12-14 21:31   ` [PATCH v7 2/2] config: include file if remote URL matches a glob Jonathan Tan
2021-12-16 21:54     ` Glen Choo
2021-12-28  0:55     ` Elijah Newren
2022-01-10 18:58       ` Jonathan Tan
2021-12-16 21:57   ` [PATCH v7 0/2] Conditional config includes based on remote URL Glen Choo
2021-12-28  1:13   ` Elijah Newren
2021-12-28 23:13     ` Glen Choo
2022-01-10 19:22     ` Jonathan Tan
2022-01-10 20:17       ` Elijah Newren
2022-01-25 13:26         ` Scalar vs JGit, was " Johannes Schindelin
2022-01-18 17:47 ` [PATCH v8 " Jonathan Tan
2022-01-18 17:47   ` [PATCH v8 1/2] config: make git_config_include() static Jonathan Tan
2022-01-18 17:47   ` [PATCH v8 2/2] config: include file if remote URL matches a glob Jonathan Tan
2022-01-18 20:54   ` [PATCH v8 0/2] Conditional config includes based on remote URL Elijah Newren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211213233546.773716-1-jonathantanmy@google.com \
    --to=jonathantanmy@google.com \
    --cc=avarab@gmail.com \
    --cc=chooglen@google.com \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).