[PATCH] ref-filter: allow merged and no-merged filters

[PATCH] ref-filter: allow merged and no-merged filters

[Aaron Lipman]

Enable ref-filter to process multiple merged and no-merged filters, and
extend functionality to git branch, git tag and git for-each-ref. This
provides an easy way to check for branches that are "graduation
candidates:"

$ git branch --no-merged master --merged next

To accomplish this, store the merged and no-merged filters in two
commit_list structs (reachable_from and unreachable_from) rather than
using a single commit struct (merge_commit).

If passed more than one merged (or more than one no-merged) filter,
mirror the existing boolean logic used by contains/no-contains filters:
refs must satisfy any of the merged filters, and all of the no-merged
filters.

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
This functionality was originally proposed in 2013:
https://lore.kernel.org/git/87fvzwmp23.fsf@59A2.org/T/

In deciding how to handle multiple merged or multiple no-merged
filters, I mirrored the existing behavior for contains/no-contains
filters. However, I noticed this existing behavior isn't documented or
covered by tests. I think it makes sense to hold off on officially
documenting this behavior for now in case we wish to refine it later,
but I'm curious what others think.

If merging into next, please be sure to also include my previous
patch correcting tests in t3200:
https://lore.kernel.org/git/20200830224200.21103-1-alipman88@gmail.com

(Apologies, realizing I should have submitted both together.)


 Documentation/git-branch.txt       |  6 +--
 Documentation/git-for-each-ref.txt |  6 +--
 Documentation/git-tag.txt          |  4 +-
 builtin/branch.c                   |  4 +-
 builtin/tag.c                      |  6 +--
 ref-filter.c                       | 71 ++++++++++++++++--------------
 ref-filter.h                       |  9 +---
 t/t3200-branch.sh                  |  4 +-
 t/t3201-branch-contains.sh         | 12 +++++
 t/t6302-for-each-ref-filter.sh     |  4 +-
 t/t7004-tag.sh                     |  2 +-
 11 files changed, 68 insertions(+), 60 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 03c0824d52..8f0dbcd0ac 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -252,13 +252,11 @@ start-point is either a local or remote-tracking branch.
 
 --merged [<commit>]::
 	Only list branches whose tips are reachable from the
-	specified commit (HEAD if not specified). Implies `--list`,
-	incompatible with `--no-merged`.
+	specified commit (HEAD if not specified). Implies `--list`.
 
 --no-merged [<commit>]::
 	Only list branches whose tips are not reachable from the
-	specified commit (HEAD if not specified). Implies `--list`,
-	incompatible with `--merged`.
+	specified commit (HEAD if not specified). Implies `--list`.
 
 <branchname>::
 	The name of the branch to create or delete.
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 2ea71c5f6c..bb113da5a2 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -76,13 +76,11 @@ OPTIONS
 
 --merged[=<object>]::
 	Only list refs whose tips are reachable from the
-	specified commit (HEAD if not specified),
-	incompatible with `--no-merged`.
+	specified commit (HEAD if not specified).
 
 --no-merged[=<object>]::
 	Only list refs whose tips are not reachable from the
-	specified commit (HEAD if not specified),
-	incompatible with `--merged`.
+	specified commit (HEAD if not specified).
 
 --contains[=<object>]::
 	Only list refs which contain the specified commit (HEAD if not
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index f6d9791780..786d4dfd6f 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -149,11 +149,11 @@ This option is only applicable when listing tags without annotation lines.
 
 --merged [<commit>]::
 	Only list tags whose commits are reachable from the specified
-	commit (`HEAD` if not specified), incompatible with `--no-merged`.
+	commit (`HEAD` if not specified).
 
 --no-merged [<commit>]::
 	Only list tags whose commits are not reachable from the specified
-	commit (`HEAD` if not specified), incompatible with `--merged`.
+	commit (`HEAD` if not specified).
 
 --points-at <object>::
 	Only list tags of the given object (HEAD if not
diff --git a/builtin/branch.c b/builtin/branch.c
index e82301fb1b..4bdb700dd5 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -688,8 +688,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	    !show_current && !unset_upstream && argc == 0)
 		list = 1;
 
-	if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr ||
-	    filter.no_commit)
+	if (filter.with_commit || filter.no_commit ||
+	    filter.reachable_from || filter.unreachable_from || filter.points_at.nr)
 		list = 1;
 
 	if (!!delete + !!rename + !!copy + !!new_upstream + !!show_current +
diff --git a/builtin/tag.c b/builtin/tag.c
index 5cbd80dc3e..b1a0398c85 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -457,8 +457,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		if (argc == 0)
 			cmdmode = 'l';
 		else if (filter.with_commit || filter.no_commit ||
-			 filter.points_at.nr || filter.merge_commit ||
-			 filter.lines != -1)
+			 filter.reachable_from || filter.unreachable_from ||
+			 filter.points_at.nr || filter.lines != -1)
 			cmdmode = 'l';
 	}
 
@@ -509,7 +509,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		die(_("--no-contains option is only allowed in list mode"));
 	if (filter.points_at.nr)
 		die(_("--points-at option is only allowed in list mode"));
-	if (filter.merge_commit)
+	if (filter.reachable_from || filter.unreachable_from)
 		die(_("--merged and --no-merged options are only allowed in list mode"));
 	if (cmdmode == 'd')
 		return for_each_tag_name(argv, delete_tag, NULL);
diff --git a/ref-filter.c b/ref-filter.c
index 8ba0e31915..21dca5e68b 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2119,9 +2119,9 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * obtain the commit using the 'oid' available and discard all
 	 * non-commits early. The actual filtering is done later.
 	 */
-	if (filter->merge_commit || filter->with_commit || filter->no_commit || filter->verbose) {
-		commit = lookup_commit_reference_gently(the_repository, oid,
-							1);
+	if (filter->reachable_from || filter->unreachable_from ||
+	    filter->with_commit || filter->no_commit || filter->verbose) {
+		commit = lookup_commit_reference_gently(the_repository, oid, 1);
 		if (!commit)
 			return 0;
 		/* We perform the filtering for the '--contains' option... */
@@ -2183,11 +2183,17 @@ void ref_array_clear(struct ref_array *array)
 	}
 }
 
-static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
+static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)
 {
+	struct commit_list *check_reachable_list = reachable ?
+		ref_cbdata->filter->reachable_from :
+		ref_cbdata->filter->unreachable_from;
+
+	if (!check_reachable_list)
+		return;
+
 	struct rev_info revs;
 	int i, old_nr;
-	struct ref_filter *filter = ref_cbdata->filter;
 	struct ref_array *array = ref_cbdata->array;
 	struct commit **to_clear = xcalloc(sizeof(struct commit *), array->nr);
 
@@ -2199,12 +2205,15 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
 		to_clear[i] = item->commit;
 	}
 
-	filter->merge_commit->object.flags |= UNINTERESTING;
-	add_pending_object(&revs, &filter->merge_commit->object, "");
+	for (struct commit_list *rl = check_reachable_list; rl; rl = rl->next) {
+		struct commit *merge_commit = rl->item;
+		merge_commit->object.flags |= UNINTERESTING;
+		add_pending_object(&revs, &merge_commit->object, "");
 
-	revs.limited = 1;
-	if (prepare_revision_walk(&revs))
-		die(_("revision walk setup failed"));
+		revs.limited = 1;
+		if (prepare_revision_walk(&revs))
+			die(_("revision walk setup failed"));
+	}
 
 	old_nr = array->nr;
 	array->nr = 0;
@@ -2215,14 +2224,19 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
 
 		int is_merged = !!(commit->object.flags & UNINTERESTING);
 
-		if (is_merged == (filter->merge == REF_FILTER_MERGED_INCLUDE))
+		if (is_merged == reachable)
 			array->items[array->nr++] = array->items[i];
 		else
 			free_array_item(item);
-	}
+  }
 
 	clear_commit_marks_many(old_nr, to_clear, ALL_REV_FLAGS);
-	clear_commit_marks(filter->merge_commit, ALL_REV_FLAGS);
+
+	while (check_reachable_list) {
+		struct commit *merge_commit = pop_commit(&check_reachable_list);
+		clear_commit_marks(merge_commit, ALL_REV_FLAGS);
+	}
+
 	free(to_clear);
 }
 
@@ -2274,8 +2288,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 	clear_contains_cache(&ref_cbdata.no_contains_cache);
 
 	/*  Filters that need revision walking */
-	if (filter->merge_commit)
-		do_merge_filter(&ref_cbdata);
+	do_merge_filter(&ref_cbdata, 1);
+	do_merge_filter(&ref_cbdata, 0);
 
 	return ret;
 }
@@ -2493,31 +2507,22 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
 {
 	struct ref_filter *rf = opt->value;
 	struct object_id oid;
-	int no_merged = starts_with(opt->long_name, "no");
 
 	BUG_ON_OPT_NEG(unset);
 
-	if (rf->merge) {
-		if (no_merged) {
-			return error(_("option `%s' is incompatible with --merged"),
-				     opt->long_name);
-		} else {
-			return error(_("option `%s' is incompatible with --no-merged"),
-				     opt->long_name);
-		}
-	}
-
-	rf->merge = no_merged
-		? REF_FILTER_MERGED_OMIT
-		: REF_FILTER_MERGED_INCLUDE;
-
 	if (get_oid(arg, &oid))
 		die(_("malformed object name %s"), arg);
 
-	rf->merge_commit = lookup_commit_reference_gently(the_repository,
-							  &oid, 0);
-	if (!rf->merge_commit)
+	struct commit *merge_commit = lookup_commit_reference_gently(the_repository,
+								     &oid, 0);
+
+	if (!merge_commit)
 		return error(_("option `%s' must point to a commit"), opt->long_name);
 
+	if (starts_with(opt->long_name, "no"))
+		commit_list_insert(merge_commit, &rf->unreachable_from);
+	else
+		commit_list_insert(merge_commit, &rf->reachable_from);
+
 	return 0;
 }
