Re: [PATCH 3/4] clone: validate --origin option before use

From: Derrick Stolee <stolee@gmail.com>
Date: Sep 11, 2020, 19:24 UTC
Message-ID: <bf0107fb-2a6c-68d3-df24-72c6a9df6182@gmail.com>
In-Reply-To: <0dff8cd66930130ffd5f0d7d068ad3ed47cd1c81.1599848727.git.gitgitgadget@gmail.com>

On 9/11/2020 2:25 PM, Sean Barag via GitGitGadget wrote:

> +	strbuf_addf(&resolved_refspec, "refs/heads/test:refs/remotes/%s/test", option_origin);
> +	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);

Looking at this again, I'm not sure the translators note is necessary. Also, I would say "is not a valid remote name". That makes the string align with the already-translated string in builtin/remote.c.

This code is duplicated from builtin/remote.c, so I'd rather see this be a helper method in refspec.c and have both builtin/clone.c and builtin/remote.c call that helper.

Here is the helper:

void valid_remote_name(const char *name)
{
	int result;
	struct strbuf refspec = STRBUF_INIT;
	strbuf_addf(&refspec, "refs/heads/test:refs/remotes/%s/test", name);
	result = valid_fetch_refspec(refspec.buf);
	strbuf_release(&refspec);
	return result;
}

And here is the use in builtin/clone.c:

	if (!valid_remote_name(option_origin))
		die(_("'%s' is not a valid remote name"), option_origin);

and in builtin/remote.c:

	if (!valid_remote_name(name))
		die(_("'%s' is not a valid remote name"), name);

Show 8 quoted lines

> +test_expect_success 'rejects invalid -o/--origin' '
> +
> +	test_expect_code 128 git clone -o "bad...name" parent clone-bad-name 2>err &&
> +	test_debug "cat err" &&
> +	test_i18ngrep "'\''bad...name'\'' is not a valid origin name" err
> +
> +'
> +

Double newlines here! I personally appreciate newlines to spread out content, but it doesn't fit our guidelines.

Thanks, -Stolee