"Sean Barag via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Sean Barag <sean@xxxxxxxxx>
>
> Providing a bad origin name to `git clone` currently reports an
> 'invalid refspec' error instead of a more explicit message explaining
> that the `--origin` option was malformed. Per Junio, it's been this way
> since 8434c2f1 (Build in clone, 2008-04-27).
If you know it as a fact that it has been this way since the first
version of rewritten "git clone", don't blame others.
A micronit. We describe the current status in present tense when
presenting the problem to be solved, so "currently" can be dropped.
> This patch reintroduces
When presenting the solution, we write as if we are giving an order
to a patch monkey to "make the code like so".
"This patch reintroduces" -> "Reintroduce".
> validation for the provided `--origin` option, but notably _doesn't_
> include a multi-level check (e.g. "foo/bar") that was present in the
> original `git-clone.sh`. It seems `git remote` allows multi-level
> remote names, so applying that same validation in `git clone` seems
> reasonable.
Even though I suspect "git remote" is being overly loose and
careless here, I am not sure if it is a good idea to tighten it
retroactively. But if this is meant as a bugfix for misconversion
made in 8434c2f1, we should be as strict as the original. I dunno.
> + strbuf_addf(&resolved_refspec, "refs/heads/test:refs/remotes/%s/test", option_origin);
> + if (!valid_fetch_refspec(resolved_refspec.buf))
If we reintroduce pre-8434c2f1 check, then we would want
if (!valid_fetch_refspec(...) || strchr(option_origin, '/'))
or something like that?
> + /* TRANSLATORS: %s will be the user-provided --origin / -o option */
> + die(_("'%s' is not a valid origin name"), option_origin);
I am not sure if this translator comment is helping.
The message makes it sound as if "%s" here is used to switch between
'-o' or '--origin'. If it said "'%s' will be the value given to
--origin/-o option", it would become much less confusing.
> + strbuf_release(&resolved_refspec);
> +
> repo_name = argv[0];
>
> path = get_repo_path(repo_name, &is_bundle);
> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> index d20a78f84b..c865f96def 100755
> --- a/t/t5606-clone-options.sh
> +++ b/t/t5606-clone-options.sh
> @@ -19,6 +19,14 @@ test_expect_success 'clone -o' '
>
> '
>
> +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
> +
> +'
... and we can also test for "bad/name" here in another test.
> test_expect_success 'disallows --bare with --origin' '
>
> test_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&
Thanks.