diff --git a/ref-filter.h b/ref-filter.h
index 8ecc33cdfa..feaef4a8fd 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -54,13 +54,8 @@ struct ref_filter {
 	struct oid_array points_at;
 	struct commit_list *with_commit;
 	struct commit_list *no_commit;
-
-	enum {
-		REF_FILTER_MERGED_NONE = 0,
-		REF_FILTER_MERGED_INCLUDE,
-		REF_FILTER_MERGED_OMIT
-	} merge;
-	struct commit *merge_commit;
+	struct commit_list *reachable_from;
+	struct commit_list *unreachable_from;
 
 	unsigned int with_commit_tag_algo : 1,
 		match_as_path : 1,
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 028c88d1b2..be8f61b751 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1299,8 +1299,8 @@ test_expect_success '--merged catches invalid object names' '
 	test_must_fail git branch --merged 0000000000000000000000000000000000000000
 '
 
-test_expect_success '--merged is incompatible with --no-merged' '
-	test_must_fail git branch --merged HEAD --no-merged HEAD
+test_expect_success '--merged is compatible with --no-merged' '
+	git branch --merged master --no-merged master
 '
 
 test_expect_success '--list during rebase' '
diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh
index 40251c9f8f..6e73789995 100755
--- a/t/t3201-branch-contains.sh
+++ b/t/t3201-branch-contains.sh
@@ -211,4 +211,16 @@ test_expect_success 'branch --contains combined with --no-contains' '
 
 '
 
+test_expect_success 'branch --merged combined with --no-merged' '
+	git checkout master &&
+	git checkout -b next &&
+	git merge side &&
+	git branch --merged next --no-merged master >actual &&
+	cat >expect <<-\EOF &&
+	* next
+	  side
+	EOF
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 35408d53fd..781e470aea 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -437,8 +437,8 @@ test_expect_success 'check %(if:notequals=<string>)' '
 	test_cmp expect actual
 '
 
-test_expect_success '--merged is incompatible with --no-merged' '
-	test_must_fail git for-each-ref --merged HEAD --no-merged HEAD
+test_expect_success '--merged is compatible with --no-merged' '
+	git for-each-ref --merged HEAD --no-merged HEAD
 '
 
 test_expect_success 'validate worktree atom' '
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 74b637deb2..7d544eceda 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -2016,7 +2016,7 @@ test_expect_success '--merged can be used in non-list mode' '
 '
 
 test_expect_success '--merged is incompatible with --no-merged' '
-	test_must_fail git tag --merged HEAD --no-merged HEAD
+	git tag --merged HEAD --no-merged HEAD
 '
 
 test_expect_success '--merged shows merged tags' '
-- 
2.24.3 (Apple Git-128)

Re: [PATCH] ref-filter: allow merged and no-merged filters

[Junio C Hamano]

Aaron Lipman <alipman88@gmail.com> writes:

> Enable ref-filter to process multiple merged and no-merged filters, and
> extend functionality to git branch, git tag and git for-each-ref. This
> provides an easy way to check for branches that are "graduation
> candidates:"
>
> $ git branch --no-merged master --merged next
>
> To accomplish this, store the merged and no-merged filters in two
> commit_list structs (reachable_from and unreachable_from) rather than
> using a single commit struct (merge_commit).
>
> If passed more than one merged (or more than one no-merged) filter,
> mirror the existing boolean logic used by contains/no-contains filters:
> refs must satisfy any of the merged filters, and all of the no-merged
> filters.

I am not sure if the parallel with "contains" should hold in the
first place.

The way I would look at the --[no-]merged is that I'm enumerating
commits that would be shown in this corresponding revision walking
command:

	$ git log --decorate --oneline ^master ^maint next seen

If the tip of a branch appears in that output, then the branch is
included in the corresponding "git branch" output, and if it does
not, the branch is excluded from the corresponding "git branch"
output.  

	$ git branch \
		--no-merged master --no-merged maint \
		--merged next --merged seen

That would mean the rule would be "refs must be reachable by any one
(or more) of the <merged> commits, and must be reachable by none of
the <no-merged> commits".  I am not using the same phrasing you used
(i.e. "must satisify ... filter"), but are we saying the same thing?
It is unclear to me what you meant by "all of the no-merged filters".

The expectation is that topics in flight are either reachable from
'next' or 'seen' (there can be commits in 'next' but not in 'seen'
when fixes to mismerges are involved) and those already graduated
are either in 'master' or 'maint', and the above "log" and "branch"
would show the stuff still in flight.  But if you mean by "all of
the no-merged filters" that it must be in both 'master' and 'maint'
for a commit to be excluded from the output, it would not behave as
I would expect.

In _my_ history, since 'maint' is always kept as a subset to
'master' because I refrain from cherry-picking a fix that is in
'master' down to 'maint', I can use a single "--no-merged master" to
work around the problem, but in a larger scale projects where
cherry-picking fixes to maintainance track is more common, the above
does not sound so useful, at least at the first glance.

So I dunno.

Thanks.

Re: [PATCH] ref-filter: allow merged and no-merged filters

[Aaron Lipman]

> That would mean the rule would be "refs must be reachable by any one
> (or more) of the <merged> commits, and must be reachable by none of
> the <no-merged> commits".  I am not using the same phrasing you used
> (i.e. "must satisify ... filter"), but are we saying the same thing?

Yes, that is how I've implemented this. (Your wording may be more
clear, I'll plan on updating the commit message.)

> The expectation is that topics in flight are either reachable from
> 'next' or 'seen' (there can be commits in 'next' but not in 'seen'
> when fixes to mismerges are involved) and those already graduated
> are either in 'master' or 'maint', and the above "log" and "branch"
> would show the stuff still in flight.

I think we're on the same page: If a branch is merged into 'seen' but
not 'next', it should show up in the output of
"git branch --merged seen --merged next".

If a branch is merged into 'master' but not 'maint', it should not
show up in the output of
"git branch --no-merged master --no-merged maint".

To clarify, is that the behavior that makes sense/seems useful to you?
If so, I can add some test cases for those filter combinations in v2.

Re: [PATCH] ref-filter: allow merged and no-merged filters

[Junio C Hamano]

Aaron Lipman <alipman88@gmail.com> writes:

>> That would mean the rule would be "refs must be reachable by any one
>> (or more) of the <merged> commits, and must be reachable by none of
>> the <no-merged> commits".  I am not using the same phrasing you used
>> (i.e. "must satisify ... filter"), but are we saying the same thing?
>
> Yes, that is how I've implemented this. (Your wording may be more
> clear, I'll plan on updating the commit message.)
>
>> The expectation is that topics in flight are either reachable from
>> 'next' or 'seen' (there can be commits in 'next' but not in 'seen'
>> when fixes to mismerges are involved) and those already graduated
>> are either in 'master' or 'maint', and the above "log" and "branch"
>> would show the stuff still in flight.
>
> I think we're on the same page: If a branch is merged into 'seen' but
> not 'next', it should show up in the output of
> "git branch --merged seen --merged next".
>
> If a branch is merged into 'master' but not 'maint', it should not
> show up in the output of
> "git branch --no-merged master --no-merged maint".
>
> To clarify, is that the behavior that makes sense/seems useful to you?
> If so, I can add some test cases for those filter combinations in v2.

Yup, drawing the parallel to how revision walker combines multiple
positive and negative starting points would feel the most natural to
experienced Git users, I would say.

Thanks.

[PATCH v2 0/2] git branch: allow combining merged and no-merged filters

[Aaron Lipman]

Enable ref-filter to process multiple merged and no-merged filters,
and extend functionality to git branch, git tag and git
for-each-ref. This provides an easy way to check for branches that
are "graduation candidates:"

$ git branch --merged next --no-merged master

Currently, this required a command like:

$ grep -xf \
  <(git branch --merged next) \
  <(git branch --no-merged master)

Also, add tests describing git branch's behavior when combining
multiple contains/no-contains filters - this helps demonstrate
consistency between merged/no-merged and contains/no-contains.

Aaron Lipman (2):
  t3201: test multiple branch filter combinations
  ref-filter: allow merged and no-merged filters

 Documentation/git-branch.txt       |  6 +--
 Documentation/git-for-each-ref.txt |  6 +--
 Documentation/git-tag.txt          |  4 +-
 builtin/branch.c                   |  4 +-
 builtin/tag.c                      |  6 +--
 ref-filter.c                       | 71 ++++++++++++++++--------------
 ref-filter.h                       |  9 +---
 t/t3200-branch.sh                  |  4 +-
 t/t3201-branch-contains.sh         | 69 +++++++++++++++++++++++++++--
 t/t6302-for-each-ref-filter.sh     |  4 +-
 t/t7004-tag.sh                     |  2 +-
 11 files changed, 122 insertions(+), 63 deletions(-)

-- 
2.24.3 (Apple Git-128)

[PATCH v2 1/2] t3201: test multiple branch filter combinations

[Aaron Lipman]

Add tests covering the behavior of passing multiple contains/no-contains
filters to git branch, e.g.:

$ git branch --contains feature_a --contains feature_b
$ git branch --no-contains feature_a --no-contains feature_b

When passed more than one contains (or no-contains) filter, the tips of
the branches returned must be reachable from any of the contains commits
and from none of the the no-contains commits. (This logic is useful to
describe prior to enabling multiple merged/no-merged filters, so that
future tests will demonstrate consistent behavior between
merged/no-merged and contains/no-contains filters.)

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 t/t3201-branch-contains.sh | 44 ++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh
index 40251c9f8f..cd205b5560 100755
--- a/t/t3201-branch-contains.sh
+++ b/t/t3201-branch-contains.sh
@@ -200,15 +200,51 @@ test_expect_success 'branch --merged with --verbose' '
 	test_i18ncmp expect actual
 '
 
-test_expect_success 'branch --contains combined with --no-contains' '
-	git branch --contains zzz --no-contains topic >actual &&
+# The next series of tests covers multiple filter combinations
+test_expect_success 'set up repo for multiple filter combination tests' '
+	git checkout master &&
+	git branch | grep -v master | xargs git branch -D &&
+	git checkout -b feature_a master &&
+	>feature_a &&
+	git add feature_a &&
+	git commit -m "add feature a" &&
+	git checkout -b feature_b master &&
+	>feature_b &&
+	git add feature_b &&
+	git commit -m "add feature b"
+'
+
+test_expect_success 'multiple branch --contains' '
+	git checkout -b next master &&
+	git merge feature_a &&
+	git branch --contains feature_a --contains feature_b >actual &&
+	cat >expect <<-\EOF &&
+	  feature_a
+	  feature_b
+	* next
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'multiple branch --no-contains' '
+	git branch --no-contains feature_a --no-contains feature_b >actual &&
 	cat >expect <<-\EOF &&
 	  master
-	  side
-	  zzz
 	EOF
 	test_cmp expect actual
+'
 
+test_expect_success 'branch --contains combined with --no-contains' '
+	git checkout master &&
+	git merge feature_a &&
+	git checkout next &&
+	git merge feature_b &&
+	git branch --contains feature_a --no-contains feature_b >actual &&
+	cat >expect <<-\EOF &&
+	  feature_a
+	  master
+	EOF
+	test_cmp expect actual
 '
 
 test_done
-- 
2.24.3 (Apple Git-128)

[PATCH v2 2/2] ref-filter: allow merged and no-merged filters

[Aaron Lipman]

Enable ref-filter to process multiple merged and no-merged filters, and
extend functionality to git branch, git tag and git for-each-ref. This
provides an easy way to check for branches that are "graduation
candidates:"

$ git branch --merged next --no-merged master

To accomplish this, store the merged and no-merged filters in two
commit_list structs (reachable_from and unreachable_from) rather than
using a single commit struct (merge_commit).

If passed more than one merged (or more than one no-merged) filter, refs
must be reachable from any one of the merged commits, and reachable from
none of the no-merged commits.

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 Documentation/git-branch.txt       |  6 +--
 Documentation/git-for-each-ref.txt |  6 +--
 Documentation/git-tag.txt          |  4 +-
 builtin/branch.c                   |  4 +-
 builtin/tag.c                      |  6 +--
 ref-filter.c                       | 71 ++++++++++++++++--------------
 ref-filter.h                       |  9 +---
 t/t3200-branch.sh                  |  4 +-
 t/t3201-branch-contains.sh         | 27 ++++++++++++
 t/t6302-for-each-ref-filter.sh     |  4 +-
 t/t7004-tag.sh                     |  2 +-
 11 files changed, 83 insertions(+), 60 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 03c0824d52..8f0dbcd0ac 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -252,13 +252,11 @@ start-point is either a local or remote-tracking branch.
 
 --merged [<commit>]::
 	Only list branches whose tips are reachable from the
-	specified commit (HEAD if not specified). Implies `--list`,
-	incompatible with `--no-merged`.
+	specified commit (HEAD if not specified). Implies `--list`.
 
 --no-merged [<commit>]::
 	Only list branches whose tips are not reachable from the
-	specified commit (HEAD if not specified). Implies `--list`,
-	incompatible with `--merged`.
+	specified commit (HEAD if not specified). Implies `--list`.
 
 <branchname>::
 	The name of the branch to create or delete.
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 616ce46087..f8322a7be2 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -76,13 +76,11 @@ OPTIONS
 
 --merged[=<object>]::
 	Only list refs whose tips are reachable from the
-	specified commit (HEAD if not specified),
-	incompatible with `--no-merged`.
+	specified commit (HEAD if not specified).
 
 --no-merged[=<object>]::
 	Only list refs whose tips are not reachable from the
-	specified commit (HEAD if not specified),
-	incompatible with `--merged`.
+	specified commit (HEAD if not specified).
 
 --contains[=<object>]::
 	Only list refs which contain the specified commit (HEAD if not
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index f6d9791780..786d4dfd6f 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -149,11 +149,11 @@ This option is only applicable when listing tags without annotation lines.
 
 --merged [<commit>]::
 	Only list tags whose commits are reachable from the specified
-	commit (`HEAD` if not specified), incompatible with `--no-merged`.
+	commit (`HEAD` if not specified).
 
 --no-merged [<commit>]::
 	Only list tags whose commits are not reachable from the specified
-	commit (`HEAD` if not specified), incompatible with `--merged`.
+	commit (`HEAD` if not specified).
 
 --points-at <object>::
 	Only list tags of the given object (HEAD if not
diff --git a/builtin/branch.c b/builtin/branch.c
index e82301fb1b..4bdb700dd5 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -688,8 +688,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	    !show_current && !unset_upstream && argc == 0)
 		list = 1;
 
-	if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr ||
-	    filter.no_commit)
+	if (filter.with_commit || filter.no_commit ||
+	    filter.reachable_from || filter.unreachable_from || filter.points_at.nr)
 		list = 1;
 
 	if (!!delete + !!rename + !!copy + !!new_upstream + !!show_current +
diff --git a/builtin/tag.c b/builtin/tag.c
index 5cbd80dc3e..b1a0398c85 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -457,8 +457,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		if (argc == 0)
 			cmdmode = 'l';
 		else if (filter.with_commit || filter.no_commit ||
-			 filter.points_at.nr || filter.merge_commit ||
-			 filter.lines != -1)
+			 filter.reachable_from || filter.unreachable_from ||
+			 filter.points_at.nr || filter.lines != -1)
 			cmdmode = 'l';
 	}
 
@@ -509,7 +509,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		die(_("--no-contains option is only allowed in list mode"));
 	if (filter.points_at.nr)
 		die(_("--points-at option is only allowed in list mode"));
-	if (filter.merge_commit)
+	if (filter.reachable_from || filter.unreachable_from)
 		die(_("--merged and --no-merged options are only allowed in list mode"));
 	if (cmdmode == 'd')
 		return for_each_tag_name(argv, delete_tag, NULL);
diff --git a/ref-filter.c b/ref-filter.c
index 110bcd741a..c04dca47d1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2167,9 +2167,9 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * obtain the commit using the 'oid' available and discard all
 	 * non-commits early. The actual filtering is done later.
 	 */
-	if (filter->merge_commit || filter->with_commit || filter->no_commit || filter->verbose) {
-		commit = lookup_commit_reference_gently(the_repository, oid,
-							1);
+	if (filter->reachable_from || filter->unreachable_from ||
+	    filter->with_commit || filter->no_commit || filter->verbose) {
+		commit = lookup_commit_reference_gently(the_repository, oid, 1);
 		if (!commit)
 			return 0;
 		/* We perform the filtering for the '--contains' option... */
@@ -2231,11 +2231,17 @@ void ref_array_clear(struct ref_array *array)
 	}
 }
 
-static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
+static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)
 {
+	struct commit_list *check_reachable_list = reachable ?
+		ref_cbdata->filter->reachable_from :
+		ref_cbdata->filter->unreachable_from;
+
+	if (!check_reachable_list)
+		return;
+
 	struct rev_info revs;
 	int i, old_nr;
-	struct ref_filter *filter = ref_cbdata->filter;
 	struct ref_array *array = ref_cbdata->array;
 	struct commit **to_clear = xcalloc(sizeof(struct commit *), array->nr);
 
@@ -2247,12 +2253,15 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
 		to_clear[i] = item->commit;
 	}
 
-	filter->merge_commit->object.flags |= UNINTERESTING;
-	add_pending_object(&revs, &filter->merge_commit->object, "");
+	for (struct commit_list *rl = check_reachable_list; rl; rl = rl->next) {
+		struct commit *merge_commit = rl->item;
+		merge_commit->object.flags |= UNINTERESTING;
+		add_pending_object(&revs, &merge_commit->object, "");
 
-	revs.limited = 1;
-	if (prepare_revision_walk(&revs))
-		die(_("revision walk setup failed"));
+		revs.limited = 1;
+		if (prepare_revision_walk(&revs))
+			die(_("revision walk setup failed"));
+	}
 
 	old_nr = array->nr;
 	array->nr = 0;
@@ -2263,14 +2272,19 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
 
 		int is_merged = !!(commit->object.flags & UNINTERESTING);
 
-		if (is_merged == (filter->merge == REF_FILTER_MERGED_INCLUDE))
+		if (is_merged == reachable)
 			array->items[array->nr++] = array->items[i];
 		else
 			free_array_item(item);
-	}
+  }
 
 	clear_commit_marks_many(old_nr, to_clear, ALL_REV_FLAGS);
-	clear_commit_marks(filter->merge_commit, ALL_REV_FLAGS);
+
+	while (check_reachable_list) {
+		struct commit *merge_commit = pop_commit(&check_reachable_list);
+		clear_commit_marks(merge_commit, ALL_REV_FLAGS);
+	}
+
 	free(to_clear);
 }
 
@@ -2322,8 +2336,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 	clear_contains_cache(&ref_cbdata.no_contains_cache);
 
 	/*  Filters that need revision walking */
-	if (filter->merge_commit)
-		do_merge_filter(&ref_cbdata);
+	do_merge_filter(&ref_cbdata, 1);
+	do_merge_filter(&ref_cbdata, 0);
 
 	return ret;
 }
@@ -2541,31 +2555,22 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
 {
 	struct ref_filter *rf = opt->value;
 	struct object_id oid;
-	int no_merged = starts_with(opt->long_name, "no");
 
 	BUG_ON_OPT_NEG(unset);
 
-	if (rf->merge) {
-		if (no_merged) {
-			return error(_("option `%s' is incompatible with --merged"),
-				     opt->long_name);
-		} else {
-			return error(_("option `%s' is incompatible with --no-merged"),
-				     opt->long_name);
-		}
-	}
-
-	rf->merge = no_merged
-		? REF_FILTER_MERGED_OMIT
-		: REF_FILTER_MERGED_INCLUDE;
-
 	if (get_oid(arg, &oid))
 		die(_("malformed object name %s"), arg);
 
-	rf->merge_commit = lookup_commit_reference_gently(the_repository,
-							  &oid, 0);
-	if (!rf->merge_commit)
+	struct commit *merge_commit = lookup_commit_reference_gently(the_repository,
+								     &oid, 0);
+
+	if (!merge_commit)
 		return error(_("option `%s' must point to a commit"), opt->long_name);
 
+	if (starts_with(opt->long_name, "no"))
+		commit_list_insert(merge_commit, &rf->unreachable_from);
+	else
+		commit_list_insert(merge_commit, &rf->reachable_from);
+
 	return 0;
 }
