threads / patch / 44905

v3, 5 partsdescribe: teach --match to accept multiple patterns

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

## tl;dr

22 messages between Jan 18, 2017 and Jan 19, 2017. Diffs are folded; open one to read it.

replies: 21people: 5as markdown or json

Jacob Keller· Jan 18, 2017, 00:09 UTC · lore

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

From: Jacob Keller <jacob.keller@gmail.com>
** v3 fixes a minor typo in one of the test cases, so please ignore v2
   I left the interdiff as between v1 and v3 instead of v2 **

Teach git describe and git name-rev the ability to match multiple patterns inclusively. Additionally, teach these commands to also accept negative patterns to exclude 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.

- v2
* use --exclude instead of --discard
* use modern style in tests
- v3
* fix broken test (sorry for the thrash!)

I chose *not* to implement the suggestion of ordered values for exclude and match, since this is not how the current implementation of git-describe worked, and it didn't really make sense to me what was being requested. I looked at the interface for git-log, and it appears that the command accepts multiple invocations of --branches, --remotes, and similar. I do not believe these need to be identical interfaces. I welcome feedback on this decision, but I am not convinced yet that the ordered arguments are worth the trouble.

Show changes to diff +40 −37
diff --git c/Documentation/git-describe.txt w/Documentation/git-describe.txt
index a89bbde207b2..21a43b78924a 100644
--- c/Documentation/git-describe.txt
+++ w/Documentation/git-describe.txt
@@ -88,12 +88,12 @@ OPTIONS
	patterns will be considered. Use `--no-match` to clear and reset the
	list of patterns.
 
---discard <pattern>::
+--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 discarded. Use `--no-discard` to clear and
+	of the patterns will be excluded. Use `--no-exclude` to clear and
	reset the list of patterns.
 
 --always::
diff --git c/Documentation/git-name-rev.txt w/Documentation/git-name-rev.txt
index 9b46e5ea9aae..301b4a8d55e6 100644
--- c/Documentation/git-name-rev.txt
+++ w/Documentation/git-name-rev.txt
@@ -30,11 +30,11 @@ 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>::
+--exclude=<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
+	name. If given multiple times, exclude refs that match any of the given
+	shell patterns. Use `--no-exclude` to clear the list of exclude
	patterns.
 
 --all::
