Re: [PATCH] clone: add remote.cloneDefault config option

[Junio C Hamano <gitster@pobox.com>]

"Sean Barag via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Sean Barag <sean@barag.org>
>
> While the default remote name of `origin` can be overridden both
> pre-clone (with `git clone --origin foo`) and post-clone (with `git
> remote rename origin foo`), it'd be handy to have a configurable
> system-wide default for clones!  

I doubt it is handy enough to deserve an explamation point.  Replace
it with a plain-vanilla full-stop instead.

I however tend to agree that, even evidenced by the manual page
description of "git clone", i.e.

    -o <name>::
    --origin <name>::
            Instead of using the remote name `origin` to keep track
            of the upstream repository, use `<name>`.

that it is understandable if many users and projects wish to call it
"upstream".

> This commit implements
> `remote.cloneDefault` as a parallel to `remote.pushDefault`,
> with prioritized name resolution:

I highly doubt that .cloneDefault is a good name.  After reading
only the title of the patch e-mail, i.e. when the only available
information on the change available to me was the name of the
configuration variable and the fact that it pertains to the command
"git clone", I thought it is to specify a URL, from which "git
clone" without the URL would clone from that single repository.

And the name will cause the same misunderstanding to normal users,
not just to reviewers of your patch, after this change hits a future
Git release.

Taking a parallel from init.defaultBranchName, I would probably call
it clone.defaultUpstreamName if I were writing this feature.

> diff --git a/builtin/clone.c b/builtin/clone.c
> index b087ee40c2..b0dbb848c6 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -941,6 +941,29 @@ static int path_exists(const char *path)
>  	return !stat(path, &sb);
>  }
>  
> +struct clone_default_info
> +{
> +	enum config_scope scope;
> +	struct strbuf remote_name;
> +	int linenr;
> +};
> +
> +static int config_read_clone_default(const char *key, const char *value,
> +	void *cb)
> +{
> +	struct clone_default_info* info = cb;
> +	if (strcmp(key, "remote.clonedefault") || !value) {
> +		return 0;
> +	}
> +
> +	info->scope = current_config_scope();
> +	strbuf_reset(&info->remote_name);
> +	strbuf_addstr(&info->remote_name, value);
> +	info->linenr = current_config_line();
> +
> +	return 0;
> +}

This feels way overkill and insufficient at the same time.  It does
not need scope, it does not need linenr, and we already have a place
to store end-user specified name for the upstream in the form of the
variable option_origin.  And the code is not diagnosing any error.

static int git_clone_config(const char *k, const char *v, void *cb)
{
	if (option_origin)
		return 0; /* ignore -- the user gave us an option */

	if (!strcmp(k, "clone.defaultupstreamname")) {
		if (!v)
			return config_error_nonbool(k);
		if (strchr(v, '/') || check_refname_format(v, REFNAME_ALLOW_ONELEVEL))
                	return error(_("invalid upstream name '%s'"), v);
		option_origin = xstrdup(v);
		return 0;
	}
	return 0;
}

would be sufficient, and at the same time makes sure it rejects
names like 'o..ri..gin', 'o/ri/gin', etc.

> @@ -992,8 +1015,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  		option_no_checkout = 1;
>  	}
>  
> -	if (!option_origin)
> -		option_origin = "origin";
> +	if (!option_origin) {
> +		struct clone_default_info clone_default = { CONFIG_SCOPE_UNKNOWN, STRBUF_INIT, -1 };
> +		git_config(config_read_clone_default, &clone_default);
> +		if (strcmp("", (const char*) clone_default.remote_name.buf))
> +			option_origin = clone_default.remote_name.buf;
> +		else
> +			option_origin = "origin";
> +	}
> +

It is somewhat sad that we have the git_config(git_default_config)
call so late in the control flow.  I wonder if we can update the
start-up sequence to match the usual flow, e.g. these things happen
in the following order in a normal program.

    - variables are given their default value.

    - call git_config(git_commandspecific_config, ...) early in the
      program.

      - git_commandspecific_config() interprets the command specific
        variables and passes everything else to git_default_config()

      variables like option_origin are given values of configuration
      variable at this point.

    - call parse_options() next, which may override the variables
      from the value on the command line.

    - main control flow uses the variable.  "Command-line wins over
      configuration which wins over the default" falls out naturally.

One oddity "git clone" has is that it wants to delay the reading of
configuration files (they are read only once, and second and
subsequent git_config() calls will reuse what was read before [*])
so that it can read what clone.c::write_config() wrote, so if we were
to "fix" the start-up sequence to match the usual flow, we need to
satisfy what that odd arrangement wanted to achieve in some other
way (e.g. feed what is in option_config to git_default_config
ourselves, without using git_config(), as part of the "main control
flow uses the variable" part), but it should be doable.

	[*Side note*].  The above means that this patch, even when
	the configuration variable does not give upstream name, may
	break the option_config feature by breaking the second call
	to git_config().  We need to have a test for that.

> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> index e69427f881..8aac67b385 100755
> --- a/t/t5606-clone-options.sh
> +++ b/t/t5606-clone-options.sh
> @@ -19,6 +19,20 @@ test_expect_success 'clone -o' '
>  
>  '
>  
> +test_expect_success 'clone respects remote.cloneDefault' '
> +
> +	git -c remote.cloneDefault=bar clone parent clone-config &&
> +	(cd clone-config && git rev-parse --verify refs/remotes/bar/master)
> +
> +'
> +
> +test_expect_success 'clone chooses correct remote name' '
> +
> +	git -c remote.cloneDefault=bar clone -o foo parent clone-o-and-config &&
> +	(cd clone-o-and-config && git rev-parse --verify refs/remotes/foo/master)
> +
> +'

These two are "showing off my shiny new toy" tests, which are
needed, but we also need negative tests where the shiny new toy does
not kick in when it should not.  For example

	git -c remote.cloneDefault="bad.../...name" clone parent

should fail, no?

Thanks.