[PATCH 1/2] name-rev: allow passing multiple patterns using --refs

[PATCH 1/2] name-rev: allow passing multiple patterns using --refs

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

Allow git name-rev to take a string list of patterns instead of only
a single pattern. All patterns are matched inclusively, meaning that
a ref only needs to match one pattern to be included.

Add tests and documentation for this change.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/git-name-rev.txt       |  4 +++-
 builtin/name-rev.c                   | 41 +++++++++++++++++++++++++-----------
 t/t6007-rev-list-cherry-pick-file.sh | 30 ++++++++++++++++++++++++++
 3 files changed, 62 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index ca28fb8e2a07..7433627db12d 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -26,7 +26,9 @@ OPTIONS
 
 --refs=<pattern>::
 	Only use refs whose names match a given shell pattern.  The pattern
-	can be one of branch name, tag name or fully qualified ref name.
+	can be one of branch name, tag name or fully qualified ref name. If
+	given multiple times, use refs whose names match any of the given shell
+	patterns. Use `--no-refs` to clear any previous ref patterns given.
 
 --all::
 	List all commits reachable from all refs
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index cd89d48b65e8..000a2a700ed3 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -108,7 +108,7 @@ static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)
 struct name_ref_data {
 	int tags_only;
 	int name_only;
-	const char *ref_filter;
+	struct string_list ref_filters;
 };
 
 static struct tip_table {
@@ -150,16 +150,33 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
 	if (data->tags_only && !starts_with(path, "refs/tags/"))
 		return 0;
 
-	if (data->ref_filter) {
-		switch (subpath_matches(path, data->ref_filter)) {
-		case -1: /* did not match */
-			return 0;
-		case 0:  /* matched fully */
-			break;
-		default: /* matched subpath */
-			can_abbreviate_output = 1;
-			break;
+	if (data->ref_filters.nr) {
+		struct string_list_item *item;
+		int matched = 0;
+
+		/* See if any of the patterns match. */
+		for_each_string_list_item(item, &data->ref_filters) {
+			/*
+			 * We want to check every pattern even if we already
+			 * found a match, just in case one of the later
+			 * patterns could abbreviate the output.
+			 */
+			switch (subpath_matches(path, item->string)) {
+			case -1: /* did not match */
+				break;
+			case 0: /* matched fully */
+				matched = 1;
+				break;
+			default: /* matched subpath */
+				matched = 1;
+				can_abbreviate_output = 1;
+				break;
+			}
 		}
+
+		/* If none of the patterns matched, stop now */
+		if (!matched)
+			return 0;
 	}
 
 	add_to_tip_table(oid->hash, path, can_abbreviate_output);
@@ -306,11 +323,11 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 {
 	struct object_array revs = OBJECT_ARRAY_INIT;
 	int all = 0, transform_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0;
-	struct name_ref_data data = { 0, 0, NULL };
+	struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP };
 	struct option opts[] = {
 		OPT_BOOL(0, "name-only", &data.name_only, N_("print only names (no SHA-1)")),
 		OPT_BOOL(0, "tags", &data.tags_only, N_("only use tags to name the commits")),
-		OPT_STRING(0, "refs", &data.ref_filter, N_("pattern"),
+		OPT_STRING_LIST(0, "refs", &data.ref_filters, N_("pattern"),
 				   N_("only use refs matching <pattern>")),
 		OPT_GROUP(""),
 		OPT_BOOL(0, "all", &all, N_("list all commits reachable from all refs")),
diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index 1408b608eb03..d072ec43b016 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -99,6 +99,36 @@ test_expect_success '--cherry-pick bar does not come up empty (II)' '
 	test_cmp actual.named expect
 '
 
+test_expect_success 'name-rev multiple --refs combine inclusive' '
+	git rev-list --left-right --cherry-pick F...E -- bar > actual &&
+	git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" \
+		< actual > actual.named &&
+	test_cmp actual.named expect
+'
+
+cat >expect <<EOF
+<tags/F
+$(git rev-list --left-right --right-only --cherry-pick F...E -- bar)
+EOF
+
+test_expect_success 'name-rev --refs excludes non-matched patterns' '
+	git rev-list --left-right --cherry-pick F...E -- bar > actual &&
+	git name-rev --stdin --name-only --refs="*tags/F" \
+		< actual > actual.named &&
+	test_cmp actual.named expect
+'
+
+cat >expect <<EOF
+$(git rev-list --left-right --cherry-pick F...E -- bar)
+EOF
+
+test_expect_success 'name-rev --no-refs clears the refs list' '
+	git rev-list --left-right --cherry-pick F...E -- bar > actual &&
+	git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" --no-refs --refs="*tags/G" \
+		< actual > actual.named &&
+	test_cmp actual.named expect
+'
+
 cat >expect <<EOF
 +tags/F
 =tags/D
-- 
2.11.0.rc2.152.g4d04e67

[PATCH 2/2] describe: add support for multiple match patterns

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

Convert "--match" into a string list that accumulates patterns. If any
patterns are given, then include all tags that match at least one
pattern. This allows the user to construct multiple small patterns and
compose them. If desired, a future patch could expand functionality by
adding a new option to determine the style of combining multiple
patterns.

The primary use of this feature can be seen when trying to find which
release tag a given commit made it into. Suppose that you version all
your official releases such as "v1.2", "v1.3", "v1.4", "v2.1" and so on.
Now, you also have other tags which represent -rc releases and other
such tags. If you want to find the first major release that contains
a given commit you might try

git describe --contains --match="v?.?" <commit>

This will work as long as you have only single digits. But if you start
adding multiple digits, the pattern becomes not enough to match all the
tags you wanted while excluding the ones you didn't.

By implementing multiple --match invocations, this can be avoided by
simply passing multiple match patterns to the command:

git describe --contains --match="v[0-9].[0-9]" --match="v[0-9].[0-9][0-9]" ...

The end result is the ability to easily search tag space for which
tag included a given commit, without including -rc and other tags which
aren't interesting to you.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/git-describe.txt |  6 +++++-
 builtin/describe.c             | 31 ++++++++++++++++++++++++-------
 t/t6120-describe.sh            | 19 +++++++++++++++++++
 3 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index e4ac448ff565..c85f2811ce28 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -83,7 +83,11 @@ 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.
+	leaking private tags from the repository, or to shrink the scope of
+	searched tags to avoid -rc tags or similar. If given multiple 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.
 
 --always::
 	Show uniquely abbreviated commit object as fallback.
diff --git a/builtin/describe.c b/builtin/describe.c
index 01490a157efc..e3ceab65e273 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -28,7 +28,7 @@ static int abbrev = -1; /* unspecified */
 static int max_candidates = 10;
 static struct hashmap names;
 static int have_util;
-static const char *pattern;
+static struct string_list patterns = STRING_LIST_INIT_NODUP;
 static int always;
 static const char *dirty;
 
@@ -129,9 +129,25 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi
 	if (!all && !is_tag)
 		return 0;
 
-	/* Accept only tags that match the pattern, if given */
-	if (pattern && (!is_tag || wildmatch(pattern, path + 10, 0, NULL)))
-		return 0;
+	/*
+	 * If we're given patterns, accept only tags which match at least one
+	 * pattern.
+	 */
+	if (patterns.nr) {
+		struct string_list_item *item;
+
+		if (!is_tag)
+			return 0;
+
+		for_each_string_list_item(item, &patterns) {
+			if (!wildmatch(item->string, path + 10, 0, NULL)) {
+				break;
+			}
+
+			/* If we get here, no pattern matched. */
+			return 0;
+		}
+	}
 
 	/* Is it annotated? */
 	if (!peel_ref(path, peeled.hash)) {
@@ -404,7 +420,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 			    N_("only output exact matches"), 0),
 		OPT_INTEGER(0, "candidates", &max_candidates,
 			    N_("consider <n> most recent tags (default: 10)")),
-		OPT_STRING(0, "match",       &pattern, N_("pattern"),
+		OPT_STRING_LIST(0, "match", &patterns, N_("pattern"),
 			   N_("only consider tags matching <pattern>")),
 		OPT_BOOL(0, "always",        &always,
 			N_("show abbreviated commit object as fallback")),
@@ -430,6 +446,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		die(_("--long is incompatible with --abbrev=0"));
 
 	if (contains) {
+		struct string_list_item *item;
 		struct argv_array args;
 
 		argv_array_init(&args);
@@ -440,8 +457,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 			argv_array_push(&args, "--always");
 		if (!all) {
 			argv_array_push(&args, "--tags");
-			if (pattern)
-				argv_array_pushf(&args, "--refs=refs/tags/%s", pattern);
+			for_each_string_list_item(item, &patterns)
+				argv_array_pushf(&args, "--refs=refs/tags/%s", item->string);
 		}
 		if (argc)
 			argv_array_pushv(&args, argv);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 85f269411cb3..9e5db9b87a1f 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -182,6 +182,10 @@ check_describe "test2-lightweight-*" --tags --match="test2-*"
 
 check_describe "test2-lightweight-*" --long --tags --match="test2-*" HEAD^
 
+check_describe "test1-lightweight-*" --long --tags --match="test1-*" --match="test2-*" HEAD^
+
+check_describe "test2-lightweight-*" --long --tags --match="test1-*" --no-match --match="test2-*" HEAD^
+
 test_expect_success 'name-rev with exact tags' '
 	echo A >expect &&
 	tag_object=$(git rev-parse refs/tags/A) &&
@@ -206,4 +210,19 @@ test_expect_success 'describe --contains with the exact tags' '
 	test_cmp expect actual
 '
 
+test_expect_success 'describe --contains and --match' '
+	echo "A^0" >expect &&
+	tagged_commit=$(git rev-parse "refs/tags/A^0") &&
+	test_must_fail git describe --contains --match="B" $tagged_commit &&
+	git describe --contains --match="B" --match="A" $tagged_commit >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'describe --contains and --no-match' '
+	echo "A^0" >expect &&
+	tagged_commit=$(git rev-parse "refs/tags/A^0") &&
+	git describe --contains --match="B" --no-match $tagged_commit >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.11.0.rc2.152.g4d04e67

Re: [PATCH 2/2] describe: add support for multiple match patterns

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> ... Suppose that you version all
> your official releases such as "v1.2", "v1.3", "v1.4", "v2.1" and so on.
> Now, you also have other tags which represent -rc releases and other
> such tags. If you want to find the first major release that contains
> a given commit you might try
>
> git describe --contains --match="v?.?" <commit>
>
> This will work as long as you have only single digits. But if you start
> adding multiple digits, the pattern becomes not enough to match all the
> tags you wanted while excluding the ones you didn't.

Isn't what you really want for the use case a negative pattern,
i.e. "I want ones that match v* but not the ones that match *-rc*",
though?

Re: [PATCH 2/2] describe: add support for multiple match patterns

[Jacob Keller]

On Wed, Dec 7, 2016 at 2:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> ... Suppose that you version all
>> your official releases such as "v1.2", "v1.3", "v1.4", "v2.1" and so on.
>> Now, you also have other tags which represent -rc releases and other
>> such tags. If you want to find the first major release that contains
>> a given commit you might try
>>
>> git describe --contains --match="v?.?" <commit>
>>
>> This will work as long as you have only single digits. But if you start
>> adding multiple digits, the pattern becomes not enough to match all the
>> tags you wanted while excluding the ones you didn't.
>
> Isn't what you really want for the use case a negative pattern,
> i.e. "I want ones that match v* but not the ones that match *-rc*",
> though?

That's another way of implementing it. I just went with straight
forward patterns that I was already using in sequence.

Basically, this started as a script to try each pattern in sequence,
but this is slow, cumbersome and easy to mess up.

You're suggesting just add a single second pattern that we will do
matches and discard any tag that matches that first?

I think I can implement that pretty easily, and it should have simpler
semantics. We can discard first, and then match what remains easily.

Thanks,
Jake

Re: [PATCH 2/2] describe: add support for multiple match patterns

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

> Basically, this started as a script to try each pattern in sequence,
> but this is slow, cumbersome and easy to mess up.
>
> You're suggesting just add a single second pattern that we will do
> matches and discard any tag that matches that first?

I am not suggesting anything. I was just trying to see how well what
was designed and implemented supports the use case that motivated
the feature. Think of it as a sanity check and review of the design.

> I think I can implement that pretty easily, and it should have simpler
> semantics. We can discard first, and then match what remains easily.

I actually think "multiple" and "negative" are orthogonal and both
are good things.  If we are enhancing the filtering by refname
patterns to allow multiple patterns (i.e. your patch), that is good,
and it would be ideal if we can also have support for negative ones.

Re: [PATCH 2/2] describe: add support for multiple match patterns

[Jacob Keller]

On Wed, Dec 7, 2016 at 4:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> Basically, this started as a script to try each pattern in sequence,
>> but this is slow, cumbersome and easy to mess up.
>>
>> You're suggesting just add a single second pattern that we will do
>> matches and discard any tag that matches that first?
>
> I am not suggesting anything. I was just trying to see how well what
> was designed and implemented supports the use case that motivated
> the feature. Think of it as a sanity check and review of the design.
>

Makes sense.

>> I think I can implement that pretty easily, and it should have simpler
>> semantics. We can discard first, and then match what remains easily.
>
> I actually think "multiple" and "negative" are orthogonal and both
> are good things.  If we are enhancing the filtering by refname
> patterns to allow multiple patterns (i.e. your patch), that is good,
> and it would be ideal if we can also have support for negative ones.

I can add support for negative matches pretty easily. I personally
don't see the value of "logical and" filters, ie to match only tags
that match all the given filters, though that does allow some other
forms of expression.

I do like the idea of negative filters, and I'll go ahead and work on
adding that as another extension.

Thanks,
Jake