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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 29, 2020, 19:59 UTC
Message-ID: <xmqqa6x8icbp.fsf@gitster.c.googlers.com>
In-Reply-To: <737f91c6244220eec196c327fcea9a6548c45310.1601350615.git.gitgitgadget@gmail.com>

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

Show 6 quoted lines

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

Show 7 quoted lines

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

Show 8 quoted lines

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