{"thread":{"id":"55374","subject":"Pass or not to pass config environment down...","startedAt":"2021-03-23T18:58:42Z","lastAt":"2021-03-31T11:26:19Z","messageCount":7,"participants":["Junio C Hamano","Jacob Keller","Jeff King","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"420055","messageId":"xmqqk0px3dfu.fsf@gitster.g","threadId":"55374","inReplyTo":null,"subject":"Pass or not to pass config environment down...","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-23T18:57:57Z","receivedAt":"2021-03-23T18:58:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I was grepping around and found this piece of code today:\n\n        static void prepare_submodule_repo_env_no_git_dir(struct strvec *out)\n        {\n                const char * const *var;\n\n                for (var = local_repo_env; *var; var++) {\n                        if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))\n                                strvec_push(out, *var);\n                }\n        }\n\nwhich tries to \"unsetenv\" the environment variables that pertain to\nthe current repository listed in loocal_repo_env[], but makes\nexception for GIT_CONFIG_PARAMETERS.\n\nIt originally came from 14111fc4 (git: submodule honor -c\ncredential.* from command line, 2016-02-29) and later simplified by\n89044baa (submodule: stop sanitizing config options, 2016-05-04).\n\nNow after d8d77153 (config: allow specifying config entries via\nenvvar pairs, 2021-01-12), we have yet another way to pass a set of\ncustom one-shot configuration via the environment variable, using\nGIT_CONFIG_COUNT (which is in local_repo_env[] and will be removed\nfrom the environment by this helper function), GIT_CONFIG_KEY_$n and\nGIT_CONFIG_VALUE_$n (which are unbound set and naturally not in\nlocal_repo_env[]).  Leaving the latter two exported will not hurt if\nwe do intend to hide the custom configuration from the subprocess by\nunsetting GIT_CONFIG_COUNT, but should we be doing so?\n\nThere are many run_command() users that just pass local_repo_env[]\nto the child.env when running a subprocess.  Given that the code\nthat works in a submodule, which presumably is THE primary target\nof the \"we do not want to pass environment variables that pertain to\nthe current repository but not to the repository the child process\nworks in\" consideration that the local_repo_env[] is about, does *not*\nwant the GIT_CONFIG_PARAMETERS cleansed, I have to wonder if the\nenvironment variables (the original GIT_CONFIG_PARAMETERS as well as\nPatrick's GIT_CONFIG_{COUNT,KEY_$n,VALUE_$n}) should be in that\nlocal_repo_env[] array in the first place.  If we remove them, the\nabove helper function can just go away and be replaced with the\nusual child.env = local_repo_env assignment like everybody else.\n\nComments?\n\n environment.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git c/environment.c w/environment.c\nindex 2f27008424..6dcee6a9c5 100644\n--- c/environment.c\n+++ w/environment.c\n@@ -116,8 +116,6 @@ static char *super_prefix;\n const char * const local_repo_env[] = {\n \tALTERNATE_DB_ENVIRONMENT,\n \tCONFIG_ENVIRONMENT,\n-\tCONFIG_DATA_ENVIRONMENT,\n-\tCONFIG_COUNT_ENVIRONMENT,\n \tDB_ENVIRONMENT,\n \tGIT_DIR_ENVIRONMENT,\n \tGIT_WORK_TREE_ENVIRONMENT,\n"},{"id":"420069","messageId":"CA+P7+xokjz-2nC5+F9HG0tBqS948uZcuq-1eY9uGuNyzbFGbZQ@mail.gmail.com","threadId":"55374","inReplyTo":"xmqqk0px3dfu.fsf@gitster.g","subject":"Re: Pass or not to pass config environment down...","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2021-03-23T20:52:31Z","receivedAt":"2021-03-23T20:53:45Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Mar 23, 2021 at 11:58 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> I was grepping around and found this piece of code today:\n>\n>         static void prepare_submodule_repo_env_no_git_dir(struct strvec *out)\n>         {\n>                 const char * const *var;\n>\n>                 for (var = local_repo_env; *var; var++) {\n>                         if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))\n>                                 strvec_push(out, *var);\n>                 }\n>         }\n>\n> which tries to \"unsetenv\" the environment variables that pertain to\n> the current repository listed in loocal_repo_env[], but makes\n> exception for GIT_CONFIG_PARAMETERS.\n\nRight. We need to unset some parameters, but not everything...\n\n>\n> It originally came from 14111fc4 (git: submodule honor -c\n> credential.* from command line, 2016-02-29) and later simplified by\n> 89044baa (submodule: stop sanitizing config options, 2016-05-04).\n>\n> Now after d8d77153 (config: allow specifying config entries via\n> envvar pairs, 2021-01-12), we have yet another way to pass a set of\n> custom one-shot configuration via the environment variable, using\n> GIT_CONFIG_COUNT (which is in local_repo_env[] and will be removed\n> from the environment by this helper function), GIT_CONFIG_KEY_$n and\n> GIT_CONFIG_VALUE_$n (which are unbound set and naturally not in\n> local_repo_env[]).  Leaving the latter two exported will not hurt if\n> we do intend to hide the custom configuration from the subprocess by\n> unsetting GIT_CONFIG_COUNT, but should we be doing so?\n>\n\nI think it's a difficult question because if I recall, there was some\nquestion/concern about what values need to stay for the submodule vs\nwhich ones do not..?\n\n> There are many run_command() users that just pass local_repo_env[]\n> to the child.env when running a subprocess.  Given that the code\n> that works in a submodule, which presumably is THE primary target\n> of the \"we do not want to pass environment variables that pertain to\n> the current repository but not to the repository the child process\n> works in\" consideration that the local_repo_env[] is about, does *not*\n> want the GIT_CONFIG_PARAMETERS cleansed, I have to wonder if the\n> environment variables (the original GIT_CONFIG_PARAMETERS as well as\n> Patrick's GIT_CONFIG_{COUNT,KEY_$n,VALUE_$n}) should be in that\n> local_repo_env[] array in the first place.  If we remove them, the\n> above helper function can just go away and be replaced with the\n> usual child.env = local_repo_env assignment like everybody else.\n>\n\nI think that makes a reasonable amount of sense. So if I understand\nright, this change makes it so that CONFIG_DATA_ENVIRONMENT and\nCONFIG_COUNT_ENVIRONMENT will no longer be forwarded? I guess I am a\nbit confused about the current status vs what we want here.\n\n> Comments?\n>\n>  environment.c | 2 --\n>  1 file changed, 2 deletions(-)\n>\n> diff --git c/environment.c w/environment.c\n> index 2f27008424..6dcee6a9c5 100644\n> --- c/environment.c\n> +++ w/environment.c\n> @@ -116,8 +116,6 @@ static char *super_prefix;\n>  const char * const local_repo_env[] = {\n>         ALTERNATE_DB_ENVIRONMENT,\n>         CONFIG_ENVIRONMENT,\n> -       CONFIG_DATA_ENVIRONMENT,\n> -       CONFIG_COUNT_ENVIRONMENT,\n>         DB_ENVIRONMENT,\n>         GIT_DIR_ENVIRONMENT,\n>         GIT_WORK_TREE_ENVIRONMENT,\n"},{"id":"420071","messageId":"xmqqmtut1qyj.fsf@gitster.g","threadId":"55374","inReplyTo":"CA+P7+xokjz-2nC5+F9HG0tBqS948uZcuq-1eY9uGuNyzbFGbZQ@mail.gmail.com","subject":"Re: Pass or not to pass config environment down...","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-23T21:48:52Z","receivedAt":"2021-03-23T21:49:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.keller@gmail.com> writes:\n\n> On Tue, Mar 23, 2021 at 11:58 AM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> I was grepping around and found this piece of code today:\n>>\n>>         static void prepare_submodule_repo_env_no_git_dir(struct strvec *out)\n>>         {\n>>                 const char * const *var;\n>>\n>>                 for (var = local_repo_env; *var; var++) {\n>>                         if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))\n>>                                 strvec_push(out, *var);\n>>                 }\n>>         }\n>>\n>> which tries to \"unsetenv\" the environment variables that pertain to\n>> the current repository listed in loocal_repo_env[], but makes\n>> exception for GIT_CONFIG_PARAMETERS.\n>\n> Right. We need to unset some parameters, but not everything...\n>\n>>\n>> It originally came from 14111fc4 (git: submodule honor -c\n>> credential.* from command line, 2016-02-29) and later simplified by\n>> 89044baa (submodule: stop sanitizing config options, 2016-05-04).\n>>\n>> Now after d8d77153 (config: allow specifying config entries via\n>> envvar pairs, 2021-01-12), we have yet another way to pass a set of\n>> custom one-shot configuration via the environment variable, using\n>> GIT_CONFIG_COUNT (which is in local_repo_env[] and will be removed\n>> from the environment by this helper function), GIT_CONFIG_KEY_$n and\n>> GIT_CONFIG_VALUE_$n (which are unbound set and naturally not in\n>> local_repo_env[]).  Leaving the latter two exported will not hurt if\n>> we do intend to hide the custom configuration from the subprocess by\n>> unsetting GIT_CONFIG_COUNT, but should we be doing so?\n>>\n>\n> I think it's a difficult question because if I recall, there was some\n> question/concern about what values need to stay for the submodule vs\n> which ones do not..?\n>\n>> There are many run_command() users that just pass local_repo_env[]\n>> to the child.env when running a subprocess.  Given that the code\n>> that works in a submodule, which presumably is THE primary target\n>> of the \"we do not want to pass environment variables that pertain to\n>> the current repository but not to the repository the child process\n>> works in\" consideration that the local_repo_env[] is about, does *not*\n>> want the GIT_CONFIG_PARAMETERS cleansed, I have to wonder if the\n>> environment variables (the original GIT_CONFIG_PARAMETERS as well as\n>> Patrick's GIT_CONFIG_{COUNT,KEY_$n,VALUE_$n}) should be in that\n>> local_repo_env[] array in the first place.  If we remove them, the\n>> above helper function can just go away and be replaced with the\n>> usual child.env = local_repo_env assignment like everybody else.\n>>\n>\n> I think that makes a reasonable amount of sense. So if I understand\n> right, this change makes it so that CONFIG_DATA_ENVIRONMENT and\n> CONFIG_COUNT_ENVIRONMENT will no longer be forwarded? I guess I am a\n> bit confused about the current status vs what we want here.\n\nMany callers do child.env = local_repo_env when spawning a\nsubprocess.  The elements of child.env is treated as if each of them\nwere passed to setenv() (if there is '=') or unsetenv (otherwise),\nand because local_repo_env[] spells only the variable names, the\neffect is to unexport them.  The helper function shown at the\nbeginning of the message you are responding to, which you wrote more\nthan 5 years ago, is to exclude GIT_CONFIG_PARAMETERS from that\ntreatment.  I.e. the code wants run_command() not to drop that\nparticular environment variable when running a subprocess.\n\nRemoving GIT_CONFIG_PARAMETERS from local_repo_env[] should have the\nsame effect, without the helper to special case it in its loop.\nWe have been passing GIT_CONFIG_PARAMETERS, and we will keep passing\nit even if we make such a change to remove it from local_repo_env[].\n\nThe configuration parameters passed via the newer GIT_CONFIG_COUNT\nmechanism, because local_repo_env[] has it but the above helper does\nnot special case it, are dropped and not seen by the subprocess.\nAssuming that it is a bug and we would want to pass them to the\nsubprocess the same way as GIT_CONFIG_PARAMETERS environment\nvariable, we could tweak the helper function to make it special case\nGIT_CONFIG_COUNT the same way as we've done GIT_CONFIG_PARAMETERS\nfor the past 5 years.  But if we suspect that other codepaths (not\nthe ones that use the above helper) may be doing it wrong and they\ntoo should pass the configuration parameters to the subprocess, a\nsimpler way would be to remove them from local_repo_env[].\n\nThat is the summary of the current status and what would happen if\nwe did the attached patch.\n\n>> Comments?\n>>\n>>  environment.c | 2 --\n>>  1 file changed, 2 deletions(-)\n>>\n>> diff --git c/environment.c w/environment.c\n>> index 2f27008424..6dcee6a9c5 100644\n>> --- c/environment.c\n>> +++ w/environment.c\n>> @@ -116,8 +116,6 @@ static char *super_prefix;\n>>  const char * const local_repo_env[] = {\n>>         ALTERNATE_DB_ENVIRONMENT,\n>>         CONFIG_ENVIRONMENT,\n>> -       CONFIG_DATA_ENVIRONMENT,\n>> -       CONFIG_COUNT_ENVIRONMENT,\n>>         DB_ENVIRONMENT,\n>>         GIT_DIR_ENVIRONMENT,\n>>         GIT_WORK_TREE_ENVIRONMENT,\n"},{"id":"420072","messageId":"CA+P7+xo9rUYWzCT0YZFU5Nz1ojP1qRt8VfGtK08-LmYV9KK0hw@mail.gmail.com","threadId":"55374","inReplyTo":"xmqqmtut1qyj.fsf@gitster.g","subject":"Re: Pass or not to pass config environment down...","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2021-03-23T21:57:50Z","receivedAt":"2021-03-23T21:58:49Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Mar 23, 2021 at 2:48 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Many callers do child.env = local_repo_env when spawning a\n> subprocess.  The elements of child.env is treated as if each of them\n> were passed to setenv() (if there is '=') or unsetenv (otherwise),\n> and because local_repo_env[] spells only the variable names, the\n> effect is to unexport them.  The helper function shown at the\n> beginning of the message you are responding to, which you wrote more\n> than 5 years ago, is to exclude GIT_CONFIG_PARAMETERS from that\n> treatment.  I.e. the code wants run_command() not to drop that\n> particular environment variable when running a subprocess.\n>\n\nOk, right. So the part that was confusing me is that by adding the\nvalue into the local_repo_env, we were telling it to clear the\nvariables. Yep.\n\n> Removing GIT_CONFIG_PARAMETERS from local_repo_env[] should have the\n> same effect, without the helper to special case it in its loop.\n> We have been passing GIT_CONFIG_PARAMETERS, and we will keep passing\n> it even if we make such a change to remove it from local_repo_env[].\n>\n\nYea, makes sense. I think that's a much better approach than special\ncasing in a separate function.\n\n> The configuration parameters passed via the newer GIT_CONFIG_COUNT\n> mechanism, because local_repo_env[] has it but the above helper does\n> not special case it, are dropped and not seen by the subprocess.\n> Assuming that it is a bug and we would want to pass them to the\n> subprocess the same way as GIT_CONFIG_PARAMETERS environment\n> variable, we could tweak the helper function to make it special case\n> GIT_CONFIG_COUNT the same way as we've done GIT_CONFIG_PARAMETERS\n> for the past 5 years.  But if we suspect that other codepaths (not\n> the ones that use the above helper) may be doing it wrong and they\n> too should pass the configuration parameters to the subprocess, a\n> simpler way would be to remove them from local_repo_env[].\n>\n> That is the summary of the current status and what would happen if\n> we did the attached patch.\n>\n\nThank you for restating and clarifying. I think this is the right\napproach, and I agree that GIT_CONFIG_COUNT should be treated in the\nsame way as GIT_CONFIG_PARAMETERS.\n\nSo, I think this direction is good. I imagine a full patch would\ninclude also dropping the specialized helper function that is no\nlonger needed, and possibly adding new tests for the behavior of\nGIT_CONFIG_COUNT?\n\nThanks,\nJake\n"},{"id":"420074","messageId":"xmqqim5h1otg.fsf@gitster.g","threadId":"55374","inReplyTo":"CA+P7+xo9rUYWzCT0YZFU5Nz1ojP1qRt8VfGtK08-LmYV9KK0hw@mail.gmail.com","subject":"Re: Pass or not to pass config environment down...","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-23T22:35:07Z","receivedAt":"2021-03-23T22:36:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.keller@gmail.com> writes:\n\n> So, I think this direction is good. I imagine a full patch would\n> include also dropping the specialized helper function that is no\n> longer needed, and possibly adding new tests for the behavior of\n> GIT_CONFIG_COUNT?\n\nYeah, coding that is the easiest part.  Thinking through\nramifications of making (or not making) such a change is much\nharder.\n\nI said \"assuming\" number of times, because I am not so sure if the\nsubprocesses spawned from other codepaths do or do not want to see\nthe one-shot custom configuration settings.  If that assumption\nturns out to be wrong and the processes spawned using the helper in\nvarious helper functions in submodule.c are the oddball cases that\nwant to see the custom configuration, then such a change would break\nexisting users.\n\nI _think_ the one in connect.c, which runs either the ssh transport\n(for which the processes that run on the other side in the other\nrepository won't be affected by our environment anyway) or the file\ntransport that runs another process and talks with it over a pipe is\nprobably OK if the configuration on the \"client\" side leaks through\nto the \"server\" side, e.g.\n\n    $ git -c advice.ignoredHook=false clone file:///the/repo.git/ here\n\nwould probably want the other end (i.e. the one that runs upload-pack\nin /the/repo.git/ directory) to see the one-shot configuration, too.\n\nI do not think it makes much difference to the use of local_repo_env\nin object-file.c::for_each_alternate_ref() either way; it could be\nused (via core.alternateRefsCommand) an arbitrary command in each\nalternate repository, but by default it runs for-each-ref in them,\nand I do not think of any configuration variables that would be\nuseful on \"the other side\".\n\nAnd I suspect that trailers.c::apply_command() excludes these\nenvironment variables just out of habit without much deep thinking.\nIt is not going in a different repository to run the command, and\nsantitizing the environment that pertains to this repository should\nnot have any meaningful effect [*].\n\nSo, I would not be surprised if it were a totally safe change, but I\nam not yet sure.\n\nThanks.\n\n\n\n[Footnote]\n\n* Christian CC'ed for 85039fb6 (trailer: execute command from\n  'trailer.<name>.command', 2014-10-13) that introduced the code\n  together with its use of local_repo_env[].\n"},{"id":"420126","messageId":"YFuHd1MMlJAvtdzb@coredump.intra.peff.net","threadId":"55374","inReplyTo":"xmqqim5h1otg.fsf@gitster.g","subject":"Re: Pass or not to pass config environment down...","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-03-24T18:39:51Z","receivedAt":"2021-03-24T18:41:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 23, 2021 at 03:35:07PM -0700, Junio C Hamano wrote:\n\n> > So, I think this direction is good. I imagine a full patch would\n> > include also dropping the specialized helper function that is no\n> > longer needed, and possibly adding new tests for the behavior of\n> > GIT_CONFIG_COUNT?\n> \n> Yeah, coding that is the easiest part.  Thinking through\n> ramifications of making (or not making) such a change is much\n> harder.\n> \n> I said \"assuming\" number of times, because I am not so sure if the\n> subprocesses spawned from other codepaths do or do not want to see\n> the one-shot custom configuration settings.  If that assumption\n> turns out to be wrong and the processes spawned using the helper in\n> various helper functions in submodule.c are the oddball cases that\n> want to see the custom configuration, then such a change would break\n> existing users.\n\nI think it really depends on the command being spawned. But keep in mind\nthat the local_repo_env list is not limited just to callers inside of\nGit. We expose it to the user via rev-parse, so scripts can do:\n\n  unset $(git rev-parse --local-env-vars)\n  cd /some/other/repo\n\nI'm hesitant to change the output there, since we don't know exactly how\nit's used in the wild[1]. Changing what our internal callers do is less\nrisky, though I'd generally avoid doing so unless there is a known\nbenefit. And I'm sure what the benefit is; I think this came up mostly\nbecause you were looking at harmonizing the behavior of the two config\nsystems (and I think that _is_ worth doing, but I'd probably choose the\nhistorical behavior for the new system).\n\nI also think it really depends on the specific config the user is\nexpecting to get passed. Remember we used to have a whitelist for \"this\nconfig is OK to pass to submodules\", but it was such a mess that we did\naway with it in 89044baa8b (submodule: stop sanitizing config options,\n2016-05-04).\n\n> I _think_ the one in connect.c, which runs either the ssh transport\n> (for which the processes that run on the other side in the other\n> repository won't be affected by our environment anyway) or the file\n> transport that runs another process and talks with it over a pipe is\n> probably OK if the configuration on the \"client\" side leaks through\n> to the \"server\" side, e.g.\n> \n>     $ git -c advice.ignoredHook=false clone file:///the/repo.git/ here\n> \n> would probably want the other end (i.e. the one that runs upload-pack\n> in /the/repo.git/ directory) to see the one-shot configuration, too.\n\nThat example is one of the reasons I prefer _not_ to pass config here.\nIt only works over local-process invocations! Not over ssh://, nor\ngit://, nor https://. Even though it will do what you want in this case,\nthe overall behavior is more confusing.\n\nThe more-consistent (or less inconsistent, perhaps) way is:\n\n  git clone -u 'git -c advice.ignoredHook=false upload-pack' \\\n    file:///the/repo.git\n\nwhich also works with ssh. It of course _doesn't_ work with other\nprotocols, but I think the technique at least makes it more clear why\nthat is the case (you do not get to specify arbitrary shell commands to\nhttps servers).\n\n> I do not think it makes much difference to the use of local_repo_env\n> in object-file.c::for_each_alternate_ref() either way; it could be\n> used (via core.alternateRefsCommand) an arbitrary command in each\n> alternate repository, but by default it runs for-each-ref in them,\n> and I do not think of any configuration variables that would be\n> useful on \"the other side\".\n> \n> And I suspect that trailers.c::apply_command() excludes these\n> environment variables just out of habit without much deep thinking.\n> It is not going in a different repository to run the command, and\n> santitizing the environment that pertains to this repository should\n> not have any meaningful effect [*].\n> \n> So, I would not be surprised if it were a totally safe change, but I\n> am not yet sure.\n\nMy suspicion is that for most cases, nobody cares that much either way\n(which is why we have not seen people ask \"hey, why is my config not\npassed down\" in any context _except_ submodules).\n\n-Peff\n\n[1] The one place I have used \"rev-parse --local-env-vars\" is in scripts\n    to handle alternates storage. When we \"sync\" from a fork to the\n    shared-object repo, there are some Git commands that must run in\n    one, and some in the other. Likewise, there is \"do maintenance\"\n    script which can be triggered from a fork, but then moves into the\n    shared repo. Some of those sites explicitly allow config by doing:\n\n      unset $(git rev-parse --local-env-vars | grep -v '^GIT_CONFIG')\n\n    because we want \"git -c pack.threads=17 do-the-maintenance\" to\n    respect that config after moving into the shared repo to do the\n    repack. But others don't. _Probably_ nothing would go too wrong if\n    we retained config, but I'd want to look at each. Which makes me\n    concerned that other scripts in the wild would likewise be\n    potentially surprised by this.\n"},{"id":"420702","messageId":"YGRb6CCH06rga6QO@ncase","threadId":"55374","inReplyTo":"YFuHd1MMlJAvtdzb@coredump.intra.peff.net","subject":"Re: Pass or not to pass config environment down...","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-03-31T11:24:24Z","receivedAt":"2021-03-31T11:26:19Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 24, 2021 at 02:39:51PM -0400, Jeff King wrote:\n> On Tue, Mar 23, 2021 at 03:35:07PM -0700, Junio C Hamano wrote:\n> \n> > > So, I think this direction is good. I imagine a full patch would\n> > > include also dropping the specialized helper function that is no\n> > > longer needed, and possibly adding new tests for the behavior of\n> > > GIT_CONFIG_COUNT?\n> > \n> > Yeah, coding that is the easiest part.  Thinking through\n> > ramifications of making (or not making) such a change is much\n> > harder.\n> > \n> > I said \"assuming\" number of times, because I am not so sure if the\n> > subprocesses spawned from other codepaths do or do not want to see\n> > the one-shot custom configuration settings.  If that assumption\n> > turns out to be wrong and the processes spawned using the helper in\n> > various helper functions in submodule.c are the oddball cases that\n> > want to see the custom configuration, then such a change would break\n> > existing users.\n> \n> I think it really depends on the command being spawned. But keep in mind\n> that the local_repo_env list is not limited just to callers inside of\n> Git. We expose it to the user via rev-parse, so scripts can do:\n> \n>   unset $(git rev-parse --local-env-vars)\n>   cd /some/other/repo\n> \n> I'm hesitant to change the output there, since we don't know exactly how\n> it's used in the wild[1]. Changing what our internal callers do is less\n> risky, though I'd generally avoid doing so unless there is a known\n> benefit. And I'm sure what the benefit is; I think this came up mostly\n> because you were looking at harmonizing the behavior of the two config\n> systems (and I think that _is_ worth doing, but I'd probably choose the\n> historical behavior for the new system).\n\nAgreed. I wasn't aware of this helper function at all, and aligning both\nconfig systems so they have the same behaviour there seems like the\nright thing to do to me.\n\n> I also think it really depends on the specific config the user is\n> expecting to get passed. Remember we used to have a whitelist for \"this\n> config is OK to pass to submodules\", but it was such a mess that we did\n> away with it in 89044baa8b (submodule: stop sanitizing config options,\n> 2016-05-04).\n\nThat also came to my mind while this thread. I can see why it would be\nuseful if e.g. `gc.auto=0` gets passed down to all subcommands spawned\nby git. But if the user for example injects remote configuration via\nconfig envvars, then it'd certainly be unexpected if submodules would\ntry to fetch from the same in-memory remote as the parent on a recursive\nfetch.\n\n> > I _think_ the one in connect.c, which runs either the ssh transport\n> > (for which the processes that run on the other side in the other\n> > repository won't be affected by our environment anyway) or the file\n> > transport that runs another process and talks with it over a pipe is\n> > probably OK if the configuration on the \"client\" side leaks through\n> > to the \"server\" side, e.g.\n> > \n> >     $ git -c advice.ignoredHook=false clone file:///the/repo.git/ here\n> > \n> > would probably want the other end (i.e. the one that runs upload-pack\n> > in /the/repo.git/ directory) to see the one-shot configuration, too.\n> \n> That example is one of the reasons I prefer _not_ to pass config here.\n> It only works over local-process invocations! Not over ssh://, nor\n> git://, nor https://. Even though it will do what you want in this case,\n> the overall behavior is more confusing.\n> \n> The more-consistent (or less inconsistent, perhaps) way is:\n> \n>   git clone -u 'git -c advice.ignoredHook=false upload-pack' \\\n>     file:///the/repo.git\n> \n> which also works with ssh. It of course _doesn't_ work with other\n> protocols, but I think the technique at least makes it more clear why\n> that is the case (you do not get to specify arbitrary shell commands to\n> https servers).\n> \n> > I do not think it makes much difference to the use of local_repo_env\n> > in object-file.c::for_each_alternate_ref() either way; it could be\n> > used (via core.alternateRefsCommand) an arbitrary command in each\n> > alternate repository, but by default it runs for-each-ref in them,\n> > and I do not think of any configuration variables that would be\n> > useful on \"the other side\".\n> > \n> > And I suspect that trailers.c::apply_command() excludes these\n> > environment variables just out of habit without much deep thinking.\n> > It is not going in a different repository to run the command, and\n> > santitizing the environment that pertains to this repository should\n> > not have any meaningful effect [*].\n> > \n> > So, I would not be surprised if it were a totally safe change, but I\n> > am not yet sure.\n> \n> My suspicion is that for most cases, nobody cares that much either way\n> (which is why we have not seen people ask \"hey, why is my config not\n> passed down\" in any context _except_ submodules).\n\nProbably not, but it may be a good idea to document config boundaries\nsuch that nobody is caught by surprise there.\n\nPatrick\n"}]}