{"thread":{"id":"54884","subject":"[PATCH] git-remote: load config whenever create_symref() might be called","startedAt":"2020-12-26T13:55:20Z","lastAt":"2020-12-27T06:26:08Z","messageCount":5,"participants":["Jérémie Detrey","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"413016","messageId":"20201226134420.722643-1-Jeremie.Detrey@altu.fr","threadId":"54884","inReplyTo":null,"subject":"[PATCH] git-remote: load config whenever create_symref() might be called","fromName":"Jérémie Detrey","fromEmail":"jeremie.detrey@altu.fr","sentAt":"2020-12-26T13:44:20Z","receivedAt":"2020-12-26T13:55:20Z","isPatch":true,"sender":{"key":"jeremie.detrey@altu.fr","avatar":null},"body":"Currently, the calls to `create_symref()` do not honor the\n`core.logAllRefUpdates` nor the `user.{name,email}` configuration\nsettings, and therefore write reflogs even when explicitly asked not\nto, and ignore the configured user identity.\n\nThis happens on `git remote add --fetch`, `git remote set-head`, and\n`git remote rename`: these are the three commands which may create\na symbolic-ref for the remote's HEAD.\n\nA call to `git_config(git_default_config, NULL);` for these three\ncommands is enough to load the necessary configuration beforehand.\n\nThe call to `git_config()` was inserted right after command-line\nprocessing for these three commands. One might also decide to push\nit closer to the actual call to `create_symref()`, as it is not\nneeded elsewhere, but it made more sense not to have it buried into\nnested `if` conditions.\n\nSigned-off-by: Jérémie Detrey \n---\n builtin/remote.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 64b4b551eb..f797fc3f65 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -190,6 +190,8 @@ static int add(int argc, const char **argv)\n \tname = argv[0];\n \turl = argv[1];\n \n+\tgit_config(git_default_config, NULL);\n+\n \tremote = remote_get(name);\n \tif (remote_is_configured(remote, 1))\n \t\tdie(_(\"remote %s already exists.\"), name);\n@@ -685,6 +687,8 @@ static int mv(int argc, const char **argv)\n \trename.new_name = argv[2];\n \trename.remote_branches = &remote_branches;\n \n+\tgit_config(git_default_config, NULL);\n+\n \toldremote = remote_get(rename.old_name);\n \tif (!remote_is_configured(oldremote, 1))\n \t\tdie(_(\"No such remote: '%s'\"), rename.old_name);\n@@ -1326,6 +1330,8 @@ static int set_head(int argc, const char **argv)\n \tif (argc)\n \t\tstrbuf_addf(&buf, \"refs/remotes/%s/HEAD\", argv[0]);\n \n+\tgit_config(git_default_config, NULL);\n+\n \tif (!opt_a && !opt_d && argc == 2) {\n \t\thead_name = xstrdup(argv[1]);\n \t} else if (opt_a && !opt_d && argc == 1) {\n-- \n2.29.2\n\n"},{"id":"413017","messageId":"xmqqeejcqlpq.fsf@gitster.c.googlers.com","threadId":"54884","inReplyTo":"20201226134420.722643-1-Jeremie.Detrey@altu.fr","subject":"Re: [PATCH] git-remote: load config whenever create_symref() might be called","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-26T21:52:17Z","receivedAt":"2020-12-26T21:57:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jérémie Detrey writes:\n\n> Currently, the calls to `create_symref()` do not honor the\n> `core.logAllRefUpdates` nor the `user.{name,email}` configuration\n> settings, and therefore write reflogs even when explicitly asked not\n> to, and ignore the configured user identity.\n>\n> This happens on `git remote add --fetch`, `git remote set-head`, and\n> `git remote rename`: these are the three commands which may create\n> a symbolic-ref for the remote's HEAD.\n\nGood problem description.\n\nThe usual way we structure the program start-up is do things in the\nfollowing order:\n\n - have the hard-coded built-in default in the form of file-scope\n static variable definition with initial values; remote.c::value\n is an example that by default we are not verbose unless anybody\n else (below) says otherwise.\n\n - call git_config() to read and set configured defaults, which\n would override the hard-coded built-in default values.\n\n - call parse_options(), or other use some other means, to parse the\n command line options to further override the values that may have\n been set by reading the configuration files.\n\nMy gut feeling says that the best place to call git_config() on\ngit_default_config would actually be in cmd_remote(), immediately\nbefore we call parse_options() to parse the main options shared by\nall subcommands of \"git remote\". Is there a reason why codepaths\nother than the three you singled out should *not* be affected by the\nbasic set of configuration variables?\n\nThanks.\n"},{"id":"413019","messageId":"X+e48UEE4wf3fMNI@pica","threadId":"54884","inReplyTo":"xmqqeejcqlpq.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] git-remote: load config whenever create_symref() might be called","fromName":"Jérémie Detrey","fromEmail":"jeremie.detrey@altu.fr","sentAt":"2020-12-26T22:28:01Z","receivedAt":"2020-12-26T22:28:55Z","isPatch":true,"sender":{"key":"jeremie.detrey@altu.fr","avatar":null},"body":"Hi,\n\nThanks for the feedback!\n\nJunio C Hamano:\n> My gut feeling says that the best place to call git_config() on\n> git_default_config would actually be in cmd_remote(), immediately\n> before we call parse_options() to parse the main options shared by\n> all subcommands of \"git remote\". Is there a reason why codepaths\n> other than the three you singled out should *not* be affected by the\n> basic set of configuration variables?\n\nNo, none that I can think of. I'll leave more knowledgeable people\nthan me confirm this, but if that's the way to go, I'll be happy to\nsubmit a revised patch following your suggestion :)\n\nCheers,\nJérémie.\n"},{"id":"413020","messageId":"CAPig+cQqObQj6djn_EvfurbwsEKuU2fB+GZ8EE1HA-Hn1hUkuQ@mail.gmail.com","threadId":"54884","inReplyTo":"X+e48UEE4wf3fMNI@pica","subject":"Re: [PATCH] git-remote: load config whenever create_symref() might be called","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-27T00:34:30Z","receivedAt":"2020-12-27T00:40:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Dec 26, 2020 at 5:31 PM Jérémie Detrey wrote:\n> Junio C Hamano:\n> > My gut feeling says that the best place to call git_config() on\n> > git_default_config would actually be in cmd_remote(), immediately\n> > before we call parse_options() to parse the main options shared by\n> > all subcommands of \"git remote\". Is there a reason why codepaths\n> > other than the three you singled out should *not* be affected by the\n> > basic set of configuration variables?\n>\n> No, none that I can think of. I'll leave more knowledgeable people\n> than me confirm this, but if that's the way to go, I'll be happy to\n> submit a revised patch following your suggestion :)\n\nI took a quick look at builtin/remote.c. It appears that several of\nthe subcommands already make their own calls to git_config(), each\nwith its own special-purpose `config_fn_t` as argument. Jérémie's\npatch, which invokes git_config() for specific cases, therefore, is\nconsistent with existing practice in that file. Taking the approach of\ncalling git_config() once before parse_options() should be possible\nbut would require more surgery. The end result after surgery likely\nwould be more maintainable going forward, but the localized fixes his\npatch makes might be reasonable in the short term. (But I'm not an\narea expert, so don't take my word for it.)\n"},{"id":"413021","messageId":"xmqqa6tzrcnn.fsf@gitster.c.googlers.com","threadId":"54884","inReplyTo":"CAPig+cQqObQj6djn_EvfurbwsEKuU2fB+GZ8EE1HA-Hn1hUkuQ@mail.gmail.com","subject":"Re: [PATCH] git-remote: load config whenever create_symref() might be called","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-27T06:22:36Z","receivedAt":"2020-12-27T06:26:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> ... The end result after surgery likely\n> would be more maintainable going forward, but the localized fixes his\n> patch makes might be reasonable in the short term.\n\nOK, I guess.\n\nThanks.\n"}]}