From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org, chooglen@google.com, avarab@gmail.com
Subject: Re: [PATCH v4 2/2] config: include file if remote URL matches a glob
Date: Wed, 01 Dec 2021 22:57:05 -0800 [thread overview]
Message-ID: <xmqqpmqfwl32.fsf@gitster.g> (raw)
In-Reply-To: <3b3af0da983c65ac8282b14141495f6859cd575a.1638217387.git.jonathantanmy@google.com> (Jonathan Tan's message of "Mon, 29 Nov 2021 12:23:42 -0800")
Jonathan Tan <jonathantanmy@google.com> writes:
> +static int add_remote_url(const char *var, const char *value, void *data)
> +{
> +...
> +}
> +
> +static void populate_remote_urls(struct config_include_data *inc)
> +{
> +...
> +}
> +
> +static int forbid_remote_url(const char *var, const char *value, void *data)
> +{
> +...
> +}
> +
> +static int at_least_one_url_matches_glob(const char *glob, int glob_len,
> + struct string_list *remote_urls)
> +{
> +...
> +}
All of the above makes sense; you prepare the remote URLs defined in
a string list, and have these helper functions that can determine if
the value given to hasremoteurl:* condition satisfies the condition.
> static int git_config_include(const char *var, const char *value, void *data)
> {
> struct config_include_data *inc = data;
> const char *cond, *key;
> size_t cond_len;
> - int ret;
> + int ret = 0;
>
> /*
> * Pass along all values, including "include" directives; this makes it
> @@ -335,9 +412,29 @@ static int git_config_include(const char *var, const char *value, void *data)
> ret = handle_path_include(value, inc);
>
> if (!parse_config_key(var, "includeif", &cond, &cond_len, &key) &&
> - (cond && include_condition_is_true(inc->opts, cond, cond_len)) &&
> - !strcmp(key, "path"))
> - ret = handle_path_include(value, inc);
> + cond && !strcmp(key, "path")) {
> + const char *url;
> + size_t url_len;
> +
> + if (skip_prefix_mem(cond, cond_len, "hasremoteurl:", &url,
> + &url_len)) {
> + if (inc->opts->unconditional_remote_url) {
> + config_fn_t old_fn = inc->fn;
> +
> + inc->fn = forbid_remote_url;
> + ret = handle_path_include(value, inc);
> + inc->fn = old_fn;
> + } else {
> + if (!inc->remote_urls)
> + populate_remote_urls(inc);
> + if (at_least_one_url_matches_glob(
> + url, url_len, inc->remote_urls))
> + ret = handle_path_include(value, inc);
> + }
> + } else if (include_condition_is_true(inc->opts, cond, cond_len)) {
> + ret = handle_path_include(value, inc);
> + }
This looks iffy, especialy in a patch that is not marked as [RFC].
I can see that include_condition_is_true() only passes inc->opts and
you need some other parts of inc for your purpose, and it may be the
primary reason why you munge this caller instead of adding function
include_by_remoteurl() and making include_condition_is_true() call
it. But wouldn't it be sufficient to pass inc (not inc->opts) to
include_condition_is_true(), and have it dereference inc->opts when
calling include_by_gitdir() and friends that want opts, while
passing inc to include_by_remoteurl()?
next prev parent reply other threads:[~2021-12-02 6:57 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 [this message]
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=xmqqpmqfwl32.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=avarab@gmail.com \
--cc=chooglen@google.com \
--cc=git@vger.kernel.org \
--cc=jonathantanmy@google.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).