diff --git a/ref-filter.h b/ref-filter.h
index 8ecc33cdfa..feaef4a8fd 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -54,13 +54,8 @@ struct ref_filter {
 	struct oid_array points_at;
 	struct commit_list *with_commit;
 	struct commit_list *no_commit;
-
-	enum {
-		REF_FILTER_MERGED_NONE = 0,
-		REF_FILTER_MERGED_INCLUDE,
-		REF_FILTER_MERGED_OMIT
-	} merge;
-	struct commit *merge_commit;
+	struct commit_list *reachable_from;
+	struct commit_list *unreachable_from;
 
 	unsigned int with_commit_tag_algo : 1,
 		match_as_path : 1,
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 028c88d1b2..be8f61b751 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1299,8 +1299,8 @@ test_expect_success '--merged catches invalid object names' '
 	test_must_fail git branch --merged 0000000000000000000000000000000000000000
 '
 
-test_expect_success '--merged is incompatible with --no-merged' '
-	test_must_fail git branch --merged HEAD --no-merged HEAD
+test_expect_success '--merged is compatible with --no-merged' '
+	git branch --merged master --no-merged master
 '
 
 test_expect_success '--list during rebase' '
diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh
index cd205b5560..d1ec594f6a 100755
--- a/t/t3201-branch-contains.sh
+++ b/t/t3201-branch-contains.sh
@@ -226,6 +226,16 @@ test_expect_success 'multiple branch --contains' '
 	test_cmp expect actual
 '
 
+test_expect_success 'multiple branch --merged' '
+	git branch --merged next --merged master >actual &&
+	cat >expect <<-\EOF &&
+	  feature_a
+	  master
+	* next
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'multiple branch --no-contains' '
 	git branch --no-contains feature_a --no-contains feature_b >actual &&
 	cat >expect <<-\EOF &&
@@ -234,6 +244,14 @@ test_expect_success 'multiple branch --no-contains' '
 	test_cmp expect actual
 '
 
+test_expect_success 'multiple branch --no-merged' '
+	git branch --no-merged next --no-merged master >actual &&
+	cat >expect <<-\EOF &&
+	  feature_b
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'branch --contains combined with --no-contains' '
 	git checkout master &&
 	git merge feature_a &&
@@ -247,4 +265,13 @@ test_expect_success 'branch --contains combined with --no-contains' '
 	test_cmp expect actual
 '
 
+test_expect_success 'branch --merged combined with --no-merged' '
+	git branch --merged next --no-merged master >actual &&
+	cat >expect <<-\EOF &&
+	  feature_b
+	* next
+	EOF
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 35408d53fd..781e470aea 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -437,8 +437,8 @@ test_expect_success 'check %(if:notequals=<string>)' '
 	test_cmp expect actual
 '
 
-test_expect_success '--merged is incompatible with --no-merged' '
-	test_must_fail git for-each-ref --merged HEAD --no-merged HEAD
+test_expect_success '--merged is compatible with --no-merged' '
+	git for-each-ref --merged HEAD --no-merged HEAD
 '
 
 test_expect_success 'validate worktree atom' '
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 74b637deb2..7d544eceda 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -2016,7 +2016,7 @@ test_expect_success '--merged can be used in non-list mode' '
 '
 
 test_expect_success '--merged is incompatible with --no-merged' '
-	test_must_fail git tag --merged HEAD --no-merged HEAD
+	git tag --merged HEAD --no-merged HEAD
 '
 
 test_expect_success '--merged shows merged tags' '
-- 
2.24.3 (Apple Git-128)

Re: [PATCH v2 2/2] ref-filter: allow merged and no-merged filters

[Junio C Hamano]

Aaron Lipman <alipman88@gmail.com> writes:

> If passed more than one merged (or more than one no-merged) filter, refs
> must be reachable from any one of the merged commits, and reachable from
> none of the no-merged commits.

This mirrors how the "contains" behaves, which is good.

>  Documentation/git-branch.txt       |  6 +--
>  Documentation/git-for-each-ref.txt |  6 +--
>  Documentation/git-tag.txt          |  4 +-

It is a bit sad that this only removes from documentation without
adding (the removal is because merged and no-merged are no longer
mutually exclusive).  There should be a new description on how it
behaves when both of them are given---the combination were impossible
so it was sufficient to say "they are incompatible", but now the user
needs to know how they interact with each other.

Perhaps --contains side can already be combined so they have
description we can borrow here?  I didn't look too carefully.

> diff --git a/ref-filter.c b/ref-filter.c
> index 110bcd741a..c04dca47d1 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2231,11 +2231,17 @@ void ref_array_clear(struct ref_array *array)
>  	}
>  }
>  
> -static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
> +static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)
>  {
> +	struct commit_list *check_reachable_list = reachable ?
> +		ref_cbdata->filter->reachable_from :
> +		ref_cbdata->filter->unreachable_from;
> +
> +	if (!check_reachable_list)
> +		return;
> +

We do not allow decl-after-statement.

>  	struct rev_info revs;
>  	int i, old_nr;
> -	struct ref_filter *filter = ref_cbdata->filter;
>  	struct ref_array *array = ref_cbdata->array;
>  	struct commit **to_clear = xcalloc(sizeof(struct commit *), array->nr);
>  
> @@ -2247,12 +2253,15 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
>  		to_clear[i] = item->commit;
>  	}
>  
> -	filter->merge_commit->object.flags |= UNINTERESTING;
> -	add_pending_object(&revs, &filter->merge_commit->object, "");
> +	for (struct commit_list *rl = check_reachable_list; rl; rl = rl->next) {
> +		struct commit *merge_commit = rl->item;
> +		merge_commit->object.flags |= UNINTERESTING;
> +		add_pending_object(&revs, &merge_commit->object, "");
> -	revs.limited = 1;
> -	if (prepare_revision_walk(&revs))
> -		die(_("revision walk setup failed"));
> +		revs.limited = 1;
> +		if (prepare_revision_walk(&revs))
> +			die(_("revision walk setup failed"));
> +	}

This looks wrong.  I would have expected, instead of placing the
single object to the pending queue, the loop places all the filter
objects in a queue, and then makes a limited revision walk just
once.  In general, each time after the code makes a call to
prepare_revision_walk() to perform a revision walk, before doing so
again, the object flags used for the walking must be cleared.  You
can tell that the original code structure follows the pattern.
Calling it inside a loop breaks the pattern a big time.

In any case, we mark the named commits as UNINTERESTING and walk
from them and the tips of (surviving) refs to see which refs are
reachable from _any_ of the named commits.  The original code is a
degenerated case of having only one named commit.

The idea of the original is when checking for "--merged", any ref
whose tip is reachable from the named commit is merged and should be
shown.  That extends naturally to multiple commits given to
"--merged"; any ref whose tip is reachable from one of the named
commits is selected.

For "--no-merged", any ref whose tip is reachable from the named
commit is merged and should be excluded.  That also extends to
multiple commits.  Any ref whose tip is reachable from one of the
"--no-merged" commit is rejected.

> @@ -2263,14 +2272,19 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
>  
>  		int is_merged = !!(commit->object.flags & UNINTERESTING);
>  
> -		if (is_merged == (filter->merge == REF_FILTER_MERGED_INCLUDE))
> +		if (is_merged == reachable)
>  			array->items[array->nr++] = array->items[i];
>  		else
>  			free_array_item(item);

So, in the original code, if the tip of the ref is reachable from
the given commit (i.e. painted UNINTERESTING), we keep it in the
array when looking for "--merged".  That corresponds to the case 
where reachable is true.  Similarly for the --no-merged case.

So the overall logic seems sound.  It's just the callsite of
prepare_revision_walk() which does the actual painting of the graph
looks wrong.

> @@ -2541,31 +2555,22 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
>  {
>  	struct ref_filter *rf = opt->value;
>  	struct object_id oid;
> -	int no_merged = starts_with(opt->long_name, "no");
>  
>  	BUG_ON_OPT_NEG(unset);
>  
> -	if (rf->merge) {
> -		if (no_merged) {
> -			return error(_("option `%s' is incompatible with --merged"),
> -				     opt->long_name);
> -		} else {
> -			return error(_("option `%s' is incompatible with --no-merged"),
> -				     opt->long_name);
> -		}
> -	}
> -
> -	rf->merge = no_merged
> -		? REF_FILTER_MERGED_OMIT
> -		: REF_FILTER_MERGED_INCLUDE;
> -
>  	if (get_oid(arg, &oid))
>  		die(_("malformed object name %s"), arg);
>  
> -	rf->merge_commit = lookup_commit_reference_gently(the_repository,
> -							  &oid, 0);
> -	if (!rf->merge_commit)
> +	struct commit *merge_commit = lookup_commit_reference_gently(the_repository,
> +								     &oid, 0);

decl-after-statement here, too.

> +	if (!merge_commit)
>  		return error(_("option `%s' must point to a commit"), opt->long_name);
>  
> +	if (starts_with(opt->long_name, "no"))
> +		commit_list_insert(merge_commit, &rf->unreachable_from);
> +	else
> +		commit_list_insert(merge_commit, &rf->reachable_from);
> +
>  	return 0;
>  }

Thanks.

[PATCH v3 0/3] git branch: allow combining merged and no-merged filters

[Aaron Lipman]

> It is a bit sad that this only removes from documentation without
> adding (the removal is because merged and no-merged are no longer
> mutually exclusive)...
>
> Perhaps --contains side can already be combined so they have
> description we can borrow here?  I didn't look too carefully.

Sounds good, I've added documentation covering both multiple contains/
no-contains and merged/no-merged filters to the notes sections of the
doc files for branch, for-each-ref & tag. (There was no existing
documentation for --contains, so I added that in commit 2/3.)

> We do not allow decl-after-statement.

Got it, fixed both locations.

> I would have expected, instead of placing the
> single object to the pending queue, the loop places all the filter
> objects in a queue, and then makes a limited revision walk just
> once.  In general, each time after the code makes a call to
> prepare_revision_walk() to perform a revision walk, before doing so
> again, the object flags used for the walking must be cleared.

Thanks for the explanation - updated accordingly. I'll plan on
studying that code as part of better familiarizing myself with
git's codebase.

Aaron Lipman (3):
  t3201: test multiple branch filter combinations
  Doc: cover multiple contains/no-contains filters
  ref-filter: allow merged and no-merged filters

 Documentation/git-branch.txt       | 17 +++++---
 Documentation/git-for-each-ref.txt | 20 ++++++---
 Documentation/git-tag.txt          | 17 ++++++--
 builtin/branch.c                   |  6 +--
 builtin/for-each-ref.c             |  2 +-
 builtin/tag.c                      |  8 ++--
 ref-filter.c                       | 64 ++++++++++++++-------------
 ref-filter.h                       |  9 +---
 t/t3200-branch.sh                  |  4 +-
 t/t3201-branch-contains.sh         | 69 ++++++++++++++++++++++++++++--
 t/t6302-for-each-ref-filter.sh     |  4 +-
 t/t7004-tag.sh                     |  2 +-
 12 files changed, 157 insertions(+), 65 deletions(-)

-- 
2.24.3 (Apple Git-128)

[PATCH v3 1/3] t3201: test multiple branch filter combinations

[Aaron Lipman]

Add tests covering the behavior of passing multiple contains/no-contains
filters to git branch, e.g.:

$ git branch --contains feature_a --contains feature_b
$ git branch --no-contains feature_a --no-contains feature_b

When passed more than one contains (or no-contains) filter, the tips of
the branches returned must be reachable from any of the contains commits
and from none of the no-contains commits.

This logic is useful to describe prior to enabling multiple
merged/no-merged filters, so that future tests will demonstrate
consistent behavior between merged/no-merged and contains/no-contains
filters.

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 t/t3201-branch-contains.sh | 44 ++++++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh
index 40251c9f8f..cd205b5560 100755
--- a/t/t3201-branch-contains.sh
+++ b/t/t3201-branch-contains.sh
@@ -200,15 +200,51 @@ test_expect_success 'branch --merged with --verbose' '
 	test_i18ncmp expect actual
 '
 
-test_expect_success 'branch --contains combined with --no-contains' '
-	git branch --contains zzz --no-contains topic >actual &&
+# The next series of tests covers multiple filter combinations
+test_expect_success 'set up repo for multiple filter combination tests' '
+	git checkout master &&
+	git branch | grep -v master | xargs git branch -D &&
+	git checkout -b feature_a master &&
+	>feature_a &&
+	git add feature_a &&
+	git commit -m "add feature a" &&
+	git checkout -b feature_b master &&
+	>feature_b &&
+	git add feature_b &&
+	git commit -m "add feature b"
+'
+
+test_expect_success 'multiple branch --contains' '
+	git checkout -b next master &&
+	git merge feature_a &&
+	git branch --contains feature_a --contains feature_b >actual &&
+	cat >expect <<-\EOF &&
+	  feature_a
+	  feature_b
+	* next
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'multiple branch --no-contains' '
+	git branch --no-contains feature_a --no-contains feature_b >actual &&
 	cat >expect <<-\EOF &&
 	  master
-	  side
-	  zzz
 	EOF
 	test_cmp expect actual
+'
 
+test_expect_success 'branch --contains combined with --no-contains' '
+	git checkout master &&
+	git merge feature_a &&
+	git checkout next &&
+	git merge feature_b &&
+	git branch --contains feature_a --no-contains feature_b >actual &&
+	cat >expect <<-\EOF &&
+	  feature_a
+	  master
+	EOF
+	test_cmp expect actual
 '
 
 test_done
-- 
2.24.3 (Apple Git-128)

Re: [PATCH v3 1/3] t3201: test multiple branch filter combinations

[Eric Sunshine]

On Sun, Sep 13, 2020 at 3:32 PM Aaron Lipman <alipman88@gmail.com> wrote:
> Add tests covering the behavior of passing multiple contains/no-contains
> filters to git branch, e.g.:
> ---
> diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh
> @@ -200,15 +200,51 @@ test_expect_success 'branch --merged with --verbose' '
> +# The next series of tests covers multiple filter combinations
> +test_expect_success 'set up repo for multiple filter combination tests' '
> +       git checkout master &&
> +       git branch | grep -v master | xargs git branch -D &&
> +       git checkout -b feature_a master &&
> +       >feature_a &&
> +       git add feature_a &&
> +       git commit -m "add feature a" &&
> +       git checkout -b feature_b master &&
> +       >feature_b &&
> +       git add feature_b &&
> +       git commit -m "add feature b"
> +'

A few comments:

I didn't examine it too closely, so this may be a silly question, but
is there a reason to start from scratch (by deleting all the branches)
rather than simply using or extending the existing branches like the
other tests do?

If it really does make sense to start from scratch (ignoring the
existing branches), then an alternative would be to create a new
repository and run the tests in that repository instead. Whether or
not doing so makes sense in this case is a judgment call. For
instance:

    test_create_repo features
    (
        cd features
        ...setup stuff...
    )

It's a bit concerning to see output from porcelain git-branch being
fed to 'grep' and 'xargs'. More typically, you would instead rely upon
the (stable) output of a plumbing command. For instance:

    git for-each-ref --format="%(refname:short)" refs/heads/ | ...

In new test code, normally avoid having a Git command upstream of a
pipe since its exit code will be lost. Thus, you might instead write:

    git for-each-ref ... >heads &&
    grep -v master heads | xargs git branch -D &&

