{"thread":{"id":"54136","subject":"[PATCH] clone: add remote.cloneDefault config option","startedAt":"2020-08-26T15:45:57Z","lastAt":"2020-08-27T14:09:38Z","messageCount":7,"participants":["Sean Barag via GitGitGadget","Junio C Hamano","Derrick Stolee","Sean Barag"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"404527","messageId":"pull.710.git.1598456751674.gitgitgadget@gmail.com","threadId":"54136","inReplyTo":null,"subject":"[PATCH] clone: add remote.cloneDefault config option","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-08-26T15:45:51Z","receivedAt":"2020-08-26T15:45:57Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"From: Sean Barag <sean@barag.org>\n\nWhile the default remote name of `origin` can be overridden both\npre-clone (with `git clone --origin foo`) and post-clone (with `git\nremote rename origin foo`), it'd be handy to have a configurable\nsystem-wide default for clones!  This commit implements\n`remote.cloneDefault` as a parallel to `remote.pushDefault`,\nwith prioritized name resolution:\n\n1. (Highest priority) `git clone`'s `-o` option\n2. `git config`'s `remote.cloneDefault` value\n3. `origin`\n\nThere should be no impact for existing users, as it's pretty unlikely\nthat anyone's already configured `remote.cloneDefault` and the porcelain\nhasn't changed (as best I can tell at least!).\n\nSigned-off-by: Sean Barag <sean@barag.org>\n---\n    clone: add remote.cloneDefault config option\n    \n    While the default remote name of origin can be overridden both pre-clone\n    (with git clone --origin foo) and post-clone (with git remote rename\n    origin foo), it'd be handy to have a configurable system-wide default\n    for clones! This implementsremote.cloneDefault as a parallel to \n    remote.pushDefault, with prioritized name resolution:\n    \n     1. (Highest priority) git clone's -o option\n     2. git config's remote.cloneDefault value\n     3. origin\n    \n    There should be no impact for existing users, as it's pretty unlikely\n    that anyone's already configured remote.cloneDefault and the porcelain\n    hasn't changed (as best I can tell at least!).\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-710%2Fsjbarag%2Fadd-remote.cloneDefault-config-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-710/sjbarag/add-remote.cloneDefault-config-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/710\n\n Documentation/config/remote.txt |  4 ++++\n Documentation/git-clone.txt     |  5 +++--\n builtin/clone.c                 | 34 +++++++++++++++++++++++++++++++--\n t/t5606-clone-options.sh        | 14 ++++++++++++++\n 4 files changed, 53 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\nindex a8e6437a90..debb21ecbf 100644\n--- a/Documentation/config/remote.txt\n+++ b/Documentation/config/remote.txt\n@@ -3,6 +3,10 @@ remote.pushDefault::\n \t`branch.<name>.remote` for all branches, and is overridden by\n \t`branch.<name>.pushRemote` for specific branches.\n \n+remote.cloneDefault::\n+\tThe name of the remote to create during `git clone <url>`.\n+\tDefaults to \"origin\".\n+\n remote.<name>.url::\n \tThe URL of a remote repository.  See linkgit:git-fetch[1] or\n \tlinkgit:git-push[1].\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex c898310099..2e101ba4f4 100644\n--- a/Documentation/git-clone.txt\n+++ b/Documentation/git-clone.txt\n@@ -183,8 +183,9 @@ objects from the source repository into a pack in the cloned repository.\n \n -o <name>::\n --origin <name>::\n-\tInstead of using the remote name `origin` to keep track\n-\tof the upstream repository, use `<name>`.\n+\tThe remote name used to keep track of the upstream repository.\n+\tOverrides `remote.cloneDefault` from the config, and defaults\n+\tto `origin`.\n \n -b <name>::\n --branch <name>::\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex b087ee40c2..b0dbb848c6 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -941,6 +941,29 @@ static int path_exists(const char *path)\n \treturn !stat(path, &sb);\n }\n \n+struct clone_default_info\n+{\n+\tenum config_scope scope;\n+\tstruct strbuf remote_name;\n+\tint linenr;\n+};\n+\n+static int config_read_clone_default(const char *key, const char *value,\n+\tvoid *cb)\n+{\n+\tstruct clone_default_info* info = cb;\n+\tif (strcmp(key, \"remote.clonedefault\") || !value) {\n+\t\treturn 0;\n+\t}\n+\n+\tinfo->scope = current_config_scope();\n+\tstrbuf_reset(&info->remote_name);\n+\tstrbuf_addstr(&info->remote_name, value);\n+\tinfo->linenr = current_config_line();\n+\n+\treturn 0;\n+}\n+\n int cmd_clone(int argc, const char **argv, const char *prefix)\n {\n \tint is_bundle = 0, is_local;\n@@ -992,8 +1015,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\toption_no_checkout = 1;\n \t}\n \n-\tif (!option_origin)\n-\t\toption_origin = \"origin\";\n+\tif (!option_origin) {\n+\t\tstruct clone_default_info clone_default = { CONFIG_SCOPE_UNKNOWN, STRBUF_INIT, -1 };\n+\t\tgit_config(config_read_clone_default, &clone_default);\n+\t\tif (strcmp(\"\", (const char*) clone_default.remote_name.buf))\n+\t\t\toption_origin = clone_default.remote_name.buf;\n+\t\telse\n+\t\t\toption_origin = \"origin\";\n+\t}\n+\n \n \trepo_name = argv[0];\n \ndiff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh\nindex e69427f881..8aac67b385 100755\n--- a/t/t5606-clone-options.sh\n+++ b/t/t5606-clone-options.sh\n@@ -19,6 +19,20 @@ test_expect_success 'clone -o' '\n \n '\n \n+test_expect_success 'clone respects remote.cloneDefault' '\n+\n+\tgit -c remote.cloneDefault=bar clone parent clone-config &&\n+\t(cd clone-config && git rev-parse --verify refs/remotes/bar/master)\n+\n+'\n+\n+test_expect_success 'clone chooses correct remote name' '\n+\n+\tgit -c remote.cloneDefault=bar clone -o foo parent clone-o-and-config &&\n+\t(cd clone-o-and-config && git rev-parse --verify refs/remotes/foo/master)\n+\n+'\n+\n test_expect_success 'redirected clone does not show progress' '\n \n \tgit clone \"file://$(pwd)/parent\" clone-redirected >out 2>err &&\n\nbase-commit: e9b77c84a0a0df029f2a3a8114e9f22186e7da24\n-- \ngitgitgadget\n"},{"id":"404542","messageId":"xmqqlfi1utwi.fsf@gitster.c.googlers.com","threadId":"54136","inReplyTo":"pull.710.git.1598456751674.gitgitgadget@gmail.com","subject":"Re: [PATCH] clone: add remote.cloneDefault config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T18:46:53Z","receivedAt":"2020-08-26T18:47:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Sean Barag via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Sean Barag <sean@barag.org>\n>\n> While the default remote name of `origin` can be overridden both\n> pre-clone (with `git clone --origin foo`) and post-clone (with `git\n> remote rename origin foo`), it'd be handy to have a configurable\n> system-wide default for clones!  \n\nI doubt it is handy enough to deserve an explamation point.  Replace\nit with a plain-vanilla full-stop instead.\n\nI however tend to agree that, even evidenced by the manual page\ndescription of \"git clone\", i.e.\n\n    -o <name>::\n    --origin <name>::\n            Instead of using the remote name `origin` to keep track\n            of the upstream repository, use `<name>`.\n\nthat it is understandable if many users and projects wish to call it\n\"upstream\".\n\n> This commit implements\n> `remote.cloneDefault` as a parallel to `remote.pushDefault`,\n> with prioritized name resolution:\n\nI highly doubt that .cloneDefault is a good name.  After reading\nonly the title of the patch e-mail, i.e. when the only available\ninformation on the change available to me was the name of the\nconfiguration variable and the fact that it pertains to the command\n\"git clone\", I thought it is to specify a URL, from which \"git\nclone\" without the URL would clone from that single repository.\n\nAnd the name will cause the same misunderstanding to normal users,\nnot just to reviewers of your patch, after this change hits a future\nGit release.\n\nTaking a parallel from init.defaultBranchName, I would probably call\nit clone.defaultUpstreamName if I were writing this feature.\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index b087ee40c2..b0dbb848c6 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -941,6 +941,29 @@ static int path_exists(const char *path)\n>  \treturn !stat(path, &sb);\n>  }\n>  \n> +struct clone_default_info\n> +{\n> +\tenum config_scope scope;\n> +\tstruct strbuf remote_name;\n> +\tint linenr;\n> +};\n> +\n> +static int config_read_clone_default(const char *key, const char *value,\n> +\tvoid *cb)\n> +{\n> +\tstruct clone_default_info* info = cb;\n> +\tif (strcmp(key, \"remote.clonedefault\") || !value) {\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tinfo->scope = current_config_scope();\n> +\tstrbuf_reset(&info->remote_name);\n> +\tstrbuf_addstr(&info->remote_name, value);\n> +\tinfo->linenr = current_config_line();\n> +\n> +\treturn 0;\n> +}\n\nThis feels way overkill and insufficient at the same time.  It does\nnot need scope, it does not need linenr, and we already have a place\nto store end-user specified name for the upstream in the form of the\nvariable option_origin.  And the code is not diagnosing any error.\n\nstatic int git_clone_config(const char *k, const char *v, void *cb)\n{\n\tif (option_origin)\n\t\treturn 0; /* ignore -- the user gave us an option */\n\n\tif (!strcmp(k, \"clone.defaultupstreamname\")) {\n\t\tif (!v)\n\t\t\treturn config_error_nonbool(k);\n\t\tif (strchr(v, '/') || check_refname_format(v, REFNAME_ALLOW_ONELEVEL))\n                \treturn error(_(\"invalid upstream name '%s'\"), v);\n\t\toption_origin = xstrdup(v);\n\t\treturn 0;\n\t}\n\treturn 0;\n}\n\nwould be sufficient, and at the same time makes sure it rejects\nnames like 'o..ri..gin', 'o/ri/gin', etc.\n\n> @@ -992,8 +1015,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\toption_no_checkout = 1;\n>  \t}\n>  \n> -\tif (!option_origin)\n> -\t\toption_origin = \"origin\";\n> +\tif (!option_origin) {\n> +\t\tstruct clone_default_info clone_default = { CONFIG_SCOPE_UNKNOWN, STRBUF_INIT, -1 };\n> +\t\tgit_config(config_read_clone_default, &clone_default);\n> +\t\tif (strcmp(\"\", (const char*) clone_default.remote_name.buf))\n> +\t\t\toption_origin = clone_default.remote_name.buf;\n> +\t\telse\n> +\t\t\toption_origin = \"origin\";\n> +\t}\n> +\n\nIt is somewhat sad that we have the git_config(git_default_config)\ncall so late in the control flow.  I wonder if we can update the\nstart-up sequence to match the usual flow, e.g. these things happen\nin the following order in a normal program.\n\n    - variables are given their default value.\n\n    - call git_config(git_commandspecific_config, ...) early in the\n      program.\n\n      - git_commandspecific_config() interprets the command specific\n        variables and passes everything else to git_default_config()\n\n      variables like option_origin are given values of configuration\n      variable at this point.\n\n    - call parse_options() next, which may override the variables\n      from the value on the command line.\n\n    - main control flow uses the variable.  \"Command-line wins over\n      configuration which wins over the default\" falls out naturally.\n\nOne oddity \"git clone\" has is that it wants to delay the reading of\nconfiguration files (they are read only once, and second and\nsubsequent git_config() calls will reuse what was read before [*])\nso that it can read what clone.c::write_config() wrote, so if we were\nto \"fix\" the start-up sequence to match the usual flow, we need to\nsatisfy what that odd arrangement wanted to achieve in some other\nway (e.g. feed what is in option_config to git_default_config\nourselves, without using git_config(), as part of the \"main control\nflow uses the variable\" part), but it should be doable.\n\n\t[*Side note*].  The above means that this patch, even when\n\tthe configuration variable does not give upstream name, may\n\tbreak the option_config feature by breaking the second call\n\tto git_config().  We need to have a test for that.\n\n> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh\n> index e69427f881..8aac67b385 100755\n> --- a/t/t5606-clone-options.sh\n> +++ b/t/t5606-clone-options.sh\n> @@ -19,6 +19,20 @@ test_expect_success 'clone -o' '\n>  \n>  '\n>  \n> +test_expect_success 'clone respects remote.cloneDefault' '\n> +\n> +\tgit -c remote.cloneDefault=bar clone parent clone-config &&\n> +\t(cd clone-config && git rev-parse --verify refs/remotes/bar/master)\n> +\n> +'\n> +\n> +test_expect_success 'clone chooses correct remote name' '\n> +\n> +\tgit -c remote.cloneDefault=bar clone -o foo parent clone-o-and-config &&\n> +\t(cd clone-o-and-config && git rev-parse --verify refs/remotes/foo/master)\n> +\n> +'\n\nThese two are \"showing off my shiny new toy\" tests, which are\nneeded, but we also need negative tests where the shiny new toy does\nnot kick in when it should not.  For example\n\n\tgit -c remote.cloneDefault=\"bad.../...name\" clone parent\n\nshould fail, no?\n\nThanks.\n"},{"id":"404543","messageId":"eeebff99-d585-5575-009e-83bfef5294e3@gmail.com","threadId":"54136","inReplyTo":"xmqqlfi1utwi.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] clone: add remote.cloneDefault config option","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-08-26T19:04:32Z","receivedAt":"2020-08-26T19:04:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/26/2020 2:46 PM, Junio C Hamano wrote:\n> \"Sean Barag via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>> This commit implements\n>> `remote.cloneDefault` as a parallel to `remote.pushDefault`,\n>> with prioritized name resolution:\n> \n> I highly doubt that .cloneDefault is a good name.  After reading\n> only the title of the patch e-mail, i.e. when the only available\n> information on the change available to me was the name of the\n> configuration variable and the fact that it pertains to the command\n> \"git clone\", I thought it is to specify a URL, from which \"git\n> clone\" without the URL would clone from that single repository.\n> \n> And the name will cause the same misunderstanding to normal users,\n> not just to reviewers of your patch, after this change hits a future\n> Git release.\n> \n> Taking a parallel from init.defaultBranchName, I would probably call\n> it clone.defaultUpstreamName if I were writing this feature.\n\nI was thinking \"clone.defaultRemoteName\" makes it clear we are naming\nthe remote for the provided <url> in the command. Having\n\n\tclone.defaultUpstreamName = upstream\n\nmay look a bit confusing, while\n\n\tclone.defaultRemoteName = upstream\n\nmakes it a bit clearer that we will set\n\n\tremote.upstream.url = <url>\n\n>> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh\n>> index e69427f881..8aac67b385 100755\n>> --- a/t/t5606-clone-options.sh\n>> +++ b/t/t5606-clone-options.sh\n>> @@ -19,6 +19,20 @@ test_expect_success 'clone -o' '\n>>  \n>>  '\n>>  \n>> +test_expect_success 'clone respects remote.cloneDefault' '\n>> +\n>> +\tgit -c remote.cloneDefault=bar clone parent clone-config &&\n>> +\t(cd clone-config && git rev-parse --verify refs/remotes/bar/master)\n>> +\n>> +'\n>> +\n>> +test_expect_success 'clone chooses correct remote name' '\n>> +\n>> +\tgit -c remote.cloneDefault=bar clone -o foo parent clone-o-and-config &&\n>> +\t(cd clone-o-and-config && git rev-parse --verify refs/remotes/foo/master)\n>> +\n>> +'\n> \n> These two are \"showing off my shiny new toy\" tests, which are\n> needed, but we also need negative tests where the shiny new toy does\n> not kick in when it should not.  For example\n> \n> \tgit -c remote.cloneDefault=\"bad.../...name\" clone parent\n> \n> should fail, no?\n\nThis is an important suggestion.\n\nThanks,\n-Stolee\n\n"},{"id":"404547","messageId":"xmqqeentuqk5.fsf@gitster.c.googlers.com","threadId":"54136","inReplyTo":"eeebff99-d585-5575-009e-83bfef5294e3@gmail.com","subject":"Re: [PATCH] clone: add remote.cloneDefault config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T19:59:06Z","receivedAt":"2020-08-26T19:59:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> On 8/26/2020 2:46 PM, Junio C Hamano wrote:\n>> \"Sean Barag via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>>> This commit implements\n>>> `remote.cloneDefault` as a parallel to `remote.pushDefault`,\n>>> with prioritized name resolution:\n>> \n>> I highly doubt that .cloneDefault is a good name.  After reading\n>> only the title of the patch e-mail, i.e. when the only available\n>> information on the change available to me was the name of the\n>> configuration variable and the fact that it pertains to the command\n>> \"git clone\", I thought it is to specify a URL, from which \"git\n>> clone\" without the URL would clone from that single repository.\n>> \n>> And the name will cause the same misunderstanding to normal users,\n>> not just to reviewers of your patch, after this change hits a future\n>> Git release.\n>> \n>> Taking a parallel from init.defaultBranchName, I would probably call\n>> it clone.defaultUpstreamName if I were writing this feature.\n>\n> I was thinking \"clone.defaultRemoteName\" makes it clear we are naming\n> the remote for the provided <url> in the command.\n\nI 100% agree that defaultremotename is much better.\n\n>> ...  For example\n>> \n>> \tgit -c remote.cloneDefault=\"bad.../...name\" clone parent\n>> \n>> should fail, no?\n>\n> This is an important suggestion.\n\nTo be fair, the current code does not handle the \"--origin\" command\nline option not so carefully.\n\nBack when the command was scripted, e.g. 47874d6d (revamp git-clone\n(take #2)., 2006-03-21), had both ref-format check and */*\nmulti-level check, and these checks been retained throughout its\nlife until 8434c2f1 (Build in clone, 2008-04-27) rewrote the whole\nthing while discarding these checks for --origin=bad.../...name\n\nIt would make an excellent #leftoverbits or #microproject.\n\nThanks.\n"},{"id":"404570","messageId":"20200827033854.130005-1-sean@barag.org","threadId":"54136","inReplyTo":"xmqqeentuqk5.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] clone: add remote.cloneDefault config option","fromName":"Sean Barag","fromEmail":"sean@barag.org","sentAt":"2020-08-27T03:38:54Z","receivedAt":"2020-08-27T03:39:35Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"> Derrick Stolee <stolee@gmail.com> writes:\n> \n>> On 8/26/2020 2:46 PM, Junio C Hamano wrote:\n>>> \"Sean Barag via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>>>> This commit implements\n>>>> `remote.cloneDefault` as a parallel to `remote.pushDefault`,\n>>>> with prioritized name resolution:\n>>> \n>>> I highly doubt that .cloneDefault is a good name.  After reading\n>>> only the title of the patch e-mail, i.e. when the only available\n>>> information on the change available to me was the name of the\n>>> configuration variable and the fact that it pertains to the command\n>>> \"git clone\", I thought it is to specify a URL, from which \"git\n>>> clone\" without the URL would clone from that single repository.\n>>> \n>>> And the name will cause the same misunderstanding to normal users,\n>>> not just to reviewers of your patch, after this change hits a future\n>>> Git release.\n>>> \n>>> Taking a parallel from init.defaultBranchName, I would probably call\n>>> it clone.defaultUpstreamName if I were writing this feature.\n>>\n>> I was thinking \"clone.defaultRemoteName\" makes it clear we are naming\n>> the remote for the provided <url> in the command.\n>\n>I 100% agree that defaultremotename is much better.\n\nPerfect, I'll move forward with `clone.defaultRemoteName`.  Thanks for\nthe recommendation.  This would be the first config variable inside\nthe a \"clone\" subsection -- is there anything special that needs to\nhappen when a new subsection is added?\n\n>>>> ...  For example\n>>> \n>>> \tgit -c remote.cloneDefault=\"bad.../...name\" clone parent\n>>> \n>>> should fail, no?\n>>\n>> This is an important suggestion.\n>\n> To be fair, the current code does not handle the \"--origin\" command\n> line option not so carefully.\n\nAgreed - I'm sorry for not including those tests.  They'll be present\nin v2.  I'll be sure to include some validation for\n`clone.defaultRemoteName` within `git_config` as well.\n\n> It is somewhat sad that we have the git_config(git_default_config)\n> call so late in the control flow.  I wonder if we can update the\n> start-up sequence to match the usual flow\n> ...\n> One oddity \"git clone\" has is that it wants to delay the reading of\n> configuration files (they are read only once, and second and\n> subsequent git_config() calls will reuse what was read before [*]) so\n> that it can read what clone.c::write_config() wrote, so if we were to\n> \"fix\" the start-up sequence to match the usual flow, we need to\n> satisfy what that odd arrangement wanted to achieve in some other way\n> (e.g. feed what is in option_config to git_default_config ourselves,\n> without using git_config(), as part of the \"main control flow uses the\n> variable\" part), but it should be doable.\n\nSounds like a pretty big change! I'm willing to take a crack at it,\nbut given that this is my first patch I'm frankly a bit intimidated :)\nHow would you feel about that being a separate patch?\n\nThanks for all the guidance, folks.\nSean\n"},{"id":"404573","messageId":"xmqqpn7csoqd.fsf@gitster.c.googlers.com","threadId":"54136","inReplyTo":"20200827033854.130005-1-sean@barag.org","subject":"Re: [PATCH] clone: add remote.cloneDefault config option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-27T04:21:30Z","receivedAt":"2020-08-27T04:21:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sean Barag <sean@barag.org> writes:\n\n>> It is somewhat sad that we have the git_config(git_default_config)\n>> call so late in the control flow.  I wonder if we can update the\n>> start-up sequence to match the usual flow\n>> ...\n>> One oddity \"git clone\" has is that it wants to delay the reading of\n>> configuration files (they are read only once, and second and\n>> subsequent git_config() calls will reuse what was read before [*]) so\n>> that it can read what clone.c::write_config() wrote, so if we were to\n>> \"fix\" the start-up sequence to match the usual flow, we need to\n>> satisfy what that odd arrangement wanted to achieve in some other way\n>> (e.g. feed what is in option_config to git_default_config ourselves,\n>> without using git_config(), as part of the \"main control flow uses the\n>> variable\" part), but it should be doable.\n>\n> Sounds like a pretty big change! I'm willing to take a crack at it,\n> but given that this is my first patch I'm frankly a bit intimidated :)\n> How would you feel about that being a separate patch?\n\nIt definitely needs to be a separate patch.  \n\nIn order to realize any new feature that needs to read the existing\n(i.e. per-machine or per-user) configuration files to affect the\nbehaviour of \"git clone\", whether it is the \"give default to\n--origin option\" or any other thing, first needs to fix the start-up\nsequence so that the configuration is read once before we process\ncommand line options, which is the norm.  Only after that is done,\nwe can build the clone.defaultRemoteName and other features that\nwould be affected by the settings of clone.* variables on top, and\nit is not wise to mix the foundation with a new feature that uses\nthe foundation.  So I would imagine it would at least be 3-patch\nseries:\n\n - [1/3] would be to whip the start-up sequence into the normal\n   order.  This may be the most tricky part.  I identified that the\n   handling of option_config might need adjustment, but there may be\n   others.  This may not need any new tests, but if the existing\n   tests for \"clone -c var=val\" do not catch breakage when we\n   naively move the git_config(git_default_config) call early\n   without doing any other adjustment, we might need to add more\n   tests to cover the option.  If we find things other than\n   option_config needs adjustment, they also need test coverage.\n\n - [2/3] would be to tighten the error checking of option_origin\n   that was lost when the command was reimplemented in C (already\n   discussed).  This would need new tests to see \"--origin $bogus\"\n   command line option is flagged as an error.\n\n - [3/3] would be to read option_origin from the configuration file.\n   The start-up sequence fixed by [1/3] would allow us to write the\n   config callback in a more natural way, compared to what you wrote\n   and what I suggested to rewrite.  Error checking code in [2/3]\n   would directly be reusable in the code added in this step.  We'd\n   need tests similar to we add in [2/3] for the configuration, and\n   combination of configuration and command line (you already wrote\n   most of these).\n\nThanks.\n\n\n"},{"id":"404603","messageId":"20200827140045.470411-1-sean@barag.org","threadId":"54136","inReplyTo":"xmqqpn7csoqd.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] clone: add remote.cloneDefault config option","fromName":"Sean Barag","fromEmail":"sean@barag.org","sentAt":"2020-08-27T14:00:46Z","receivedAt":"2020-08-27T14:09:38Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"> In order to realize any new feature that needs to read the existing\n> (i.e. per-machine or per-user) configuration files to affect the\n> behaviour of \"git clone\", whether it is the \"give default to\n> --origin option\" or any other thing, first needs to fix the start-up\n> sequence so that the configuration is read once before we process\n> command line options, which is the norm.  Only after that is done,\n> we can build the clone.defaultRemoteName and other features that\n> would be affected by the settings of clone.* variables on top, and\n> it is not wise to mix the foundation with a new feature that uses\n> the foundation.\n\nMakes sense!  Thanks for all your help -- I _really_ appreciate it.\nI'll give it a try over the next few days.\n"}]}