diff mbox series

[v2,7/7] clone: allow configurable default for `-o`/`--origin`

Message ID 737f91c6244220eec196c327fcea9a6548c45310.1601350615.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series clone: allow configurable default for -o/--origin | expand

Commit Message

Jean-Noël Avila via GitGitGadget Sept. 29, 2020, 3:36 a.m. UTC 
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.  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 <gitster@pobox.com>
Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Helped-by: Derrick Stolee <stolee@gmail.com>
Helped-by: Andrei Rybak <rybak.a.v@gmail.com>
Signed-off-by: Sean Barag <sean@barag.org>
---
 Documentation/config.txt       |  2 ++
 Documentation/config/clone.txt |  5 +++++
 Documentation/git-clone.txt    |  5 +++--
 builtin/clone.c                | 27 +++++++++++++++++++--------
 t/t5606-clone-options.sh       | 22 ++++++++++++++++++++++
 5 files changed, 51 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/config/clone.txt

Comments

Junio C Hamano Sept. 29, 2020, 7:59 p.m. UTC | #1 
"Sean Barag via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  static int git_clone_config(const char *k, const char *v, void *cb)
>  {
> +	if (!strcmp(k, "clone.defaultremotename")) {
> +		if (remote_name != default_remote_name)
> +			free(remote_name);
> +		remote_name = xstrdup(v);

This feels strange.  The usual arrangement is

    - initialize the variable to NULL (or any value that the code
      can tell that nobody touched it);

    - let git_config() callback to update the variable, taking care
      of freeing and strduping as needed.  Note that free(NULL) is
      kosher.

    - let parse_options() to further update the variable, taking
      care of freeing and strduping as needed.

    - finally, if the variable is still NULL, give it its default
      value.

so there is no room for the "if the variable has the value of the
fallback default, do things differently" logic to be in the
git_config() callback function.

> @@ -976,6 +982,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	int submodule_progress;
>  
>  	struct strvec ref_prefixes = STRVEC_INIT;

Missing blank line here.

> +	remote_name = default_remote_name;

Isn't the reason why the git_config() callback we saw above has an
unusual special case for default_remote_name because we have this
assignment way too early?  Would it make the control flow in a more
natural way if we removed this, and then after parse_options() did
the "-o" handling, add something like

	if (!remote_name)
		remote_name = xstrdup("origin");

instead?

> @@ -1153,10 +1154,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  
>  	/*
>  	 * re-read config after init_db and write_config to pick up any config
> -	 * injected by --template and --config, respectively
> +	 * injected by --template and --config, respectively.
>  	 */

Squash this "oops, I forgot to finish the sentence" to the step the
mistake was introduced, i.e. "use more conventional..."

>  	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)
> +		remote_name = option_origin;

And here would be where you'd fall back

	if (!remote_name)
		remote_name = xstrdup("origin");

Note that you'd need to dup the option_origin for consistency if you
want to free() it at the end.

> +	if (!valid_remote_name(remote_name))
> +		die(_("'%s' is not a valid remote name"), remote_name);
> +
Sean Barag Sept. 29, 2020, 11:47 p.m. UTC | #2 
"Junio C Hamano" <gitster@pobox.com> writes:

> >  static int git_clone_config(const char *k, const char *v, void *cb)
> >  {
> > +	if (!strcmp(k, "clone.defaultremotename")) {
> > +		if (remote_name != default_remote_name)
> > +			free(remote_name);
> > +		remote_name = xstrdup(v);
> 
> This feels strange.  The usual arrangement is
> 
>     - initialize the variable to NULL (or any value that the code
>       can tell that nobody touched it);
> 
>     - let git_config() callback to update the variable, taking care
>       of freeing and strduping as needed.  Note that free(NULL) is
>       kosher.
> 
>     - let parse_options() to further update the variable, taking
>       care of freeing and strduping as needed.
> 
>     - finally, if the variable is still NULL, give it its default
>       value.
> 
> so there is no room for the "if the variable has the value of the
> fallback default, do things differently" logic to be in the
> git_config() callback function.

I agree, it felt pretty awkward while writing it!  I was hoping to match
the start-up sequence you'd mentioned in my original attempt [1] but I'm
happy to move the default value assignment down.

> > @@ -1153,10 +1154,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> >  
> >  	/*
> >  	 * re-read config after init_db and write_config to pick up any config
> > -	 * injected by --template and --config, respectively
> > +	 * injected by --template and --config, respectively.
> >  	 */
> 
> Squash this "oops, I forgot to finish the sentence" to the step the
> mistake was introduced, i.e. "use more conventional..."

How embarrassing, I thought I'd gotten all of those.  Will do.

Sean

--
[1] Original attempt at this feature, in separate thread because of the
    divergence: https://lore.kernel.org/git/xmqqlfi1utwi.fsf@gitster.c.googlers.com/
diff mbox series

Patch

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f93b6837e4..8c58c53ae7 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 097e6a86c5..876aedcd47 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 7fdd00cd95..4c821de242 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -53,7 +53,8 @@  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 *default_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 +855,11 @@  static int checkout(int submodule_progress)
 
 static int git_clone_config(const char *k, const char *v, void *cb)
 {
+	if (!strcmp(k, "clone.defaultremotename")) {
+		if (remote_name != default_remote_name)
+			free(remote_name);
+		remote_name = xstrdup(v);
+	}
 	return git_default_config(k, v, cb);
 }
 
@@ -976,6 +982,7 @@  int cmd_clone(int argc, const char **argv, const char *prefix)
 	int submodule_progress;
 
 	struct strvec ref_prefixes = STRVEC_INIT;
+	remote_name = default_remote_name;
 
 	packet_trace_identity("clone");
 
@@ -1009,12 +1016,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);
@@ -1153,10 +1154,20 @@  int cmd_clone(int argc, const char **argv, const char *prefix)
 
 	/*
 	 * re-read config after init_db and write_config to pick up any config
-	 * injected by --template and --config, respectively
+	 * injected by --template and --config, respectively.
 	 */
 	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)
+		remote_name = option_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/";
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 &&