Re: [PATCH v3 1/3] t3201: test multiple branch filter combinations

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> A few comments:
>
> I didn't examine it too closely, so this may be a silly question, but
> is there a reason to start from scratch (by deleting all the branches)
> rather than simply using or extending the existing branches like the
> other tests do?
>
> If it really does make sense to start from scratch (ignoring the
> existing branches), then an alternative would be to create a new
> repository and run the tests in that repository instead. Whether or
> not doing so makes sense in this case is a judgment call. For
> instance:
>
>     test_create_repo features
>     (
>         cd features
>         ...setup stuff...
>     )

Good comments; I agree with both.

> It's a bit concerning to see output from porcelain git-branch being
> fed to 'grep' and 'xargs'. More typically, you would instead rely upon
> the (stable) output of a plumbing command. For instance:
>
>     git for-each-ref --format="%(refname:short)" refs/heads/ | ...
>
> In new test code, normally avoid having a Git command upstream of a
> pipe since its exit code will be lost. Thus, you might instead write:
>
>     git for-each-ref ... >heads &&
>     grep -v master heads | xargs git branch -D &&

Again, good recommendation.

Thank you always for helpful reviews.

[PATCH v3 2/3] Doc: cover multiple contains/no-contains filters

[Aaron Lipman]

Update documentation for "git branch", "git for-each-ref" and "git tag"
with notes explaining what happens when passed multiple --contains or
--no-contains filters, e.g.:

  When combining multiple `--contains` and `--no-contains` filters,
  `git for-each-ref` shows refs containing at least one of the named
  `--contains` commits and none of the named `--no-contains` commits.

This behavior is useful to document prior to enabling multiple
merged/no-merged filters, in order to demonstrate consistent behavior
between merged/no-merged and contains/no-contains filters.
---
 Documentation/git-branch.txt       | 4 ++++
 Documentation/git-for-each-ref.txt | 7 +++++++
 Documentation/git-tag.txt          | 7 +++++++
 3 files changed, 18 insertions(+)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 03c0824d52..2a36d29929 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -370,6 +370,10 @@ serve four related but different purposes:
 - `--no-merged` is used to find branches which are candidates for merging
   into HEAD, since those branches are not fully contained by HEAD.
 
+When combining multiple `--contains` and `--no-contains` filters,
+`git branch` shows branches containing at least one of the named
+`--contains` commits and none of the named `--no-contains` commits.
+
 SEE ALSO
 --------
 linkgit:git-check-ref-format[1],
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 616ce46087..e5f0226273 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -408,6 +408,13 @@ Note also that multiple copies of an object may be present in the object
 database; in this case, it is undefined which copy's size or delta base
 will be reported.
 
+NOTES
+-----
+
+When combining multiple `--contains` and `--no-contains` filters,
+`git for-each-ref` shows refs containing at least one of the named
+`--contains` commits and none of the named `--no-contains` commits.
+
 SEE ALSO
 --------
 linkgit:git-show-ref[1]
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index f6d9791780..adfeab7a93 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -377,6 +377,13 @@ $ GIT_COMMITTER_DATE="2006-10-02 10:31" git tag -s v1.0.1
 
 include::date-formats.txt[]
 
+NOTES
+-----
+
+When combining multiple `--contains` and `--no-contains` filters,
+`git tag` shows tags containing at least one of the named `--contains`
+commits and none of the named `--no-contains` commits.
+
 SEE ALSO
 --------
 linkgit:git-check-ref-format[1].
-- 
2.24.3 (Apple Git-128)

Re: [PATCH v3 2/3] Doc: cover multiple contains/no-contains filters

[Eric Sunshine]

On Sun, Sep 13, 2020 at 3:32 PM Aaron Lipman <alipman88@gmail.com> wrote:
> Update documentation for "git branch", "git for-each-ref" and "git tag"
> with notes explaining what happens when passed multiple --contains or
> --no-contains filters, e.g.:
>
>   When combining multiple `--contains` and `--no-contains` filters,
>   `git for-each-ref` shows refs containing at least one of the named
>   `--contains` commits and none of the named `--no-contains` commits.

We normally avoid repeating in the commit message what the patch
itself already says. The first paragraph alone (without the example
text) would be plenty sufficient. (Not itself worth a re-roll,
though.)

> This behavior is useful to document prior to enabling multiple
> merged/no-merged filters, in order to demonstrate consistent behavior
> between merged/no-merged and contains/no-contains filters.
> ---

Missing sign-off.

> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
> @@ -370,6 +370,10 @@ serve four related but different purposes:
> +When combining multiple `--contains` and `--no-contains` filters,
> +`git branch` shows branches containing at least one of the named
> +`--contains` commits and none of the named `--no-contains` commits.

This is repeated nearly verbatim in the other two documentation pages.
It makes one wonder if it can be generalized and placed in its own
file which is included in the other documents via
`include::contains.txt[]`. Perhaps like this:

    When combining multiple `--contains` and `--no-contains` filters,
    `git branch` shows references containing at least one of the named
    `--contains` commits and none of the named `--no-contains`
    commits.

But perhaps that's too generic?

Re: [PATCH v3 2/3] Doc: cover multiple contains/no-contains filters

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
>> @@ -370,6 +370,10 @@ serve four related but different purposes:
>> +When combining multiple `--contains` and `--no-contains` filters,
>> +`git branch` shows branches containing at least one of the named
>> +`--contains` commits and none of the named `--no-contains` commits.
>
> This is repeated nearly verbatim in the other two documentation pages.
> It makes one wonder if it can be generalized and placed in its own
> file which is included in the other documents via
> `include::contains.txt[]`. Perhaps like this:
>
>     When combining multiple `--contains` and `--no-contains` filters,
>     `git branch` shows references containing at least one of the named
>     `--contains` commits and none of the named `--no-contains`
>     commits.
>
> But perhaps that's too generic?

Replace `git branch` with "this command" and make it even more generic?
Or eliminate the subject by going passive, e.g.

    Only references that contain at least one of the '--contains'
    commits but contain none of the '--no-contains' commits are
    shown

or somesuch?

[PATCH v3 3/3] ref-filter: allow merged and no-merged filters

[Aaron Lipman]

Enable ref-filter to process multiple merged and no-merged filters, and
extend functionality to git branch, git tag and git for-each-ref. This
provides an easy way to check for branches that are "graduation
candidates:"

$ git branch --no-merged master --merged next

To accomplish this, store the merged and no-merged filters in two
commit_list structs (reachable_from and unreachable_from) rather than
using a single commit struct (merge_commit).

If passed more than one merged (or more than one no-merged) filter, refs
must be reachable from any one of the merged commits, and reachable from
none of the no-merged commits.

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 Documentation/git-branch.txt       | 13 +++---
 Documentation/git-for-each-ref.txt | 13 +++---
 Documentation/git-tag.txt          | 10 +++--
 builtin/branch.c                   |  6 +--
 builtin/for-each-ref.c             |  2 +-
 builtin/tag.c                      |  8 ++--
 ref-filter.c                       | 64 ++++++++++++++++--------------
 ref-filter.h                       |  9 +----
 t/t3200-branch.sh                  |  4 +-
 t/t3201-branch-contains.sh         | 27 +++++++++++++
 t/t6302-for-each-ref-filter.sh     |  4 +-
 t/t7004-tag.sh                     |  2 +-
 12 files changed, 100 insertions(+), 62 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 2a36d29929..25992791fb 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git branch' [--color[=<when>] | --no-color] [--show-current]
 	[-v [--abbrev=<length> | --no-abbrev]]
 	[--column[=<options>] | --no-column] [--sort=<key>]
-	[(--merged | --no-merged) [<commit>]]
+	[--merged [<commit>]] [--no-merged [<commit>]]
 	[--contains [<commit>]] [--no-contains [<commit>]]
 	[--points-at <object>] [--format=<format>]
 	[(-r | --remotes) | (-a | --all)]
@@ -252,13 +252,11 @@ start-point is either a local or remote-tracking branch.
 
 --merged [<commit>]::
 	Only list branches whose tips are reachable from the
-	specified commit (HEAD if not specified). Implies `--list`,
-	incompatible with `--no-merged`.
+	specified commit (HEAD if not specified). Implies `--list`.
 
 --no-merged [<commit>]::
 	Only list branches whose tips are not reachable from the
-	specified commit (HEAD if not specified). Implies `--list`,
-	incompatible with `--merged`.
+	specified commit (HEAD if not specified). Implies `--list`.
 
 <branchname>::
 	The name of the branch to create or delete.
@@ -374,6 +372,11 @@ When combining multiple `--contains` and `--no-contains` filters,
 `git branch` shows branches containing at least one of the named
 `--contains` commits and none of the named `--no-contains` commits.
 
+When combining multiple `--merged` and `--no-merged` filters,
+`git branch` shows branches whose tips are reachable from at least one
+of the named `--merged` commits and none of the named `--no-merged`
+commits.
+
 SEE ALSO
 --------
 linkgit:git-check-ref-format[1],
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index e5f0226273..a82716419d 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git for-each-ref' [--count=<count>] [--shell|--perl|--python|--tcl]
 		   [(--sort=<key>)...] [--format=<format>] [<pattern>...]
 		   [--points-at=<object>]
-		   (--merged[=<object>] | --no-merged[=<object>])
+		   [--merged[=<object>]] [--no-merged[=<object>]]
 		   [--contains[=<object>]] [--no-contains[=<object>]]
 
 DESCRIPTION
@@ -76,13 +76,11 @@ OPTIONS
 
 --merged[=<object>]::
 	Only list refs whose tips are reachable from the
-	specified commit (HEAD if not specified),
-	incompatible with `--no-merged`.
+	specified commit (HEAD if not specified).
 
 --no-merged[=<object>]::
 	Only list refs whose tips are not reachable from the
-	specified commit (HEAD if not specified),
-	incompatible with `--merged`.
+	specified commit (HEAD if not specified).
 
 --contains[=<object>]::
 	Only list refs which contain the specified commit (HEAD if not
@@ -415,6 +413,11 @@ When combining multiple `--contains` and `--no-contains` filters,
 `git for-each-ref` shows refs containing at least one of the named
 `--contains` commits and none of the named `--no-contains` commits.
 
+When combining multiple `--merged` and `--no-merged` filters,
+`git for-each-ref` shows refs that are reachable from at least one of
+the named `--merged` commits and none of the named `--no-merged`
+commits.
+
 SEE ALSO
 --------
 linkgit:git-show-ref[1]
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index adfeab7a93..84408784e6 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -15,7 +15,7 @@ SYNOPSIS
 'git tag' [-n[<num>]] -l [--contains <commit>] [--no-contains <commit>]
 	[--points-at <object>] [--column[=<options>] | --no-column]
 	[--create-reflog] [--sort=<key>] [--format=<format>]
-	[--[no-]merged [<commit>]] [<pattern>...]
+	[--merged <commit>] [--no-merged <commit>] [<pattern>...]
 'git tag' -v [--format=<format>] <tagname>...
 
 DESCRIPTION
@@ -149,11 +149,11 @@ This option is only applicable when listing tags without annotation lines.
 
 --merged [<commit>]::
 	Only list tags whose commits are reachable from the specified
-	commit (`HEAD` if not specified), incompatible with `--no-merged`.
+	commit (`HEAD` if not specified).
 
 --no-merged [<commit>]::
 	Only list tags whose commits are not reachable from the specified
-	commit (`HEAD` if not specified), incompatible with `--merged`.
+	commit (`HEAD` if not specified).
 
 --points-at <object>::
 	Only list tags of the given object (HEAD if not
@@ -384,6 +384,10 @@ When combining multiple `--contains` and `--no-contains` filters,
 `git tag` shows tags containing at least one of the named `--contains`
 commits and none of the named `--no-contains` commits.
 
+When combining multiple `--merged` and `--no-merged` filters, `git tag`
+shows tags whose commits are reachable from at least one of the named
+`--merged` commits and none of the named `--no-merged` commits.
+
 SEE ALSO
 --------
 linkgit:git-check-ref-format[1].
diff --git a/builtin/branch.c b/builtin/branch.c
index e82301fb1b..efb30b8820 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -26,7 +26,7 @@
 #include "commit-reach.h"
 
 static const char * const builtin_branch_usage[] = {
-	N_("git branch [<options>] [-r | -a] [--merged | --no-merged]"),
+	N_("git branch [<options>] [-r | -a] [--merged] [--no-merged]"),
 	N_("git branch [<options>] [-l] [-f] <branch-name> [<start-point>]"),
 	N_("git branch [<options>] [-r] (-d | -D) <branch-name>..."),
 	N_("git branch [<options>] (-m | -M) [<old-branch>] <new-branch>"),
@@ -688,8 +688,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	    !show_current && !unset_upstream && argc == 0)
 		list = 1;
 
-	if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr ||
-	    filter.no_commit)
+	if (filter.with_commit || filter.no_commit ||
+	    filter.reachable_from || filter.unreachable_from || filter.points_at.nr)
 		list = 1;
 
 	if (!!delete + !!rename + !!copy + !!new_upstream + !!show_current +
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 57489e4eab..9d1ecda2b8 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -9,7 +9,7 @@
 static char const * const for_each_ref_usage[] = {
 	N_("git for-each-ref [<options>] [<pattern>]"),
 	N_("git for-each-ref [--points-at <object>]"),
-	N_("git for-each-ref [(--merged | --no-merged) [<commit>]]"),
+	N_("git for-each-ref [--merged [<commit>]] [--no-merged [<commit>]]"),
 	N_("git for-each-ref [--contains [<commit>]] [--no-contains [<commit>]]"),
 	NULL
 };
diff --git a/builtin/tag.c b/builtin/tag.c
index 5cbd80dc3e..ecf011776d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -26,7 +26,7 @@ static const char * const git_tag_usage[] = {
 		"\t\t<tagname> [<head>]"),
 	N_("git tag -d <tagname>..."),
 	N_("git tag -l [-n[<num>]] [--contains <commit>] [--no-contains <commit>] [--points-at <object>]\n"
-		"\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"),
+		"\t\t[--format=<format>] [--merged <commit>] [--no-merged <commit>] [<pattern>...]"),
 	N_("git tag -v [--format=<format>] <tagname>..."),
 	NULL
 };
@@ -457,8 +457,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		if (argc == 0)
 			cmdmode = 'l';
 		else if (filter.with_commit || filter.no_commit ||
-			 filter.points_at.nr || filter.merge_commit ||
-			 filter.lines != -1)
+			 filter.reachable_from || filter.unreachable_from ||
+			 filter.points_at.nr || filter.lines != -1)
 			cmdmode = 'l';
 	}
 
@@ -509,7 +509,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		die(_("--no-contains option is only allowed in list mode"));
 	if (filter.points_at.nr)
 		die(_("--points-at option is only allowed in list mode"));
-	if (filter.merge_commit)
+	if (filter.reachable_from || filter.unreachable_from)
 		die(_("--merged and --no-merged options are only allowed in list mode"));
 	if (cmdmode == 'd')
 		return for_each_tag_name(argv, delete_tag, NULL);
diff --git a/ref-filter.c b/ref-filter.c
index 110bcd741a..030ab55d03 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2167,9 +2167,9 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * obtain the commit using the 'oid' available and discard all
 	 * non-commits early. The actual filtering is done later.
 	 */
