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

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

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

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.

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.

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.

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.