| Message ID | 5c519376c29a28bd89a712cf0b8125fc1c2c81be.1599848727.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clone: allow configurable default for -o/--origin | expand |
On 9/11/2020 2:25 PM, Sean Barag via GitGitGadget wrote: > static char *option_origin = NULL; > +static char *remote_name = "origin"; This patch could have used a prep patch that had all consumers of option_origin use remote_name instead, with this adjustment to the way to use option_origin: > - if (!option_origin) > - option_origin = "origin"; > + if (option_origin) > + remote_name = option_origin; > + else > + remote_name = "origin"; Then this patch introducing the config option would have a very limited change in the builtin consisting of these two hunks: > static int git_clone_config(const char *k, const char *v, void *cb) > { > + if (!strcmp(k, "clone.defaultremotename") && !option_origin) > + remote_name = xstrdup(v); > return git_default_config(k, v, cb); > } ... > if (option_origin) > remote_name = option_origin; > - else > - remote_name = "origin" along with this translators comment note: > if (!valid_fetch_refspec(resolved_refspec.buf)) > - /* TRANSLATORS: %s will be the user-provided --origin / -o option */ > - die(_("'%s' is not a valid origin name"), option_origin); > + /* > + * TRANSLATORS: %s will be the user-provided --origin / -o option, or the value > + * of clone.defaultremotename from the config. > + */ > + die(_("'%s' is not a valid origin name"), remote_name); > --- a/t/t5606-clone-options.sh > +++ b/t/t5606-clone-options.sh > @@ -43,13 +43,6 @@ test_expect_success 'disallows --bare with --separate-git-dir' ' > > ' > > -test_expect_success 'uses "origin" for default remote name' ' > - > - git clone parent clone-default-origin && > - (cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master) > - > -' > - Interesting that you moved this test. Probably not necessary, and just a mistake. > test_expect_success 'prefers --template config over normal config' ' > > template="$TRASH_DIRECTORY/template-with-config" && > @@ -71,6 +64,28 @@ test_expect_success 'prefers -c config over --template config' ' > > ' > > +test_expect_success 'uses "origin" for default remote name' ' > + > + git clone parent clone-default-origin && > + (cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master) I didn't notice it earlier, but perhaps this subshell should be split into its own multi-line section as follows: > + ( > + cd clone-default-origin && > + git rev-parse --verify refs/remotes/origin/master > + ) But even better, this is only one line so using "git -C clone-default-origin rev-parse" is simpler: > +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 config "clone.defaultRemoteName" over default' ' > + > + test_config_global clone.defaultRemoteName from_config && > + git clone parent clone-config-origin && This could be done using "git -c clone.defaultRemoteName=from_config" instead of setting the global config. > + (cd clone-config-origin && git 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 && And you use the -c option here. > + (cd clone-o-and-inline-config && git rev-parse --verify refs/remotes/from_option/master) > + > +' > + We have the extra newline in these tests, too. Thanks, -Stolee
"Sean Barag via GitGitGadget" <gitgitgadget@gmail.com> writes: > diff --git a/builtin/clone.c b/builtin/clone.c > index 1cd62d0001..aeb41f15f3 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"; This has a side effect of making all the code locations that used to refer to option_origin much easier to read, like ... > @@ -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); ... this place ;-) Happy. > static int git_clone_config(const char *k, const char *v, void *cb) > { > + if (!strcmp(k, "clone.defaultremotename") && !option_origin) > + remote_name = xstrdup(v); > return git_default_config(k, v, cb); > } clone.defaultremotename is a single valued configuration variable, and this correctly implements the "last one wins" behaviour (but previous remote_name will leak every time clone.defaultremotename is seen in the config stream). Also this code arrangement is not quite satisfactory. It means that we cannot re-read any configuration variable that does not have an accompanying command line option. I thought the whole point of doing the write_config() was so that anything came from the command line option can be written back to the configuration file, so I am not sure what the harm would be to update remote_name from the configuration whether option_origin is used or not here. Perhaps add "clone.defaultremotename" to the set of configuration setting write_config() uses, when --option is given from the command line, and remove this special case? By the way, I now realized why 2/4's "read twice" is OK. init_db() calls create_default_files() and we do clare the cached configset by calling git_config_clear() there. Thanks.
On 2020-09-11 20:25, Sean Barag via GitGitGadget wrote: > From: Sean Barag <sean@barag.org> > > 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. This commit > adds support for a new `clone.defaultRemoteName` config. > > It's resolved in the expected 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 > 3. A `clone.defaultRemoteName` value set in a non-template config file Number 3 is used twice in this list. > 4. The default value of `origin` > > Signed-off-by: Sean Barag <sean@barag.org> > Thanks-to: Junio C Hamano <gitster@pobox.com> > Thanks-to: Johannes Schindelin <johannes.schindelin@gmx.de>
Junio C Hamano <gitster@pobox.com> writes: > clone.defaultremotename is a single valued configuration variable, > and this correctly implements the "last one wins" behaviour (but > previous remote_name will leak every time clone.defaultremotename > is seen in the config stream). Great catch - fixed for v2. > I thought the whole point of doing the write_config() was so that > anything came from the command line option can be written back to the > configuration file, so I am not sure what the harm would be to update > remote_name from the configuration whether option_origin is used or > not here. Perhaps add "clone.defaultremotename" to the set of > configuration setting write_config() uses, when --option is given from > the command line, and remove this special case? That's true! The special case was there mostly to keep --origin as the highest priority, but that can be solved much more naturally by moving the assignment from option_origin to remote_name down below the second git_config call. Included in v2. Sean
Derrick Stolee <stolee@gmail.com> writes: > > static char *option_origin = NULL; > > +static char *remote_name = "origin"; > > This patch could have used a prep patch that had all consumers > of option_origin use remote_name instead, with this adjustment > to the way to use option_origin: > > > - if (!option_origin) > > - option_origin = "origin"; > > + if (option_origin) > > + remote_name = option_origin; > > + else > > + remote_name = "origin"; > > Then this patch introducing the config option would have a > very limited change in the builtin consisting of these two > hunks: > > > static int git_clone_config(const char *k, const char *v, void *cb) > > { > > + if (!strcmp(k, "clone.defaultremotename") && !option_origin) > > + remote_name = xstrdup(v); > > return git_default_config(k, v, cb); > > } > ... > > if (option_origin) > > remote_name = option_origin; > > - else > > - remote_name = "origin" That's a great idea! Implemented for v2 :) Sean
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..20755d413a --- /dev/null +++ b/Documentation/config/clone.txt @@ -0,0 +1,5 @@ +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 <name>:: --origin <name>:: - Instead of using the remote name `origin` to keep track - of the upstream repository, use `<name>`. + Instead of using the remote name `origin` to keep track of the upstream + repository, use `<name>`. Overrides `clone.defaultRemoteName` from the + config. -b <name>:: --branch <name>:: diff --git a/builtin/clone.c b/builtin/clone.c index 1cd62d0001..aeb41f15f3 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, @@ -853,16 +854,18 @@ static int checkout(int submodule_progress) static int git_clone_config(const char *k, const char *v, void *cb) { + if (!strcmp(k, "clone.defaultremotename") && !option_origin) + remote_name = xstrdup(v); return git_default_config(k, v, cb); } static int write_one_config(const char *key, const char *value, void *data) { /* - * give git_config_default a chance to write config values back to the environment, since + * 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_default_config(key, value, data); + int apply_failed = git_clone_config(key, value, data); if (apply_failed) return apply_failed; @@ -918,12 +921,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,13 +1012,16 @@ 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; - strbuf_addf(&resolved_refspec, "refs/heads/test:refs/remotes/%s/test", option_origin); + strbuf_addf(&resolved_refspec, "refs/heads/test:refs/remotes/%s/test", remote_name); if (!valid_fetch_refspec(resolved_refspec.buf)) - /* TRANSLATORS: %s will be the user-provided --origin / -o option */ - die(_("'%s' is not a valid origin name"), option_origin); + /* + * TRANSLATORS: %s will be the user-provided --origin / -o option, or the value + * of clone.defaultremotename from the config. + */ + die(_("'%s' is not a valid origin name"), remote_name); strbuf_release(&resolved_refspec); repo_name = argv[0]; @@ -1167,15 +1173,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); } @@ -1186,7 +1192,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); @@ -1299,7 +1305,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; @@ -1307,7 +1313,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; @@ -1319,7 +1325,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); } } @@ -1328,7 +1334,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); diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh index c865f96def..017c24a454 100755 --- a/t/t5606-clone-options.sh +++ b/t/t5606-clone-options.sh @@ -43,13 +43,6 @@ test_expect_success 'disallows --bare with --separate-git-dir' ' ' -test_expect_success 'uses "origin" for default remote name' ' - - git clone parent clone-default-origin && - (cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master) - -' - test_expect_success 'prefers --template config over normal config' ' template="$TRASH_DIRECTORY/template-with-config" && @@ -71,6 +64,28 @@ test_expect_success 'prefers -c config over --template config' ' ' +test_expect_success 'uses "origin" for default remote name' ' + + git clone parent clone-default-origin && + (cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master) + +' + +test_expect_success 'prefers config "clone.defaultRemoteName" over default' ' + + test_config_global clone.defaultRemoteName from_config && + git clone parent clone-config-origin && + (cd clone-config-origin && git 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 && + (cd clone-o-and-inline-config && git 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 &&