Re: [PATCH 2/2] refs: loosen restriction on wildcard "*" refspecs

From: Jacob Keller <jacob.keller@gmail.com>
Date: Jul 23, 2015, 17:18 UTC
Message-ID: <CA+P7+xq1BYg6xE3pWJbXadEPbm2tcyRhOoy9-Scsx7Bj2krJqw@mail.gmail.com>
In-Reply-To: <xmqq1tfyre46.fsf@gitster.dls.corp.google.com>

On Thu, Jul 23, 2015 at 10:13 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

> That was whitespace damaged, so I had to hand-tweak the file in
> place.  While at it, I noticed that we do not check a case where
> multiple asterisks appear in a single component (which is rejected
> for a reason different from having multiple components with an
> asterisk in them), which also deserves its own test.
>

Oops. Sorry about that..

Show 9 quoted lines

> I'll squash in the following instead.
>
> There is a slightly related tangent I noticed while doing so.
>
> I wonder if there is an obvious and unambiguous interpretation of
> what this command line wants to do:
>
>     $ git fetch origin refs/heads/*g*/for-linus:refs/remotes/i-*/mine
>

Personally, I do think this is "obvious" but I don't think that our parser is at all smart enough to handle it. The advantage with the current code, is that the match parser already handles it perfectly. It was just the rules up front which limited how much we could do. I don't think the added complexity is that valuable here.. For most common cases it's just prefixes or suffixes that matter.

Show 5 quoted lines

> We _might_ want to allow one (and only one) component with more than
> one asterisk on the LHS of a refspec, while requiring only one
> asterisk on the RHS to allow "this '*g*' is just like '*' but
> excluding things that do not have 'g' in it".
>

As above, I think it's obvious the intended meaning, but... the actual interpretation could be a variety of things. It's not guaranteed to be interpreted in that way, and while we could document it, I don't think that it is worth the complexity unless someone actually needs this behavior.

> Or it may not be worth the additional complexity.
>

Below diff looks fine, thanks!

Regards, Jake

Show 31 quoted lines

>
>  t/t5511-refspec.sh | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/t/t5511-refspec.sh b/t/t5511-refspec.sh
> index de6db86..f541f30 100755
> --- a/t/t5511-refspec.sh
> +++ b/t/t5511-refspec.sh
> @@ -71,15 +71,18 @@ test_refspec fetch ':refs/remotes/frotz/HEAD-to-me'
>  test_refspec push ':refs/remotes/frotz/delete me'              invalid
>  test_refspec fetch ':refs/remotes/frotz/HEAD to me'            invalid
>
> -test_refspec fetch 'refs/heads/*/for-linus:refs/remotes/mine/*-blah' invalid
> -test_refspec push 'refs/heads/*/for-linus:refs/remotes/mine/*-blah' invalid
> +test_refspec fetch 'refs/heads/*/for-linus:refs/remotes/mine/*-blah'
> +test_refspec push 'refs/heads/*/for-linus:refs/remotes/mine/*-blah'
>
> -test_refspec fetch 'refs/heads*/for-linus:refs/remotes/mine/*' invalid
> -test_refspec push 'refs/heads*/for-linus:refs/remotes/mine/*' invalid
> +test_refspec fetch 'refs/heads*/for-linus:refs/remotes/mine/*'
> +test_refspec push 'refs/heads*/for-linus:refs/remotes/mine/*'
>
>  test_refspec fetch 'refs/heads/*/*/for-linus:refs/remotes/mine/*' invalid
>  test_refspec push 'refs/heads/*/*/for-linus:refs/remotes/mine/*' invalid
>
> +test_refspec fetch 'refs/heads/*g*/for-linus:refs/remotes/mine/*' invalid
> +test_refspec push 'refs/heads/*g*/for-linus:refs/remotes/mine/*' invalid
> +
>  test_refspec fetch 'refs/heads/*/for-linus:refs/remotes/mine/*'
>  test_refspec push 'refs/heads/*/for-linus:refs/remotes/mine/*'
>