diff --git c/Documentation/technical/api-parse-options.txt w/Documentation/technical/api-parse-options.txt
index 15e876e4c804..6914f54f5f44 100644
--- c/Documentation/technical/api-parse-options.txt
+++ w/Documentation/technical/api-parse-options.txt
@@ -169,9 +169,9 @@ There are some macros to easily define options:
	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`.
+	Introduce an option with string argument.
+	The string argument is stored as an element in `&list` which must be a
+	struct string_list. Reset the list using `--no-option`.
 
 `OPT_INTEGER(short, long, &int_var, description)`::
	Introduce an option with integer argument.
diff --git c/builtin/describe.c w/builtin/describe.c
index c09288ee6321..6769446e1f57 100644
--- c/builtin/describe.c
+++ w/builtin/describe.c
@@ -29,7 +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 struct string_list exclude_patterns = STRING_LIST_INIT_NODUP;
 static int always;
 static const char *dirty;
 
@@ -131,16 +131,16 @@ 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 we're given exclude patterns, first exclude any tag which match
+	 * any of the exclude pattern.
	 */
-	if (discard_patterns.nr) {
+	if (exclude_patterns.nr) {
		struct string_list_item *item;
 
		if (!is_tag)
			return 0;
 
-		for_each_string_list_item(item, &discard_patterns) {
+		for_each_string_list_item(item, &exclude_patterns) {
			if (!wildmatch(item->string, path + 10, 0, NULL))
				return 0;
		}
@@ -438,7 +438,7 @@ 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"),
+		OPT_STRING_LIST(0, "exclude", &exclude_patterns, N_("pattern"),
			   N_("do not consider tags matching <pattern>")),
		OPT_BOOL(0, "always",        &always,
			N_("show abbreviated commit object as fallback")),
@@ -477,8 +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);
+			for_each_string_list_item(item, &exclude_patterns)
+				argv_array_pushf(&args, "--exclude=refs/tags/%s", item->string);
		}
		if (argc)
			argv_array_pushv(&args, argv);
diff --git c/builtin/name-rev.c w/builtin/name-rev.c
index 86479c17a7c9..da4a0d7c0fdf 100644
--- c/builtin/name-rev.c
+++ w/builtin/name-rev.c
@@ -109,7 +109,7 @@ struct name_ref_data {
	int tags_only;
	int name_only;
	struct string_list ref_filters;
-	struct string_list discard_filters;
+	struct string_list exclude_filters;
 };
 
 static struct tip_table {
@@ -151,10 +151,10 @@ 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) {
+	if (data->exclude_filters.nr) {
		struct string_list_item *item;
 
-		for_each_string_list_item(item, &data->discard_filters) {
+		for_each_string_list_item(item, &data->exclude_filters) {
			if (subpath_matches(path, item->string) >= 0)
				return 0;
		}
@@ -339,7 +339,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
		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"),
+		OPT_STRING_LIST(0, "exclude", &data.exclude_filters, N_("pattern"),
				   N_("ignore refs matching <pattern>")),
		OPT_GROUP(""),
		OPT_BOOL(0, "all", &all, N_("list all commits reachable from all refs")),
diff --git c/t/t6007-rev-list-cherry-pick-file.sh w/t/t6007-rev-list-cherry-pick-file.sh
index 8a4c35f6ffee..83838d0da208 100755
--- c/t/t6007-rev-list-cherry-pick-file.sh
+++ w/t/t6007-rev-list-cherry-pick-file.sh
@@ -100,42 +100,43 @@ test_expect_success '--cherry-pick bar does not come up empty (II)' '
 '
 
 test_expect_success 'name-rev multiple --refs combine inclusive' '
-	git rev-list --left-right --cherry-pick F...E -- bar > actual &&
+	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 &&
+		<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 rev-list --left-right --right-only --cherry-pick F...E -- bar >>expect &&
+	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
-'
-
-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 &&
+		<actual >actual.named &&
	test_cmp actual.named expect
 '
 
 cat >expect <<EOF
-$(git rev-list --left-right --cherry-pick F...E -- bar)
+<tags/F
 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_expect_success 'name-rev --exclude excludes matched patterns' '
+	git rev-list --left-right --right-only --cherry-pick F...E -- bar >>expect &&
+	git rev-list --left-right --cherry-pick F...E -- bar >actual &&
+	git name-rev --stdin --name-only --refs="*tags/*" --exclude="*E" \
+		<actual >actual.named &&
	test_cmp actual.named expect
 '
 
+test_expect_success 'name-rev --no-refs clears the refs list' '
+	git rev-list --left-right --cherry-pick F...E -- bar >expect &&
+	git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" --no-refs --refs="*tags/G" \
+		<expect >actual &&
+	test_cmp actual expect
+'
+
 cat >expect <<EOF
 +tags/F
 =tags/D
diff --git c/t/t6120-describe.sh w/t/t6120-describe.sh
index 4e4a9f2e5305..167491fd5b0d 100755
--- c/t/t6120-describe.sh
+++ w/t/t6120-describe.sh
@@ -218,11 +218,11 @@ test_expect_success 'describe --contains and --match' '
	test_cmp expect actual
 '
 
-test_expect_success 'describe --discard' '
+test_expect_success 'describe --exclude' '
	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 &&
+	git describe --contains --match="?" --exclude="A" $tagged_commit >actual &&
	test_cmp expect actual
 '
 

Jacob Keller (5):
  doc: add documentation for OPT_STRING_LIST
  name-rev: extend --refs to accept multiple patterns
  name-rev: add support to exclude 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          | 38 +++++++++++++++++++
 t/t6120-describe.sh                           | 27 ++++++++++++++
 7 files changed, 177 insertions(+), 21 deletions(-)
-- 
2.11.0.403.g196674b8396b
Jacob Keller· Jan 18, 2017, 00:09 UTC · re: Jacob Keller · lore
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(-)
Show changes to 3 files +46 −8

Documentation/git-describe.txt, builtin/describe.c, t/t6120-describe.sh

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
Jacob Keller· Jan 18, 2017, 00:09 UTC · re: Jacob Keller · lore

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

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

Teach git-describe the `--exclude` 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*" --exclude="*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(+)
Show changes to 3 files +37 −0

Documentation/git-describe.txt, builtin/describe.c, t/t6120-describe.sh

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index 7ad41e2f6ade..21a43b78924a 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.
 
+--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. Use `--no-exclude` 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..6769446e1f57 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 exclude_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 exclude patterns, first exclude any tag which match
+	 * any of the exclude pattern.
+	 */
+	if (exclude_patterns.nr) {
+		struct string_list_item *item;
+
+		if (!is_tag)
+			return 0;
+
+		for_each_string_list_item(item, &exclude_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, "exclude", &exclude_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, &exclude_patterns)
+				argv_array_pushf(&args, "--exclude=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..167491fd5b0d 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 --exclude' '
+	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="?" --exclude="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
Junio C Hamano· Jan 18, 2017, 20:17 UTC · re: Jacob Keller · lore

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

Jacob Keller <jacob.e.keller@intel.com> writes:
Show 18 quoted lines
> From: Jacob Keller <jacob.keller@gmail.com>
>
> Teach git-describe the `--exclude` 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*" --exclude="*rc*"
>
> Add documentation and tests for this change.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---

The above is much better than 3/5 with a concrete example (compared to the vague "certain kinds of references"). It also does not have the "we check this first and then that" ;-).

Show 16 quoted lines
> diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
> index 7ad41e2f6ade..21a43b78924a 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.
>  
> +--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. Use `--no-exclude` to clear and
> +	reset the list of patterns.
> +

Similar to 3/5, perhaps we want to say something about interaction between this one and --match?

Jacob Keller· Jan 18, 2017, 00:09 UTC · re: Jacob Keller · lore

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

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

Extend name-rev further to support matching refs by adding `--exclude` patterns. These patterns will limit the scope of refs by excluding any ref that matches at least one exclude pattern. Checking the exclude 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 | 12 ++++++++++++
 3 files changed, 32 insertions(+), 1 deletion(-)
Show changes to 3 files +32 −1

Documentation/git-name-rev.txt, builtin/name-rev.c, t/t6007-rev-list-cherry-pick-file.sh

diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index 7433627db12d..301b4a8d55e6 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.
 
+--exclude=<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, exclude refs that match any of the given
+	shell patterns. Use `--no-exclude` to clear the list of exclude
+	patterns.
+
 --all::
 	List all commits reachable from all refs
 
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 000a2a700ed3..da4a0d7c0fdf 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 exclude_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->exclude_filters.nr) {
+		struct string_list_item *item;
+
+		for_each_string_list_item(item, &data->exclude_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, "exclude", &data.exclude_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 d9827a6389a3..29597451967d 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -118,6 +118,18 @@ test_expect_success 'name-rev --refs excludes non-matched patterns' '
 	test_cmp actual.named expect
 '
 
+cat >expect <<EOF
+<tags/F
+EOF
+
+test_expect_success 'name-rev --exclude excludes matched patterns' '
+	git rev-list --left-right --right-only --cherry-pick F...E -- bar >>expect &&
+	git rev-list --left-right --cherry-pick F...E -- bar >actual &&
+	git name-rev --stdin --name-only --refs="*tags/*" --exclude="*E" \
+		<actual >actual.named &&
+	test_cmp actual.named expect
+'
+
 test_expect_success 'name-rev --no-refs clears the refs list' '
 	git rev-list --left-right --cherry-pick F...E -- bar >expect &&
 	git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" --no-refs --refs="*tags/G" \
-- 
2.11.0.403.g196674b8396b
Junio C Hamano· Jan 18, 2017, 20:11 UTC · re: Jacob Keller · lore

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

Jacob Keller <jacob.e.keller@intel.com> writes:
Show 6 quoted lines
> From: Jacob Keller <jacob.keller@gmail.com>
>
> Extend name-rev further to support matching refs by adding `--exclude`
> patterns. These patterns will limit the scope of refs by excluding any
> ref that matches at least one exclude pattern. Checking the exclude refs
> shall happen first, before checking the include --refs patterns.

I do not think we should have this "exclude first and then include" written down here, as it is an irrelevant implementation detail. The desired end result is that only refs that match at least one include and none of the exclude survive. You could implement it by first checking with include and then further narrowing that set by filtering those that match exclude (I am not saying that "include first then exclude" is better---I am saying that it is far less important than "at least one include and none of the exclude" to mention the order of application).

Show 6 quoted lines
> +--exclude=<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, exclude refs that match any of the given
> +	shell patterns. Use `--no-exclude` to clear the list of exclude
> +	patterns.
Perhaps insert
    When used together with --refs, only those that match at least
    one of the --refs patterns and none of the --exclude patterns
    are used.
before "Use `--no-exclude` to clear"?
Jacob Keller· Jan 18, 2017, 21:13 UTC · re: Junio C Hamano · lore

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

On Wed, Jan 18, 2017 at 12:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 34 quoted lines
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> Extend name-rev further to support matching refs by adding `--exclude`
>> patterns. These patterns will limit the scope of refs by excluding any
>> ref that matches at least one exclude pattern. Checking the exclude refs
>> shall happen first, before checking the include --refs patterns.
>
> I do not think we should have this "exclude first and then include"
> written down here, as it is an irrelevant implementation detail.
> The desired end result is that only refs that match at least one
> include and none of the exclude survive.  You could implement it by
> first checking with include and then further narrowing that set by
> filtering those that match exclude (I am not saying that "include
> first then exclude" is better---I am saying that it is far less
> important than "at least one include and none of the exclude" to
> mention the order of application).
>
>> +--exclude=<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, exclude refs that match any of the given
>> +     shell patterns. Use `--no-exclude` to clear the list of exclude
>> +     patterns.
>
> Perhaps insert
>
>     When used together with --refs, only those that match at least
>     one of the --refs patterns and none of the --exclude patterns
>     are used.
>
> before "Use `--no-exclude` to clear"?
>

Yes this makes sense. I'm still looking at whether the alternative implementation suggested based on the git-log style would make more sense or not, but if we keep this as is, the added text you gave is important.

Thanks, Jake

Junio C Hamano· Jan 18, 2017, 21:56 UTC · re: Jacob Keller · lore

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

Jacob Keller <jacob.keller@gmail.com> writes:
> Yes this makes sense. I'm still looking at whether the alternative
> implementation suggested based on the git-log style would make more
> sense or not, but if we keep this as is, the added text you gave is
> important.

I actually think it is a red-herring that "git log" honors "orders"; it does, but that is not a result of carefully considering the desired behaviour. It instead is a historical wart that came from the fact that "--branches" and friends uses for_each_glob_ref_in() that takes the top-level hierarchy paths like "refs/heads/" and the implementation of "--exclude" piggybacked into the function in a lazy way.

If exclusion were done independently (e.g. in a way similar to what you did in this series using subpath match), we wouldn't have had the "the user must give exclude patterns first that would affect the next inclusion pattern, at which point the exclude patterns are cleared and the user needs to start over", which is an end-user experience that is clunky.

Jacob Keller· Jan 18, 2017, 22:31 UTC · re: Junio C Hamano · lore

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

On Wed, Jan 18, 2017 at 1:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 22 quoted lines
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> Yes this makes sense. I'm still looking at whether the alternative
>> implementation suggested based on the git-log style would make more
>> sense or not, but if we keep this as is, the added text you gave is
>> important.
>
> I actually think it is a red-herring that "git log" honors "orders";
> it does, but that is not a result of carefully considering the
> desired behaviour.  It instead is a historical wart that came from
> the fact that "--branches" and friends uses for_each_glob_ref_in()
> that takes the top-level hierarchy paths like "refs/heads/" and the
> implementation of "--exclude" piggybacked into the function in a
> lazy way.
>
> If exclusion were done independently (e.g. in a way similar to what
> you did in this series using subpath match), we wouldn't have had
> the "the user must give exclude patterns first that would affect the
> next inclusion pattern, at which point the exclude patterns are
> cleared and the user needs to start over", which is an end-user
> experience that is clunky.
>

However, it is useful that exclude patterns only apply to specific match parameters? That is the advantage of the other implementation.

I think I agree that it's not really worth the complexity, as it requires a much more complex explanation of how the parameters interact, and in general doesn't provide that much more expressiveness, since at least for "git describe" by definition it either finds the tag as a match or not. Sure you could say "include all tags matching x but only if they don't match y" and include all tags matching z even if they match y" using that mechanism, but I think that makes the entire thing needlessly more complicated than "we use a tag if it matches any match and doesn't match any exclude".

Thanks, Jake

Jacob Keller· Jan 18, 2017, 00:09 UTC · re: Jacob Keller · lore

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

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. The tests do use dynamically generated output as part of the expected output because the expected output is the raw commit-id and it seemed like a bad idea to hardcode that into a tests expected output.

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 | 26 +++++++++++++++++++++++
 3 files changed, 58 insertions(+), 13 deletions(-)
Show changes to 3 files +58 −13

Documentation/git-name-rev.txt, builtin/name-rev.c, t/t6007-rev-list-cherry-pick-file.sh

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..d9827a6389a3 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -99,6 +99,32 @@ 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
+EOF
+
+test_expect_success 'name-rev --refs excludes non-matched patterns' '
+	git rev-list --left-right --right-only --cherry-pick F...E -- bar >>expect &&
+	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
+'
+
+test_expect_success 'name-rev --no-refs clears the refs list' '
+	git rev-list --left-right --cherry-pick F...E -- bar >expect &&
+	git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" --no-refs --refs="*tags/G" \
+		<expect >actual &&
+	test_cmp actual expect
+'
+
 cat >expect <<EOF
 +tags/F
 =tags/D
-- 
2.11.0.403.g196674b8396b
Junio C Hamano· Jan 18, 2017, 20:04 UTC · re: Jacob Keller · lore

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

Jacob Keller <jacob.e.keller@intel.com> writes:
Show 7 quoted lines
> 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.
I think "If a" in the last sentence should be "A".
Show 6 quoted lines
>  --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.

Unlike 1/5, this is facing the end-users, and the last sentence is very appropriate.

Show 23 quoted lines
> +	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;
> +			}
>  		}

I agree that we cannot short-cut on the first match to make sure that the outcome is stable, but I wondered what should be shown when we do have multiple matches. Say I gave

    --refs="v*" --refs="refs/tags/v1.*"

and refs/tags/v1.0 matched. The above code would say we can abbreviate.

What is the reason behind this design decision? Is it because it is clear that the user shows her willingness to accept more compact form by having --refs="v*" that would allow shortening? If that is the case, I think I agree with the reasoning. But we probably want to write it down somewhere, because another reasoning, which may also be valid, would call for an opposite behaviour (i.e. the more specific --refs="refs/tags/v1.*" also matched, so let's show that fact by not shortening).

Jacob Keller· Jan 18, 2017, 21:12 UTC · re: Junio C Hamano · lore

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

On Wed, Jan 18, 2017 at 12:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> 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.
>
> I think "If a" in the last sentence should be "A".
You are correct, that is a typo.
Show 10 quoted lines
>
>>  --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.
>
> Unlike 1/5, this is facing the end-users, and the last sentence is
> very appropriate.
Yes.
Show 43 quoted lines
>
>> +     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;
>> +                     }
>>               }
>
> I agree that we cannot short-cut on the first match to make sure
> that the outcome is stable, but I wondered what should be shown when
> we do have multiple matches.  Say I gave
>
>     --refs="v*" --refs="refs/tags/v1.*"
>
> and refs/tags/v1.0 matched.  The above code would say we can
> abbreviate.
>
> What is the reason behind this design decision?  Is it because it is
> clear that the user shows her willingness to accept more compact
> form by having --refs="v*" that would allow shortening?  If that is
> the case, I think I agree with the reasoning.  But we probably want
> to write it down somewhere, because another reasoning, which may
> also be valid, would call for an opposite behaviour (i.e. the more
> specific --refs="refs/tags/v1.*" also matched, so let's show that
> fact by not shortening).
>
I'm not sure which reasoning makes most sense. Any other opinions?

Thanks, Jake

Junio C Hamano· Jan 18, 2017, 22:42 UTC · re: Jacob Keller · lore

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

Jacob Keller <jacob.keller@gmail.com> writes:
Show 21 quoted lines
> On Wed, Jan 18, 2017 at 12:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> I agree that we cannot short-cut on the first match to make sure
>> that the outcome is stable, but I wondered what should be shown when
>> we do have multiple matches.  Say I gave
>>
>>     --refs="v*" --refs="refs/tags/v1.*"
>>
>> and refs/tags/v1.0 matched.  The above code would say we can
>> abbreviate.
>>
>> What is the reason behind this design decision?  Is it because it is
>> clear that the user shows her willingness to accept more compact
>> form by having --refs="v*" that would allow shortening?  If that is
>> the case, I think I agree with the reasoning.  But we probably want
>> to write it down somewhere, because another reasoning, which may
>> also be valid, would call for an opposite behaviour (i.e. the more
>> specific --refs="refs/tags/v1.*" also matched, so let's show that
>> fact by not shortening).
>
> I'm not sure which reasoning makes most sense. Any other opinions?

FWIW, I do think that the design decision to declare that it can be abbreviated if the ref matches at least one short pattern makes sense, and I am guessing (because you didn't answer when asked what _your_ reasoning behind the code was) that you are in agreement. I just want it to be spelled out probably as in-code comment, so that people who later come to this part of the code know why it was designed that way. And they can disagree and change it if the end result is better---I just want to make sure that they can understand what they are disagreeing when it happens, as opposed to them scratching their head saying "we do not know why it was chosen to be done this way, let's make a random change to make it behave differently".

Jacob Keller· Jan 18, 2017, 00:09 UTC · re: Jacob Keller · lore

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

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(+)
Show changes to Documentation/technical/api-parse-options.txt +5 −0
diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index 27bd701c0d68..6914f54f5f44 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 string argument.
+	The string argument is stored as an element in `&list` which must be a
+	struct string_list. Reset the list using `--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
Junio C Hamano· Jan 18, 2017, 19:45 UTC · re: Jacob Keller · lore

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