-	if (filter->merge_commit || filter->with_commit || filter->no_commit || filter->verbose) {
-		commit = lookup_commit_reference_gently(the_repository, oid,
-							1);
+	if (filter->reachable_from || filter->unreachable_from ||
+	    filter->with_commit || filter->no_commit || filter->verbose) {
+		commit = lookup_commit_reference_gently(the_repository, oid, 1);
 		if (!commit)
 			return 0;
 		/* We perform the filtering for the '--contains' option... */
@@ -2231,13 +2231,20 @@ void ref_array_clear(struct ref_array *array)
 	}
 }
 
-static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
+static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)
 {
 	struct rev_info revs;
 	int i, old_nr;
-	struct ref_filter *filter = ref_cbdata->filter;
 	struct ref_array *array = ref_cbdata->array;
 	struct commit **to_clear = xcalloc(sizeof(struct commit *), array->nr);
+	struct commit_list *rl;
+
+	struct commit_list *check_reachable_list = reachable ?
+		ref_cbdata->filter->reachable_from :
+		ref_cbdata->filter->unreachable_from;
+
+	if (!check_reachable_list)
+		return;
 
 	repo_init_revisions(the_repository, &revs, NULL);
 
@@ -2247,8 +2254,11 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
 		to_clear[i] = item->commit;
 	}
 
-	filter->merge_commit->object.flags |= UNINTERESTING;
-	add_pending_object(&revs, &filter->merge_commit->object, "");
+	for (rl = check_reachable_list; rl; rl = rl->next) {
+		struct commit *merge_commit = rl->item;
+		merge_commit->object.flags |= UNINTERESTING;
+		add_pending_object(&revs, &merge_commit->object, "");
+	}
 
 	revs.limited = 1;
 	if (prepare_revision_walk(&revs))
@@ -2263,14 +2273,19 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
 
 		int is_merged = !!(commit->object.flags & UNINTERESTING);
 
-		if (is_merged == (filter->merge == REF_FILTER_MERGED_INCLUDE))
+		if (is_merged == reachable)
 			array->items[array->nr++] = array->items[i];
 		else
 			free_array_item(item);
 	}
 
 	clear_commit_marks_many(old_nr, to_clear, ALL_REV_FLAGS);
-	clear_commit_marks(filter->merge_commit, ALL_REV_FLAGS);
+
+	while (check_reachable_list) {
+		struct commit *merge_commit = pop_commit(&check_reachable_list);
+		clear_commit_marks(merge_commit, ALL_REV_FLAGS);
+	}
+
 	free(to_clear);
 }
 
@@ -2322,8 +2337,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 	clear_contains_cache(&ref_cbdata.no_contains_cache);
 
 	/*  Filters that need revision walking */
-	if (filter->merge_commit)
-		do_merge_filter(&ref_cbdata);
+	do_merge_filter(&ref_cbdata, 1);
+	do_merge_filter(&ref_cbdata, 0);
 
 	return ret;
 }
@@ -2541,31 +2556,22 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
 {
 	struct ref_filter *rf = opt->value;
 	struct object_id oid;
-	int no_merged = starts_with(opt->long_name, "no");
+	struct commit *merge_commit;
 
 	BUG_ON_OPT_NEG(unset);
 
-	if (rf->merge) {
-		if (no_merged) {
-			return error(_("option `%s' is incompatible with --merged"),
-				     opt->long_name);
-		} else {
-			return error(_("option `%s' is incompatible with --no-merged"),
-				     opt->long_name);
-		}
-	}
-
-	rf->merge = no_merged
-		? REF_FILTER_MERGED_OMIT
-		: REF_FILTER_MERGED_INCLUDE;
-
 	if (get_oid(arg, &oid))
 		die(_("malformed object name %s"), arg);
 
-	rf->merge_commit = lookup_commit_reference_gently(the_repository,
-							  &oid, 0);
-	if (!rf->merge_commit)
+	merge_commit = lookup_commit_reference_gently(the_repository, &oid, 0);
+
+	if (!merge_commit)
 		return error(_("option `%s' must point to a commit"), opt->long_name);
 
+	if (starts_with(opt->long_name, "no"))
+		commit_list_insert(merge_commit, &rf->unreachable_from);
+	else
+		commit_list_insert(merge_commit, &rf->reachable_from);
+
 	return 0;
 }
