[PATCH 0/5] extend git-describe pattern matching

[PATCH 0/5] extend git-describe pattern matching

[Jacob Keller]

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

Teach git describe and git name-rev the ability to match multiple
patterns inclusively. Additionally, teach these commands to also accept
negative patterns to discard any refs which match.

The pattern lists for positive and negative patterns are inclusive. This
means that for the positive patterns, a reference will be considered as
long as it matches at least one of the match patterns. It need not match
all given patterns. Additionally for negative patterns, we will not
consider any ref which matches any negative pattern, even if it matches
one of the positive patterns.

Together this allows the ability to express far more sets of tags than a
single match pattern alone. It does not provide quite the same depth as
would teaching full regexp but it is simpler and easy enough to
understand.

This is a re-send of a series from a month or so ago, I've since
re-based this on next since it appears that it was not picked up before.

Jacob Keller (5):
  doc: add documentation for OPT_STRING_LIST
  name-rev: extend --refs to accept multiple patterns
  name-rev: add support to discard refs by pattern match
  describe: teach --match to accept multiple patterns
  describe: teach describe negative pattern matches

 Documentation/git-describe.txt                | 13 ++++++-
 Documentation/git-name-rev.txt                | 11 +++++-
 Documentation/technical/api-parse-options.txt |  5 +++
 builtin/describe.c                            | 51 ++++++++++++++++++++++----
 builtin/name-rev.c                            | 53 +++++++++++++++++++++------
 t/t6007-rev-list-cherry-pick-file.sh          | 37 +++++++++++++++++++
 t/t6120-describe.sh                           | 27 ++++++++++++++
 7 files changed, 176 insertions(+), 21 deletions(-)

-- 
2.11.0.403.g196674b8396b

[PATCH 1/5] doc: add documentation for OPT_STRING_LIST

[Jacob Keller]

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

Commit c8ba16391655 ("parse-options: add OPT_STRING_LIST helper",
2011-06-09) added the OPT_STRING_LIST as a way to accumulate a repeated
list of strings. However, this was not documented in the
api-parse-options documentation. Add documentation now so that future
developers may learn of its existence.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/technical/api-parse-options.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index 27bd701c0d68..15e876e4c804 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -168,6 +168,11 @@ There are some macros to easily define options:
 	Introduce an option with string argument.
 	The string argument is put into `str_var`.
 
+`OPT_STRING_LIST(short, long, &list, arg_str, description)`::
+	Introduce an option with a string argument. Repeated invocations
+	accumulate into a list of strings. Reset and clear the list with
+	`--no-option`.
+
 `OPT_INTEGER(short, long, &int_var, description)`::
 	Introduce an option with integer argument.
 	The integer is put into `int_var`.
-- 
2.11.0.403.g196674b8396b

[PATCH 2/5] name-rev: extend --refs to accept multiple patterns

[Jacob Keller]

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

Teach git name-rev to take a string list of patterns from --refs instead
of only a single pattern. The list of patterns will be matched
inclusively, such that a ref only needs to match one pattern to be
included. If a ref will only be excluded if it does not match any of the
patterns.

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

[PATCH 3/5] name-rev: add support to discard refs by pattern match

[Jacob Keller]

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

Extend name-rev further to support matching refs by adding `--discard`
patterns. These patterns will limit the scope of refs by discarding any
ref that matches at least one discard pattern. Checking the discard refs
shall happen first, before checking the include --refs patterns. This
will allow more flexibility to matching certain kinds of references.

Add tests and update Documentation for this change.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/git-name-rev.txt       |  7 +++++++
 builtin/name-rev.c                   | 14 +++++++++++++-
 t/t6007-rev-list-cherry-pick-file.sh |  7 +++++++
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index 7433627db12d..9b46e5ea9aae 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -30,6 +30,13 @@ OPTIONS
 	given multiple times, use refs whose names match any of the given shell
 	patterns. Use `--no-refs` to clear any previous ref patterns given.
 