Jacob Keller <jacob.e.keller@intel.com> writes:
Show 26 quoted lines
> 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..6914f54f5f44 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 string argument.
> +	The string argument is stored as an element in `&list` which must be a
> +	struct string_list. Reset the list using `--no-option`.
> +

I do not know if it is clear enough that 'option' in the last sentence is a placeholder. I then wondered if spelling it as `--no-<long>` would make it a bit clearer, but that is ugly.

The "Reset the list" is an instruction to the end-users who interact with a program written by readers of this document using OPT_STRING_LIST(), and it feels a bit out of place. Perhaps

	End users can reset the list by negating the option,
	i.e. passing "--no-<long>", on the command line.
I dunno.
Anyway, thanks for adding a missing doc here.
>  `OPT_INTEGER(short, long, &int_var, description)`::
>  	Introduce an option with integer argument.
>  	The integer is put into `int_var`.
Philip Oakley· Jan 18, 2017, 20:16 UTC · re: Junio C Hamano · lore

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

From: "Junio C Hamano" <gitster@pobox.com>
v3 1/5] doc: add documentation for OPT_STRING_LIST
Show 33 quoted lines
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> 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..6914f54f5f44 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 string argument.
>> + The string argument is stored as an element in `&list` which must be a
>> + struct string_list. Reset the list using `--no-option`.
>> +
>
> I do not know if it is clear enough that 'option' in the last
> sentence is a placeholder.  I then wondered if spelling it as
> `--no-<long>` would make it a bit clearer, but that is ugly.