diff --git a/ref-filter.h b/ref-filter.h
index 8ecc33cdfa..feaef4a8fd 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -54,13 +54,8 @@ struct ref_filter {
 	struct oid_array points_at;
 	struct commit_list *with_commit;
 	struct commit_list *no_commit;
-
-	enum {
-		REF_FILTER_MERGED_NONE = 0,
-		REF_FILTER_MERGED_INCLUDE,
-		REF_FILTER_MERGED_OMIT
-	} merge;
-	struct commit *merge_commit;
+	struct commit_list *reachable_from;
+	struct commit_list *unreachable_from;
 
 	unsigned int with_commit_tag_algo : 1,
 		match_as_path : 1,
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 028c88d1b2..be8f61b751 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1299,8 +1299,8 @@ test_expect_success '--merged catches invalid object names' '
 	test_must_fail git branch --merged 0000000000000000000000000000000000000000
 '
 
-test_expect_success '--merged is incompatible with --no-merged' '
-	test_must_fail git branch --merged HEAD --no-merged HEAD
+test_expect_success '--merged is compatible with --no-merged' '
+	git branch --merged master --no-merged master
 '
 
 test_expect_success '--list during rebase' '
diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh
index cd205b5560..d1ec594f6a 100755
--- a/t/t3201-branch-contains.sh
+++ b/t/t3201-branch-contains.sh
@@ -226,6 +226,16 @@ test_expect_success 'multiple branch --contains' '
 	test_cmp expect actual
 '
 
+test_expect_success 'multiple branch --merged' '
+	git branch --merged next --merged master >actual &&
+	cat >expect <<-\EOF &&
+	  feature_a
+	  master
+	* next
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'multiple branch --no-contains' '
 	git branch --no-contains feature_a --no-contains feature_b >actual &&
 	cat >expect <<-\EOF &&
@@ -234,6 +244,14 @@ test_expect_success 'multiple branch --no-contains' '
 	test_cmp expect actual
 '
 
+test_expect_success 'multiple branch --no-merged' '
+	git branch --no-merged next --no-merged master >actual &&
+	cat >expect <<-\EOF &&
+	  feature_b
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'branch --contains combined with --no-contains' '
 	git checkout master &&
 	git merge feature_a &&
@@ -247,4 +265,13 @@ test_expect_success 'branch --contains combined with --no-contains' '
 	test_cmp expect actual
 '
 
+test_expect_success 'branch --merged combined with --no-merged' '
+	git branch --merged next --no-merged master >actual &&
+	cat >expect <<-\EOF &&
+	  feature_b
+	* next
+	EOF
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 35408d53fd..781e470aea 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -437,8 +437,8 @@ test_expect_success 'check %(if:notequals=<string>)' '
 	test_cmp expect actual
 '
 
-test_expect_success '--merged is incompatible with --no-merged' '
-	test_must_fail git for-each-ref --merged HEAD --no-merged HEAD
+test_expect_success '--merged is compatible with --no-merged' '
+	git for-each-ref --merged HEAD --no-merged HEAD
 '
 
 test_expect_success 'validate worktree atom' '
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 74b637deb2..7d544eceda 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -2016,7 +2016,7 @@ test_expect_success '--merged can be used in non-list mode' '
 '
 
 test_expect_success '--merged is incompatible with --no-merged' '
-	test_must_fail git tag --merged HEAD --no-merged HEAD
+	git tag --merged HEAD --no-merged HEAD
 '
 
 test_expect_success '--merged shows merged tags' '
-- 
2.24.3 (Apple Git-128)

Re: [PATCH v3 3/3] ref-filter: allow merged and no-merged filters

[Eric Sunshine]

On Sun, Sep 13, 2020 at 3:32 PM Aaron Lipman <alipman88@gmail.com> wrote:
> Enable ref-filter to process multiple merged and no-merged filters, and
> extend functionality to git branch, git tag and git for-each-ref. This
> provides an easy way to check for branches that are "graduation
> candidates:"
>
> $ git branch --no-merged master --merged next

A few subjective comments below, not necessarily worth a re-roll...

> To accomplish this, store the merged and no-merged filters in two
> commit_list structs (reachable_from and unreachable_from) rather than
> using a single commit struct (merge_commit).

This sort of implementation detail is readily discoverable by reading
the patch itself, and since there is no complexity about it which
requires extensive explanation, we'd normally leave it out of the
commit message.

> If passed more than one merged (or more than one no-merged) filter, refs
> must be reachable from any one of the merged commits, and reachable from
> none of the no-merged commits.
>
> Signed-off-by: Aaron Lipman <alipman88@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -2231,13 +2231,20 @@ void ref_array_clear(struct ref_array *array)
> -static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
> +static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)
>  {
>         [...]
> +       struct commit_list *check_reachable_list = reachable ?
> +               ref_cbdata->filter->reachable_from :
> +               ref_cbdata->filter->unreachable_from;
> +
> +       if (!check_reachable_list)
> +               return;

Rather than adding a boolean 'reachable' parameter to the function
signature, you could instead directly pass in the `struct commit_list
*` upon which to operate, which would allow you to drop the ternary
operator here, and...

> @@ -2322,8 +2337,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
> -       if (filter->merge_commit)
> -               do_merge_filter(&ref_cbdata);
> +       do_merge_filter(&ref_cbdata, 1);
> +       do_merge_filter(&ref_cbdata, 0);

... make the callers a bit less opaque by eliminating the
less-than-meaningful 0-or-1, and making it obvious which list is being
consulted:

    do_merge_filter(&ref_cbdata, ref_cbdata->filter->reachable_from);
    do_merge_filter(&ref_cbdata, ref_cbdata->filter->unreachable_from);

> @@ -2541,31 +2556,22 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
> +       if (starts_with(opt->long_name, "no"))
> +               commit_list_insert(merge_commit, &rf->unreachable_from);
> +       else
> +               commit_list_insert(merge_commit, &rf->reachable_from);

... quite like it's obvious here into which list the item is being inserted.

> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> @@ -1299,8 +1299,8 @@ test_expect_success '--merged catches invalid object names' '
> -test_expect_success '--merged is incompatible with --no-merged' '
> -       test_must_fail git branch --merged HEAD --no-merged HEAD
> +test_expect_success '--merged is compatible with --no-merged' '
> +       git branch --merged master --no-merged master
>  '

This revised test doesn't seem to have all that much value since this
combination is checked by new tests added elsewhere by the patch.
Therefore, another option would be simply to drop the test rather than
revising it. (But, again, it's a judgment call.)

> diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh
> @@ -226,6 +226,16 @@ test_expect_success 'multiple branch --contains' '
> +test_expect_success 'multiple branch --merged' '
> +test_expect_success 'multiple branch --no-merged' '
> +test_expect_success 'branch --merged combined with --no-merged' '

Would it make sense to also test multiple --merged and multiple
--no-merged? (Genuine question, not a demand to add more tests.)

> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> @@ -2016,7 +2016,7 @@ test_expect_success '--merged can be used in non-list mode' '
>  test_expect_success '--merged is incompatible with --no-merged' '
> -       test_must_fail git tag --merged HEAD --no-merged HEAD
> +       git tag --merged HEAD --no-merged HEAD
>  '

I think you forgot s/incompatible/compatible/ in the test title (which
you changed in the other cases).

Re: [PATCH v3 3/3] ref-filter: allow merged and no-merged filters

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +       struct commit_list *check_reachable_list = reachable ?
>> +               ref_cbdata->filter->reachable_from :
>> +               ref_cbdata->filter->unreachable_from;
>> +
>> +       if (!check_reachable_list)
>> +               return;
>
> Rather than adding a boolean 'reachable' parameter to the function
> signature, you could instead directly pass in the `struct commit_list
> *` upon which to operate, which would allow you to drop the ternary
> operator here, and...
>
>> @@ -2322,8 +2337,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
>> -       if (filter->merge_commit)
>> -               do_merge_filter(&ref_cbdata);
>> +       do_merge_filter(&ref_cbdata, 1);
>> +       do_merge_filter(&ref_cbdata, 0);
>
> ... make the callers a bit less opaque by eliminating the
> less-than-meaningful 0-or-1, and making it obvious which list is being
> consulted:
>
>     do_merge_filter(&ref_cbdata, ref_cbdata->filter->reachable_from);
>     do_merge_filter(&ref_cbdata, ref_cbdata->filter->unreachable_from);


There is this code in do_merge_filter(), though.

		if (is_merged == reachable)
 			array->items[array->nr++] = array->items[i];
 		else
 			free_array_item(item);

This is a body of the loop that runs over (surviving) tips of ref,
after painting the commits from the tips of refs (interesting) and
the [un]reachable_from commits (uninteresting).  The temporary
variable is_merged signals if the tip after revision walk is painted
uninteresting, i.e. some of the [un]reachable_from commits reach the
tip.

If 'reachable' and 'is_merged' are the same, that means either

 (1) the tip is reachable from some commit given as --merged
     <commit>; or

 (2) the tip is NOT reachable from any commit given as --no-merged
     <commit>

which means it survives this round of filtering.

By losing the 'reachable' bit, you make this switch impossible.

I do not mind making the 0/1 a symbolic constant between
do_emreg_filter() and filter_refs() for enhanced readability,
though.

Thanks.

Re: [PATCH v3 3/3] ref-filter: allow merged and no-merged filters

[Eric Sunshine]

On Sun, Sep 13, 2020 at 5:56 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > Rather than adding a boolean 'reachable' parameter to the function
> > signature, you could instead directly pass in the `struct commit_list
> > *` upon which to operate, which would allow you to drop the ternary
> > operator here, and...
>
> There is this code in do_merge_filter(), though.
>
>                 if (is_merged == reachable)
>                         array->items[array->nr++] = array->items[i];
>                 else
>                         free_array_item(item);
>
> By losing the 'reachable' bit, you make this switch impossible.

True. I missed that.

[PATCH v4 0/3] git branch: allow combining merged and no-merged filters

[Aaron Lipman]

Thanks for the review, Eric & Junio.

Eric -

> I didn't examine it too closely, so this may be a silly question, but
> is there a reason to start from scratch (by deleting all the branches)
> rather than simply using or extending the existing branches like the
> other tests do?

I went back and forth on this. There were a couple reasons I leaned
towards starting fresh - I found branch names like feature_a &
feature_b more illustrative, and I didn't want readers to have to
scroll up further to find where branches came from.

But, with the tests re-ordered so "branch --merged with --verbose"
comes last (which adds new branches that might otherwise clutter up
the output of my new tests), I'm happy with using the existing test
setup - rewritten accordingly.

> It's a bit concerning to see output from porcelain git-branch being
> fed to 'grep' and 'xargs'. More typically, you would instead rely upon
> the (stable) output of a plumbing command...

Thanks, useful knowledge for future contributions.

> We normally avoid repeating in the commit message what the patch
> itself already says. The first paragraph alone (without the example
> text) would be plenty sufficient. (Not itself worth a re-roll,
> though.)

Got it, removed.

> Missing sign-off.

Whoops, fixed.

> This is repeated nearly verbatim in the other two documentation pages.
> It makes one wonder if it can be generalized and placed in its own
> file which is included in the other documents via
> `include::contains.txt[]`. Perhaps like this:
> 
>     When combining multiple `--contains` and `--no-contains` filters,
>     `git branch` shows references containing at least one of the named
>     `--contains` commits and none of the named `--no-contains`
>     commits.
> 
> But perhaps that's too generic?

Cool, I hadn't realized we could embed snippets like that. Slightly
generic, but I have no strong opinion either way. Going with the
passive wording provided by Junio.

(Looking at AsciiDoc's documentation, I think we could also set a
:command-name: variable to insert some dynamic content into an
include:: file.)

> This sort of implementation detail is readily discoverable by reading
> the patch itself, and since there is no complexity about it which
> requires extensive explanation, we'd normally leave it out of the
> commit message.

Removed.

> This revised test doesn't seem to have all that much value since this
> combination is checked by new tests added elsewhere by the patch.

Agreed, dropped.

> Would it make sense to also test multiple --merged and multiple
> --no-merged? (Genuine question, not a demand to add more tests.)

I don't see a reason to. The --merged and --no-merged filters are
applied in separate passes, so I feel it's sufficient to test them
independently. (When doing my own QA testing, I did combine multiple
merged & multiple no-merged, multiple contains & multiple no-contains,
merged/no-merged & contains/no-contains, etc.)

On the other hand, extra test cases could help prevent regressions
should someone significantly refactor ref-filter.c. If anyone has a
preference to add more tests, I'm happy to oblige.

> I think you forgot s/incompatible/compatible/ in the test title (which
> you changed in the other cases).

Thanks, fixed.

Junio -

> I do not mind making the 0/1 a symbolic constant between
> do_merge_filter() and filter_refs() for enhanced readability,
> though.

If I understand the convention, the constant names should be prefixed
with DO_MERGE_FILTER. I suggest DO_MERGE_FILTER_REACHABLE and
DO_MERGE_FILTER_UNREACHABLE. Happy to re-roll if others have a
different preference - or feel free to edit.)

Aaron Lipman (3):
  t3201: test multiple branch filter combinations
  Doc: cover multiple contains/no-contains filters
  ref-filter: allow merged and no-merged filters

 Documentation/filters.txt          |  7 +++
 Documentation/git-branch.txt       | 10 ++--
 Documentation/git-for-each-ref.txt | 13 ++++--
 Documentation/git-tag.txt          | 11 +++--
 builtin/branch.c                   |  6 +--
 builtin/for-each-ref.c             |  2 +-
 builtin/tag.c                      |  8 ++--
 ref-filter.c                       | 64 ++++++++++++++------------
 ref-filter.h                       | 12 ++---
 t/t3200-branch.sh                  |  4 --
 t/t3201-branch-contains.sh         | 74 +++++++++++++++++++++++++-----
 t/t6302-for-each-ref-filter.sh     |  4 +-
 t/t7004-tag.sh                     |  4 +-
 13 files changed, 143 insertions(+), 76 deletions(-)
 create mode 100644 Documentation/filters.txt

-- 
2.24.3 (Apple Git-128)

[PATCH v4 1/3] t3201: test multiple branch filter combinations

[Aaron Lipman]

Add tests covering the behavior of passing multiple contains/no-contains
filters to git branch, e.g.:

$ git branch --contains feature_a --contains feature_b
$ git branch --no-contains feature_a --no-contains feature_b

When passed more than one contains (or no-contains) filter, the tips of
the branches returned must be reachable from any of the contains commits
and from none of the the no-contains commits.

This logic is useful to describe prior to enabling multiple
merged/no-merged filters, so that future tests will demonstrate
consistent behavior between merged/no-merged and contains/no-contains
filters.

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 t/t3201-branch-contains.sh | 47 +++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 11 deletions(-)

diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh
index 40251c9f8f..3cb9dc6cca 100755
--- a/t/t3201-branch-contains.sh
+++ b/t/t3201-branch-contains.sh
@@ -171,6 +171,42 @@ test_expect_success 'Assert that --contains only works on commits, not trees & b
 	test_must_fail git branch --no-contains $blob
 '
 
+test_expect_success 'multiple branch --contains' '
+	git checkout -b side2 master &&
+	>feature &&
+	git add feature &&
+	git commit -m "add feature" &&
+	git checkout -b next master &&
+	git merge side &&
+	git branch --contains side --contains side2 >actual &&
+	cat >expect <<-\EOF &&
+	* next
+	  side
+	  side2
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'multiple branch --no-contains' '
+	git branch --no-contains side --no-contains side2 >actual &&
+	cat >expect <<-\EOF &&
+	  master
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'branch --contains combined with --no-contains' '
+	git checkout -b seen master &&
+	git merge side &&
+	git merge side2 &&
+	git branch --contains side --no-contains side2 >actual &&
+	cat >expect <<-\EOF &&
+	  next
+	  side
+	EOF
+	test_cmp expect actual
+'
+
 # We want to set up a case where the walk for the tracking info
 # of one branch crosses the tip of another branch (and make sure
 # that the latter walk does not mess up our flag to see if it was
@@ -200,15 +236,4 @@ test_expect_success 'branch --merged with --verbose' '
 	test_i18ncmp expect actual
 '
 
-test_expect_success 'branch --contains combined with --no-contains' '
-	git branch --contains zzz --no-contains topic >actual &&
-	cat >expect <<-\EOF &&
-	  master
-	  side
-	  zzz
-	EOF
-	test_cmp expect actual
-
-'
-
 test_done
-- 
2.24.3 (Apple Git-128)

[PATCH v4 2/3] Doc: cover multiple contains/no-contains filters

[Aaron Lipman]

Update documentation for "git branch", "git for-each-ref" and "git tag"
with notes explaining what happens when passed multiple --contains or
--no-contains filters.

This behavior is useful to document prior to enabling multiple
merged/no-merged filters, in order to demonstrate consistent behavior
between merged/no-merged and contains/no-contains filters.

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 Documentation/filters.txt          | 3 +++
 Documentation/git-branch.txt       | 2 ++
 Documentation/git-for-each-ref.txt | 5 +++++
 Documentation/git-tag.txt          | 5 +++++
 4 files changed, 15 insertions(+)
 create mode 100644 Documentation/filters.txt

diff --git a/Documentation/filters.txt b/Documentation/filters.txt
new file mode 100644
index 0000000000..4ee17afc01
--- /dev/null
+++ b/Documentation/filters.txt
@@ -0,0 +1,3 @@
+When combining multiple `--contains` and `--no-contains` filters, only
+references that contain at least one of the `--contains` commits and
+contain none of the `--no-contains` commits are shown.
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 03c0824d52..aa5e4da142 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -370,6 +370,8 @@ serve four related but different purposes:
 - `--no-merged` is used to find branches which are candidates for merging
   into HEAD, since those branches are not fully contained by HEAD.
 
+include::filters.txt[]
+
 SEE ALSO
 --------
 linkgit:git-check-ref-format[1],
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 616ce46087..c207ed9551 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -408,6 +408,11 @@ Note also that multiple copies of an object may be present in the object
 database; in this case, it is undefined which copy's size or delta base
 will be reported.
 
+NOTES
+-----
+
+include::filters.txt[]
+
 SEE ALSO
 --------
 linkgit:git-show-ref[1]
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index f6d9791780..dadbd71d62 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -377,6 +377,11 @@ $ GIT_COMMITTER_DATE="2006-10-02 10:31" git tag -s v1.0.1
 
 include::date-formats.txt[]
 
+NOTES
+-----
+
+include::filters.txt[]
+
 SEE ALSO
 --------
 linkgit:git-check-ref-format[1].
-- 
2.24.3 (Apple Git-128)

Re: [PATCH v4 2/3] Doc: cover multiple contains/no-contains filters

[Junio C Hamano]

Aaron Lipman <alipman88@gmail.com> writes:

> Update documentation for "git branch", "git for-each-ref" and "git tag"
> with notes explaining what happens when passed multiple --contains or
> --no-contains filters.
>
> This behavior is useful to document prior to enabling multiple
> merged/no-merged filters, in order to demonstrate consistent behavior
> between merged/no-merged and contains/no-contains filters.
>
> Signed-off-by: Aaron Lipman <alipman88@gmail.com>
> ---
>  Documentation/filters.txt          | 3 +++

"git" has more than one concepts that relate to the word "filter",
like the one that are used to create lazy clone, the "filter-branch"
command, "smudge/clean" filter, "textconv" filter used by the diff
and grep machinery to name some.

Make sure you do not accidentally squat on a good-sounding but
an overly generic name.  ref-reachability-filters.txt perhaps?

> diff --git a/Documentation/filters.txt b/Documentation/filters.txt
> new file mode 100644
> index 0000000000..4ee17afc01
> --- /dev/null
> +++ b/Documentation/filters.txt
> @@ -0,0 +1,3 @@
> +When combining multiple `--contains` and `--no-contains` filters, only
> +references that contain at least one of the `--contains` commits and
> +contain none of the `--no-contains` commits are shown.

[PATCH v4 3/3] ref-filter: allow merged and no-merged filters

[Aaron Lipman]

Enable ref-filter to process multiple merged and no-merged filters, and
extend functionality to git branch, git tag and git for-each-ref. This
provides an easy way to check for branches that are "graduation
candidates:"

$ git branch --no-merged master --merged next

If passed more than one merged (or more than one no-merged) filter, refs
must be reachable from any one of the merged commits, and reachable from
none of the no-merged commits.

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 Documentation/filters.txt          |  4 ++
 Documentation/git-branch.txt       |  8 ++--
 Documentation/git-for-each-ref.txt |  8 ++--
 Documentation/git-tag.txt          |  6 +--
 builtin/branch.c                   |  6 +--
 builtin/for-each-ref.c             |  2 +-
 builtin/tag.c                      |  8 ++--
 ref-filter.c                       | 64 ++++++++++++++++--------------
 ref-filter.h                       | 12 +++---
 t/t3200-branch.sh                  |  4 --
 t/t3201-branch-contains.sh         | 27 +++++++++++++
 t/t6302-for-each-ref-filter.sh     |  4 +-
 t/t7004-tag.sh                     |  4 +-
 13 files changed, 92 insertions(+), 65 deletions(-)

diff --git a/Documentation/filters.txt b/Documentation/filters.txt
index 4ee17afc01..9bae46d84c 100644
--- a/Documentation/filters.txt
+++ b/Documentation/filters.txt
@@ -1,3 +1,7 @@
 When combining multiple `--contains` and `--no-contains` filters, only
 references that contain at least one of the `--contains` commits and
 contain none of the `--no-contains` commits are shown.
+
+When combining multiple `--merged` and `--no-merged` filters, only
+references that are reachable from at least one of the `--merged`
+commits and from none of the `--no-merged` commits are shown.
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index aa5e4da142..290b90639c 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git branch' [--color[=<when>] | --no-color] [--show-current]
 	[-v [--abbrev=<length> | --no-abbrev]]
 	[--column[=<options>] | --no-column] [--sort=<key>]
-	[(--merged | --no-merged) [<commit>]]
+	[--merged [<commit>]] [--no-merged [<commit>]]
 	[--contains [<commit>]] [--no-contains [<commit>]]
 	[--points-at <object>] [--format=<format>]
 	[(-r | --remotes) | (-a | --all)]
@@ -252,13 +252,11 @@ start-point is either a local or remote-tracking branch.
 
 --merged [<commit>]::
 	Only list branches whose tips are reachable from the
-	specified commit (HEAD if not specified). Implies `--list`,
-	incompatible with `--no-merged`.
+	specified commit (HEAD if not specified). Implies `--list`.
 
 --no-merged [<commit>]::
 	Only list branches whose tips are not reachable from the
-	specified commit (HEAD if not specified). Implies `--list`,
-	incompatible with `--merged`.
+	specified commit (HEAD if not specified). Implies `--list`.
 
 <branchname>::
 	The name of the branch to create or delete.
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index c207ed9551..7b9cf0ef1f 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git for-each-ref' [--count=<count>] [--shell|--perl|--python|--tcl]
 		   [(--sort=<key>)...] [--format=<format>] [<pattern>...]
 		   [--points-at=<object>]
-		   (--merged[=<object>] | --no-merged[=<object>])
+		   [--merged[=<object>]] [--no-merged[=<object>]]
 		   [--contains[=<object>]] [--no-contains[=<object>]]
 
 DESCRIPTION
@@ -76,13 +76,11 @@ OPTIONS
 
 --merged[=<object>]::
 	Only list refs whose tips are reachable from the
-	specified commit (HEAD if not specified),
-	incompatible with `--no-merged`.
+	specified commit (HEAD if not specified).
 
 --no-merged[=<object>]::
 	Only list refs whose tips are not reachable from the
-	specified commit (HEAD if not specified),
-	incompatible with `--merged`.
+	specified commit (HEAD if not specified).
 
 --contains[=<object>]::
 	Only list refs which contain the specified commit (HEAD if not
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index dadbd71d62..cc667d7d01 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -15,7 +15,7 @@ SYNOPSIS
 'git tag' [-n[<num>]] -l [--contains <commit>] [--no-contains <commit>]
 	[--points-at <object>] [--column[=<options>] | --no-column]
 	[--create-reflog] [--sort=<key>] [--format=<format>]
-	[--[no-]merged [<commit>]] [<pattern>...]
+	[--merged <commit>] [--no-merged <commit>] [<pattern>...]
 'git tag' -v [--format=<format>] <tagname>...
 
 DESCRIPTION
@@ -149,11 +149,11 @@ This option is only applicable when listing tags without annotation lines.
 
 --merged [<commit>]::
 	Only list tags whose commits are reachable from the specified
-	commit (`HEAD` if not specified), incompatible with `--no-merged`.
+	commit (`HEAD` if not specified).
 
 --no-merged [<commit>]::
 	Only list tags whose commits are not reachable from the specified
-	commit (`HEAD` if not specified), incompatible with `--merged`.
+	commit (`HEAD` if not specified).
 
 --points-at <object>::
 	Only list tags of the given object (HEAD if not
diff --git a/builtin/branch.c b/builtin/branch.c
index e82301fb1b..efb30b8820 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -26,7 +26,7 @@
 #include "commit-reach.h"
 
 static const char * const builtin_branch_usage[] = {
-	N_("git branch [<options>] [-r | -a] [--merged | --no-merged]"),
+	N_("git branch [<options>] [-r | -a] [--merged] [--no-merged]"),
 	N_("git branch [<options>] [-l] [-f] <branch-name> [<start-point>]"),
 	N_("git branch [<options>] [-r] (-d | -D) <branch-name>..."),
 	N_("git branch [<options>] (-m | -M) [<old-branch>] <new-branch>"),
@@ -688,8 +688,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	    !show_current && !unset_upstream && argc == 0)
 		list = 1;
 
-	if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr ||
-	    filter.no_commit)
+	if (filter.with_commit || filter.no_commit ||
+	    filter.reachable_from || filter.unreachable_from || filter.points_at.nr)
 		list = 1;
 
 	if (!!delete + !!rename + !!copy + !!new_upstream + !!show_current +
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 57489e4eab..9d1ecda2b8 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -9,7 +9,7 @@
 static char const * const for_each_ref_usage[] = {
 	N_("git for-each-ref [<options>] [<pattern>]"),
 	N_("git for-each-ref [--points-at <object>]"),
-	N_("git for-each-ref [(--merged | --no-merged) [<commit>]]"),
+	N_("git for-each-ref [--merged [<commit>]] [--no-merged [<commit>]]"),
 	N_("git for-each-ref [--contains [<commit>]] [--no-contains [<commit>]]"),
 	NULL
 };
diff --git a/builtin/tag.c b/builtin/tag.c
index 5cbd80dc3e..ecf011776d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -26,7 +26,7 @@ static const char * const git_tag_usage[] = {
 		"\t\t<tagname> [<head>]"),
 	N_("git tag -d <tagname>..."),
 	N_("git tag -l [-n[<num>]] [--contains <commit>] [--no-contains <commit>] [--points-at <object>]\n"
-		"\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"),
+		"\t\t[--format=<format>] [--merged <commit>] [--no-merged <commit>] [<pattern>...]"),
 	N_("git tag -v [--format=<format>] <tagname>..."),
 	NULL
 };
@@ -457,8 +457,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		if (argc == 0)
 			cmdmode = 'l';
 		else if (filter.with_commit || filter.no_commit ||
-			 filter.points_at.nr || filter.merge_commit ||
-			 filter.lines != -1)
+			 filter.reachable_from || filter.unreachable_from ||
+			 filter.points_at.nr || filter.lines != -1)
 			cmdmode = 'l';
 	}
 