+--discard=<pattern>::
+	Do not use any ref whose name matches a given shell pattern. The
+	pattern can be one of branch name, tag name or fully qualified ref
+	name. If given multiple times, discard refs that match any of the given
+	shell patterns. Use `--no-discards` to clear the list of discard
+	patterns.
+
 --all::
 	List all commits reachable from all refs
 
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 000a2a700ed3..86479c17a7c9 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -109,6 +109,7 @@ struct name_ref_data {
 	int tags_only;
 	int name_only;
 	struct string_list ref_filters;
+	struct string_list discard_filters;
 };
 
 static struct tip_table {
@@ -150,6 +151,15 @@ 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->discard_filters.nr) {
+		struct string_list_item *item;
+
+		for_each_string_list_item(item, &data->discard_filters) {
+			if (subpath_matches(path, item->string) >= 0)
+				return 0;
+		}
+	}
+
 	if (data->ref_filters.nr) {
 		struct string_list_item *item;
 		int matched = 0;
@@ -323,12 +333,14 @@ 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, STRING_LIST_INIT_NODUP };
+	struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP, 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_LIST(0, "refs", &data.ref_filters, N_("pattern"),
 				   N_("only use refs matching <pattern>")),
+		OPT_STRING_LIST(0, "discard", &data.discard_filters, N_("pattern"),
+				   N_("ignore refs matching <pattern>")),
 		OPT_GROUP(""),
 		OPT_BOOL(0, "all", &all, N_("list all commits reachable from all refs")),
 		OPT_BOOL(0, "stdin", &transform_stdin, N_("read from stdin")),
diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index d072ec43b016..8a4c35f6ffee 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -118,6 +118,13 @@ test_expect_success 'name-rev --refs excludes non-matched patterns' '
 	test_cmp actual.named expect
 '
 
+test_expect_success 'name-rev --discard excludes matched patterns' '
+	git rev-list --left-right --cherry-pick F...E -- bar > actual &&
+	git name-rev --stdin --name-only --refs="*tags/*" --discard="*E" \
+		< actual > actual.named &&
+	test_cmp actual.named expect
+'
+
 cat >expect <<EOF
 $(git rev-list --left-right --cherry-pick F...E -- bar)
 EOF
-- 
2.11.0.403.g196674b8396b

[PATCH 4/5] describe: teach --match to accept multiple patterns

[Jacob Keller]

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

Teach `--match` to be accepted multiple times, accumulating a list of
patterns to match into a string list. Each pattern is inclusive, such
that a tag need only match one of the provided patterns to be
considered for matching.

This extension is useful as it enables more flexibility in what tags
match, and may avoid the need to run the describe command multiple
times to get the same result.

Add tests and update the documentation for this change.

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

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index e4ac448ff565..7ad41e2f6ade 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -83,7 +83,10 @@ 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. 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.
 
 --always::
 	Show uniquely abbreviated commit object as fallback.
diff --git a/builtin/describe.c b/builtin/describe.c
index 01490a157efc..5cc9e9abe798 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,24 @@ 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 +419,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 +445,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 +456,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.403.g196674b8396b

[PATCH 5/5] describe: teach describe negative pattern matches

[Jacob Keller]

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

Teach git-describe the `--discard` option which will allow specifying
a glob pattern of tags to ignore. This can be combined with the
`--match` patterns to enable more flexibility in determining which tags
to consider.

For example, suppose you wish to find the first official release tag
that contains a certain commit. If we assume that official release tags
are of the form "v*" and pre-release candidates include "*rc*" in their
name, we can now find the first tag that introduces commit abcdef via:

  git describe --contains --match="v*" --discard="*rc*"

Add documentation and tests for this change.

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

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index 7ad41e2f6ade..a89bbde207b2 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -88,6 +88,14 @@ OPTIONS
 	patterns will be considered. Use `--no-match` to clear and reset the
 	list of patterns.
 
+--discard <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 discarded. Use `--no-discard` 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 5cc9e9abe798..c09288ee6321 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -29,6 +29,7 @@ static int max_candidates = 10;
 static struct hashmap names;
 static int have_util;
 static struct string_list patterns = STRING_LIST_INIT_NODUP;
+static struct string_list discard_patterns = STRING_LIST_INIT_NODUP;
 static int always;
 static const char *dirty;
 
