git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: avarab@gmail.com
Cc: jonathantanmy@google.com, git@vger.kernel.org,
	chooglen@google.com, gitster@pobox.com
Subject: Re: [PATCH v5 0/2] Conditional config includes based on remote URL
Date: Tue,  7 Dec 2021 10:52:56 -0800	[thread overview]
Message-ID: <20211207185256.2305093-1-jonathantanmy@google.com> (raw)
In-Reply-To: <211207.86k0ggnvfo.gmgdl@evledraar.gmail.com>

> >>  * Inlining the "remote_urls" in the struct makes its management easier;
> >>    and the free/NULL checks just check .nr now, and string_list_clear() can be
> >>    unconditional.
> >
> > I don't think we can do this - nr might still be 0 after a scan if we
> > don't have remote URLs for some reason, so we still need to distinguish
> > between not-scanned and scanned-with-zero-URLs.
> 
> You mean so that we don't double-free? The way string_list_clear()
> protects against that, but maybe there's something else.
> 
> Whatever it is (if there's anything) it could use test coverage then :)

No - we only want to do one scan per config read. If we scan and there
are no remote URLs, with your scheme, next time we encounter another
includeIf.hasconfig, we would need to scan again (because nr is still
0). With my scheme, we can see that the pointer is non-NULL, so we know
that we have already scanned.

> >>  * It would be nice if e.g. the "includeIf.hasconfig:remote.*.url globs" test
> >>    were split up by condition, but maybe that's a hassle (would need a small helper).
> >> 
> >>    Just something that would have helped while hacking on this, i.e. now most of it
> >>    was an all-or-nothing failure & peek at the trace output
> >
> > What do you mean by condition? There seems to only be one condition
> > (whether the URL is there or not), unless you were thinking of smaller
> > subdivisions.
> 
> Maybe I'm just misunderstanding the intent here, but aren't you trying
> to guard against the case of having a ~/.gitconfig that includes
> ~/.gitconfig.d/for-this-url, and *that* file in turns changes the
> remote's "url" in its config, followed by another "include if url
> matches" condition therein?
> 
> I.e. I read (more like skimmed) the documentation & test at the end as
> forbidding that, but maybe that's OK?

If we're including "~/.gitconfig.d/for-this-url" by includeIf.hasconfig,
then yes, I'm guarding against that and other similar conditions.

> > We can decide later what the future facility will be, but I envision
> > that we will not allow included files to set config that can affect any
> > include directives in use. So, for example, if I have a user.email-based
> > include, none of my config-conditionally included files can set user.email.
> 
> I didn't look deeply at the implementation at all, but why would this be
> a problem?
> 
> You parse ~/.gitconfig, it has user.name=foo, then right after in that
> file we do:
> 
>     [includeIf "hasconfig:user.name:*foo*"]
>     path = ~/.gitconfig.d/foo
> 
> Now the top of  ~/.gitconfig.d/foo we have:
> 
>     [user]
>     name = bar
>     [includeIf "hasconfig:user.name:*bar*"]
>     path = ~/.gitconfig.d/bar
> 
> Why would it matter that we included on user.name=foo before?
> 
> Doesn't that only matter *while* we process that first "path" line? Once
> we move past it we update our configset to user.name=bar once we hit the
> "name" line of the included file.
> 
> Then when we get another "hasconfig:user.name" we just match it to our
> current user.name=*bar*.
> 
> No?
> 
> Anyway, I think it's fine to punt on it for now or whatever, just
> curious...

Well, we can't punt on it because what you describe also applies to
remote URL :-)

So what you're saying is that once we have decided to include a file, we
always include it in its entirety regardless of whether the condition
changes during the file's include. That's reasonable, but other people
could have differing opinions. In this case, I think it's fine just to
prohibit it entirely. In the future, we may look into relaxing this
condition.

  reply	other threads:[~2021-12-07 18: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
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 [this message]
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=20211207185256.2305093-1-jonathantanmy@google.com \
    --to=jonathantanmy@google.com \
    --cc=avarab@gmail.com \
    --cc=chooglen@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /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).