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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 18, 2017, 23:52 UTC
Message-ID: <xmqqzi9rsgxz.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170917142416.30685-1-max@max630.net>

Max Kirillov <max@max630.net> writes:

Show 31 quoted lines

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

Show 13 quoted lines

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

Show 7 quoted lines

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

Show 9 quoted lines

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