On Fri, 11 Sep 2020 at 13:39:20 -0700, Junio C Hamano 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.
Oh gosh, I'm so sorry! I didn't mean for that to read as blaming, was
just trying to cite you as my source. On a second read, it comes across
more "blame-y" than I originally intended. I'll fix this in v2 (and
have learned a valuable lesson :) ).
> 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".
Great to know! Thanks again for helping a newbie fit in. Will fix in
v2.
> > 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.
To be honest, I'm struggling to decide which route to go. It seems
like multilevel fetch and push refspecs are allowed as far back as
46220ca100 (remote.c: Fix overtight refspec validation, 2008-03-20) or
ef00d150e4 (Tighten refspec processing, 2008-03-17), so perhaps
removing the multilevel check in 8434c2f1 (Build in clone, 2008-04-27)
was intentional? If removing that check in 8434c2f1 was a mistake and
we reintroduce it, that's probably a breaking change for some users.
What sort of accommodations would I need to include in this patchset to
ease that pain for users?
> > + 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?
Absolutely! Happy to include the multilevel check if you think it's the
right path forward (see above).
> > + /* 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.
Agreed. I plan to take Derrick's suggestion [1] and use the same
" is not a valid remote name" message from `builtin/remote.c`, which
should make that translator comment a non-issue.
[1] https://lore.kernel.org/git/bf0107fb-2a6c-68d3-df24-72c6a9df6182@xxxxxxxxx/
I can't stress enough how sorry I am for the improper blame, and how
much I appreciate your help!
--
Sean
