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