Bikeshedding:: `--no-<option>` maybe, i.e. just surround the option word with the angle brackets to indicate it is to be replaced by the real option's name.

Show 16 quoted lines
>
> The "Reset the list" is an instruction to the end-users who interact
> with a program written by readers of this document using
> OPT_STRING_LIST(), and it feels a bit out of place.  Perhaps
>
> End users can reset the list by negating the option,
> i.e. passing "--no-<long>", on the command line.
>
> I dunno.
>
> Anyway, thanks for adding a missing doc here.
>
>>  `OPT_INTEGER(short, long, &int_var, description)`::
>>  Introduce an option with integer argument.
>>  The integer is put into `int_var`.
>
--
Philip 
Junio C Hamano· Jan 18, 2017, 20:58 UTC · re: Philip Oakley · lore

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

"Philip Oakley" <philipoakley@iee.org> writes:
Show 13 quoted lines
>>> +`OPT_STRING_LIST(short, long, &list, arg_str, description)`::
>>> + Introduce an option with string argument.
>>> + The string argument is stored as an element in `&list` which must be a
>>> + struct string_list. Reset the list using `--no-option`.
>>> +
>>
>> I do not know if it is clear enough that 'option' in the last
>> sentence is a placeholder.  I then wondered if spelling it as
>> `--no-<long>` would make it a bit clearer, but that is ugly.
>
> Bikeshedding:: `--no-<option>` maybe, i.e. just surround the option
> word with the angle brackets to indicate it is to be replaced by the
> real option's name.