@@ -509,7 +509,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		die(_("--no-contains option is only allowed in list mode"));
 	if (filter.points_at.nr)
 		die(_("--points-at option is only allowed in list mode"));
-	if (filter.merge_commit)
+	if (filter.reachable_from || filter.unreachable_from)
 		die(_("--merged and --no-merged options are only allowed in list mode"));
 	if (cmdmode == 'd')
 		return for_each_tag_name(argv, delete_tag, NULL);
diff --git a/ref-filter.c b/ref-filter.c
index 110bcd741a..785785a757 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2167,9 +2167,9 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,
 	 * obtain the commit using the 'oid' available and discard all
 	 * non-commits early. The actual filtering is done later.
 	 */
-	if (filter->merge_commit || filter->with_commit || filter->no_commit || filter->verbose) {
-		commit = lookup_commit_reference_gently(the_repository, oid,
-							1);
+	if (filter->reachable_from || filter->unreachable_from ||
+	    filter->with_commit || filter->no_commit || filter->verbose) {
+		commit = lookup_commit_reference_gently(the_repository, oid, 1);
 		if (!commit)
 			return 0;
 		/* We perform the filtering for the '--contains' option... */
@@ -2231,13 +2231,20 @@ void ref_array_clear(struct ref_array *array)
 	}
 }
 
-static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
+static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)
 {
 	struct rev_info revs;
 	int i, old_nr;
-	struct ref_filter *filter = ref_cbdata->filter;
 	struct ref_array *array = ref_cbdata->array;
 	struct commit **to_clear = xcalloc(sizeof(struct commit *), array->nr);
+	struct commit_list *rl;
+
+	struct commit_list *check_reachable_list = reachable ?
+		ref_cbdata->filter->reachable_from :
+		ref_cbdata->filter->unreachable_from;
+
+	if (!check_reachable_list)
+		return;
 
 	repo_init_revisions(the_repository, &revs, NULL);
 
@@ -2247,8 +2254,11 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
 		to_clear[i] = item->commit;
 	}
 
-	filter->merge_commit->object.flags |= UNINTERESTING;
-	add_pending_object(&revs, &filter->merge_commit->object, "");
+	for (rl = check_reachable_list; rl; rl = rl->next) {
+		struct commit *merge_commit = rl->item;
+		merge_commit->object.flags |= UNINTERESTING;
+		add_pending_object(&revs, &merge_commit->object, "");
+	}
 
 	revs.limited = 1;
 	if (prepare_revision_walk(&revs))
@@ -2263,14 +2273,19 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
 
 		int is_merged = !!(commit->object.flags & UNINTERESTING);
 
-		if (is_merged == (filter->merge == REF_FILTER_MERGED_INCLUDE))
+		if (is_merged == reachable)
 			array->items[array->nr++] = array->items[i];
 		else
 			free_array_item(item);
 	}
 
 	clear_commit_marks_many(old_nr, to_clear, ALL_REV_FLAGS);
-	clear_commit_marks(filter->merge_commit, ALL_REV_FLAGS);
+
+	while (check_reachable_list) {
+		struct commit *merge_commit = pop_commit(&check_reachable_list);
+		clear_commit_marks(merge_commit, ALL_REV_FLAGS);
+	}
+
 	free(to_clear);
 }
 
@@ -2322,8 +2337,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 	clear_contains_cache(&ref_cbdata.no_contains_cache);
 
 	/*  Filters that need revision walking */
-	if (filter->merge_commit)
-		do_merge_filter(&ref_cbdata);
+	do_merge_filter(&ref_cbdata, DO_MERGE_FILTER_REACHABLE);
+	do_merge_filter(&ref_cbdata, DO_MERGE_FILTER_UNREACHABLE);
 
 	return ret;
 }
@@ -2541,31 +2556,22 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
 {
 	struct ref_filter *rf = opt->value;
 	struct object_id oid;
-	int no_merged = starts_with(opt->long_name, "no");
+	struct commit *merge_commit;
 
 	BUG_ON_OPT_NEG(unset);
 
-	if (rf->merge) {
-		if (no_merged) {
-			return error(_("option `%s' is incompatible with --merged"),
-				     opt->long_name);
-		} else {
-			return error(_("option `%s' is incompatible with --no-merged"),
-				     opt->long_name);
-		}
-	}
-
-	rf->merge = no_merged
-		? REF_FILTER_MERGED_OMIT
-		: REF_FILTER_MERGED_INCLUDE;
-
 	if (get_oid(arg, &oid))
 		die(_("malformed object name %s"), arg);
 
-	rf->merge_commit = lookup_commit_reference_gently(the_repository,
-							  &oid, 0);
-	if (!rf->merge_commit)
+	merge_commit = lookup_commit_reference_gently(the_repository, &oid, 0);
+
+	if (!merge_commit)
 		return error(_("option `%s' must point to a commit"), opt->long_name);
 
+	if (starts_with(opt->long_name, "no"))
+		commit_list_insert(merge_commit, &rf->unreachable_from);
+	else
+		commit_list_insert(merge_commit, &rf->reachable_from);
+
 	return 0;
 }
diff --git a/ref-filter.h b/ref-filter.h
index 8ecc33cdfa..2d13928455 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -23,6 +23,9 @@
 #define FILTER_REFS_DETACHED_HEAD  0x0020
 #define FILTER_REFS_KIND_MASK      (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)
 
+#define DO_MERGE_FILTER_UNREACHABLE 0
+#define DO_MERGE_FILTER_REACHABLE   1
+
 struct atom_value;
 
 struct ref_sorting {
@@ -54,13 +57,8 @@ struct ref_filter {
 	struct oid_array points_at;
 	struct commit_list *with_commit;
 	struct commit_list *no_commit;
-
-	enum {
-		REF_FILTER_MERGED_NONE = 0,
-		REF_FILTER_MERGED_INCLUDE,
-		REF_FILTER_MERGED_OMIT
-	} merge;
-	struct commit *merge_commit;
+	struct commit_list *reachable_from;
+	struct commit_list *unreachable_from;
 
 	unsigned int with_commit_tag_algo : 1,
 		match_as_path : 1,
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 028c88d1b2..c24c6632ee 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1299,10 +1299,6 @@ test_expect_success '--merged catches invalid object names' '
 	test_must_fail git branch --merged 0000000000000000000000000000000000000000
 '
 
-test_expect_success '--merged is incompatible with --no-merged' '
-	test_must_fail git branch --merged HEAD --no-merged HEAD
-'
-
 test_expect_success '--list during rebase' '
 	test_when_finished "reset_rebase" &&
 	git checkout master &&
diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh
index 3cb9dc6cca..efea5c4971 100755
--- a/t/t3201-branch-contains.sh
+++ b/t/t3201-branch-contains.sh
@@ -187,6 +187,16 @@ test_expect_success 'multiple branch --contains' '
 	test_cmp expect actual
 '
 
+test_expect_success 'multiple branch --merged' '
+	git branch --merged next --merged master >actual &&
+	cat >expect <<-\EOF &&
+	  master
+	* next
+	  side
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'multiple branch --no-contains' '
 	git branch --no-contains side --no-contains side2 >actual &&
 	cat >expect <<-\EOF &&
@@ -195,6 +205,14 @@ test_expect_success 'multiple branch --no-contains' '
 	test_cmp expect actual
 '
 
+test_expect_success 'multiple branch --no-merged' '
+	git branch --no-merged next --no-merged master >actual &&
+	cat >expect <<-\EOF &&
+	  side2
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'branch --contains combined with --no-contains' '
 	git checkout -b seen master &&
 	git merge side &&
@@ -207,6 +225,15 @@ test_expect_success 'branch --contains combined with --no-contains' '
 	test_cmp expect actual
 '
 
+test_expect_success 'branch --merged combined with --no-merged' '
+	git branch --merged seen --no-merged next >actual &&
+	cat >expect <<-\EOF &&
+	* seen
+	  side2
+	EOF
+	test_cmp expect actual
+'
+
 # We want to set up a case where the walk for the tracking info
 # of one branch crosses the tip of another branch (and make sure
 # that the latter walk does not mess up our flag to see if it was
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 35408d53fd..781e470aea 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -437,8 +437,8 @@ test_expect_success 'check %(if:notequals=<string>)' '
 	test_cmp expect actual
 '
 
-test_expect_success '--merged is incompatible with --no-merged' '
-	test_must_fail git for-each-ref --merged HEAD --no-merged HEAD
+test_expect_success '--merged is compatible with --no-merged' '
+	git for-each-ref --merged HEAD --no-merged HEAD
 '
 
 test_expect_success 'validate worktree atom' '
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 74b637deb2..05f411c821 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -2015,8 +2015,8 @@ test_expect_success '--merged can be used in non-list mode' '
 	test_cmp expect actual
 '
 
-test_expect_success '--merged is incompatible with --no-merged' '
-	test_must_fail git tag --merged HEAD --no-merged HEAD
+test_expect_success '--merged is compatible with --no-merged' '
+	git tag --merged HEAD --no-merged HEAD
 '
 
 test_expect_success '--merged shows merged tags' '
-- 
2.24.3 (Apple Git-128)

Re: [PATCH v4 0/3] git branch: allow combining merged and no-merged filters

[Junio C Hamano]

Aaron Lipman <alipman88@gmail.com> writes:

>> I do not mind making the 0/1 a symbolic constant between
>> do_merge_filter() and filter_refs() for enhanced readability,
>> though.
>
> If I understand the convention, the constant names should be prefixed
> with DO_MERGE_FILTER. I suggest DO_MERGE_FILTER_REACHABLE and
> DO_MERGE_FILTER_UNREACHABLE. Happy to re-roll if others have a
> different preference - or feel free to edit.)

Even though I said "I do not mind", using DO_MERGE_FILTER prefix is
way too much noise.  The common prefix is appropriate for external
API, but this is merely an internal contract between a private
helper do_merge_filter() and its only caller.

If I were redesigning the code around this area, I probably would
rename do_merge_filter() to something more descriptive, and tweak
its parameters a bit more focused, e.g.

    #define EXCLUDE_REACHED 0
    #define INCLUDE_REACHED 1
    static void reach_filter(struct ref_array *array, 
			     struct commit_list *check_reachable,
			     int include_reached) {
		...
		for (i = 0; i < old_nr; i++) {
			struct commit *c = array->items[i];
			int is_reached = !!(c->object.flags & UNINTERESTING);

			if (is_reached == include_reached)
				array->items[array->nr++] = array->items[i];
			else
				free_array_item(array->items[i]);
		}
		...
    }

    /* the caller */
    ...
    reach_filter(array, filter->reachable_from, INCLUDE_REACHED);
    reach_filter(array, filter->unreachable_from, EXCLUDE_REACHED);

to make it clear which part of the callback struct is really passed
between the caller and the helper.  Even if we are not renaming
things that much, a locally defined preprocessor macro with shorter
names, defined just before the callee, would be more appropriate for
a case like this, with a single callee called by only one caller.

Thanks.

Re: [PATCH v4 0/3] git branch: allow combining merged and no-merged filters

[Eric Sunshine]

On Wed, Sep 16, 2020 at 12:53 AM Junio C Hamano <gitster@pobox.com> wrote:
> Even though I said "I do not mind", using DO_MERGE_FILTER prefix is
> way too much noise.  The common prefix is appropriate for external
> API, but this is merely an internal contract between a private
> helper do_merge_filter() and its only caller.
>
>     #define EXCLUDE_REACHED 0
>     #define INCLUDE_REACHED 1
>     static void reach_filter(struct ref_array *array,
>                              struct commit_list *check_reachable,
>                              int include_reached) {
>
> to make it clear which part of the callback struct is really passed
> between the caller and the helper.  Even if we are not renaming
> things that much, a locally defined preprocessor macro with shorter
> names, defined just before the callee, would be more appropriate for
> a case like this, with a single callee called by only one caller.

Thanks for stating what I was planning on saying, with particular
emphasis on keeping these #defines in the .c file rather than the .h
file since they are not part of the public API.

Aside from that, this re-roll seems to address all of my previous
review comments. Thanks.

[PATCH 0/2] ref-filter: merged & no-merged touch-ups

[Aaron Lipman]

> > Even if we are not renaming
> > things that much, a locally defined preprocessor macro with shorter
> > names, defined just before the callee, would be more appropriate for
> > a case like this, with a single callee called by only one caller.

> Thanks for stating what I was planning on saying, with particular
> emphasis on keeping these #defines in the .c file rather than the .h
> file since they are not part of the public API.

Thanks for the insight into defining macros in source vs header files-
I'm still in the process of learning C and figuring out how to convey
purpose & encapsulation without relying on features of higher order
languages I'm used to (e.g. keyword args).

Since this got moved to next, I'm not entirely clear on whether folks
want me to keep tweaking this, or if the last set of comments were
general advice for future contributions.

But in the interest of diligence, I'd like to offer a couple touch-ups
applying Junio's suggestions.

Aaron Lipman (2):
  ref-filter: refactor do_merge_filter()
  Doc: prefer more specific file name

 Documentation/git-branch.txt                  |  2 +-
 Documentation/git-for-each-ref.txt            |  2 +-
 Documentation/git-tag.txt                     |  2 +-
 ...lters.txt => ref-reachability-filters.txt} |  0
 ref-filter.c                                  | 29 +++++++++----------
 ref-filter.h                                  |  3 --
 6 files changed, 17 insertions(+), 21 deletions(-)
 rename Documentation/{filters.txt => ref-reachability-filters.txt} (100%)

-- 
2.24.3 (Apple Git-128)

[PATCH 1/2] ref-filter: refactor do_merge_filter()

[Aaron Lipman]

Rename to reach_filter() to be more descriptive.

Separate parameters to explicitly identify what data is used by the
function, instead of passing an entire ref_filter_cbdata struct.

Rename and move associated preprocessor macros from header to source
file, as they're only used internally in a single location.

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 ref-filter.c | 29 ++++++++++++++---------------
 ref-filter.h |  3 ---
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 785785a757..5550a0d34c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2231,19 +2231,18 @@ void ref_array_clear(struct ref_array *array)
 	}
 }
 
