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

Junio C Hamano [off-list ref] writes:

Will squash the changes; no need to resend (unless people discover
other issues; let's hope that I wouldn't be the one to do so ;-).

Thanks.

quoted

diff --git i/t/t5511-refspec.sh w/t/t5511-refspec.sh
index de6db86ccff0..7bfca7962d41 100755
--- i/t/t5511-refspec.sh
+++ w/t/t5511-refspec.sh

@@ -71,11 +71,11 @@ test_refspec fetch ':refs/remotes/frotz/HEAD-to-me'

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.

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

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

Or it may not be worth the additional complexity.


 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/*'
 

On Thu, Jul 23, 2015 at 10:13 AM, Junio C Hamano [off-list ref] wrote:

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

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.

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