Yeah, I bikeshedded that myself, and rejected it because there is no <option> mentioned anywhere in the enumeration header.

Johannes Schindelin· Jan 19, 2017, 16:55 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Wed, 18 Jan 2017, Junio C Hamano wrote:
Show 18 quoted lines
> "Philip Oakley" <philipoakley@iee.org> writes:
> 
> >>> +`OPT_STRING_LIST(short, long, &list, arg_str, description)`::
> >>> + Introduce an option with string argument.
> >>> + The string argument is stored as an element in `&list` which must be a
> >>> + struct string_list. Reset the list using `--no-option`.
> >>> +
> >>
> >> I do not know if it is clear enough that 'option' in the last
> >> sentence is a placeholder.  I then wondered if spelling it as
> >> `--no-<long>` would make it a bit clearer, but that is ugly.
> >
> > Bikeshedding:: `--no-<option>` maybe, i.e. just surround the option
> > word with the angle brackets to indicate it is to be replaced by the
> > real option's name.
> 
> Yeah, I bikeshedded that myself, and rejected it because there is no
> <option> mentioned anywhere in the enumeration header.

As I pointed out in a previous review round: the surrounding test uses --no-option (I agree that it is tedious to go back to the original code for review, rather than a mere patch review that lacks context, but I suspected that Jake did not come up with that `--no-option` himself), so by our own recommendation (imitate the surrounding, existing code/text) Jake did exactly the right thing:

$ git grep -e --no-option upstream/master -- Documentation/technical/api-parse-options.txt upstream/master:Documentation/technical/api-parse-options.txt: `--option` and set to zero with `--no-option`. upstream/master:Documentation/technical/api-parse-options.txt: (even if initially negative), and `--no-option` resets it to upstream/master:Documentation/technical/api-parse-options.txt: zero. To determine if `--option` or `--no-option` was encountered at upstream/master:Documentation/technical/api-parse-options.txt: `--no-option` was seen. upstream/master:Documentation/technical/api-parse-options.txt: reset to zero with `--no-option`.

Ciao, Johannes

Jacob Keller· Jan 18, 2017, 21:10 UTC · re: Junio C Hamano · lore

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

On Wed, Jan 18, 2017 at 11:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
>
> I do not know if it is clear enough that 'option' in the last
> sentence is a placeholder.  I then wondered if spelling it as
> `--no-<long>` would make it a bit clearer, but that is ugly.
>

To be fair, this is exactly how the rest of the doc spells these things, so I would rather be consistent with the doc as is, and a future patch could clean this up. See OPT_SET_INT, for an example of `--no-option`.

Show 8 quoted lines
> The "Reset the list" is an instruction to the end-users who interact
> with a program written by readers of this document using
> OPT_STRING_LIST(), and it feels a bit out of place.  Perhaps
>
>         End users can reset the list by negating the option,
>         i.e. passing "--no-<long>", on the command line.
>
> I dunno.
Maybe we can rephrase this "The list is reset via `--no-option`"?