-static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)
+#define EXCLUDE_REACHED 0
+#define INCLUDE_REACHED 1
+static void reach_filter(struct ref_array *array,
+			 struct commit_list *check_reachable,
+			 int include_reached)
 {
 	struct rev_info revs;
 	int i, old_nr;
-	struct ref_array *array = ref_cbdata->array;
 	struct commit **to_clear = xcalloc(sizeof(struct commit *), array->nr);
-	struct commit_list *rl;
+	struct commit_list *cr;
 
-	struct commit_list *check_reachable_list = reachable ?
-		ref_cbdata->filter->reachable_from :
-		ref_cbdata->filter->unreachable_from;
-
-	if (!check_reachable_list)
+	if (!check_reachable)
 		return;
 
 	repo_init_revisions(the_repository, &revs, NULL);
@@ -2254,8 +2253,8 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)
 		to_clear[i] = item->commit;
 	}
 
-	for (rl = check_reachable_list; rl; rl = rl->next) {
-		struct commit *merge_commit = rl->item;
+	for (cr = check_reachable; cr; cr = cr->next) {
+		struct commit *merge_commit = cr->item;
 		merge_commit->object.flags |= UNINTERESTING;
 		add_pending_object(&revs, &merge_commit->object, "");
 	}
@@ -2273,7 +2272,7 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)
 
 		int is_merged = !!(commit->object.flags & UNINTERESTING);
 
-		if (is_merged == reachable)
+		if (is_merged == include_reached)
 			array->items[array->nr++] = array->items[i];
 		else
 			free_array_item(item);
@@ -2281,8 +2280,8 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)
 
 	clear_commit_marks_many(old_nr, to_clear, ALL_REV_FLAGS);
 
-	while (check_reachable_list) {
-		struct commit *merge_commit = pop_commit(&check_reachable_list);
+	while (check_reachable) {
+		struct commit *merge_commit = pop_commit(&check_reachable);
 		clear_commit_marks(merge_commit, ALL_REV_FLAGS);
 	}
 
@@ -2337,8 +2336,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 	clear_contains_cache(&ref_cbdata.no_contains_cache);
 
 	/*  Filters that need revision walking */
-	do_merge_filter(&ref_cbdata, DO_MERGE_FILTER_REACHABLE);
-	do_merge_filter(&ref_cbdata, DO_MERGE_FILTER_UNREACHABLE);
+	reach_filter(array, filter->reachable_from, INCLUDE_REACHED);
+	reach_filter(array, filter->unreachable_from, EXCLUDE_REACHED);
 
 	return ret;
 }
diff --git a/ref-filter.h b/ref-filter.h
index 2d13928455..feaef4a8fd 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -23,9 +23,6 @@
 #define FILTER_REFS_DETACHED_HEAD  0x0020
 #define FILTER_REFS_KIND_MASK      (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)
 
-#define DO_MERGE_FILTER_UNREACHABLE 0
-#define DO_MERGE_FILTER_REACHABLE   1
-
 struct atom_value;
 
 struct ref_sorting {
-- 
2.24.3 (Apple Git-128)

Re: [PATCH 1/2] ref-filter: refactor do_merge_filter()

[Eric Sunshine]

On Thu, Sep 17, 2020 at 8:49 PM Aaron Lipman <alipman88@gmail.com> wrote:
> ref-filter: refactor do_merge_filter()
>
> Rename to reach_filter() to be more descriptive.
>
> Separate parameters to explicitly identify what data is used by the
> function, instead of passing an entire ref_filter_cbdata struct.
>
> Rename and move associated preprocessor macros from header to source
> file, as they're only used internally in a single location.

One thing that this commit message lacks is a high-level explanation
of why these changes are being made. One possible rewrite would be:

    ref-filter: make internal reachable-filter API more precise

    The internal reachable-filter API is a bit loose and imprecise; it
    also bleeds unnecessarily into the public header. Tighten the API
    by:

    * renaming do_merge_filter() to reach_filter()

    * separating parameters to explicitly identify what data is used
      by the function instead of passing an entire ref_filter_cbdata
      struct

    * renaming and moving internal constants from header to source
      file

> Signed-off-by: Aaron Lipman <alipman88@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -2231,19 +2231,18 @@ void ref_array_clear(struct ref_array *array)
> +static void reach_filter(struct ref_array *array,
> +                        struct commit_list *check_reachable,
> +                        int include_reached)
>  {
>         [...]
> +       if (!check_reachable)
>                 return;

I would probably drop this conditional return altogether since it
incorrectly conveys to the reader that it is legal to call this
function with NULL for 'check_reachable', whereas, in reality, it
would be pointless to do so. If this function were public, and the
possible list of callers open-ended, then it might be reasonable for
it to do this to alert callers early of their error:

    if (!check_reachable)
        BUG("check_reachable must not be NULL");

However, as this function is private, and the set of callers can be
precisely determined, it's not necessary to make the check at all. If
anyone adds a new call in this file which incorrectly passes NULL,
they will discover the error quickly enough when the program crashes
at the new call site.

If you were to drop this conditional, then that change would deserve
another bullet point in the commit message.

Re: [PATCH 1/2] ref-filter: refactor do_merge_filter()

[Eric Sunshine]

On Fri, Sep 18, 2020 at 1:03 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > +       if (!check_reachable)
> >                 return;
>
> I would probably drop this conditional return altogether since it
> incorrectly conveys to the reader that it is legal to call this
> function with NULL for 'check_reachable', whereas, in reality, it
> would be pointless to do so. [...]

Nevermind. I'm an idiot. I forgot that filter->reachable_from and
filter->unreachable_from may indeed validly be NULL.

Re: [PATCH 1/2] ref-filter: refactor do_merge_filter()

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> One thing that this commit message lacks is a high-level explanation
> of why these changes are being made. One possible rewrite would be:
>
>     ref-filter: make internal reachable-filter API more precise
>
>     The internal reachable-filter API is a bit loose and imprecise; it
>     also bleeds unnecessarily into the public header. Tighten the API
>     by:
>
>     * renaming do_merge_filter() to reach_filter()
>
>     * separating parameters to explicitly identify what data is used
>       by the function instead of passing an entire ref_filter_cbdata
>       struct
>
>     * renaming and moving internal constants from header to source
>       file

Sounds good.  Aaron?

>> +static void reach_filter(struct ref_array *array,
>> +                        struct commit_list *check_reachable,
>> +                        int include_reached)
>>  {
>>         [...]
>> +       if (!check_reachable)
>>                 return;
>
> I would probably drop this conditional return altogether ...

As you said downthread, this part is fine as-is.

[PATCH 2/2] Doc: prefer more specific file name

[Aaron Lipman]

Change filters.txt to ref-reachability-filters.txt in order to avoid
squatting on a file name that might be useful for another purpose.

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 Documentation/git-branch.txt                                | 2 +-
 Documentation/git-for-each-ref.txt                          | 2 +-
 Documentation/git-tag.txt                                   | 2 +-
 Documentation/{filters.txt => ref-reachability-filters.txt} | 0
 4 files changed, 3 insertions(+), 3 deletions(-)
 rename Documentation/{filters.txt => ref-reachability-filters.txt} (100%)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 290b90639c..ace4ad3da8 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -368,7 +368,7 @@ serve four related but different purposes:
 - `--no-merged` is used to find branches which are candidates for merging
   into HEAD, since those branches are not fully contained by HEAD.
 
-include::filters.txt[]
+include::ref-reachability-filters.txt[]
 
 SEE ALSO
 --------
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 7b9cf0ef1f..2962f85a50 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -409,7 +409,7 @@ will be reported.
 NOTES
 -----
 
-include::filters.txt[]
+include::ref-reachability-filters.txt[]
 
 SEE ALSO
 --------
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index cc667d7d01..56656d1be6 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -380,7 +380,7 @@ include::date-formats.txt[]
 NOTES
 -----
 
-include::filters.txt[]
+include::ref-reachability-filters.txt[]
 
 SEE ALSO
 --------
diff --git a/Documentation/filters.txt b/Documentation/ref-reachability-filters.txt
similarity index 100%
rename from Documentation/filters.txt
rename to Documentation/ref-reachability-filters.txt
-- 
2.24.3 (Apple Git-128)

Re: [PATCH 0/2] ref-filter: merged & no-merged touch-ups

[Eric Sunshine]

On Thu, Sep 17, 2020 at 8:49 PM Aaron Lipman <alipman88@gmail.com> wrote:
> Since this got moved to next, I'm not entirely clear on whether folks
> want me to keep tweaking this, or if the last set of comments were
> general advice for future contributions.
>
> But in the interest of diligence, I'd like to offer a couple touch-ups
> applying Junio's suggestions.

In this case, the review comments were reasonably important, so
tweaking it by patching what has already been moved to 'next', as you
do here, is a good choice. Thanks for the diligence. I'll leave one or
two minor comments on the patches themselves (although, it's probably
not worth re-rolling just for them).

[PATCH v2 0/2] ref-filter: merged & no-merged touch-ups

[Aaron Lipman]

> Sounds good.  Aaron?

Sounds excellent! Rebased using Eric's wording.

Aaron Lipman (2):
  ref-filter: make internal reachable-filter API more precise
  Doc: prefer more specific file name

 Documentation/git-branch.txt                  |  2 +-
 Documentation/git-for-each-ref.txt            |  2 +-
 Documentation/git-tag.txt                     |  2 +-
 ...lters.txt => ref-reachability-filters.txt} |  0
 ref-filter.c                                  | 29 +++++++++----------
 ref-filter.h                                  |  3 --
 6 files changed, 17 insertions(+), 21 deletions(-)
 rename Documentation/{filters.txt => ref-reachability-filters.txt} (100%)

-- 
2.24.3 (Apple Git-128)

[PATCH v2 1/2] ref-filter: make internal reachable-filter API more precise

[Aaron Lipman]

The internal reachable-filter API is a bit loose and imprecise; it
also bleeds unnecessarily into the public header. Tighten the API
by:

* renaming do_merge_filter() to reach_filter()

* separating parameters to explicitly identify what data is used
  by the function instead of passing an entire ref_filter_cbdata
  struct

* renaming and moving internal constants from header to source
  file

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 ref-filter.c | 29 ++++++++++++++---------------
 ref-filter.h |  3 ---
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 785785a757..5550a0d34c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2231,19 +2231,18 @@ void ref_array_clear(struct ref_array *array)
 	}
 }
 
-static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)
+#define EXCLUDE_REACHED 0
+#define INCLUDE_REACHED 1
+static void reach_filter(struct ref_array *array,
+			 struct commit_list *check_reachable,
+			 int include_reached)
 {
 	struct rev_info revs;
 	int i, old_nr;
-	struct ref_array *array = ref_cbdata->array;
 	struct commit **to_clear = xcalloc(sizeof(struct commit *), array->nr);
-	struct commit_list *rl;
+	struct commit_list *cr;
 
-	struct commit_list *check_reachable_list = reachable ?
-		ref_cbdata->filter->reachable_from :
-		ref_cbdata->filter->unreachable_from;
-
-	if (!check_reachable_list)
+	if (!check_reachable)
 		return;
 
 	repo_init_revisions(the_repository, &revs, NULL);
@@ -2254,8 +2253,8 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)
 		to_clear[i] = item->commit;
 	}
 
-	for (rl = check_reachable_list; rl; rl = rl->next) {
-		struct commit *merge_commit = rl->item;
+	for (cr = check_reachable; cr; cr = cr->next) {
+		struct commit *merge_commit = cr->item;
 		merge_commit->object.flags |= UNINTERESTING;
 		add_pending_object(&revs, &merge_commit->object, "");
 	}
@@ -2273,7 +2272,7 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)
 
 		int is_merged = !!(commit->object.flags & UNINTERESTING);
 
-		if (is_merged == reachable)
+		if (is_merged == include_reached)
 			array->items[array->nr++] = array->items[i];
 		else
 			free_array_item(item);
@@ -2281,8 +2280,8 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)
 
 	clear_commit_marks_many(old_nr, to_clear, ALL_REV_FLAGS);
 
-	while (check_reachable_list) {
-		struct commit *merge_commit = pop_commit(&check_reachable_list);
+	while (check_reachable) {
+		struct commit *merge_commit = pop_commit(&check_reachable);
 		clear_commit_marks(merge_commit, ALL_REV_FLAGS);
 	}
 
@@ -2337,8 +2336,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
 	clear_contains_cache(&ref_cbdata.no_contains_cache);
 
 	/*  Filters that need revision walking */
-	do_merge_filter(&ref_cbdata, DO_MERGE_FILTER_REACHABLE);
-	do_merge_filter(&ref_cbdata, DO_MERGE_FILTER_UNREACHABLE);
+	reach_filter(array, filter->reachable_from, INCLUDE_REACHED);
+	reach_filter(array, filter->unreachable_from, EXCLUDE_REACHED);
 
 	return ret;
 }
diff --git a/ref-filter.h b/ref-filter.h
index 2d13928455..feaef4a8fd 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -23,9 +23,6 @@
 #define FILTER_REFS_DETACHED_HEAD  0x0020
 #define FILTER_REFS_KIND_MASK      (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)
 
-#define DO_MERGE_FILTER_UNREACHABLE 0
-#define DO_MERGE_FILTER_REACHABLE   1
-
 struct atom_value;
 
 struct ref_sorting {
-- 
2.24.3 (Apple Git-128)

[PATCH v2 2/2] Doc: prefer more specific file name

[Aaron Lipman]

Change filters.txt to ref-reachability-filters.txt in order to avoid
squatting on a file name that might be useful for another purpose.

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 Documentation/git-branch.txt                                | 2 +-
 Documentation/git-for-each-ref.txt                          | 2 +-
 Documentation/git-tag.txt                                   | 2 +-
 Documentation/{filters.txt => ref-reachability-filters.txt} | 0
 4 files changed, 3 insertions(+), 3 deletions(-)
 rename Documentation/{filters.txt => ref-reachability-filters.txt} (100%)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 290b90639c..ace4ad3da8 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -368,7 +368,7 @@ serve four related but different purposes:
 - `--no-merged` is used to find branches which are candidates for merging
   into HEAD, since those branches are not fully contained by HEAD.
 
-include::filters.txt[]
+include::ref-reachability-filters.txt[]
 
 SEE ALSO
 --------
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 7b9cf0ef1f..2962f85a50 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -409,7 +409,7 @@ will be reported.
 NOTES
 -----
 
-include::filters.txt[]
+include::ref-reachability-filters.txt[]
 
 SEE ALSO
 --------
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index cc667d7d01..56656d1be6 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -380,7 +380,7 @@ include::date-formats.txt[]
 NOTES
 -----
 
-include::filters.txt[]
+include::ref-reachability-filters.txt[]
 
 SEE ALSO
 --------
diff --git a/Documentation/filters.txt b/Documentation/ref-reachability-filters.txt
similarity index 100%
rename from Documentation/filters.txt
rename to Documentation/ref-reachability-filters.txt
-- 
2.24.3 (Apple Git-128)