@@ -130,6 +131,22 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi
 		return 0;
 
 	/*
+	 * If we're given discard patterns, first discard any tag which match
+	 * any of the discard pattern.
+	 */
+	if (discard_patterns.nr) {
+		struct string_list_item *item;
+
+		if (!is_tag)
+			return 0;
+
+		for_each_string_list_item(item, &discard_patterns) {
+			if (!wildmatch(item->string, path + 10, 0, NULL))
+				return 0;
+		}
+	}
+
+	/*
 	 * If we're given patterns, accept only tags which match at least one
 	 * pattern.
 	 */
@@ -421,6 +438,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 			    N_("consider <n> most recent tags (default: 10)")),
 		OPT_STRING_LIST(0, "match", &patterns, N_("pattern"),
 			   N_("only consider tags matching <pattern>")),
+		OPT_STRING_LIST(0, "discard", &discard_patterns, N_("pattern"),
+			   N_("do not consider tags matching <pattern>")),
 		OPT_BOOL(0, "always",        &always,
 			N_("show abbreviated commit object as fallback")),
 		{OPTION_STRING, 0, "dirty",  &dirty, N_("mark"),
@@ -458,6 +477,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 			argv_array_push(&args, "--tags");
 			for_each_string_list_item(item, &patterns)
 				argv_array_pushf(&args, "--refs=refs/tags/%s", item->string);
+			for_each_string_list_item(item, &discard_patterns)
+				argv_array_pushf(&args, "--discard=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 9e5db9b87a1f..4e4a9f2e5305 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -218,6 +218,14 @@ test_expect_success 'describe --contains and --match' '
 	test_cmp expect actual
 '
 
+test_expect_success 'describe --discard' '
+	echo "c~1" >expect &&
+	tagged_commit=$(git rev-parse "refs/tags/A^0") &&
+	test_must_fail git describe --contains --match="B" $tagged_commit &&
+	git describe --contains --match="?" --discard="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") &&
-- 
2.11.0.403.g196674b8396b

Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Johannes Schindelin]

Hi Jake,

On Wed, 11 Jan 2017, Jacob Keller wrote:

> From: Jacob Keller <jacob.keller@gmail.com>
> 
> Teach git-describe the `--discard` option which will allow specifying
> a glob pattern of tags to ignore.

IMHO "discard" is the wrong word, it almost sounds as if the matching tags
would be *deleted*.

Maybe `--exclude` or `--unmatch` instead?

Ciao,
Dscho

Re: [PATCH 1/5] doc: add documentation for OPT_STRING_LIST

[Johannes Schindelin]

Hi Jake,

On Wed, 11 Jan 2017, Jacob Keller wrote:

> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
> index 27bd701c0d68..15e876e4c804 100644
> --- a/Documentation/technical/api-parse-options.txt
> +++ b/Documentation/technical/api-parse-options.txt
> @@ -168,6 +168,11 @@ There are some macros to easily define options:
>  	Introduce an option with string argument.
>  	The string argument is put into `str_var`.
>  
> +`OPT_STRING_LIST(short, long, &list, arg_str, description)`::
> +	Introduce an option with a string argument. Repeated invocations
> +	accumulate into a list of strings. Reset and clear the list with
> +	`--no-option`.

One suggestions: as the list parameter is not type-safe (apart from
checking that it can be cast to a `void *`), it would be good to mention
in the documentation that `list` must be of type `struct string_list`.

I was about to suggest that `--no-option` may be misleading, as the
command-line option is not really called `--option` in almost all cases,
but I see that the rest of that document uses that convention to refer to
the negated option already...

Ciao,
Dscho

Re: [PATCH 2/5] name-rev: extend --refs to accept multiple patterns

[Johannes Schindelin]

Hi Jake,

On Wed, 11 Jan 2017, Jacob Keller wrote:

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

Our current coding style seems to skip the space between `>` and `actual`
(this applies to all redirections added in this patch).

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

In the current revision of t6007, we seem to list the expected output
explicitly, i.e. *not* generating it dynamically.

If you *do* insist to generate the `expect` file dynamically, a better way
would be to include that generation in the `test_expect_success` code so
that errors in the call can be caught, too:

