Re: [PATCH] describe: teach --match to handle branches and remotes
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
Re: [PATCH] describe: teach --match to handle branches and remotes
On Mon, Sep 18, 2017 at 4:52 PM, Junio C Hamano wrote:
> Max Kirillov writes:
>
>> --match ::
>> 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 ::
>> 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 ch
Re: [PATCH] describe: teach --match to handle branches and remotes
Max Kirillov writes:
> --match ::
> 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 ::
> 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);
> +
[PATCH] 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.
Signed-off-by: Max Kirillov
---
Requires https://public-inbox.org/git/20170916055344.31866-1-...@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 ::
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 ::
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_p
