Re: [PATCH 2/2] refs: loosen restriction on wildcard * refspecs
Eric Sunshine sunsh...@sunshineco.com writes:
On Wed, Jul 22, 2015 at 5:05 PM, Jacob Keller jacob.e.kel...@intel.com
wrote:
From: Jacob Keller jacob.kel...@gmail.com
Modify logic of check_refname_component and add a new disposition
regarding *. Allow refspecs that contain a single * if
REFNAME_REFSPEC_PATTERN is set. Change the function to pass the flags as
a pointer, and clear REFNAME_REFSPEC_PATTERN after the first * so that
only a single * is accepted.
This loosens restrictions on refspecs by allowing patterns that have
a * within a component instead of only as the whole component. Also
remove the code that hangled refspec patterns in check_refname_format
s/hangled/handled/
...
Thanks; here is what I queued yesterday.
-- 8 --
From: Jacob Keller jacob.kel...@gmail.com
Date: Wed, 22 Jul 2015 14:05:33 -0700
Subject: [PATCH] refs: loosen restriction on wildcard * refspecs
Loosen restrictions on refspecs by allowing patterns that have a *
within a component instead of only as the whole component.
Remove the logic to accept a single * as a whole component from
check_refname_format(), and implement an extended form of that logic
in check_refname_component(). Pass the pointer to the flags argument
to the latter, as it has to clear REFNAME_REFSPEC_PATTERN bit when
it sees *.
Teach check_refname_component() function to allow an asterisk *
only when REFNAME_REFSPEC_PATTERN is set in the flags, and drop the
bit after seeing a *, to ensure that one side of a refspec
contains at most one asterisk.
This will allow us to accept refspecs such as `for/bar*:foo/baz*`.
Any refspec which functioned before shall continue functioning with
the new logic.
Signed-off-by: Jacob Keller jacob.kel...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
Documentation/git-check-ref-format.txt | 4 ++--
refs.c | 36 +++---
refs.h | 4 ++--
t/t1402-check-ref-format.sh| 8 +---
t/t5511-refspec.sh | 11 +++
5 files changed, 36 insertions(+), 27 deletions(-)
diff --git a/Documentation/git-check-ref-format.txt
b/Documentation/git-check-ref-format.txt
index fc02959..9044dfa 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -94,8 +94,8 @@ OPTIONS
Interpret refname as a reference name pattern for a refspec
(as used with remote repositories). If this option is
enabled, refname is allowed to contain a single `*`
- in place of a one full pathname component (e.g.,
- `foo/*/bar` but not `foo/bar*`).
+ in the refspec (e.g., `foo/bar*/baz` or `foo/bar*baz/`
+ but not `foo/bar*/baz*`).
--normalize::
Normalize 'refname' by removing any leading slash (`/`)
diff --git a/refs.c b/refs.c
index 0900f54..3127518 100644
--- a/refs.c
+++ b/refs.c
@@ -21,12 +21,13 @@ struct ref_lock {
* 2: ., look for a preceding . to reject .. in refs
* 3: {, look for a preceding @ to reject @{ in refs
* 4: A bad character: ASCII control characters, and
- **, :, ?, [, \, ^, ~, SP, or TAB
+ *:, ?, [, \, ^, ~, SP, or TAB
+ * 5: *, reject unless REFNAME_REFSPEC_PATTERN is set
*/
static unsigned char refname_disposition[256] = {
1, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4,
4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4,
- 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 2, 1,
+ 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 5, 0, 0, 0, 2, 1,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 0, 4,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 4, 0, 4, 0,
@@ -73,12 +74,13 @@ static unsigned char refname_disposition[256] = {
* - any path component of it begins with ., or
* - it has double dots .., or
* - it has ASCII control characters, or
- * - it has *, :, ?, [, \, ^, ~, SP, or TAB anywhere, or
+ * - it has :, ?, [, \, ^, ~, SP, or TAB anywhere, or
+ * - it has * anywhere unless REFNAME_REFSPEC_PATTERN is set, or
* - it ends with a /, or
* - it ends with .lock, or
* - it contains a @{ portion
*/
-static int check_refname_component(const char *refname, int flags)
+static int check_refname_component(const char *refname, int *flags)
{
const char *cp;
char last = '\0';
@@ -99,6 +101,16 @@ static int check_refname_component(const char *refname, int
flags)
break;
case 4:
return -1;
+ case 5:
+ if (!(*flags REFNAME_REFSPEC_PATTERN))
+ return -1; /* refspec can't be a pattern */
+
+ /*
+* Unset the pattern flag so that we only accept
+* a single asterisk for one side of refspec.
+*/
+ *flags = ~ REFNAME_REFSPEC_PATTERN;
+ break;
Re: [PATCH 2/2] refs: loosen restriction on wildcard * refspecs
On Thu, 2015-07-23 at 15:12 -0700, Junio C Hamano wrote:
Eric Sunshine sunsh...@sunshineco.com writes:
On Wed, Jul 22, 2015 at 5:05 PM, Jacob Keller
jacob.e.kel...@intel.com wrote:
From: Jacob Keller jacob.kel...@gmail.com
Modify logic of check_refname_component and add a new disposition
regarding *. Allow refspecs that contain a single * if
REFNAME_REFSPEC_PATTERN is set. Change the function to pass the
flags as
a pointer, and clear REFNAME_REFSPEC_PATTERN after the first *
so that
only a single * is accepted.
This loosens restrictions on refspecs by allowing patterns that
have
a * within a component instead of only as the whole component.
Also
remove the code that hangled refspec patterns in
check_refname_format
s/hangled/handled/
...
Thanks; here is what I queued yesterday.
Woah. I bow to your imperative commit message abilities. The new commit
message is very nicely worded. Thanks for taking the time to reword my
jumble correctly.
Everything looked great to me.
Regards,
JakeN�r��yb�X��ǧv�^�){.n�+ا���ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf
Re: [PATCH 2/2] refs: loosen restriction on wildcard * refspecs
Keller, Jacob E jacob.e.kel...@intel.com writes: s/hangled/handled/ ... Thanks; here is what I queued yesterday. Woah. I bow to your imperative commit message abilities. The new commit message is very nicely worded. Heh, imperative is secondary. I just wanted to straighten out the use of refs and refspecs there. Thanks for a patch that was cleanly done. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] refs: loosen restriction on wildcard * refspecs
On Wed, Jul 22, 2015 at 5:05 PM, Jacob Keller jacob.e.kel...@intel.com wrote: From: Jacob Keller jacob.kel...@gmail.com Modify logic of check_refname_component and add a new disposition regarding *. Allow refspecs that contain a single * if REFNAME_REFSPEC_PATTERN is set. Change the function to pass the flags as a pointer, and clear REFNAME_REFSPEC_PATTERN after the first * so that only a single * is accepted. This loosens restrictions on refspecs by allowing patterns that have a * within a component instead of only as the whole component. Also remove the code that hangled refspec patterns in check_refname_format s/hangled/handled/ since it is now handled via the check_refname_component logic. Now refs such as `for/bar*:foo/bar*` and even `foo/bar*:foo/baz*` will be accepted. This allows users more control over what is fetched where. Since users must modify the default by hand to make use of this functionality it is not considered a large risk. Any refspec which functioned before shall continue functioning with the new logic. Signed-off-by: Jacob Keller jacob.kel...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] refs: loosen restriction on wildcard * refspecs
Junio C Hamano gits...@pobox.com 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. 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/*' -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] refs: loosen restriction on wildcard * refspecs
On Thu, Jul 23, 2015 at 10:13 AM, Junio C Hamano gits...@pobox.com 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 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/*' -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] refs: loosen restriction on wildcard * refspecs
On Thu, Jul 23, 2015 at 9:44 AM, Junio C Hamano gits...@pobox.com wrote: Jacob Keller jacob.kel...@gmail.com writes: By the way, have you run test suite before sending this (or any previous round of this) patch? This seems to break t5511-refspec.sh for me. Looks like another location I forgot to update. I can send a re-spin if you need with the following diff. Basically looks like the tests just didn't get updated to count the new behavior is valid. Yeah, basically looks like an untested patch was sent and nobody noticed during earlier rounds, even the patch was rerolled a few times, before I finally took it to 'pu' to take a look. A typical slow summer moment---people rightfully find it is more important to have fun themselves than to help polishing others' patches ;-). I think what happened, is that I ran some tests when the patch was configurable and I had modified the one set of tests to try with the option enabled, but it didn't fail in the t5511-refspec.sh since this series of tests didn't enable the new option. Then, I never re-tested again (OOPS!) when I removed the optional portion. 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. Thank you! :) Regards, Jake 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' 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 Regards, Jake -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] refs: loosen restriction on wildcard * refspecs
Jacob Keller jacob.kel...@gmail.com writes: By the way, have you run test suite before sending this (or any previous round of this) patch? This seems to break t5511-refspec.sh for me. Looks like another location I forgot to update. I can send a re-spin if you need with the following diff. Basically looks like the tests just didn't get updated to count the new behavior is valid. Yeah, basically looks like an untested patch was sent and nobody noticed during earlier rounds, even the patch was rerolled a few times, before I finally took it to 'pu' to take a look. A typical slow summer moment---people rightfully find it is more important to have fun themselves than to help polishing others' patches ;-). 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. 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' 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 Regards, Jake -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] refs: loosen restriction on wildcard * refspecs
Jacob Keller jacob.e.kel...@intel.com writes: From: Jacob Keller jacob.kel...@gmail.com Modify logic of check_refname_component and add a new disposition regarding *. Allow refspecs that contain a single * if REFNAME_REFSPEC_PATTERN is set. Change the function to pass the flags as a pointer, and clear REFNAME_REFSPEC_PATTERN after the first * so that only a single * is accepted. This loosens restrictions on refspecs by allowing patterns that have a * within a component instead of only as the whole component. Also remove the code that hangled refspec patterns in check_refname_format since it is now handled via the check_refname_component logic. Now refs such as `for/bar*:foo/bar*` and even `foo/bar*:foo/baz*` will be accepted. This allows users more control over what is fetched where. Since users must modify the default by hand to make use of this functionality it is not considered a large risk. Any refspec which functioned before shall continue functioning with the new logic. Thanks. Now I can read the changes to the code in these two commits and see that they both make sense ;-) The above description seem to use ref and refspec rather liberally, so I'll rewrite some parts of it to clarify while queuing. By the way, have you run test suite before sending this (or any previous round of this) patch? This seems to break t5511-refspec.sh for me. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] refs: loosen restriction on wildcard * refspecs
From: Jacob Keller jacob.kel...@gmail.com
Modify logic of check_refname_component and add a new disposition
regarding *. Allow refspecs that contain a single * if
REFNAME_REFSPEC_PATTERN is set. Change the function to pass the flags as
a pointer, and clear REFNAME_REFSPEC_PATTERN after the first * so that
only a single * is accepted.
This loosens restrictions on refspecs by allowing patterns that have
a * within a component instead of only as the whole component. Also
remove the code that hangled refspec patterns in check_refname_format
since it is now handled via the check_refname_component logic.
Now refs such as `for/bar*:foo/bar*` and even `foo/bar*:foo/baz*` will
be accepted. This allows users more control over what is fetched where.
Since users must modify the default by hand to make use of this
functionality it is not considered a large risk. Any refspec which
functioned before shall continue functioning with the new logic.
Signed-off-by: Jacob Keller jacob.kel...@gmail.com
Cc: Daniel Barkalow barka...@iabervon.iabervon.org
Cc: Junio C Hamano gits...@pobox.com
---
Documentation/git-check-ref-format.txt | 4 ++--
refs.c | 36 +++---
refs.h | 4 ++--
t/t1402-check-ref-format.sh| 8 +---
4 files changed, 29 insertions(+), 23 deletions(-)
diff --git a/Documentation/git-check-ref-format.txt
b/Documentation/git-check-ref-format.txt
index fc02959ba4ab..9044dfaadae1 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -94,8 +94,8 @@ OPTIONS
Interpret refname as a reference name pattern for a refspec
(as used with remote repositories). If this option is
enabled, refname is allowed to contain a single `*`
- in place of a one full pathname component (e.g.,
- `foo/*/bar` but not `foo/bar*`).
+ in the refspec (e.g., `foo/bar*/baz` or `foo/bar*baz/`
+ but not `foo/bar*/baz*`).
--normalize::
Normalize 'refname' by removing any leading slash (`/`)
diff --git a/refs.c b/refs.c
index 8c08fc0489eb..16a1d8305579 100644
--- a/refs.c
+++ b/refs.c
@@ -20,12 +20,13 @@ struct ref_lock {
* 2: ., look for a preceding . to reject .. in refs
* 3: {, look for a preceding @ to reject @{ in refs
* 4: A bad character: ASCII control characters, and
- **, :, ?, [, \, ^, ~, SP, or TAB
+ *:, ?, [, \, ^, ~, SP, or TAB
+ * 5: *, reject unless REFNAME_REFSPEC_PATTERN is set
*/
static unsigned char refname_disposition[256] = {
1, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4,
4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4,
- 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 2, 1,
+ 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 5, 0, 0, 0, 2, 1,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 0, 4,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 4, 0, 4, 0,
@@ -72,12 +73,13 @@ static unsigned char refname_disposition[256] = {
* - any path component of it begins with ., or
* - it has double dots .., or
* - it has ASCII control characters, or
- * - it has *, :, ?, [, \, ^, ~, SP, or TAB anywhere, or
+ * - it has :, ?, [, \, ^, ~, SP, or TAB anywhere, or
+ * - it has * anywhere unless REFNAME_REFSPEC_PATTERN is set, or
* - it ends with a /, or
* - it ends with .lock, or
* - it contains a @{ portion
*/
-static int check_refname_component(const char *refname, int flags)
+static int check_refname_component(const char *refname, int *flags)
{
const char *cp;
char last = '\0';
@@ -98,6 +100,16 @@ static int check_refname_component(const char *refname, int
flags)
break;
case 4:
return -1;
+ case 5:
+ if (!(*flags REFNAME_REFSPEC_PATTERN))
+ return -1; /* refspec can't be a pattern */
+
+ /*
+* Unset the pattern flag so that we only accept a
single glob for
+* the entire refspec.
+*/
+ *flags = ~ REFNAME_REFSPEC_PATTERN;
+ break;
}
last = ch;
}
@@ -122,18 +134,10 @@ int check_refname_format(const char *refname, int flags)
while (1) {
/* We are at the start of a path component. */
- component_len = check_refname_component(refname, flags);
- if (component_len = 0) {
- if ((flags REFNAME_REFSPEC_PATTERN)
- refname[0] == '*'
- (refname[1] == '\0' || refname[1] ==
'/')) {
- /* Accept one wildcard as a full refname
component. */
- flags = ~REFNAME_REFSPEC_PATTERN;
- component_len = 1;
-
Re: [PATCH 2/2] refs: loosen restriction on wildcard * refspecs
On Wed, Jul 22, 2015 at 3:04 PM, Junio C Hamano gits...@pobox.com wrote: Jacob Keller jacob.e.kel...@intel.com writes: From: Jacob Keller jacob.kel...@gmail.com Modify logic of check_refname_component and add a new disposition regarding *. Allow refspecs that contain a single * if REFNAME_REFSPEC_PATTERN is set. Change the function to pass the flags as a pointer, and clear REFNAME_REFSPEC_PATTERN after the first * so that only a single * is accepted. This loosens restrictions on refspecs by allowing patterns that have a * within a component instead of only as the whole component. Also remove the code that hangled refspec patterns in check_refname_format since it is now handled via the check_refname_component logic. Now refs such as `for/bar*:foo/bar*` and even `foo/bar*:foo/baz*` will be accepted. This allows users more control over what is fetched where. Since users must modify the default by hand to make use of this functionality it is not considered a large risk. Any refspec which functioned before shall continue functioning with the new logic. Thanks. Now I can read the changes to the code in these two commits and see that they both make sense ;-) The above description seem to use ref and refspec rather liberally, so I'll rewrite some parts of it to clarify while queuing. By the way, have you run test suite before sending this (or any previous round of this) patch? This seems to break t5511-refspec.sh for me. Looks like another location I forgot to update. I can send a re-spin if you need with the following diff. Basically looks like the tests just didn't get updated to count the new behavior is valid. 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' 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 Regards, Jake -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