test_expect_success 'name-rev --refs excludes non-matched patterns' '
	echo "<tags/F" >expect &&
	git rev-list --left-right --right-only --cherry-pick F...E -- \
		bar >>expect &&
	[...]

However, if I was asked for my preference, I would suggest to specify the
`expect` contents explicitly, to document the expectation as of time of
writing. The reason: I debugged my share of test breakages and these
dynamically-generated `expect` files are the worst. When things break, you
have to dig *real* deep to figure out what is going wrong, as sometimes
the *generation of the `expect` file* regresses.

Ciao,
Dscho

Re: [PATCH 3/5] name-rev: add support to discard refs by pattern match

[Johannes Schindelin]

Hi Jake,

On Wed, 11 Jan 2017, Jacob Keller wrote:

> From: Jacob Keller <jacob.keller@gmail.com>
> 
> Extend name-rev further to support matching refs by adding `--discard`
> patterns.

Same comment applies as for 5/5: `--exclude-refs` may be a better name
than `--discard`.

Ciao,
Dscho

Re: [PATCH 0/5] extend git-describe pattern matching

[Johannes Schindelin]

Hi Jake,

On Wed, 11 Jan 2017, Jacob Keller wrote:

> From: Jacob Keller <jacob.keller@gmail.com>
> 
> Teach git describe and git name-rev the ability to match multiple
> patterns inclusively. Additionally, teach these commands to also accept
> negative patterns to discard any refs which match.

I like the idea, and I think this patch series is already in a pretty good
shape, offering a couple of comments as individual replies to the
respective patches.

Ciao,
Dscho

Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Johannes Sixt]

Am 12.01.2017 um 01:17 schrieb Jacob Keller:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> Teach git-describe the `--discard` option which will allow specifying
> a glob pattern of tags to ignore. This can be combined with the
> `--match` patterns to enable more flexibility in determining which tags
> to consider.
>
> For example, suppose you wish to find the first official release tag
> that contains a certain commit. If we assume that official release tags
> are of the form "v*" and pre-release candidates include "*rc*" in their
> name, we can now find the first tag that introduces commit abcdef via:
>
>   git describe --contains --match="v*" --discard="*rc*"

