{"thread":{"id":"51750","subject":"[PATCH 0/2] config: make config_with_options() handle any repo","startedAt":"2019-08-26T23:57:42Z","lastAt":"2019-08-30T09:09:51Z","messageCount":10,"participants":["Matheus Tavares","Duy Nguyen","Matheus Tavares Bernardino","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"381270","messageId":"cover.1566863604.git.matheus.bernardino@usp.br","threadId":"51750","inReplyTo":null,"subject":"[PATCH 0/2] config: make config_with_options() handle any repo","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-08-26T23:57:26Z","receivedAt":"2019-08-26T23:57:42Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"This patchset makes more config.c functions handle arbitrary repos and\nuse that to remove an add_to_alternates_memory() call in\nsubmodule-config.c. This should hopefully benefit performance and\nmemory.\n\nMatheus Tavares (2):\n config: allow config_with_options() to handle any repo\n submodule: pass repo instead of adding to alternates list\n\n Documentation/technical/api-config.txt | 5 +++-\n config.c | 26 +++++++++----------\n config.h | 16 ++++++++----\n .../coccinelle/the_repository.pending.cocci | 11 ++++++++\n submodule-config.c | 9 +++----\n 5 files changed, 42 insertions(+), 25 deletions(-)\n\n-- \n2.22.0\n\n"},{"id":"381271","messageId":"4920d3c474375abb39ed163c5ed6138a5e5dccc6.1566863604.git.matheus.bernardino@usp.br","threadId":"51750","inReplyTo":"cover.1566863604.git.matheus.bernardino@usp.br","subject":"[PATCH 1/2] config: allow config_with_options() to handle any repo","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-08-26T23:57:27Z","receivedAt":"2019-08-26T23:57:54Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Currently, config_with_options() relies on the global the_repository\nwhen it has to configure from a blob. A possible way to bypass this\nlimitation, when working with arbitrary repos, is to add their object\ndirectories into the in-memory alternates list. That's what\nsubmodule-config.c::config_from_gitmodules() does, for example. But this\napproach can negatively affect performance and memory. So introduce\nrepo_config_with_options() which takes a struct repository as an\nadditional parameter and pass it down in the call stack. Also, leave the\noriginal config_with_options() as a macro behind\nNO_THE_REPOSITORY_COMPATIBILITY_MACROS.\n\nFinally, adjust documentation and add a rule to coccinelle to reflect\nthe change.\n\nNote: the following patch will take care of actually using the added\nfunction in place of config_with_options() at config_from_gitmodules().\n\nSigned-off-by: Matheus Tavares \n---\n Documentation/technical/api-config.txt | 5 +++-\n config.c | 26 +++++++++----------\n config.h | 16 ++++++++----\n .../coccinelle/the_repository.pending.cocci | 11 ++++++++\n submodule-config.c | 2 +-\n 5 files changed, 39 insertions(+), 21 deletions(-)\n\ndiff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nindex 7d20716c32..ad99353df8 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -47,7 +47,7 @@ will first feed the user-wide one to the callback, and then the\n repo-specific one; by overwriting, the higher-priority repo-specific\n value is left at the end).\n \n-The `config_with_options` function lets the caller examine config\n+The `repo_config_with_options` function lets the caller examine config\n while adjusting some of the default behavior of `git_config`. It should\n almost never be used by \"regular\" Git code that is looking up\n configuration variables. It is intended for advanced callers like\n@@ -65,6 +65,9 @@ Specify options to adjust the behavior of parsing config files. See `struct\n config_options` in `config.h` for details. As an example: regular `git_config`\n sets `opts.respect_includes` to `1` by default.\n \n+The `config_with_options` macro is provided as an alias for\n+`repo_config_with_options`, using 'the_repository' by default.\n+\n Reading Specific Files\n ----------------------\n \ndiff --git a/config.c b/config.c\nindex 3900e4947b..dc116f2582 100644\n--- a/config.c\n+++ b/config.c\n@@ -1624,17 +1624,16 @@ int git_config_from_mem(config_fn_t fn,\n \treturn do_config_from(&top, fn, data, opts);\n }\n \n-int git_config_from_blob_oid(config_fn_t fn,\n-\t\t\t const char *name,\n-\t\t\t const struct object_id *oid,\n-\t\t\t void *data)\n+int git_config_from_blob_oid(struct repository *r, config_fn_t fn,\n+\t\t\t const char *name, const struct object_id *oid,\n+\t\t\t void *data)\n {\n \tenum object_type type;\n \tchar *buf;\n \tunsigned long size;\n \tint ret;\n \n-\tbuf = read_object_file(oid, &type, &size);\n+\tbuf = repo_read_object_file(r, oid, &type, &size);\n \tif (!buf)\n \t\treturn error(_(\"unable to load config blob object '%s'\"), name);\n \tif (type != OBJ_BLOB) {\n@@ -1649,15 +1648,14 @@ int git_config_from_blob_oid(config_fn_t fn,\n \treturn ret;\n }\n \n-static int git_config_from_blob_ref(config_fn_t fn,\n-\t\t\t\t const char *name,\n-\t\t\t\t void *data)\n+static int git_config_from_blob_ref(struct repository *r, config_fn_t fn,\n+\t\t\t\t const char *name, void *data)\n {\n \tstruct object_id oid;\n \n-\tif (get_oid(name, &oid) < 0)\n+\tif (repo_get_oid(r, name, &oid) < 0)\n \t\treturn error(_(\"unable to resolve config blob '%s'\"), name);\n-\treturn git_config_from_blob_oid(fn, name, &oid, data);\n+\treturn git_config_from_blob_oid(r, fn, name, &oid, data);\n }\n \n const char *git_etc_gitconfig(void)\n@@ -1751,9 +1749,9 @@ static int do_git_config_sequence(const struct config_options *opts,\n \treturn ret;\n }\n \n-int config_with_options(config_fn_t fn, void *data,\n-\t\t\tstruct git_config_source *config_source,\n-\t\t\tconst struct config_options *opts)\n+int repo_config_with_options(struct repository *r, config_fn_t fn, void *data,\n+\t\t\t struct git_config_source *config_source,\n+\t\t\t const struct config_options *opts)\n {\n \tstruct config_include_data inc = CONFIG_INCLUDE_INIT;\n \n@@ -1774,7 +1772,7 @@ int config_with_options(config_fn_t fn, void *data,\n \telse if (config_source && config_source->file)\n \t\treturn git_config_from_file(fn, config_source->file, data);\n \telse if (config_source && config_source->blob)\n-\t\treturn git_config_from_blob_ref(fn, config_source->blob, data);\n+\t\treturn git_config_from_blob_ref(r, fn, config_source->blob, data);\n \n \treturn do_git_config_sequence(opts, fn, data);\n }\ndiff --git a/config.h b/config.h\nindex f0ed464004..33ce46ecc3 100644\n--- a/config.h\n+++ b/config.h\n@@ -82,16 +82,22 @@ int git_config_from_mem(config_fn_t fn,\n \t\t\tconst char *name,\n \t\t\tconst char *buf, size_t len,\n \t\t\tvoid *data, const struct config_options *opts);\n-int git_config_from_blob_oid(config_fn_t fn, const char *name,\n-\t\t\t const struct object_id *oid, void *data);\n+int git_config_from_blob_oid(struct repository *r, config_fn_t fn,\n+\t\t\t const char *name, const struct object_id *oid,\n+\t\t\t void *data);\n void git_config_push_parameter(const char *text);\n int git_config_from_parameters(config_fn_t fn, void *data);\n void read_early_config(config_fn_t cb, void *data);\n void read_very_early_config(config_fn_t cb, void *data);\n void git_config(config_fn_t fn, void *);\n-int config_with_options(config_fn_t fn, void *,\n-\t\t\tstruct git_config_source *config_source,\n-\t\t\tconst struct config_options *opts);\n+int repo_config_with_options(struct repository *r, config_fn_t fn, void *data,\n+\t\t\t struct git_config_source *config_source,\n+\t\t\t const struct config_options *opts);\n+#ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n+#define config_with_options(fn, data, config_source, opts) \\\n+\trepo_config_with_options(the_repository, fn, data, config_source, opts)\n+#endif\n+\n int git_parse_ssize_t(const char *, ssize_t *);\n int git_parse_ulong(const char *, unsigned long *);\n int git_parse_maybe_bool(const char *);\ndiff --git a/contrib/coccinelle/the_repository.pending.cocci b/contrib/coccinelle/the_repository.pending.cocci\nindex 2ee702ecf7..53ecc975bf 100644\n--- a/contrib/coccinelle/the_repository.pending.cocci\n+++ b/contrib/coccinelle/the_repository.pending.cocci\n@@ -142,3 +142,14 @@ expression H;\n - format_commit_message(\n + repo_format_commit_message(the_repository,\n E, F, G, H);\n+\n+@@\n+expression E;\n+expression F;\n+expression G;\n+expression H;\n+@@\n+- config_with_options(\n++ repo_config_with_options(the_repository,\n+ E, F, G, H);\n+\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 4264ee216f..1d28b17071 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -681,7 +681,7 @@ void gitmodules_config_oid(const struct object_id *commit_oid)\n \tsubmodule_cache_check_init(the_repository);\n \n \tif (gitmodule_oid_from_commit(commit_oid, &oid, &rev)) {\n-\t\tgit_config_from_blob_oid(gitmodules_cb, rev.buf,\n+\t\tgit_config_from_blob_oid(the_repository, gitmodules_cb, rev.buf,\n \t\t\t\t\t &oid, the_repository);\n \t}\n \tstrbuf_release(&rev);\n-- \n2.22.0\n\n"},{"id":"381272","messageId":"4619e04ecca575e7eb5f73f555be0bb55d11385e.1566863604.git.matheus.bernardino@usp.br","threadId":"51750","inReplyTo":"cover.1566863604.git.matheus.bernardino@usp.br","subject":"[PATCH 2/2] submodule: pass repo instead of adding to alternates list","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-08-26T23:57:28Z","receivedAt":"2019-08-26T23:57:56Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Previously, config_with_options() wouldn't handle arbitrary repositories\nbesides the_repository. Because of that, when retrieving .gitmodules\nfrom the cache, config_from_gitmodules() first needed to add the object\ndirectories of the given repo to the in-memory alternates list. But we\nhave repo_config_with_options() now, which takes a repository as\nargument. So let's use it and remove the call to\nadd_to_alternates_memory(). This should bring better performance to\ncommands using the function (there'll be fewer odb entries to process)\nbesides saving memory (repos may be free'd right after use whereas\nthe_repository's alternates list doesn't).\n\nWhile we are here, let's also adjust the comment on top of\nconfig_from_gitmodules() to be explicit that it also handles the case\nwhere .gitmodules is not present at the working tree.\n\nSigned-off-by: Matheus Tavares \n---\n submodule-config.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 1d28b17071..8271aa3834 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -616,7 +616,8 @@ static void submodule_cache_check_init(struct repository *repo)\n * the repository.\n *\n * Runs the provided config function on the '.gitmodules' file found in the\n- * working directory.\n+ * working directory. If the file is not present, tries to retrieve it from\n+ * the staging area or HEAD.\n */\n static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void *data)\n {\n@@ -633,13 +634,11 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void\n \t\t} else if (repo_get_oid(repo, GITMODULES_INDEX, &oid) >= 0 ||\n \t\t\t repo_get_oid(repo, GITMODULES_HEAD, &oid) >= 0) {\n \t\t\tconfig_source.blob = oidstr = xstrdup(oid_to_hex(&oid));\n-\t\t\tif (repo != the_repository)\n-\t\t\t\tadd_to_alternates_memory(repo->objects->odb->path);\n \t\t} else {\n \t\t\tgoto out;\n \t\t}\n \n-\t\tconfig_with_options(fn, data, &config_source, &opts);\n+\t\trepo_config_with_options(repo, fn, data, &config_source, &opts);\n \n out:\n \t\tfree(oidstr);\n-- \n2.22.0\n\n"},{"id":"381330","messageId":"CACsJy8Dry7MfKBi5EKw4Ka9r63QVmDjPv9nAozS0mC6Z7-sG=w@mail.gmail.com","threadId":"51750","inReplyTo":"4920d3c474375abb39ed163c5ed6138a5e5dccc6.1566863604.git.matheus.bernardino@usp.br","subject":"Re: [PATCH 1/2] config: allow config_with_options() to handle any repo","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-08-27T09:26:15Z","receivedAt":"2019-08-27T09:26:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Aug 27, 2019 at 6:57 AM Matheus Tavares\n wrote:\n>\n> Currently, config_with_options() relies on the global the_repository\n> when it has to configure from a blob.\n\nNot really reading the patch, but my last experience with moving\nconfig.c away from the_repo [1] shows that there are more hidden\ndependencies, in git_path() and particularly the git_config_clear()\ncall in git_config_set_multivar_... Not really sure if those deps\nreally affect your goals or not. Have a look at that branch, filtering\non config.c for more info (and if you want to pick up some patches\nfrom that, you have my sign-off).\n\n[1] https://gitlab.com/pclouds/git/commits/submodules-in-worktrees\n\n--\nDuy\n"},{"id":"381408","messageId":"CAHd-oW4h80xrp6y65dqZbq_a67ncArC9rrNq7F7rAhBbrALOkA@mail.gmail.com","threadId":"51750","inReplyTo":"CACsJy8Dry7MfKBi5EKw4Ka9r63QVmDjPv9nAozS0mC6Z7-sG=w@mail.gmail.com","subject":"Re: [PATCH 1/2] config: allow config_with_options() to handle any repo","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-08-27T23:46:10Z","receivedAt":"2019-08-27T23:46:25Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, Duy\n\nOn Tue, Aug 27, 2019 at 6:26 AM Duy Nguyen wrote:\n>\n> On Tue, Aug 27, 2019 at 6:57 AM Matheus Tavares\n> wrote:\n> >\n> > Currently, config_with_options() relies on the global the_repository\n> > when it has to configure from a blob.\n>\n> Not really reading the patch, but my last experience with moving\n> config.c away from the_repo [1] shows that there are more hidden\n> dependencies, in git_path() and particularly the git_config_clear()\n> call in git_config_set_multivar_... Not really sure if those deps\n> really affect your goals or not. Have a look at that branch, filtering\n> on config.c for more info (and if you want to pick up some patches\n> from that, you have my sign-off).\n\nThanks for the advice. Indeed, I see now that do_git_config_sequence()\nmay call git_pathdup(), which relies on the_repo. For my use in patch\n2/2, repo_config_with_options() won't ever get to call\ndo_git_config_sequence(), so that's fine. But in other use cases it\nmay have to, so I'll need to check that.\n\n> [1] https://gitlab.com/pclouds/git/commits/submodules-in-worktrees\n>\n> --\n> Duy\n"},{"id":"381495","messageId":"CAHd-oW6doh=06nxUkMLWZOwNMyUOogLRbLstD0bJVQxLauR_Aw@mail.gmail.com","threadId":"51750","inReplyTo":"CAHd-oW4h80xrp6y65dqZbq_a67ncArC9rrNq7F7rAhBbrALOkA@mail.gmail.com","subject":"Re: [PATCH 1/2] config: allow config_with_options() to handle any repo","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-08-29T04:24:44Z","receivedAt":"2019-08-29T04:24:58Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Tue, Aug 27, 2019 at 8:46 PM Matheus Tavares Bernardino\n wrote:\n>\n> Hi, Duy\n>\n> On Tue, Aug 27, 2019 at 6:26 AM Duy Nguyen wrote:\n> >\n> > On Tue, Aug 27, 2019 at 6:57 AM Matheus Tavares\n> > wrote:\n> > >\n> > > Currently, config_with_options() relies on the global the_repository\n> > > when it has to configure from a blob.\n> >\n> > Not really reading the patch, but my last experience with moving\n> > config.c away from the_repo [1] shows that there are more hidden\n> > dependencies, in git_path() and particularly the git_config_clear()\n> > call in git_config_set_multivar_... Not really sure if those deps\n> > really affect your goals or not. Have a look at that branch, filtering\n> > on config.c for more info (and if you want to pick up some patches\n> > from that, you have my sign-off).\n>\n> Thanks for the advice. Indeed, I see now that do_git_config_sequence()\n> may call git_pathdup(), which relies on the_repo. For my use in patch\n> 2/2, repo_config_with_options() won't ever get to call\n> do_git_config_sequence(), so that's fine. But in other use cases it\n> may have to, so I'll need to check that.\n\nWhile working on this, I think I may have found a bug: The\nrepo_read_config() function takes a repository R as parameter and\ncalls this chain of functions:\n\nrepo_read_config(struct repository *R) > config_with_options() >\ndo_git_config_sequence() > git_pathdup(\"config.worktree\")\n\nShouldn't, however, the last call consider R instead of using\nthe_repository? i.e., use repo_git_path(R, \"config.worktree\"),\ninstead?\n\nIf so, how could we get R there? I mean, we could pass it through this\nchain, but the chain already passes a \"struct config_options\", which\ncarries the \"commondir\" and \"git_dir\" fields. So it would probably be\nconfusing to have them and an extra repository parameter (which also\nhas \"commondir\" and \"git_dir\"), right? Any ideas on how to better\napproach this?\n\n> > [1] https://gitlab.com/pclouds/git/commits/submodules-in-worktrees\n> >\n> > --\n> > Duy\n"},{"id":"381499","messageId":"CACsJy8DpTxpejkOHCYPnt3saC-h-3Ez0TthAPnPvHHThaG64bQ@mail.gmail.com","threadId":"51750","inReplyTo":"CAHd-oW6doh=06nxUkMLWZOwNMyUOogLRbLstD0bJVQxLauR_Aw@mail.gmail.com","subject":"Re: [PATCH 1/2] config: allow config_with_options() to handle any repo","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-08-29T09:31:34Z","receivedAt":"2019-08-29T09:32:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Aug 29, 2019 at 11:24 AM Matheus Tavares Bernardino\n wrote:\n>\n> On Tue, Aug 27, 2019 at 8:46 PM Matheus Tavares Bernardino\n> wrote:\n> >\n> > Hi, Duy\n> >\n> > On Tue, Aug 27, 2019 at 6:26 AM Duy Nguyen wrote:\n> > >\n> > > On Tue, Aug 27, 2019 at 6:57 AM Matheus Tavares\n> > > wrote:\n> > > >\n> > > > Currently, config_with_options() relies on the global the_repository\n> > > > when it has to configure from a blob.\n> > >\n> > > Not really reading the patch, but my last experience with moving\n> > > config.c away from the_repo [1] shows that there are more hidden\n> > > dependencies, in git_path() and particularly the git_config_clear()\n> > > call in git_config_set_multivar_... Not really sure if those deps\n> > > really affect your goals or not. Have a look at that branch, filtering\n> > > on config.c for more info (and if you want to pick up some patches\n> > > from that, you have my sign-off).\n> >\n> > Thanks for the advice. Indeed, I see now that do_git_config_sequence()\n> > may call git_pathdup(), which relies on the_repo. For my use in patch\n> > 2/2, repo_config_with_options() won't ever get to call\n> > do_git_config_sequence(), so that's fine. But in other use cases it\n> > may have to, so I'll need to check that.\n>\n> While working on this, I think I may have found a bug: The\n> repo_read_config() function takes a repository R as parameter and\n> calls this chain of functions:\n>\n> repo_read_config(struct repository *R) > config_with_options() >\n> do_git_config_sequence() > git_pathdup(\"config.worktree\")\n>\n> Shouldn't, however, the last call consider R instead of using\n> the_repository? i.e., use repo_git_path(R, \"config.worktree\"),\n> instead?\n\nYes. You just found one of the plenty traps because the_repository is\nstill hidden in many core functions.\n\n> If so, how could we get R there? I mean, we could pass it through this\n> chain, but the chain already passes a \"struct config_options\", which\n> carries the \"commondir\" and \"git_dir\" fields. So it would probably be\n> confusing to have them and an extra repository parameter (which also\n> has \"commondir\" and \"git_dir\"), right? Any ideas on how to better\n> approach this?\n\nI would change 'struct config_options' to carry 'struct repository'\nwhich also contains git_dir and other info inside. Though I have no\nidea how big that change would be (didn't check the code). Config code\nrelies on plenty callbacks without \"void *cb_data\" so relying on\nglobal state is the only way in some cases.\n-- \nDuy\n"},{"id":"381508","messageId":"20190829140013.GC1797@sigill.intra.peff.net","threadId":"51750","inReplyTo":"CACsJy8DpTxpejkOHCYPnt3saC-h-3Ez0TthAPnPvHHThaG64bQ@mail.gmail.com","subject":"Re: [PATCH 1/2] config: allow config_with_options() to handle any repo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-29T14:00:13Z","receivedAt":"2019-08-29T14:00:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 29, 2019 at 04:31:34PM +0700, Duy Nguyen wrote:\n\n> > If so, how could we get R there? I mean, we could pass it through this\n> > chain, but the chain already passes a \"struct config_options\", which\n> > carries the \"commondir\" and \"git_dir\" fields. So it would probably be\n> > confusing to have them and an extra repository parameter (which also\n> > has \"commondir\" and \"git_dir\"), right? Any ideas on how to better\n> > approach this?\n> \n> I would change 'struct config_options' to carry 'struct repository'\n> which also contains git_dir and other info inside. Though I have no\n> idea how big that change would be (didn't check the code). Config code\n> relies on plenty callbacks without \"void *cb_data\" so relying on\n> global state is the only way in some cases.\n\nI'm not sure about that, at least for this particular git_pathdup(). We\npass along the git_dir because we might not have a repository struct yet\n(i.e., when reading config before repo discovery has happened).\n\nSo it might be that this case should actually be making a path out of\n$git_dir/config.worktree (but I'm not 100% sure, as I don't know the ins\nand outs of worktree config files).\n\nI'm sure there are other gotchas in the config code, though, related to\nthings for which we _do_ need a repository. E.g., include_by_branch()\nlooks at the_repository, and should use a repository struct matching the\ngit_dir we're looking at (though it may be acceptable to bail during\nearly pre-repo-initialization config and just disallow branch includes,\nwhich is what happens now).\n\n-Peff\n"},{"id":"381517","messageId":"CAHd-oW6JdTSWHy6UwGhL5PUoiscayh9xVqs5ktsXzotg_vexmQ@mail.gmail.com","threadId":"51750","inReplyTo":"20190829140013.GC1797@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] config: allow config_with_options() to handle any repo","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-08-29T16:44:03Z","receivedAt":"2019-08-29T16:44:16Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Thu, Aug 29, 2019 at 11:00 AM Jeff King wrote:\n>\n> On Thu, Aug 29, 2019 at 04:31:34PM +0700, Duy Nguyen wrote:\n>\n> > > If so, how could we get R there? I mean, we could pass it through this\n> > > chain, but the chain already passes a \"struct config_options\", which\n> > > carries the \"commondir\" and \"git_dir\" fields. So it would probably be\n> > > confusing to have them and an extra repository parameter (which also\n> > > has \"commondir\" and \"git_dir\"), right? Any ideas on how to better\n> > > approach this?\n> >\n> > I would change 'struct config_options' to carry 'struct repository'\n> > which also contains git_dir and other info inside. Though I have no\n> > idea how big that change would be (didn't check the code). Config code\n> > relies on plenty callbacks without \"void *cb_data\" so relying on\n> > global state is the only way in some cases.\n>\n> I'm not sure about that, at least for this particular git_pathdup(). We\n> pass along the git_dir because we might not have a repository struct yet\n> (i.e., when reading config before repo discovery has happened).\n\nYes, I think read_early_config(), for example, may call\nconfig_with_options() before the_repo is initialized.\n\n> So it might be that this case should actually be making a path out of\n> $git_dir/config.worktree (but I'm not 100% sure, as I don't know the ins\n> and outs of worktree config files).\n\nMakes sense, config.worktree files are per-worktree, which have\ndifferent git_dir's, right?\n\n> I'm sure there are other gotchas in the config code, though, related to\n> things for which we _do_ need a repository. E.g., include_by_branch()\n> looks at the_repository, and should use a repository struct matching the\n> git_dir we're looking at (though it may be acceptable to bail during\n> early pre-repo-initialization config and just disallow branch includes,\n> which is what happens now).\n\nI think config_with_options() is another example of a place where we\nshould have a reference to a repo (but we currently don't). When\nconfiguring from a given blob, it will call\ngit_config_from_blob_ref(), which calls get_oid() and\nread_object_file(). Both of these functions will use the_repo by\ndefault. But the git_dir and commondir fields passed to\nconfig_with_options() through 'struct config_options' may not refer to\nthe_repo, right?\n\nI'm not sure what is the best solution to this, though. I mean, we\ncould add a 'struct repository' in 'struct config_options', but as you\nalready pointed out, some callers might not have a repository struct\nyet...\n\n> -Peff\n"},{"id":"381568","messageId":"CACsJy8CzNBWHtaO4E4efTKU5ZJ=rH5uNEW1YNzRbofxwAVpTzQ@mail.gmail.com","threadId":"51750","inReplyTo":"CAHd-oW6JdTSWHy6UwGhL5PUoiscayh9xVqs5ktsXzotg_vexmQ@mail.gmail.com","subject":"Re: [PATCH 1/2] config: allow config_with_options() to handle any repo","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-08-30T09:09:23Z","receivedAt":"2019-08-30T09:09:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Aug 29, 2019 at 11:44 PM Matheus Tavares Bernardino\n wrote:\n> > I'm sure there are other gotchas in the config code, though, related to\n> > things for which we _do_ need a repository. E.g., include_by_branch()\n> > looks at the_repository, and should use a repository struct matching the\n> > git_dir we're looking at (though it may be acceptable to bail during\n> > early pre-repo-initialization config and just disallow branch includes,\n> > which is what happens now).\n>\n> I think config_with_options() is another example of a place where we\n> should have a reference to a repo (but we currently don't). When\n> configuring from a given blob, it will call\n> git_config_from_blob_ref(), which calls get_oid() and\n> read_object_file(). Both of these functions will use the_repo by\n> default. But the git_dir and commondir fields passed to\n> config_with_options() through 'struct config_options' may not refer to\n> the_repo, right?\n>\n> I'm not sure what is the best solution to this, though. I mean, we\n> could add a 'struct repository' in 'struct config_options', but as you\n> already pointed out, some callers might not have a repository struct\n> yet...\n\nEarly setup code has always been special (there's a lot of stuff you\ndon't have access too). Ideally we could have a lower level API that\ntakes git_dir and git_commondir only, no access to 'struct\nrepository'. This is used for early access. And we have a higher level\nAPI that only takes struct repo and pass repo->gitdir down to the that\nlowlevel one. But I guess that's not the reality we're in.\n\nSince early setup code is special, perhaps you could make 'struct\nconfig_options' take both git_dir and struct repo, but not both at the\nsame time. Early setup code sets repo to NULL and git_dir something\nelse. Other code always leave git_dir and git_common_dir to NULL,\ndocumented to say those are for early setup only.\n\nPS. Again still not looking at the code so I may just be talking rubbish here.\n-- \nDuy\n"}]}