{"thread":{"id":"54211","subject":"[PATCH] ref-filter: allow merged and no-merged filters","startedAt":"2020-09-08T19:50:34Z","lastAt":"2020-09-18T22:00:28Z","messageCount":36,"participants":["Aaron Lipman","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"405188","messageId":"20200908153759.36504-1-alipman88@gmail.com","threadId":"54211","inReplyTo":null,"subject":"[PATCH] ref-filter: allow merged and no-merged filters","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-08T15:37:59Z","receivedAt":"2020-09-08T19:50:34Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"Enable ref-filter to process multiple merged and no-merged filters, and\nextend functionality to git branch, git tag and git for-each-ref. This\nprovides an easy way to check for branches that are \"graduation\ncandidates:\"\n\n$ git branch --no-merged master --merged next\n\nTo accomplish this, store the merged and no-merged filters in two\ncommit_list structs (reachable_from and unreachable_from) rather than\nusing a single commit struct (merge_commit).\n\nIf passed more than one merged (or more than one no-merged) filter,\nmirror the existing boolean logic used by contains/no-contains filters:\nrefs must satisfy any of the merged filters, and all of the no-merged\nfilters.\n\nSigned-off-by: Aaron Lipman \n---\nThis functionality was originally proposed in 2013:\nhttps://lore.kernel.org/git/87fvzwmp23.fsf@59A2.org/T/\n\nIn deciding how to handle multiple merged or multiple no-merged\nfilters, I mirrored the existing behavior for contains/no-contains\nfilters. However, I noticed this existing behavior isn't documented or\ncovered by tests. I think it makes sense to hold off on officially\ndocumenting this behavior for now in case we wish to refine it later,\nbut I'm curious what others think.\n\nIf merging into next, please be sure to also include my previous\npatch correcting tests in t3200:\nhttps://lore.kernel.org/git/20200830224200.21103-1-alipman88@gmail.com\n\n(Apologies, realizing I should have submitted both together.)\n\n\n Documentation/git-branch.txt | 6 +--\n Documentation/git-for-each-ref.txt | 6 +--\n Documentation/git-tag.txt | 4 +-\n builtin/branch.c | 4 +-\n builtin/tag.c | 6 +--\n ref-filter.c | 71 ++++++++++++++++--------------\n ref-filter.h | 9 +---\n t/t3200-branch.sh | 4 +-\n t/t3201-branch-contains.sh | 12 +++++\n t/t6302-for-each-ref-filter.sh | 4 +-\n t/t7004-tag.sh | 2 +-\n 11 files changed, 68 insertions(+), 60 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 03c0824d52..8f0dbcd0ac 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -252,13 +252,11 @@ start-point is either a local or remote-tracking branch.\n \n --merged []::\n \tOnly list branches whose tips are reachable from the\n-\tspecified commit (HEAD if not specified). Implies `--list`,\n-\tincompatible with `--no-merged`.\n+\tspecified commit (HEAD if not specified). Implies `--list`.\n \n --no-merged []::\n \tOnly list branches whose tips are not reachable from the\n-\tspecified commit (HEAD if not specified). Implies `--list`,\n-\tincompatible with `--merged`.\n+\tspecified commit (HEAD if not specified). Implies `--list`.\n \n ::\n \tThe name of the branch to create or delete.\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 2ea71c5f6c..bb113da5a2 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -76,13 +76,11 @@ OPTIONS\n \n --merged[=]::\n \tOnly list refs whose tips are reachable from the\n-\tspecified commit (HEAD if not specified),\n-\tincompatible with `--no-merged`.\n+\tspecified commit (HEAD if not specified).\n \n --no-merged[=]::\n \tOnly list refs whose tips are not reachable from the\n-\tspecified commit (HEAD if not specified),\n-\tincompatible with `--merged`.\n+\tspecified commit (HEAD if not specified).\n \n --contains[=]::\n \tOnly list refs which contain the specified commit (HEAD if not\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex f6d9791780..786d4dfd6f 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -149,11 +149,11 @@ This option is only applicable when listing tags without annotation lines.\n \n --merged []::\n \tOnly list tags whose commits are reachable from the specified\n-\tcommit (`HEAD` if not specified), incompatible with `--no-merged`.\n+\tcommit (`HEAD` if not specified).\n \n --no-merged []::\n \tOnly list tags whose commits are not reachable from the specified\n-\tcommit (`HEAD` if not specified), incompatible with `--merged`.\n+\tcommit (`HEAD` if not specified).\n \n --points-at ::\n \tOnly list tags of the given object (HEAD if not\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex e82301fb1b..4bdb700dd5 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -688,8 +688,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t !show_current && !unset_upstream && argc == 0)\n \t\tlist = 1;\n \n-\tif (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr ||\n-\t filter.no_commit)\n+\tif (filter.with_commit || filter.no_commit ||\n+\t filter.reachable_from || filter.unreachable_from || filter.points_at.nr)\n \t\tlist = 1;\n \n \tif (!!delete + !!rename + !!copy + !!new_upstream + !!show_current +\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 5cbd80dc3e..b1a0398c85 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -457,8 +457,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tif (argc == 0)\n \t\t\tcmdmode = 'l';\n \t\telse if (filter.with_commit || filter.no_commit ||\n-\t\t\t filter.points_at.nr || filter.merge_commit ||\n-\t\t\t filter.lines != -1)\n+\t\t\t filter.reachable_from || filter.unreachable_from ||\n+\t\t\t filter.points_at.nr || filter.lines != -1)\n \t\t\tcmdmode = 'l';\n \t}\n \n@@ -509,7 +509,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"--no-contains option is only allowed in list mode\"));\n \tif (filter.points_at.nr)\n \t\tdie(_(\"--points-at option is only allowed in list mode\"));\n-\tif (filter.merge_commit)\n+\tif (filter.reachable_from || filter.unreachable_from)\n \t\tdie(_(\"--merged and --no-merged options are only allowed in list mode\"));\n \tif (cmdmode == 'd')\n \t\treturn for_each_tag_name(argv, delete_tag, NULL);\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 8ba0e31915..21dca5e68b 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2119,9 +2119,9 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \t * obtain the commit using the 'oid' available and discard all\n \t * non-commits early. The actual filtering is done later.\n \t */\n-\tif (filter->merge_commit || filter->with_commit || filter->no_commit || filter->verbose) {\n-\t\tcommit = lookup_commit_reference_gently(the_repository, oid,\n-\t\t\t\t\t\t\t1);\n+\tif (filter->reachable_from || filter->unreachable_from ||\n+\t filter->with_commit || filter->no_commit || filter->verbose) {\n+\t\tcommit = lookup_commit_reference_gently(the_repository, oid, 1);\n \t\tif (!commit)\n \t\t\treturn 0;\n \t\t/* We perform the filtering for the '--contains' option... */\n@@ -2183,11 +2183,17 @@ void ref_array_clear(struct ref_array *array)\n \t}\n }\n \n-static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)\n+static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)\n {\n+\tstruct commit_list *check_reachable_list = reachable ?\n+\t\tref_cbdata->filter->reachable_from :\n+\t\tref_cbdata->filter->unreachable_from;\n+\n+\tif (!check_reachable_list)\n+\t\treturn;\n+\n \tstruct rev_info revs;\n \tint i, old_nr;\n-\tstruct ref_filter *filter = ref_cbdata->filter;\n \tstruct ref_array *array = ref_cbdata->array;\n \tstruct commit **to_clear = xcalloc(sizeof(struct commit *), array->nr);\n \n@@ -2199,12 +2205,15 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)\n \t\tto_clear[i] = item->commit;\n \t}\n \n-\tfilter->merge_commit->object.flags |= UNINTERESTING;\n-\tadd_pending_object(&revs, &filter->merge_commit->object, \"\");\n+\tfor (struct commit_list *rl = check_reachable_list; rl; rl = rl->next) {\n+\t\tstruct commit *merge_commit = rl->item;\n+\t\tmerge_commit->object.flags |= UNINTERESTING;\n+\t\tadd_pending_object(&revs, &merge_commit->object, \"\");\n \n-\trevs.limited = 1;\n-\tif (prepare_revision_walk(&revs))\n-\t\tdie(_(\"revision walk setup failed\"));\n+\t\trevs.limited = 1;\n+\t\tif (prepare_revision_walk(&revs))\n+\t\t\tdie(_(\"revision walk setup failed\"));\n+\t}\n \n \told_nr = array->nr;\n \tarray->nr = 0;\n@@ -2215,14 +2224,19 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)\n \n \t\tint is_merged = !!(commit->object.flags & UNINTERESTING);\n \n-\t\tif (is_merged == (filter->merge == REF_FILTER_MERGED_INCLUDE))\n+\t\tif (is_merged == reachable)\n \t\t\tarray->items[array->nr++] = array->items[i];\n \t\telse\n \t\t\tfree_array_item(item);\n-\t}\n+ }\n \n \tclear_commit_marks_many(old_nr, to_clear, ALL_REV_FLAGS);\n-\tclear_commit_marks(filter->merge_commit, ALL_REV_FLAGS);\n+\n+\twhile (check_reachable_list) {\n+\t\tstruct commit *merge_commit = pop_commit(&check_reachable_list);\n+\t\tclear_commit_marks(merge_commit, ALL_REV_FLAGS);\n+\t}\n+\n \tfree(to_clear);\n }\n \n@@ -2274,8 +2288,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \tclear_contains_cache(&ref_cbdata.no_contains_cache);\n \n \t/* Filters that need revision walking */\n-\tif (filter->merge_commit)\n-\t\tdo_merge_filter(&ref_cbdata);\n+\tdo_merge_filter(&ref_cbdata, 1);\n+\tdo_merge_filter(&ref_cbdata, 0);\n \n \treturn ret;\n }\n@@ -2493,31 +2507,22 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)\n {\n \tstruct ref_filter *rf = opt->value;\n \tstruct object_id oid;\n-\tint no_merged = starts_with(opt->long_name, \"no\");\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tif (rf->merge) {\n-\t\tif (no_merged) {\n-\t\t\treturn error(_(\"option `%s' is incompatible with --merged\"),\n-\t\t\t\t opt->long_name);\n-\t\t} else {\n-\t\t\treturn error(_(\"option `%s' is incompatible with --no-merged\"),\n-\t\t\t\t opt->long_name);\n-\t\t}\n-\t}\n-\n-\trf->merge = no_merged\n-\t\t? REF_FILTER_MERGED_OMIT\n-\t\t: REF_FILTER_MERGED_INCLUDE;\n-\n \tif (get_oid(arg, &oid))\n \t\tdie(_(\"malformed object name %s\"), arg);\n \n-\trf->merge_commit = lookup_commit_reference_gently(the_repository,\n-\t\t\t\t\t\t\t &oid, 0);\n-\tif (!rf->merge_commit)\n+\tstruct commit *merge_commit = lookup_commit_reference_gently(the_repository,\n+\t\t\t\t\t\t\t\t &oid, 0);\n+\n+\tif (!merge_commit)\n \t\treturn error(_(\"option `%s' must point to a commit\"), opt->long_name);\n \n+\tif (starts_with(opt->long_name, \"no\"))\n+\t\tcommit_list_insert(merge_commit, &rf->unreachable_from);\n+\telse\n+\t\tcommit_list_insert(merge_commit, &rf->reachable_from);\n+\n \treturn 0;\n }\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 8ecc33cdfa..feaef4a8fd 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -54,13 +54,8 @@ struct ref_filter {\n \tstruct oid_array points_at;\n \tstruct commit_list *with_commit;\n \tstruct commit_list *no_commit;\n-\n-\tenum {\n-\t\tREF_FILTER_MERGED_NONE = 0,\n-\t\tREF_FILTER_MERGED_INCLUDE,\n-\t\tREF_FILTER_MERGED_OMIT\n-\t} merge;\n-\tstruct commit *merge_commit;\n+\tstruct commit_list *reachable_from;\n+\tstruct commit_list *unreachable_from;\n \n \tunsigned int with_commit_tag_algo : 1,\n \t\tmatch_as_path : 1,\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 028c88d1b2..be8f61b751 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1299,8 +1299,8 @@ test_expect_success '--merged catches invalid object names' '\n \ttest_must_fail git branch --merged 0000000000000000000000000000000000000000\n '\n \n-test_expect_success '--merged is incompatible with --no-merged' '\n-\ttest_must_fail git branch --merged HEAD --no-merged HEAD\n+test_expect_success '--merged is compatible with --no-merged' '\n+\tgit branch --merged master --no-merged master\n '\n \n test_expect_success '--list during rebase' '\ndiff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh\nindex 40251c9f8f..6e73789995 100755\n--- a/t/t3201-branch-contains.sh\n+++ b/t/t3201-branch-contains.sh\n@@ -211,4 +211,16 @@ test_expect_success 'branch --contains combined with --no-contains' '\n \n '\n \n+test_expect_success 'branch --merged combined with --no-merged' '\n+\tgit checkout master &&\n+\tgit checkout -b next &&\n+\tgit merge side &&\n+\tgit branch --merged next --no-merged master >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t* next\n+\t side\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 35408d53fd..781e470aea 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -437,8 +437,8 @@ test_expect_success 'check %(if:notequals=)' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success '--merged is incompatible with --no-merged' '\n-\ttest_must_fail git for-each-ref --merged HEAD --no-merged HEAD\n+test_expect_success '--merged is compatible with --no-merged' '\n+\tgit for-each-ref --merged HEAD --no-merged HEAD\n '\n \n test_expect_success 'validate worktree atom' '\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 74b637deb2..7d544eceda 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -2016,7 +2016,7 @@ test_expect_success '--merged can be used in non-list mode' '\n '\n \n test_expect_success '--merged is incompatible with --no-merged' '\n-\ttest_must_fail git tag --merged HEAD --no-merged HEAD\n+\tgit tag --merged HEAD --no-merged HEAD\n '\n \n test_expect_success '--merged shows merged tags' '\n-- \n2.24.3 (Apple Git-128)\n\n"},{"id":"405206","messageId":"xmqqsgbrx4xo.fsf@gitster.c.googlers.com","threadId":"54211","inReplyTo":"20200908153759.36504-1-alipman88@gmail.com","subject":"Re: [PATCH] ref-filter: allow merged and no-merged filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-08T22:46:27Z","receivedAt":"2020-09-08T22:46:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Aaron Lipman writes:\n\n> Enable ref-filter to process multiple merged and no-merged filters, and\n> extend functionality to git branch, git tag and git for-each-ref. This\n> provides an easy way to check for branches that are \"graduation\n> candidates:\"\n>\n> $ git branch --no-merged master --merged next\n>\n> To accomplish this, store the merged and no-merged filters in two\n> commit_list structs (reachable_from and unreachable_from) rather than\n> using a single commit struct (merge_commit).\n>\n> If passed more than one merged (or more than one no-merged) filter,\n> mirror the existing boolean logic used by contains/no-contains filters:\n> refs must satisfy any of the merged filters, and all of the no-merged\n> filters.\n\nI am not sure if the parallel with \"contains\" should hold in the\nfirst place.\n\nThe way I would look at the --[no-]merged is that I'm enumerating\ncommits that would be shown in this corresponding revision walking\ncommand:\n\n\t$ git log --decorate --oneline ^master ^maint next seen\n\nIf the tip of a branch appears in that output, then the branch is\nincluded in the corresponding \"git branch\" output, and if it does\nnot, the branch is excluded from the corresponding \"git branch\"\noutput. \n\n\t$ git branch \\\n\t\t--no-merged master --no-merged maint \\\n\t\t--merged next --merged seen\n\nThat would mean the rule would be \"refs must be reachable by any one\n(or more) of the commits, and must be reachable by none of\nthe commits\". I am not using the same phrasing you used\n(i.e. \"must satisify ... filter\"), but are we saying the same thing?\nIt is unclear to me what you meant by \"all of the no-merged filters\".\n\nThe expectation is that topics in flight are either reachable from\n'next' or 'seen' (there can be commits in 'next' but not in 'seen'\nwhen fixes to mismerges are involved) and those already graduated\nare either in 'master' or 'maint', and the above \"log\" and \"branch\"\nwould show the stuff still in flight. But if you mean by \"all of\nthe no-merged filters\" that it must be in both 'master' and 'maint'\nfor a commit to be excluded from the output, it would not behave as\nI would expect.\n\nIn _my_ history, since 'maint' is always kept as a subset to\n'master' because I refrain from cherry-picking a fix that is in\n'master' down to 'maint', I can use a single \"--no-merged master\" to\nwork around the problem, but in a larger scale projects where\ncherry-picking fixes to maintainance track is more common, the above\ndoes not sound so useful, at least at the first glance.\n\nSo I dunno.\n\nThanks.\n"},{"id":"405211","messageId":"CAEJZ43ivtDS4mG35gt7HyKVWqLdS6XCwt5j8ZEYgDZDTdwE0OA@mail.gmail.com","threadId":"54211","inReplyTo":"xmqqsgbrx4xo.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] ref-filter: allow merged and no-merged filters","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-08T23:57:52Z","receivedAt":"2020-09-08T23:58:08Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"> That would mean the rule would be \"refs must be reachable by any one\n> (or more) of the commits, and must be reachable by none of\n> the commits\". I am not using the same phrasing you used\n> (i.e. \"must satisify ... filter\"), but are we saying the same thing?\n\nYes, that is how I've implemented this. (Your wording may be more\nclear, I'll plan on updating the commit message.)\n\n> The expectation is that topics in flight are either reachable from\n> 'next' or 'seen' (there can be commits in 'next' but not in 'seen'\n> when fixes to mismerges are involved) and those already graduated\n> are either in 'master' or 'maint', and the above \"log\" and \"branch\"\n> would show the stuff still in flight.\n\nI think we're on the same page: If a branch is merged into 'seen' but\nnot 'next', it should show up in the output of\n\"git branch --merged seen --merged next\".\n\nIf a branch is merged into 'master' but not 'maint', it should not\nshow up in the output of\n\"git branch --no-merged master --no-merged maint\".\n\nTo clarify, is that the behavior that makes sense/seems useful to you?\nIf so, I can add some test cases for those filter combinations in v2.\n"},{"id":"405213","messageId":"xmqqblifx1hm.fsf@gitster.c.googlers.com","threadId":"54211","inReplyTo":"CAEJZ43ivtDS4mG35gt7HyKVWqLdS6XCwt5j8ZEYgDZDTdwE0OA@mail.gmail.com","subject":"Re: [PATCH] ref-filter: allow merged and no-merged filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-09T00:00:53Z","receivedAt":"2020-09-09T00:01:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Aaron Lipman writes:\n\n>> That would mean the rule would be \"refs must be reachable by any one\n>> (or more) of the commits, and must be reachable by none of\n>> the commits\". I am not using the same phrasing you used\n>> (i.e. \"must satisify ... filter\"), but are we saying the same thing?\n>\n> Yes, that is how I've implemented this. (Your wording may be more\n> clear, I'll plan on updating the commit message.)\n>\n>> The expectation is that topics in flight are either reachable from\n>> 'next' or 'seen' (there can be commits in 'next' but not in 'seen'\n>> when fixes to mismerges are involved) and those already graduated\n>> are either in 'master' or 'maint', and the above \"log\" and \"branch\"\n>> would show the stuff still in flight.\n>\n> I think we're on the same page: If a branch is merged into 'seen' but\n> not 'next', it should show up in the output of\n> \"git branch --merged seen --merged next\".\n>\n> If a branch is merged into 'master' but not 'maint', it should not\n> show up in the output of\n> \"git branch --no-merged master --no-merged maint\".\n>\n> To clarify, is that the behavior that makes sense/seems useful to you?\n> If so, I can add some test cases for those filter combinations in v2.\n\nYup, drawing the parallel to how revision walker combines multiple\npositive and negative starting points would feel the most natural to\nexperienced Git users, I would say.\n\nThanks.\n"},{"id":"405435","messageId":"20200911185754.64173-1-alipman88@gmail.com","threadId":"54211","inReplyTo":"20200908153759.36504-1-alipman88@gmail.com","subject":"[PATCH v2 0/2] git branch: allow combining merged and no-merged filters","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-11T18:57:52Z","receivedAt":"2020-09-11T18:58:16Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"Enable ref-filter to process multiple merged and no-merged filters,\nand extend functionality to git branch, git tag and git\nfor-each-ref. This provides an easy way to check for branches that\nare \"graduation candidates:\"\n\n$ git branch --merged next --no-merged master\n\nCurrently, this required a command like:\n\n$ grep -xf \\\n <(git branch --merged next) \\\n <(git branch --no-merged master)\n\nAlso, add tests describing git branch's behavior when combining\nmultiple contains/no-contains filters - this helps demonstrate\nconsistency between merged/no-merged and contains/no-contains.\n\nAaron Lipman (2):\n t3201: test multiple branch filter combinations\n ref-filter: allow merged and no-merged filters\n\n Documentation/git-branch.txt | 6 +--\n Documentation/git-for-each-ref.txt | 6 +--\n Documentation/git-tag.txt | 4 +-\n builtin/branch.c | 4 +-\n builtin/tag.c | 6 +--\n ref-filter.c | 71 ++++++++++++++++--------------\n ref-filter.h | 9 +---\n t/t3200-branch.sh | 4 +-\n t/t3201-branch-contains.sh | 69 +++++++++++++++++++++++++++--\n t/t6302-for-each-ref-filter.sh | 4 +-\n t/t7004-tag.sh | 2 +-\n 11 files changed, 122 insertions(+), 63 deletions(-)\n\n-- \n2.24.3 (Apple Git-128)\n\n"},{"id":"405436","messageId":"20200911185754.64173-2-alipman88@gmail.com","threadId":"54211","inReplyTo":"20200911185754.64173-1-alipman88@gmail.com","subject":"[PATCH v2 1/2] t3201: test multiple branch filter combinations","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-11T18:57:53Z","receivedAt":"2020-09-11T18:58:27Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"Add tests covering the behavior of passing multiple contains/no-contains\nfilters to git branch, e.g.:\n\n$ git branch --contains feature_a --contains feature_b\n$ git branch --no-contains feature_a --no-contains feature_b\n\nWhen passed more than one contains (or no-contains) filter, the tips of\nthe branches returned must be reachable from any of the contains commits\nand from none of the the no-contains commits. (This logic is useful to\ndescribe prior to enabling multiple merged/no-merged filters, so that\nfuture tests will demonstrate consistent behavior between\nmerged/no-merged and contains/no-contains filters.)\n\nSigned-off-by: Aaron Lipman \n---\n t/t3201-branch-contains.sh | 44 ++++++++++++++++++++++++++++++++++----\n 1 file changed, 40 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh\nindex 40251c9f8f..cd205b5560 100755\n--- a/t/t3201-branch-contains.sh\n+++ b/t/t3201-branch-contains.sh\n@@ -200,15 +200,51 @@ test_expect_success 'branch --merged with --verbose' '\n \ttest_i18ncmp expect actual\n '\n \n-test_expect_success 'branch --contains combined with --no-contains' '\n-\tgit branch --contains zzz --no-contains topic >actual &&\n+# The next series of tests covers multiple filter combinations\n+test_expect_success 'set up repo for multiple filter combination tests' '\n+\tgit checkout master &&\n+\tgit branch | grep -v master | xargs git branch -D &&\n+\tgit checkout -b feature_a master &&\n+\t>feature_a &&\n+\tgit add feature_a &&\n+\tgit commit -m \"add feature a\" &&\n+\tgit checkout -b feature_b master &&\n+\t>feature_b &&\n+\tgit add feature_b &&\n+\tgit commit -m \"add feature b\"\n+'\n+\n+test_expect_success 'multiple branch --contains' '\n+\tgit checkout -b next master &&\n+\tgit merge feature_a &&\n+\tgit branch --contains feature_a --contains feature_b >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t feature_a\n+\t feature_b\n+\t* next\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'multiple branch --no-contains' '\n+\tgit branch --no-contains feature_a --no-contains feature_b >actual &&\n \tcat >expect <<-\\EOF &&\n \t master\n-\t side\n-\t zzz\n \tEOF\n \ttest_cmp expect actual\n+'\n \n+test_expect_success 'branch --contains combined with --no-contains' '\n+\tgit checkout master &&\n+\tgit merge feature_a &&\n+\tgit checkout next &&\n+\tgit merge feature_b &&\n+\tgit branch --contains feature_a --no-contains feature_b >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t feature_a\n+\t master\n+\tEOF\n+\ttest_cmp expect actual\n '\n \n test_done\n-- \n2.24.3 (Apple Git-128)\n\n"},{"id":"405437","messageId":"20200911185754.64173-3-alipman88@gmail.com","threadId":"54211","inReplyTo":"20200911185754.64173-1-alipman88@gmail.com","subject":"[PATCH v2 2/2] ref-filter: allow merged and no-merged filters","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-11T18:57:54Z","receivedAt":"2020-09-11T18:58:33Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"Enable ref-filter to process multiple merged and no-merged filters, and\nextend functionality to git branch, git tag and git for-each-ref. This\nprovides an easy way to check for branches that are \"graduation\ncandidates:\"\n\n$ git branch --merged next --no-merged master\n\nTo accomplish this, store the merged and no-merged filters in two\ncommit_list structs (reachable_from and unreachable_from) rather than\nusing a single commit struct (merge_commit).\n\nIf passed more than one merged (or more than one no-merged) filter, refs\nmust be reachable from any one of the merged commits, and reachable from\nnone of the no-merged commits.\n\nSigned-off-by: Aaron Lipman \n---\n Documentation/git-branch.txt | 6 +--\n Documentation/git-for-each-ref.txt | 6 +--\n Documentation/git-tag.txt | 4 +-\n builtin/branch.c | 4 +-\n builtin/tag.c | 6 +--\n ref-filter.c | 71 ++++++++++++++++--------------\n ref-filter.h | 9 +---\n t/t3200-branch.sh | 4 +-\n t/t3201-branch-contains.sh | 27 ++++++++++++\n t/t6302-for-each-ref-filter.sh | 4 +-\n t/t7004-tag.sh | 2 +-\n 11 files changed, 83 insertions(+), 60 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 03c0824d52..8f0dbcd0ac 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -252,13 +252,11 @@ start-point is either a local or remote-tracking branch.\n \n --merged []::\n \tOnly list branches whose tips are reachable from the\n-\tspecified commit (HEAD if not specified). Implies `--list`,\n-\tincompatible with `--no-merged`.\n+\tspecified commit (HEAD if not specified). Implies `--list`.\n \n --no-merged []::\n \tOnly list branches whose tips are not reachable from the\n-\tspecified commit (HEAD if not specified). Implies `--list`,\n-\tincompatible with `--merged`.\n+\tspecified commit (HEAD if not specified). Implies `--list`.\n \n ::\n \tThe name of the branch to create or delete.\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 616ce46087..f8322a7be2 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -76,13 +76,11 @@ OPTIONS\n \n --merged[=]::\n \tOnly list refs whose tips are reachable from the\n-\tspecified commit (HEAD if not specified),\n-\tincompatible with `--no-merged`.\n+\tspecified commit (HEAD if not specified).\n \n --no-merged[=]::\n \tOnly list refs whose tips are not reachable from the\n-\tspecified commit (HEAD if not specified),\n-\tincompatible with `--merged`.\n+\tspecified commit (HEAD if not specified).\n \n --contains[=]::\n \tOnly list refs which contain the specified commit (HEAD if not\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex f6d9791780..786d4dfd6f 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -149,11 +149,11 @@ This option is only applicable when listing tags without annotation lines.\n \n --merged []::\n \tOnly list tags whose commits are reachable from the specified\n-\tcommit (`HEAD` if not specified), incompatible with `--no-merged`.\n+\tcommit (`HEAD` if not specified).\n \n --no-merged []::\n \tOnly list tags whose commits are not reachable from the specified\n-\tcommit (`HEAD` if not specified), incompatible with `--merged`.\n+\tcommit (`HEAD` if not specified).\n \n --points-at ::\n \tOnly list tags of the given object (HEAD if not\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex e82301fb1b..4bdb700dd5 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -688,8 +688,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t !show_current && !unset_upstream && argc == 0)\n \t\tlist = 1;\n \n-\tif (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr ||\n-\t filter.no_commit)\n+\tif (filter.with_commit || filter.no_commit ||\n+\t filter.reachable_from || filter.unreachable_from || filter.points_at.nr)\n \t\tlist = 1;\n \n \tif (!!delete + !!rename + !!copy + !!new_upstream + !!show_current +\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 5cbd80dc3e..b1a0398c85 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -457,8 +457,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tif (argc == 0)\n \t\t\tcmdmode = 'l';\n \t\telse if (filter.with_commit || filter.no_commit ||\n-\t\t\t filter.points_at.nr || filter.merge_commit ||\n-\t\t\t filter.lines != -1)\n+\t\t\t filter.reachable_from || filter.unreachable_from ||\n+\t\t\t filter.points_at.nr || filter.lines != -1)\n \t\t\tcmdmode = 'l';\n \t}\n \n@@ -509,7 +509,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"--no-contains option is only allowed in list mode\"));\n \tif (filter.points_at.nr)\n \t\tdie(_(\"--points-at option is only allowed in list mode\"));\n-\tif (filter.merge_commit)\n+\tif (filter.reachable_from || filter.unreachable_from)\n \t\tdie(_(\"--merged and --no-merged options are only allowed in list mode\"));\n \tif (cmdmode == 'd')\n \t\treturn for_each_tag_name(argv, delete_tag, NULL);\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 110bcd741a..c04dca47d1 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2167,9 +2167,9 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \t * obtain the commit using the 'oid' available and discard all\n \t * non-commits early. The actual filtering is done later.\n \t */\n-\tif (filter->merge_commit || filter->with_commit || filter->no_commit || filter->verbose) {\n-\t\tcommit = lookup_commit_reference_gently(the_repository, oid,\n-\t\t\t\t\t\t\t1);\n+\tif (filter->reachable_from || filter->unreachable_from ||\n+\t filter->with_commit || filter->no_commit || filter->verbose) {\n+\t\tcommit = lookup_commit_reference_gently(the_repository, oid, 1);\n \t\tif (!commit)\n \t\t\treturn 0;\n \t\t/* We perform the filtering for the '--contains' option... */\n@@ -2231,11 +2231,17 @@ void ref_array_clear(struct ref_array *array)\n \t}\n }\n \n-static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)\n+static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)\n {\n+\tstruct commit_list *check_reachable_list = reachable ?\n+\t\tref_cbdata->filter->reachable_from :\n+\t\tref_cbdata->filter->unreachable_from;\n+\n+\tif (!check_reachable_list)\n+\t\treturn;\n+\n \tstruct rev_info revs;\n \tint i, old_nr;\n-\tstruct ref_filter *filter = ref_cbdata->filter;\n \tstruct ref_array *array = ref_cbdata->array;\n \tstruct commit **to_clear = xcalloc(sizeof(struct commit *), array->nr);\n \n@@ -2247,12 +2253,15 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)\n \t\tto_clear[i] = item->commit;\n \t}\n \n-\tfilter->merge_commit->object.flags |= UNINTERESTING;\n-\tadd_pending_object(&revs, &filter->merge_commit->object, \"\");\n+\tfor (struct commit_list *rl = check_reachable_list; rl; rl = rl->next) {\n+\t\tstruct commit *merge_commit = rl->item;\n+\t\tmerge_commit->object.flags |= UNINTERESTING;\n+\t\tadd_pending_object(&revs, &merge_commit->object, \"\");\n \n-\trevs.limited = 1;\n-\tif (prepare_revision_walk(&revs))\n-\t\tdie(_(\"revision walk setup failed\"));\n+\t\trevs.limited = 1;\n+\t\tif (prepare_revision_walk(&revs))\n+\t\t\tdie(_(\"revision walk setup failed\"));\n+\t}\n \n \told_nr = array->nr;\n \tarray->nr = 0;\n@@ -2263,14 +2272,19 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)\n \n \t\tint is_merged = !!(commit->object.flags & UNINTERESTING);\n \n-\t\tif (is_merged == (filter->merge == REF_FILTER_MERGED_INCLUDE))\n+\t\tif (is_merged == reachable)\n \t\t\tarray->items[array->nr++] = array->items[i];\n \t\telse\n \t\t\tfree_array_item(item);\n-\t}\n+ }\n \n \tclear_commit_marks_many(old_nr, to_clear, ALL_REV_FLAGS);\n-\tclear_commit_marks(filter->merge_commit, ALL_REV_FLAGS);\n+\n+\twhile (check_reachable_list) {\n+\t\tstruct commit *merge_commit = pop_commit(&check_reachable_list);\n+\t\tclear_commit_marks(merge_commit, ALL_REV_FLAGS);\n+\t}\n+\n \tfree(to_clear);\n }\n \n@@ -2322,8 +2336,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \tclear_contains_cache(&ref_cbdata.no_contains_cache);\n \n \t/* Filters that need revision walking */\n-\tif (filter->merge_commit)\n-\t\tdo_merge_filter(&ref_cbdata);\n+\tdo_merge_filter(&ref_cbdata, 1);\n+\tdo_merge_filter(&ref_cbdata, 0);\n \n \treturn ret;\n }\n@@ -2541,31 +2555,22 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)\n {\n \tstruct ref_filter *rf = opt->value;\n \tstruct object_id oid;\n-\tint no_merged = starts_with(opt->long_name, \"no\");\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tif (rf->merge) {\n-\t\tif (no_merged) {\n-\t\t\treturn error(_(\"option `%s' is incompatible with --merged\"),\n-\t\t\t\t opt->long_name);\n-\t\t} else {\n-\t\t\treturn error(_(\"option `%s' is incompatible with --no-merged\"),\n-\t\t\t\t opt->long_name);\n-\t\t}\n-\t}\n-\n-\trf->merge = no_merged\n-\t\t? REF_FILTER_MERGED_OMIT\n-\t\t: REF_FILTER_MERGED_INCLUDE;\n-\n \tif (get_oid(arg, &oid))\n \t\tdie(_(\"malformed object name %s\"), arg);\n \n-\trf->merge_commit = lookup_commit_reference_gently(the_repository,\n-\t\t\t\t\t\t\t &oid, 0);\n-\tif (!rf->merge_commit)\n+\tstruct commit *merge_commit = lookup_commit_reference_gently(the_repository,\n+\t\t\t\t\t\t\t\t &oid, 0);\n+\n+\tif (!merge_commit)\n \t\treturn error(_(\"option `%s' must point to a commit\"), opt->long_name);\n \n+\tif (starts_with(opt->long_name, \"no\"))\n+\t\tcommit_list_insert(merge_commit, &rf->unreachable_from);\n+\telse\n+\t\tcommit_list_insert(merge_commit, &rf->reachable_from);\n+\n \treturn 0;\n }\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 8ecc33cdfa..feaef4a8fd 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -54,13 +54,8 @@ struct ref_filter {\n \tstruct oid_array points_at;\n \tstruct commit_list *with_commit;\n \tstruct commit_list *no_commit;\n-\n-\tenum {\n-\t\tREF_FILTER_MERGED_NONE = 0,\n-\t\tREF_FILTER_MERGED_INCLUDE,\n-\t\tREF_FILTER_MERGED_OMIT\n-\t} merge;\n-\tstruct commit *merge_commit;\n+\tstruct commit_list *reachable_from;\n+\tstruct commit_list *unreachable_from;\n \n \tunsigned int with_commit_tag_algo : 1,\n \t\tmatch_as_path : 1,\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 028c88d1b2..be8f61b751 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1299,8 +1299,8 @@ test_expect_success '--merged catches invalid object names' '\n \ttest_must_fail git branch --merged 0000000000000000000000000000000000000000\n '\n \n-test_expect_success '--merged is incompatible with --no-merged' '\n-\ttest_must_fail git branch --merged HEAD --no-merged HEAD\n+test_expect_success '--merged is compatible with --no-merged' '\n+\tgit branch --merged master --no-merged master\n '\n \n test_expect_success '--list during rebase' '\ndiff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh\nindex cd205b5560..d1ec594f6a 100755\n--- a/t/t3201-branch-contains.sh\n+++ b/t/t3201-branch-contains.sh\n@@ -226,6 +226,16 @@ test_expect_success 'multiple branch --contains' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'multiple branch --merged' '\n+\tgit branch --merged next --merged master >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t feature_a\n+\t master\n+\t* next\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'multiple branch --no-contains' '\n \tgit branch --no-contains feature_a --no-contains feature_b >actual &&\n \tcat >expect <<-\\EOF &&\n@@ -234,6 +244,14 @@ test_expect_success 'multiple branch --no-contains' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'multiple branch --no-merged' '\n+\tgit branch --no-merged next --no-merged master >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t feature_b\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'branch --contains combined with --no-contains' '\n \tgit checkout master &&\n \tgit merge feature_a &&\n@@ -247,4 +265,13 @@ test_expect_success 'branch --contains combined with --no-contains' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'branch --merged combined with --no-merged' '\n+\tgit branch --merged next --no-merged master >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t feature_b\n+\t* next\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 35408d53fd..781e470aea 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -437,8 +437,8 @@ test_expect_success 'check %(if:notequals=)' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success '--merged is incompatible with --no-merged' '\n-\ttest_must_fail git for-each-ref --merged HEAD --no-merged HEAD\n+test_expect_success '--merged is compatible with --no-merged' '\n+\tgit for-each-ref --merged HEAD --no-merged HEAD\n '\n \n test_expect_success 'validate worktree atom' '\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 74b637deb2..7d544eceda 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -2016,7 +2016,7 @@ test_expect_success '--merged can be used in non-list mode' '\n '\n \n test_expect_success '--merged is incompatible with --no-merged' '\n-\ttest_must_fail git tag --merged HEAD --no-merged HEAD\n+\tgit tag --merged HEAD --no-merged HEAD\n '\n \n test_expect_success '--merged shows merged tags' '\n-- \n2.24.3 (Apple Git-128)\n\n"},{"id":"405458","messageId":"xmqqlfhgm1ez.fsf@gitster.c.googlers.com","threadId":"54211","inReplyTo":"20200911185754.64173-3-alipman88@gmail.com","subject":"Re: [PATCH v2 2/2] ref-filter: allow merged and no-merged filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-11T21:47:00Z","receivedAt":"2020-09-11T21:47:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Aaron Lipman writes:\n\n> If passed more than one merged (or more than one no-merged) filter, refs\n> must be reachable from any one of the merged commits, and reachable from\n> none of the no-merged commits.\n\nThis mirrors how the \"contains\" behaves, which is good.\n\n> Documentation/git-branch.txt | 6 +--\n> Documentation/git-for-each-ref.txt | 6 +--\n> Documentation/git-tag.txt | 4 +-\n\nIt is a bit sad that this only removes from documentation without\nadding (the removal is because merged and no-merged are no longer\nmutually exclusive). There should be a new description on how it\nbehaves when both of them are given---the combination were impossible\nso it was sufficient to say \"they are incompatible\", but now the user\nneeds to know how they interact with each other.\n\nPerhaps --contains side can already be combined so they have\ndescription we can borrow here? I didn't look too carefully.\n\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 110bcd741a..c04dca47d1 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2231,11 +2231,17 @@ void ref_array_clear(struct ref_array *array)\n> \t}\n> }\n> \n> -static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)\n> +static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)\n> {\n> +\tstruct commit_list *check_reachable_list = reachable ?\n> +\t\tref_cbdata->filter->reachable_from :\n> +\t\tref_cbdata->filter->unreachable_from;\n> +\n> +\tif (!check_reachable_list)\n> +\t\treturn;\n> +\n\nWe do not allow decl-after-statement.\n\n> \tstruct rev_info revs;\n> \tint i, old_nr;\n> -\tstruct ref_filter *filter = ref_cbdata->filter;\n> \tstruct ref_array *array = ref_cbdata->array;\n> \tstruct commit **to_clear = xcalloc(sizeof(struct commit *), array->nr);\n> \n> @@ -2247,12 +2253,15 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)\n> \t\tto_clear[i] = item->commit;\n> \t}\n> \n> -\tfilter->merge_commit->object.flags |= UNINTERESTING;\n> -\tadd_pending_object(&revs, &filter->merge_commit->object, \"\");\n> +\tfor (struct commit_list *rl = check_reachable_list; rl; rl = rl->next) {\n> +\t\tstruct commit *merge_commit = rl->item;\n> +\t\tmerge_commit->object.flags |= UNINTERESTING;\n> +\t\tadd_pending_object(&revs, &merge_commit->object, \"\");\n> -\trevs.limited = 1;\n> -\tif (prepare_revision_walk(&revs))\n> -\t\tdie(_(\"revision walk setup failed\"));\n> +\t\trevs.limited = 1;\n> +\t\tif (prepare_revision_walk(&revs))\n> +\t\t\tdie(_(\"revision walk setup failed\"));\n> +\t}\n\nThis looks wrong. I would have expected, instead of placing the\nsingle object to the pending queue, the loop places all the filter\nobjects in a queue, and then makes a limited revision walk just\nonce. In general, each time after the code makes a call to\nprepare_revision_walk() to perform a revision walk, before doing so\nagain, the object flags used for the walking must be cleared. You\ncan tell that the original code structure follows the pattern.\nCalling it inside a loop breaks the pattern a big time.\n\nIn any case, we mark the named commits as UNINTERESTING and walk\nfrom them and the tips of (surviving) refs to see which refs are\nreachable from _any_ of the named commits. The original code is a\ndegenerated case of having only one named commit.\n\nThe idea of the original is when checking for \"--merged\", any ref\nwhose tip is reachable from the named commit is merged and should be\nshown. That extends naturally to multiple commits given to\n\"--merged\"; any ref whose tip is reachable from one of the named\ncommits is selected.\n\nFor \"--no-merged\", any ref whose tip is reachable from the named\ncommit is merged and should be excluded. That also extends to\nmultiple commits. Any ref whose tip is reachable from one of the\n\"--no-merged\" commit is rejected.\n\n> @@ -2263,14 +2272,19 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)\n> \n> \t\tint is_merged = !!(commit->object.flags & UNINTERESTING);\n> \n> -\t\tif (is_merged == (filter->merge == REF_FILTER_MERGED_INCLUDE))\n> +\t\tif (is_merged == reachable)\n> \t\t\tarray->items[array->nr++] = array->items[i];\n> \t\telse\n> \t\t\tfree_array_item(item);\n\nSo, in the original code, if the tip of the ref is reachable from\nthe given commit (i.e. painted UNINTERESTING), we keep it in the\narray when looking for \"--merged\". That corresponds to the case \nwhere reachable is true. Similarly for the --no-merged case.\n\nSo the overall logic seems sound. It's just the callsite of\nprepare_revision_walk() which does the actual painting of the graph\nlooks wrong.\n\n> @@ -2541,31 +2555,22 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)\n> {\n> \tstruct ref_filter *rf = opt->value;\n> \tstruct object_id oid;\n> -\tint no_merged = starts_with(opt->long_name, \"no\");\n> \n> \tBUG_ON_OPT_NEG(unset);\n> \n> -\tif (rf->merge) {\n> -\t\tif (no_merged) {\n> -\t\t\treturn error(_(\"option `%s' is incompatible with --merged\"),\n> -\t\t\t\t opt->long_name);\n> -\t\t} else {\n> -\t\t\treturn error(_(\"option `%s' is incompatible with --no-merged\"),\n> -\t\t\t\t opt->long_name);\n> -\t\t}\n> -\t}\n> -\n> -\trf->merge = no_merged\n> -\t\t? REF_FILTER_MERGED_OMIT\n> -\t\t: REF_FILTER_MERGED_INCLUDE;\n> -\n> \tif (get_oid(arg, &oid))\n> \t\tdie(_(\"malformed object name %s\"), arg);\n> \n> -\trf->merge_commit = lookup_commit_reference_gently(the_repository,\n> -\t\t\t\t\t\t\t &oid, 0);\n> -\tif (!rf->merge_commit)\n> +\tstruct commit *merge_commit = lookup_commit_reference_gently(the_repository,\n> +\t\t\t\t\t\t\t\t &oid, 0);\n\ndecl-after-statement here, too.\n\n> +\tif (!merge_commit)\n> \t\treturn error(_(\"option `%s' must point to a commit\"), opt->long_name);\n> \n> +\tif (starts_with(opt->long_name, \"no\"))\n> +\t\tcommit_list_insert(merge_commit, &rf->unreachable_from);\n> +\telse\n> +\t\tcommit_list_insert(merge_commit, &rf->reachable_from);\n> +\n> \treturn 0;\n> }\n\nThanks.\n"},{"id":"405504","messageId":"20200913193140.66906-1-alipman88@gmail.com","threadId":"54211","inReplyTo":"20200911185754.64173-3-alipman88@gmail.com","subject":"[PATCH v3 0/3] git branch: allow combining merged and no-merged filters","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-13T19:31:37Z","receivedAt":"2020-09-13T19:31:57Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"> It is a bit sad that this only removes from documentation without\n> adding (the removal is because merged and no-merged are no longer\n> mutually exclusive)...\n>\n> Perhaps --contains side can already be combined so they have\n> description we can borrow here? I didn't look too carefully.\n\nSounds good, I've added documentation covering both multiple contains/\nno-contains and merged/no-merged filters to the notes sections of the\ndoc files for branch, for-each-ref & tag. (There was no existing\ndocumentation for --contains, so I added that in commit 2/3.)\n\n> We do not allow decl-after-statement.\n\nGot it, fixed both locations.\n\n> I would have expected, instead of placing the\n> single object to the pending queue, the loop places all the filter\n> objects in a queue, and then makes a limited revision walk just\n> once. In general, each time after the code makes a call to\n> prepare_revision_walk() to perform a revision walk, before doing so\n> again, the object flags used for the walking must be cleared.\n\nThanks for the explanation - updated accordingly. I'll plan on\nstudying that code as part of better familiarizing myself with\ngit's codebase.\n\nAaron Lipman (3):\n t3201: test multiple branch filter combinations\n Doc: cover multiple contains/no-contains filters\n ref-filter: allow merged and no-merged filters\n\n Documentation/git-branch.txt | 17 +++++---\n Documentation/git-for-each-ref.txt | 20 ++++++---\n Documentation/git-tag.txt | 17 ++++++--\n builtin/branch.c | 6 +--\n builtin/for-each-ref.c | 2 +-\n builtin/tag.c | 8 ++--\n ref-filter.c | 64 ++++++++++++++-------------\n ref-filter.h | 9 +---\n t/t3200-branch.sh | 4 +-\n t/t3201-branch-contains.sh | 69 ++++++++++++++++++++++++++++--\n t/t6302-for-each-ref-filter.sh | 4 +-\n t/t7004-tag.sh | 2 +-\n 12 files changed, 157 insertions(+), 65 deletions(-)\n\n-- \n2.24.3 (Apple Git-128)\n\n"},{"id":"405505","messageId":"20200913193140.66906-2-alipman88@gmail.com","threadId":"54211","inReplyTo":"20200913193140.66906-1-alipman88@gmail.com","subject":"[PATCH v3 1/3] t3201: test multiple branch filter combinations","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-13T19:31:38Z","receivedAt":"2020-09-13T19:32:15Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"Add tests covering the behavior of passing multiple contains/no-contains\nfilters to git branch, e.g.:\n\n$ git branch --contains feature_a --contains feature_b\n$ git branch --no-contains feature_a --no-contains feature_b\n\nWhen passed more than one contains (or no-contains) filter, the tips of\nthe branches returned must be reachable from any of the contains commits\nand from none of the no-contains commits.\n\nThis logic is useful to describe prior to enabling multiple\nmerged/no-merged filters, so that future tests will demonstrate\nconsistent behavior between merged/no-merged and contains/no-contains\nfilters.\n\nSigned-off-by: Aaron Lipman \n---\n t/t3201-branch-contains.sh | 44 ++++++++++++++++++++++++++++++++++----\n 1 file changed, 40 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh\nindex 40251c9f8f..cd205b5560 100755\n--- a/t/t3201-branch-contains.sh\n+++ b/t/t3201-branch-contains.sh\n@@ -200,15 +200,51 @@ test_expect_success 'branch --merged with --verbose' '\n \ttest_i18ncmp expect actual\n '\n \n-test_expect_success 'branch --contains combined with --no-contains' '\n-\tgit branch --contains zzz --no-contains topic >actual &&\n+# The next series of tests covers multiple filter combinations\n+test_expect_success 'set up repo for multiple filter combination tests' '\n+\tgit checkout master &&\n+\tgit branch | grep -v master | xargs git branch -D &&\n+\tgit checkout -b feature_a master &&\n+\t>feature_a &&\n+\tgit add feature_a &&\n+\tgit commit -m \"add feature a\" &&\n+\tgit checkout -b feature_b master &&\n+\t>feature_b &&\n+\tgit add feature_b &&\n+\tgit commit -m \"add feature b\"\n+'\n+\n+test_expect_success 'multiple branch --contains' '\n+\tgit checkout -b next master &&\n+\tgit merge feature_a &&\n+\tgit branch --contains feature_a --contains feature_b >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t feature_a\n+\t feature_b\n+\t* next\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'multiple branch --no-contains' '\n+\tgit branch --no-contains feature_a --no-contains feature_b >actual &&\n \tcat >expect <<-\\EOF &&\n \t master\n-\t side\n-\t zzz\n \tEOF\n \ttest_cmp expect actual\n+'\n \n+test_expect_success 'branch --contains combined with --no-contains' '\n+\tgit checkout master &&\n+\tgit merge feature_a &&\n+\tgit checkout next &&\n+\tgit merge feature_b &&\n+\tgit branch --contains feature_a --no-contains feature_b >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t feature_a\n+\t master\n+\tEOF\n+\ttest_cmp expect actual\n '\n \n test_done\n-- \n2.24.3 (Apple Git-128)\n\n"},{"id":"405506","messageId":"20200913193140.66906-3-alipman88@gmail.com","threadId":"54211","inReplyTo":"20200913193140.66906-1-alipman88@gmail.com","subject":"[PATCH v3 2/3] Doc: cover multiple contains/no-contains filters","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-13T19:31:39Z","receivedAt":"2020-09-13T19:32:16Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"Update documentation for \"git branch\", \"git for-each-ref\" and \"git tag\"\nwith notes explaining what happens when passed multiple --contains or\n--no-contains filters, e.g.:\n\n When combining multiple `--contains` and `--no-contains` filters,\n `git for-each-ref` shows refs containing at least one of the named\n `--contains` commits and none of the named `--no-contains` commits.\n\nThis behavior is useful to document prior to enabling multiple\nmerged/no-merged filters, in order to demonstrate consistent behavior\nbetween merged/no-merged and contains/no-contains filters.\n---\n Documentation/git-branch.txt | 4 ++++\n Documentation/git-for-each-ref.txt | 7 +++++++\n Documentation/git-tag.txt | 7 +++++++\n 3 files changed, 18 insertions(+)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 03c0824d52..2a36d29929 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -370,6 +370,10 @@ serve four related but different purposes:\n - `--no-merged` is used to find branches which are candidates for merging\n into HEAD, since those branches are not fully contained by HEAD.\n \n+When combining multiple `--contains` and `--no-contains` filters,\n+`git branch` shows branches containing at least one of the named\n+`--contains` commits and none of the named `--no-contains` commits.\n+\n SEE ALSO\n --------\n linkgit:git-check-ref-format[1],\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 616ce46087..e5f0226273 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -408,6 +408,13 @@ Note also that multiple copies of an object may be present in the object\n database; in this case, it is undefined which copy's size or delta base\n will be reported.\n \n+NOTES\n+-----\n+\n+When combining multiple `--contains` and `--no-contains` filters,\n+`git for-each-ref` shows refs containing at least one of the named\n+`--contains` commits and none of the named `--no-contains` commits.\n+\n SEE ALSO\n --------\n linkgit:git-show-ref[1]\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex f6d9791780..adfeab7a93 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -377,6 +377,13 @@ $ GIT_COMMITTER_DATE=\"2006-10-02 10:31\" git tag -s v1.0.1\n \n include::date-formats.txt[]\n \n+NOTES\n+-----\n+\n+When combining multiple `--contains` and `--no-contains` filters,\n+`git tag` shows tags containing at least one of the named `--contains`\n+commits and none of the named `--no-contains` commits.\n+\n SEE ALSO\n --------\n linkgit:git-check-ref-format[1].\n-- \n2.24.3 (Apple Git-128)\n\n"},{"id":"405507","messageId":"20200913193140.66906-4-alipman88@gmail.com","threadId":"54211","inReplyTo":"20200913193140.66906-1-alipman88@gmail.com","subject":"[PATCH v3 3/3] ref-filter: allow merged and no-merged filters","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-13T19:31:40Z","receivedAt":"2020-09-13T19:32:18Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"Enable ref-filter to process multiple merged and no-merged filters, and\nextend functionality to git branch, git tag and git for-each-ref. This\nprovides an easy way to check for branches that are \"graduation\ncandidates:\"\n\n$ git branch --no-merged master --merged next\n\nTo accomplish this, store the merged and no-merged filters in two\ncommit_list structs (reachable_from and unreachable_from) rather than\nusing a single commit struct (merge_commit).\n\nIf passed more than one merged (or more than one no-merged) filter, refs\nmust be reachable from any one of the merged commits, and reachable from\nnone of the no-merged commits.\n\nSigned-off-by: Aaron Lipman \n---\n Documentation/git-branch.txt | 13 +++---\n Documentation/git-for-each-ref.txt | 13 +++---\n Documentation/git-tag.txt | 10 +++--\n builtin/branch.c | 6 +--\n builtin/for-each-ref.c | 2 +-\n builtin/tag.c | 8 ++--\n ref-filter.c | 64 ++++++++++++++++--------------\n ref-filter.h | 9 +----\n t/t3200-branch.sh | 4 +-\n t/t3201-branch-contains.sh | 27 +++++++++++++\n t/t6302-for-each-ref-filter.sh | 4 +-\n t/t7004-tag.sh | 2 +-\n 12 files changed, 100 insertions(+), 62 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 2a36d29929..25992791fb 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n 'git branch' [--color[=] | --no-color] [--show-current]\n \t[-v [--abbrev= | --no-abbrev]]\n \t[--column[=] | --no-column] [--sort=]\n-\t[(--merged | --no-merged) []]\n+\t[--merged []] [--no-merged []]\n \t[--contains []] [--no-contains []]\n \t[--points-at ] [--format=]\n \t[(-r | --remotes) | (-a | --all)]\n@@ -252,13 +252,11 @@ start-point is either a local or remote-tracking branch.\n \n --merged []::\n \tOnly list branches whose tips are reachable from the\n-\tspecified commit (HEAD if not specified). Implies `--list`,\n-\tincompatible with `--no-merged`.\n+\tspecified commit (HEAD if not specified). Implies `--list`.\n \n --no-merged []::\n \tOnly list branches whose tips are not reachable from the\n-\tspecified commit (HEAD if not specified). Implies `--list`,\n-\tincompatible with `--merged`.\n+\tspecified commit (HEAD if not specified). Implies `--list`.\n \n ::\n \tThe name of the branch to create or delete.\n@@ -374,6 +372,11 @@ When combining multiple `--contains` and `--no-contains` filters,\n `git branch` shows branches containing at least one of the named\n `--contains` commits and none of the named `--no-contains` commits.\n \n+When combining multiple `--merged` and `--no-merged` filters,\n+`git branch` shows branches whose tips are reachable from at least one\n+of the named `--merged` commits and none of the named `--no-merged`\n+commits.\n+\n SEE ALSO\n --------\n linkgit:git-check-ref-format[1],\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex e5f0226273..a82716419d 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n 'git for-each-ref' [--count=] [--shell|--perl|--python|--tcl]\n \t\t [(--sort=)...] [--format=] [...]\n \t\t [--points-at=]\n-\t\t (--merged[=] | --no-merged[=])\n+\t\t [--merged[=]] [--no-merged[=]]\n \t\t [--contains[=]] [--no-contains[=]]\n \n DESCRIPTION\n@@ -76,13 +76,11 @@ OPTIONS\n \n --merged[=]::\n \tOnly list refs whose tips are reachable from the\n-\tspecified commit (HEAD if not specified),\n-\tincompatible with `--no-merged`.\n+\tspecified commit (HEAD if not specified).\n \n --no-merged[=]::\n \tOnly list refs whose tips are not reachable from the\n-\tspecified commit (HEAD if not specified),\n-\tincompatible with `--merged`.\n+\tspecified commit (HEAD if not specified).\n \n --contains[=]::\n \tOnly list refs which contain the specified commit (HEAD if not\n@@ -415,6 +413,11 @@ When combining multiple `--contains` and `--no-contains` filters,\n `git for-each-ref` shows refs containing at least one of the named\n `--contains` commits and none of the named `--no-contains` commits.\n \n+When combining multiple `--merged` and `--no-merged` filters,\n+`git for-each-ref` shows refs that are reachable from at least one of\n+the named `--merged` commits and none of the named `--no-merged`\n+commits.\n+\n SEE ALSO\n --------\n linkgit:git-show-ref[1]\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex adfeab7a93..84408784e6 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -15,7 +15,7 @@ SYNOPSIS\n 'git tag' [-n[]] -l [--contains ] [--no-contains ]\n \t[--points-at ] [--column[=] | --no-column]\n \t[--create-reflog] [--sort=] [--format=]\n-\t[--[no-]merged []] [...]\n+\t[--merged ] [--no-merged ] [...]\n 'git tag' -v [--format=] ...\n \n DESCRIPTION\n@@ -149,11 +149,11 @@ This option is only applicable when listing tags without annotation lines.\n \n --merged []::\n \tOnly list tags whose commits are reachable from the specified\n-\tcommit (`HEAD` if not specified), incompatible with `--no-merged`.\n+\tcommit (`HEAD` if not specified).\n \n --no-merged []::\n \tOnly list tags whose commits are not reachable from the specified\n-\tcommit (`HEAD` if not specified), incompatible with `--merged`.\n+\tcommit (`HEAD` if not specified).\n \n --points-at ::\n \tOnly list tags of the given object (HEAD if not\n@@ -384,6 +384,10 @@ When combining multiple `--contains` and `--no-contains` filters,\n `git tag` shows tags containing at least one of the named `--contains`\n commits and none of the named `--no-contains` commits.\n \n+When combining multiple `--merged` and `--no-merged` filters, `git tag`\n+shows tags whose commits are reachable from at least one of the named\n+`--merged` commits and none of the named `--no-merged` commits.\n+\n SEE ALSO\n --------\n linkgit:git-check-ref-format[1].\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex e82301fb1b..efb30b8820 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -26,7 +26,7 @@\n #include \"commit-reach.h\"\n \n static const char * const builtin_branch_usage[] = {\n-\tN_(\"git branch [] [-r | -a] [--merged | --no-merged]\"),\n+\tN_(\"git branch [] [-r | -a] [--merged] [--no-merged]\"),\n \tN_(\"git branch [] [-l] [-f] []\"),\n \tN_(\"git branch [] [-r] (-d | -D) ...\"),\n \tN_(\"git branch [] (-m | -M) [] \"),\n@@ -688,8 +688,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t !show_current && !unset_upstream && argc == 0)\n \t\tlist = 1;\n \n-\tif (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr ||\n-\t filter.no_commit)\n+\tif (filter.with_commit || filter.no_commit ||\n+\t filter.reachable_from || filter.unreachable_from || filter.points_at.nr)\n \t\tlist = 1;\n \n \tif (!!delete + !!rename + !!copy + !!new_upstream + !!show_current +\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 57489e4eab..9d1ecda2b8 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -9,7 +9,7 @@\n static char const * const for_each_ref_usage[] = {\n \tN_(\"git for-each-ref [] []\"),\n \tN_(\"git for-each-ref [--points-at ]\"),\n-\tN_(\"git for-each-ref [(--merged | --no-merged) []]\"),\n+\tN_(\"git for-each-ref [--merged []] [--no-merged []]\"),\n \tN_(\"git for-each-ref [--contains []] [--no-contains []]\"),\n \tNULL\n };\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 5cbd80dc3e..ecf011776d 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -26,7 +26,7 @@ static const char * const git_tag_usage[] = {\n \t\t\"\\t\\t []\"),\n \tN_(\"git tag -d ...\"),\n \tN_(\"git tag -l [-n[]] [--contains ] [--no-contains ] [--points-at ]\\n\"\n-\t\t\"\\t\\t[--format=] [--[no-]merged []] [...]\"),\n+\t\t\"\\t\\t[--format=] [--merged ] [--no-merged ] [...]\"),\n \tN_(\"git tag -v [--format=] ...\"),\n \tNULL\n };\n@@ -457,8 +457,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tif (argc == 0)\n \t\t\tcmdmode = 'l';\n \t\telse if (filter.with_commit || filter.no_commit ||\n-\t\t\t filter.points_at.nr || filter.merge_commit ||\n-\t\t\t filter.lines != -1)\n+\t\t\t filter.reachable_from || filter.unreachable_from ||\n+\t\t\t filter.points_at.nr || filter.lines != -1)\n \t\t\tcmdmode = 'l';\n \t}\n \n@@ -509,7 +509,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"--no-contains option is only allowed in list mode\"));\n \tif (filter.points_at.nr)\n \t\tdie(_(\"--points-at option is only allowed in list mode\"));\n-\tif (filter.merge_commit)\n+\tif (filter.reachable_from || filter.unreachable_from)\n \t\tdie(_(\"--merged and --no-merged options are only allowed in list mode\"));\n \tif (cmdmode == 'd')\n \t\treturn for_each_tag_name(argv, delete_tag, NULL);\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 110bcd741a..030ab55d03 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2167,9 +2167,9 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \t * obtain the commit using the 'oid' available and discard all\n \t * non-commits early. The actual filtering is done later.\n \t */\n-\tif (filter->merge_commit || filter->with_commit || filter->no_commit || filter->verbose) {\n-\t\tcommit = lookup_commit_reference_gently(the_repository, oid,\n-\t\t\t\t\t\t\t1);\n+\tif (filter->reachable_from || filter->unreachable_from ||\n+\t filter->with_commit || filter->no_commit || filter->verbose) {\n+\t\tcommit = lookup_commit_reference_gently(the_repository, oid, 1);\n \t\tif (!commit)\n \t\t\treturn 0;\n \t\t/* We perform the filtering for the '--contains' option... */\n@@ -2231,13 +2231,20 @@ void ref_array_clear(struct ref_array *array)\n \t}\n }\n \n-static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)\n+static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)\n {\n \tstruct rev_info revs;\n \tint i, old_nr;\n-\tstruct ref_filter *filter = ref_cbdata->filter;\n \tstruct ref_array *array = ref_cbdata->array;\n \tstruct commit **to_clear = xcalloc(sizeof(struct commit *), array->nr);\n+\tstruct commit_list *rl;\n+\n+\tstruct commit_list *check_reachable_list = reachable ?\n+\t\tref_cbdata->filter->reachable_from :\n+\t\tref_cbdata->filter->unreachable_from;\n+\n+\tif (!check_reachable_list)\n+\t\treturn;\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \n@@ -2247,8 +2254,11 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)\n \t\tto_clear[i] = item->commit;\n \t}\n \n-\tfilter->merge_commit->object.flags |= UNINTERESTING;\n-\tadd_pending_object(&revs, &filter->merge_commit->object, \"\");\n+\tfor (rl = check_reachable_list; rl; rl = rl->next) {\n+\t\tstruct commit *merge_commit = rl->item;\n+\t\tmerge_commit->object.flags |= UNINTERESTING;\n+\t\tadd_pending_object(&revs, &merge_commit->object, \"\");\n+\t}\n \n \trevs.limited = 1;\n \tif (prepare_revision_walk(&revs))\n@@ -2263,14 +2273,19 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)\n \n \t\tint is_merged = !!(commit->object.flags & UNINTERESTING);\n \n-\t\tif (is_merged == (filter->merge == REF_FILTER_MERGED_INCLUDE))\n+\t\tif (is_merged == reachable)\n \t\t\tarray->items[array->nr++] = array->items[i];\n \t\telse\n \t\t\tfree_array_item(item);\n \t}\n \n \tclear_commit_marks_many(old_nr, to_clear, ALL_REV_FLAGS);\n-\tclear_commit_marks(filter->merge_commit, ALL_REV_FLAGS);\n+\n+\twhile (check_reachable_list) {\n+\t\tstruct commit *merge_commit = pop_commit(&check_reachable_list);\n+\t\tclear_commit_marks(merge_commit, ALL_REV_FLAGS);\n+\t}\n+\n \tfree(to_clear);\n }\n \n@@ -2322,8 +2337,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \tclear_contains_cache(&ref_cbdata.no_contains_cache);\n \n \t/* Filters that need revision walking */\n-\tif (filter->merge_commit)\n-\t\tdo_merge_filter(&ref_cbdata);\n+\tdo_merge_filter(&ref_cbdata, 1);\n+\tdo_merge_filter(&ref_cbdata, 0);\n \n \treturn ret;\n }\n@@ -2541,31 +2556,22 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)\n {\n \tstruct ref_filter *rf = opt->value;\n \tstruct object_id oid;\n-\tint no_merged = starts_with(opt->long_name, \"no\");\n+\tstruct commit *merge_commit;\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tif (rf->merge) {\n-\t\tif (no_merged) {\n-\t\t\treturn error(_(\"option `%s' is incompatible with --merged\"),\n-\t\t\t\t opt->long_name);\n-\t\t} else {\n-\t\t\treturn error(_(\"option `%s' is incompatible with --no-merged\"),\n-\t\t\t\t opt->long_name);\n-\t\t}\n-\t}\n-\n-\trf->merge = no_merged\n-\t\t? REF_FILTER_MERGED_OMIT\n-\t\t: REF_FILTER_MERGED_INCLUDE;\n-\n \tif (get_oid(arg, &oid))\n \t\tdie(_(\"malformed object name %s\"), arg);\n \n-\trf->merge_commit = lookup_commit_reference_gently(the_repository,\n-\t\t\t\t\t\t\t &oid, 0);\n-\tif (!rf->merge_commit)\n+\tmerge_commit = lookup_commit_reference_gently(the_repository, &oid, 0);\n+\n+\tif (!merge_commit)\n \t\treturn error(_(\"option `%s' must point to a commit\"), opt->long_name);\n \n+\tif (starts_with(opt->long_name, \"no\"))\n+\t\tcommit_list_insert(merge_commit, &rf->unreachable_from);\n+\telse\n+\t\tcommit_list_insert(merge_commit, &rf->reachable_from);\n+\n \treturn 0;\n }\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 8ecc33cdfa..feaef4a8fd 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -54,13 +54,8 @@ struct ref_filter {\n \tstruct oid_array points_at;\n \tstruct commit_list *with_commit;\n \tstruct commit_list *no_commit;\n-\n-\tenum {\n-\t\tREF_FILTER_MERGED_NONE = 0,\n-\t\tREF_FILTER_MERGED_INCLUDE,\n-\t\tREF_FILTER_MERGED_OMIT\n-\t} merge;\n-\tstruct commit *merge_commit;\n+\tstruct commit_list *reachable_from;\n+\tstruct commit_list *unreachable_from;\n \n \tunsigned int with_commit_tag_algo : 1,\n \t\tmatch_as_path : 1,\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 028c88d1b2..be8f61b751 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1299,8 +1299,8 @@ test_expect_success '--merged catches invalid object names' '\n \ttest_must_fail git branch --merged 0000000000000000000000000000000000000000\n '\n \n-test_expect_success '--merged is incompatible with --no-merged' '\n-\ttest_must_fail git branch --merged HEAD --no-merged HEAD\n+test_expect_success '--merged is compatible with --no-merged' '\n+\tgit branch --merged master --no-merged master\n '\n \n test_expect_success '--list during rebase' '\ndiff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh\nindex cd205b5560..d1ec594f6a 100755\n--- a/t/t3201-branch-contains.sh\n+++ b/t/t3201-branch-contains.sh\n@@ -226,6 +226,16 @@ test_expect_success 'multiple branch --contains' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'multiple branch --merged' '\n+\tgit branch --merged next --merged master >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t feature_a\n+\t master\n+\t* next\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'multiple branch --no-contains' '\n \tgit branch --no-contains feature_a --no-contains feature_b >actual &&\n \tcat >expect <<-\\EOF &&\n@@ -234,6 +244,14 @@ test_expect_success 'multiple branch --no-contains' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'multiple branch --no-merged' '\n+\tgit branch --no-merged next --no-merged master >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t feature_b\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'branch --contains combined with --no-contains' '\n \tgit checkout master &&\n \tgit merge feature_a &&\n@@ -247,4 +265,13 @@ test_expect_success 'branch --contains combined with --no-contains' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'branch --merged combined with --no-merged' '\n+\tgit branch --merged next --no-merged master >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t feature_b\n+\t* next\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 35408d53fd..781e470aea 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -437,8 +437,8 @@ test_expect_success 'check %(if:notequals=)' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success '--merged is incompatible with --no-merged' '\n-\ttest_must_fail git for-each-ref --merged HEAD --no-merged HEAD\n+test_expect_success '--merged is compatible with --no-merged' '\n+\tgit for-each-ref --merged HEAD --no-merged HEAD\n '\n \n test_expect_success 'validate worktree atom' '\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 74b637deb2..7d544eceda 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -2016,7 +2016,7 @@ test_expect_success '--merged can be used in non-list mode' '\n '\n \n test_expect_success '--merged is incompatible with --no-merged' '\n-\ttest_must_fail git tag --merged HEAD --no-merged HEAD\n+\tgit tag --merged HEAD --no-merged HEAD\n '\n \n test_expect_success '--merged shows merged tags' '\n-- \n2.24.3 (Apple Git-128)\n\n"},{"id":"405508","messageId":"CAPig+cRrgC5vz9S1qDpEjoEvniBLCbuWt=bkpa2fj=suUm3Bog@mail.gmail.com","threadId":"54211","inReplyTo":"20200913193140.66906-2-alipman88@gmail.com","subject":"Re: [PATCH v3 1/3] t3201: test multiple branch filter combinations","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-13T20:58:14Z","receivedAt":"2020-09-13T20:58:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Sep 13, 2020 at 3:32 PM Aaron Lipman wrote:\n> Add tests covering the behavior of passing multiple contains/no-contains\n> filters to git branch, e.g.:\n> ---\n> diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh\n> @@ -200,15 +200,51 @@ test_expect_success 'branch --merged with --verbose' '\n> +# The next series of tests covers multiple filter combinations\n> +test_expect_success 'set up repo for multiple filter combination tests' '\n> + git checkout master &&\n> + git branch | grep -v master | xargs git branch -D &&\n> + git checkout -b feature_a master &&\n> + >feature_a &&\n> + git add feature_a &&\n> + git commit -m \"add feature a\" &&\n> + git checkout -b feature_b master &&\n> + >feature_b &&\n> + git add feature_b &&\n> + git commit -m \"add feature b\"\n> +'\n\nA few comments:\n\nI didn't examine it too closely, so this may be a silly question, but\nis there a reason to start from scratch (by deleting all the branches)\nrather than simply using or extending the existing branches like the\nother tests do?\n\nIf it really does make sense to start from scratch (ignoring the\nexisting branches), then an alternative would be to create a new\nrepository and run the tests in that repository instead. Whether or\nnot doing so makes sense in this case is a judgment call. For\ninstance:\n\n test_create_repo features\n (\n cd features\n ...setup stuff...\n )\n\nIt's a bit concerning to see output from porcelain git-branch being\nfed to 'grep' and 'xargs'. More typically, you would instead rely upon\nthe (stable) output of a plumbing command. For instance:\n\n git for-each-ref --format=\"%(refname:short)\" refs/heads/ | ...\n\nIn new test code, normally avoid having a Git command upstream of a\npipe since its exit code will be lost. Thus, you might instead write:\n\n git for-each-ref ... >heads &&\n grep -v master heads | xargs git branch -D &&\n"},{"id":"405509","messageId":"CAPig+cRaz+HPNW_WFX7UqcqtRwG_Td59LkXWF6HHoYOO1M6=bg@mail.gmail.com","threadId":"54211","inReplyTo":"20200913193140.66906-3-alipman88@gmail.com","subject":"Re: [PATCH v3 2/3] Doc: cover multiple contains/no-contains filters","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-13T21:10:33Z","receivedAt":"2020-09-13T21:10:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Sep 13, 2020 at 3:32 PM Aaron Lipman wrote:\n> Update documentation for \"git branch\", \"git for-each-ref\" and \"git tag\"\n> with notes explaining what happens when passed multiple --contains or\n> --no-contains filters, e.g.:\n>\n> When combining multiple `--contains` and `--no-contains` filters,\n> `git for-each-ref` shows refs containing at least one of the named\n> `--contains` commits and none of the named `--no-contains` commits.\n\nWe normally avoid repeating in the commit message what the patch\nitself already says. The first paragraph alone (without the example\ntext) would be plenty sufficient. (Not itself worth a re-roll,\nthough.)\n\n> This behavior is useful to document prior to enabling multiple\n> merged/no-merged filters, in order to demonstrate consistent behavior\n> between merged/no-merged and contains/no-contains filters.\n> ---\n\nMissing sign-off.\n\n> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\n> @@ -370,6 +370,10 @@ serve four related but different purposes:\n> +When combining multiple `--contains` and `--no-contains` filters,\n> +`git branch` shows branches containing at least one of the named\n> +`--contains` commits and none of the named `--no-contains` commits.\n\nThis is repeated nearly verbatim in the other two documentation pages.\nIt makes one wonder if it can be generalized and placed in its own\nfile which is included in the other documents via\n`include::contains.txt[]`. Perhaps like this:\n\n When combining multiple `--contains` and `--no-contains` filters,\n `git branch` shows references containing at least one of the named\n `--contains` commits and none of the named `--no-contains`\n commits.\n\nBut perhaps that's too generic?\n"},{"id":"405510","messageId":"CAPig+cTNp84Dm=0n-Bb9o=1nZNDFRE20XDWUPJT8yjdefv15rA@mail.gmail.com","threadId":"54211","inReplyTo":"20200913193140.66906-4-alipman88@gmail.com","subject":"Re: [PATCH v3 3/3] ref-filter: allow merged and no-merged filters","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-13T21:36:39Z","receivedAt":"2020-09-13T21:36:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Sep 13, 2020 at 3:32 PM Aaron Lipman wrote:\n> Enable ref-filter to process multiple merged and no-merged filters, and\n> extend functionality to git branch, git tag and git for-each-ref. This\n> provides an easy way to check for branches that are \"graduation\n> candidates:\"\n>\n> $ git branch --no-merged master --merged next\n\nA few subjective comments below, not necessarily worth a re-roll...\n\n> To accomplish this, store the merged and no-merged filters in two\n> commit_list structs (reachable_from and unreachable_from) rather than\n> using a single commit struct (merge_commit).\n\nThis sort of implementation detail is readily discoverable by reading\nthe patch itself, and since there is no complexity about it which\nrequires extensive explanation, we'd normally leave it out of the\ncommit message.\n\n> If passed more than one merged (or more than one no-merged) filter, refs\n> must be reachable from any one of the merged commits, and reachable from\n> none of the no-merged commits.\n>\n> Signed-off-by: Aaron Lipman \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -2231,13 +2231,20 @@ void ref_array_clear(struct ref_array *array)\n> -static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)\n> +static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)\n> {\n> [...]\n> + struct commit_list *check_reachable_list = reachable ?\n> + ref_cbdata->filter->reachable_from :\n> + ref_cbdata->filter->unreachable_from;\n> +\n> + if (!check_reachable_list)\n> + return;\n\nRather than adding a boolean 'reachable' parameter to the function\nsignature, you could instead directly pass in the `struct commit_list\n*` upon which to operate, which would allow you to drop the ternary\noperator here, and...\n\n> @@ -2322,8 +2337,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n> - if (filter->merge_commit)\n> - do_merge_filter(&ref_cbdata);\n> + do_merge_filter(&ref_cbdata, 1);\n> + do_merge_filter(&ref_cbdata, 0);\n\n... make the callers a bit less opaque by eliminating the\nless-than-meaningful 0-or-1, and making it obvious which list is being\nconsulted:\n\n do_merge_filter(&ref_cbdata, ref_cbdata->filter->reachable_from);\n do_merge_filter(&ref_cbdata, ref_cbdata->filter->unreachable_from);\n\n> @@ -2541,31 +2556,22 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)\n> + if (starts_with(opt->long_name, \"no\"))\n> + commit_list_insert(merge_commit, &rf->unreachable_from);\n> + else\n> + commit_list_insert(merge_commit, &rf->reachable_from);\n\n... quite like it's obvious here into which list the item is being inserted.\n\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> @@ -1299,8 +1299,8 @@ test_expect_success '--merged catches invalid object names' '\n> -test_expect_success '--merged is incompatible with --no-merged' '\n> - test_must_fail git branch --merged HEAD --no-merged HEAD\n> +test_expect_success '--merged is compatible with --no-merged' '\n> + git branch --merged master --no-merged master\n> '\n\nThis revised test doesn't seem to have all that much value since this\ncombination is checked by new tests added elsewhere by the patch.\nTherefore, another option would be simply to drop the test rather than\nrevising it. (But, again, it's a judgment call.)\n\n> diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh\n> @@ -226,6 +226,16 @@ test_expect_success 'multiple branch --contains' '\n> +test_expect_success 'multiple branch --merged' '\n> +test_expect_success 'multiple branch --no-merged' '\n> +test_expect_success 'branch --merged combined with --no-merged' '\n\nWould it make sense to also test multiple --merged and multiple\n--no-merged? (Genuine question, not a demand to add more tests.)\n\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> @@ -2016,7 +2016,7 @@ test_expect_success '--merged can be used in non-list mode' '\n> test_expect_success '--merged is incompatible with --no-merged' '\n> - test_must_fail git tag --merged HEAD --no-merged HEAD\n> + git tag --merged HEAD --no-merged HEAD\n> '\n\nI think you forgot s/incompatible/compatible/ in the test title (which\nyou changed in the other cases).\n"},{"id":"405512","messageId":"xmqqo8m9mjcb.fsf@gitster.c.googlers.com","threadId":"54211","inReplyTo":"CAPig+cTNp84Dm=0n-Bb9o=1nZNDFRE20XDWUPJT8yjdefv15rA@mail.gmail.com","subject":"Re: [PATCH v3 3/3] ref-filter: allow merged and no-merged filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-13T21:56:36Z","receivedAt":"2020-09-13T21:57:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> + struct commit_list *check_reachable_list = reachable ?\n>> + ref_cbdata->filter->reachable_from :\n>> + ref_cbdata->filter->unreachable_from;\n>> +\n>> + if (!check_reachable_list)\n>> + return;\n>\n> Rather than adding a boolean 'reachable' parameter to the function\n> signature, you could instead directly pass in the `struct commit_list\n> *` upon which to operate, which would allow you to drop the ternary\n> operator here, and...\n>\n>> @@ -2322,8 +2337,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n>> - if (filter->merge_commit)\n>> - do_merge_filter(&ref_cbdata);\n>> + do_merge_filter(&ref_cbdata, 1);\n>> + do_merge_filter(&ref_cbdata, 0);\n>\n> ... make the callers a bit less opaque by eliminating the\n> less-than-meaningful 0-or-1, and making it obvious which list is being\n> consulted:\n>\n> do_merge_filter(&ref_cbdata, ref_cbdata->filter->reachable_from);\n> do_merge_filter(&ref_cbdata, ref_cbdata->filter->unreachable_from);\n\n\nThere is this code in do_merge_filter(), though.\n\n\t\tif (is_merged == reachable)\n \t\t\tarray->items[array->nr++] = array->items[i];\n \t\telse\n \t\t\tfree_array_item(item);\n\nThis is a body of the loop that runs over (surviving) tips of ref,\nafter painting the commits from the tips of refs (interesting) and\nthe [un]reachable_from commits (uninteresting). The temporary\nvariable is_merged signals if the tip after revision walk is painted\nuninteresting, i.e. some of the [un]reachable_from commits reach the\ntip.\n\nIf 'reachable' and 'is_merged' are the same, that means either\n\n (1) the tip is reachable from some commit given as --merged\n ; or\n\n (2) the tip is NOT reachable from any commit given as --no-merged\n \n\nwhich means it survives this round of filtering.\n\nBy losing the 'reachable' bit, you make this switch impossible.\n\nI do not mind making the 0/1 a symbolic constant between\ndo_emreg_filter() and filter_refs() for enhanced readability,\nthough.\n\nThanks.\n"},{"id":"405513","messageId":"CAPig+cQewCckH2P5qW9DZqWOHU3ZsGyBUGStv6cKAwzBOh4Ptw@mail.gmail.com","threadId":"54211","inReplyTo":"xmqqo8m9mjcb.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 3/3] ref-filter: allow merged and no-merged filters","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-13T22:31:11Z","receivedAt":"2020-09-13T22:33:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Sep 13, 2020 at 5:56 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > Rather than adding a boolean 'reachable' parameter to the function\n> > signature, you could instead directly pass in the `struct commit_list\n> > *` upon which to operate, which would allow you to drop the ternary\n> > operator here, and...\n>\n> There is this code in do_merge_filter(), though.\n>\n> if (is_merged == reachable)\n> array->items[array->nr++] = array->items[i];\n> else\n> free_array_item(item);\n>\n> By losing the 'reachable' bit, you make this switch impossible.\n\nTrue. I missed that.\n"},{"id":"405523","messageId":"xmqqd02oktq8.fsf@gitster.c.googlers.com","threadId":"54211","inReplyTo":"CAPig+cRrgC5vz9S1qDpEjoEvniBLCbuWt=bkpa2fj=suUm3Bog@mail.gmail.com","subject":"Re: [PATCH v3 1/3] t3201: test multiple branch filter combinations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-14T20:07:27Z","receivedAt":"2020-09-14T20:07:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> A few comments:\n>\n> I didn't examine it too closely, so this may be a silly question, but\n> is there a reason to start from scratch (by deleting all the branches)\n> rather than simply using or extending the existing branches like the\n> other tests do?\n>\n> If it really does make sense to start from scratch (ignoring the\n> existing branches), then an alternative would be to create a new\n> repository and run the tests in that repository instead. Whether or\n> not doing so makes sense in this case is a judgment call. For\n> instance:\n>\n> test_create_repo features\n> (\n> cd features\n> ...setup stuff...\n> )\n\nGood comments; I agree with both.\n\n> It's a bit concerning to see output from porcelain git-branch being\n> fed to 'grep' and 'xargs'. More typically, you would instead rely upon\n> the (stable) output of a plumbing command. For instance:\n>\n> git for-each-ref --format=\"%(refname:short)\" refs/heads/ | ...\n>\n> In new test code, normally avoid having a Git command upstream of a\n> pipe since its exit code will be lost. Thus, you might instead write:\n>\n> git for-each-ref ... >heads &&\n> grep -v master heads | xargs git branch -D &&\n\nAgain, good recommendation.\n\nThank you always for helpful reviews.\n"},{"id":"405524","messageId":"xmqq5z8gktq2.fsf@gitster.c.googlers.com","threadId":"54211","inReplyTo":"CAPig+cRaz+HPNW_WFX7UqcqtRwG_Td59LkXWF6HHoYOO1M6=bg@mail.gmail.com","subject":"Re: [PATCH v3 2/3] Doc: cover multiple contains/no-contains filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-14T20:07:33Z","receivedAt":"2020-09-14T20:07:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\n>> @@ -370,6 +370,10 @@ serve four related but different purposes:\n>> +When combining multiple `--contains` and `--no-contains` filters,\n>> +`git branch` shows branches containing at least one of the named\n>> +`--contains` commits and none of the named `--no-contains` commits.\n>\n> This is repeated nearly verbatim in the other two documentation pages.\n> It makes one wonder if it can be generalized and placed in its own\n> file which is included in the other documents via\n> `include::contains.txt[]`. Perhaps like this:\n>\n> When combining multiple `--contains` and `--no-contains` filters,\n> `git branch` shows references containing at least one of the named\n> `--contains` commits and none of the named `--no-contains`\n> commits.\n>\n> But perhaps that's too generic?\n\nReplace `git branch` with \"this command\" and make it even more generic?\nOr eliminate the subject by going passive, e.g.\n\n Only references that contain at least one of the '--contains'\n commits but contain none of the '--no-contains' commits are\n shown\n\nor somesuch?\n\n"},{"id":"405593","messageId":"20200916020840.84892-1-alipman88@gmail.com","threadId":"54211","inReplyTo":"20200913193140.66906-1-alipman88@gmail.com","subject":"[PATCH v4 0/3] git branch: allow combining merged and no-merged filters","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-16T02:08:37Z","receivedAt":"2020-09-16T02:09:19Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"Thanks for the review, Eric & Junio.\n\nEric -\n\n> I didn't examine it too closely, so this may be a silly question, but\n> is there a reason to start from scratch (by deleting all the branches)\n> rather than simply using or extending the existing branches like the\n> other tests do?\n\nI went back and forth on this. There were a couple reasons I leaned\ntowards starting fresh - I found branch names like feature_a &\nfeature_b more illustrative, and I didn't want readers to have to\nscroll up further to find where branches came from.\n\nBut, with the tests re-ordered so \"branch --merged with --verbose\"\ncomes last (which adds new branches that might otherwise clutter up\nthe output of my new tests), I'm happy with using the existing test\nsetup - rewritten accordingly.\n\n> It's a bit concerning to see output from porcelain git-branch being\n> fed to 'grep' and 'xargs'. More typically, you would instead rely upon\n> the (stable) output of a plumbing command...\n\nThanks, useful knowledge for future contributions.\n\n> We normally avoid repeating in the commit message what the patch\n> itself already says. The first paragraph alone (without the example\n> text) would be plenty sufficient. (Not itself worth a re-roll,\n> though.)\n\nGot it, removed.\n\n> Missing sign-off.\n\nWhoops, fixed.\n\n> This is repeated nearly verbatim in the other two documentation pages.\n> It makes one wonder if it can be generalized and placed in its own\n> file which is included in the other documents via\n> `include::contains.txt[]`. Perhaps like this:\n> \n> When combining multiple `--contains` and `--no-contains` filters,\n> `git branch` shows references containing at least one of the named\n> `--contains` commits and none of the named `--no-contains`\n> commits.\n> \n> But perhaps that's too generic?\n\nCool, I hadn't realized we could embed snippets like that. Slightly\ngeneric, but I have no strong opinion either way. Going with the\npassive wording provided by Junio.\n\n(Looking at AsciiDoc's documentation, I think we could also set a\n:command-name: variable to insert some dynamic content into an\ninclude:: file.)\n\n> This sort of implementation detail is readily discoverable by reading\n> the patch itself, and since there is no complexity about it which\n> requires extensive explanation, we'd normally leave it out of the\n> commit message.\n\nRemoved.\n\n> This revised test doesn't seem to have all that much value since this\n> combination is checked by new tests added elsewhere by the patch.\n\nAgreed, dropped.\n\n> Would it make sense to also test multiple --merged and multiple\n> --no-merged? (Genuine question, not a demand to add more tests.)\n\nI don't see a reason to. The --merged and --no-merged filters are\napplied in separate passes, so I feel it's sufficient to test them\nindependently. (When doing my own QA testing, I did combine multiple\nmerged & multiple no-merged, multiple contains & multiple no-contains,\nmerged/no-merged & contains/no-contains, etc.)\n\nOn the other hand, extra test cases could help prevent regressions\nshould someone significantly refactor ref-filter.c. If anyone has a\npreference to add more tests, I'm happy to oblige.\n\n> I think you forgot s/incompatible/compatible/ in the test title (which\n> you changed in the other cases).\n\nThanks, fixed.\n\nJunio -\n\n> I do not mind making the 0/1 a symbolic constant between\n> do_merge_filter() and filter_refs() for enhanced readability,\n> though.\n\nIf I understand the convention, the constant names should be prefixed\nwith DO_MERGE_FILTER. I suggest DO_MERGE_FILTER_REACHABLE and\nDO_MERGE_FILTER_UNREACHABLE. Happy to re-roll if others have a\ndifferent preference - or feel free to edit.)\n\nAaron Lipman (3):\n t3201: test multiple branch filter combinations\n Doc: cover multiple contains/no-contains filters\n ref-filter: allow merged and no-merged filters\n\n Documentation/filters.txt | 7 +++\n Documentation/git-branch.txt | 10 ++--\n Documentation/git-for-each-ref.txt | 13 ++++--\n Documentation/git-tag.txt | 11 +++--\n builtin/branch.c | 6 +--\n builtin/for-each-ref.c | 2 +-\n builtin/tag.c | 8 ++--\n ref-filter.c | 64 ++++++++++++++------------\n ref-filter.h | 12 ++---\n t/t3200-branch.sh | 4 --\n t/t3201-branch-contains.sh | 74 +++++++++++++++++++++++++-----\n t/t6302-for-each-ref-filter.sh | 4 +-\n t/t7004-tag.sh | 4 +-\n 13 files changed, 143 insertions(+), 76 deletions(-)\n create mode 100644 Documentation/filters.txt\n\n-- \n2.24.3 (Apple Git-128)\n\n"},{"id":"405594","messageId":"20200916020840.84892-2-alipman88@gmail.com","threadId":"54211","inReplyTo":"20200916020840.84892-1-alipman88@gmail.com","subject":"[PATCH v4 1/3] t3201: test multiple branch filter combinations","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-16T02:08:38Z","receivedAt":"2020-09-16T02:09:30Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"Add tests covering the behavior of passing multiple contains/no-contains\nfilters to git branch, e.g.:\n\n$ git branch --contains feature_a --contains feature_b\n$ git branch --no-contains feature_a --no-contains feature_b\n\nWhen passed more than one contains (or no-contains) filter, the tips of\nthe branches returned must be reachable from any of the contains commits\nand from none of the the no-contains commits.\n\nThis logic is useful to describe prior to enabling multiple\nmerged/no-merged filters, so that future tests will demonstrate\nconsistent behavior between merged/no-merged and contains/no-contains\nfilters.\n\nSigned-off-by: Aaron Lipman \n---\n t/t3201-branch-contains.sh | 47 +++++++++++++++++++++++++++++---------\n 1 file changed, 36 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh\nindex 40251c9f8f..3cb9dc6cca 100755\n--- a/t/t3201-branch-contains.sh\n+++ b/t/t3201-branch-contains.sh\n@@ -171,6 +171,42 @@ test_expect_success 'Assert that --contains only works on commits, not trees & b\n \ttest_must_fail git branch --no-contains $blob\n '\n \n+test_expect_success 'multiple branch --contains' '\n+\tgit checkout -b side2 master &&\n+\t>feature &&\n+\tgit add feature &&\n+\tgit commit -m \"add feature\" &&\n+\tgit checkout -b next master &&\n+\tgit merge side &&\n+\tgit branch --contains side --contains side2 >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t* next\n+\t side\n+\t side2\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'multiple branch --no-contains' '\n+\tgit branch --no-contains side --no-contains side2 >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t master\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'branch --contains combined with --no-contains' '\n+\tgit checkout -b seen master &&\n+\tgit merge side &&\n+\tgit merge side2 &&\n+\tgit branch --contains side --no-contains side2 >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t next\n+\t side\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n # We want to set up a case where the walk for the tracking info\n # of one branch crosses the tip of another branch (and make sure\n # that the latter walk does not mess up our flag to see if it was\n@@ -200,15 +236,4 @@ test_expect_success 'branch --merged with --verbose' '\n \ttest_i18ncmp expect actual\n '\n \n-test_expect_success 'branch --contains combined with --no-contains' '\n-\tgit branch --contains zzz --no-contains topic >actual &&\n-\tcat >expect <<-\\EOF &&\n-\t master\n-\t side\n-\t zzz\n-\tEOF\n-\ttest_cmp expect actual\n-\n-'\n-\n test_done\n-- \n2.24.3 (Apple Git-128)\n\n"},{"id":"405595","messageId":"20200916020840.84892-3-alipman88@gmail.com","threadId":"54211","inReplyTo":"20200916020840.84892-1-alipman88@gmail.com","subject":"[PATCH v4 2/3] Doc: cover multiple contains/no-contains filters","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-16T02:08:39Z","receivedAt":"2020-09-16T02:09:33Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"Update documentation for \"git branch\", \"git for-each-ref\" and \"git tag\"\nwith notes explaining what happens when passed multiple --contains or\n--no-contains filters.\n\nThis behavior is useful to document prior to enabling multiple\nmerged/no-merged filters, in order to demonstrate consistent behavior\nbetween merged/no-merged and contains/no-contains filters.\n\nSigned-off-by: Aaron Lipman \n---\n Documentation/filters.txt | 3 +++\n Documentation/git-branch.txt | 2 ++\n Documentation/git-for-each-ref.txt | 5 +++++\n Documentation/git-tag.txt | 5 +++++\n 4 files changed, 15 insertions(+)\n create mode 100644 Documentation/filters.txt\n\ndiff --git a/Documentation/filters.txt b/Documentation/filters.txt\nnew file mode 100644\nindex 0000000000..4ee17afc01\n--- /dev/null\n+++ b/Documentation/filters.txt\n@@ -0,0 +1,3 @@\n+When combining multiple `--contains` and `--no-contains` filters, only\n+references that contain at least one of the `--contains` commits and\n+contain none of the `--no-contains` commits are shown.\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 03c0824d52..aa5e4da142 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -370,6 +370,8 @@ serve four related but different purposes:\n - `--no-merged` is used to find branches which are candidates for merging\n into HEAD, since those branches are not fully contained by HEAD.\n \n+include::filters.txt[]\n+\n SEE ALSO\n --------\n linkgit:git-check-ref-format[1],\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 616ce46087..c207ed9551 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -408,6 +408,11 @@ Note also that multiple copies of an object may be present in the object\n database; in this case, it is undefined which copy's size or delta base\n will be reported.\n \n+NOTES\n+-----\n+\n+include::filters.txt[]\n+\n SEE ALSO\n --------\n linkgit:git-show-ref[1]\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex f6d9791780..dadbd71d62 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -377,6 +377,11 @@ $ GIT_COMMITTER_DATE=\"2006-10-02 10:31\" git tag -s v1.0.1\n \n include::date-formats.txt[]\n \n+NOTES\n+-----\n+\n+include::filters.txt[]\n+\n SEE ALSO\n --------\n linkgit:git-check-ref-format[1].\n-- \n2.24.3 (Apple Git-128)\n\n"},{"id":"405596","messageId":"20200916020840.84892-4-alipman88@gmail.com","threadId":"54211","inReplyTo":"20200916020840.84892-1-alipman88@gmail.com","subject":"[PATCH v4 3/3] ref-filter: allow merged and no-merged filters","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-16T02:08:40Z","receivedAt":"2020-09-16T02:09:38Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"Enable ref-filter to process multiple merged and no-merged filters, and\nextend functionality to git branch, git tag and git for-each-ref. This\nprovides an easy way to check for branches that are \"graduation\ncandidates:\"\n\n$ git branch --no-merged master --merged next\n\nIf passed more than one merged (or more than one no-merged) filter, refs\nmust be reachable from any one of the merged commits, and reachable from\nnone of the no-merged commits.\n\nSigned-off-by: Aaron Lipman \n---\n Documentation/filters.txt | 4 ++\n Documentation/git-branch.txt | 8 ++--\n Documentation/git-for-each-ref.txt | 8 ++--\n Documentation/git-tag.txt | 6 +--\n builtin/branch.c | 6 +--\n builtin/for-each-ref.c | 2 +-\n builtin/tag.c | 8 ++--\n ref-filter.c | 64 ++++++++++++++++--------------\n ref-filter.h | 12 +++---\n t/t3200-branch.sh | 4 --\n t/t3201-branch-contains.sh | 27 +++++++++++++\n t/t6302-for-each-ref-filter.sh | 4 +-\n t/t7004-tag.sh | 4 +-\n 13 files changed, 92 insertions(+), 65 deletions(-)\n\ndiff --git a/Documentation/filters.txt b/Documentation/filters.txt\nindex 4ee17afc01..9bae46d84c 100644\n--- a/Documentation/filters.txt\n+++ b/Documentation/filters.txt\n@@ -1,3 +1,7 @@\n When combining multiple `--contains` and `--no-contains` filters, only\n references that contain at least one of the `--contains` commits and\n contain none of the `--no-contains` commits are shown.\n+\n+When combining multiple `--merged` and `--no-merged` filters, only\n+references that are reachable from at least one of the `--merged`\n+commits and from none of the `--no-merged` commits are shown.\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex aa5e4da142..290b90639c 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n 'git branch' [--color[=] | --no-color] [--show-current]\n \t[-v [--abbrev= | --no-abbrev]]\n \t[--column[=] | --no-column] [--sort=]\n-\t[(--merged | --no-merged) []]\n+\t[--merged []] [--no-merged []]\n \t[--contains []] [--no-contains []]\n \t[--points-at ] [--format=]\n \t[(-r | --remotes) | (-a | --all)]\n@@ -252,13 +252,11 @@ start-point is either a local or remote-tracking branch.\n \n --merged []::\n \tOnly list branches whose tips are reachable from the\n-\tspecified commit (HEAD if not specified). Implies `--list`,\n-\tincompatible with `--no-merged`.\n+\tspecified commit (HEAD if not specified). Implies `--list`.\n \n --no-merged []::\n \tOnly list branches whose tips are not reachable from the\n-\tspecified commit (HEAD if not specified). Implies `--list`,\n-\tincompatible with `--merged`.\n+\tspecified commit (HEAD if not specified). Implies `--list`.\n \n ::\n \tThe name of the branch to create or delete.\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex c207ed9551..7b9cf0ef1f 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n 'git for-each-ref' [--count=] [--shell|--perl|--python|--tcl]\n \t\t [(--sort=)...] [--format=] [...]\n \t\t [--points-at=]\n-\t\t (--merged[=] | --no-merged[=])\n+\t\t [--merged[=]] [--no-merged[=]]\n \t\t [--contains[=]] [--no-contains[=]]\n \n DESCRIPTION\n@@ -76,13 +76,11 @@ OPTIONS\n \n --merged[=]::\n \tOnly list refs whose tips are reachable from the\n-\tspecified commit (HEAD if not specified),\n-\tincompatible with `--no-merged`.\n+\tspecified commit (HEAD if not specified).\n \n --no-merged[=]::\n \tOnly list refs whose tips are not reachable from the\n-\tspecified commit (HEAD if not specified),\n-\tincompatible with `--merged`.\n+\tspecified commit (HEAD if not specified).\n \n --contains[=]::\n \tOnly list refs which contain the specified commit (HEAD if not\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex dadbd71d62..cc667d7d01 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -15,7 +15,7 @@ SYNOPSIS\n 'git tag' [-n[]] -l [--contains ] [--no-contains ]\n \t[--points-at ] [--column[=] | --no-column]\n \t[--create-reflog] [--sort=] [--format=]\n-\t[--[no-]merged []] [...]\n+\t[--merged ] [--no-merged ] [...]\n 'git tag' -v [--format=] ...\n \n DESCRIPTION\n@@ -149,11 +149,11 @@ This option is only applicable when listing tags without annotation lines.\n \n --merged []::\n \tOnly list tags whose commits are reachable from the specified\n-\tcommit (`HEAD` if not specified), incompatible with `--no-merged`.\n+\tcommit (`HEAD` if not specified).\n \n --no-merged []::\n \tOnly list tags whose commits are not reachable from the specified\n-\tcommit (`HEAD` if not specified), incompatible with `--merged`.\n+\tcommit (`HEAD` if not specified).\n \n --points-at ::\n \tOnly list tags of the given object (HEAD if not\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex e82301fb1b..efb30b8820 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -26,7 +26,7 @@\n #include \"commit-reach.h\"\n \n static const char * const builtin_branch_usage[] = {\n-\tN_(\"git branch [] [-r | -a] [--merged | --no-merged]\"),\n+\tN_(\"git branch [] [-r | -a] [--merged] [--no-merged]\"),\n \tN_(\"git branch [] [-l] [-f] []\"),\n \tN_(\"git branch [] [-r] (-d | -D) ...\"),\n \tN_(\"git branch [] (-m | -M) [] \"),\n@@ -688,8 +688,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t !show_current && !unset_upstream && argc == 0)\n \t\tlist = 1;\n \n-\tif (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr ||\n-\t filter.no_commit)\n+\tif (filter.with_commit || filter.no_commit ||\n+\t filter.reachable_from || filter.unreachable_from || filter.points_at.nr)\n \t\tlist = 1;\n \n \tif (!!delete + !!rename + !!copy + !!new_upstream + !!show_current +\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 57489e4eab..9d1ecda2b8 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -9,7 +9,7 @@\n static char const * const for_each_ref_usage[] = {\n \tN_(\"git for-each-ref [] []\"),\n \tN_(\"git for-each-ref [--points-at ]\"),\n-\tN_(\"git for-each-ref [(--merged | --no-merged) []]\"),\n+\tN_(\"git for-each-ref [--merged []] [--no-merged []]\"),\n \tN_(\"git for-each-ref [--contains []] [--no-contains []]\"),\n \tNULL\n };\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 5cbd80dc3e..ecf011776d 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -26,7 +26,7 @@ static const char * const git_tag_usage[] = {\n \t\t\"\\t\\t []\"),\n \tN_(\"git tag -d ...\"),\n \tN_(\"git tag -l [-n[]] [--contains ] [--no-contains ] [--points-at ]\\n\"\n-\t\t\"\\t\\t[--format=] [--[no-]merged []] [...]\"),\n+\t\t\"\\t\\t[--format=] [--merged ] [--no-merged ] [...]\"),\n \tN_(\"git tag -v [--format=] ...\"),\n \tNULL\n };\n@@ -457,8 +457,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tif (argc == 0)\n \t\t\tcmdmode = 'l';\n \t\telse if (filter.with_commit || filter.no_commit ||\n-\t\t\t filter.points_at.nr || filter.merge_commit ||\n-\t\t\t filter.lines != -1)\n+\t\t\t filter.reachable_from || filter.unreachable_from ||\n+\t\t\t filter.points_at.nr || filter.lines != -1)\n \t\t\tcmdmode = 'l';\n \t}\n \n@@ -509,7 +509,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"--no-contains option is only allowed in list mode\"));\n \tif (filter.points_at.nr)\n \t\tdie(_(\"--points-at option is only allowed in list mode\"));\n-\tif (filter.merge_commit)\n+\tif (filter.reachable_from || filter.unreachable_from)\n \t\tdie(_(\"--merged and --no-merged options are only allowed in list mode\"));\n \tif (cmdmode == 'd')\n \t\treturn for_each_tag_name(argv, delete_tag, NULL);\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 110bcd741a..785785a757 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2167,9 +2167,9 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \t * obtain the commit using the 'oid' available and discard all\n \t * non-commits early. The actual filtering is done later.\n \t */\n-\tif (filter->merge_commit || filter->with_commit || filter->no_commit || filter->verbose) {\n-\t\tcommit = lookup_commit_reference_gently(the_repository, oid,\n-\t\t\t\t\t\t\t1);\n+\tif (filter->reachable_from || filter->unreachable_from ||\n+\t filter->with_commit || filter->no_commit || filter->verbose) {\n+\t\tcommit = lookup_commit_reference_gently(the_repository, oid, 1);\n \t\tif (!commit)\n \t\t\treturn 0;\n \t\t/* We perform the filtering for the '--contains' option... */\n@@ -2231,13 +2231,20 @@ void ref_array_clear(struct ref_array *array)\n \t}\n }\n \n-static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)\n+static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)\n {\n \tstruct rev_info revs;\n \tint i, old_nr;\n-\tstruct ref_filter *filter = ref_cbdata->filter;\n \tstruct ref_array *array = ref_cbdata->array;\n \tstruct commit **to_clear = xcalloc(sizeof(struct commit *), array->nr);\n+\tstruct commit_list *rl;\n+\n+\tstruct commit_list *check_reachable_list = reachable ?\n+\t\tref_cbdata->filter->reachable_from :\n+\t\tref_cbdata->filter->unreachable_from;\n+\n+\tif (!check_reachable_list)\n+\t\treturn;\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \n@@ -2247,8 +2254,11 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)\n \t\tto_clear[i] = item->commit;\n \t}\n \n-\tfilter->merge_commit->object.flags |= UNINTERESTING;\n-\tadd_pending_object(&revs, &filter->merge_commit->object, \"\");\n+\tfor (rl = check_reachable_list; rl; rl = rl->next) {\n+\t\tstruct commit *merge_commit = rl->item;\n+\t\tmerge_commit->object.flags |= UNINTERESTING;\n+\t\tadd_pending_object(&revs, &merge_commit->object, \"\");\n+\t}\n \n \trevs.limited = 1;\n \tif (prepare_revision_walk(&revs))\n@@ -2263,14 +2273,19 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)\n \n \t\tint is_merged = !!(commit->object.flags & UNINTERESTING);\n \n-\t\tif (is_merged == (filter->merge == REF_FILTER_MERGED_INCLUDE))\n+\t\tif (is_merged == reachable)\n \t\t\tarray->items[array->nr++] = array->items[i];\n \t\telse\n \t\t\tfree_array_item(item);\n \t}\n \n \tclear_commit_marks_many(old_nr, to_clear, ALL_REV_FLAGS);\n-\tclear_commit_marks(filter->merge_commit, ALL_REV_FLAGS);\n+\n+\twhile (check_reachable_list) {\n+\t\tstruct commit *merge_commit = pop_commit(&check_reachable_list);\n+\t\tclear_commit_marks(merge_commit, ALL_REV_FLAGS);\n+\t}\n+\n \tfree(to_clear);\n }\n \n@@ -2322,8 +2337,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \tclear_contains_cache(&ref_cbdata.no_contains_cache);\n \n \t/* Filters that need revision walking */\n-\tif (filter->merge_commit)\n-\t\tdo_merge_filter(&ref_cbdata);\n+\tdo_merge_filter(&ref_cbdata, DO_MERGE_FILTER_REACHABLE);\n+\tdo_merge_filter(&ref_cbdata, DO_MERGE_FILTER_UNREACHABLE);\n \n \treturn ret;\n }\n@@ -2541,31 +2556,22 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)\n {\n \tstruct ref_filter *rf = opt->value;\n \tstruct object_id oid;\n-\tint no_merged = starts_with(opt->long_name, \"no\");\n+\tstruct commit *merge_commit;\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tif (rf->merge) {\n-\t\tif (no_merged) {\n-\t\t\treturn error(_(\"option `%s' is incompatible with --merged\"),\n-\t\t\t\t opt->long_name);\n-\t\t} else {\n-\t\t\treturn error(_(\"option `%s' is incompatible with --no-merged\"),\n-\t\t\t\t opt->long_name);\n-\t\t}\n-\t}\n-\n-\trf->merge = no_merged\n-\t\t? REF_FILTER_MERGED_OMIT\n-\t\t: REF_FILTER_MERGED_INCLUDE;\n-\n \tif (get_oid(arg, &oid))\n \t\tdie(_(\"malformed object name %s\"), arg);\n \n-\trf->merge_commit = lookup_commit_reference_gently(the_repository,\n-\t\t\t\t\t\t\t &oid, 0);\n-\tif (!rf->merge_commit)\n+\tmerge_commit = lookup_commit_reference_gently(the_repository, &oid, 0);\n+\n+\tif (!merge_commit)\n \t\treturn error(_(\"option `%s' must point to a commit\"), opt->long_name);\n \n+\tif (starts_with(opt->long_name, \"no\"))\n+\t\tcommit_list_insert(merge_commit, &rf->unreachable_from);\n+\telse\n+\t\tcommit_list_insert(merge_commit, &rf->reachable_from);\n+\n \treturn 0;\n }\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 8ecc33cdfa..2d13928455 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -23,6 +23,9 @@\n #define FILTER_REFS_DETACHED_HEAD 0x0020\n #define FILTER_REFS_KIND_MASK (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)\n \n+#define DO_MERGE_FILTER_UNREACHABLE 0\n+#define DO_MERGE_FILTER_REACHABLE 1\n+\n struct atom_value;\n \n struct ref_sorting {\n@@ -54,13 +57,8 @@ struct ref_filter {\n \tstruct oid_array points_at;\n \tstruct commit_list *with_commit;\n \tstruct commit_list *no_commit;\n-\n-\tenum {\n-\t\tREF_FILTER_MERGED_NONE = 0,\n-\t\tREF_FILTER_MERGED_INCLUDE,\n-\t\tREF_FILTER_MERGED_OMIT\n-\t} merge;\n-\tstruct commit *merge_commit;\n+\tstruct commit_list *reachable_from;\n+\tstruct commit_list *unreachable_from;\n \n \tunsigned int with_commit_tag_algo : 1,\n \t\tmatch_as_path : 1,\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 028c88d1b2..c24c6632ee 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1299,10 +1299,6 @@ test_expect_success '--merged catches invalid object names' '\n \ttest_must_fail git branch --merged 0000000000000000000000000000000000000000\n '\n \n-test_expect_success '--merged is incompatible with --no-merged' '\n-\ttest_must_fail git branch --merged HEAD --no-merged HEAD\n-'\n-\n test_expect_success '--list during rebase' '\n \ttest_when_finished \"reset_rebase\" &&\n \tgit checkout master &&\ndiff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh\nindex 3cb9dc6cca..efea5c4971 100755\n--- a/t/t3201-branch-contains.sh\n+++ b/t/t3201-branch-contains.sh\n@@ -187,6 +187,16 @@ test_expect_success 'multiple branch --contains' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'multiple branch --merged' '\n+\tgit branch --merged next --merged master >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t master\n+\t* next\n+\t side\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'multiple branch --no-contains' '\n \tgit branch --no-contains side --no-contains side2 >actual &&\n \tcat >expect <<-\\EOF &&\n@@ -195,6 +205,14 @@ test_expect_success 'multiple branch --no-contains' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'multiple branch --no-merged' '\n+\tgit branch --no-merged next --no-merged master >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t side2\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'branch --contains combined with --no-contains' '\n \tgit checkout -b seen master &&\n \tgit merge side &&\n@@ -207,6 +225,15 @@ test_expect_success 'branch --contains combined with --no-contains' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'branch --merged combined with --no-merged' '\n+\tgit branch --merged seen --no-merged next >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t* seen\n+\t side2\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n # We want to set up a case where the walk for the tracking info\n # of one branch crosses the tip of another branch (and make sure\n # that the latter walk does not mess up our flag to see if it was\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 35408d53fd..781e470aea 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -437,8 +437,8 @@ test_expect_success 'check %(if:notequals=)' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success '--merged is incompatible with --no-merged' '\n-\ttest_must_fail git for-each-ref --merged HEAD --no-merged HEAD\n+test_expect_success '--merged is compatible with --no-merged' '\n+\tgit for-each-ref --merged HEAD --no-merged HEAD\n '\n \n test_expect_success 'validate worktree atom' '\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 74b637deb2..05f411c821 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -2015,8 +2015,8 @@ test_expect_success '--merged can be used in non-list mode' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success '--merged is incompatible with --no-merged' '\n-\ttest_must_fail git tag --merged HEAD --no-merged HEAD\n+test_expect_success '--merged is compatible with --no-merged' '\n+\tgit tag --merged HEAD --no-merged HEAD\n '\n \n test_expect_success '--merged shows merged tags' '\n-- \n2.24.3 (Apple Git-128)\n\n"},{"id":"405600","messageId":"xmqq8sdaxqyu.fsf@gitster.c.googlers.com","threadId":"54211","inReplyTo":"20200916020840.84892-1-alipman88@gmail.com","subject":"Re: [PATCH v4 0/3] git branch: allow combining merged and no-merged filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-16T04:53:13Z","receivedAt":"2020-09-16T04:53:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Aaron Lipman writes:\n\n>> I do not mind making the 0/1 a symbolic constant between\n>> do_merge_filter() and filter_refs() for enhanced readability,\n>> though.\n>\n> If I understand the convention, the constant names should be prefixed\n> with DO_MERGE_FILTER. I suggest DO_MERGE_FILTER_REACHABLE and\n> DO_MERGE_FILTER_UNREACHABLE. Happy to re-roll if others have a\n> different preference - or feel free to edit.)\n\nEven though I said \"I do not mind\", using DO_MERGE_FILTER prefix is\nway too much noise. The common prefix is appropriate for external\nAPI, but this is merely an internal contract between a private\nhelper do_merge_filter() and its only caller.\n\nIf I were redesigning the code around this area, I probably would\nrename do_merge_filter() to something more descriptive, and tweak\nits parameters a bit more focused, e.g.\n\n #define EXCLUDE_REACHED 0\n #define INCLUDE_REACHED 1\n static void reach_filter(struct ref_array *array, \n\t\t\t struct commit_list *check_reachable,\n\t\t\t int include_reached) {\n\t\t...\n\t\tfor (i = 0; i < old_nr; i++) {\n\t\t\tstruct commit *c = array->items[i];\n\t\t\tint is_reached = !!(c->object.flags & UNINTERESTING);\n\n\t\t\tif (is_reached == include_reached)\n\t\t\t\tarray->items[array->nr++] = array->items[i];\n\t\t\telse\n\t\t\t\tfree_array_item(array->items[i]);\n\t\t}\n\t\t...\n }\n\n /* the caller */\n ...\n reach_filter(array, filter->reachable_from, INCLUDE_REACHED);\n reach_filter(array, filter->unreachable_from, EXCLUDE_REACHED);\n\nto make it clear which part of the callback struct is really passed\nbetween the caller and the helper. Even if we are not renaming\nthings that much, a locally defined preprocessor macro with shorter\nnames, defined just before the callee, would be more appropriate for\na case like this, with a single callee called by only one caller.\n\nThanks.\n"},{"id":"405601","messageId":"CAPig+cR7DnufEALmmW4e-6zxDsrGa_MeHT32mG5n1EQ6xEs8+A@mail.gmail.com","threadId":"54211","inReplyTo":"xmqq8sdaxqyu.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v4 0/3] git branch: allow combining merged and no-merged filters","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-16T05:08:24Z","receivedAt":"2020-09-16T05:08:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 16, 2020 at 12:53 AM Junio C Hamano wrote:\n> Even though I said \"I do not mind\", using DO_MERGE_FILTER prefix is\n> way too much noise. The common prefix is appropriate for external\n> API, but this is merely an internal contract between a private\n> helper do_merge_filter() and its only caller.\n>\n> #define EXCLUDE_REACHED 0\n> #define INCLUDE_REACHED 1\n> static void reach_filter(struct ref_array *array,\n> struct commit_list *check_reachable,\n> int include_reached) {\n>\n> to make it clear which part of the callback struct is really passed\n> between the caller and the helper. Even if we are not renaming\n> things that much, a locally defined preprocessor macro with shorter\n> names, defined just before the callee, would be more appropriate for\n> a case like this, with a single callee called by only one caller.\n\nThanks for stating what I was planning on saying, with particular\nemphasis on keeping these #defines in the .c file rather than the .h\nfile since they are not part of the public API.\n\nAside from that, this re-roll seems to address all of my previous\nreview comments. Thanks.\n"},{"id":"405670","messageId":"xmqq4knxy08w.fsf@gitster.c.googlers.com","threadId":"54211","inReplyTo":"20200916020840.84892-3-alipman88@gmail.com","subject":"Re: [PATCH v4 2/3] Doc: cover multiple contains/no-contains filters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-16T19:45:03Z","receivedAt":"2020-09-16T19:45:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Aaron Lipman writes:\n\n> Update documentation for \"git branch\", \"git for-each-ref\" and \"git tag\"\n> with notes explaining what happens when passed multiple --contains or\n> --no-contains filters.\n>\n> This behavior is useful to document prior to enabling multiple\n> merged/no-merged filters, in order to demonstrate consistent behavior\n> between merged/no-merged and contains/no-contains filters.\n>\n> Signed-off-by: Aaron Lipman \n> ---\n> Documentation/filters.txt | 3 +++\n\n\"git\" has more than one concepts that relate to the word \"filter\",\nlike the one that are used to create lazy clone, the \"filter-branch\"\ncommand, \"smudge/clean\" filter, \"textconv\" filter used by the diff\nand grep machinery to name some.\n\nMake sure you do not accidentally squat on a good-sounding but\nan overly generic name. ref-reachability-filters.txt perhaps?\n\n> diff --git a/Documentation/filters.txt b/Documentation/filters.txt\n> new file mode 100644\n> index 0000000000..4ee17afc01\n> --- /dev/null\n> +++ b/Documentation/filters.txt\n> @@ -0,0 +1,3 @@\n> +When combining multiple `--contains` and `--no-contains` filters, only\n> +references that contain at least one of the `--contains` commits and\n> +contain none of the `--no-contains` commits are shown.\n"},{"id":"405813","messageId":"20200918004909.32474-1-alipman88@gmail.com","threadId":"54211","inReplyTo":"20200916020840.84892-1-alipman88@gmail.com","subject":"[PATCH 0/2] ref-filter: merged & no-merged touch-ups","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-18T00:49:07Z","receivedAt":"2020-09-18T00:49:25Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"> > Even if we are not renaming\n> > things that much, a locally defined preprocessor macro with shorter\n> > names, defined just before the callee, would be more appropriate for\n> > a case like this, with a single callee called by only one caller.\n\n> Thanks for stating what I was planning on saying, with particular\n> emphasis on keeping these #defines in the .c file rather than the .h\n> file since they are not part of the public API.\n\nThanks for the insight into defining macros in source vs header files-\nI'm still in the process of learning C and figuring out how to convey\npurpose & encapsulation without relying on features of higher order\nlanguages I'm used to (e.g. keyword args).\n\nSince this got moved to next, I'm not entirely clear on whether folks\nwant me to keep tweaking this, or if the last set of comments were\ngeneral advice for future contributions.\n\nBut in the interest of diligence, I'd like to offer a couple touch-ups\napplying Junio's suggestions.\n\nAaron Lipman (2):\n ref-filter: refactor do_merge_filter()\n Doc: prefer more specific file name\n\n Documentation/git-branch.txt | 2 +-\n Documentation/git-for-each-ref.txt | 2 +-\n Documentation/git-tag.txt | 2 +-\n ...lters.txt => ref-reachability-filters.txt} | 0\n ref-filter.c | 29 +++++++++----------\n ref-filter.h | 3 --\n 6 files changed, 17 insertions(+), 21 deletions(-)\n rename Documentation/{filters.txt => ref-reachability-filters.txt} (100%)\n\n-- \n2.24.3 (Apple Git-128)\n\n"},{"id":"405814","messageId":"20200918004909.32474-2-alipman88@gmail.com","threadId":"54211","inReplyTo":"20200918004909.32474-1-alipman88@gmail.com","subject":"[PATCH 1/2] ref-filter: refactor do_merge_filter()","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-18T00:49:08Z","receivedAt":"2020-09-18T00:49:26Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"Rename to reach_filter() to be more descriptive.\n\nSeparate parameters to explicitly identify what data is used by the\nfunction, instead of passing an entire ref_filter_cbdata struct.\n\nRename and move associated preprocessor macros from header to source\nfile, as they're only used internally in a single location.\n\nSigned-off-by: Aaron Lipman \n---\n ref-filter.c | 29 ++++++++++++++---------------\n ref-filter.h | 3 ---\n 2 files changed, 14 insertions(+), 18 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 785785a757..5550a0d34c 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2231,19 +2231,18 @@ void ref_array_clear(struct ref_array *array)\n \t}\n }\n \n-static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)\n+#define EXCLUDE_REACHED 0\n+#define INCLUDE_REACHED 1\n+static void reach_filter(struct ref_array *array,\n+\t\t\t struct commit_list *check_reachable,\n+\t\t\t int include_reached)\n {\n \tstruct rev_info revs;\n \tint i, old_nr;\n-\tstruct ref_array *array = ref_cbdata->array;\n \tstruct commit **to_clear = xcalloc(sizeof(struct commit *), array->nr);\n-\tstruct commit_list *rl;\n+\tstruct commit_list *cr;\n \n-\tstruct commit_list *check_reachable_list = reachable ?\n-\t\tref_cbdata->filter->reachable_from :\n-\t\tref_cbdata->filter->unreachable_from;\n-\n-\tif (!check_reachable_list)\n+\tif (!check_reachable)\n \t\treturn;\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n@@ -2254,8 +2253,8 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)\n \t\tto_clear[i] = item->commit;\n \t}\n \n-\tfor (rl = check_reachable_list; rl; rl = rl->next) {\n-\t\tstruct commit *merge_commit = rl->item;\n+\tfor (cr = check_reachable; cr; cr = cr->next) {\n+\t\tstruct commit *merge_commit = cr->item;\n \t\tmerge_commit->object.flags |= UNINTERESTING;\n \t\tadd_pending_object(&revs, &merge_commit->object, \"\");\n \t}\n@@ -2273,7 +2272,7 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)\n \n \t\tint is_merged = !!(commit->object.flags & UNINTERESTING);\n \n-\t\tif (is_merged == reachable)\n+\t\tif (is_merged == include_reached)\n \t\t\tarray->items[array->nr++] = array->items[i];\n \t\telse\n \t\t\tfree_array_item(item);\n@@ -2281,8 +2280,8 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)\n \n \tclear_commit_marks_many(old_nr, to_clear, ALL_REV_FLAGS);\n \n-\twhile (check_reachable_list) {\n-\t\tstruct commit *merge_commit = pop_commit(&check_reachable_list);\n+\twhile (check_reachable) {\n+\t\tstruct commit *merge_commit = pop_commit(&check_reachable);\n \t\tclear_commit_marks(merge_commit, ALL_REV_FLAGS);\n \t}\n \n@@ -2337,8 +2336,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \tclear_contains_cache(&ref_cbdata.no_contains_cache);\n \n \t/* Filters that need revision walking */\n-\tdo_merge_filter(&ref_cbdata, DO_MERGE_FILTER_REACHABLE);\n-\tdo_merge_filter(&ref_cbdata, DO_MERGE_FILTER_UNREACHABLE);\n+\treach_filter(array, filter->reachable_from, INCLUDE_REACHED);\n+\treach_filter(array, filter->unreachable_from, EXCLUDE_REACHED);\n \n \treturn ret;\n }\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 2d13928455..feaef4a8fd 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -23,9 +23,6 @@\n #define FILTER_REFS_DETACHED_HEAD 0x0020\n #define FILTER_REFS_KIND_MASK (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)\n \n-#define DO_MERGE_FILTER_UNREACHABLE 0\n-#define DO_MERGE_FILTER_REACHABLE 1\n-\n struct atom_value;\n \n struct ref_sorting {\n-- \n2.24.3 (Apple Git-128)\n\n"},{"id":"405815","messageId":"20200918004909.32474-3-alipman88@gmail.com","threadId":"54211","inReplyTo":"20200918004909.32474-1-alipman88@gmail.com","subject":"[PATCH 2/2] Doc: prefer more specific file name","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-18T00:49:09Z","receivedAt":"2020-09-18T00:49:30Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"Change filters.txt to ref-reachability-filters.txt in order to avoid\nsquatting on a file name that might be useful for another purpose.\n\nSigned-off-by: Aaron Lipman \n---\n Documentation/git-branch.txt | 2 +-\n Documentation/git-for-each-ref.txt | 2 +-\n Documentation/git-tag.txt | 2 +-\n Documentation/{filters.txt => ref-reachability-filters.txt} | 0\n 4 files changed, 3 insertions(+), 3 deletions(-)\n rename Documentation/{filters.txt => ref-reachability-filters.txt} (100%)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 290b90639c..ace4ad3da8 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -368,7 +368,7 @@ serve four related but different purposes:\n - `--no-merged` is used to find branches which are candidates for merging\n into HEAD, since those branches are not fully contained by HEAD.\n \n-include::filters.txt[]\n+include::ref-reachability-filters.txt[]\n \n SEE ALSO\n --------\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 7b9cf0ef1f..2962f85a50 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -409,7 +409,7 @@ will be reported.\n NOTES\n -----\n \n-include::filters.txt[]\n+include::ref-reachability-filters.txt[]\n \n SEE ALSO\n --------\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex cc667d7d01..56656d1be6 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -380,7 +380,7 @@ include::date-formats.txt[]\n NOTES\n -----\n \n-include::filters.txt[]\n+include::ref-reachability-filters.txt[]\n \n SEE ALSO\n --------\ndiff --git a/Documentation/filters.txt b/Documentation/ref-reachability-filters.txt\nsimilarity index 100%\nrename from Documentation/filters.txt\nrename to Documentation/ref-reachability-filters.txt\n-- \n2.24.3 (Apple Git-128)\n\n"},{"id":"405821","messageId":"CAPig+cSptROTM4xDSj97g1UVEpVe3OCL4m9=R8f0rn-PRMhzPw@mail.gmail.com","threadId":"54211","inReplyTo":"20200918004909.32474-1-alipman88@gmail.com","subject":"Re: [PATCH 0/2] ref-filter: merged & no-merged touch-ups","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-18T02:52:23Z","receivedAt":"2020-09-18T02:52:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Sep 17, 2020 at 8:49 PM Aaron Lipman wrote:\n> Since this got moved to next, I'm not entirely clear on whether folks\n> want me to keep tweaking this, or if the last set of comments were\n> general advice for future contributions.\n>\n> But in the interest of diligence, I'd like to offer a couple touch-ups\n> applying Junio's suggestions.\n\nIn this case, the review comments were reasonably important, so\ntweaking it by patching what has already been moved to 'next', as you\ndo here, is a good choice. Thanks for the diligence. I'll leave one or\ntwo minor comments on the patches themselves (although, it's probably\nnot worth re-rolling just for them).\n"},{"id":"405837","messageId":"CAPig+cRXJ5hCEM0dFsSSnGWaYu76gPhpM3fV4FaV+Db4r6dX4g@mail.gmail.com","threadId":"54211","inReplyTo":"20200918004909.32474-2-alipman88@gmail.com","subject":"Re: [PATCH 1/2] ref-filter: refactor do_merge_filter()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-18T05:03:00Z","receivedAt":"2020-09-18T05:03:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Sep 17, 2020 at 8:49 PM Aaron Lipman wrote:\n> ref-filter: refactor do_merge_filter()\n>\n> Rename to reach_filter() to be more descriptive.\n>\n> Separate parameters to explicitly identify what data is used by the\n> function, instead of passing an entire ref_filter_cbdata struct.\n>\n> Rename and move associated preprocessor macros from header to source\n> file, as they're only used internally in a single location.\n\nOne thing that this commit message lacks is a high-level explanation\nof why these changes are being made. One possible rewrite would be:\n\n ref-filter: make internal reachable-filter API more precise\n\n The internal reachable-filter API is a bit loose and imprecise; it\n also bleeds unnecessarily into the public header. Tighten the API\n by:\n\n * renaming do_merge_filter() to reach_filter()\n\n * separating parameters to explicitly identify what data is used\n by the function instead of passing an entire ref_filter_cbdata\n struct\n\n * renaming and moving internal constants from header to source\n file\n\n> Signed-off-by: Aaron Lipman \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> @@ -2231,19 +2231,18 @@ void ref_array_clear(struct ref_array *array)\n> +static void reach_filter(struct ref_array *array,\n> + struct commit_list *check_reachable,\n> + int include_reached)\n> {\n> [...]\n> + if (!check_reachable)\n> return;\n\nI would probably drop this conditional return altogether since it\nincorrectly conveys to the reader that it is legal to call this\nfunction with NULL for 'check_reachable', whereas, in reality, it\nwould be pointless to do so. If this function were public, and the\npossible list of callers open-ended, then it might be reasonable for\nit to do this to alert callers early of their error:\n\n if (!check_reachable)\n BUG(\"check_reachable must not be NULL\");\n\nHowever, as this function is private, and the set of callers can be\nprecisely determined, it's not necessary to make the check at all. If\nanyone adds a new call in this file which incorrectly passes NULL,\nthey will discover the error quickly enough when the program crashes\nat the new call site.\n\nIf you were to drop this conditional, then that change would deserve\nanother bullet point in the commit message.\n"},{"id":"405838","messageId":"CAPig+cRso0DJm3KwYnBZZz7w2pJaWSd+gzfLsRfbrRqLNHAE-A@mail.gmail.com","threadId":"54211","inReplyTo":"CAPig+cRXJ5hCEM0dFsSSnGWaYu76gPhpM3fV4FaV+Db4r6dX4g@mail.gmail.com","subject":"Re: [PATCH 1/2] ref-filter: refactor do_merge_filter()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-09-18T05:04:33Z","receivedAt":"2020-09-18T05:04:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Sep 18, 2020 at 1:03 AM Eric Sunshine wrote:\n> > + if (!check_reachable)\n> > return;\n>\n> I would probably drop this conditional return altogether since it\n> incorrectly conveys to the reader that it is legal to call this\n> function with NULL for 'check_reachable', whereas, in reality, it\n> would be pointless to do so. [...]\n\nNevermind. I'm an idiot. I forgot that filter->reachable_from and\nfilter->unreachable_from may indeed validly be NULL.\n"},{"id":"405883","messageId":"xmqqtuvvm331.fsf@gitster.c.googlers.com","threadId":"54211","inReplyTo":"CAPig+cRXJ5hCEM0dFsSSnGWaYu76gPhpM3fV4FaV+Db4r6dX4g@mail.gmail.com","subject":"Re: [PATCH 1/2] ref-filter: refactor do_merge_filter()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-18T17:01:22Z","receivedAt":"2020-09-18T17:01:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> One thing that this commit message lacks is a high-level explanation\n> of why these changes are being made. One possible rewrite would be:\n>\n> ref-filter: make internal reachable-filter API more precise\n>\n> The internal reachable-filter API is a bit loose and imprecise; it\n> also bleeds unnecessarily into the public header. Tighten the API\n> by:\n>\n> * renaming do_merge_filter() to reach_filter()\n>\n> * separating parameters to explicitly identify what data is used\n> by the function instead of passing an entire ref_filter_cbdata\n> struct\n>\n> * renaming and moving internal constants from header to source\n> file\n\nSounds good. Aaron?\n\n>> +static void reach_filter(struct ref_array *array,\n>> + struct commit_list *check_reachable,\n>> + int include_reached)\n>> {\n>> [...]\n>> + if (!check_reachable)\n>> return;\n>\n> I would probably drop this conditional return altogether ...\n\nAs you said downthread, this part is fine as-is.\n"},{"id":"405916","messageId":"20200918215842.62232-1-alipman88@gmail.com","threadId":"54211","inReplyTo":"20200918004909.32474-1-alipman88@gmail.com","subject":"[PATCH v2 0/2] ref-filter: merged & no-merged touch-ups","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-18T21:58:40Z","receivedAt":"2020-09-18T22:00:24Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"> Sounds good. Aaron?\n\nSounds excellent! Rebased using Eric's wording.\n\nAaron Lipman (2):\n ref-filter: make internal reachable-filter API more precise\n Doc: prefer more specific file name\n\n Documentation/git-branch.txt | 2 +-\n Documentation/git-for-each-ref.txt | 2 +-\n Documentation/git-tag.txt | 2 +-\n ...lters.txt => ref-reachability-filters.txt} | 0\n ref-filter.c | 29 +++++++++----------\n ref-filter.h | 3 --\n 6 files changed, 17 insertions(+), 21 deletions(-)\n rename Documentation/{filters.txt => ref-reachability-filters.txt} (100%)\n\n-- \n2.24.3 (Apple Git-128)\n\n"},{"id":"405917","messageId":"20200918215842.62232-2-alipman88@gmail.com","threadId":"54211","inReplyTo":"20200918215842.62232-1-alipman88@gmail.com","subject":"[PATCH v2 1/2] ref-filter: make internal reachable-filter API more precise","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-18T21:58:41Z","receivedAt":"2020-09-18T22:00:27Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"The internal reachable-filter API is a bit loose and imprecise; it\nalso bleeds unnecessarily into the public header. Tighten the API\nby:\n\n* renaming do_merge_filter() to reach_filter()\n\n* separating parameters to explicitly identify what data is used\n by the function instead of passing an entire ref_filter_cbdata\n struct\n\n* renaming and moving internal constants from header to source\n file\n\nSigned-off-by: Aaron Lipman \n---\n ref-filter.c | 29 ++++++++++++++---------------\n ref-filter.h | 3 ---\n 2 files changed, 14 insertions(+), 18 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 785785a757..5550a0d34c 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2231,19 +2231,18 @@ void ref_array_clear(struct ref_array *array)\n \t}\n }\n \n-static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)\n+#define EXCLUDE_REACHED 0\n+#define INCLUDE_REACHED 1\n+static void reach_filter(struct ref_array *array,\n+\t\t\t struct commit_list *check_reachable,\n+\t\t\t int include_reached)\n {\n \tstruct rev_info revs;\n \tint i, old_nr;\n-\tstruct ref_array *array = ref_cbdata->array;\n \tstruct commit **to_clear = xcalloc(sizeof(struct commit *), array->nr);\n-\tstruct commit_list *rl;\n+\tstruct commit_list *cr;\n \n-\tstruct commit_list *check_reachable_list = reachable ?\n-\t\tref_cbdata->filter->reachable_from :\n-\t\tref_cbdata->filter->unreachable_from;\n-\n-\tif (!check_reachable_list)\n+\tif (!check_reachable)\n \t\treturn;\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n@@ -2254,8 +2253,8 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)\n \t\tto_clear[i] = item->commit;\n \t}\n \n-\tfor (rl = check_reachable_list; rl; rl = rl->next) {\n-\t\tstruct commit *merge_commit = rl->item;\n+\tfor (cr = check_reachable; cr; cr = cr->next) {\n+\t\tstruct commit *merge_commit = cr->item;\n \t\tmerge_commit->object.flags |= UNINTERESTING;\n \t\tadd_pending_object(&revs, &merge_commit->object, \"\");\n \t}\n@@ -2273,7 +2272,7 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)\n \n \t\tint is_merged = !!(commit->object.flags & UNINTERESTING);\n \n-\t\tif (is_merged == reachable)\n+\t\tif (is_merged == include_reached)\n \t\t\tarray->items[array->nr++] = array->items[i];\n \t\telse\n \t\t\tfree_array_item(item);\n@@ -2281,8 +2280,8 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)\n \n \tclear_commit_marks_many(old_nr, to_clear, ALL_REV_FLAGS);\n \n-\twhile (check_reachable_list) {\n-\t\tstruct commit *merge_commit = pop_commit(&check_reachable_list);\n+\twhile (check_reachable) {\n+\t\tstruct commit *merge_commit = pop_commit(&check_reachable);\n \t\tclear_commit_marks(merge_commit, ALL_REV_FLAGS);\n \t}\n \n@@ -2337,8 +2336,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \tclear_contains_cache(&ref_cbdata.no_contains_cache);\n \n \t/* Filters that need revision walking */\n-\tdo_merge_filter(&ref_cbdata, DO_MERGE_FILTER_REACHABLE);\n-\tdo_merge_filter(&ref_cbdata, DO_MERGE_FILTER_UNREACHABLE);\n+\treach_filter(array, filter->reachable_from, INCLUDE_REACHED);\n+\treach_filter(array, filter->unreachable_from, EXCLUDE_REACHED);\n \n \treturn ret;\n }\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 2d13928455..feaef4a8fd 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -23,9 +23,6 @@\n #define FILTER_REFS_DETACHED_HEAD 0x0020\n #define FILTER_REFS_KIND_MASK (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)\n \n-#define DO_MERGE_FILTER_UNREACHABLE 0\n-#define DO_MERGE_FILTER_REACHABLE 1\n-\n struct atom_value;\n \n struct ref_sorting {\n-- \n2.24.3 (Apple Git-128)\n\n"},{"id":"405918","messageId":"20200918215842.62232-3-alipman88@gmail.com","threadId":"54211","inReplyTo":"20200918215842.62232-1-alipman88@gmail.com","subject":"[PATCH v2 2/2] Doc: prefer more specific file name","fromName":"Aaron Lipman","fromEmail":"alipman88@gmail.com","sentAt":"2020-09-18T21:58:42Z","receivedAt":"2020-09-18T22:00:28Z","isPatch":true,"sender":{"key":"alipman88@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3922511?v=4"},"body":"Change filters.txt to ref-reachability-filters.txt in order to avoid\nsquatting on a file name that might be useful for another purpose.\n\nSigned-off-by: Aaron Lipman \n---\n Documentation/git-branch.txt | 2 +-\n Documentation/git-for-each-ref.txt | 2 +-\n Documentation/git-tag.txt | 2 +-\n Documentation/{filters.txt => ref-reachability-filters.txt} | 0\n 4 files changed, 3 insertions(+), 3 deletions(-)\n rename Documentation/{filters.txt => ref-reachability-filters.txt} (100%)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 290b90639c..ace4ad3da8 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -368,7 +368,7 @@ serve four related but different purposes:\n - `--no-merged` is used to find branches which are candidates for merging\n into HEAD, since those branches are not fully contained by HEAD.\n \n-include::filters.txt[]\n+include::ref-reachability-filters.txt[]\n \n SEE ALSO\n --------\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 7b9cf0ef1f..2962f85a50 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -409,7 +409,7 @@ will be reported.\n NOTES\n -----\n \n-include::filters.txt[]\n+include::ref-reachability-filters.txt[]\n \n SEE ALSO\n --------\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex cc667d7d01..56656d1be6 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -380,7 +380,7 @@ include::date-formats.txt[]\n NOTES\n -----\n \n-include::filters.txt[]\n+include::ref-reachability-filters.txt[]\n \n SEE ALSO\n --------\ndiff --git a/Documentation/filters.txt b/Documentation/ref-reachability-filters.txt\nsimilarity index 100%\nrename from Documentation/filters.txt\nrename to Documentation/ref-reachability-filters.txt\n-- \n2.24.3 (Apple Git-128)\n\n"}]}