Re: [PATCH 4/4] clone: allow configurable default for `-o`/`--origin`

From: Derrick Stolee <stolee@gmail.com>
Date: Sep 11, 2020, 19:13 UTC
Message-ID: <0739858e-83f1-3b3f-aba0-7549e7e47cfc@gmail.com>
In-Reply-To: <5c519376c29a28bd89a712cf0b8125fc1c2c81be.1599848727.git.gitgitgadget@gmail.com>

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:

Show 6 quoted lines

> -	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:

Show 6 quoted lines

>  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:

Show 8 quoted lines

>  	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);

Show 13 quoted lines

> --- 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.

Show 11 quoted lines

>  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:

Show 5 quoted lines

> +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.

Show 7 quoted lines

> +	(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