describe: teach --match to handle branches and remotes
PATCH 1/1 describe: teach --match to handle branches and remotes
When git describe uses --match, it matches only tags, basically ignoring the --all argument even when it is specified.
Fix it by also matching branch name and $remote_name/$remote_branch_name, for remote-tracking references, with the specified patterns. Update documentation accordingly and add tests.
---
Requires https://public-inbox.org/git/[email protected]/
This extends --match to branches and remote-tracking references. It is in some respect regression, if anybody have used --all and --match together this would find another reference, but since that combination did not make sense anyway probably it is not a big issue.
There are ambiguity with this approach if --match=foo matches tag "foo", or branch "foo". Probably to resolve it there should appear some --match-full option, so that --match would mean full reference name, with prefix. It could be a room for further improvement.
From documentation I removed the usage example part, mainly to not expand the size too much, but probably they do not really belong there.
Documentation/git-describe.txt | 24 ++++++++++++++---------- builtin/describe.c | 26 ++++++++++++++++++++++---- t/t6120-describe.sh | 18 ++++++++++++++++++ 3 files changed, 54 insertions(+), 14 deletions(-)
Documentation/git-describe.txt +14 -10
@@ -87,19 +87,23 @@ OPTIONS --match <pattern>:: Only consider tags matching the given `glob(7)` pattern, - excluding the "refs/tags/" prefix. This can be used to avoid - leaking private tags from the repository. If given multiple times, a - list of patterns will be accumulated, and tags matching any of the - patterns will be considered. Use `--no-match` to clear and reset the - list of patterns. + excluding the "refs/tags/" prefix. If used with `--all`, it also + considers local branches and remote-tracking references matching the + pattern, excluding respectively "refs/heads/" and "refs/remotes/" + prefix; references of other types are never considered. If given + multiple times, a list of patterns will be accumulated, and tags + matching any of the patterns will be considered. Use `--no-match` to + clear and reset the list of patterns. --exclude <pattern>:: Do not consider tags matching the given `glob(7)` pattern, excluding - the "refs/tags/" prefix. This can be used to narrow the tag space and - find only tags matching some meaningful criteria. If given multiple - times, a list of patterns will be accumulated and tags matching any - of the patterns will be excluded. When combined with --match a tag will - be considered when it matches at least one --match pattern and does not + the "refs/tags/" prefix. If used with `--all`, it also does not consider + local branches and remote-tracking references matching the pattern, + excluding respectively "refs/heads/" and "refs/remotes/" prefix; + references of other types are never considered. If given multiple times, + a list of patterns will be accumulated and tags matching any of the + patterns will be excluded. When combined with --match a tag will be + considered when it matches at least one --match pattern and does not match any of the --exclude patterns. Use `--no-exclude` to clear and reset the list of patterns.
builtin/describe.c +22 -4
@@ -124,6 +124,22 @@ static void add_to_known_names(const char *path, } } +/* Drops prefix. Returns NULL if the path is not expected with current settings. */ +static const char *get_path_to_match (int is_tag,int all,const char *path) +{ +if (is_tag) +return path +10 ; +else if (all) { +if (starts_with (path,"refs/heads/" )) +return path +11 ;/* "refs/heads/..." */ +else if (starts_with (path,"refs/remotes/" )) +return path +13 ;/* "refs/remotes/..." */ +else +return 0 ; + }else +return NULL ; +} +static int get_name (const char *path,const struct object_id *oid,int flag,void *cb_data) {int is_tag =starts_with (path,"refs/tags/" );
@@ -140,12 +156,13 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi*/ if (exclude_patterns.nr) {struct string_list_item *item; +const char *path_to_match =get_path_to_match (is_tag, all, path); -if (!is_tag) +if (!path_to_match)return 0 ;for_each_string_list_item (item, &exclude_patterns) { -if (!wildmatch (item->string, path +10 ,0 )) +if (!wildmatch (item->string, path_to_match,0 ))return 0 ; } }
@@ -156,13 +173,14 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi*/ if (patterns.nr) {int found =0 ; +const char *path_to_match =get_path_to_match (is_tag, all, path);struct string_list_item *item; -if (!is_tag) +if (!path_to_match)return 0 ;for_each_string_list_item (item, &patterns) { -if (!wildmatch (item->string, path +10 ,0 )) { +if (!wildmatch (item->string, path_to_match,0 )) { found =1 ;break ; }
t/t6120-describe.sh +18 -0
@@ -190,6 +190,24 @@ check_describe "test1-lightweight-*" --long --tags --match="test1-*" --match="te check_describe "test1-lightweight-*" --long --tags --match="test3-*" --match="test1-*" HEAD +test_expect_success 'set-up branches' ' + git branch branch_A A && + git branch branch_c c && + git update-ref refs/remotes/origin/remote_branch_A "A^{commit}" && + git update-ref refs/remotes/origin/remote_branch_c "c^{commit}" && + git update-ref refs/original/original_branch_A test-annotated~2 +' + +check_describe "heads/branch_A*" --all --match="branch_*" --exclude="branch_c" HEAD + +check_describe "remotes/origin/remote_branch_A*" --all --match="origin/remote_branch_*" --exclude="origin/remote_branch_c" HEAD + +check_describe "original/original_branch_A*" --all test-annotated~1 + +test_expect_success '--match does not work for other types' ' + test_must_fail git describe --all --match="*original_branch_*" test-annotated~1 +' + test_expect_success 'name-rev with exact tags' ' echo A >expect && tag_object=$(git rev-parse refs/tags/A) &&
Signature · 1 line
2.11.0.1122.gc3fec58.dirty
Quoted 21 lines · Max Kirillov
--match <pattern>::
Documentation/git-describe.txt +7
-9Only consider tags matching the given `glob(7)` pattern, - excluding the "refs/tags/" prefix. This can be used to avoid - leaking private tags from the repository. If given multiple times, a - list of patterns will be accumulated, and tags matching any of the - patterns will be considered. Use `--no-match` to clear and reset the - list of patterns. + excluding the "refs/tags/" prefix. If used with `--all`, it also + considers local branches and remote-tracking references matching the + pattern, excluding respectively "refs/heads/" and "refs/remotes/" + prefix; references of other types are never considered. If given + multiple times, a list of patterns will be accumulated, and tags + matching any of the patterns will be considered. Use `--no-match` to + clear and reset the list of patterns. --exclude <pattern>:: Do not consider tags matching the given `glob(7)` pattern, excluding - the "refs/tags/" prefix. This can be used to narrow the tag space and - find only tags matching some meaningful criteria. If given multiple - times, a list of patterns will be accumulated and tags matching any - of the patterns will be excluded. When combined with --match a tag will
Documentation/git-describe.txt OPTIONS · line 108
Quoted 10 lines
Documentation/git-describe.txt +7
-1- be considered when it matches at least one --match pattern and does not + the "refs/tags/" prefix. If used with `--all`, it also does not consider + local branches and remote-tracking references matching the pattern, + excluding respectively "refs/heads/" and "refs/remotes/" prefix; + references of other types are never considered. If given multiple times, + a list of patterns will be accumulated and tags matching any of the + patterns will be excluded. When combined with --match a tag will be + considered when it matches at least one --match pattern and does not match any of the --exclude patterns. Use `--no-exclude` to clear and reset the list of patterns.
OK, I find this written clearly enough.
builtin/describe.c static void add_to_known_names(const char *path, · line 131
Quoted 13 lines
builtin/describe.c +5
-0@@ -124,6 +124,22 @@ static void add_to_known_names(const char *path, } } +/* Drops prefix. Returns NULL if the path is not expected with current settings. */ +static const char *get_path_to_match (int is_tag,int all,const char *path) +{ +if (is_tag) +return path +10 ;
This is a faithful conversion of the existing code that wants to behave the same as original, but a bit more on this later.
builtin/describe.c static void add_to_known_names(const char *path, · line 138
Quoted 7 lines
builtin/describe.c +7
-0+else if (all) { +if (starts_with (path,"refs/heads/" )) +return path +11 ;/* "refs/heads/..." */ +else if (starts_with (path,"refs/remotes/" )) +return path +13 ;/* "refs/remotes/..." */ +else +return 0 ;
I think you can use skip_prefix() to avoid counting the length of the prefix yourself, i.e.
else if all {
const char *body;
if (skip_prefix(path, "refs/heads/", &body))
return body;
else if (skip_prefix(path, "refs/remotes/", &body))
...
}Whether you do the above or not, the last one that returns 0 should return NULL (to the language it is the same thing, but to humans, we write NULL when it is the null pointer, not the number 0).
builtin/describe.c static void add_to_known_names(const char *path, · line 141
Quoted 3 lines
builtin/describe.c +3
-0+ }else +return NULL ; +}
Perhaps the whole thing may want to be a bit more simplified, like:
static const *skip_ref_prefix(const char *path, int all)
{
const char *prefix[] = {
"refs/tags/", "refs/heads/", "refs/remotes/"
};
const char *body;
int cnt;
int bound = all ? ARRAY_SIZE(prefix) : 1;
for (cnt = 0; cnt < bound; cnt++)
if (skip_prefix(path, prefix[cnt], &body);
return body;
return NULL;
}The hardcoded +10 for "is_tag" case assumes that anything other than "refs/tags/something" would ever be used to call into this function when is_tag is true, and that may well be true in the current code and have been so ever since the original code was written, but it still smells like an invitation for future bugs.
I dunno.
builtin/describe.c static int get_name(const char *path, const struct object_id *oid, int flag, voi · line 163
Quoted 13 lines
builtin/describe.c +5
-0+static int get_name (const char *path,const struct object_id *oid,int flag,void *cb_data) {int is_tag =starts_with (path,"refs/tags/" );@@ -140,12 +156,13 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi*/ if (exclude_patterns.nr) {struct string_list_item *item; +const char *path_to_match =get_path_to_match (is_tag, all, path); +test_expect_success 'set-up branches' ' + git branch branch_A A && + git branch branch_c c &&
Was there a reason why A and c are in different cases? Are we worried about case insensitive filesystems or something?
builtin/describe.c static int get_name(const char *path, const struct object_id *oid, int flag, voi · line 167
Quoted 4 lines
builtin/describe.c +4
-0+ git update-ref refs/remotes/origin/remote_branch_A "A^{commit}" && + git update-ref refs/remotes/origin/remote_branch_c "c^{commit}" && + git update-ref refs/original/original_branch_A test-annotated~2 +'
Thanks.
Quoted 83 lines · Junio C Hamano
Quoted 21 lines · Max Kirillov
--match <pattern>::
Only consider tags matching the givenglob(7)pattern,Documentation/git-describe.txt +7
-9- excluding the "refs/tags/" prefix. This can be used to avoid - leaking private tags from the repository. If given multiple times, a - list of patterns will be accumulated, and tags matching any of the - patterns will be considered. Use `--no-match` to clear and reset the - list of patterns. + excluding the "refs/tags/" prefix. If used with `--all`, it also + considers local branches and remote-tracking references matching the + pattern, excluding respectively "refs/heads/" and "refs/remotes/" + prefix; references of other types are never considered. If given + multiple times, a list of patterns will be accumulated, and tags + matching any of the patterns will be considered. Use `--no-match` to + clear and reset the list of patterns. --exclude <pattern>:: Do not consider tags matching the given `glob(7)` pattern, excluding - the "refs/tags/" prefix. This can be used to narrow the tag space and - find only tags matching some meaningful criteria. If given multiple - times, a list of patterns will be accumulated and tags matching any - of the patterns will be excluded. When combined with --match a tag willDocumentation/git-describe.txt OPTIONS · line 108
Quoted 10 lines
Documentation/git-describe.txt +7
-1- be considered when it matches at least one --match pattern and does not + the "refs/tags/" prefix. If used with `--all`, it also does not consider + local branches and remote-tracking references matching the pattern, + excluding respectively "refs/heads/" and "refs/remotes/" prefix; + references of other types are never considered. If given multiple times, + a list of patterns will be accumulated and tags matching any of the + patterns will be excluded. When combined with --match a tag will be + considered when it matches at least one --match pattern and does not match any of the --exclude patterns. Use `--no-exclude` to clear and reset the list of patterns.OK, I find this written clearly enough.
builtin/describe.c static void add_to_known_names(const char *path, · line 131
Quoted 13 lines
builtin/describe.c +5
-0@@ -124,6 +124,22 @@ static void add_to_known_names(const char *path, } } +/* Drops prefix. Returns NULL if the path is not expected with current settings. */ +static const char *get_path_to_match (int is_tag,int all,const char *path) +{ +if (is_tag) +return path +10 ;This is a faithful conversion of the existing code that wants to behave the same as original, but a bit more on this later.
builtin/describe.c static void add_to_known_names(const char *path, · line 138
Quoted 7 lines
builtin/describe.c +7
-0+else if (all) { +if (starts_with (path,"refs/heads/" )) +return path +11 ;/* "refs/heads/..." */ +else if (starts_with (path,"refs/remotes/" )) +return path +13 ;/* "refs/remotes/..." */ +else +return 0 ;I think you can use
skip_prefix()to avoid counting the length of the prefix yourself, i.e.else if all { const char *body; if (skip_prefix(path, "refs/heads/", &body)) return body; else if (skip_prefix(path, "refs/remotes/", &body)) ... }Whether you do the above or not, the last one that returns 0 should return NULL (to the language it is the same thing, but to humans, we write NULL when it is the null pointer, not the number 0).
builtin/describe.c static void add_to_known_names(const char *path, · line 141
Quoted 3 lines
builtin/describe.c +3
-0+ }else +return NULL ; +}Perhaps the whole thing may want to be a bit more simplified, like:
Quoted 9 lines
static const *skip_ref_prefix(const char *path, int all)
{
const char *prefix[] = {
"refs/tags/", "refs/heads/", "refs/remotes/"
};
const char *body;
int cnt;
int bound = all ? ARRAY_SIZE(prefix) : 1;I found the implicit use of "bound = 1" means "we only care about tags" to be a bit weird here. I guess it's not really that big a deal overall, and this is definitely cleaner than the original implementation.
Quoted 37 lines
for (cnt = 0; cnt < bound; cnt++) if (skip_prefix(path, prefix[cnt], &body); return body; return NULL; }The hardcoded +10 for "
is_tag" case assumes that anything other than "refs/tags/something" would ever be used to call into this function whenis_tagis true, and that may well be true in the current code and have been so ever since the original code was written, but it still smells like an invitation for future bugs.I dunno.
builtin/describe.c static int get_name(const char *path, const struct object_id *oid, int flag, voi · line 163
Quoted 13 lines
builtin/describe.c +5
-0+static int get_name (const char *path,const struct object_id *oid,int flag,void *cb_data) {int is_tag =starts_with (path,"refs/tags/" );@@ -140,12 +156,13 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi*/ if (exclude_patterns.nr) {struct string_list_item *item; +const char *path_to_match =get_path_to_match (is_tag, all, path); +test_expect_success 'set-up branches' ' + git branch branch_A A && + git branch branch_c c &&Was there a reason why A and c are in different cases? Are we worried about case insensitive filesystems or something?
builtin/describe.c static int get_name(const char *path, const struct object_id *oid, int flag, voi · line 167
Quoted 4 lines
builtin/describe.c +4
-0+ git update-ref refs/remotes/origin/remote_branch_A "A^{commit}" && + git update-ref refs/remotes/origin/remote_branch_c "c^{commit}" && + git update-ref refs/original/original_branch_A test-annotated~2 +'Thanks.
Quoted 2 lines · Junio C Hamano
I think you can use
skip_prefix()to avoid counting the length of the prefix yourself, i.e.
Thanks, will use it.
Quoted 5 lines
The hardcoded +10 for "
is_tag" case assumes that anything other than "refs/tags/something" would ever be used to call into this function whenis_tagis true, and that may well be true in the current code and have been so ever since the original code was written, but it still smells like an invitation for future bugs.
is_tag is used later. I'll chance it so that it does not rely on it to match, but it still has to produce it.
Quoted 2 lines
Was there a reason why A and c are in different cases? Are we worried about case insensitive filesystems or something?
The tags have been there of different case already. I don't know why. I'll change the branch names but I'm reluctant to touch existing tests.
Signature · 1 line
Max
PATCH v2 1/1 describe: teach --match to handle branches and remotes
When git describe uses --match, it matches only tags, basically ignoring the --all argument even when it is specified.
Fix it by also matching branch name and $remote_name/$remote_branch_name, for remote-tracking references, with the specified patterns. Update documentation accordingly and add tests.
---
Changed to use skip_prefix(). Calculate path_to_match only once.
Add case of discarding unknown type with exclude
Documentation/git-describe.txt | 24 ++++++++++++++---------- builtin/describe.c | 29 +++++++++++++++++------------ t/t6120-describe.sh | 27 +++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 22 deletions(-)
Documentation/git-describe.txt +14 -10
@@ -87,19 +87,23 @@ OPTIONS --match <pattern>:: Only consider tags matching the given `glob(7)` pattern, - excluding the "refs/tags/" prefix. This can be used to avoid - leaking private tags from the repository. If given multiple times, a - list of patterns will be accumulated, and tags matching any of the - patterns will be considered. Use `--no-match` to clear and reset the - list of patterns. + excluding the "refs/tags/" prefix. If used with `--all`, it also + considers local branches and remote-tracking references matching the + pattern, excluding respectively "refs/heads/" and "refs/remotes/" + prefix; references of other types are never considered. If given + multiple times, a list of patterns will be accumulated, and tags + matching any of the patterns will be considered. Use `--no-match` to + clear and reset the list of patterns. --exclude <pattern>:: Do not consider tags matching the given `glob(7)` pattern, excluding - the "refs/tags/" prefix. This can be used to narrow the tag space and - find only tags matching some meaningful criteria. If given multiple - times, a list of patterns will be accumulated and tags matching any - of the patterns will be excluded. When combined with --match a tag will - be considered when it matches at least one --match pattern and does not + the "refs/tags/" prefix. If used with `--all`, it also does not consider + local branches and remote-tracking references matching the pattern, + excluding respectively "refs/heads/" and "refs/remotes/" prefix; + references of other types are never considered. If given multiple times, + a list of patterns will be accumulated and tags matching any of the + patterns will be excluded. When combined with --match a tag will be + considered when it matches at least one --match pattern and does not match any of the --exclude patterns. Use `--no-exclude` to clear and reset the list of patterns.
builtin/describe.c +17 -12
@@ -129,13 +129,24 @@ static void add_to_known_names(const char *path,static int get_name (const char *path,const struct object_id *oid,int flag,void *cb_data) { -int is_tag =starts_with (path,"refs/tags/" ); +int is_tag =0 ;struct object_id peeled;int is_annotated, prio; - -/* Reject anything outside refs/tags/ unless --all */ -if (!all && !is_tag) +const char *path_to_match =NULL ; + +if (skip_prefix (path,"refs/tags/" , &path_to_match)) { + is_tag =1 ; + }else if (all) { +if ((exclude_patterns.nr || patterns.nr) && + !skip_prefix (path,"refs/heads/" , &path_to_match) && + !skip_prefix (path,"refs/remotes/" , &path_to_match)) { +/* Only accept reference of known type if there are match/exclude patterns */ +return 0 ; + } + }else { +/* Reject anything outside refs/tags/ unless --all */ return 0 ; + }/* * If we're given exclude patterns, first exclude any tag which match
@@ -144,11 +155,8 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voiif (exclude_patterns.nr) {struct string_list_item *item; -if (!is_tag) -return 0 ; -for_each_string_list_item (item, &exclude_patterns) { -if (!wildmatch (item->string, path +10 ,0 )) +if (!wildmatch (item->string, path_to_match,0 ))return 0 ; } }
@@ -161,11 +169,8 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voiint found =0 ;struct string_list_item *item; -if (!is_tag) -return 0 ; -for_each_string_list_item (item, &patterns) { -if (!wildmatch (item->string, path +10 ,0 )) { +if (!wildmatch (item->string, path_to_match,0 )) { found =1 ;break ; }
t/t6120-describe.sh +27 -0
@@ -190,6 +190,33 @@ check_describe "test1-lightweight-*" --long --tags --match="test1-*" --match="te check_describe "test1-lightweight-*" --long --tags --match="test3-*" --match="test1-*" HEAD +test_expect_success 'set-up branches' ' + git branch branch_A A && + git branch branch_C c && + git update-ref refs/remotes/origin/remote_branch_A "A^{commit}" && + git update-ref refs/remotes/origin/remote_branch_C "c^{commit}" && + git update-ref refs/original/original_branch_A test-annotated~2 +' + +check_describe "heads/branch_A*" --all --match="branch_*" --exclude="branch_C" HEAD + +check_describe "remotes/origin/remote_branch_A*" --all --match="origin/remote_branch_*" --exclude="origin/remote_branch_C" HEAD + +check_describe "original/original_branch_A*" --all test-annotated~1 + +test_expect_success '--match does not work for other types' ' + test_must_fail git describe --all --match="*original_branch_*" test-annotated~1 +' + +test_expect_success '--exclude does not work for other types' ' + R=$(git describe --all --exclude="any_pattern_even_not_matching" test-annotated~1) && + case "$R" in + *original_branch_A*) echo "fail: Found unknown reference $R with --exclude" + false;; + *) echo ok: Found some known type;; + esac +' + test_expect_success 'name-rev with exact tags' ' echo A >expect && tag_object=$(git rev-parse refs/tags/A) &&
Signature · 1 line
2.11.0.1122.gc3fec58.dirty