I have a few dozen topic branches, many of them are work in progress and 
named wip/something. To see the completed branches, I routinely say

     gitk --exclude=wip/* --branches

these days.

It would be great if you could provide the same user interface here. The 
example in the commit message would then look like this:

    git describe --contains --exclude="*rc*" --match="v*"

(I'm not saying that you should add --branches, but that you should 
prefer --exclude over --discard. Also, the order of --exclude and 
--match would be important.)

-- Hannes

Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Jake,
>
> On Wed, 11 Jan 2017, Jacob Keller wrote:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>> 
>> Teach git-describe the `--discard` option which will allow specifying
>> a glob pattern of tags to ignore.
>
> IMHO "discard" is the wrong word, it almost sounds as if the matching tags
> would be *deleted*.
>
> Maybe `--exclude` or `--unmatch` instead?

Yeah, as j6t mentions, I think --exclude would be a good choice.

Re: [PATCH 1/5] doc: add documentation for OPT_STRING_LIST

[Jacob Keller]

On Thu, Jan 12, 2017 at 1:47 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Jake,
>
> On Wed, 11 Jan 2017, Jacob Keller wrote:
>
>> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
>> index 27bd701c0d68..15e876e4c804 100644
>> --- a/Documentation/technical/api-parse-options.txt
>> +++ b/Documentation/technical/api-parse-options.txt
>> @@ -168,6 +168,11 @@ There are some macros to easily define options:
>>       Introduce an option with string argument.
>>       The string argument is put into `str_var`.
>>
>> +`OPT_STRING_LIST(short, long, &list, arg_str, description)`::
>> +     Introduce an option with a string argument. Repeated invocations
>> +     accumulate into a list of strings. Reset and clear the list with
>> +     `--no-option`.
>
> One suggestions: as the list parameter is not type-safe (apart from
> checking that it can be cast to a `void *`), it would be good to mention
> in the documentation that `list` must be of type `struct string_list`.
>

Ok.

> I was about to suggest that `--no-option` may be misleading, as the
> command-line option is not really called `--option` in almost all cases,
> but I see that the rest of that document uses that convention to refer to
> the negated option already...

Also, I am merely documenting what already existed, since the original
commit failed to add documentation. I don't know if we could make a
better implementation, but it certainly would be a behavior change for
the current users.

Thanks,
Jake

>
> Ciao,
> Dscho

Re: [PATCH 2/5] name-rev: extend --refs to accept multiple patterns

[Jacob Keller]

On Thu, Jan 12, 2017 at 1:56 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Jake,
>
> On Wed, 11 Jan 2017, Jacob Keller wrote:
>
>> 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 &&
>
> Our current coding style seems to skip the space between `>` and `actual`
> (this applies to all redirections added in this patch).
>

Right that's easy to fix.

>> +     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
>
> In the current revision of t6007, we seem to list the expected output
> explicitly, i.e. *not* generating it dynamically.
>
> If you *do* insist to generate the `expect` file dynamically, a better way
> would be to include that generation in the `test_expect_success` code so
> that errors in the call can be caught, too:
>

The main reason I don't like static expecations is that often other
tests inserted before or after my test now have to know what to put
here. Specifically, the problem is that we expect the output not to be
named, but actually to be sha1 hex output. This seems really brittle
for a test.

> test_expect_success 'name-rev --refs excludes non-matched patterns' '
>         echo "<tags/F" >expect &&
>         git rev-list --left-right --right-only --cherry-pick F...E -- \
>                 bar >>expect &&
>         [...]
>
> However, if I was asked for my preference, I would suggest to specify the
> `expect` contents explicitly, to document the expectation as of time of
> writing. The reason: I debugged my share of test breakages and these
> dynamically-generated `expect` files are the worst. When things break, you
> have to dig *real* deep to figure out what is going wrong, as sometimes
> the *generation of the `expect` file* regresses.
>

Do you have a better suggestion for how to check the expect vs actual
output ignoring the raw hex data that would be there otherwise? What I
want to avoid is a brittle test that depends precisely on actions of
prior tests in generating commits and tags. Specifically in this case,
we're going to end up with the sha1 hex ID of the commit in the
expected value.. hard-coding that feels wrong.

Thanks,
Jake

> Ciao,
> Dscho

Re: [PATCH 3/5] name-rev: add support to discard refs by pattern match

[Jacob Keller]

On Thu, Jan 12, 2017 at 1:57 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Jake,
>
> On Wed, 11 Jan 2017, Jacob Keller wrote:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> Extend name-rev further to support matching refs by adding `--discard`
>> patterns.
>
> Same comment applies as for 5/5: `--exclude-refs` may be a better name
> than `--discard`.
>

I agree, will change.

Thanks,
Jake

> Ciao,
> Dscho

Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Jacob Keller]

On Thu, Jan 12, 2017 at 5:45 AM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 12.01.2017 um 01:17 schrieb Jacob Keller:
>>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> Teach git-describe the `--discard` option which will allow specifying
>> a glob pattern of tags to ignore. This can be combined with the
>> `--match` patterns to enable more flexibility in determining which tags
>> to consider.
>>
>> For example, suppose you wish to find the first official release tag
>> that contains a certain commit. If we assume that official release tags
>> are of the form "v*" and pre-release candidates include "*rc*" in their
>> name, we can now find the first tag that introduces commit abcdef via:
>>
>>   git describe --contains --match="v*" --discard="*rc*"
>
>
> I have a few dozen topic branches, many of them are work in progress and
> named wip/something. To see the completed branches, I routinely say
>
>     gitk --exclude=wip/* --branches
>
> these days.
>
> It would be great if you could provide the same user interface here. The
> example in the commit message would then look like this:
>
>    git describe --contains --exclude="*rc*" --match="v*"
>
> (I'm not saying that you should add --branches, but that you should prefer
> --exclude over --discard. Also, the order of --exclude and --match would be
> important.)

I think that --exclude makes sense, but the current implementation
does not differentiate ordering, since both are merely accumulated
into string_lists and then matched together. I'm not sure how order
would impact things here? In the current implementation, if something
is excluded and matched, it will be excluded. That is, exclusion
patterns take precedence over match patterns. I think this makes the
most sense semantically.

Thanks,
Jake

>
> -- Hannes
>

Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Johannes Sixt]

Am 13.01.2017 um 01:59 schrieb Jacob Keller:
> I think that --exclude makes sense, but the current implementation
> does not differentiate ordering, since both are merely accumulated
> into string_lists and then matched together. I'm not sure how order
> would impact things here? In the current implementation, if something
> is excluded and matched, it will be excluded. That is, exclusion
> patterns take precedence over match patterns. I think this makes the
> most sense semantically.

When you write

   git log --exclude=wip/* --branches --remotes

--exclude applies only to --branches, not to --remotes.

  When you write

   git log --branches --exclude=origin/* --remotes

--exclude=origin/* applies only to --remotes, but not to --branches.

-- Hannes

Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Jacob Keller]

On Thu, Jan 12, 2017 at 10:43 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 13.01.2017 um 01:59 schrieb Jacob Keller:
>>
>> I think that --exclude makes sense, but the current implementation
>> does not differentiate ordering, since both are merely accumulated
>> into string_lists and then matched together. I'm not sure how order
>> would impact things here? In the current implementation, if something
>> is excluded and matched, it will be excluded. That is, exclusion
>> patterns take precedence over match patterns. I think this makes the
>> most sense semantically.
>
>
> When you write
>
>   git log --exclude=wip/* --branches --remotes
>
> --exclude applies only to --branches, not to --remotes.
>
>  When you write
>
>   git log --branches --exclude=origin/* --remotes
>
> --exclude=origin/* applies only to --remotes, but not to --branches.
>
> -- Hannes
>

Well for describe I don't think the order matters.

Thanks,
Jake

Re: [PATCH 0/5] extend git-describe pattern matching

[Junio C Hamano]

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

> From: Jacob Keller <jacob.keller@gmail.com>
>
> Teach git describe and git name-rev the ability to match multiple
> patterns inclusively. Additionally, teach these commands to also accept
> negative patterns to discard any refs which match.

You made quick responses to reviews with "will change", so I am not
queuing this round to my tree; please don't mistake that as my
indifference or opposition to the topic.  This sounds like a good
thing.

As to the semantics of mixing positives and negatives, I would
recommend this to follow how positive and negative pathspecs mix.
IIRC we chose to use the most simple and easy to explain option,
i.e. a thing must match at least one of the positives and must not
match any of the negatives to be considered a match.

Re: [PATCH 0/5] extend git-describe pattern matching

[Jacob Keller]

On Fri, Jan 13, 2017 at 10:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> Teach git describe and git name-rev the ability to match multiple
>> patterns inclusively. Additionally, teach these commands to also accept
>> negative patterns to discard any refs which match.
>
> You made quick responses to reviews with "will change", so I am not
> queuing this round to my tree; please don't mistake that as my
> indifference or opposition to the topic.  This sounds like a good
> thing.

Perfect. I will probably take a few days till I am back at a computer
and can do this, but I will be submitting with the suggested changes
soon.

>
> As to the semantics of mixing positives and negatives, I would
> recommend this to follow how positive and negative pathspecs mix.
> IIRC we chose to use the most simple and easy to explain option,
> i.e. a thing must match at least one of the positives and must not
> match any of the negatives to be considered a match.
>
>

That is the current implementation, so I will stick with it. It's the
simplest, and easiest to implement.

Thanks,
Jake

Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Johannes Sixt]

Am 13.01.2017 um 07:57 schrieb Jacob Keller:
> On Thu, Jan 12, 2017 at 10:43 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>>  When you write
>>
>>   git log --branches --exclude=origin/* --remotes
>>
>> --exclude=origin/* applies only to --remotes, but not to --branches.
>
> Well for describe I don't think the order matters.

That is certainly true today. But I would value consistency more. We 
would lose it if some time in the future 'describe' accepts --branches 
and --remotes in addition to --tags and --all.

-- Hannes

Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Jacob Keller]

On Fri, Jan 13, 2017 at 1:31 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 13.01.2017 um 07:57 schrieb Jacob Keller:
>>
>> On Thu, Jan 12, 2017 at 10:43 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>>>
>>>  When you write
>>>
>>>   git log --branches --exclude=origin/* --remotes
>>>
>>> --exclude=origin/* applies only to --remotes, but not to --branches.
>>
>>
>> Well for describe I don't think the order matters.
>
>
> That is certainly true today. But I would value consistency more. We would
> lose it if some time in the future 'describe' accepts --branches and
> --remotes in addition to --tags and --all.
>
> -- Hannes
>

I am not sure that the interface for git-log and git-describe are
similar enough to make this distinction work. --match already seems to
imply that it only works on refs in refs/tags, as it says it considers
globs matching excluding the "refs/tags" prefix.

In git-describe, we already have "--tags" and "--all" but they are
mutually exclusive. We don't support using more than one at once, and
I'm not really convinced that describe will ever support more than one
at a time. Additionally, match already doesn't respect order.

Thanks,
Jake

Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Johannes Schindelin]

Hi Jake,

On Tue, 17 Jan 2017, Jacob Keller wrote:

> On Fri, Jan 13, 2017 at 1:31 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> > Am 13.01.2017 um 07:57 schrieb Jacob Keller:
> >>
> >> On Thu, Jan 12, 2017 at 10:43 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> >>>
> >>>  When you write
> >>>
> >>>   git log --branches --exclude=origin/* --remotes
> >>>
> >>> --exclude=origin/* applies only to --remotes, but not to --branches.
> >>
> >>
> >> Well for describe I don't think the order matters.
> >
> >
> > That is certainly true today. But I would value consistency more. We would
> > lose it if some time in the future 'describe' accepts --branches and
> > --remotes in addition to --tags and --all.
> >
> > -- Hannes
> >
> 
> I am not sure that the interface for git-log and git-describe are
> similar enough to make this distinction work. --match already seems to
> imply that it only works on refs in refs/tags, as it says it considers
> globs matching excluding the "refs/tags" prefix.
> 
> In git-describe, we already have "--tags" and "--all" but they are
> mutually exclusive. We don't support using more than one at once, and
> I'm not really convinced that describe will ever support more than one
> at a time. Additionally, match already doesn't respect order.

I agree that it would keep the code much simpler if you kept the order
"exclude before include".

However, should you decide to look into making the logic dependent on the
order in which the flags were specified in the command-line, we do have a
data structure for such a beast: we use it in gitignore and in
sparse-checkout, it is called struct exclude_list.

Just some food for thought,
Johannes

Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Jacob Keller]

On Wed, Jan 18, 2017 at 4:44 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Jake,
>
> On Tue, 17 Jan 2017, Jacob Keller wrote:
>
>> On Fri, Jan 13, 2017 at 1:31 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> > Am 13.01.2017 um 07:57 schrieb Jacob Keller:
>> >>
>> >> On Thu, Jan 12, 2017 at 10:43 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> >>>
>> >>>  When you write
>> >>>
>> >>>   git log --branches --exclude=origin/* --remotes
>> >>>
>> >>> --exclude=origin/* applies only to --remotes, but not to --branches.
>> >>
>> >>
>> >> Well for describe I don't think the order matters.
>> >
>> >
>> > That is certainly true today. But I would value consistency more. We would
>> > lose it if some time in the future 'describe' accepts --branches and
>> > --remotes in addition to --tags and --all.
>> >
>> > -- Hannes
>> >
>>
>> I am not sure that the interface for git-log and git-describe are
>> similar enough to make this distinction work. --match already seems to
>> imply that it only works on refs in refs/tags, as it says it considers
>> globs matching excluding the "refs/tags" prefix.
>>
>> In git-describe, we already have "--tags" and "--all" but they are
>> mutually exclusive. We don't support using more than one at once, and
>> I'm not really convinced that describe will ever support more than one
>> at a time. Additionally, match already doesn't respect order.
>
> I agree that it would keep the code much simpler if you kept the order
> "exclude before include".
>
> However, should you decide to look into making the logic dependent on the
> order in which the flags were specified in the command-line, we do have a
> data structure for such a beast: we use it in gitignore and in
> sparse-checkout, it is called struct exclude_list.
>
> Just some food for thought,
> Johannes

That might help make it easier to go this route. I'll take a look.

Thanks,
Jake