git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: peff@peff.net
Cc: jonathantanmy@google.com, git@vger.kernel.org, gitster@pobox.com
Subject: Re: [RFC PATCH 0/2] Conditional config includes based on remote URL
Date: Wed, 27 Oct 2021 10:52:59 -0700	[thread overview]
Message-ID: <20211027175259.2230232-1-jonathantanmy@google.com> (raw)
In-Reply-To: <YXk+TRnndNZkdsGF@coredump.intra.peff.net>

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

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.

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

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

Well, let me finish up (2), and let's see.

  reply	other threads:[~2021-10-27 17:53 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 [this message]
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
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=20211027175259.2230232-1-jonathantanmy@google.com \
    --to=jonathantanmy@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /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).