From 349cff76de7fdb3b44c7018474134566a7a4ee19 Mon Sep 17 00:00:00 2001 From: Sean Barag Date: Tue, 29 Sep 2020 03:36:49 +0000 Subject: [PATCH 1/7] clone: add tests for --template and some disallowed option pairs Some combinations of command-line options to `git clone` are invalid, but there were previously no tests ensuring those combinations reported errors. Similarly, `git clone --template` didn't appear to have any tests. Helped-by: Jeff King Helped-by: Derrick Stolee Signed-off-by: Sean Barag Signed-off-by: Junio C Hamano --- t/t5606-clone-options.sh | 46 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh index e69427f881..0422b24258 100755 --- a/t/t5606-clone-options.sh +++ b/t/t5606-clone-options.sh @@ -15,7 +15,51 @@ test_expect_success 'setup' ' test_expect_success 'clone -o' ' git clone -o foo parent clone-o && - (cd clone-o && git rev-parse --verify refs/remotes/foo/master) + git -C clone-o rev-parse --verify refs/remotes/foo/master + +' + +test_expect_success 'disallows --bare with --origin' ' + + test_must_fail git clone -o foo --bare parent clone-bare-o 2>err && + test_debug "cat err" && + test_i18ngrep -e "--bare and --origin foo options are incompatible" err + +' + +test_expect_success 'disallows --bare with --separate-git-dir' ' + + test_must_fail git clone --bare --separate-git-dir dot-git-destiation parent clone-bare-sgd 2>err && + test_debug "cat err" && + test_i18ngrep -e "--bare and --separate-git-dir are incompatible" err + +' + +test_expect_success 'uses "origin" for default remote name' ' + + git clone parent clone-default-origin && + git -C clone-default-origin rev-parse --verify refs/remotes/origin/master + +' + +test_expect_success 'prefers --template config over normal config' ' + + template="$TRASH_DIRECTORY/template-with-config" && + mkdir "$template" && + git config --file "$template/config" foo.bar from_template && + test_config_global foo.bar from_global && + git clone "--template=$template" parent clone-template-config && + test "$(git -C clone-template-config config --local foo.bar)" = "from_template" + +' + +test_expect_success 'prefers -c config over --template config' ' + + template="$TRASH_DIRECTORY/template-with-ignored-config" && + mkdir "$template" && + git config --file "$template/config" foo.bar from_template && + git clone "--template=$template" -c foo.bar=inline parent clone-template-inline-config && + test "$(git -C clone-template-inline-config config --local foo.bar)" = "inline" ' From 552955ed7fcbc3706f2d6192487df2fb8f3283c8 Mon Sep 17 00:00:00 2001 From: Sean Barag Date: Thu, 1 Oct 2020 03:46:11 +0000 Subject: [PATCH 2/7] clone: use more conventional config/option layering Parsing command-line options before reading from config required careful handling to ensure CLI options were treated with higher priority. Read config first to let parsed CLI naively overwrite matching config values. Helped-by: Junio C Hamano Helped-by: Johannes Schindelin Signed-off-by: Sean Barag Signed-off-by: Junio C Hamano --- builtin/clone.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/builtin/clone.c b/builtin/clone.c index b087ee40c2..258bfd65b7 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -851,8 +851,22 @@ static int checkout(int submodule_progress) return err; } +static int git_clone_config(const char *k, const char *v, void *cb) +{ + return git_default_config(k, v, cb); +} + static int write_one_config(const char *key, const char *value, void *data) { + /* + * give git_clone_config a chance to write config values back to the + * environment, since git_config_set_multivar_gently only deals with + * config-file writes + */ + int apply_failed = git_clone_config(key, value, data); + if (apply_failed) + return apply_failed; + return git_config_set_multivar_gently(key, value ? value : "true", CONFIG_REGEX_NONE, 0); @@ -964,6 +978,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix) struct strvec ref_prefixes = STRVEC_INIT; packet_trace_identity("clone"); + + git_config(git_clone_config, NULL); + argc = parse_options(argc, argv, prefix, builtin_clone_options, builtin_clone_usage, 0); @@ -1125,9 +1142,17 @@ int cmd_clone(int argc, const char **argv, const char *prefix) if (real_git_dir) git_dir = real_git_dir; + /* + * additional config can be injected with -c, make sure it's included + * after init_db, which clears the entire config environment. + */ write_config(&option_config); - git_config(git_default_config, NULL); + /* + * re-read config after init_db and write_config to pick up any config + * injected by --template and --config, respectively. + */ + git_config(git_clone_config, NULL); if (option_bare) { if (option_mirror) From 444825c7c1148c0a38708238467c33ba370c58b2 Mon Sep 17 00:00:00 2001 From: Sean Barag Date: Thu, 1 Oct 2020 03:46:12 +0000 Subject: [PATCH 3/7] remote: add tests for add and rename with invalid names In preparation for a future patch that moves `builtin/remote.c`'s remote-name validation, ensure `git remote add` and `git remote rename` report errors when the new name isn't valid. Signed-off-by: Sean Barag Signed-off-by: Junio C Hamano --- t/t5505-remote.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 8d62edd98b..1156f52069 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -179,6 +179,13 @@ test_expect_success 'rename errors out early when deleting non-existent branch' ) ' +test_expect_success 'rename errors out early when when new name is invalid' ' + test_config remote.foo.vcs bar && + echo "fatal: '\''invalid...name'\'' is not a valid remote name" >expect && + test_must_fail git remote rename foo invalid...name 2>actual && + test_i18ncmp expect actual +' + test_expect_success 'add existing foreign_vcs remote' ' test_config remote.foo.vcs bar && echo "fatal: remote foo already exists." >expect && @@ -194,6 +201,12 @@ test_expect_success 'add existing foreign_vcs remote' ' test_i18ncmp expect actual ' +test_expect_success 'add invalid foreign_vcs remote' ' + echo "fatal: '\''invalid...name'\'' is not a valid remote name" >expect && + test_must_fail git remote add invalid...name bar 2>actual && + test_i18ncmp expect actual +' + cat >test/expect < Date: Thu, 1 Oct 2020 03:46:13 +0000 Subject: [PATCH 4/7] refs: consolidate remote name validation In preparation for a future patch, extract from remote.c a function that validates possible remote names so that its rules can be used consistently in other places. Helped-by: Derrick Stolee Signed-off-by: Sean Barag Signed-off-by: Junio C Hamano --- builtin/remote.c | 7 ++----- refspec.c | 10 ++++++++++ refspec.h | 1 + 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/builtin/remote.c b/builtin/remote.c index 542f56e387..b2e0959746 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -194,8 +194,7 @@ static int add(int argc, const char **argv) if (remote_is_configured(remote, 1)) die(_("remote %s already exists."), name); - strbuf_addf(&buf2, "refs/heads/test:refs/remotes/%s/test", name); - if (!valid_fetch_refspec(buf2.buf)) + if (!valid_remote_name(name)) die(_("'%s' is not a valid remote name"), name); strbuf_addf(&buf, "remote.%s.url", name); @@ -696,11 +695,9 @@ static int mv(int argc, const char **argv) if (remote_is_configured(newremote, 1)) die(_("remote %s already exists."), rename.new_name); - strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new_name); - if (!valid_fetch_refspec(buf.buf)) + if (!valid_remote_name(rename.new_name)) die(_("'%s' is not a valid remote name"), rename.new_name); - strbuf_reset(&buf); strbuf_addf(&buf, "remote.%s", rename.old_name); strbuf_addf(&buf2, "remote.%s", rename.new_name); if (git_config_rename_section(buf.buf, buf2.buf) < 1) diff --git a/refspec.c b/refspec.c index f10ef284ce..98d1caaa0a 100644 --- a/refspec.c +++ b/refspec.c @@ -201,6 +201,16 @@ int valid_fetch_refspec(const char *fetch_refspec_str) return ret; } +int valid_remote_name(const char *name) +{ + int result; + struct strbuf refspec = STRBUF_INIT; + strbuf_addf(&refspec, "refs/heads/test:refs/remotes/%s/test", name); + result = valid_fetch_refspec(refspec.buf); + strbuf_release(&refspec); + return result; +} + void refspec_ref_prefixes(const struct refspec *rs, struct strvec *ref_prefixes) { diff --git a/refspec.h b/refspec.h index 8d654e3a3a..ee93196e35 100644 --- a/refspec.h +++ b/refspec.h @@ -60,6 +60,7 @@ void refspec_appendn(struct refspec *rs, const char **refspecs, int nr); void refspec_clear(struct refspec *rs); int valid_fetch_refspec(const char *refspec); +int valid_remote_name(const char *name); struct strvec; /* From ebe7e28a3600f4e67e9a1781e335adb36f3f139b Mon Sep 17 00:00:00 2001 From: Sean Barag Date: Thu, 1 Oct 2020 03:46:14 +0000 Subject: [PATCH 5/7] clone: validate --origin option before use Providing a bad origin name to `git clone` currently reports an 'invalid refspec' error instead of a more explicit message explaining that the `--origin` option was malformed. This behavior dates back to since 8434c2f1 (Build in clone, 2008-04-27). Reintroduce validation for the provided `--origin` option, but notably _don't_ include a multi-level check (e.g. "foo/bar") that was present in the original `git-clone.sh`. `git remote` allows multi-level remote names since at least 46220ca100 (remote.c: Fix overtight refspec validation, 2008-03-20), so that appears to be the desired behavior. Helped-by: Junio C Hamano Helped-by: Derrick Stolee Helped-by: Jeff King Signed-off-by: Sean Barag Signed-off-by: Junio C Hamano --- builtin/clone.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/builtin/clone.c b/builtin/clone.c index 258bfd65b7..78364a0861 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -1012,6 +1012,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix) if (!option_origin) option_origin = "origin"; + if (!valid_remote_name(option_origin)) + die(_("'%s' is not a valid remote name"), option_origin); + repo_name = argv[0]; path = get_repo_path(repo_name, &is_bundle); From 75ca3906b1ea6a00a20ef16c889e9d6a15a8defc Mon Sep 17 00:00:00 2001 From: Sean Barag Date: Thu, 1 Oct 2020 03:46:15 +0000 Subject: [PATCH 6/7] clone: read new remote name from remote_name instead of option_origin In a future patch, the name of the remote created by `git clone` may come from multiple sources. To avoid confusion, convert most uses of option_origin to remote_name, leaving option_origin to exclusively represent the -o/--origin option. Helped-by: Derrick Stolee Signed-off-by: Sean Barag Signed-off-by: Junio C Hamano --- builtin/clone.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 78364a0861..c33fa127ce 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -53,6 +53,7 @@ static int option_shallow_submodules; static int deepen; static char *option_template, *option_depth, *option_since; static char *option_origin = NULL; +static char *remote_name = "origin"; static char *option_branch = NULL; static struct string_list option_not = STRING_LIST_INIT_NODUP; static const char *real_git_dir; @@ -721,7 +722,7 @@ static void update_head(const struct ref *our, const struct ref *remote, if (!option_bare) { update_ref(msg, "HEAD", &our->old_oid, NULL, 0, UPDATE_REFS_DIE_ON_ERR); - install_branch_config(0, head, option_origin, our->name); + install_branch_config(0, head, remote_name, our->name); } } else if (our) { struct commit *c = lookup_commit_reference(the_repository, @@ -919,12 +920,12 @@ static void write_refspec_config(const char *src_ref_prefix, } /* Configure the remote */ if (value.len) { - strbuf_addf(&key, "remote.%s.fetch", option_origin); + strbuf_addf(&key, "remote.%s.fetch", remote_name); git_config_set_multivar(key.buf, value.buf, "^$", 0); strbuf_reset(&key); if (option_mirror) { - strbuf_addf(&key, "remote.%s.mirror", option_origin); + strbuf_addf(&key, "remote.%s.mirror", remote_name); git_config_set(key.buf, "true"); strbuf_reset(&key); } @@ -1009,11 +1010,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix) option_no_checkout = 1; } - if (!option_origin) - option_origin = "origin"; + if (option_origin) + remote_name = option_origin; - if (!valid_remote_name(option_origin)) - die(_("'%s' is not a valid remote name"), option_origin); + if (!valid_remote_name(remote_name)) + die(_("'%s' is not a valid remote name"), remote_name); repo_name = argv[0]; @@ -1164,15 +1165,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix) git_config_set("core.bare", "true"); } else { - strbuf_addf(&branch_top, "refs/remotes/%s/", option_origin); + strbuf_addf(&branch_top, "refs/remotes/%s/", remote_name); } - strbuf_addf(&key, "remote.%s.url", option_origin); + strbuf_addf(&key, "remote.%s.url", remote_name); git_config_set(key.buf, repo); strbuf_reset(&key); if (option_no_tags) { - strbuf_addf(&key, "remote.%s.tagOpt", option_origin); + strbuf_addf(&key, "remote.%s.tagOpt", remote_name); git_config_set(key.buf, "--no-tags"); strbuf_reset(&key); } @@ -1183,7 +1184,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) if (option_sparse_checkout && git_sparse_checkout_init(dir)) return 1; - remote = remote_get(option_origin); + remote = remote_get(remote_name); strbuf_addf(&default_refspec, "+%s*:%s*", src_ref_prefix, branch_top.buf); @@ -1296,7 +1297,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) if (!our_head_points_at) die(_("Remote branch %s not found in upstream %s"), - option_branch, option_origin); + option_branch, remote_name); } else our_head_points_at = remote_head_points_at; @@ -1304,7 +1305,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) else { if (option_branch) die(_("Remote branch %s not found in upstream %s"), - option_branch, option_origin); + option_branch, remote_name); warning(_("You appear to have cloned an empty repository.")); mapped_refs = NULL; @@ -1316,7 +1317,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) const char *branch = git_default_branch_name(); char *ref = xstrfmt("refs/heads/%s", branch); - install_branch_config(0, branch, option_origin, ref); + install_branch_config(0, branch, remote_name, ref); free(ref); } } @@ -1325,7 +1326,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) remote_head_points_at, &branch_top); if (filter_options.choice) - partial_clone_register(option_origin, &filter_options); + partial_clone_register(remote_name, &filter_options); if (is_local) clone_local(path, git_dir); From de9ed3ef3740f8227cc924e845032954d1f1b1b7 Mon Sep 17 00:00:00 2001 From: Sean Barag Date: Thu, 1 Oct 2020 03:46:16 +0000 Subject: [PATCH 7/7] clone: allow configurable default for `-o`/`--origin` While the default remote name of "origin" can be changed at clone-time with `git clone`'s `--origin` option, it was previously not possible to specify a default value for the name of that remote. Add support for a new `clone.defaultRemoteName` config, with the newly-created remote name resolved in priority order: 1. (Highest priority) A remote name passed directly to `git clone -o` 2. A `clone.defaultRemoteName=new_name` in config `git clone -c` 3. A `clone.defaultRemoteName` value set in `/path/to/template/config`, where `--template=/path/to/template` is provided 4. A `clone.defaultRemoteName` value set in a non-template config file 5. The default value of `origin` Helped-by: Junio C Hamano Helped-by: Johannes Schindelin Helped-by: Derrick Stolee Helped-by: Andrei Rybak Signed-off-by: Sean Barag Signed-off-by: Junio C Hamano --- Documentation/config.txt | 2 ++ Documentation/config/clone.txt | 4 ++++ Documentation/git-clone.txt | 5 +++-- builtin/clone.c | 26 +++++++++++++++++++------- t/t5606-clone-options.sh | 22 ++++++++++++++++++++++ 5 files changed, 50 insertions(+), 9 deletions(-) create mode 100644 Documentation/config/clone.txt diff --git a/Documentation/config.txt b/Documentation/config.txt index 3042d80978..354874facf 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -334,6 +334,8 @@ include::config/checkout.txt[] include::config/clean.txt[] +include::config/clone.txt[] + include::config/color.txt[] include::config/column.txt[] diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt new file mode 100644 index 0000000000..47de36a5fe --- /dev/null +++ b/Documentation/config/clone.txt @@ -0,0 +1,4 @@ +clone.defaultRemoteName:: + The name of the remote to create when cloning a repository. Defaults to + `origin`, and can be overridden by passing the `--origin` command-line + option to linkgit:git-clone[1]. diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt index c898310099..f04bf6e6ba 100644 --- a/Documentation/git-clone.txt +++ b/Documentation/git-clone.txt @@ -183,8 +183,9 @@ objects from the source repository into a pack in the cloned repository. -o :: --origin :: - Instead of using the remote name `origin` to keep track - of the upstream repository, use ``. + Instead of using the remote name `origin` to keep track of the upstream + repository, use ``. Overrides `clone.defaultRemoteName` from the + config. -b :: --branch :: diff --git a/builtin/clone.c b/builtin/clone.c index c33fa127ce..a144e7f35b 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -53,7 +53,7 @@ static int option_shallow_submodules; static int deepen; static char *option_template, *option_depth, *option_since; static char *option_origin = NULL; -static char *remote_name = "origin"; +static char *remote_name = NULL; static char *option_branch = NULL; static struct string_list option_not = STRING_LIST_INIT_NODUP; static const char *real_git_dir; @@ -854,6 +854,10 @@ static int checkout(int submodule_progress) static int git_clone_config(const char *k, const char *v, void *cb) { + if (!strcmp(k, "clone.defaultremotename")) { + free(remote_name); + remote_name = xstrdup(v); + } return git_default_config(k, v, cb); } @@ -1010,12 +1014,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix) option_no_checkout = 1; } - if (option_origin) - remote_name = option_origin; - - if (!valid_remote_name(remote_name)) - die(_("'%s' is not a valid remote name"), remote_name); - repo_name = argv[0]; path = get_repo_path(repo_name, &is_bundle); @@ -1158,6 +1156,19 @@ int cmd_clone(int argc, const char **argv, const char *prefix) */ git_config(git_clone_config, NULL); + /* + * apply the remote name provided by --origin only after this second + * call to git_config, to ensure it overrides all config-based values. + */ + if (option_origin != NULL) + remote_name = xstrdup(option_origin); + + if (remote_name == NULL) + remote_name = xstrdup("origin"); + + if (!valid_remote_name(remote_name)) + die(_("'%s' is not a valid remote name"), remote_name); + if (option_bare) { if (option_mirror) src_ref_prefix = "refs/"; @@ -1358,6 +1369,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) junk_mode = JUNK_LEAVE_REPO; err = checkout(submodule_progress); + free(remote_name); strbuf_release(&reflog_msg); strbuf_release(&branch_top); strbuf_release(&key); diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh index 0422b24258..5e201e7d85 100755 --- a/t/t5606-clone-options.sh +++ b/t/t5606-clone-options.sh @@ -19,6 +19,13 @@ test_expect_success 'clone -o' ' ' +test_expect_success 'rejects invalid -o/--origin' ' + + test_must_fail git clone -o "bad...name" parent clone-bad-name 2>err && + test_i18ngrep "'\''bad...name'\'' is not a valid remote name" err + +' + test_expect_success 'disallows --bare with --origin' ' test_must_fail git clone -o foo --bare parent clone-bare-o 2>err && @@ -63,6 +70,21 @@ test_expect_success 'prefers -c config over --template config' ' ' +test_expect_success 'prefers config "clone.defaultRemoteName" over default' ' + + test_config_global clone.defaultRemoteName from_config && + git clone parent clone-config-origin && + git -C clone-config-origin rev-parse --verify refs/remotes/from_config/master + +' + +test_expect_success 'prefers --origin over -c config' ' + + git clone -c clone.defaultRemoteName=inline --origin from_option parent clone-o-and-inline-config && + git -C clone-o-and-inline-config rev-parse --verify refs/remotes/from_option/master + +' + test_expect_success 'redirected clone does not show progress' ' git clone "file://$(pwd)/parent" clone-redirected >out 2>err &&