{"thread":{"id":"54232","subject":"[PATCH 0/4] clone: allow configurable default for -o/--origin","startedAt":"2020-09-11T18:25:37Z","lastAt":"2020-10-02T12:57:14Z","messageCount":49,"participants":["Sean Barag via GitGitGadget","Derrick Stolee","Junio C Hamano","Jeff King","Eric Sunshine","Taylor Blau","Sean Barag","Andrei Rybak"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"405429","messageId":"pull.727.git.1599848727.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":null,"subject":"[PATCH 0/4] clone: allow configurable default for -o/--origin","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-11T18:25:23Z","receivedAt":"2020-09-11T18:25:37Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"Took another pass at supporting a configurable default for-o/--origin, this\ntime following Junio's suggestions from a previous approach as much as\npossible [1]. Unfortunately, Johannes mentioned that --template can write\nnew config values that aren't automatically merged without re-calling \ngit_config. There doesn't appear to be a way around this without rewriting\nsignificant amounts of init and config logic across the codebase.\n\nWhile this could have been v2 of the original patchset, it's diverged so\ndrastically from the original that it likely warrants its own root thread.\nIf that's not appropriate though, I'd be happy to restructure!\n\nThanks for all the advice Junio and Johannes! This feels significantly\nbetter than my first attempt.\n\n[1] \nhttps://lore.kernel.org/git/pull.710.git.1598456751674.gitgitgadget@gmail.com/\n[2] https://github.com/gitgitgadget/git/pull/727#issuecomment-689740195\n\nSean Barag (4):\n clone: add tests for --template and some disallowed option pairs\n clone: call git_config before parse_options\n clone: validate --origin option before use\n clone: allow configurable default for `-o`/`--origin`\n\n Documentation/config.txt | 2 +\n Documentation/config/clone.txt | 5 +++\n Documentation/git-clone.txt | 5 ++-\n builtin/clone.c | 65 ++++++++++++++++++++++++++-------\n t/t5606-clone-options.sh | 67 ++++++++++++++++++++++++++++++++++\n 5 files changed, 128 insertions(+), 16 deletions(-)\n create mode 100644 Documentation/config/clone.txt\n\n\nbase-commit: 54e85e7af1ac9e9a92888060d6811ae767fea1bc\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-727%2Fsjbarag%2Fclone_add-configurable-default-for--o-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-727/sjbarag/clone_add-configurable-default-for--o-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/727\n-- \ngitgitgadget\n"},{"id":"405430","messageId":"51ef776f8523d29dfe03d15f0d1958f5c456c057.1599848727.git.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":"pull.727.git.1599848727.gitgitgadget@gmail.com","subject":"[PATCH 2/4] clone: call git_config before parse_options","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-11T18:25:25Z","receivedAt":"2020-09-11T18:25:42Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"From: Sean Barag \n\nWhile Junio's request [1] was to avoids the unusual \"write config then\nimmediately read it\" pattern that exists in `cmd_clone`, Johannes\nmentioned that --template can write new config values that aren't\nautomatically included in the environment [2]. This requires a config\nre-read after `init_db` is called.\n\nMoving the initial config up does allow settings from config to be\noverwritten by ones provided via CLI options in a more natural way\nthough, so that part of Junio's suggestion remains.\n\n[1] https://lore.kernel.org/git/pull.710.git.1598456751674.gitgitgadget@gmail.com/\n[2] https://github.com/gitgitgadget/git/pull/727#issuecomment-689740195\n\nSigned-off-by: Sean Barag \nThanks-to: Junio C Hamano \nThanks-to: Johannes Schindelin \n---\n builtin/clone.c | 26 +++++++++++++++++++++++++-\n 1 file changed, 25 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex b087ee40c2..bf095815f0 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -851,8 +851,21 @@ static int checkout(int submodule_progress)\n \treturn err;\n }\n \n+static int git_clone_config(const char *k, const char *v, void *cb)\n+{\n+\treturn git_default_config(k, v, cb);\n+}\n+\n static int write_one_config(const char *key, const char *value, void *data)\n {\n+\t/*\n+\t * give git_config_default a chance to write config values back to the environment, since\n+\t * git_config_set_multivar_gently only deals with config-file writes\n+\t */\n+\tint apply_failed = git_default_config(key, value, data);\n+\tif (apply_failed)\n+\t\treturn apply_failed;\n+\n \treturn git_config_set_multivar_gently(key,\n \t\t\t\t\t value ? value : \"true\",\n \t\t\t\t\t CONFIG_REGEX_NONE, 0);\n@@ -964,6 +977,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstruct strvec ref_prefixes = STRVEC_INIT;\n \n \tpacket_trace_identity(\"clone\");\n+\n+\tgit_config(git_clone_config, NULL);\n+\n \targc = parse_options(argc, argv, prefix, builtin_clone_options,\n \t\t\t builtin_clone_usage, 0);\n \n@@ -1125,9 +1141,17 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (real_git_dir)\n \t\tgit_dir = real_git_dir;\n \n+\t/*\n+\t * additional config can be injected with -c, make sure it's included\n+\t * after init_db, which clears the entire config environment.\n+\t */\n \twrite_config(&option_config);\n \n-\tgit_config(git_default_config, NULL);\n+\t/*\n+\t * re-read config after init_db and write_config to pick up any config\n+\t * injected by --template and --config, respectively\n+\t */\n+\tgit_config(git_clone_config, NULL);\n \n \tif (option_bare) {\n \t\tif (option_mirror)\n-- \ngitgitgadget\n\n"},{"id":"405431","messageId":"4cdcedff313751da8c91d701c095f1051e759ce2.1599848727.git.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":"pull.727.git.1599848727.gitgitgadget@gmail.com","subject":"[PATCH 1/4] clone: add tests for --template and some disallowed option pairs","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-11T18:25:24Z","receivedAt":"2020-09-11T18:25:47Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"From: Sean Barag \n\nSome combinations of command-line options to `git clone` are invalid,\nbut there were previously no tests ensuring those combinations reported\nerrors. Similarly, `git clone --template` didn't appear to have any\ntests.\n\nSigned-off-by: Sean Barag \n---\n t/t5606-clone-options.sh | 44 ++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 44 insertions(+)\n\ndiff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh\nindex e69427f881..d20a78f84b 100755\n--- a/t/t5606-clone-options.sh\n+++ b/t/t5606-clone-options.sh\n@@ -19,6 +19,50 @@ test_expect_success 'clone -o' '\n \n '\n \n+test_expect_success 'disallows --bare with --origin' '\n+\n+\ttest_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&\n+\ttest_debug \"cat err\" &&\n+\ttest_i18ngrep \"\\-\\-bare and --origin foo options are incompatible\" err\n+\n+'\n+\n+test_expect_success 'disallows --bare with --separate-git-dir' '\n+\n+\ttest_expect_code 128 git clone --bare --separate-git-dir dot-git-destiation parent clone-bare-sgd 2>err &&\n+\ttest_debug \"cat err\" &&\n+\ttest_i18ngrep \"\\-\\-bare and --separate-git-dir are incompatible\" err\n+\n+'\n+\n+test_expect_success 'uses \"origin\" for default remote name' '\n+\n+\tgit clone parent clone-default-origin &&\n+\t(cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master)\n+\n+'\n+\n+test_expect_success 'prefers --template config over normal config' '\n+\n+\ttemplate=\"$TRASH_DIRECTORY/template-with-config\" &&\n+\tmkdir \"$template\" &&\n+\tgit config --file \"$template/config\" foo.bar from_template &&\n+\ttest_config_global foo.bar from_global &&\n+\tgit clone \"--template=$template\" parent clone-template-config &&\n+\t(cd clone-template-config && test \"$(git config --local foo.bar)\" = \"from_template\")\n+\n+'\n+\n+test_expect_success 'prefers -c config over --template config' '\n+\n+\ttemplate=\"$TRASH_DIRECTORY/template-with-ignored-config\" &&\n+\tmkdir \"$template\" &&\n+\tgit config --file \"$template/config\" foo.bar from_template &&\n+\tgit clone \"--template=$template\" -c foo.bar=inline parent clone-template-inline-config &&\n+\t(cd clone-template-inline-config && test \"$(git config --local foo.bar)\" = \"inline\")\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-- \ngitgitgadget\n\n"},{"id":"405432","messageId":"0dff8cd66930130ffd5f0d7d068ad3ed47cd1c81.1599848727.git.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":"pull.727.git.1599848727.gitgitgadget@gmail.com","subject":"[PATCH 3/4] clone: validate --origin option before use","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-11T18:25:26Z","receivedAt":"2020-09-11T18:25:51Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"From: Sean Barag \n\nProviding a bad origin name to `git clone` currently reports an\n'invalid refspec' error instead of a more explicit message explaining\nthat the `--origin` option was malformed. Per Junio, it's been this way\nsince 8434c2f1 (Build in clone, 2008-04-27). This patch reintroduces\nvalidation for the provided `--origin` option, but notably _doesn't_\ninclude a multi-level check (e.g. \"foo/bar\") that was present in the\noriginal `git-clone.sh`. It seems `git remote` allows multi-level\nremote names, so applying that same validation in `git clone` seems\nreasonable.\n\nSigned-off-by: Sean Barag \nThanks-to: Junio C Hamano \n---\n builtin/clone.c | 7 +++++++\n t/t5606-clone-options.sh | 8 ++++++++\n 2 files changed, 15 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex bf095815f0..1cd62d0001 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -967,6 +967,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tconst struct ref *ref;\n \tstruct strbuf key = STRBUF_INIT;\n \tstruct strbuf default_refspec = STRBUF_INIT;\n+\tstruct strbuf resolved_refspec = STRBUF_INIT;\n \tstruct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;\n \tstruct transport *transport = NULL;\n \tconst char *src_ref_prefix = \"refs/heads/\";\n@@ -1011,6 +1012,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (!option_origin)\n \t\toption_origin = \"origin\";\n \n+\tstrbuf_addf(&resolved_refspec, \"refs/heads/test:refs/remotes/%s/test\", option_origin);\n+\tif (!valid_fetch_refspec(resolved_refspec.buf))\n+\t\t/* TRANSLATORS: %s will be the user-provided --origin / -o option */\n+\t\tdie(_(\"'%s' is not a valid origin name\"), option_origin);\n+\tstrbuf_release(&resolved_refspec);\n+\n \trepo_name = argv[0];\n \n \tpath = get_repo_path(repo_name, &is_bundle);\ndiff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh\nindex d20a78f84b..c865f96def 100755\n--- a/t/t5606-clone-options.sh\n+++ b/t/t5606-clone-options.sh\n@@ -19,6 +19,14 @@ test_expect_success 'clone -o' '\n \n '\n \n+test_expect_success 'rejects invalid -o/--origin' '\n+\n+\ttest_expect_code 128 git clone -o \"bad...name\" parent clone-bad-name 2>err &&\n+\ttest_debug \"cat err\" &&\n+\ttest_i18ngrep \"'\\''bad...name'\\'' is not a valid origin name\" err\n+\n+'\n+\n test_expect_success 'disallows --bare with --origin' '\n \n \ttest_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&\n-- \ngitgitgadget\n\n"},{"id":"405433","messageId":"5c519376c29a28bd89a712cf0b8125fc1c2c81be.1599848727.git.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":"pull.727.git.1599848727.gitgitgadget@gmail.com","subject":"[PATCH 4/4] clone: allow configurable default for `-o`/`--origin`","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-11T18:25:27Z","receivedAt":"2020-09-11T18:25:53Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"From: Sean Barag \n\nWhile the default remote name of \"origin\" can be changed at clone-time\nwith `git clone`'s `--origin` option, it was previously not possible\nto specify a default value for the name of that remote. This commit\nadds support for a new `clone.defaultRemoteName` config.\n\nIt's resolved in the expected priority order:\n\n1. (Highest priority) A remote name passed directly to `git clone -o`\n2. A `clone.defaultRemoteName=new_name` in config `git clone -c`\n3. A `clone.defaultRemoteName` value set in `/path/to/template/config`,\n where `--template=/path/to/template` is provided\n3. A `clone.defaultRemoteName` value set in a non-template config file\n4. The default value of `origin`\n\nSigned-off-by: Sean Barag \nThanks-to: Junio C Hamano \nThanks-to: Johannes Schindelin \n---\n Documentation/config.txt | 2 ++\n Documentation/config/clone.txt | 5 ++++\n Documentation/git-clone.txt | 5 ++--\n builtin/clone.c | 42 +++++++++++++++++++---------------\n t/t5606-clone-options.sh | 29 +++++++++++++++++------\n 5 files changed, 56 insertions(+), 27 deletions(-)\n create mode 100644 Documentation/config/clone.txt\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 3042d80978..354874facf 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -334,6 +334,8 @@ include::config/checkout.txt[]\n \n include::config/clean.txt[]\n \n+include::config/clone.txt[]\n+\n include::config/color.txt[]\n \n include::config/column.txt[]\ndiff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt\nnew file mode 100644\nindex 0000000000..20755d413a\n--- /dev/null\n+++ b/Documentation/config/clone.txt\n@@ -0,0 +1,5 @@\n+clone.defaultRemoteName::\n+\tThe name of the remote to create when cloning a repository. Defaults to\n+\t`origin`, and can be overridden by passing the `--origin` command-line\n+\toption to linkgit:git-clone[1].\n+\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex c898310099..f04bf6e6ba 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 ::\n --origin ::\n-\tInstead of using the remote name `origin` to keep track\n-\tof the upstream repository, use ``.\n+\tInstead of using the remote name `origin` to keep track of the upstream\n+\trepository, use ``. Overrides `clone.defaultRemoteName` from the\n+\tconfig.\n \n -b ::\n --branch ::\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 1cd62d0001..aeb41f15f3 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -53,6 +53,7 @@ static int option_shallow_submodules;\n static int deepen;\n static char *option_template, *option_depth, *option_since;\n static char *option_origin = NULL;\n+static char *remote_name = \"origin\";\n static char *option_branch = NULL;\n static struct string_list option_not = STRING_LIST_INIT_NODUP;\n static const char *real_git_dir;\n@@ -721,7 +722,7 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t\tif (!option_bare) {\n \t\t\tupdate_ref(msg, \"HEAD\", &our->old_oid, NULL, 0,\n \t\t\t\t UPDATE_REFS_DIE_ON_ERR);\n-\t\t\tinstall_branch_config(0, head, option_origin, our->name);\n+\t\t\tinstall_branch_config(0, head, remote_name, our->name);\n \t\t}\n \t} else if (our) {\n \t\tstruct commit *c = lookup_commit_reference(the_repository,\n@@ -853,16 +854,18 @@ static int checkout(int submodule_progress)\n \n static int git_clone_config(const char *k, const char *v, void *cb)\n {\n+\tif (!strcmp(k, \"clone.defaultremotename\") && !option_origin)\n+\t\tremote_name = xstrdup(v);\n \treturn git_default_config(k, v, cb);\n }\n \n static int write_one_config(const char *key, const char *value, void *data)\n {\n \t/*\n-\t * give git_config_default a chance to write config values back to the environment, since\n+\t * give git_clone_config a chance to write config values back to the environment, since\n \t * git_config_set_multivar_gently only deals with config-file writes\n \t */\n-\tint apply_failed = git_default_config(key, value, data);\n+\tint apply_failed = git_clone_config(key, value, data);\n \tif (apply_failed)\n \t\treturn apply_failed;\n \n@@ -918,12 +921,12 @@ static void write_refspec_config(const char *src_ref_prefix,\n \t\t}\n \t\t/* Configure the remote */\n \t\tif (value.len) {\n-\t\t\tstrbuf_addf(&key, \"remote.%s.fetch\", option_origin);\n+\t\t\tstrbuf_addf(&key, \"remote.%s.fetch\", remote_name);\n \t\t\tgit_config_set_multivar(key.buf, value.buf, \"^$\", 0);\n \t\t\tstrbuf_reset(&key);\n \n \t\t\tif (option_mirror) {\n-\t\t\t\tstrbuf_addf(&key, \"remote.%s.mirror\", option_origin);\n+\t\t\t\tstrbuf_addf(&key, \"remote.%s.mirror\", remote_name);\n \t\t\t\tgit_config_set(key.buf, \"true\");\n \t\t\t\tstrbuf_reset(&key);\n \t\t\t}\n@@ -1009,13 +1012,16 @@ 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\tremote_name = option_origin;\n \n-\tstrbuf_addf(&resolved_refspec, \"refs/heads/test:refs/remotes/%s/test\", option_origin);\n+\tstrbuf_addf(&resolved_refspec, \"refs/heads/test:refs/remotes/%s/test\", remote_name);\n \tif (!valid_fetch_refspec(resolved_refspec.buf))\n-\t\t/* TRANSLATORS: %s will be the user-provided --origin / -o option */\n-\t\tdie(_(\"'%s' is not a valid origin name\"), option_origin);\n+\t\t/*\n+\t\t * TRANSLATORS: %s will be the user-provided --origin / -o option, or the value\n+\t\t * of clone.defaultremotename from the config.\n+\t\t */\n+\t\tdie(_(\"'%s' is not a valid origin name\"), remote_name);\n \tstrbuf_release(&resolved_refspec);\n \n \trepo_name = argv[0];\n@@ -1167,15 +1173,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \t\tgit_config_set(\"core.bare\", \"true\");\n \t} else {\n-\t\tstrbuf_addf(&branch_top, \"refs/remotes/%s/\", option_origin);\n+\t\tstrbuf_addf(&branch_top, \"refs/remotes/%s/\", remote_name);\n \t}\n \n-\tstrbuf_addf(&key, \"remote.%s.url\", option_origin);\n+\tstrbuf_addf(&key, \"remote.%s.url\", remote_name);\n \tgit_config_set(key.buf, repo);\n \tstrbuf_reset(&key);\n \n \tif (option_no_tags) {\n-\t\tstrbuf_addf(&key, \"remote.%s.tagOpt\", option_origin);\n+\t\tstrbuf_addf(&key, \"remote.%s.tagOpt\", remote_name);\n \t\tgit_config_set(key.buf, \"--no-tags\");\n \t\tstrbuf_reset(&key);\n \t}\n@@ -1186,7 +1192,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (option_sparse_checkout && git_sparse_checkout_init(dir))\n \t\treturn 1;\n \n-\tremote = remote_get(option_origin);\n+\tremote = remote_get(remote_name);\n \n \tstrbuf_addf(&default_refspec, \"+%s*:%s*\", src_ref_prefix,\n \t\t branch_top.buf);\n@@ -1299,7 +1305,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \t\t\tif (!our_head_points_at)\n \t\t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n-\t\t\t\t option_branch, option_origin);\n+\t\t\t\t option_branch, remote_name);\n \t\t}\n \t\telse\n \t\t\tour_head_points_at = remote_head_points_at;\n@@ -1307,7 +1313,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \telse {\n \t\tif (option_branch)\n \t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n-\t\t\t\t\toption_branch, option_origin);\n+\t\t\t\t\toption_branch, remote_name);\n \n \t\twarning(_(\"You appear to have cloned an empty repository.\"));\n \t\tmapped_refs = NULL;\n@@ -1319,7 +1325,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\tconst char *branch = git_default_branch_name();\n \t\t\tchar *ref = xstrfmt(\"refs/heads/%s\", branch);\n \n-\t\t\tinstall_branch_config(0, branch, option_origin, ref);\n+\t\t\tinstall_branch_config(0, branch, remote_name, ref);\n \t\t\tfree(ref);\n \t\t}\n \t}\n@@ -1328,7 +1334,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\tremote_head_points_at, &branch_top);\n \n \tif (filter_options.choice)\n-\t\tpartial_clone_register(option_origin, &filter_options);\n+\t\tpartial_clone_register(remote_name, &filter_options);\n \n \tif (is_local)\n \t\tclone_local(path, git_dir);\ndiff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh\nindex c865f96def..017c24a454 100755\n--- a/t/t5606-clone-options.sh\n+++ b/t/t5606-clone-options.sh\n@@ -43,13 +43,6 @@ test_expect_success 'disallows --bare with --separate-git-dir' '\n \n '\n \n-test_expect_success 'uses \"origin\" for default remote name' '\n-\n-\tgit clone parent clone-default-origin &&\n-\t(cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master)\n-\n-'\n-\n test_expect_success 'prefers --template config over normal config' '\n \n \ttemplate=\"$TRASH_DIRECTORY/template-with-config\" &&\n@@ -71,6 +64,28 @@ test_expect_success 'prefers -c config over --template config' '\n \n '\n \n+test_expect_success 'uses \"origin\" for default remote name' '\n+\n+\tgit clone parent clone-default-origin &&\n+\t(cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master)\n+\n+'\n+\n+test_expect_success 'prefers config \"clone.defaultRemoteName\" over default' '\n+\n+\ttest_config_global clone.defaultRemoteName from_config &&\n+\tgit clone parent clone-config-origin &&\n+\t(cd clone-config-origin && git rev-parse --verify refs/remotes/from_config/master)\n+\n+'\n+\n+test_expect_success 'prefers --origin over -c config' '\n+\n+\tgit clone -c clone.defaultRemoteName=inline --origin from_option parent clone-o-and-inline-config &&\n+\t(cd clone-o-and-inline-config && git rev-parse --verify refs/remotes/from_option/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-- \ngitgitgadget\n"},{"id":"405434","messageId":"8f864b6c-4b05-a9d6-b649-5f7c50697730@gmail.com","threadId":"54232","inReplyTo":"4cdcedff313751da8c91d701c095f1051e759ce2.1599848727.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] clone: add tests for --template and some disallowed option pairs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-09-11T18:57:11Z","receivedAt":"2020-09-11T18:57:23Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/11/2020 2:25 PM, Sean Barag via GitGitGadget wrote:\n> From: Sean Barag \n> \n> Some combinations of command-line options to `git clone` are invalid,\n> but there were previously no tests ensuring those combinations reported\n> errors. Similarly, `git clone --template` didn't appear to have any\n> tests.\n> \n> Signed-off-by: Sean Barag \n> ---\n> t/t5606-clone-options.sh | 44 ++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 44 insertions(+)\n> \n> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh\n> index e69427f881..d20a78f84b 100755\n> --- a/t/t5606-clone-options.sh\n> +++ b/t/t5606-clone-options.sh\n> @@ -19,6 +19,50 @@ test_expect_success 'clone -o' '\n> \n> '\n> \n> +test_expect_success 'disallows --bare with --origin' '\n> +\n> +\ttest_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&\n> +\ttest_debug \"cat err\" &&\n> +\ttest_i18ngrep \"\\-\\-bare and --origin foo options are incompatible\" err\n> +\n> +'\n\nIt seems that all of your tests have an extraneous newline\nat the end.\n\nThanks,\n-Stolee\n"},{"id":"405439","messageId":"48b160f0-0cb3-e120-dc11-1d5de7e02af1@gmail.com","threadId":"54232","inReplyTo":"51ef776f8523d29dfe03d15f0d1958f5c456c057.1599848727.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] clone: call git_config before parse_options","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-09-11T18:59:59Z","receivedAt":"2020-09-11T19:00:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/11/2020 2:25 PM, Sean Barag via GitGitGadget wrote:\n> From: Sean Barag \n> \n> While Junio's request [1] was to avoids the unusual \"write config then\n> immediately read it\" pattern that exists in `cmd_clone`, Johannes\n> mentioned that --template can write new config values that aren't\n> automatically included in the environment [2]. This requires a config\n> re-read after `init_db` is called.\n> \n> Moving the initial config up does allow settings from config to be\n> overwritten by ones provided via CLI options in a more natural way\n> though, so that part of Junio's suggestion remains.\n> \n> [1] https://lore.kernel.org/git/pull.710.git.1598456751674.gitgitgadget@gmail.com/\n> [2] https://github.com/gitgitgadget/git/pull/727#issuecomment-689740195\n> \n> Signed-off-by: Sean Barag \n> Thanks-to: Junio C Hamano \n> Thanks-to: Johannes Schindelin \n> ---\n> builtin/clone.c | 26 +++++++++++++++++++++++++-\n> 1 file changed, 25 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index b087ee40c2..bf095815f0 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -851,8 +851,21 @@ static int checkout(int submodule_progress)\n> \treturn err;\n> }\n> \n> +static int git_clone_config(const char *k, const char *v, void *cb)\n> +{\n> +\treturn git_default_config(k, v, cb);\n> +}\n> +\n\nIntroducing this no-op seems a bit premature, but as long\nas it makes your future patches cleaner, then it is\nappropriate.\n\n> static int write_one_config(const char *key, const char *value, void *data)\n> {\n> +\t/*\n> +\t * give git_config_default a chance to write config values back to the environment, since\n> +\t * git_config_set_multivar_gently only deals with config-file writes\n> +\t */\n> +\tint apply_failed = git_default_config(key, value, data);\n\nHowever, you change this to git_clone_config() in patch 4. Perhaps\nuse git_clone_config() here instead?\n\nThanks,\n-Stolee\n\n"},{"id":"405442","messageId":"0739858e-83f1-3b3f-aba0-7549e7e47cfc@gmail.com","threadId":"54232","inReplyTo":"5c519376c29a28bd89a712cf0b8125fc1c2c81be.1599848727.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] clone: allow configurable default for `-o`/`--origin`","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-09-11T19:13:04Z","receivedAt":"2020-09-11T19:13:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/11/2020 2:25 PM, Sean Barag via GitGitGadget wrote:\n\n> static char *option_origin = NULL;\n> +static char *remote_name = \"origin\";\n\nThis patch could have used a prep patch that had all consumers\nof option_origin use remote_name instead, with this adjustment\nto the way to use option_origin:\n \n> -\tif (!option_origin)\n> -\t\toption_origin = \"origin\";\n> +\tif (option_origin)\n> +\t\tremote_name = option_origin;\n> +\telse\n> +\t\tremote_name = \"origin\";\n \nThen this patch introducing the config option would have a\nvery limited change in the builtin consisting of these two\nhunks:\n\n> static int git_clone_config(const char *k, const char *v, void *cb)\n> {\n> +\tif (!strcmp(k, \"clone.defaultremotename\") && !option_origin)\n> +\t\tremote_name = xstrdup(v);\n> \treturn git_default_config(k, v, cb);\n> }\n...\n> \tif (option_origin)\n> \t\tremote_name = option_origin;\n> -\telse\n> -\t\tremote_name = \"origin\"\n\nalong with this translators comment note:\n\n> \tif (!valid_fetch_refspec(resolved_refspec.buf))\n> -\t\t/* TRANSLATORS: %s will be the user-provided --origin / -o option */\n> -\t\tdie(_(\"'%s' is not a valid origin name\"), option_origin);\n> +\t\t/*\n> +\t\t * TRANSLATORS: %s will be the user-provided --origin / -o option, or the value\n> +\t\t * of clone.defaultremotename from the config.\n> +\t\t */\n> +\t\tdie(_(\"'%s' is not a valid origin name\"), remote_name);\n\n\n> --- a/t/t5606-clone-options.sh\n> +++ b/t/t5606-clone-options.sh\n> @@ -43,13 +43,6 @@ test_expect_success 'disallows --bare with --separate-git-dir' '\n> \n> '\n> \n> -test_expect_success 'uses \"origin\" for default remote name' '\n> -\n> -\tgit clone parent clone-default-origin &&\n> -\t(cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master)\n> -\n> -'\n> -\n\nInteresting that you moved this test. Probably not necessary, and\njust a mistake.\n\n> test_expect_success 'prefers --template config over normal config' '\n> \n> \ttemplate=\"$TRASH_DIRECTORY/template-with-config\" &&\n> @@ -71,6 +64,28 @@ test_expect_success 'prefers -c config over --template config' '\n> \n> '\n> \n> +test_expect_success 'uses \"origin\" for default remote name' '\n> +\n> +\tgit clone parent clone-default-origin &&\n> +\t(cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master)\n\nI didn't notice it earlier, but perhaps this subshell should be split\ninto its own multi-line section as follows:\n\n> +\t(\n> +\t\tcd clone-default-origin &&\n> +\t\tgit rev-parse --verify refs/remotes/origin/master\n> +\t)\n\nBut even better, this is only one line so using\n\"git -C clone-default-origin rev-parse\" is simpler:\n\n\n> +test_expect_success 'uses \"origin\" for default remote name' '\n> +\n> +\tgit clone parent clone-default-origin &&\n> +\tgit -C clone-default-origin rev-parse --verify refs/remotes/origin/master\n> +'\n\n> +test_expect_success 'prefers config \"clone.defaultRemoteName\" over default' '\n> +\n> +\ttest_config_global clone.defaultRemoteName from_config &&\n> +\tgit clone parent clone-config-origin &&\n\nThis could be done using \"git -c clone.defaultRemoteName=from_config\" instead\nof setting the global config.\n\n> +\t(cd clone-config-origin && git rev-parse --verify refs/remotes/from_config/master)\n> +\n> +'\n> +\n> +test_expect_success 'prefers --origin over -c config' '\n> +\n> +\tgit clone -c clone.defaultRemoteName=inline --origin from_option parent clone-o-and-inline-config &&\n\nAnd you use the -c option here.\n\n> +\t(cd clone-o-and-inline-config && git rev-parse --verify refs/remotes/from_option/master)\n> +\n> +'\n> +\n\nWe have the extra newline in these tests, too.\n\nThanks,\n-Stolee\n\n"},{"id":"405444","messageId":"bf0107fb-2a6c-68d3-df24-72c6a9df6182@gmail.com","threadId":"54232","inReplyTo":"0dff8cd66930130ffd5f0d7d068ad3ed47cd1c81.1599848727.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] clone: validate --origin option before use","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-09-11T19:24:15Z","receivedAt":"2020-09-11T19:24:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/11/2020 2:25 PM, Sean Barag via GitGitGadget wrote:\n> +\tstrbuf_addf(&resolved_refspec, \"refs/heads/test:refs/remotes/%s/test\", option_origin);\n> +\tif (!valid_fetch_refspec(resolved_refspec.buf))\n> +\t\t/* TRANSLATORS: %s will be the user-provided --origin / -o option */\n> +\t\tdie(_(\"'%s' is not a valid origin name\"), option_origin);\n\nLooking at this again, I'm not sure the translators note is\nnecessary. Also, I would say \"is not a valid remote name\".\nThat makes the string align with the already-translated string\nin builtin/remote.c.\n\nThis code is duplicated from builtin/remote.c, so I'd rather\nsee this be a helper method in refspec.c and have both\nbuiltin/clone.c and builtin/remote.c call that helper.\n\nHere is the helper:\n\nvoid valid_remote_name(const char *name)\n{\n\tint result;\n\tstruct strbuf refspec = STRBUF_INIT;\n\tstrbuf_addf(&refspec, \"refs/heads/test:refs/remotes/%s/test\", name);\n\tresult = valid_fetch_refspec(refspec.buf);\n\tstrbuf_release(&refspec);\n\treturn result;\n}\n\nAnd here is the use in builtin/clone.c:\n\n\tif (!valid_remote_name(option_origin))\n\t\tdie(_(\"'%s' is not a valid remote name\"), option_origin);\n\nand in builtin/remote.c:\n\n\tif (!valid_remote_name(name))\n\t\tdie(_(\"'%s' is not a valid remote name\"), name);\n\n> +test_expect_success 'rejects invalid -o/--origin' '\n> +\n> +\ttest_expect_code 128 git clone -o \"bad...name\" parent clone-bad-name 2>err &&\n> +\ttest_debug \"cat err\" &&\n> +\ttest_i18ngrep \"'\\''bad...name'\\'' is not a valid origin name\" err\n> +\n> +'\n> +\n\nDouble newlines here! I personally appreciate newlines to\nspread out content, but it doesn't fit our guidelines.\n\nThanks,\n-Stolee\n"},{"id":"405445","messageId":"66e46aab-b8c5-0d9e-7a67-9a5ae6a38a93@gmail.com","threadId":"54232","inReplyTo":"pull.727.git.1599848727.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] clone: allow configurable default for -o/--origin","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-09-11T19:25:43Z","receivedAt":"2020-09-11T19:25:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/11/2020 2:25 PM, Sean Barag via GitGitGadget wrote:\n> Took another pass at supporting a configurable default for-o/--origin, this\n> time following Junio's suggestions from a previous approach as much as\n> possible [1]. Unfortunately, Johannes mentioned that --template can write\n> new config values that aren't automatically merged without re-calling \n> git_config. There doesn't appear to be a way around this without rewriting\n> significant amounts of init and config logic across the codebase.\n\nThanks for this update. I like the feature quite a bit, and all\nof my comments are about style and organization instead of\nfunctional issues.\n\n> While this could have been v2 of the original patchset, it's diverged s\n> drastically from the original that it likely warrants its own root thread.\n> If that's not appropriate though, I'd be happy to restructure!\n\nI think it is fine to restart the thread if the range-diff is not helpful.\n\nThanks,\n-Stolee\n"},{"id":"405448","messageId":"xmqqblicnm4f.fsf@gitster.c.googlers.com","threadId":"54232","inReplyTo":"pull.727.git.1599848727.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] clone: allow configurable default for -o/--origin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-11T19:34:24Z","receivedAt":"2020-09-11T19:34:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Sean Barag via GitGitGadget\" writes:\n\n> Took another pass at supporting a configurable default for-o/--origin, this\n> time following Junio's suggestions from a previous approach as much as\n> possible [1]. Unfortunately, Johannes mentioned that --template can write\n> new config values that aren't automatically merged without re-calling \n> git_config. There doesn't appear to be a way around this without rewriting\n> significant amounts of init and config logic across the codebase.\n>\n> While this could have been v2 of the original patchset, it's diverged so\n> drastically from the original that it likely warrants its own root thread.\n> If that's not appropriate though, I'd be happy to restructure!\n\nI did wonder if this series came from the same motivation while\nscanning messages in the mailbox and saw no \"v2\" in the subject, but\nI am OK either way in a case like this. \n\nI DO appreciate this note to explain why it is not marked with \"v2\".\n"},{"id":"405450","messageId":"20200911195622.GB2697399@coredump.intra.peff.net","threadId":"54232","inReplyTo":"8f864b6c-4b05-a9d6-b649-5f7c50697730@gmail.com","subject":"Re: [PATCH 1/4] clone: add tests for --template and some disallowed option pairs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-11T19:56:22Z","receivedAt":"2020-09-11T19:56:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 11, 2020 at 02:57:11PM -0400, Derrick Stolee wrote:\n\n> > diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh\n> > index e69427f881..d20a78f84b 100755\n> > --- a/t/t5606-clone-options.sh\n> > +++ b/t/t5606-clone-options.sh\n> > @@ -19,6 +19,50 @@ test_expect_success 'clone -o' '\n> > \n> > '\n> > \n> > +test_expect_success 'disallows --bare with --origin' '\n> > +\n> > +\ttest_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&\n> > +\ttest_debug \"cat err\" &&\n> > +\ttest_i18ngrep \"\\-\\-bare and --origin foo options are incompatible\" err\n> > +\n> > +'\n> \n> It seems that all of your tests have an extraneous newline\n> at the end.\n\nAnd the beginning. :)\n\nIt's matching the surrounding test, which are written in an older\ninconsistent style. I think it's OK to match them, but cleaning them\nwould be welcome, too.\n\nWhile I'm looking at this hunk, two other things:\n\n - do we really care about code 128, or just failure? test_must_fail\n might be a better choice\n\n - I didn't even know we had test_debug. ;) The last time somebody added\n a call to it was in 2012. I think it's being used as intended here,\n but I'm not sure that the clutter to the test is worth it (we have\n other tools like \"-i\" to stop at the right spot and let you inspect\n the broken state).\n\n - the backslash escapes confused me for a moment. I guess they are\n trying to hide the dashes from grep's option parser. That's OK,\n though I'd have probably just started with \"bare\" since we're\n matching a substring anyway. I think you could also use \"-e\" with\n test_i18ngrep.\n\n-Peff\n"},{"id":"405451","messageId":"CAPig+cTNjkOSRbP6sQMAhr-etXMt7ejJo3Zm2gu=nvpF00CMvw@mail.gmail.com","threadId":"54232","inReplyTo":"20200911195622.GB2697399@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] clone: add tests for --template and some disallowed option pairs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-11T20:07:16Z","receivedAt":"2020-09-11T20:07:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Sep 11, 2020 at 3:56 PM Jeff King wrote:\n> And the beginning. :)\n>\n> It's matching the surrounding test, which are written in an older\n> inconsistent style. I think it's OK to match them, but cleaning them\n> would be welcome, too.\n>\n> While I'm looking at this hunk, two other things:\n>\n> - do we really care about code 128, or just failure? test_must_fail\n> might be a better choice\n>\n> - I didn't even know we had test_debug. ;) The last time somebody added\n> a call to it was in 2012. I think it's being used as intended here,\n> but I'm not sure that the clutter to the test is worth it (we have\n> other tools like \"-i\" to stop at the right spot and let you inspect\n> the broken state).\n>\n> - the backslash escapes confused me for a moment. I guess they are\n> trying to hide the dashes from grep's option parser. That's OK,\n> though I'd have probably just started with \"bare\" since we're\n> matching a substring anyway. I think you could also use \"-e\" with\n> test_i18ngrep.\n\nThanks, you said everything I was going to say about this, thus saving\nme some typing.\n\nA couple more observations related to a few of the subsequent tests. This:\n\n template=\"$TRASH_DIRECTORY/template-with-config\" &&\n\nprobably doesn't need $TRASH_DIRECTORY since that happens to be the\ncurrent working directory anyhow, so:\n\n template=template-with-config &&\n\nshould suffice (unless you had a problem doing it that way). Or you\ncould drop the variable altogether and use the literal name where you\nneed it.\n\nAlso, instead of:\n\n (cd clone-template-config && test \"$(git config --local foo.bar)\"\n= \"from_template\")\n\nwould probably be written these days as:\n\n echo from_template >expect &&\n git -C clone-template-config config --local foo.bar >actual &&\n test_cmp expect actual\n"},{"id":"405453","messageId":"xmqq5z8knjou.fsf@gitster.c.googlers.com","threadId":"54232","inReplyTo":"51ef776f8523d29dfe03d15f0d1958f5c456c057.1599848727.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] clone: call git_config before parse_options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-11T20:26:57Z","receivedAt":"2020-09-11T20:27:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Sean Barag via GitGitGadget\" writes:\n\n> From: Sean Barag \n>\n> While Junio's request [1] was to avoids the unusual \"write config then\n> immediately read it\" pattern that exists in `cmd_clone`, Johannes\n> mentioned that --template can write new config values that aren't\n> automatically included in the environment [2]. This requires a config\n> re-read after `init_db` is called.\n>\n> Moving the initial config up does allow settings from config to be\n> overwritten by ones provided via CLI options in a more natural way\n> though, so that part of Junio's suggestion remains.\n\nThe title says what the code does after this change. The code calls\ngit_config() before calling parse_options(), but not much in the\nproposed log message explains what the patch tries to achieve by\ndoing so.\n\nThe above refers to suggestions but does not describe what problem\nthe patch is trying to address and what approach is taken to address\nit.\n\n> Signed-off-by: Sean Barag \n> Thanks-to: Junio C Hamano \n> Thanks-to: Johannes Schindelin \n\nUsually these two are spelled Helped-by: in this project, and are\ngiven in chronological order. People gave input to you and then\nfinally you send out a signed copy, so your sign-off is placed at\nthe end of the sequence.\n\n> +static int git_clone_config(const char *k, const char *v, void *cb)\n> +{\n> +\treturn git_default_config(k, v, cb);\n> +}\n> +\n> static int write_one_config(const char *key, const char *value, void *data)\n> {\n> +\t/*\n> +\t * give git_config_default a chance to write config values back to the environment, since\n> +\t * git_config_set_multivar_gently only deals with config-file writes\n> +\t */\n\nOverlong lines...\n\n> +\tint apply_failed = git_default_config(key, value, data);\n\nNot git_clone_config()? Presumably you'll make git_clone_config()\nrecognise more variables than git_default_config() does, and the\ncaller of this helper wants us to recognise \"clone.*\" that are\nignored by git_default_config() callback, no?\n\n> +\tif (apply_failed)\n> +\t\treturn apply_failed;\n> +\n> \treturn git_config_set_multivar_gently(key,\n> \t\t\t\t\t value ? value : \"true\",\n> \t\t\t\t\t CONFIG_REGEX_NONE, 0);\n> @@ -964,6 +977,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \tstruct strvec ref_prefixes = STRVEC_INIT;\n> \n> \tpacket_trace_identity(\"clone\");\n> +\n> +\tgit_config(git_clone_config, NULL);\n> +\n> \targc = parse_options(argc, argv, prefix, builtin_clone_options,\n> \t\t\t builtin_clone_usage, 0);\n> \n> @@ -1125,9 +1141,17 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \tif (real_git_dir)\n> \t\tgit_dir = real_git_dir;\n> \n> +\t/*\n> +\t * additional config can be injected with -c, make sure it's included\n> +\t * after init_db, which clears the entire config environment.\n> +\t */\n> \twrite_config(&option_config);\n\nThe comment that explains the location is very much appropriate.\n\n> -\tgit_config(git_default_config, NULL);\n> +\t/*\n> +\t * re-read config after init_db and write_config to pick up any config\n> +\t * injected by --template and --config, respectively\n> +\t */\n> +\tgit_config(git_clone_config, NULL);\n\nDoes this call read from the freshly written file?\n\nI thought git_config() iterates over the in-core configset that was\nread by the first call to git_config(), which in turn calls\ngit_config_check_init() and calls repo_read_config() only once to\npopulate the in-core configset, and I suspect we are not clearing\nit in between.\n"},{"id":"405454","messageId":"xmqq1rj8nj47.fsf@gitster.c.googlers.com","threadId":"54232","inReplyTo":"0dff8cd66930130ffd5f0d7d068ad3ed47cd1c81.1599848727.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] clone: validate --origin option before use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-11T20:39:20Z","receivedAt":"2020-09-11T20:39:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Sean Barag via GitGitGadget\" writes:\n\n> From: Sean Barag \n>\n> Providing a bad origin name to `git clone` currently reports an\n> 'invalid refspec' error instead of a more explicit message explaining\n> that the `--origin` option was malformed. Per Junio, it's been this way\n> since 8434c2f1 (Build in clone, 2008-04-27). \n\nIf you know it as a fact that it has been this way since the first\nversion of rewritten \"git clone\", don't blame others.\n\nA micronit. We describe the current status in present tense when\npresenting the problem to be solved, so \"currently\" can be dropped.\n\n> This patch reintroduces\n\nWhen presenting the solution, we write as if we are giving an order\nto a patch monkey to \"make the code like so\".\n\n\"This patch reintroduces\" -> \"Reintroduce\". \n\n> validation for the provided `--origin` option, but notably _doesn't_\n> include a multi-level check (e.g. \"foo/bar\") that was present in the\n> original `git-clone.sh`. It seems `git remote` allows multi-level\n> remote names, so applying that same validation in `git clone` seems\n> reasonable.\n\nEven though I suspect \"git remote\" is being overly loose and\ncareless here, I am not sure if it is a good idea to tighten it\nretroactively. But if this is meant as a bugfix for misconversion\nmade in 8434c2f1, we should be as strict as the original. I dunno.\n\n> +\tstrbuf_addf(&resolved_refspec, \"refs/heads/test:refs/remotes/%s/test\", option_origin);\n> +\tif (!valid_fetch_refspec(resolved_refspec.buf))\n\nIf we reintroduce pre-8434c2f1 check, then we would want\n\n\tif (!valid_fetch_refspec(...) || strchr(option_origin, '/'))\n\nor something like that?\n\n> +\t\t/* TRANSLATORS: %s will be the user-provided --origin / -o option */\n> +\t\tdie(_(\"'%s' is not a valid origin name\"), option_origin);\n\nI am not sure if this translator comment is helping.\n\nThe message makes it sound as if \"%s\" here is used to switch between\n'-o' or '--origin'. If it said \"'%s' will be the value given to\n--origin/-o option\", it would become much less confusing.\n\n> +\tstrbuf_release(&resolved_refspec);\n> +\n> \trepo_name = argv[0];\n> \n> \tpath = get_repo_path(repo_name, &is_bundle);\n> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh\n> index d20a78f84b..c865f96def 100755\n> --- a/t/t5606-clone-options.sh\n> +++ b/t/t5606-clone-options.sh\n> @@ -19,6 +19,14 @@ test_expect_success 'clone -o' '\n> \n> '\n> \n> +test_expect_success 'rejects invalid -o/--origin' '\n> +\n> +\ttest_expect_code 128 git clone -o \"bad...name\" parent clone-bad-name 2>err &&\n> +\ttest_debug \"cat err\" &&\n> +\ttest_i18ngrep \"'\\''bad...name'\\'' is not a valid origin name\" err\n> +\n> +'\n\n... and we can also test for \"bad/name\" here in another test.\n\n> test_expect_success 'disallows --bare with --origin' '\n> \n> \ttest_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&\n\nThanks.\n"},{"id":"405455","messageId":"xmqqwo10m3kv.fsf@gitster.c.googlers.com","threadId":"54232","inReplyTo":"5c519376c29a28bd89a712cf0b8125fc1c2c81be.1599848727.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] clone: allow configurable default for `-o`/`--origin`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-11T21:00:16Z","receivedAt":"2020-09-11T21:00:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Sean Barag via GitGitGadget\" writes:\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 1cd62d0001..aeb41f15f3 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -53,6 +53,7 @@ static int option_shallow_submodules;\n> static int deepen;\n> static char *option_template, *option_depth, *option_since;\n> static char *option_origin = NULL;\n> +static char *remote_name = \"origin\";\n\nThis has a side effect of making all the code locations that used to\nrefer to option_origin much easier to read, like ...\n\n> @@ -721,7 +722,7 @@ static void update_head(const struct ref *our, const struct ref *remote,\n> \t\tif (!option_bare) {\n> \t\t\tupdate_ref(msg, \"HEAD\", &our->old_oid, NULL, 0,\n> \t\t\t\t UPDATE_REFS_DIE_ON_ERR);\n> -\t\t\tinstall_branch_config(0, head, option_origin, our->name);\n> +\t\t\tinstall_branch_config(0, head, remote_name, our->name);\n\n... this place ;-) Happy.\n\n> static int git_clone_config(const char *k, const char *v, void *cb)\n> {\n> +\tif (!strcmp(k, \"clone.defaultremotename\") && !option_origin)\n> +\t\tremote_name = xstrdup(v);\n> \treturn git_default_config(k, v, cb);\n> }\n\nclone.defaultremotename is a single valued configuration variable,\nand this correctly implements the \"last one wins\" behaviour (but\nprevious remote_name will leak every time clone.defaultremotename\nis seen in the config stream).\n\nAlso this code arrangement is not quite satisfactory. It means that\nwe cannot re-read any configuration variable that does not have an\naccompanying command line option. I thought the whole point of\ndoing the write_config() was so that anything came from the command\nline option can be written back to the configuration file, so I am\nnot sure what the harm would be to update remote_name from the\nconfiguration whether option_origin is used or not here. Perhaps\nadd \"clone.defaultremotename\" to the set of configuration setting\nwrite_config() uses, when --option is given from the command line,\nand remove this special case?\n\nBy the way, I now realized why 2/4's \"read twice\" is OK. init_db()\ncalls create_default_files() and we do clare the cached configset by\ncalling git_config_clear() there.\n\nThanks.\n"},{"id":"405456","messageId":"xmqqsgbom3ho.fsf@gitster.c.googlers.com","threadId":"54232","inReplyTo":"8f864b6c-4b05-a9d6-b649-5f7c50697730@gmail.com","subject":"Re: [PATCH 1/4] clone: add tests for --template and some disallowed option pairs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-11T21:02:11Z","receivedAt":"2020-09-11T21:02:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n>> +test_expect_success 'disallows --bare with --origin' '\n>> +\n>> +\ttest_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&\n>> +\ttest_debug \"cat err\" &&\n>> +\ttest_i18ngrep \"\\-\\-bare and --origin foo options are incompatible\" err\n>> +\n>> +'\n>\n> It seems that all of your tests have an extraneous newline\n> at the end.\n\nThat matches the older style to make the test body stand out by\nhaving blank lines around it. All existing tests from the era (not\nlimited to this script) used to do so.\n\nIt is OK to update them to modern style, but let's not do so in the\nmiddle of the series.\n\nThanks.\n"},{"id":"405461","messageId":"bfb8ed4a-604c-7c70-19cf-dea4a8bfe4c4@gmail.com","threadId":"54232","inReplyTo":"xmqqsgbom3ho.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/4] clone: add tests for --template and some disallowed option pairs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-09-12T00:41:17Z","receivedAt":"2020-09-12T00:41:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/11/2020 5:02 PM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n>>> +test_expect_success 'disallows --bare with --origin' '\n>>> +\n>>> +\ttest_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&\n>>> +\ttest_debug \"cat err\" &&\n>>> +\ttest_i18ngrep \"\\-\\-bare and --origin foo options are incompatible\" err\n>>> +\n>>> +'\n>>\n>> It seems that all of your tests have an extraneous newline\n>> at the end.\n> \n> That matches the older style to make the test body stand out by\n> having blank lines around it. All existing tests from the era (not\n> limited to this script) used to do so.\n> \n> It is OK to update them to modern style, but let's not do so in the\n> middle of the series.\n\nThanks for correcting me. (and Peff for doing the same).\n\nI should have looked at the context better, because that\nis frequently the reason for \"inconsistent\" style. My\napologies.\n\n-Stolee\n\n\n"},{"id":"405463","messageId":"20200912031752.GA1988@nand.local","threadId":"54232","inReplyTo":"20200911195622.GB2697399@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] clone: add tests for --template and some disallowed option pairs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-09-12T03:17:52Z","receivedAt":"2020-09-12T03:18:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Sep 11, 2020 at 03:56:22PM -0400, Jeff King wrote:\n> - I didn't even know we had test_debug. ;) The last time somebody added\n> a call to it was in 2012. I think it's being used as intended here,\n> but I'm not sure that the clutter to the test is worth it (we have\n> other tools like \"-i\" to stop at the right spot and let you inspect\n> the broken state).\n\nMe either :). I think what you said about using -i to inspect whatever\nis broken applies not only to this test, but also to all other tests in\nthe suite.\n\nI'd be pretty happy to see 'test_debug' go away. There are only 104 uses\nof it in the tree, and I think most of them could simply be removed\nwithout needing to touch anything else.\n\nI've never found it useful in debugging a problem on my end, but perhaps\nothers have. Food for thought.\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"405583","messageId":"20200915160943.1253867-1-sean@barag.org","threadId":"54232","inReplyTo":"20200911195622.GB2697399@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] clone: add tests for --template and some disallowed option pairs","fromName":"Sean Barag","fromEmail":"sean@barag.org","sentAt":"2020-09-15T16:09:44Z","receivedAt":"2020-09-15T22:39:03Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"On Fri, 11 Sep 2020 15:56:22 -0400, Jeff King wrote:\n > do we really care about code 128, or just failure? test_must_fail\n might be a better choice\n\nGood point - `test_must_fail` is probably fine here. I went with an\nexplicit error code so this test wouldn't pass in the event of an\noutright crash, but I'm happy to adjust for v2.\n\n> I didn't even know we had test_debug. ;) The last time somebody added\n a call to it was in 2012. I think it's being used as intended here,\n but I'm not sure that the clutter to the test is worth it (we have\n other tools like \"-i\" to stop at the right spot and let you inspect\n the broken state).\n\nFrankly I'd forgotten I'd included it! It's definitely not necessary.\nWill remove for v2 as well.\n\n> the backslash escapes confused me for a moment. I guess they are\n trying to hide the dashes from grep's option parser. That's OK,\n though I'd have probably just started with \"bare\" since we're\n matching a substring anyway. I think you could also use \"-e\" with\n test_i18ngrep.\n\nAdding `-e` would solve this handily. Thanks for the suggestion!\n\nSean Barag\n"},{"id":"405597","messageId":"20200916031549.815253-1-sean@barag.org","threadId":"54232","inReplyTo":"CAPig+cTNjkOSRbP6sQMAhr-etXMt7ejJo3Zm2gu=nvpF00CMvw@mail.gmail.com","subject":"Re: [PATCH 0/4] clone: allow configurable default for -o/--origin","fromName":"Sean Barag","fromEmail":"sean@barag.org","sentAt":"2020-09-16T03:15:49Z","receivedAt":"2020-09-16T03:16:19Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"> A couple more observations related to a few of the subsequent tests.\n> This:\n>\n> template=\"$TRASH_DIRECTORY/template-with-config\" &&\n>\n> probably doesn't need $TRASH_DIRECTORY since that happens to be the\n> current working directory anyhow, so:\n>\n> template=template-with-config &&\n>\n> should suffice (unless you had a problem doing it that way). Or you\n> could drop the variable altogether and use the literal name where you\n> need it.\n\nI hadn't realized $TRASH_DIRECTORY was the pwd, but it makes perfect\nsense. Will update for v2.\n\n> Also, instead of:\n>\n> (cd clone-template-config && test \"$(git config --local foo.bar)\"\n> = \"from_template\")\n>\n> would probably be written these days as:\n>\n> echo from_template >expect &&\n> git -C clone-template-config config --local foo.bar >actual &&\n> test_cmp expect actual\n\nOooh `test_cmp` looks much better. Thanks for the tip!\n"},{"id":"405680","messageId":"20200916161227.1873247-1-sean@barag.org","threadId":"54232","inReplyTo":"xmqq5z8knjou.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/4] clone: call git_config before parse_options","fromName":"Sean Barag","fromEmail":"sean@barag.org","sentAt":"2020-09-16T16:12:27Z","receivedAt":"2020-09-16T20:24:42Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"> From: Junio C Hamano \n> > From: Sean Barag \n> >\n> > While Junio's request [1] was to avoids the unusual \"write config\n> > then immediately read it\" pattern that exists in `cmd_clone`,\n> > Johannes mentioned that --template can write new config values that\n> > aren't automatically included in the environment [2]. This requires\n> > a config re-read after `init_db` is called.\n> >\n> > Moving the initial config up does allow settings from config to be\n> > overwritten by ones provided via CLI options in a more natural way\n> > though, so that part of Junio's suggestion remains.\n>\n> The title says what the code does after this change. The code calls\n> git_config() before calling parse_options(), but not much in the\n> proposed log message explains what the patch tries to achieve by doing\n> so.\n>\n> The above refers to suggestions but does not describe what problem the\n> patch is trying to address and what approach is taken to address it.\n\nThanks for the pointer - I completely agree. Rewriting for v2.\n\n> > +static int git_clone_config(const char *k, const char *v, void *cb)\n> > +{\n> > +\treturn git_default_config(k, v, cb);\n> > +}\n> > +\n> > static int write_one_config(const char *key, const char *value, void *data)\n> > {\n> > +\t/*\n> > +\t * give git_config_default a chance to write config values back to the environment, since\n> > +\t * git_config_set_multivar_gently only deals with config-file writes\n> > +\t */\n>\n> Overlong lines...\n\nHow embarrassing! Re-wrapped for v2.\n\n> > +\tint apply_failed = git_default_config(key, value, data);\n>\n> Not git_clone_config()? Presumably you'll make git_clone_config()\n> recognise more variables than git_default_config() does, and the\n> caller of this helper wants us to recognise \"clone.*\" that are\n> ignored by git_default_config() callback, no?\n\nGreat catch! This gets changed to `git_clone_config` in patch 4/4\nanyway, so there's no need for the confusing intermediate state in 2/4.\nWill be fixed in v2.\n\n--\nThanks for being so patient with a newbie :)\nSean\n"},{"id":"405682","messageId":"20200916171151.1890682-1-sean@barag.org","threadId":"54232","inReplyTo":"xmqq1rj8nj47.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/4] clone: validate --origin option before use","fromName":"Sean Barag","fromEmail":"sean@barag.org","sentAt":"2020-09-16T17:11:51Z","receivedAt":"2020-09-16T20:35:17Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"On Fri, 11 Sep 2020 at 13:39:20 -0700, Junio C Hamano writes:\n> > From: Sean Barag \n> >\n> > Providing a bad origin name to `git clone` currently reports an\n> > 'invalid refspec' error instead of a more explicit message\n> > explaining that the `--origin` option was malformed. Per Junio,\n> > it's been this way since 8434c2f1 (Build in clone, 2008-04-27).\n>\n> If you know it as a fact that it has been this way since the first\n> version of rewritten \"git clone\", don't blame others.\n\nOh gosh, I'm so sorry! I didn't mean for that to read as blaming, was\njust trying to cite you as my source. On a second read, it comes across\nmore \"blame-y\" than I originally intended. I'll fix this in v2 (and\nhave learned a valuable lesson :) ).\n\n> A micronit. We describe the current status in present tense when\n> presenting the problem to be solved, so \"currently\" can be dropped.\n>\n> > This patch reintroduces\n>\n> When presenting the solution, we write as if we are giving an order\n> to a patch monkey to \"make the code like so\".\n>\n> \"This patch reintroduces\" -> \"Reintroduce\".\n\nGreat to know! Thanks again for helping a newbie fit in. Will fix in\nv2.\n\n> > validation for the provided `--origin` option, but notably _doesn't_\n> > include a multi-level check (e.g. \"foo/bar\") that was present in the\n> > original `git-clone.sh`. It seems `git remote` allows multi-level\n> > remote names, so applying that same validation in `git clone` seems\n> > reasonable.\n>\n> Even though I suspect \"git remote\" is being overly loose and\n> careless here, I am not sure if it is a good idea to tighten it\n> retroactively. But if this is meant as a bugfix for misconversion\n> made in 8434c2f1, we should be as strict as the original. I dunno.\n\n\nTo be honest, I'm struggling to decide which route to go. It seems\nlike multilevel fetch and push refspecs are allowed as far back as\n46220ca100 (remote.c: Fix overtight refspec validation, 2008-03-20) or\nef00d150e4 (Tighten refspec processing, 2008-03-17), so perhaps\nremoving the multilevel check in 8434c2f1 (Build in clone, 2008-04-27)\nwas intentional? If removing that check in 8434c2f1 was a mistake and\nwe reintroduce it, that's probably a breaking change for some users.\nWhat sort of accommodations would I need to include in this patchset to\nease that pain for users?\n\n> > +\tstrbuf_addf(&resolved_refspec, \"refs/heads/test:refs/remotes/%s/test\", option_origin);\n> > +\tif (!valid_fetch_refspec(resolved_refspec.buf))\n>\n> If we reintroduce pre-8434c2f1 check, then we would want\n>\n> \tif (!valid_fetch_refspec(...) || strchr(option_origin, '/'))\n>\n> or something like that?\n\nAbsolutely! Happy to include the multilevel check if you think it's the\nright path forward (see above).\n\n> > +\t\t/* TRANSLATORS: %s will be the user-provided --origin / -o option */\n> > +\t\tdie(_(\"'%s' is not a valid origin name\"), option_origin);\n>\n> I am not sure if this translator comment is helping.\n>\n> The message makes it sound as if \"%s\" here is used to switch between\n> '-o' or '--origin'. If it said \"'%s' will be the value given to\n> --origin/-o option\", it would become much less confusing.\n\nAgreed. I plan to take Derrick's suggestion [1] and use the same\n\" is not a valid remote name\" message from `builtin/remote.c`, which\nshould make that translator comment a non-issue.\n\n[1] https://lore.kernel.org/git/bf0107fb-2a6c-68d3-df24-72c6a9df6182@gmail.com/\n\n\nI can't stress enough how sorry I am for the improper blame, and how\nmuch I appreciate your help!\n--\nSean\n"},{"id":"405684","messageId":"20200916163617.GC17726@coredump.intra.peff.net","threadId":"54232","inReplyTo":"20200915160943.1253867-1-sean@barag.org","subject":"Re: [PATCH 1/4] clone: add tests for --template and some disallowed option pairs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-16T16:36:17Z","receivedAt":"2020-09-16T20:45:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 15, 2020 at 09:09:44AM -0700, Sean Barag wrote:\n\n> On Fri, 11 Sep 2020 15:56:22 -0400, Jeff King wrote:\n> > do we really care about code 128, or just failure? test_must_fail\n> might be a better choice\n> \n> Good point - `test_must_fail` is probably fine here. I went with an\n> explicit error code so this test wouldn't pass in the event of an\n> outright crash, but I'm happy to adjust for v2.\n\nThat's good thinking, but test_must_fail already has you covered; it\nwill complain about any death-by-signal. It wouldn't distinguish\nbetween, say exit codes 128 and 1, but 128 is the code used by our die()\nfunction, so expecting it isn't very specific anyway. :)\n\n-Peff\n"},{"id":"405687","messageId":"20200916162839.1877248-1-sean@barag.org","threadId":"54232","inReplyTo":"bf0107fb-2a6c-68d3-df24-72c6a9df6182@gmail.com","subject":"Re: [PATCH 3/4] clone: validate --origin option before use","fromName":"Sean Barag","fromEmail":"sean@barag.org","sentAt":"2020-09-16T16:28:39Z","receivedAt":"2020-09-16T20:48:00Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"On Fri, 11 Sep 2020 at 15:24:15 -0400, Derrick Stolee writes:\n> On 9/11/2020 2:25 PM, Sean Barag via GitGitGadget wrote:\n> > +\tstrbuf_addf(&resolved_refspec, \"refs/heads/test:refs/remotes/%s/test\", option_origin);\n> > +\tif (!valid_fetch_refspec(resolved_refspec.buf))\n> > +\t\t/* TRANSLATORS: %s will be the user-provided --origin / -o option */\n> > +\t\tdie(_(\"'%s' is not a valid origin name\"), option_origin);\n>\n> Looking at this again, I'm not sure the translators note is\n> necessary. Also, I would say \"is not a valid remote name\".\n> That makes the string align with the already-translated string\n> in builtin/remote.c.\n\nMakes perfect sense! My original intention was to provide a separate\nuse-case specific message, but you're right: \"is not a valid remote\nname\" (as in `builtin/remote.c`) is still very clear.\n\n> This code is duplicated from builtin/remote.c, so I'd rather\n> see this be a helper method in refspec.c and have both\n> builtin/clone.c and builtin/remote.c call that helper.\n>\n> Here is the helper:\n>\n> void valid_remote_name(const char *name)\n> {\n> \tint result;\n> \tstruct strbuf refspec = STRBUF_INIT;\n> \tstrbuf_addf(&refspec, \"refs/heads/test:refs/remotes/%s/test\", name);\n> \tresult = valid_fetch_refspec(refspec.buf);\n> \tstrbuf_release(&refspec);\n> \treturn result;\n> }\n>\n> And here is the use in builtin/clone.c:\n>\n> \tif (!valid_remote_name(option_origin))\n> \t\tdie(_(\"'%s' is not a valid remote name\"), option_origin);\n>\n> and in builtin/remote.c:\n>\n> \tif (!valid_remote_name(name))\n> \t\tdie(_(\"'%s' is not a valid remote name\"), name);\n\nThat's a fantastic idea -- thanks for the suggestion! I'll do that for\nv2.\n\n> > +test_expect_success 'rejects invalid -o/--origin' '\n> > +\n> > +\ttest_expect_code 128 git clone -o \"bad...name\" parent clone-bad-name 2>err &&\n> > +\ttest_debug \"cat err\" &&\n> > +\ttest_i18ngrep \"'\\''bad...name'\\'' is not a valid origin name\" err\n> > +\n> > +'\n> > +\n>\n> Double newlines here! I personally appreciate newlines to\n> spread out content, but it doesn't fit our guidelines.\n\nDarn, I missed this one. Thanks for the heads-up :)\n\n--\nSean\n"},{"id":"405759","messageId":"83b5a9a0-6982-2bf7-139e-5cf2823f223c@gmail.com","threadId":"54232","inReplyTo":"5c519376c29a28bd89a712cf0b8125fc1c2c81be.1599848727.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] clone: allow configurable default for `-o`/`--origin`","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2020-09-17T15:25:07Z","receivedAt":"2020-09-17T15:26:49Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 2020-09-11 20:25, Sean Barag via GitGitGadget wrote:\n> From: Sean Barag \n>\n> While the default remote name of \"origin\" can be changed at clone-time\n> with `git clone`'s `--origin` option, it was previously not possible\n> to specify a default value for the name of that remote. This commit\n> adds support for a new `clone.defaultRemoteName` config.\n>\n> It's resolved in the expected priority order:\n>\n> 1. (Highest priority) A remote name passed directly to `git clone -o`\n> 2. A `clone.defaultRemoteName=new_name` in config `git clone -c`\n> 3. A `clone.defaultRemoteName` value set in `/path/to/template/config`,\n> where `--template=/path/to/template` is provided\n> 3. A `clone.defaultRemoteName` value set in a non-template config file\n\nNumber 3 is used twice in this list.\n\n> 4. The default value of `origin`\n>\n> Signed-off-by: Sean Barag \n> Thanks-to: Junio C Hamano \n> Thanks-to: Johannes Schindelin \n\n"},{"id":"406053","messageId":"20200921161323.2381099-1-sean@barag.org","threadId":"54232","inReplyTo":"20200916171151.1890682-1-sean@barag.org","subject":"Re: [PATCH 3/4] clone: validate --origin option before use","fromName":"Sean Barag","fromEmail":"sean@barag.org","sentAt":"2020-09-21T16:13:23Z","receivedAt":"2020-09-21T16:13:42Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"On Wed, 16 Sep 2020 at 10:11:51 -0700, Sean Barag writes:\n> > > validation for the provided `--origin` option, but notably\n> > > _doesn't_ include a multi-level check (e.g. \"foo/bar\") that was\n> > > present in the original `git-clone.sh`. It seems `git remote`\n> > > allows multi-level remote names, so applying that same validation\n> > > in `git clone` seems reasonable.\n> >\n> > Even though I suspect \"git remote\" is being overly loose and\n> > careless here, I am not sure if it is a good idea to tighten it\n> > retroactively. But if this is meant as a bugfix for misconversion\n> > made in 8434c2f1, we should be as strict as the original. I dunno.\n>\n>\n> To be honest, I'm struggling to decide which route to go. It seems\n> like multilevel fetch and push refspecs are allowed as far back as\n> 46220ca100 (remote.c: Fix overtight refspec validation, 2008-03-20) or\n> ef00d150e4 (Tighten refspec processing, 2008-03-17), so perhaps\n> removing the multilevel check in 8434c2f1 (Build in clone, 2008-04-27)\n> was intentional? If removing that check in 8434c2f1 was a mistake and\n> we reintroduce it, that's probably a breaking change for some users.\n> What sort of accommodations would I need to include in this patchset\n> to ease that pain for users?\n\nThinking about this more, I'd be very surprised as a `git` user if\nintroducing a new config option broke backwards compatibility. Other\n`git` UIs have mixed support for slashes in remote names:\n\n* GitHub Desktop has an open issue regarding remote names that contain\n slashes: https://github.com/desktop/desktop/issues/3618\n* Sublime Merge (as-of build 2032) renders remote names with slashes as\n a tree of remotes, e.g.:\n\n $ git remote -v\n foo/bar /tmp/example_repo\n foo/baz /tmp/example_repo2\n\n is rendered with as a collapsible tree, roughly:\n\n REMOTES (2)\n v foo\n bar\n baz\n\n* `tig` (2.4.1) renders remote names with slashes identically to those\n without slashes\n\nRetroactively tightening those rules would cause more harm than good\n(both for end-users and for downstream projects), especially with no\nsafe way to fix existing /-containing remote names. I'm going to keep\nthe multilevel check out of this patchset (thus continuing to allowing\nmultilevel remote names), but if anyone feels strongly either way please\nlet me know! :)\n\nSean\n"},{"id":"406540","messageId":"20200928160217.3412067-1-sean@barag.org","threadId":"54232","inReplyTo":"xmqqwo10m3kv.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 4/4] clone: allow configurable default for `-o`/`--origin`","fromName":"Sean Barag","fromEmail":"sean@barag.org","sentAt":"2020-09-28T16:02:17Z","receivedAt":"2020-09-28T16:02:39Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"Junio C Hamano writes:\n> clone.defaultremotename is a single valued configuration variable,\n> and this correctly implements the \"last one wins\" behaviour (but\n> previous remote_name will leak every time clone.defaultremotename\n> is seen in the config stream).\n\nGreat catch - fixed for v2.\n\n> I thought the whole point of doing the write_config() was so that\n> anything came from the command line option can be written back to the\n> configuration file, so I am not sure what the harm would be to update\n> remote_name from the configuration whether option_origin is used or\n> not here. Perhaps add \"clone.defaultremotename\" to the set of\n> configuration setting write_config() uses, when --option is given from\n> the command line, and remove this special case?\n\nThat's true! The special case was there mostly to keep --origin as the\nhighest priority, but that can be solved much more naturally by moving\nthe assignment from option_origin to remote_name down below the second\ngit_config call. Included in v2.\n\nSean\n"},{"id":"406541","messageId":"20200928160457.3412744-1-sean@barag.org","threadId":"54232","inReplyTo":"0739858e-83f1-3b3f-aba0-7549e7e47cfc@gmail.com","subject":"Re: [PATCH 4/4] clone: allow configurable default for `-o`/`--origin`","fromName":"Sean Barag","fromEmail":"sean@barag.org","sentAt":"2020-09-28T16:04:57Z","receivedAt":"2020-09-28T16:05:20Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"Derrick Stolee writes:\n> > static char *option_origin = NULL;\n> > +static char *remote_name = \"origin\";\n> \n> This patch could have used a prep patch that had all consumers\n> of option_origin use remote_name instead, with this adjustment\n> to the way to use option_origin:\n> \n> > -\tif (!option_origin)\n> > -\t\toption_origin = \"origin\";\n> > +\tif (option_origin)\n> > +\t\tremote_name = option_origin;\n> > +\telse\n> > +\t\tremote_name = \"origin\";\n> \n> Then this patch introducing the config option would have a\n> very limited change in the builtin consisting of these two\n> hunks:\n> \n> > static int git_clone_config(const char *k, const char *v, void *cb)\n> > {\n> > +\tif (!strcmp(k, \"clone.defaultremotename\") && !option_origin)\n> > +\t\tremote_name = xstrdup(v);\n> > \treturn git_default_config(k, v, cb);\n> > }\n> ...\n> > \tif (option_origin)\n> > \t\tremote_name = option_origin;\n> > -\telse\n> > -\t\tremote_name = \"origin\"\n\nThat's a great idea! Implemented for v2 :)\n\nSean\n"},{"id":"406610","messageId":"29ab418b1b8028c566b1008a0673ad0f6c6d112f.1601350615.git.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":"pull.727.v2.git.1601350615.gitgitgadget@gmail.com","subject":"[PATCH v2 1/7] clone: add tests for --template and some disallowed option pairs","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-29T03:36:49Z","receivedAt":"2020-09-29T03:37:01Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"From: Sean Barag \n\nSome combinations of command-line options to `git clone` are invalid,\nbut there were previously no tests ensuring those combinations reported\nerrors. Similarly, `git clone --template` didn't appear to have any\ntests.\n\nHelped-by: Jeff King \nHelped-by: Derrick Stolee \nSigned-off-by: Sean Barag \n---\n t/t5606-clone-options.sh | 46 +++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 45 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh\nindex e69427f881..0422b24258 100755\n--- a/t/t5606-clone-options.sh\n+++ b/t/t5606-clone-options.sh\n@@ -15,7 +15,51 @@ test_expect_success 'setup' '\n test_expect_success 'clone -o' '\n \n \tgit clone -o foo parent clone-o &&\n-\t(cd clone-o && git rev-parse --verify refs/remotes/foo/master)\n+\tgit -C clone-o rev-parse --verify refs/remotes/foo/master\n+\n+'\n+\n+test_expect_success 'disallows --bare with --origin' '\n+\n+\ttest_must_fail git clone -o foo --bare parent clone-bare-o 2>err &&\n+\ttest_debug \"cat err\" &&\n+\ttest_i18ngrep -e \"--bare and --origin foo options are incompatible\" err\n+\n+'\n+\n+test_expect_success 'disallows --bare with --separate-git-dir' '\n+\n+\ttest_must_fail git clone --bare --separate-git-dir dot-git-destiation parent clone-bare-sgd 2>err &&\n+\ttest_debug \"cat err\" &&\n+\ttest_i18ngrep -e \"--bare and --separate-git-dir are incompatible\" err\n+\n+'\n+\n+test_expect_success 'uses \"origin\" for default remote name' '\n+\n+\tgit clone parent clone-default-origin &&\n+\tgit -C clone-default-origin rev-parse --verify refs/remotes/origin/master\n+\n+'\n+\n+test_expect_success 'prefers --template config over normal config' '\n+\n+\ttemplate=\"$TRASH_DIRECTORY/template-with-config\" &&\n+\tmkdir \"$template\" &&\n+\tgit config --file \"$template/config\" foo.bar from_template &&\n+\ttest_config_global foo.bar from_global &&\n+\tgit clone \"--template=$template\" parent clone-template-config &&\n+\ttest \"$(git -C clone-template-config config --local foo.bar)\" = \"from_template\"\n+\n+'\n+\n+test_expect_success 'prefers -c config over --template config' '\n+\n+\ttemplate=\"$TRASH_DIRECTORY/template-with-ignored-config\" &&\n+\tmkdir \"$template\" &&\n+\tgit config --file \"$template/config\" foo.bar from_template &&\n+\tgit clone \"--template=$template\" -c foo.bar=inline parent clone-template-inline-config &&\n+\ttest \"$(git -C clone-template-inline-config config --local foo.bar)\" = \"inline\"\n \n '\n \n-- \ngitgitgadget\n\n"},{"id":"406611","messageId":"pull.727.v2.git.1601350615.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":"pull.727.git.1599848727.gitgitgadget@gmail.com","subject":"[PATCH v2 0/7] clone: allow configurable default for -o/--origin","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-29T03:36:48Z","receivedAt":"2020-09-29T03:37:01Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"v2 (changes since v1):\n\n * Convert Thanks-to trailer to Helped-by\n * Rewrite several commit titles and messages\n * Unify error reporting between clone.c and remote.c\n * Add tests for git remote add and git remote rename with invalid remote\n names\n * Prevent leak of old remote_name\n\nv1: Took another pass at supporting a configurable default for-o/--origin,\nthis time following Junio's suggestions from a previous approach as much as\npossible [1]. Unfortunately, Johannes mentioned that --template can write\nnew config values that aren't automatically merged without re-calling \ngit_config. There doesn't appear to be a way around this without rewriting\nsignificant amounts of init and config logic across the codebase.\n\nWhile this could have been v2 of the original patchset, it's diverged so\ndrastically from the original that it likely warrants its own root thread.\nIf that's not appropriate though, I'd be happy to restructure!\n\nThanks for all the advice Junio and Johannes! This feels significantly\nbetter than my first attempt.\n\n[1] \nhttps://lore.kernel.org/git/pull.710.git.1598456751674.gitgitgadget@gmail.com/\n[2] https://github.com/gitgitgadget/git/pull/727#issuecomment-689740195\n\nSean Barag (7):\n clone: add tests for --template and some disallowed option pairs\n clone: use more conventional config/option layering\n remote: add tests for add and rename with invalid names\n refs: consolidate remote name validation\n clone: validate --origin option before use\n clone: read new remote name from remote_name instead of option_origin\n clone: allow configurable default for `-o`/`--origin`\n\n Documentation/config.txt | 2 +\n Documentation/config/clone.txt | 5 +++\n Documentation/git-clone.txt | 5 ++-\n builtin/clone.c | 70 ++++++++++++++++++++++++++--------\n builtin/remote.c | 7 +---\n refspec.c | 10 +++++\n refspec.h | 1 +\n t/t5505-remote.sh | 13 +++++++\n t/t5606-clone-options.sh | 68 ++++++++++++++++++++++++++++++++-\n 9 files changed, 158 insertions(+), 23 deletions(-)\n create mode 100644 Documentation/config/clone.txt\n\n\nbase-commit: 9bc233ae1cf19a49e51842c7959d80a675dbd1c0\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-727%2Fsjbarag%2Fclone_add-configurable-default-for--o-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-727/sjbarag/clone_add-configurable-default-for--o-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/727\n\nRange-diff vs v1:\n\n 1: 4cdcedff31 ! 1: 29ab418b1b clone: add tests for --template and some disallowed option pairs\n @@ Commit message\n errors. Similarly, `git clone --template` didn't appear to have any\n tests.\n \n + Helped-by: Jeff King \n + Helped-by: Derrick Stolee \n Signed-off-by: Sean Barag \n \n ## t/t5606-clone-options.sh ##\n -@@ t/t5606-clone-options.sh: test_expect_success 'clone -o' '\n - \n - '\n +@@ t/t5606-clone-options.sh: test_expect_success 'setup' '\n + test_expect_success 'clone -o' '\n \n + \tgit clone -o foo parent clone-o &&\n +-\t(cd clone-o && git rev-parse --verify refs/remotes/foo/master)\n ++\tgit -C clone-o rev-parse --verify refs/remotes/foo/master\n ++\n ++'\n ++\n +test_expect_success 'disallows --bare with --origin' '\n +\n -+\ttest_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&\n ++\ttest_must_fail git clone -o foo --bare parent clone-bare-o 2>err &&\n +\ttest_debug \"cat err\" &&\n -+\ttest_i18ngrep \"\\-\\-bare and --origin foo options are incompatible\" err\n ++\ttest_i18ngrep -e \"--bare and --origin foo options are incompatible\" err\n +\n +'\n +\n +test_expect_success 'disallows --bare with --separate-git-dir' '\n +\n -+\ttest_expect_code 128 git clone --bare --separate-git-dir dot-git-destiation parent clone-bare-sgd 2>err &&\n ++\ttest_must_fail git clone --bare --separate-git-dir dot-git-destiation parent clone-bare-sgd 2>err &&\n +\ttest_debug \"cat err\" &&\n -+\ttest_i18ngrep \"\\-\\-bare and --separate-git-dir are incompatible\" err\n ++\ttest_i18ngrep -e \"--bare and --separate-git-dir are incompatible\" err\n +\n +'\n +\n +test_expect_success 'uses \"origin\" for default remote name' '\n +\n +\tgit clone parent clone-default-origin &&\n -+\t(cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master)\n ++\tgit -C clone-default-origin rev-parse --verify refs/remotes/origin/master\n +\n +'\n +\n @@ t/t5606-clone-options.sh: test_expect_success 'clone -o' '\n +\tgit config --file \"$template/config\" foo.bar from_template &&\n +\ttest_config_global foo.bar from_global &&\n +\tgit clone \"--template=$template\" parent clone-template-config &&\n -+\t(cd clone-template-config && test \"$(git config --local foo.bar)\" = \"from_template\")\n ++\ttest \"$(git -C clone-template-config config --local foo.bar)\" = \"from_template\"\n +\n +'\n +\n @@ t/t5606-clone-options.sh: test_expect_success 'clone -o' '\n +\tmkdir \"$template\" &&\n +\tgit config --file \"$template/config\" foo.bar from_template &&\n +\tgit clone \"--template=$template\" -c foo.bar=inline parent clone-template-inline-config &&\n -+\t(cd clone-template-inline-config && test \"$(git config --local foo.bar)\" = \"inline\")\n -+\n -+'\n -+\n - test_expect_success 'redirected clone does not show progress' '\n ++\ttest \"$(git -C clone-template-inline-config config --local foo.bar)\" = \"inline\"\n + \n + '\n \n - \tgit clone \"file://$(pwd)/parent\" clone-redirected >out 2>err &&\n 2: 51ef776f85 ! 2: e3479e7b35 clone: call git_config before parse_options\n @@ Metadata\n Author: Sean Barag \n \n ## Commit message ##\n - clone: call git_config before parse_options\n + clone: use more conventional config/option layering\n \n - While Junio's request [1] was to avoids the unusual \"write config then\n - immediately read it\" pattern that exists in `cmd_clone`, Johannes\n - mentioned that --template can write new config values that aren't\n - automatically included in the environment [2]. This requires a config\n - re-read after `init_db` is called.\n -\n - Moving the initial config up does allow settings from config to be\n - overwritten by ones provided via CLI options in a more natural way\n - though, so that part of Junio's suggestion remains.\n -\n - [1] https://lore.kernel.org/git/pull.710.git.1598456751674.gitgitgadget@gmail.com/\n - [2] https://github.com/gitgitgadget/git/pull/727#issuecomment-689740195\n + Parsing command-line options before reading from config required careful\n + handling to ensure CLI options were treated with higher priority. Read\n + config first to let parsed CLI naively overwrite matching config values.\n \n + Helped-by: Junio C Hamano \n + Helped-by: Johannes Schindelin \n Signed-off-by: Sean Barag \n - Thanks-to: Junio C Hamano \n - Thanks-to: Johannes Schindelin \n \n ## builtin/clone.c ##\n @@ builtin/clone.c: static int checkout(int submodule_progress)\n @@ builtin/clone.c: static int checkout(int submodule_progress)\n static int write_one_config(const char *key, const char *value, void *data)\n {\n +\t/*\n -+\t * give git_config_default a chance to write config values back to the environment, since\n -+\t * git_config_set_multivar_gently only deals with config-file writes\n ++\t * give git_clone_config a chance to write config values back to the\n ++\t * environment, since git_config_set_multivar_gently only deals with\n ++\t * config-file writes\n +\t */\n -+\tint apply_failed = git_default_config(key, value, data);\n ++\tint apply_failed = git_clone_config(key, value, data);\n +\tif (apply_failed)\n +\t\treturn apply_failed;\n +\n -: ---------- > 3: 85be780b8e remote: add tests for add and rename with invalid names\n -: ---------- > 4: 42dc18601a refs: consolidate remote name validation\n -: ---------- > 5: fdc9d3b369 clone: validate --origin option before use\n 4: 5c519376c2 ! 6: 99f4d765b4 clone: allow configurable default for `-o`/`--origin`\n @@ Metadata\n Author: Sean Barag \n \n ## Commit message ##\n - clone: allow configurable default for `-o`/`--origin`\n + clone: read new remote name from remote_name instead of option_origin\n \n - While the default remote name of \"origin\" can be changed at clone-time\n - with `git clone`'s `--origin` option, it was previously not possible\n - to specify a default value for the name of that remote. This commit\n - adds support for a new `clone.defaultRemoteName` config.\n -\n - It's resolved in the expected priority order:\n -\n - 1. (Highest priority) A remote name passed directly to `git clone -o`\n - 2. A `clone.defaultRemoteName=new_name` in config `git clone -c`\n - 3. A `clone.defaultRemoteName` value set in `/path/to/template/config`,\n - where `--template=/path/to/template` is provided\n - 3. A `clone.defaultRemoteName` value set in a non-template config file\n - 4. The default value of `origin`\n + In a future patch, the name of the remote created by `git clone` may\n + come from multiple sources. To avoid confusion, convert most uses of\n + option_origin to remote_name, leaving option_origin to exclusively\n + represent the -o/--origin option.\n \n + Helped-by: Derrick Stolee \n Signed-off-by: Sean Barag \n - Thanks-to: Junio C Hamano \n - Thanks-to: Johannes Schindelin \n -\n - ## Documentation/config.txt ##\n -@@ Documentation/config.txt: include::config/checkout.txt[]\n - \n - include::config/clean.txt[]\n - \n -+include::config/clone.txt[]\n -+\n - include::config/color.txt[]\n - \n - include::config/column.txt[]\n -\n - ## Documentation/config/clone.txt (new) ##\n -@@\n -+clone.defaultRemoteName::\n -+\tThe name of the remote to create when cloning a repository. Defaults to\n -+\t`origin`, and can be overridden by passing the `--origin` command-line\n -+\toption to linkgit:git-clone[1].\n -+\n -\n - ## Documentation/git-clone.txt ##\n -@@ Documentation/git-clone.txt: objects from the source repository into a pack in the cloned repository.\n - \n - -o ::\n - --origin ::\n --\tInstead of using the remote name `origin` to keep track\n --\tof the upstream repository, use ``.\n -+\tInstead of using the remote name `origin` to keep track of the upstream\n -+\trepository, use ``. Overrides `clone.defaultRemoteName` from the\n -+\tconfig.\n - \n - -b ::\n - --branch ::\n \n ## builtin/clone.c ##\n @@ builtin/clone.c: static int option_shallow_submodules;\n @@ builtin/clone.c: static void update_head(const struct ref *our, const struct ref\n \t\t}\n \t} else if (our) {\n \t\tstruct commit *c = lookup_commit_reference(the_repository,\n -@@ builtin/clone.c: static int checkout(int submodule_progress)\n - \n - static int git_clone_config(const char *k, const char *v, void *cb)\n - {\n -+\tif (!strcmp(k, \"clone.defaultremotename\") && !option_origin)\n -+\t\tremote_name = xstrdup(v);\n - \treturn git_default_config(k, v, cb);\n - }\n - \n - static int write_one_config(const char *key, const char *value, void *data)\n - {\n - \t/*\n --\t * give git_config_default a chance to write config values back to the environment, since\n -+\t * give git_clone_config a chance to write config values back to the environment, since\n - \t * git_config_set_multivar_gently only deals with config-file writes\n - \t */\n --\tint apply_failed = git_default_config(key, value, data);\n -+\tint apply_failed = git_clone_config(key, value, data);\n - \tif (apply_failed)\n - \t\treturn apply_failed;\n - \n @@ builtin/clone.c: static void write_refspec_config(const char *src_ref_prefix,\n \t\t}\n \t\t/* Configure the remote */\n @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n +\tif (option_origin)\n +\t\tremote_name = option_origin;\n \n --\tstrbuf_addf(&resolved_refspec, \"refs/heads/test:refs/remotes/%s/test\", option_origin);\n -+\tstrbuf_addf(&resolved_refspec, \"refs/heads/test:refs/remotes/%s/test\", remote_name);\n - \tif (!valid_fetch_refspec(resolved_refspec.buf))\n --\t\t/* TRANSLATORS: %s will be the user-provided --origin / -o option */\n --\t\tdie(_(\"'%s' is not a valid origin name\"), option_origin);\n -+\t\t/*\n -+\t\t * TRANSLATORS: %s will be the user-provided --origin / -o option, or the value\n -+\t\t * of clone.defaultremotename from the config.\n -+\t\t */\n -+\t\tdie(_(\"'%s' is not a valid origin name\"), remote_name);\n - \tstrbuf_release(&resolved_refspec);\n - \n - \trepo_name = argv[0];\n + \tif (!valid_remote_name(remote_name))\n + \t\tdie(_(\"'%s' is not a valid remote name\"), remote_name);\n @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \t\tgit_config_set(\"core.bare\", \"true\");\n @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n -\tremote = remote_get(option_origin);\n +\tremote = remote_get(remote_name);\n \n - \tstrbuf_addf(&default_refspec, \"+%s*:%s*\", src_ref_prefix,\n - \t\t branch_top.buf);\n + \trefspec_appendf(&remote->fetch, \"+%s*:%s*\", src_ref_prefix,\n + \t\t\tbranch_top.buf);\n @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \t\t\tif (!our_head_points_at)\n @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \tif (is_local)\n \t\tclone_local(path, git_dir);\n -\n - ## t/t5606-clone-options.sh ##\n -@@ t/t5606-clone-options.sh: test_expect_success 'disallows --bare with --separate-git-dir' '\n - \n - '\n - \n --test_expect_success 'uses \"origin\" for default remote name' '\n --\n --\tgit clone parent clone-default-origin &&\n --\t(cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master)\n --\n --'\n --\n - test_expect_success 'prefers --template config over normal config' '\n - \n - \ttemplate=\"$TRASH_DIRECTORY/template-with-config\" &&\n -@@ t/t5606-clone-options.sh: test_expect_success 'prefers -c config over --template config' '\n - \n - '\n - \n -+test_expect_success 'uses \"origin\" for default remote name' '\n -+\n -+\tgit clone parent clone-default-origin &&\n -+\t(cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master)\n -+\n -+'\n -+\n -+test_expect_success 'prefers config \"clone.defaultRemoteName\" over default' '\n -+\n -+\ttest_config_global clone.defaultRemoteName from_config &&\n -+\tgit clone parent clone-config-origin &&\n -+\t(cd clone-config-origin && git rev-parse --verify refs/remotes/from_config/master)\n -+\n -+'\n -+\n -+test_expect_success 'prefers --origin over -c config' '\n -+\n -+\tgit clone -c clone.defaultRemoteName=inline --origin from_option parent clone-o-and-inline-config &&\n -+\t(cd clone-o-and-inline-config && git rev-parse --verify refs/remotes/from_option/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 3: 0dff8cd669 ! 7: 737f91c624 clone: validate --origin option before use\n @@ Metadata\n Author: Sean Barag \n \n ## Commit message ##\n - clone: validate --origin option before use\n -\n - Providing a bad origin name to `git clone` currently reports an\n - 'invalid refspec' error instead of a more explicit message explaining\n - that the `--origin` option was malformed. Per Junio, it's been this way\n - since 8434c2f1 (Build in clone, 2008-04-27). This patch reintroduces\n - validation for the provided `--origin` option, but notably _doesn't_\n - include a multi-level check (e.g. \"foo/bar\") that was present in the\n - original `git-clone.sh`. It seems `git remote` allows multi-level\n - remote names, so applying that same validation in `git clone` seems\n - reasonable.\n + clone: allow configurable default for `-o`/`--origin`\n \n + While the default remote name of \"origin\" can be changed at clone-time\n + with `git clone`'s `--origin` option, it was previously not possible\n + to specify a default value for the name of that remote. Add support for\n + a new `clone.defaultRemoteName` config, with the newly-created remote\n + name resolved in priority order:\n +\n + 1. (Highest priority) A remote name passed directly to `git clone -o`\n + 2. A `clone.defaultRemoteName=new_name` in config `git clone -c`\n + 3. A `clone.defaultRemoteName` value set in `/path/to/template/config`,\n + where `--template=/path/to/template` is provided\n + 4. A `clone.defaultRemoteName` value set in a non-template config file\n + 5. The default value of `origin`\n +\n + Helped-by: Junio C Hamano \n + Helped-by: Johannes Schindelin \n + Helped-by: Derrick Stolee \n + Helped-by: Andrei Rybak \n Signed-off-by: Sean Barag \n - Thanks-to: Junio C Hamano \n +\n + ## Documentation/config.txt ##\n +@@ Documentation/config.txt: include::config/checkout.txt[]\n + \n + include::config/clean.txt[]\n + \n ++include::config/clone.txt[]\n ++\n + include::config/color.txt[]\n + \n + include::config/column.txt[]\n +\n + ## Documentation/config/clone.txt (new) ##\n +@@\n ++clone.defaultRemoteName::\n ++\tThe name of the remote to create when cloning a repository. Defaults to\n ++\t`origin`, and can be overridden by passing the `--origin` command-line\n ++\toption to linkgit:git-clone[1].\n ++\n +\n + ## Documentation/git-clone.txt ##\n +@@ Documentation/git-clone.txt: objects from the source repository into a pack in the cloned repository.\n + \n + -o ::\n + --origin ::\n +-\tInstead of using the remote name `origin` to keep track\n +-\tof the upstream repository, use ``.\n ++\tInstead of using the remote name `origin` to keep track of the upstream\n ++\trepository, use ``. Overrides `clone.defaultRemoteName` from the\n ++\tconfig.\n + \n + -b ::\n + --branch ::\n \n ## builtin/clone.c ##\n +@@ builtin/clone.c: static int option_shallow_submodules;\n + static int deepen;\n + static char *option_template, *option_depth, *option_since;\n + static char *option_origin = NULL;\n +-static char *remote_name = \"origin\";\n ++static char *default_remote_name = \"origin\";\n ++static char *remote_name = NULL;\n + static char *option_branch = NULL;\n + static struct string_list option_not = STRING_LIST_INIT_NODUP;\n + static const char *real_git_dir;\n +@@ builtin/clone.c: static int checkout(int submodule_progress)\n + \n + static int git_clone_config(const char *k, const char *v, void *cb)\n + {\n ++\tif (!strcmp(k, \"clone.defaultremotename\")) {\n ++\t\tif (remote_name != default_remote_name)\n ++\t\t\tfree(remote_name);\n ++\t\tremote_name = xstrdup(v);\n ++\t}\n + \treturn git_default_config(k, v, cb);\n + }\n + \n @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n - \tconst struct ref *ref;\n - \tstruct strbuf key = STRBUF_INIT;\n - \tstruct strbuf default_refspec = STRBUF_INIT;\n -+\tstruct strbuf resolved_refspec = STRBUF_INIT;\n - \tstruct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;\n - \tstruct transport *transport = NULL;\n - \tconst char *src_ref_prefix = \"refs/heads/\";\n + \tint submodule_progress;\n + \n + \tstruct strvec ref_prefixes = STRVEC_INIT;\n ++\tremote_name = default_remote_name;\n + \n + \tpacket_trace_identity(\"clone\");\n + \n @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n - \tif (!option_origin)\n - \t\toption_origin = \"origin\";\n + \t\toption_no_checkout = 1;\n + \t}\n \n -+\tstrbuf_addf(&resolved_refspec, \"refs/heads/test:refs/remotes/%s/test\", option_origin);\n -+\tif (!valid_fetch_refspec(resolved_refspec.buf))\n -+\t\t/* TRANSLATORS: %s will be the user-provided --origin / -o option */\n -+\t\tdie(_(\"'%s' is not a valid origin name\"), option_origin);\n -+\tstrbuf_release(&resolved_refspec);\n -+\n +-\tif (option_origin)\n +-\t\tremote_name = option_origin;\n +-\n +-\tif (!valid_remote_name(remote_name))\n +-\t\tdie(_(\"'%s' is not a valid remote name\"), remote_name);\n +-\n \trepo_name = argv[0];\n \n \tpath = get_repo_path(repo_name, &is_bundle);\n +@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n + \n + \t/*\n + \t * re-read config after init_db and write_config to pick up any config\n +-\t * injected by --template and --config, respectively\n ++\t * injected by --template and --config, respectively.\n + \t */\n + \tgit_config(git_clone_config, NULL);\n + \n ++\t/*\n ++\t * apply the remote name provided by --origin only after this second\n ++\t * call to git_config, to ensure it overrides all config-based values.\n ++\t */\n ++\tif (option_origin)\n ++\t\tremote_name = option_origin;\n ++\n ++\tif (!valid_remote_name(remote_name))\n ++\t\tdie(_(\"'%s' is not a valid remote name\"), remote_name);\n ++\n + \tif (option_bare) {\n + \t\tif (option_mirror)\n + \t\t\tsrc_ref_prefix = \"refs/\";\n \n ## t/t5606-clone-options.sh ##\n @@ t/t5606-clone-options.sh: test_expect_success 'clone -o' '\n @@ t/t5606-clone-options.sh: test_expect_success 'clone -o' '\n \n +test_expect_success 'rejects invalid -o/--origin' '\n +\n -+\ttest_expect_code 128 git clone -o \"bad...name\" parent clone-bad-name 2>err &&\n -+\ttest_debug \"cat err\" &&\n -+\ttest_i18ngrep \"'\\''bad...name'\\'' is not a valid origin name\" err\n ++\ttest_must_fail git clone -o \"bad...name\" parent clone-bad-name 2>err &&\n ++\ttest_i18ngrep \"'\\''bad...name'\\'' is not a valid remote name\" err\n +\n +'\n +\n test_expect_success 'disallows --bare with --origin' '\n \n - \ttest_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&\n + \ttest_must_fail git clone -o foo --bare parent clone-bare-o 2>err &&\n +@@ t/t5606-clone-options.sh: test_expect_success 'prefers -c config over --template config' '\n + \n + '\n + \n ++test_expect_success 'prefers config \"clone.defaultRemoteName\" over default' '\n ++\n ++\ttest_config_global clone.defaultRemoteName from_config &&\n ++\tgit clone parent clone-config-origin &&\n ++\tgit -C clone-config-origin rev-parse --verify refs/remotes/from_config/master\n ++\n ++'\n ++\n ++test_expect_success 'prefers --origin over -c config' '\n ++\n ++\tgit clone -c clone.defaultRemoteName=inline --origin from_option parent clone-o-and-inline-config &&\n ++\tgit -C clone-o-and-inline-config rev-parse --verify refs/remotes/from_option/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\n-- \ngitgitgadget\n"},{"id":"406612","messageId":"e3479e7b351361a4aa0c6aa4d98f91cd30ea14dc.1601350615.git.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":"pull.727.v2.git.1601350615.gitgitgadget@gmail.com","subject":"[PATCH v2 2/7] clone: use more conventional config/option layering","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-29T03:36:50Z","receivedAt":"2020-09-29T03:37:02Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"From: Sean Barag \n\nParsing command-line options before reading from config required careful\nhandling to ensure CLI options were treated with higher priority. Read\nconfig first to let parsed CLI naively overwrite matching config values.\n\nHelped-by: Junio C Hamano \nHelped-by: Johannes Schindelin \nSigned-off-by: Sean Barag \n---\n builtin/clone.c | 27 ++++++++++++++++++++++++++-\n 1 file changed, 26 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex fbfd6568cd..93ccd05b5d 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -851,8 +851,22 @@ static int checkout(int submodule_progress)\n \treturn err;\n }\n \n+static int git_clone_config(const char *k, const char *v, void *cb)\n+{\n+\treturn git_default_config(k, v, cb);\n+}\n+\n static int write_one_config(const char *key, const char *value, void *data)\n {\n+\t/*\n+\t * give git_clone_config a chance to write config values back to the\n+\t * environment, since git_config_set_multivar_gently only deals with\n+\t * config-file writes\n+\t */\n+\tint apply_failed = git_clone_config(key, value, data);\n+\tif (apply_failed)\n+\t\treturn apply_failed;\n+\n \treturn git_config_set_multivar_gently(key,\n \t\t\t\t\t value ? value : \"true\",\n \t\t\t\t\t CONFIG_REGEX_NONE, 0);\n@@ -963,6 +977,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstruct strvec ref_prefixes = STRVEC_INIT;\n \n \tpacket_trace_identity(\"clone\");\n+\n+\tgit_config(git_clone_config, NULL);\n+\n \targc = parse_options(argc, argv, prefix, builtin_clone_options,\n \t\t\t builtin_clone_usage, 0);\n \n@@ -1124,9 +1141,17 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (real_git_dir)\n \t\tgit_dir = real_git_dir;\n \n+\t/*\n+\t * additional config can be injected with -c, make sure it's included\n+\t * after init_db, which clears the entire config environment.\n+\t */\n \twrite_config(&option_config);\n \n-\tgit_config(git_default_config, NULL);\n+\t/*\n+\t * re-read config after init_db and write_config to pick up any config\n+\t * injected by --template and --config, respectively\n+\t */\n+\tgit_config(git_clone_config, NULL);\n \n \tif (option_bare) {\n \t\tif (option_mirror)\n-- \ngitgitgadget\n\n"},{"id":"406613","messageId":"85be780b8eff988f01461caccabd61a0ebfd8578.1601350615.git.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":"pull.727.v2.git.1601350615.gitgitgadget@gmail.com","subject":"[PATCH v2 3/7] remote: add tests for add and rename with invalid names","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-29T03:36:51Z","receivedAt":"2020-09-29T03:37:07Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"From: Sean Barag \n\nIn preparation for a future patch that moves `builtin/remote.c`'s\nremote-name validation, ensure `git remote add` and `git remote rename`\nreport errors when the new name isn't valid.\n\nSigned-off-by: Sean Barag \n---\n t/t5505-remote.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 8d62edd98b..1156f52069 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -179,6 +179,13 @@ test_expect_success 'rename errors out early when deleting non-existent branch'\n \t)\n '\n \n+test_expect_success 'rename errors out early when when new name is invalid' '\n+\ttest_config remote.foo.vcs bar &&\n+\techo \"fatal: '\\''invalid...name'\\'' is not a valid remote name\" >expect &&\n+\ttest_must_fail git remote rename foo invalid...name 2>actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n test_expect_success 'add existing foreign_vcs remote' '\n \ttest_config remote.foo.vcs bar &&\n \techo \"fatal: remote foo already exists.\" >expect &&\n@@ -194,6 +201,12 @@ test_expect_success 'add existing foreign_vcs remote' '\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'add invalid foreign_vcs remote' '\n+\techo \"fatal: '\\''invalid...name'\\'' is not a valid remote name\" >expect &&\n+\ttest_must_fail git remote add invalid...name bar 2>actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n cat >test/expect <\n\nIn a future patch, the name of the remote created by `git clone` may\ncome from multiple sources. To avoid confusion, convert most uses of\noption_origin to remote_name, leaving option_origin to exclusively\nrepresent the -o/--origin option.\n\nHelped-by: Derrick Stolee \nSigned-off-by: Sean Barag \n---\n builtin/clone.c | 27 ++++++++++++++-------------\n 1 file changed, 14 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 673f7b68c3..7fdd00cd95 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -53,6 +53,7 @@ static int option_shallow_submodules;\n static int deepen;\n static char *option_template, *option_depth, *option_since;\n static char *option_origin = NULL;\n+static char *remote_name = \"origin\";\n static char *option_branch = NULL;\n static struct string_list option_not = STRING_LIST_INIT_NODUP;\n static const char *real_git_dir;\n@@ -721,7 +722,7 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t\tif (!option_bare) {\n \t\t\tupdate_ref(msg, \"HEAD\", &our->old_oid, NULL, 0,\n \t\t\t\t UPDATE_REFS_DIE_ON_ERR);\n-\t\t\tinstall_branch_config(0, head, option_origin, our->name);\n+\t\t\tinstall_branch_config(0, head, remote_name, our->name);\n \t\t}\n \t} else if (our) {\n \t\tstruct commit *c = lookup_commit_reference(the_repository,\n@@ -919,12 +920,12 @@ static void write_refspec_config(const char *src_ref_prefix,\n \t\t}\n \t\t/* Configure the remote */\n \t\tif (value.len) {\n-\t\t\tstrbuf_addf(&key, \"remote.%s.fetch\", option_origin);\n+\t\t\tstrbuf_addf(&key, \"remote.%s.fetch\", remote_name);\n \t\t\tgit_config_set_multivar(key.buf, value.buf, \"^$\", 0);\n \t\t\tstrbuf_reset(&key);\n \n \t\t\tif (option_mirror) {\n-\t\t\t\tstrbuf_addf(&key, \"remote.%s.mirror\", option_origin);\n+\t\t\t\tstrbuf_addf(&key, \"remote.%s.mirror\", remote_name);\n \t\t\t\tgit_config_set(key.buf, \"true\");\n \t\t\t\tstrbuf_reset(&key);\n \t\t\t}\n@@ -1008,8 +1009,8 @@ 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\tremote_name = option_origin;\n \n \tif (!valid_remote_name(remote_name))\n \t\tdie(_(\"'%s' is not a valid remote name\"), remote_name);\n@@ -1163,15 +1164,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \t\tgit_config_set(\"core.bare\", \"true\");\n \t} else {\n-\t\tstrbuf_addf(&branch_top, \"refs/remotes/%s/\", option_origin);\n+\t\tstrbuf_addf(&branch_top, \"refs/remotes/%s/\", remote_name);\n \t}\n \n-\tstrbuf_addf(&key, \"remote.%s.url\", option_origin);\n+\tstrbuf_addf(&key, \"remote.%s.url\", remote_name);\n \tgit_config_set(key.buf, repo);\n \tstrbuf_reset(&key);\n \n \tif (option_no_tags) {\n-\t\tstrbuf_addf(&key, \"remote.%s.tagOpt\", option_origin);\n+\t\tstrbuf_addf(&key, \"remote.%s.tagOpt\", remote_name);\n \t\tgit_config_set(key.buf, \"--no-tags\");\n \t\tstrbuf_reset(&key);\n \t}\n@@ -1182,7 +1183,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (option_sparse_checkout && git_sparse_checkout_init(dir))\n \t\treturn 1;\n \n-\tremote = remote_get(option_origin);\n+\tremote = remote_get(remote_name);\n \n \trefspec_appendf(&remote->fetch, \"+%s*:%s*\", src_ref_prefix,\n \t\t\tbranch_top.buf);\n@@ -1294,7 +1295,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \t\t\tif (!our_head_points_at)\n \t\t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n-\t\t\t\t option_branch, option_origin);\n+\t\t\t\t option_branch, remote_name);\n \t\t}\n \t\telse\n \t\t\tour_head_points_at = remote_head_points_at;\n@@ -1302,7 +1303,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \telse {\n \t\tif (option_branch)\n \t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n-\t\t\t\t\toption_branch, option_origin);\n+\t\t\t\t\toption_branch, remote_name);\n \n \t\twarning(_(\"You appear to have cloned an empty repository.\"));\n \t\tmapped_refs = NULL;\n@@ -1314,7 +1315,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\tconst char *branch = git_default_branch_name();\n \t\t\tchar *ref = xstrfmt(\"refs/heads/%s\", branch);\n \n-\t\t\tinstall_branch_config(0, branch, option_origin, ref);\n+\t\t\tinstall_branch_config(0, branch, remote_name, ref);\n \t\t\tfree(ref);\n \t\t}\n \t}\n@@ -1323,7 +1324,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\tremote_head_points_at, &branch_top);\n \n \tif (filter_options.choice)\n-\t\tpartial_clone_register(option_origin, &filter_options);\n+\t\tpartial_clone_register(remote_name, &filter_options);\n \n \tif (is_local)\n \t\tclone_local(path, git_dir);\n-- \ngitgitgadget\n\n"},{"id":"406615","messageId":"fdc9d3b36979c1c4733e9326cc455006e3329d64.1601350615.git.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":"pull.727.v2.git.1601350615.gitgitgadget@gmail.com","subject":"[PATCH v2 5/7] clone: validate --origin option before use","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-29T03:36:53Z","receivedAt":"2020-09-29T03:37:10Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"From: Sean Barag \n\nProviding a bad origin name to `git clone` currently reports an\n'invalid refspec' error instead of a more explicit message explaining\nthat the `--origin` option was malformed. This behavior dates back to\nsince 8434c2f1 (Build in clone, 2008-04-27). Reintroduce\nvalidation for the provided `--origin` option, but notably _don't_\ninclude a multi-level check (e.g. \"foo/bar\") that was present in the\noriginal `git-clone.sh`. `git remote` allows multi-level remote names\nsince at least 46220ca100 (remote.c: Fix overtight refspec validation,\n2008-03-20), so that appears to be the desired behavior.\n\nHelped-by: Junio C Hamano \nHelped-by: Derrick Stolee \nHelped-by: Jeff King \nSigned-off-by: Sean Barag \n---\n builtin/clone.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 93ccd05b5d..673f7b68c3 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1011,6 +1011,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (!option_origin)\n \t\toption_origin = \"origin\";\n \n+\tif (!valid_remote_name(remote_name))\n+\t\tdie(_(\"'%s' is not a valid remote name\"), remote_name);\n+\n \trepo_name = argv[0];\n \n \tpath = get_repo_path(repo_name, &is_bundle);\n-- \ngitgitgadget\n\n"},{"id":"406616","messageId":"42dc18601ab09d1f9b45f0a37e7eda726d14599d.1601350615.git.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":"pull.727.v2.git.1601350615.gitgitgadget@gmail.com","subject":"[PATCH v2 4/7] refs: consolidate remote name validation","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-29T03:36:52Z","receivedAt":"2020-09-29T03:37:11Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"From: Sean Barag \n\nIn preparation for a future patch, extract from remote.c a function that\nvalidates possible remote names so that its rules can be used\nconsistently in other places.\n\nHelped-by: Derrick Stolee \nSigned-off-by: Sean Barag \n---\n builtin/remote.c | 7 ++-----\n refspec.c | 10 ++++++++++\n refspec.h | 1 +\n 3 files changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 64b4b551eb..63f2b46c3d 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -194,8 +194,7 @@ static int add(int argc, const char **argv)\n \tif (remote_is_configured(remote, 1))\n \t\tdie(_(\"remote %s already exists.\"), name);\n \n-\tstrbuf_addf(&buf2, \"refs/heads/test:refs/remotes/%s/test\", name);\n-\tif (!valid_fetch_refspec(buf2.buf))\n+\tif (!valid_remote_name(name))\n \t\tdie(_(\"'%s' is not a valid remote name\"), name);\n \n \tstrbuf_addf(&buf, \"remote.%s.url\", name);\n@@ -696,11 +695,9 @@ static int mv(int argc, const char **argv)\n \tif (remote_is_configured(newremote, 1))\n \t\tdie(_(\"remote %s already exists.\"), rename.new_name);\n \n-\tstrbuf_addf(&buf, \"refs/heads/test:refs/remotes/%s/test\", rename.new_name);\n-\tif (!valid_fetch_refspec(buf.buf))\n+\tif (!valid_remote_name(rename.new_name))\n \t\tdie(_(\"'%s' is not a valid remote name\"), rename.new_name);\n \n-\tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"remote.%s\", rename.old_name);\n \tstrbuf_addf(&buf2, \"remote.%s\", rename.new_name);\n \tif (git_config_rename_section(buf.buf, buf2.buf) < 1)\ndiff --git a/refspec.c b/refspec.c\nindex 8d0affc34a..3056ffdfb8 100644\n--- a/refspec.c\n+++ b/refspec.c\n@@ -215,6 +215,16 @@ int valid_fetch_refspec(const char *fetch_refspec_str)\n \treturn ret;\n }\n \n+int valid_remote_name(const char *name)\n+{\n+\tint result;\n+\tstruct strbuf refspec = STRBUF_INIT;\n+\tstrbuf_addf(&refspec, \"refs/heads/test:refs/remotes/%s/test\", name);\n+\tresult = valid_fetch_refspec(refspec.buf);\n+\tstrbuf_release(&refspec);\n+\treturn result;\n+}\n+\n void refspec_ref_prefixes(const struct refspec *rs,\n \t\t\t struct strvec *ref_prefixes)\n {\ndiff --git a/refspec.h b/refspec.h\nindex 7569248d11..bbb25968a8 100644\n--- a/refspec.h\n+++ b/refspec.h\n@@ -62,6 +62,7 @@ void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);\n void refspec_clear(struct refspec *rs);\n \n int valid_fetch_refspec(const char *refspec);\n+int valid_remote_name(const char *name);\n \n struct strvec;\n /*\n-- \ngitgitgadget\n\n"},{"id":"406617","messageId":"737f91c6244220eec196c327fcea9a6548c45310.1601350615.git.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":"pull.727.v2.git.1601350615.gitgitgadget@gmail.com","subject":"[PATCH v2 7/7] clone: allow configurable default for `-o`/`--origin`","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-09-29T03:36:55Z","receivedAt":"2020-09-29T03:37:12Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"From: Sean Barag \n\nWhile the default remote name of \"origin\" can be changed at clone-time\nwith `git clone`'s `--origin` option, it was previously not possible\nto specify a default value for the name of that remote. Add support for\na new `clone.defaultRemoteName` config, with the newly-created remote\nname resolved in priority order:\n\n1. (Highest priority) A remote name passed directly to `git clone -o`\n2. A `clone.defaultRemoteName=new_name` in config `git clone -c`\n3. A `clone.defaultRemoteName` value set in `/path/to/template/config`,\n where `--template=/path/to/template` is provided\n4. A `clone.defaultRemoteName` value set in a non-template config file\n5. The default value of `origin`\n\nHelped-by: Junio C Hamano \nHelped-by: Johannes Schindelin \nHelped-by: Derrick Stolee \nHelped-by: Andrei Rybak \nSigned-off-by: Sean Barag \n---\n Documentation/config.txt | 2 ++\n Documentation/config/clone.txt | 5 +++++\n Documentation/git-clone.txt | 5 +++--\n builtin/clone.c | 27 +++++++++++++++++++--------\n t/t5606-clone-options.sh | 22 ++++++++++++++++++++++\n 5 files changed, 51 insertions(+), 10 deletions(-)\n create mode 100644 Documentation/config/clone.txt\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex f93b6837e4..8c58c53ae7 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -334,6 +334,8 @@ include::config/checkout.txt[]\n \n include::config/clean.txt[]\n \n+include::config/clone.txt[]\n+\n include::config/color.txt[]\n \n include::config/column.txt[]\ndiff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt\nnew file mode 100644\nindex 0000000000..20755d413a\n--- /dev/null\n+++ b/Documentation/config/clone.txt\n@@ -0,0 +1,5 @@\n+clone.defaultRemoteName::\n+\tThe name of the remote to create when cloning a repository. Defaults to\n+\t`origin`, and can be overridden by passing the `--origin` command-line\n+\toption to linkgit:git-clone[1].\n+\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex 097e6a86c5..876aedcd47 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 ::\n --origin ::\n-\tInstead of using the remote name `origin` to keep track\n-\tof the upstream repository, use ``.\n+\tInstead of using the remote name `origin` to keep track of the upstream\n+\trepository, use ``. Overrides `clone.defaultRemoteName` from the\n+\tconfig.\n \n -b ::\n --branch ::\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 7fdd00cd95..4c821de242 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -53,7 +53,8 @@ static int option_shallow_submodules;\n static int deepen;\n static char *option_template, *option_depth, *option_since;\n static char *option_origin = NULL;\n-static char *remote_name = \"origin\";\n+static char *default_remote_name = \"origin\";\n+static char *remote_name = NULL;\n static char *option_branch = NULL;\n static struct string_list option_not = STRING_LIST_INIT_NODUP;\n static const char *real_git_dir;\n@@ -854,6 +855,11 @@ static int checkout(int submodule_progress)\n \n static int git_clone_config(const char *k, const char *v, void *cb)\n {\n+\tif (!strcmp(k, \"clone.defaultremotename\")) {\n+\t\tif (remote_name != default_remote_name)\n+\t\t\tfree(remote_name);\n+\t\tremote_name = xstrdup(v);\n+\t}\n \treturn git_default_config(k, v, cb);\n }\n \n@@ -976,6 +982,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tint submodule_progress;\n \n \tstruct strvec ref_prefixes = STRVEC_INIT;\n+\tremote_name = default_remote_name;\n \n \tpacket_trace_identity(\"clone\");\n \n@@ -1009,12 +1016,6 @@ 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\tremote_name = option_origin;\n-\n-\tif (!valid_remote_name(remote_name))\n-\t\tdie(_(\"'%s' is not a valid remote name\"), remote_name);\n-\n \trepo_name = argv[0];\n \n \tpath = get_repo_path(repo_name, &is_bundle);\n@@ -1153,10 +1154,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \t/*\n \t * re-read config after init_db and write_config to pick up any config\n-\t * injected by --template and --config, respectively\n+\t * injected by --template and --config, respectively.\n \t */\n \tgit_config(git_clone_config, NULL);\n \n+\t/*\n+\t * apply the remote name provided by --origin only after this second\n+\t * call to git_config, to ensure it overrides all config-based values.\n+\t */\n+\tif (option_origin)\n+\t\tremote_name = option_origin;\n+\n+\tif (!valid_remote_name(remote_name))\n+\t\tdie(_(\"'%s' is not a valid remote name\"), remote_name);\n+\n \tif (option_bare) {\n \t\tif (option_mirror)\n \t\t\tsrc_ref_prefix = \"refs/\";\ndiff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh\nindex 0422b24258..5e201e7d85 100755\n--- a/t/t5606-clone-options.sh\n+++ b/t/t5606-clone-options.sh\n@@ -19,6 +19,13 @@ test_expect_success 'clone -o' '\n \n '\n \n+test_expect_success 'rejects invalid -o/--origin' '\n+\n+\ttest_must_fail git clone -o \"bad...name\" parent clone-bad-name 2>err &&\n+\ttest_i18ngrep \"'\\''bad...name'\\'' is not a valid remote name\" err\n+\n+'\n+\n test_expect_success 'disallows --bare with --origin' '\n \n \ttest_must_fail git clone -o foo --bare parent clone-bare-o 2>err &&\n@@ -63,6 +70,21 @@ test_expect_success 'prefers -c config over --template config' '\n \n '\n \n+test_expect_success 'prefers config \"clone.defaultRemoteName\" over default' '\n+\n+\ttest_config_global clone.defaultRemoteName from_config &&\n+\tgit clone parent clone-config-origin &&\n+\tgit -C clone-config-origin rev-parse --verify refs/remotes/from_config/master\n+\n+'\n+\n+test_expect_success 'prefers --origin over -c config' '\n+\n+\tgit clone -c clone.defaultRemoteName=inline --origin from_option parent clone-o-and-inline-config &&\n+\tgit -C clone-o-and-inline-config rev-parse --verify refs/remotes/from_option/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-- \ngitgitgadget\n"},{"id":"406618","messageId":"20200929034417.874963-1-sean@barag.org","threadId":"54232","inReplyTo":"pull.727.v2.git.1601350615.gitgitgadget@gmail.com","subject":"[PATCH v2 0/7] clone: allow configurable default for -o/--origin","fromName":"Sean Barag","fromEmail":"sean@barag.org","sentAt":"2020-09-29T03:44:17Z","receivedAt":"2020-09-29T03:45:00Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"Self reply, but I wanted to thank you *all* for being so supportive and\nhelpful through my first contribution. This is a great experience :)\n\nSean\n"},{"id":"406642","messageId":"xmqqa6x8icbp.fsf@gitster.c.googlers.com","threadId":"54232","inReplyTo":"737f91c6244220eec196c327fcea9a6548c45310.1601350615.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 7/7] clone: allow configurable default for `-o`/`--origin`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-29T19:59:22Z","receivedAt":"2020-09-29T19:59:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Sean Barag via GitGitGadget\" writes:\n\n> static int git_clone_config(const char *k, const char *v, void *cb)\n> {\n> +\tif (!strcmp(k, \"clone.defaultremotename\")) {\n> +\t\tif (remote_name != default_remote_name)\n> +\t\t\tfree(remote_name);\n> +\t\tremote_name = xstrdup(v);\n\nThis feels strange. The usual arrangement is\n\n - initialize the variable to NULL (or any value that the code\n can tell that nobody touched it);\n\n - let git_config() callback to update the variable, taking care\n of freeing and strduping as needed. Note that free(NULL) is\n kosher.\n\n - let parse_options() to further update the variable, taking\n care of freeing and strduping as needed.\n\n - finally, if the variable is still NULL, give it its default\n value.\n\nso there is no room for the \"if the variable has the value of the\nfallback default, do things differently\" logic to be in the\ngit_config() callback function.\n\n> @@ -976,6 +982,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \tint submodule_progress;\n> \n> \tstruct strvec ref_prefixes = STRVEC_INIT;\n\nMissing blank line here.\n\n> +\tremote_name = default_remote_name;\n\nIsn't the reason why the git_config() callback we saw above has an\nunusual special case for default_remote_name because we have this\nassignment way too early? Would it make the control flow in a more\nnatural way if we removed this, and then after parse_options() did\nthe \"-o\" handling, add something like\n\n\tif (!remote_name)\n\t\tremote_name = xstrdup(\"origin\");\n\ninstead?\n\n> @@ -1153,10 +1154,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \n> \t/*\n> \t * re-read config after init_db and write_config to pick up any config\n> -\t * injected by --template and --config, respectively\n> +\t * injected by --template and --config, respectively.\n> \t */\n\nSquash this \"oops, I forgot to finish the sentence\" to the step the\nmistake was introduced, i.e. \"use more conventional...\"\n\n> \tgit_config(git_clone_config, NULL);\n> \n> +\t/*\n> +\t * apply the remote name provided by --origin only after this second\n> +\t * call to git_config, to ensure it overrides all config-based values.\n> +\t */\n> +\tif (option_origin)\n> +\t\tremote_name = option_origin;\n\nAnd here would be where you'd fall back\n\n\tif (!remote_name)\n\t\tremote_name = xstrdup(\"origin\");\n\nNote that you'd need to dup the option_origin for consistency if you\nwant to free() it at the end.\n\n> +\tif (!valid_remote_name(remote_name))\n> +\t\tdie(_(\"'%s' is not a valid remote name\"), remote_name);\n> +\n"},{"id":"406654","messageId":"20200929234750.879484-1-sean@barag.org","threadId":"54232","inReplyTo":"xmqqa6x8icbp.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] clone: add remote.cloneDefault config option","fromName":"Sean Barag","fromEmail":"sean@barag.org","sentAt":"2020-09-29T23:47:50Z","receivedAt":"2020-09-29T23:48:20Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"\"Junio C Hamano\" writes:\n\n> > static int git_clone_config(const char *k, const char *v, void *cb)\n> > {\n> > +\tif (!strcmp(k, \"clone.defaultremotename\")) {\n> > +\t\tif (remote_name != default_remote_name)\n> > +\t\t\tfree(remote_name);\n> > +\t\tremote_name = xstrdup(v);\n> \n> This feels strange. The usual arrangement is\n> \n> - initialize the variable to NULL (or any value that the code\n> can tell that nobody touched it);\n> \n> - let git_config() callback to update the variable, taking care\n> of freeing and strduping as needed. Note that free(NULL) is\n> kosher.\n> \n> - let parse_options() to further update the variable, taking\n> care of freeing and strduping as needed.\n> \n> - finally, if the variable is still NULL, give it its default\n> value.\n> \n> so there is no room for the \"if the variable has the value of the\n> fallback default, do things differently\" logic to be in the\n> git_config() callback function.\n\nI agree, it felt pretty awkward while writing it! I was hoping to match\nthe start-up sequence you'd mentioned in my original attempt [1] but I'm\nhappy to move the default value assignment down.\n\n> > @@ -1153,10 +1154,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> > \n> > \t/*\n> > \t * re-read config after init_db and write_config to pick up any config\n> > -\t * injected by --template and --config, respectively\n> > +\t * injected by --template and --config, respectively.\n> > \t */\n> \n> Squash this \"oops, I forgot to finish the sentence\" to the step the\n> mistake was introduced, i.e. \"use more conventional...\"\n\nHow embarrassing, I thought I'd gotten all of those. Will do.\n\nSean\n\n--\n[1] Original attempt at this feature, in separate thread because of the\n divergence: https://lore.kernel.org/git/xmqqlfi1utwi.fsf@gitster.c.googlers.com/\n"},{"id":"406744","messageId":"pull.727.v3.git.1601523977.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":"pull.727.v2.git.1601350615.gitgitgadget@gmail.com","subject":"[PATCH v3 0/7] clone: allow configurable default for -o/--origin","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-01T03:46:09Z","receivedAt":"2020-10-01T03:46:22Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"v3 (changes since v2):\n\n * [5/7] fix compilation error: validate option_origin since remote_name\n doesn't exist yet\n * [7/7] remove default_remote_name; apply default value inline if no other\n value applied\n\nv2 (changes since v1):\n\n * Convert Thanks-to trailer to Helped-by\n * Rewrite several commit titles and messages\n * Unify error reporting between clone.c and remote.c\n * Add tests for git remote add and git remote rename with invalid remote\n names\n * Prevent leak of old remote_name\n\nv1: Took another pass at supporting a configurable default for-o/--origin,\nthis time following Junio's suggestions from a previous approach as much as\npossible [1]. Unfortunately, Johannes mentioned that --template can write\nnew config values that aren't automatically merged without re-calling \ngit_config. There doesn't appear to be a way around this without rewriting\nsignificant amounts of init and config logic across the codebase.\n\nWhile this could have been v2 of the original patchset, it's diverged so\ndrastically from the original that it likely warrants its own root thread.\nIf that's not appropriate though, I'd be happy to restructure!\n\nThanks for all the advice Junio and Johannes! This feels significantly\nbetter than my first attempt.\n\n[1] \nhttps://lore.kernel.org/git/pull.710.git.1598456751674.gitgitgadget@gmail.com/\n[2] https://github.com/gitgitgadget/git/pull/727#issuecomment-689740195\n\nSean Barag (7):\n clone: add tests for --template and some disallowed option pairs\n clone: use more conventional config/option layering\n remote: add tests for add and rename with invalid names\n refs: consolidate remote name validation\n clone: validate --origin option before use\n clone: read new remote name from remote_name instead of option_origin\n clone: allow configurable default for `-o`/`--origin`\n\n Documentation/config.txt | 2 +\n Documentation/config/clone.txt | 5 +++\n Documentation/git-clone.txt | 5 ++-\n builtin/clone.c | 71 +++++++++++++++++++++++++++-------\n builtin/remote.c | 7 +---\n refspec.c | 10 +++++\n refspec.h | 1 +\n t/t5505-remote.sh | 13 +++++++\n t/t5606-clone-options.sh | 68 +++++++++++++++++++++++++++++++-\n 9 files changed, 159 insertions(+), 23 deletions(-)\n create mode 100644 Documentation/config/clone.txt\n\n\nbase-commit: 306ee63a703ad67c54ba1209dc11dd9ea500dc1f\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-727%2Fsjbarag%2Fclone_add-configurable-default-for--o-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-727/sjbarag/clone_add-configurable-default-for--o-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/727\n\nRange-diff vs v2:\n\n 1: 29ab418b1b = 1: 4195dec00c clone: add tests for --template and some disallowed option pairs\n 2: e3479e7b35 ! 2: 1abcf417d9 clone: use more conventional config/option layering\n @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n -\tgit_config(git_default_config, NULL);\n +\t/*\n +\t * re-read config after init_db and write_config to pick up any config\n -+\t * injected by --template and --config, respectively\n ++\t * injected by --template and --config, respectively.\n +\t */\n +\tgit_config(git_clone_config, NULL);\n \n 3: 85be780b8e = 3: ec371306ec remote: add tests for add and rename with invalid names\n 4: 42dc18601a = 4: cb686b4129 refs: consolidate remote name validation\n 5: fdc9d3b369 ! 5: e1d3b54fdc clone: validate --origin option before use\n @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (!option_origin)\n \t\toption_origin = \"origin\";\n \n -+\tif (!valid_remote_name(remote_name))\n -+\t\tdie(_(\"'%s' is not a valid remote name\"), remote_name);\n ++\tif (!valid_remote_name(option_origin))\n ++\t\tdie(_(\"'%s' is not a valid remote name\"), option_origin);\n +\n \trepo_name = argv[0];\n \n 6: 99f4d765b4 ! 6: c3fba50092 clone: read new remote name from remote_name instead of option_origin\n @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n +\tif (option_origin)\n +\t\tremote_name = option_origin;\n \n - \tif (!valid_remote_name(remote_name))\n - \t\tdie(_(\"'%s' is not a valid remote name\"), remote_name);\n +-\tif (!valid_remote_name(option_origin))\n +-\t\tdie(_(\"'%s' is not a valid remote name\"), option_origin);\n ++\tif (!valid_remote_name(remote_name))\n ++\t\tdie(_(\"'%s' is not a valid remote name\"), remote_name);\n + \n + \trepo_name = argv[0];\n + \n @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \t\tgit_config_set(\"core.bare\", \"true\");\n 7: 737f91c624 ! 7: da8212e500 clone: allow configurable default for `-o`/`--origin`\n @@ builtin/clone.c: static int option_shallow_submodules;\n static char *option_template, *option_depth, *option_since;\n static char *option_origin = NULL;\n -static char *remote_name = \"origin\";\n -+static char *default_remote_name = \"origin\";\n +static char *remote_name = NULL;\n static char *option_branch = NULL;\n static struct string_list option_not = STRING_LIST_INIT_NODUP;\n @@ builtin/clone.c: static int checkout(int submodule_progress)\n static int git_clone_config(const char *k, const char *v, void *cb)\n {\n +\tif (!strcmp(k, \"clone.defaultremotename\")) {\n -+\t\tif (remote_name != default_remote_name)\n -+\t\t\tfree(remote_name);\n ++\t\tfree(remote_name);\n +\t\tremote_name = xstrdup(v);\n +\t}\n \treturn git_default_config(k, v, cb);\n }\n \n -@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n - \tint submodule_progress;\n - \n - \tstruct strvec ref_prefixes = STRVEC_INIT;\n -+\tremote_name = default_remote_name;\n - \n - \tpacket_trace_identity(\"clone\");\n - \n @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\toption_no_checkout = 1;\n \t}\n @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \tpath = get_repo_path(repo_name, &is_bundle);\n @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n - \n - \t/*\n - \t * re-read config after init_db and write_config to pick up any config\n --\t * injected by --template and --config, respectively\n -+\t * injected by --template and --config, respectively.\n \t */\n \tgit_config(git_clone_config, NULL);\n \n @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n +\t * apply the remote name provided by --origin only after this second\n +\t * call to git_config, to ensure it overrides all config-based values.\n +\t */\n -+\tif (option_origin)\n -+\t\tremote_name = option_origin;\n ++\tif (option_origin != NULL)\n ++\t\tremote_name = xstrdup(option_origin);\n ++\n ++\tif (remote_name == NULL)\n ++\t\tremote_name = xstrdup(\"origin\");\n +\n +\tif (!valid_remote_name(remote_name))\n +\t\tdie(_(\"'%s' is not a valid remote name\"), remote_name);\n @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (option_bare) {\n \t\tif (option_mirror)\n \t\t\tsrc_ref_prefix = \"refs/\";\n +@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)\n + \tjunk_mode = JUNK_LEAVE_REPO;\n + \terr = checkout(submodule_progress);\n + \n ++\tfree(remote_name);\n + \tstrbuf_release(&reflog_msg);\n + \tstrbuf_release(&branch_top);\n + \tstrbuf_release(&key);\n \n ## t/t5606-clone-options.sh ##\n @@ t/t5606-clone-options.sh: test_expect_success 'clone -o' '\n\n-- \ngitgitgadget\n"},{"id":"406745","messageId":"4195dec00c846e2a1ebcd32d0cb5d0ed2626328d.1601523977.git.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":"pull.727.v3.git.1601523977.gitgitgadget@gmail.com","subject":"[PATCH v3 1/7] clone: add tests for --template and some disallowed option pairs","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-01T03:46:10Z","receivedAt":"2020-10-01T03:46:26Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"From: Sean Barag \n\nSome combinations of command-line options to `git clone` are invalid,\nbut there were previously no tests ensuring those combinations reported\nerrors. Similarly, `git clone --template` didn't appear to have any\ntests.\n\nHelped-by: Jeff King \nHelped-by: Derrick Stolee \nSigned-off-by: Sean Barag \n---\n t/t5606-clone-options.sh | 46 +++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 45 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh\nindex e69427f881..0422b24258 100755\n--- a/t/t5606-clone-options.sh\n+++ b/t/t5606-clone-options.sh\n@@ -15,7 +15,51 @@ test_expect_success 'setup' '\n test_expect_success 'clone -o' '\n \n \tgit clone -o foo parent clone-o &&\n-\t(cd clone-o && git rev-parse --verify refs/remotes/foo/master)\n+\tgit -C clone-o rev-parse --verify refs/remotes/foo/master\n+\n+'\n+\n+test_expect_success 'disallows --bare with --origin' '\n+\n+\ttest_must_fail git clone -o foo --bare parent clone-bare-o 2>err &&\n+\ttest_debug \"cat err\" &&\n+\ttest_i18ngrep -e \"--bare and --origin foo options are incompatible\" err\n+\n+'\n+\n+test_expect_success 'disallows --bare with --separate-git-dir' '\n+\n+\ttest_must_fail git clone --bare --separate-git-dir dot-git-destiation parent clone-bare-sgd 2>err &&\n+\ttest_debug \"cat err\" &&\n+\ttest_i18ngrep -e \"--bare and --separate-git-dir are incompatible\" err\n+\n+'\n+\n+test_expect_success 'uses \"origin\" for default remote name' '\n+\n+\tgit clone parent clone-default-origin &&\n+\tgit -C clone-default-origin rev-parse --verify refs/remotes/origin/master\n+\n+'\n+\n+test_expect_success 'prefers --template config over normal config' '\n+\n+\ttemplate=\"$TRASH_DIRECTORY/template-with-config\" &&\n+\tmkdir \"$template\" &&\n+\tgit config --file \"$template/config\" foo.bar from_template &&\n+\ttest_config_global foo.bar from_global &&\n+\tgit clone \"--template=$template\" parent clone-template-config &&\n+\ttest \"$(git -C clone-template-config config --local foo.bar)\" = \"from_template\"\n+\n+'\n+\n+test_expect_success 'prefers -c config over --template config' '\n+\n+\ttemplate=\"$TRASH_DIRECTORY/template-with-ignored-config\" &&\n+\tmkdir \"$template\" &&\n+\tgit config --file \"$template/config\" foo.bar from_template &&\n+\tgit clone \"--template=$template\" -c foo.bar=inline parent clone-template-inline-config &&\n+\ttest \"$(git -C clone-template-inline-config config --local foo.bar)\" = \"inline\"\n \n '\n \n-- \ngitgitgadget\n\n"},{"id":"406746","messageId":"e1d3b54fdc6a2dbf10eb1fc75e41b9dc233a5383.1601523977.git.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":"pull.727.v3.git.1601523977.gitgitgadget@gmail.com","subject":"[PATCH v3 5/7] clone: validate --origin option before use","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-01T03:46:14Z","receivedAt":"2020-10-01T03:46:27Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"From: Sean Barag \n\nProviding a bad origin name to `git clone` currently reports an\n'invalid refspec' error instead of a more explicit message explaining\nthat the `--origin` option was malformed. This behavior dates back to\nsince 8434c2f1 (Build in clone, 2008-04-27). Reintroduce\nvalidation for the provided `--origin` option, but notably _don't_\ninclude a multi-level check (e.g. \"foo/bar\") that was present in the\noriginal `git-clone.sh`. `git remote` allows multi-level remote names\nsince at least 46220ca100 (remote.c: Fix overtight refspec validation,\n2008-03-20), so that appears to be the desired behavior.\n\nHelped-by: Junio C Hamano \nHelped-by: Derrick Stolee \nHelped-by: Jeff King \nSigned-off-by: Sean Barag \n---\n builtin/clone.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a76dacd988..bd67ff7b01 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1011,6 +1011,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (!option_origin)\n \t\toption_origin = \"origin\";\n \n+\tif (!valid_remote_name(option_origin))\n+\t\tdie(_(\"'%s' is not a valid remote name\"), option_origin);\n+\n \trepo_name = argv[0];\n \n \tpath = get_repo_path(repo_name, &is_bundle);\n-- \ngitgitgadget\n\n"},{"id":"406747","messageId":"da8212e500a5876d5427a3be3ea60e5a03a9b9ae.1601523977.git.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":"pull.727.v3.git.1601523977.gitgitgadget@gmail.com","subject":"[PATCH v3 7/7] clone: allow configurable default for `-o`/`--origin`","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-01T03:46:16Z","receivedAt":"2020-10-01T03:46:28Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"From: Sean Barag \n\nWhile the default remote name of \"origin\" can be changed at clone-time\nwith `git clone`'s `--origin` option, it was previously not possible\nto specify a default value for the name of that remote. Add support for\na new `clone.defaultRemoteName` config, with the newly-created remote\nname resolved in priority order:\n\n1. (Highest priority) A remote name passed directly to `git clone -o`\n2. A `clone.defaultRemoteName=new_name` in config `git clone -c`\n3. A `clone.defaultRemoteName` value set in `/path/to/template/config`,\n where `--template=/path/to/template` is provided\n4. A `clone.defaultRemoteName` value set in a non-template config file\n5. The default value of `origin`\n\nHelped-by: Junio C Hamano \nHelped-by: Johannes Schindelin \nHelped-by: Derrick Stolee \nHelped-by: Andrei Rybak \nSigned-off-by: Sean Barag \n---\n Documentation/config.txt | 2 ++\n Documentation/config/clone.txt | 5 +++++\n Documentation/git-clone.txt | 5 +++--\n builtin/clone.c | 26 +++++++++++++++++++-------\n t/t5606-clone-options.sh | 22 ++++++++++++++++++++++\n 5 files changed, 51 insertions(+), 9 deletions(-)\n create mode 100644 Documentation/config/clone.txt\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex bf706b950e..025ca4df11 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -334,6 +334,8 @@ include::config/checkout.txt[]\n \n include::config/clean.txt[]\n \n+include::config/clone.txt[]\n+\n include::config/color.txt[]\n \n include::config/column.txt[]\ndiff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt\nnew file mode 100644\nindex 0000000000..20755d413a\n--- /dev/null\n+++ b/Documentation/config/clone.txt\n@@ -0,0 +1,5 @@\n+clone.defaultRemoteName::\n+\tThe name of the remote to create when cloning a repository. Defaults to\n+\t`origin`, and can be overridden by passing the `--origin` command-line\n+\toption to linkgit:git-clone[1].\n+\ndiff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt\nindex 097e6a86c5..876aedcd47 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 ::\n --origin ::\n-\tInstead of using the remote name `origin` to keep track\n-\tof the upstream repository, use ``.\n+\tInstead of using the remote name `origin` to keep track of the upstream\n+\trepository, use ``. Overrides `clone.defaultRemoteName` from the\n+\tconfig.\n \n -b ::\n --branch ::\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 6bcdfa4cd6..a0841923cf 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -53,7 +53,7 @@ static int option_shallow_submodules;\n static int deepen;\n static char *option_template, *option_depth, *option_since;\n static char *option_origin = NULL;\n-static char *remote_name = \"origin\";\n+static char *remote_name = NULL;\n static char *option_branch = NULL;\n static struct string_list option_not = STRING_LIST_INIT_NODUP;\n static const char *real_git_dir;\n@@ -854,6 +854,10 @@ static int checkout(int submodule_progress)\n \n static int git_clone_config(const char *k, const char *v, void *cb)\n {\n+\tif (!strcmp(k, \"clone.defaultremotename\")) {\n+\t\tfree(remote_name);\n+\t\tremote_name = xstrdup(v);\n+\t}\n \treturn git_default_config(k, v, cb);\n }\n \n@@ -1009,12 +1013,6 @@ 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\tremote_name = option_origin;\n-\n-\tif (!valid_remote_name(remote_name))\n-\t\tdie(_(\"'%s' is not a valid remote name\"), remote_name);\n-\n \trepo_name = argv[0];\n \n \tpath = get_repo_path(repo_name, &is_bundle);\n@@ -1157,6 +1155,19 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t */\n \tgit_config(git_clone_config, NULL);\n \n+\t/*\n+\t * apply the remote name provided by --origin only after this second\n+\t * call to git_config, to ensure it overrides all config-based values.\n+\t */\n+\tif (option_origin != NULL)\n+\t\tremote_name = xstrdup(option_origin);\n+\n+\tif (remote_name == NULL)\n+\t\tremote_name = xstrdup(\"origin\");\n+\n+\tif (!valid_remote_name(remote_name))\n+\t\tdie(_(\"'%s' is not a valid remote name\"), remote_name);\n+\n \tif (option_bare) {\n \t\tif (option_mirror)\n \t\t\tsrc_ref_prefix = \"refs/\";\n@@ -1356,6 +1367,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tjunk_mode = JUNK_LEAVE_REPO;\n \terr = checkout(submodule_progress);\n \n+\tfree(remote_name);\n \tstrbuf_release(&reflog_msg);\n \tstrbuf_release(&branch_top);\n \tstrbuf_release(&key);\ndiff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh\nindex 0422b24258..5e201e7d85 100755\n--- a/t/t5606-clone-options.sh\n+++ b/t/t5606-clone-options.sh\n@@ -19,6 +19,13 @@ test_expect_success 'clone -o' '\n \n '\n \n+test_expect_success 'rejects invalid -o/--origin' '\n+\n+\ttest_must_fail git clone -o \"bad...name\" parent clone-bad-name 2>err &&\n+\ttest_i18ngrep \"'\\''bad...name'\\'' is not a valid remote name\" err\n+\n+'\n+\n test_expect_success 'disallows --bare with --origin' '\n \n \ttest_must_fail git clone -o foo --bare parent clone-bare-o 2>err &&\n@@ -63,6 +70,21 @@ test_expect_success 'prefers -c config over --template config' '\n \n '\n \n+test_expect_success 'prefers config \"clone.defaultRemoteName\" over default' '\n+\n+\ttest_config_global clone.defaultRemoteName from_config &&\n+\tgit clone parent clone-config-origin &&\n+\tgit -C clone-config-origin rev-parse --verify refs/remotes/from_config/master\n+\n+'\n+\n+test_expect_success 'prefers --origin over -c config' '\n+\n+\tgit clone -c clone.defaultRemoteName=inline --origin from_option parent clone-o-and-inline-config &&\n+\tgit -C clone-o-and-inline-config rev-parse --verify refs/remotes/from_option/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-- \ngitgitgadget\n"},{"id":"406748","messageId":"1abcf417d91128e09632f6c74cc14de3dce29623.1601523977.git.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":"pull.727.v3.git.1601523977.gitgitgadget@gmail.com","subject":"[PATCH v3 2/7] clone: use more conventional config/option layering","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-01T03:46:11Z","receivedAt":"2020-10-01T03:46:28Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"From: Sean Barag \n\nParsing command-line options before reading from config required careful\nhandling to ensure CLI options were treated with higher priority. Read\nconfig first to let parsed CLI naively overwrite matching config values.\n\nHelped-by: Junio C Hamano \nHelped-by: Johannes Schindelin \nSigned-off-by: Sean Barag \n---\n builtin/clone.c | 27 ++++++++++++++++++++++++++-\n 1 file changed, 26 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 391aa41075..a76dacd988 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -851,8 +851,22 @@ static int checkout(int submodule_progress)\n \treturn err;\n }\n \n+static int git_clone_config(const char *k, const char *v, void *cb)\n+{\n+\treturn git_default_config(k, v, cb);\n+}\n+\n static int write_one_config(const char *key, const char *value, void *data)\n {\n+\t/*\n+\t * give git_clone_config a chance to write config values back to the\n+\t * environment, since git_config_set_multivar_gently only deals with\n+\t * config-file writes\n+\t */\n+\tint apply_failed = git_clone_config(key, value, data);\n+\tif (apply_failed)\n+\t\treturn apply_failed;\n+\n \treturn git_config_set_multivar_gently(key,\n \t\t\t\t\t value ? value : \"true\",\n \t\t\t\t\t CONFIG_REGEX_NONE, 0);\n@@ -963,6 +977,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstruct strvec ref_prefixes = STRVEC_INIT;\n \n \tpacket_trace_identity(\"clone\");\n+\n+\tgit_config(git_clone_config, NULL);\n+\n \targc = parse_options(argc, argv, prefix, builtin_clone_options,\n \t\t\t builtin_clone_usage, 0);\n \n@@ -1124,9 +1141,17 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (real_git_dir)\n \t\tgit_dir = real_git_dir;\n \n+\t/*\n+\t * additional config can be injected with -c, make sure it's included\n+\t * after init_db, which clears the entire config environment.\n+\t */\n \twrite_config(&option_config);\n \n-\tgit_config(git_default_config, NULL);\n+\t/*\n+\t * re-read config after init_db and write_config to pick up any config\n+\t * injected by --template and --config, respectively.\n+\t */\n+\tgit_config(git_clone_config, NULL);\n \n \tif (option_bare) {\n \t\tif (option_mirror)\n-- \ngitgitgadget\n\n"},{"id":"406749","messageId":"ec371306ec52e157778c0d9591253e6a6b93b552.1601523977.git.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":"pull.727.v3.git.1601523977.gitgitgadget@gmail.com","subject":"[PATCH v3 3/7] remote: add tests for add and rename with invalid names","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-01T03:46:12Z","receivedAt":"2020-10-01T03:46:30Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"From: Sean Barag \n\nIn preparation for a future patch that moves `builtin/remote.c`'s\nremote-name validation, ensure `git remote add` and `git remote rename`\nreport errors when the new name isn't valid.\n\nSigned-off-by: Sean Barag \n---\n t/t5505-remote.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 8d62edd98b..1156f52069 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -179,6 +179,13 @@ test_expect_success 'rename errors out early when deleting non-existent branch'\n \t)\n '\n \n+test_expect_success 'rename errors out early when when new name is invalid' '\n+\ttest_config remote.foo.vcs bar &&\n+\techo \"fatal: '\\''invalid...name'\\'' is not a valid remote name\" >expect &&\n+\ttest_must_fail git remote rename foo invalid...name 2>actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n test_expect_success 'add existing foreign_vcs remote' '\n \ttest_config remote.foo.vcs bar &&\n \techo \"fatal: remote foo already exists.\" >expect &&\n@@ -194,6 +201,12 @@ test_expect_success 'add existing foreign_vcs remote' '\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'add invalid foreign_vcs remote' '\n+\techo \"fatal: '\\''invalid...name'\\'' is not a valid remote name\" >expect &&\n+\ttest_must_fail git remote add invalid...name bar 2>actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n cat >test/expect <\n\nIn a future patch, the name of the remote created by `git clone` may\ncome from multiple sources. To avoid confusion, convert most uses of\noption_origin to remote_name, leaving option_origin to exclusively\nrepresent the -o/--origin option.\n\nHelped-by: Derrick Stolee \nSigned-off-by: Sean Barag \n---\n builtin/clone.c | 31 ++++++++++++++++---------------\n 1 file changed, 16 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex bd67ff7b01..6bcdfa4cd6 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -53,6 +53,7 @@ static int option_shallow_submodules;\n static int deepen;\n static char *option_template, *option_depth, *option_since;\n static char *option_origin = NULL;\n+static char *remote_name = \"origin\";\n static char *option_branch = NULL;\n static struct string_list option_not = STRING_LIST_INIT_NODUP;\n static const char *real_git_dir;\n@@ -721,7 +722,7 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \t\tif (!option_bare) {\n \t\t\tupdate_ref(msg, \"HEAD\", &our->old_oid, NULL, 0,\n \t\t\t\t UPDATE_REFS_DIE_ON_ERR);\n-\t\t\tinstall_branch_config(0, head, option_origin, our->name);\n+\t\t\tinstall_branch_config(0, head, remote_name, our->name);\n \t\t}\n \t} else if (our) {\n \t\tstruct commit *c = lookup_commit_reference(the_repository,\n@@ -919,12 +920,12 @@ static void write_refspec_config(const char *src_ref_prefix,\n \t\t}\n \t\t/* Configure the remote */\n \t\tif (value.len) {\n-\t\t\tstrbuf_addf(&key, \"remote.%s.fetch\", option_origin);\n+\t\t\tstrbuf_addf(&key, \"remote.%s.fetch\", remote_name);\n \t\t\tgit_config_set_multivar(key.buf, value.buf, \"^$\", 0);\n \t\t\tstrbuf_reset(&key);\n \n \t\t\tif (option_mirror) {\n-\t\t\t\tstrbuf_addf(&key, \"remote.%s.mirror\", option_origin);\n+\t\t\t\tstrbuf_addf(&key, \"remote.%s.mirror\", remote_name);\n \t\t\t\tgit_config_set(key.buf, \"true\");\n \t\t\t\tstrbuf_reset(&key);\n \t\t\t}\n@@ -1008,11 +1009,11 @@ 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\tremote_name = option_origin;\n \n-\tif (!valid_remote_name(option_origin))\n-\t\tdie(_(\"'%s' is not a valid remote name\"), option_origin);\n+\tif (!valid_remote_name(remote_name))\n+\t\tdie(_(\"'%s' is not a valid remote name\"), remote_name);\n \n \trepo_name = argv[0];\n \n@@ -1163,15 +1164,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \t\tgit_config_set(\"core.bare\", \"true\");\n \t} else {\n-\t\tstrbuf_addf(&branch_top, \"refs/remotes/%s/\", option_origin);\n+\t\tstrbuf_addf(&branch_top, \"refs/remotes/%s/\", remote_name);\n \t}\n \n-\tstrbuf_addf(&key, \"remote.%s.url\", option_origin);\n+\tstrbuf_addf(&key, \"remote.%s.url\", remote_name);\n \tgit_config_set(key.buf, repo);\n \tstrbuf_reset(&key);\n \n \tif (option_no_tags) {\n-\t\tstrbuf_addf(&key, \"remote.%s.tagOpt\", option_origin);\n+\t\tstrbuf_addf(&key, \"remote.%s.tagOpt\", remote_name);\n \t\tgit_config_set(key.buf, \"--no-tags\");\n \t\tstrbuf_reset(&key);\n \t}\n@@ -1182,7 +1183,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (option_sparse_checkout && git_sparse_checkout_init(dir))\n \t\treturn 1;\n \n-\tremote = remote_get(option_origin);\n+\tremote = remote_get(remote_name);\n \n \trefspec_appendf(&remote->fetch, \"+%s*:%s*\", src_ref_prefix,\n \t\t\tbranch_top.buf);\n@@ -1294,7 +1295,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \t\t\tif (!our_head_points_at)\n \t\t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n-\t\t\t\t option_branch, option_origin);\n+\t\t\t\t option_branch, remote_name);\n \t\t}\n \t\telse\n \t\t\tour_head_points_at = remote_head_points_at;\n@@ -1302,7 +1303,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \telse {\n \t\tif (option_branch)\n \t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n-\t\t\t\t\toption_branch, option_origin);\n+\t\t\t\t\toption_branch, remote_name);\n \n \t\twarning(_(\"You appear to have cloned an empty repository.\"));\n \t\tmapped_refs = NULL;\n@@ -1314,7 +1315,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\tconst char *branch = git_default_branch_name();\n \t\t\tchar *ref = xstrfmt(\"refs/heads/%s\", branch);\n \n-\t\t\tinstall_branch_config(0, branch, option_origin, ref);\n+\t\t\tinstall_branch_config(0, branch, remote_name, ref);\n \t\t\tfree(ref);\n \t\t}\n \t}\n@@ -1323,7 +1324,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\tremote_head_points_at, &branch_top);\n \n \tif (filter_options.choice)\n-\t\tpartial_clone_register(option_origin, &filter_options);\n+\t\tpartial_clone_register(remote_name, &filter_options);\n \n \tif (is_local)\n \t\tclone_local(path, git_dir);\n-- \ngitgitgadget\n\n"},{"id":"406751","messageId":"cb686b4129902335ef98c83c8cfbcac1c88da856.1601523977.git.gitgitgadget@gmail.com","threadId":"54232","inReplyTo":"pull.727.v3.git.1601523977.gitgitgadget@gmail.com","subject":"[PATCH v3 4/7] refs: consolidate remote name validation","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-01T03:46:13Z","receivedAt":"2020-10-01T03:46:31Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"From: Sean Barag \n\nIn preparation for a future patch, extract from remote.c a function that\nvalidates possible remote names so that its rules can be used\nconsistently in other places.\n\nHelped-by: Derrick Stolee \nSigned-off-by: Sean Barag \n---\n builtin/remote.c | 7 ++-----\n refspec.c | 10 ++++++++++\n refspec.h | 1 +\n 3 files changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 64b4b551eb..63f2b46c3d 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -194,8 +194,7 @@ static int add(int argc, const char **argv)\n \tif (remote_is_configured(remote, 1))\n \t\tdie(_(\"remote %s already exists.\"), name);\n \n-\tstrbuf_addf(&buf2, \"refs/heads/test:refs/remotes/%s/test\", name);\n-\tif (!valid_fetch_refspec(buf2.buf))\n+\tif (!valid_remote_name(name))\n \t\tdie(_(\"'%s' is not a valid remote name\"), name);\n \n \tstrbuf_addf(&buf, \"remote.%s.url\", name);\n@@ -696,11 +695,9 @@ static int mv(int argc, const char **argv)\n \tif (remote_is_configured(newremote, 1))\n \t\tdie(_(\"remote %s already exists.\"), rename.new_name);\n \n-\tstrbuf_addf(&buf, \"refs/heads/test:refs/remotes/%s/test\", rename.new_name);\n-\tif (!valid_fetch_refspec(buf.buf))\n+\tif (!valid_remote_name(rename.new_name))\n \t\tdie(_(\"'%s' is not a valid remote name\"), rename.new_name);\n \n-\tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"remote.%s\", rename.old_name);\n \tstrbuf_addf(&buf2, \"remote.%s\", rename.new_name);\n \tif (git_config_rename_section(buf.buf, buf2.buf) < 1)\ndiff --git a/refspec.c b/refspec.c\nindex 8d0affc34a..3056ffdfb8 100644\n--- a/refspec.c\n+++ b/refspec.c\n@@ -215,6 +215,16 @@ int valid_fetch_refspec(const char *fetch_refspec_str)\n \treturn ret;\n }\n \n+int valid_remote_name(const char *name)\n+{\n+\tint result;\n+\tstruct strbuf refspec = STRBUF_INIT;\n+\tstrbuf_addf(&refspec, \"refs/heads/test:refs/remotes/%s/test\", name);\n+\tresult = valid_fetch_refspec(refspec.buf);\n+\tstrbuf_release(&refspec);\n+\treturn result;\n+}\n+\n void refspec_ref_prefixes(const struct refspec *rs,\n \t\t\t struct strvec *ref_prefixes)\n {\ndiff --git a/refspec.h b/refspec.h\nindex 7569248d11..bbb25968a8 100644\n--- a/refspec.h\n+++ b/refspec.h\n@@ -62,6 +62,7 @@ void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);\n void refspec_clear(struct refspec *rs);\n \n int valid_fetch_refspec(const char *refspec);\n+int valid_remote_name(const char *name);\n \n struct strvec;\n /*\n-- \ngitgitgadget\n\n"},{"id":"406816","messageId":"3225fd9a-6b44-8c9c-52a9-614ee8470a8c@gmail.com","threadId":"54232","inReplyTo":"pull.727.v3.git.1601523977.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/7] clone: allow configurable default for -o/--origin","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-10-02T12:56:55Z","receivedAt":"2020-10-02T12:57:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/30/2020 11:46 PM, Sean Barag via GitGitGadget wrote:\n> v3 (changes since v2):\n> \n> * [5/7] fix compilation error: validate option_origin since remote_name\n> doesn't exist yet\n> * [7/7] remove default_remote_name; apply default value inline if no other\n> value applied\n> \n> v2 (changes since v1):\n> \n> * Convert Thanks-to trailer to Helped-by\n> * Rewrite several commit titles and messages\n> * Unify error reporting between clone.c and remote.c\n> * Add tests for git remote add and git remote rename with invalid remote\n> names\n> * Prevent leak of old remote_name\n\nSorry for being late to these versions, but I think\nthe changes across these two versions are excellent\nimprovements. I'm happy with this patch series!\n\nThanks,\n-Stolee\n"}]}