Max Kirillov [off-list ref] writes:
--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.
OK, I find this written clearly enough.
quoted hunk
diff --git a/builtin/describe.c b/builtin/describe.c
index 94ff2fba0b..2a2e998063 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -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.
+ 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).
+ } 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.
quoted hunk
+
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?
+ 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.
On Mon, Sep 18, 2017 at 4:52 PM, Junio C Hamano [off-list ref] wrote:
Max Kirillov [off-list ref] writes:
quoted
--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.
OK, I find this written clearly enough.
quoted
diff --git a/builtin/describe.c b/builtin/describe.c
index 94ff2fba0b..2a2e998063 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -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.
quoted
+ 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).
quoted
+ } 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;
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.
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.
quoted
+
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);
quoted
+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?
quoted
+ 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.
On Tue, Sep 19, 2017 at 08:52:24AM +0900, Junio C Hamano wrote:
I think you can use skip_prefix() to avoid counting the length of
the prefix yourself, i.e.
Thanks, will use it.
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.
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.
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.
--
Max
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.
Signed-off-by: Max Kirillov <redacted>
---
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(-)
diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index 26f19d3b07..c924c945ba 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -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.
diff --git a/builtin/describe.c b/builtin/describe.c
index 42afa1e244..f15b6e531d 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -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, voi
if (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, voi
int 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;
}diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 25110ea55d..0a8f754100 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -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) &&--
2.11.0.1122.gc3fec58.dirty