[PATCH] describe: teach --match to handle branches and remotes

[PATCH] describe: teach --match to handle branches and remotes

[Max Kirillov]

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 <max@max630.net>
---
Requires https://public-inbox.org/git/20170916055344.31866-1-max@max630.net/

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(-)

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 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;
+	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;
 			}
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 25110ea55d..fac52bd9dc 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -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) &&
-- 
2.11.0.1122.gc3fec58.dirty

Re: [PATCH] describe: teach --match to handle branches and remotes

[Junio C Hamano]

Max Kirillov <max@max630.net> 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.

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

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

Re: [PATCH] describe: teach --match to handle branches and remotes

[Jacob Keller]

On Mon, Sep 18, 2017 at 4:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Max Kirillov <max@max630.net> 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.
>
>> 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;
>

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

Re: [PATCH] describe: teach --match to handle branches and remotes

[Max Kirillov]

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

[PATCH v2] describe: teach --match to handle branches and remotes

[Max Kirillov]

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 <max@max630.net>
---
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