Thanks, Jake

Junio C Hamano· Jan 19, 2017, 17:58 UTC · re: Jacob Keller · lore

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

Jacob Keller <jacob.keller@gmail.com> writes:
Show 10 quoted lines
> On Wed, Jan 18, 2017 at 11:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> I do not know if it is clear enough that 'option' in the last
>> sentence is a placeholder.  I then wondered if spelling it as
>> `--no-<long>` would make it a bit clearer, but that is ugly.
>
> To be fair, this is exactly how the rest of the doc spells these
> things, so I would rather be consistent with the doc as is, and a
> future patch could clean this up. See OPT_SET_INT, for an example of
> `--no-option`.
Ah, OK, then I have no issues with it.
> Maybe we can rephrase this "The list is reset via `--no-option`"?
I think I saw that in your latest interdiff and I think it is good.
Thanks.
Junio C Hamano· Jan 18, 2017, 20:18 UTC · re: Jacob Keller · lore

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

Jacob Keller <jacob.e.keller@intel.com> writes:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> ** v3 fixes a minor typo in one of the test cases, so please ignore v2
>    I left the interdiff as between v1 and v3 instead of v2 **
Very much appreciated.

I just finished reading this round through and didn't have any major issues. I sent comments on the way the feature is explained to the end users and history readers, though.

Thanks.
Jacob Keller· Jan 18, 2017, 21:06 UTC · re: Junio C Hamano · lore

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

On Wed, Jan 18, 2017 at 12:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> ** v3 fixes a minor typo in one of the test cases, so please ignore v2
>>    I left the interdiff as between v1 and v3 instead of v2 **
>
> Very much appreciated.
>
> I just finished reading this round through and didn't have any major
> issues.  I sent comments on the way the feature is explained to the
> end users and history readers, though.
>
> Thanks.

Ok. I am currently taking a look at the exclude_list that was mentioned, and I will see if it makes sense to go that route. If we do, it would open up a way to make the git-describe logic similar to git-log (which is probably good), and if we want to do this, it's best to do it now before we add any additional arguments (ie: before we create a conflicting user interface).

Thanks, Jake

← back to recent threads