{"thread":{"id":"58314","subject":"[PATCH 1/2] rev-list-options.txt: fix simple typo","startedAt":"2022-08-17T02:48:29Z","lastAt":"2022-08-20T00:13:32Z","messageCount":29,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano","Elijah Newren","Derrick Stolee","Ævar Arnfjörð Bjarmason","Eric Sunshine","Jonathan Tan","Derrick Stolee via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"461335","messageId":"68ab719d99c68c81159c65ee42930a3023f275db.1660704498.git.gitgitgadget@gmail.com","threadId":"58314","inReplyTo":"pull.1303.git.1660704498.gitgitgadget@gmail.com","subject":"[PATCH 1/2] rev-list-options.txt: fix simple typo","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-17T02:48:17Z","receivedAt":"2022-08-17T02:48:29Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nSigned-off-by: Elijah Newren \n---\n Documentation/rev-list-options.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 195e74eec63..2f85726745a 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -659,7 +659,7 @@ Here, the merge commits `O` and `P` contribute extra noise, as they did\n not actually contribute a change to `file.txt`. They only merged a topic\n that was based on an older version of `file.txt`. This is a common\n issue in repositories using a workflow where many contributors work in\n-parallel and merge their topic branches along a single trunk: manu\n+parallel and merge their topic branches along a single trunk: many\n unrelated merges appear in the `--full-history` results.\n \n When using the `--simplify-merges` option, the commits `O` and `P`\n-- \ngitgitgadget\n\n"},{"id":"461336","messageId":"pull.1303.git.1660704498.gitgitgadget@gmail.com","threadId":"58314","inReplyTo":null,"subject":"[PATCH 0/2] Allow --ancestry-path to take an argument","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-17T02:48:16Z","receivedAt":"2022-08-17T02:48:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This came out of a previous thread[1], where I wanted to be able to run\nsomething like\n\ngit log --oneline --ancestry-path=ab/submodule-cleanup main..seen\n\n\nand see the commits in main..seen which contained ab/submodule-cleanup in\ntheir ancestry path. Let me start by defining the terminology \"X is in a\ncommit's ancestry path\". By that, I just mean that either the commit is X,\nthe commit is an ancestor of X, or the commit is a descendant of X. With\nthat definition...\n\nThe command\n\ngit log --ancestry-path A..B\n\n\nmeans find the commits in A..B which contain A in their ancestry path. I\nsometimes still want to use A..B to get the basic range, but would like to\nuse a commit other than A for specifying which ancestry path is of interest.\nSo, for example, I might want to use\n\ngit log --ancestry-path=C A..B\n\n\nto mean find the commits in A..B which contain C in their ancestry path, or\nuse\n\ngit log --ancestry-path=C --ancestry-path=D A..B\n\n\nto mean find the commits in A..B which contain either C or D in their\nancestry path.\n\nThis series implements this request, by allowing --ancestry-path to take an\noptional argument. With it, I can find the answer to my question in the\nthread at [1] within the git.git repository (replacing branch names with\nactual hashes since the branches have since moved on):\n\n$ git log --oneline --ancestry-path=5b893f7d81 8168d5e9c2..ac0248bfba | wc -l\n36\n\n\nThis returns the answer I want, whereas dropping the '=5b893f7d81' from the\ncommand line gives me 192 unwanted commits (228 total), and various other\ncommand line flags (--first-parent, --boundary, etc.) also fail to give me\nthe set of commits I am looking for.\n\n[1]\nhttps://lore.kernel.org/git/CABPp-BF+8aqysioP_e27Q9kJ02rE2SuSqXu+XphzKWnk5a_Q+A@mail.gmail.com/\n\nElijah Newren (2):\n rev-list-options.txt: fix simple typo\n revision: allow --ancestry-path to take an argument\n\n Documentation/rev-list-options.txt | 47 +++++++++++++----\n object.h | 2 +-\n revision.c | 83 ++++++++++++++++++++----------\n revision.h | 3 ++\n t/t6019-rev-list-ancestry-path.sh | 47 ++++++++++++++++-\n 5 files changed, 140 insertions(+), 42 deletions(-)\n\n\nbase-commit: 6a475b71f8c4ce708d69fdc9317aefbde3769e25\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1303%2Fnewren%2Fancestry-path-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1303/newren/ancestry-path-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1303\n-- \ngitgitgadget\n"},{"id":"461337","messageId":"99287b67fd1c1e9fbceb4877738fb0aed722ec4a.1660704498.git.gitgitgadget@gmail.com","threadId":"58314","inReplyTo":"pull.1303.git.1660704498.gitgitgadget@gmail.com","subject":"[PATCH 2/2] revision: allow --ancestry-path to take an argument","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-17T02:48:18Z","receivedAt":"2022-08-17T02:48:33Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWe have long allowed users to run e.g.\n git log --ancestry-path next..seen\nwhich shows all commits which satisfy all three of these criteria:\n * are an ancestor of seen\n * are not an ancestor next\n * have next as an ancestor\n\nThis commit allows another variant:\n git log --ancestry-path=$TOPIC next..seen\nwhich shows all commits which satisfy all of these criteria:\n * are an ancestor of seen\n * are not an ancestor of next\n * have $TOPIC in their ancestry-path\nthat last bullet can be defined as commits meeting any of these\ncriteria:\n * are an ancestor of $TOPIC\n * have $TOPIC as an ancestor\n * are $TOPIC\n\nThis also allows multiple --ancestry-path arguments, which can be\nused to find commits with any of the given topics in their ancestry\npath.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/rev-list-options.txt | 45 ++++++++++++----\n object.h | 2 +-\n revision.c | 83 ++++++++++++++++++++----------\n revision.h | 3 ++\n t/t6019-rev-list-ancestry-path.sh | 47 ++++++++++++++++-\n 5 files changed, 139 insertions(+), 41 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 2f85726745a..001e49cee55 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -389,12 +389,15 @@ Default mode::\n \tmerges from the resulting history, as there are no selected\n \tcommits contributing to this merge.\n \n---ancestry-path::\n+--ancestry-path[=]::\n \tWhen given a range of commits to display (e.g. 'commit1..commit2'\n-\tor 'commit2 {caret}commit1'), only display commits that exist\n-\tdirectly on the ancestry chain between the 'commit1' and\n-\t'commit2', i.e. commits that are both descendants of 'commit1',\n-\tand ancestors of 'commit2'.\n+\tor 'commit2 {caret}commit1'), only display commits in that\n+\trange where is part of the ancestry chain. By \"part of\n+\tthe ancestry chain\", we mean including itself and\n+\tcommits that are either ancestors or descendants of .\n+\tIf no commit is specified, use 'commit1' (the excluded part of\n+\tthe range) as . Can be passed multiple times to look for\n+\tcommits in the ancestry range of multiple commits.\n \n A more detailed explanation follows.\n \n@@ -568,11 +571,10 @@ Note the major differences in `N`, `P`, and `Q` over `--full-history`:\n \n There is another simplification mode available:\n \n---ancestry-path::\n-\tLimit the displayed commits to those directly on the ancestry\n-\tchain between the ``from'' and ``to'' commits in the given commit\n-\trange. I.e. only display commits that are ancestor of the ``to''\n-\tcommit and descendants of the ``from'' commit.\n+--ancestry-path[=]::\n+\tLimit the displayed commits to those containing in their\n+\tancestry path. I.e. only display and commits which have\n+\t as either a direct ancestor or descendant.\n +\n As an example use case, consider the following commit history:\n +\n@@ -604,6 +606,29 @@ option does. Applied to the 'D..M' range, it results in:\n \t\t\t \\\n \t\t\t\tL--M\n -----------------------------------------------------------------------\n++\n+We can also use `--ancestry-path=D` instead of `--ancestry-path` which\n+means the same thing when applied to the 'D..M' range but is just more\n+explicit.\n++\n+If we instead are interested in a given topic within this range, and all\n+commits affected by that topic, we may only want to view the subset of\n+`D..M` which contain that topic in their ancestry path. So, using\n+`--ancestry-path=H D..M` for example would result in:\n++\n+-----------------------------------------------------------------------\n+\t\tE\n+\t\t \\\n+\t\t G---H---I---J\n+\t\t\t \\\n+\t\t\t\tL--M\n+-----------------------------------------------------------------------\n++\n+Whereas `--ancestry-path=K D..M` would result in\n++\n+-----------------------------------------------------------------------\n+\t\tK---------------L--M\n+-----------------------------------------------------------------------\n \n Before discussing another option, `--show-pulls`, we need to\n create a new example history.\ndiff --git a/object.h b/object.h\nindex a2219464c2b..9caef89f1f0 100644\n--- a/object.h\n+++ b/object.h\n@@ -59,7 +59,7 @@ struct object_array {\n \n /*\n * object flag allocation:\n- * revision.h: 0---------10 15 23------26\n+ * revision.h: 0---------10 15 23------27\n * fetch-pack.c: 01 67\n * negotiator/default.c: 2--5\n * walker.c: 0-2\ndiff --git a/revision.c b/revision.c\nindex 0c6e26cd9c8..660f1dd1b9b 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1105,7 +1105,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \t\t\t struct commit_list **list, struct prio_queue *queue)\n {\n \tstruct commit_list *parent = commit->parents;\n-\tunsigned left_flag;\n+\tunsigned left_flag, ancestry_flag;\n \n \tif (commit->object.flags & ADDED)\n \t\treturn 0;\n@@ -1161,6 +1161,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \t\treturn 0;\n \n \tleft_flag = (commit->object.flags & SYMMETRIC_LEFT);\n+\tancestry_flag = (commit->object.flags & ANCESTRY_PATH);\n \n \tfor (parent = commit->parents; parent; parent = parent->next) {\n \t\tstruct commit *p = parent->item;\n@@ -1181,6 +1182,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \t\t\tif (!*slot)\n \t\t\t\t*slot = *revision_sources_at(revs->sources, commit);\n \t\t}\n+\t\tif (revs->ancestry_path)\n+\t\t\tp->object.flags |= ancestry_flag;\n \t\tp->object.flags |= left_flag;\n \t\tif (!(p->object.flags & SEEN)) {\n \t\t\tp->object.flags |= (SEEN | NOT_USER_GIVEN);\n@@ -1304,13 +1307,20 @@ static int still_interesting(struct commit_list *src, timestamp_t date, int slop\n }\n \n /*\n- * \"rev-list --ancestry-path A..B\" computes commits that are ancestors\n+ * \"rev-list --ancestry-path=C A..B\" computes commits that are ancestors\n * of B but not ancestors of A but further limits the result to those\n- * that are descendants of A. This takes the list of bottom commits and\n- * the result of \"A..B\" without --ancestry-path, and limits the latter\n- * further to the ones that can reach one of the commits in \"bottom\".\n+ * that have C in their ancestry path (i.e. are either ancestors of C,\n+ * descendants of C, or are C). If multiple --ancestry-path=$COMMITTISH\n+ * arguments are supplied, we limit the result to those that have at\n+ * least one of those COMMITTISH in their ancestry path. If\n+ * --ancestry-path is specified with no commit, we use all bottom\n+ * commits for C.\n+ *\n+ * Before this function is called, ancestors of C will have already been\n+ * marked with ANCESTRY_PATH previously, so we just need to also mark\n+ * the descendants here, then collect both sets of commits.\n */\n-static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *list)\n+static void limit_to_ancestry(struct commit_list *bottoms, struct commit_list *list)\n {\n \tstruct commit_list *p;\n \tstruct commit_list *rlist = NULL;\n@@ -1323,7 +1333,7 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li\n \tfor (p = list; p; p = p->next)\n \t\tcommit_list_insert(p->item, &rlist);\n \n-\tfor (p = bottom; p; p = p->next)\n+\tfor (p = bottoms; p; p = p->next)\n \t\tp->item->object.flags |= TMP_MARK;\n \n \t/*\n@@ -1356,38 +1366,39 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li\n \t */\n \n \t/*\n-\t * The ones that are not marked with TMP_MARK are uninteresting\n+\t * The ones that are not marked with either TMP_MARK or\n+\t * ANCESTRY_PATH are uninteresting\n \t */\n \tfor (p = list; p; p = p->next) {\n \t\tstruct commit *c = p->item;\n-\t\tif (c->object.flags & TMP_MARK)\n+\t\tif (c->object.flags & (TMP_MARK | ANCESTRY_PATH))\n \t\t\tcontinue;\n \t\tc->object.flags |= UNINTERESTING;\n \t}\n \n-\t/* We are done with the TMP_MARK */\n+\t/* We are done with TMP_MARK and ANCESTRY_PATH */\n \tfor (p = list; p; p = p->next)\n-\t\tp->item->object.flags &= ~TMP_MARK;\n-\tfor (p = bottom; p; p = p->next)\n-\t\tp->item->object.flags &= ~TMP_MARK;\n+\t\tp->item->object.flags &= ~(TMP_MARK | ANCESTRY_PATH);\n+\tfor (p = bottoms; p; p = p->next)\n+\t\tp->item->object.flags &= ~(TMP_MARK | ANCESTRY_PATH);\n \tfree_commit_list(rlist);\n }\n \n /*\n- * Before walking the history, keep the set of \"negative\" refs the\n- * caller has asked to exclude.\n+ * Before walking the history, add the set of \"negative\" refs the\n+ * caller has asked to exclude to the bottom list.\n *\n * This is used to compute \"rev-list --ancestry-path A..B\", as we need\n * to filter the result of \"A..B\" further to the ones that can actually\n * reach A.\n */\n-static struct commit_list *collect_bottom_commits(struct commit_list *list)\n+static void collect_bottom_commits(struct commit_list *list,\n+\t\t\t\t struct commit_list **bottom)\n {\n-\tstruct commit_list *elem, *bottom = NULL;\n+\tstruct commit_list *elem;\n \tfor (elem = list; elem; elem = elem->next)\n \t\tif (elem->item->object.flags & BOTTOM)\n-\t\t\tcommit_list_insert(elem->item, &bottom);\n-\treturn bottom;\n+\t\t\tcommit_list_insert(elem->item, bottom);\n }\n \n /* Assumes either left_only or right_only is set */\n@@ -1414,12 +1425,12 @@ static int limit_list(struct rev_info *revs)\n \tstruct commit_list *original_list = revs->commits;\n \tstruct commit_list *newlist = NULL;\n \tstruct commit_list **p = &newlist;\n-\tstruct commit_list *bottom = NULL;\n \tstruct commit *interesting_cache = NULL;\n \n-\tif (revs->ancestry_path) {\n-\t\tbottom = collect_bottom_commits(original_list);\n-\t\tif (!bottom)\n+\tif (revs->ancestry_path_need_bottoms) {\n+\t\tcollect_bottom_commits(original_list,\n+\t\t\t\t &revs->ancestry_path_bottoms);\n+\t\tif (!revs->ancestry_path_bottoms)\n \t\t\tdie(\"--ancestry-path given but there are no bottom commits\");\n \t}\n \n@@ -1464,9 +1475,8 @@ static int limit_list(struct rev_info *revs)\n \tif (revs->left_only || revs->right_only)\n \t\tlimit_left_right(newlist, revs);\n \n-\tif (bottom)\n-\t\tlimit_to_ancestry(bottom, newlist);\n-\tfree_commit_list(bottom);\n+\tif (revs->ancestry_path)\n+\t\tlimit_to_ancestry(revs->ancestry_path_bottoms, newlist);\n \n \t/*\n \t * Check if any commits have become TREESAME by some of their parents\n@@ -2213,7 +2223,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\t\t const struct setup_revision_opt* opt)\n {\n \tconst char *arg = argv[0];\n-\tconst char *optarg;\n+\tconst char *optarg = NULL;\n \tint argcount;\n \tconst unsigned hexsz = the_hash_algo->hexsz;\n \n@@ -2280,10 +2290,26 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->first_parent_only = 1;\n \t} else if (!strcmp(arg, \"--exclude-first-parent-only\")) {\n \t\trevs->exclude_first_parent_only = 1;\n-\t} else if (!strcmp(arg, \"--ancestry-path\")) {\n+\t} else if (!strcmp(arg, \"--ancestry-path\") ||\n+\t\t skip_prefix(arg, \"--ancestry-path=\", &optarg)) {\n \t\trevs->ancestry_path = 1;\n \t\trevs->simplify_history = 0;\n \t\trevs->limited = 1;\n+\t\tif (optarg) {\n+\t\t\tstruct commit *c;\n+\t\t\tstruct object_id oid;\n+\t\t\tconst char *msg = _(\"could not get commit for ancestry-path argument %s\");\n+\n+\t\t\tif (repo_get_oid_committish(revs->repo, optarg, &oid))\n+\t\t\t\treturn error(msg, optarg);\n+\t\t\tget_reference(revs, optarg, &oid, ANCESTRY_PATH);\n+\t\t\tc = lookup_commit_reference(revs->repo, &oid);\n+\t\t\tif (!c)\n+\t\t\t\treturn error(msg, optarg);\n+\t\t\tcommit_list_insert(c, &revs->ancestry_path_bottoms);\n+\t\t} else {\n+\t\t\trevs->ancestry_path_need_bottoms = 1;\n+\t\t}\n \t} else if (!strcmp(arg, \"-g\") || !strcmp(arg, \"--walk-reflogs\")) {\n \t\tinit_reflog_walk(&revs->reflog_info);\n \t} else if (!strcmp(arg, \"--default\")) {\n@@ -2991,6 +3017,7 @@ static void release_revisions_topo_walk_info(struct topo_walk_info *info);\n void release_revisions(struct rev_info *revs)\n {\n \tfree_commit_list(revs->commits);\n+\tfree_commit_list(revs->ancestry_path_bottoms);\n \tobject_array_clear(&revs->pending);\n \tobject_array_clear(&revs->boundary_commits);\n \trelease_revisions_cmdline(&revs->cmdline);\ndiff --git a/revision.h b/revision.h\nindex e576845cdd1..d86241ad8fc 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -48,6 +48,7 @@\n */\n #define NOT_USER_GIVEN\t(1u<<25)\n #define TRACK_LINEAR\t(1u<<26)\n+#define ANCESTRY_PATH\t(1u<<27)\n #define ALL_REV_FLAGS\t(((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR | PULL_MERGE)\n \n #define DECORATE_SHORT_REFS\t1\n@@ -164,6 +165,7 @@ struct rev_info {\n \t\t\tcherry_mark:1,\n \t\t\tbisect:1,\n \t\t\tancestry_path:1,\n+\t\t\tancestry_path_need_bottoms:1,\n \t\t\tfirst_parent_only:1,\n \t\t\texclude_first_parent_only:1,\n \t\t\tline_level_traverse:1,\n@@ -306,6 +308,7 @@ struct rev_info {\n \tstruct saved_parents *saved_parents_slab;\n \n \tstruct commit_list *previous_parents;\n+\tstruct commit_list *ancestry_path_bottoms;\n \tconst char *break_bar;\n \n \tstruct revision_sources *sources;\ndiff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh\nindex af57a04b7ff..d3657737fa6 100755\n--- a/t/t6019-rev-list-ancestry-path.sh\n+++ b/t/t6019-rev-list-ancestry-path.sh\n@@ -8,8 +8,13 @@ test_description='--ancestry-path'\n # / \\\n # A-------K---------------L--M\n #\n-# D..M == E F G H I J K L M\n-# --ancestry-path D..M == E F H I J L M\n+# D..M == E F G H I J K L M\n+# --ancestry-path D..M == E F H I J L M\n+# --ancestry-path=F D..M == E F J L M\n+# --ancestry-path=G D..M == G H I J L M\n+# --ancestry-path=H D..M == E G H I J L M\n+# --ancestry-path=K D..M == K L M\n+# --ancestry-path=K --ancestry-path=F D..M == E F J K L M\n #\n # D..M -- M.t == M\n # --ancestry-path D..M -- M.t == M\n@@ -66,6 +71,44 @@ test_expect_success 'rev-list --ancestry-path D..M' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'rev-list --ancestry-path=F D..M' '\n+\ttest_write_lines E F J L M >expect &&\n+\tgit rev-list --ancestry-path=F --format=%s D..M |\n+\tsed -e \"/^commit /d\" |\n+\tsort >actual &&\n+\ttest_cmp expect actual\n+'\n+test_expect_success 'rev-list --ancestry-path=G D..M' '\n+\ttest_write_lines G H I J L M >expect &&\n+\tgit rev-list --ancestry-path=G --format=%s D..M |\n+\tsed -e \"/^commit /d\" |\n+\tsort >actual &&\n+\ttest_cmp expect actual\n+'\n+test_expect_success 'rev-list --ancestry-path=H D..M' '\n+\ttest_write_lines E G H I J L M >expect &&\n+\tgit rev-list --ancestry-path=H --format=%s D..M |\n+\tsed -e \"/^commit /d\" |\n+\tsort >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'rev-list --ancestry-path=K D..M' '\n+\ttest_write_lines K L M >expect &&\n+\tgit rev-list --ancestry-path=K --format=%s D..M |\n+\tsed -e \"/^commit /d\" |\n+\tsort >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'rev-list --ancestry-path=F --ancestry-path=K D..M' '\n+\ttest_write_lines E F J K L M >expect &&\n+\tgit rev-list --ancestry-path=F --ancestry-path=K --format=%s D..M |\n+\tsed -e \"/^commit /d\" |\n+\tsort >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'rev-list D..M -- M.t' '\n \techo M >expect &&\n \tgit rev-list --format=%s D..M -- M.t |\n-- \ngitgitgadget\n"},{"id":"461391","messageId":"xmqqpmgysda8.fsf@gitster.g","threadId":"58314","inReplyTo":"99287b67fd1c1e9fbceb4877738fb0aed722ec4a.1660704498.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] revision: allow --ancestry-path to take an argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-17T22:42:07Z","receivedAt":"2022-08-17T22:42:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> We have long allowed users to run e.g.\n> git log --ancestry-path next..seen\n> which shows all commits which satisfy all three of these criteria:\n> * are an ancestor of seen\n> * are not an ancestor next\n> * have next as an ancestor\n\nIs it a very good example, though? Nothing builds on next, and next\nis not an ancestor of seen, so the command without --ancestry-path\ndoes give us individual commits that are not in 'next' yet, plus all\nthe merge commits in master..seen but with --ancestry-path the\nanswer is most likely an empty set.\n\nIf you replace 'next' with 'master', it does start to make sense,\nbut that is a bit too straight first-parent merge chain that may not\nbe all that interesting.\n\n> This commit allows another variant:\n> git log --ancestry-path=$TOPIC next..seen\n> which shows all commits which satisfy all of these criteria:\n> * are an ancestor of seen\n> * are not an ancestor of next\n> * have $TOPIC in their ancestry-path\n> that last bullet can be defined as commits meeting any of these\n> criteria:\n> * are an ancestor of $TOPIC\n> * have $TOPIC as an ancestor\n> * are $TOPIC\n\nSo, I have en/ancestry-path-in-a-range topic merged somewhere\nbetween 'master' and 'seen'. Here is what I see:\n\n $ seen/git log --oneline --ancestry-path=en/ancestry-path-in-a-range master..seen\n 21aef6c754 Merge branch 'ab/submodule-helper-leakfix' into seen\n 2a57fcc25e Merge branch 'ab/submodule-helper-prep' into seen\n 72ff5f5d3a ###\n edb5cf4c31 Merge branch 'cw/remote-object-info' into seen\n cc8f65a665 Merge branch 'ag/merge-strategies-in-c' into seen\n c1bacacabf Merge branch 'es/mark-gc-cruft-as-experimental' into seen\n fdf2d207d2 Merge branch 'js/bisect-in-c' into seen\n 2a1bbfc016 Merge branch 'po/glossary-around-traversal' into seen\n 7ecf004b9e Merge branch 'vd/scalar-enables-fsmonitor' into jch\n 9dba189986 Merge branch 'en/ancestry-path-in-a-range' into jch\n 4461e34d7d revision: allow --ancestry-path to take an argument\n 0605b4aad9 rev-list-options.txt: fix simple typo\n\nwhich is very much expected. Two commits are what we want to\nhighlight, and its merge into the first-parent-chain that leads to\n'seen', and all its descendants on 'seen' are shown.\n\nDue to the way \"ancestry-path\" is defined, replacing the value to\n\"--ancestry-path\" from 4461e34d7d to 0605b4aad9 would not change the\noutput, which is also expected. If this were a three-commit topic,\ngiving the middle commit would find both the first one (i.e. the\nancestor) and the third one (i.e. the descendant) in the topic, while\nexcluding the much-less-interesting base commit and its ancestors\nthe topic builds on. \n\nI am not exactly sure when this feature is useful, though. It is\nhandy to be able to enumerate descendants of a given commit, so\nperhaps the user knows about 0605b4aad9 and is trying to find other\ncommits on the same topic, or something? But then the merges nearer\nthe tip of 'seen' than 9dba189986 are not very useful for that\npurpose. It somehow feels like a solution in search of a problem.\n\n> diff --git a/revision.c b/revision.c\n> index 0c6e26cd9c8..660f1dd1b9b 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1105,7 +1105,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n> \t\t\t struct commit_list **list, struct prio_queue *queue)\n> {\n> \tstruct commit_list *parent = commit->parents;\n> -\tunsigned left_flag;\n> +\tunsigned left_flag, ancestry_flag;\n> \n> \tif (commit->object.flags & ADDED)\n> \t\treturn 0;\n> @@ -1161,6 +1161,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n> \t\treturn 0;\n> \n> \tleft_flag = (commit->object.flags & SYMMETRIC_LEFT);\n> +\tancestry_flag = (commit->object.flags & ANCESTRY_PATH);\n\nWouldn't we want\n\n\tif (revs->ancestry_path)\n\t\tancestry_flag = (commit->object.flags & ANCESTRY_PATH);\n\ninstead, so that the propagation of contaminated flag bits ...\n\n> \tfor (parent = commit->parents; parent; parent = parent->next) {\n> \t\tstruct commit *p = parent->item;\n> @@ -1181,6 +1182,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n> \t\t\tif (!*slot)\n> \t\t\t\t*slot = *revision_sources_at(revs->sources, commit);\n> \t\t}\n> +\t\tif (revs->ancestry_path)\n> +\t\t\tp->object.flags |= ancestry_flag;\n> \t\tp->object.flags |= left_flag;\n\n... can become a simple\n\n\t\tp->object.flags |= ancestry_flag;\n\nhere? Or even just use a single variable to compute the set of\nflags to pass down i.e.\n\n\tpass_flags = commit->object.flags & (SYMMETRIC_LEFT | ANCESTRY_PATH);\n\nbefore the loop, and then pass these two bits down at once, i.e.\n\n- \t\tp->object.flags |= left_flag;\n+ \t\tp->object.flags |= pass_flags;\n\ntaking advantage of the fact that ANCESTRY_PATH and SYMMETRIC_LEFT\nbits can be set to any object only when these features are in use?\n\nOr did I misread the patch and sometimes ANCESTRY_PATH bit is set on\nobjects even when revs->ancestry_path is not in use?\n\n> +static void limit_to_ancestry(struct commit_list *bottoms, struct commit_list *list)\n> {\n> \tstruct commit_list *p;\n> \tstruct commit_list *rlist = NULL;\n> @@ -1323,7 +1333,7 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li\n> \tfor (p = list; p; p = p->next)\n> \t\tcommit_list_insert(p->item, &rlist);\n> \n> -\tfor (p = bottom; p; p = p->next)\n> +\tfor (p = bottoms; p; p = p->next)\n> \t\tp->item->object.flags |= TMP_MARK;\n> \n> \t/*\n> @@ -1356,38 +1366,39 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li\n> \t */\n> \n> \t/*\n> -\t * The ones that are not marked with TMP_MARK are uninteresting\n> +\t * The ones that are not marked with either TMP_MARK or\n> +\t * ANCESTRY_PATH are uninteresting\n> \t */\n> \tfor (p = list; p; p = p->next) {\n> \t\tstruct commit *c = p->item;\n> -\t\tif (c->object.flags & TMP_MARK)\n> +\t\tif (c->object.flags & (TMP_MARK | ANCESTRY_PATH))\n> \t\t\tcontinue;\n> \t\tc->object.flags |= UNINTERESTING;\n> \t}\n> \n> -\t/* We are done with the TMP_MARK */\n> +\t/* We are done with TMP_MARK and ANCESTRY_PATH */\n> \tfor (p = list; p; p = p->next)\n> -\t\tp->item->object.flags &= ~TMP_MARK;\n> -\tfor (p = bottom; p; p = p->next)\n> -\t\tp->item->object.flags &= ~TMP_MARK;\n> +\t\tp->item->object.flags &= ~(TMP_MARK | ANCESTRY_PATH);\n> +\tfor (p = bottoms; p; p = p->next)\n> +\t\tp->item->object.flags &= ~(TMP_MARK | ANCESTRY_PATH);\n> \tfree_commit_list(rlist);\n> }\n\nWe have called process_parents() to paint ancestor commits that can\nbe reached from the commit(s) of interest. This helper function is\ncalled after that is done, and propagates the ancestry_path bit in\nthe reverse direction, i.e. from parent to child.\n\nOnce we are done with this processing, we no longer need\nANCESTRY_PATH bit because the surviving ones without UNINTERESTING\nbit set are the commits on the ancestry_path. OK.\n\n"},{"id":"461394","messageId":"CABPp-BHmj+QCBFDrH77iNfEU41V=UDu7nhBYkAbCsbXhshJzzw@mail.gmail.com","threadId":"58314","inReplyTo":"xmqqpmgysda8.fsf@gitster.g","subject":"Re: [PATCH 2/2] revision: allow --ancestry-path to take an argument","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-18T04:01:59Z","receivedAt":"2022-08-18T04:02:46Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Aug 17, 2022 at 3:42 PM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > From: Elijah Newren \n> >\n> > We have long allowed users to run e.g.\n> > git log --ancestry-path next..seen\n> > which shows all commits which satisfy all three of these criteria:\n> > * are an ancestor of seen\n> > * are not an ancestor next\n> > * have next as an ancestor\n>\n> Is it a very good example, though? Nothing builds on next, and next\n> is not an ancestor of seen, so the command without --ancestry-path\n> does give us individual commits that are not in 'next' yet, plus all\n> the merge commits in master..seen but with --ancestry-path the\n> answer is most likely an empty set.\n>\n> If you replace 'next' with 'master', it does start to make sense,\n> but that is a bit too straight first-parent merge chain that may not\n> be all that interesting.\n\nYeah, I should have written master..seen.\n\n> > This commit allows another variant:\n> > git log --ancestry-path=$TOPIC next..seen\n> > which shows all commits which satisfy all of these criteria:\n> > * are an ancestor of seen\n> > * are not an ancestor of next\n> > * have $TOPIC in their ancestry-path\n> > that last bullet can be defined as commits meeting any of these\n> > criteria:\n> > * are an ancestor of $TOPIC\n> > * have $TOPIC as an ancestor\n> > * are $TOPIC\n>\n> So, I have en/ancestry-path-in-a-range topic merged somewhere\n> between 'master' and 'seen'. Here is what I see:\n>\n> $ seen/git log --oneline --ancestry-path=en/ancestry-path-in-a-range master..seen\n> 21aef6c754 Merge branch 'ab/submodule-helper-leakfix' into seen\n> 2a57fcc25e Merge branch 'ab/submodule-helper-prep' into seen\n> 72ff5f5d3a ###\n> edb5cf4c31 Merge branch 'cw/remote-object-info' into seen\n> cc8f65a665 Merge branch 'ag/merge-strategies-in-c' into seen\n> c1bacacabf Merge branch 'es/mark-gc-cruft-as-experimental' into seen\n> fdf2d207d2 Merge branch 'js/bisect-in-c' into seen\n> 2a1bbfc016 Merge branch 'po/glossary-around-traversal' into seen\n> 7ecf004b9e Merge branch 'vd/scalar-enables-fsmonitor' into jch\n> 9dba189986 Merge branch 'en/ancestry-path-in-a-range' into jch\n> 4461e34d7d revision: allow --ancestry-path to take an argument\n> 0605b4aad9 rev-list-options.txt: fix simple typo\n>\n> which is very much expected. Two commits are what we want to\n> highlight, and its merge into the first-parent-chain that leads to\n> 'seen', and all its descendants on 'seen' are shown.\n>\n> Due to the way \"ancestry-path\" is defined, replacing the value to\n> \"--ancestry-path\" from 4461e34d7d to 0605b4aad9 would not change the\n> output, which is also expected. If this were a three-commit topic,\n> giving the middle commit would find both the first one (i.e. the\n> ancestor) and the third one (i.e. the descendant) in the topic, while\n> excluding the much-less-interesting base commit and its ancestors\n> the topic builds on.\n\nYes.\n\n> I am not exactly sure when this feature is useful, though. It is\n> handy to be able to enumerate descendants of a given commit, so\n> perhaps the user knows about 0605b4aad9 and is trying to find other\n> commits on the same topic, or something? But then the merges nearer\n> the tip of 'seen' than 9dba189986 are not very useful for that\n> purpose. It somehow feels like a solution in search of a problem.\n\nHere's my usecase:\n git replay --i --keep-base --contained --ancestry-path=$TOPIC master..seen\n\nExplanation of usecase: I want to be able to replay commits, much like\nthe current interactive rebase feature. Unlike rebase, the selection\nof commits is done via a standard , for flexibility.\nNow, replaying of commits in $TOPIC implies I probably want all the\ntopics that depend on it to be updated, and all the merges that depend\non any of those to be updated. But, to achieve that, I don't want to\nhave to manually run N rebases and manually remerge things and whatnot\n-- I want it all done in one command. (And yes, replaying of merges\nneeds to handle squashed-in semantic fixes and replay those.) So, I\nwant this command to replay all commits in the ancestry path of $TOPIC\nin the range master..seen, keeping the current base (--keep-base), and\nI want it to also update any --contained branches (meaning any\nbranches pointing to commits in the range being replayed).\n\nAlternatives: For this usecase,\n\n * I cannot just use $TOPIC_TO_EDIT..seen, because that excludes the\n commits I want to tweak.\n\n * I could technically use `--ancestry-path master..seen`, but it's\n almost uselessly suboptimal as it would put hundreds of irrelevant\n commits in the TODO list (making it hard for the user to find what\n they want to edit) and then wastes time replaying all those commits\n unnecessarily as well.\n\n * As far as I can tell, there are no good alternatives here; and my\n question about it on the list didn't spur any satisfying answers[1].\n\nIf you'd rather I waited to submit the patch as part of a much larger\nseries implementing git-replay, I can do that. I just thought it was\nuseful to send upstream independently. But then I ran into the problem\nthat I thought it was weird to describe a usecase depending on something\nthat isn't yet very usable, and tried to explain it without that.\n\n[1] https://lore.kernel.org/git/CABPp-BEqWX3Nr2HDxwS9d-_QjcKb_jS=fSjsP_Pbutw7-P5gbg@mail.gmail.com/\n\n> > diff --git a/revision.c b/revision.c\n> > index 0c6e26cd9c8..660f1dd1b9b 100644\n> > --- a/revision.c\n> > +++ b/revision.c\n> > @@ -1105,7 +1105,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n> > struct commit_list **list, struct prio_queue *queue)\n> > {\n> > struct commit_list *parent = commit->parents;\n> > - unsigned left_flag;\n> > + unsigned left_flag, ancestry_flag;\n> >\n> > if (commit->object.flags & ADDED)\n> > return 0;\n> > @@ -1161,6 +1161,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n> > return 0;\n> >\n> > left_flag = (commit->object.flags & SYMMETRIC_LEFT);\n> > + ancestry_flag = (commit->object.flags & ANCESTRY_PATH);\n>\n> Wouldn't we want\n>\n> if (revs->ancestry_path)\n> ancestry_flag = (commit->object.flags & ANCESTRY_PATH);\n>\n> instead, so that the propagation of contaminated flag bits ...\n>\n> > for (parent = commit->parents; parent; parent = parent->next) {\n> > struct commit *p = parent->item;\n> > @@ -1181,6 +1182,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n> > if (!*slot)\n> > *slot = *revision_sources_at(revs->sources, commit);\n> > }\n> > + if (revs->ancestry_path)\n> > + p->object.flags |= ancestry_flag;\n> > p->object.flags |= left_flag;\n>\n> ... can become a simple\n>\n> p->object.flags |= ancestry_flag;\n>\n> here? Or even just use a single variable to compute the set of\n> flags to pass down i.e.\n>\n> pass_flags = commit->object.flags & (SYMMETRIC_LEFT | ANCESTRY_PATH);\n>\n> before the loop, and then pass these two bits down at once, i.e.\n>\n> - p->object.flags |= left_flag;\n> + p->object.flags |= pass_flags;\n>\n> taking advantage of the fact that ANCESTRY_PATH and SYMMETRIC_LEFT\n> bits can be set to any object only when these features are in use?\n\nOoh, that does seem better. I'll make that change.\n\n> Or did I misread the patch and sometimes ANCESTRY_PATH bit is set on\n> objects even when revs->ancestry_path is not in use?\n>\n> > +static void limit_to_ancestry(struct commit_list *bottoms, struct commit_list *list)\n> > {\n> > struct commit_list *p;\n> > struct commit_list *rlist = NULL;\n> > @@ -1323,7 +1333,7 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li\n> > for (p = list; p; p = p->next)\n> > commit_list_insert(p->item, &rlist);\n> >\n> > - for (p = bottom; p; p = p->next)\n> > + for (p = bottoms; p; p = p->next)\n> > p->item->object.flags |= TMP_MARK;\n> >\n> > /*\n> > @@ -1356,38 +1366,39 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li\n> > */\n> >\n> > /*\n> > - * The ones that are not marked with TMP_MARK are uninteresting\n> > + * The ones that are not marked with either TMP_MARK or\n> > + * ANCESTRY_PATH are uninteresting\n> > */\n> > for (p = list; p; p = p->next) {\n> > struct commit *c = p->item;\n> > - if (c->object.flags & TMP_MARK)\n> > + if (c->object.flags & (TMP_MARK | ANCESTRY_PATH))\n> > continue;\n> > c->object.flags |= UNINTERESTING;\n> > }\n> >\n> > - /* We are done with the TMP_MARK */\n> > + /* We are done with TMP_MARK and ANCESTRY_PATH */\n> > for (p = list; p; p = p->next)\n> > - p->item->object.flags &= ~TMP_MARK;\n> > - for (p = bottom; p; p = p->next)\n> > - p->item->object.flags &= ~TMP_MARK;\n> > + p->item->object.flags &= ~(TMP_MARK | ANCESTRY_PATH);\n> > + for (p = bottoms; p; p = p->next)\n> > + p->item->object.flags &= ~(TMP_MARK | ANCESTRY_PATH);\n> > free_commit_list(rlist);\n> > }\n>\n> We have called process_parents() to paint ancestor commits that can\n> be reached from the commit(s) of interest. This helper function is\n> called after that is done, and propagates the ancestry_path bit in\n> the reverse direction, i.e. from parent to child.\n>\n> Once we are done with this processing, we no longer need\n> ANCESTRY_PATH bit because the surviving ones without UNINTERESTING\n> bit set are the commits on the ancestry_path. OK.\n"},{"id":"461399","messageId":"pull.1303.v2.git.1660803467.gitgitgadget@gmail.com","threadId":"58314","inReplyTo":"pull.1303.git.1660704498.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] Allow --ancestry-path to take an argument","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-18T06:17:45Z","receivedAt":"2022-08-18T06:17:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Changes since v1:\n\n * Tweaked the commit message, and incorporated Junio's suggestion to update\n left_flag and ancestry_flag together.\n\nSeries description:\n\nThis came out of a previous thread[1], where I wanted to be able to run\nsomething like\n\ngit log --oneline --ancestry-path=ab/submodule-cleanup main..seen\n\n\nand see the commits in main..seen which contained ab/submodule-cleanup in\ntheir ancestry path. Let me start by defining the terminology \"X is in a\ncommit's ancestry path\". By that, I just mean that either the commit is X,\nthe commit is an ancestor of X, or the commit is a descendant of X. With\nthat definition...\n\nThe command\n\ngit log --ancestry-path A..B\n\n\nmeans find the commits in A..B which contain A in their ancestry path. I\nsometimes still want to use A..B to get the basic range, but would like to\nuse a commit other than A for specifying which ancestry path is of interest.\nSo, for example, I might want to use\n\ngit log --ancestry-path=C A..B\n\n\nto mean find the commits in A..B which contain C in their ancestry path, or\nuse\n\ngit log --ancestry-path=C --ancestry-path=D A..B\n\n\nto mean find the commits in A..B which contain either C or D in their\nancestry path.\n\nThis series implements this request, by allowing --ancestry-path to take an\noptional argument. With it, I can find the answer to my question in the\nthread at [1] within the git.git repository (replacing branch names with\nactual hashes since the branches have since moved on):\n\n$ git log --oneline --ancestry-path=5b893f7d81 8168d5e9c2..ac0248bfba | wc -l\n36\n\n\nThis returns the answer I want, whereas dropping the '=5b893f7d81' from the\ncommand line gives me 192 unwanted commits (228 total), and various other\ncommand line flags (--first-parent, --boundary, etc.) also fail to give me\nthe set of commits I am looking for.\n\n[1]\nhttps://lore.kernel.org/git/CABPp-BF+8aqysioP_e27Q9kJ02rE2SuSqXu+XphzKWnk5a_Q+A@mail.gmail.com/\n\nElijah Newren (2):\n rev-list-options.txt: fix simple typo\n revision: allow --ancestry-path to take an argument\n\n Documentation/rev-list-options.txt | 47 +++++++++++++----\n object.h | 2 +-\n revision.c | 84 +++++++++++++++++++-----------\n revision.h | 3 ++\n t/t6019-rev-list-ancestry-path.sh | 47 ++++++++++++++++-\n 5 files changed, 139 insertions(+), 44 deletions(-)\n\n\nbase-commit: 6a475b71f8c4ce708d69fdc9317aefbde3769e25\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1303%2Fnewren%2Fancestry-path-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1303/newren/ancestry-path-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1303\n\nRange-diff vs v1:\n\n 1: 68ab719d99c = 1: 68ab719d99c rev-list-options.txt: fix simple typo\n 2: 99287b67fd1 ! 2: f580ec6d060 revision: allow --ancestry-path to take an argument\n @@ Commit message\n revision: allow --ancestry-path to take an argument\n \n We have long allowed users to run e.g.\n - git log --ancestry-path next..seen\n + git log --ancestry-path master..seen\n which shows all commits which satisfy all three of these criteria:\n * are an ancestor of seen\n - * are not an ancestor next\n - * have next as an ancestor\n + * are not an ancestor master\n + * have master as an ancestor\n \n This commit allows another variant:\n - git log --ancestry-path=$TOPIC next..seen\n + git log --ancestry-path=$TOPIC master..seen\n which shows all commits which satisfy all of these criteria:\n * are an ancestor of seen\n - * are not an ancestor of next\n + * are not an ancestor of master\n * have $TOPIC in their ancestry-path\n that last bullet can be defined as commits meeting any of these\n criteria:\n @@ revision.c: static int process_parents(struct rev_info *revs, struct commit *com\n {\n \tstruct commit_list *parent = commit->parents;\n -\tunsigned left_flag;\n -+\tunsigned left_flag, ancestry_flag;\n ++\tunsigned pass_flags;\n \n \tif (commit->object.flags & ADDED)\n \t\treturn 0;\n @@ revision.c: static int process_parents(struct rev_info *revs, struct commit *commit,\n + \tif (revs->no_walk)\n \t\treturn 0;\n \n - \tleft_flag = (commit->object.flags & SYMMETRIC_LEFT);\n -+\tancestry_flag = (commit->object.flags & ANCESTRY_PATH);\n +-\tleft_flag = (commit->object.flags & SYMMETRIC_LEFT);\n ++\tpass_flags = (commit->object.flags & (SYMMETRIC_LEFT | ANCESTRY_PATH));\n \n \tfor (parent = commit->parents; parent; parent = parent->next) {\n \t\tstruct commit *p = parent->item;\n @@ revision.c: static int process_parents(struct rev_info *revs, struct commit *com\n \t\t\tif (!*slot)\n \t\t\t\t*slot = *revision_sources_at(revs->sources, commit);\n \t\t}\n -+\t\tif (revs->ancestry_path)\n -+\t\t\tp->object.flags |= ancestry_flag;\n - \t\tp->object.flags |= left_flag;\n +-\t\tp->object.flags |= left_flag;\n ++\t\tp->object.flags |= pass_flags;\n \t\tif (!(p->object.flags & SEEN)) {\n \t\t\tp->object.flags |= (SEEN | NOT_USER_GIVEN);\n + \t\t\tif (list)\n @@ revision.c: static int still_interesting(struct commit_list *src, timestamp_t date, int slop\n }\n \n\n-- \ngitgitgadget\n"},{"id":"461400","messageId":"68ab719d99c68c81159c65ee42930a3023f275db.1660803467.git.gitgitgadget@gmail.com","threadId":"58314","inReplyTo":"pull.1303.v2.git.1660803467.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] rev-list-options.txt: fix simple typo","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-18T06:17:46Z","receivedAt":"2022-08-18T06:17:57Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nSigned-off-by: Elijah Newren \n---\n Documentation/rev-list-options.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 195e74eec63..2f85726745a 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -659,7 +659,7 @@ Here, the merge commits `O` and `P` contribute extra noise, as they did\n not actually contribute a change to `file.txt`. They only merged a topic\n that was based on an older version of `file.txt`. This is a common\n issue in repositories using a workflow where many contributors work in\n-parallel and merge their topic branches along a single trunk: manu\n+parallel and merge their topic branches along a single trunk: many\n unrelated merges appear in the `--full-history` results.\n \n When using the `--simplify-merges` option, the commits `O` and `P`\n-- \ngitgitgadget\n\n"},{"id":"461401","messageId":"f580ec6d06072ea6ed2ecc4f8142b94fccbe4c0f.1660803467.git.gitgitgadget@gmail.com","threadId":"58314","inReplyTo":"pull.1303.v2.git.1660803467.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] revision: allow --ancestry-path to take an argument","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-18T06:17:47Z","receivedAt":"2022-08-18T06:17:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWe have long allowed users to run e.g.\n git log --ancestry-path master..seen\nwhich shows all commits which satisfy all three of these criteria:\n * are an ancestor of seen\n * are not an ancestor master\n * have master as an ancestor\n\nThis commit allows another variant:\n git log --ancestry-path=$TOPIC master..seen\nwhich shows all commits which satisfy all of these criteria:\n * are an ancestor of seen\n * are not an ancestor of master\n * have $TOPIC in their ancestry-path\nthat last bullet can be defined as commits meeting any of these\ncriteria:\n * are an ancestor of $TOPIC\n * have $TOPIC as an ancestor\n * are $TOPIC\n\nThis also allows multiple --ancestry-path arguments, which can be\nused to find commits with any of the given topics in their ancestry\npath.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/rev-list-options.txt | 45 ++++++++++++----\n object.h | 2 +-\n revision.c | 84 +++++++++++++++++++-----------\n revision.h | 3 ++\n t/t6019-rev-list-ancestry-path.sh | 47 ++++++++++++++++-\n 5 files changed, 138 insertions(+), 43 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 2f85726745a..001e49cee55 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -389,12 +389,15 @@ Default mode::\n \tmerges from the resulting history, as there are no selected\n \tcommits contributing to this merge.\n \n---ancestry-path::\n+--ancestry-path[=]::\n \tWhen given a range of commits to display (e.g. 'commit1..commit2'\n-\tor 'commit2 {caret}commit1'), only display commits that exist\n-\tdirectly on the ancestry chain between the 'commit1' and\n-\t'commit2', i.e. commits that are both descendants of 'commit1',\n-\tand ancestors of 'commit2'.\n+\tor 'commit2 {caret}commit1'), only display commits in that\n+\trange where is part of the ancestry chain. By \"part of\n+\tthe ancestry chain\", we mean including itself and\n+\tcommits that are either ancestors or descendants of .\n+\tIf no commit is specified, use 'commit1' (the excluded part of\n+\tthe range) as . Can be passed multiple times to look for\n+\tcommits in the ancestry range of multiple commits.\n \n A more detailed explanation follows.\n \n@@ -568,11 +571,10 @@ Note the major differences in `N`, `P`, and `Q` over `--full-history`:\n \n There is another simplification mode available:\n \n---ancestry-path::\n-\tLimit the displayed commits to those directly on the ancestry\n-\tchain between the ``from'' and ``to'' commits in the given commit\n-\trange. I.e. only display commits that are ancestor of the ``to''\n-\tcommit and descendants of the ``from'' commit.\n+--ancestry-path[=]::\n+\tLimit the displayed commits to those containing in their\n+\tancestry path. I.e. only display and commits which have\n+\t as either a direct ancestor or descendant.\n +\n As an example use case, consider the following commit history:\n +\n@@ -604,6 +606,29 @@ option does. Applied to the 'D..M' range, it results in:\n \t\t\t \\\n \t\t\t\tL--M\n -----------------------------------------------------------------------\n++\n+We can also use `--ancestry-path=D` instead of `--ancestry-path` which\n+means the same thing when applied to the 'D..M' range but is just more\n+explicit.\n++\n+If we instead are interested in a given topic within this range, and all\n+commits affected by that topic, we may only want to view the subset of\n+`D..M` which contain that topic in their ancestry path. So, using\n+`--ancestry-path=H D..M` for example would result in:\n++\n+-----------------------------------------------------------------------\n+\t\tE\n+\t\t \\\n+\t\t G---H---I---J\n+\t\t\t \\\n+\t\t\t\tL--M\n+-----------------------------------------------------------------------\n++\n+Whereas `--ancestry-path=K D..M` would result in\n++\n+-----------------------------------------------------------------------\n+\t\tK---------------L--M\n+-----------------------------------------------------------------------\n \n Before discussing another option, `--show-pulls`, we need to\n create a new example history.\ndiff --git a/object.h b/object.h\nindex a2219464c2b..9caef89f1f0 100644\n--- a/object.h\n+++ b/object.h\n@@ -59,7 +59,7 @@ struct object_array {\n \n /*\n * object flag allocation:\n- * revision.h: 0---------10 15 23------26\n+ * revision.h: 0---------10 15 23------27\n * fetch-pack.c: 01 67\n * negotiator/default.c: 2--5\n * walker.c: 0-2\ndiff --git a/revision.c b/revision.c\nindex 0c6e26cd9c8..4ebec71677c 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1105,7 +1105,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \t\t\t struct commit_list **list, struct prio_queue *queue)\n {\n \tstruct commit_list *parent = commit->parents;\n-\tunsigned left_flag;\n+\tunsigned pass_flags;\n \n \tif (commit->object.flags & ADDED)\n \t\treturn 0;\n@@ -1160,7 +1160,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \tif (revs->no_walk)\n \t\treturn 0;\n \n-\tleft_flag = (commit->object.flags & SYMMETRIC_LEFT);\n+\tpass_flags = (commit->object.flags & (SYMMETRIC_LEFT | ANCESTRY_PATH));\n \n \tfor (parent = commit->parents; parent; parent = parent->next) {\n \t\tstruct commit *p = parent->item;\n@@ -1181,7 +1181,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \t\t\tif (!*slot)\n \t\t\t\t*slot = *revision_sources_at(revs->sources, commit);\n \t\t}\n-\t\tp->object.flags |= left_flag;\n+\t\tp->object.flags |= pass_flags;\n \t\tif (!(p->object.flags & SEEN)) {\n \t\t\tp->object.flags |= (SEEN | NOT_USER_GIVEN);\n \t\t\tif (list)\n@@ -1304,13 +1304,20 @@ static int still_interesting(struct commit_list *src, timestamp_t date, int slop\n }\n \n /*\n- * \"rev-list --ancestry-path A..B\" computes commits that are ancestors\n+ * \"rev-list --ancestry-path=C A..B\" computes commits that are ancestors\n * of B but not ancestors of A but further limits the result to those\n- * that are descendants of A. This takes the list of bottom commits and\n- * the result of \"A..B\" without --ancestry-path, and limits the latter\n- * further to the ones that can reach one of the commits in \"bottom\".\n+ * that have C in their ancestry path (i.e. are either ancestors of C,\n+ * descendants of C, or are C). If multiple --ancestry-path=$COMMITTISH\n+ * arguments are supplied, we limit the result to those that have at\n+ * least one of those COMMITTISH in their ancestry path. If\n+ * --ancestry-path is specified with no commit, we use all bottom\n+ * commits for C.\n+ *\n+ * Before this function is called, ancestors of C will have already been\n+ * marked with ANCESTRY_PATH previously, so we just need to also mark\n+ * the descendants here, then collect both sets of commits.\n */\n-static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *list)\n+static void limit_to_ancestry(struct commit_list *bottoms, struct commit_list *list)\n {\n \tstruct commit_list *p;\n \tstruct commit_list *rlist = NULL;\n@@ -1323,7 +1330,7 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li\n \tfor (p = list; p; p = p->next)\n \t\tcommit_list_insert(p->item, &rlist);\n \n-\tfor (p = bottom; p; p = p->next)\n+\tfor (p = bottoms; p; p = p->next)\n \t\tp->item->object.flags |= TMP_MARK;\n \n \t/*\n@@ -1356,38 +1363,39 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li\n \t */\n \n \t/*\n-\t * The ones that are not marked with TMP_MARK are uninteresting\n+\t * The ones that are not marked with either TMP_MARK or\n+\t * ANCESTRY_PATH are uninteresting\n \t */\n \tfor (p = list; p; p = p->next) {\n \t\tstruct commit *c = p->item;\n-\t\tif (c->object.flags & TMP_MARK)\n+\t\tif (c->object.flags & (TMP_MARK | ANCESTRY_PATH))\n \t\t\tcontinue;\n \t\tc->object.flags |= UNINTERESTING;\n \t}\n \n-\t/* We are done with the TMP_MARK */\n+\t/* We are done with TMP_MARK and ANCESTRY_PATH */\n \tfor (p = list; p; p = p->next)\n-\t\tp->item->object.flags &= ~TMP_MARK;\n-\tfor (p = bottom; p; p = p->next)\n-\t\tp->item->object.flags &= ~TMP_MARK;\n+\t\tp->item->object.flags &= ~(TMP_MARK | ANCESTRY_PATH);\n+\tfor (p = bottoms; p; p = p->next)\n+\t\tp->item->object.flags &= ~(TMP_MARK | ANCESTRY_PATH);\n \tfree_commit_list(rlist);\n }\n \n /*\n- * Before walking the history, keep the set of \"negative\" refs the\n- * caller has asked to exclude.\n+ * Before walking the history, add the set of \"negative\" refs the\n+ * caller has asked to exclude to the bottom list.\n *\n * This is used to compute \"rev-list --ancestry-path A..B\", as we need\n * to filter the result of \"A..B\" further to the ones that can actually\n * reach A.\n */\n-static struct commit_list *collect_bottom_commits(struct commit_list *list)\n+static void collect_bottom_commits(struct commit_list *list,\n+\t\t\t\t struct commit_list **bottom)\n {\n-\tstruct commit_list *elem, *bottom = NULL;\n+\tstruct commit_list *elem;\n \tfor (elem = list; elem; elem = elem->next)\n \t\tif (elem->item->object.flags & BOTTOM)\n-\t\t\tcommit_list_insert(elem->item, &bottom);\n-\treturn bottom;\n+\t\t\tcommit_list_insert(elem->item, bottom);\n }\n \n /* Assumes either left_only or right_only is set */\n@@ -1414,12 +1422,12 @@ static int limit_list(struct rev_info *revs)\n \tstruct commit_list *original_list = revs->commits;\n \tstruct commit_list *newlist = NULL;\n \tstruct commit_list **p = &newlist;\n-\tstruct commit_list *bottom = NULL;\n \tstruct commit *interesting_cache = NULL;\n \n-\tif (revs->ancestry_path) {\n-\t\tbottom = collect_bottom_commits(original_list);\n-\t\tif (!bottom)\n+\tif (revs->ancestry_path_need_bottoms) {\n+\t\tcollect_bottom_commits(original_list,\n+\t\t\t\t &revs->ancestry_path_bottoms);\n+\t\tif (!revs->ancestry_path_bottoms)\n \t\t\tdie(\"--ancestry-path given but there are no bottom commits\");\n \t}\n \n@@ -1464,9 +1472,8 @@ static int limit_list(struct rev_info *revs)\n \tif (revs->left_only || revs->right_only)\n \t\tlimit_left_right(newlist, revs);\n \n-\tif (bottom)\n-\t\tlimit_to_ancestry(bottom, newlist);\n-\tfree_commit_list(bottom);\n+\tif (revs->ancestry_path)\n+\t\tlimit_to_ancestry(revs->ancestry_path_bottoms, newlist);\n \n \t/*\n \t * Check if any commits have become TREESAME by some of their parents\n@@ -2213,7 +2220,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\t\t const struct setup_revision_opt* opt)\n {\n \tconst char *arg = argv[0];\n-\tconst char *optarg;\n+\tconst char *optarg = NULL;\n \tint argcount;\n \tconst unsigned hexsz = the_hash_algo->hexsz;\n \n@@ -2280,10 +2287,26 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->first_parent_only = 1;\n \t} else if (!strcmp(arg, \"--exclude-first-parent-only\")) {\n \t\trevs->exclude_first_parent_only = 1;\n-\t} else if (!strcmp(arg, \"--ancestry-path\")) {\n+\t} else if (!strcmp(arg, \"--ancestry-path\") ||\n+\t\t skip_prefix(arg, \"--ancestry-path=\", &optarg)) {\n \t\trevs->ancestry_path = 1;\n \t\trevs->simplify_history = 0;\n \t\trevs->limited = 1;\n+\t\tif (optarg) {\n+\t\t\tstruct commit *c;\n+\t\t\tstruct object_id oid;\n+\t\t\tconst char *msg = _(\"could not get commit for ancestry-path argument %s\");\n+\n+\t\t\tif (repo_get_oid_committish(revs->repo, optarg, &oid))\n+\t\t\t\treturn error(msg, optarg);\n+\t\t\tget_reference(revs, optarg, &oid, ANCESTRY_PATH);\n+\t\t\tc = lookup_commit_reference(revs->repo, &oid);\n+\t\t\tif (!c)\n+\t\t\t\treturn error(msg, optarg);\n+\t\t\tcommit_list_insert(c, &revs->ancestry_path_bottoms);\n+\t\t} else {\n+\t\t\trevs->ancestry_path_need_bottoms = 1;\n+\t\t}\n \t} else if (!strcmp(arg, \"-g\") || !strcmp(arg, \"--walk-reflogs\")) {\n \t\tinit_reflog_walk(&revs->reflog_info);\n \t} else if (!strcmp(arg, \"--default\")) {\n@@ -2991,6 +3014,7 @@ static void release_revisions_topo_walk_info(struct topo_walk_info *info);\n void release_revisions(struct rev_info *revs)\n {\n \tfree_commit_list(revs->commits);\n+\tfree_commit_list(revs->ancestry_path_bottoms);\n \tobject_array_clear(&revs->pending);\n \tobject_array_clear(&revs->boundary_commits);\n \trelease_revisions_cmdline(&revs->cmdline);\ndiff --git a/revision.h b/revision.h\nindex e576845cdd1..d86241ad8fc 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -48,6 +48,7 @@\n */\n #define NOT_USER_GIVEN\t(1u<<25)\n #define TRACK_LINEAR\t(1u<<26)\n+#define ANCESTRY_PATH\t(1u<<27)\n #define ALL_REV_FLAGS\t(((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR | PULL_MERGE)\n \n #define DECORATE_SHORT_REFS\t1\n@@ -164,6 +165,7 @@ struct rev_info {\n \t\t\tcherry_mark:1,\n \t\t\tbisect:1,\n \t\t\tancestry_path:1,\n+\t\t\tancestry_path_need_bottoms:1,\n \t\t\tfirst_parent_only:1,\n \t\t\texclude_first_parent_only:1,\n \t\t\tline_level_traverse:1,\n@@ -306,6 +308,7 @@ struct rev_info {\n \tstruct saved_parents *saved_parents_slab;\n \n \tstruct commit_list *previous_parents;\n+\tstruct commit_list *ancestry_path_bottoms;\n \tconst char *break_bar;\n \n \tstruct revision_sources *sources;\ndiff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh\nindex af57a04b7ff..d3657737fa6 100755\n--- a/t/t6019-rev-list-ancestry-path.sh\n+++ b/t/t6019-rev-list-ancestry-path.sh\n@@ -8,8 +8,13 @@ test_description='--ancestry-path'\n # / \\\n # A-------K---------------L--M\n #\n-# D..M == E F G H I J K L M\n-# --ancestry-path D..M == E F H I J L M\n+# D..M == E F G H I J K L M\n+# --ancestry-path D..M == E F H I J L M\n+# --ancestry-path=F D..M == E F J L M\n+# --ancestry-path=G D..M == G H I J L M\n+# --ancestry-path=H D..M == E G H I J L M\n+# --ancestry-path=K D..M == K L M\n+# --ancestry-path=K --ancestry-path=F D..M == E F J K L M\n #\n # D..M -- M.t == M\n # --ancestry-path D..M -- M.t == M\n@@ -66,6 +71,44 @@ test_expect_success 'rev-list --ancestry-path D..M' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'rev-list --ancestry-path=F D..M' '\n+\ttest_write_lines E F J L M >expect &&\n+\tgit rev-list --ancestry-path=F --format=%s D..M |\n+\tsed -e \"/^commit /d\" |\n+\tsort >actual &&\n+\ttest_cmp expect actual\n+'\n+test_expect_success 'rev-list --ancestry-path=G D..M' '\n+\ttest_write_lines G H I J L M >expect &&\n+\tgit rev-list --ancestry-path=G --format=%s D..M |\n+\tsed -e \"/^commit /d\" |\n+\tsort >actual &&\n+\ttest_cmp expect actual\n+'\n+test_expect_success 'rev-list --ancestry-path=H D..M' '\n+\ttest_write_lines E G H I J L M >expect &&\n+\tgit rev-list --ancestry-path=H --format=%s D..M |\n+\tsed -e \"/^commit /d\" |\n+\tsort >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'rev-list --ancestry-path=K D..M' '\n+\ttest_write_lines K L M >expect &&\n+\tgit rev-list --ancestry-path=K --format=%s D..M |\n+\tsed -e \"/^commit /d\" |\n+\tsort >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'rev-list --ancestry-path=F --ancestry-path=K D..M' '\n+\ttest_write_lines E F J K L M >expect &&\n+\tgit rev-list --ancestry-path=F --ancestry-path=K --format=%s D..M |\n+\tsed -e \"/^commit /d\" |\n+\tsort >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'rev-list D..M -- M.t' '\n \techo M >expect &&\n \tgit rev-list --format=%s D..M -- M.t |\n-- \ngitgitgadget\n"},{"id":"461434","messageId":"ee4a7a7f-d646-6357-233a-1fefde5607bf@github.com","threadId":"58314","inReplyTo":"f580ec6d06072ea6ed2ecc4f8142b94fccbe4c0f.1660803467.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] revision: allow --ancestry-path to take an argument","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-18T15:30:12Z","receivedAt":"2022-08-18T15:30:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/18/2022 2:17 AM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n\nCode looks good!\n\n> diff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh\n> index af57a04b7ff..d3657737fa6 100755\n> --- a/t/t6019-rev-list-ancestry-path.sh\n> +++ b/t/t6019-rev-list-ancestry-path.sh\n> @@ -8,8 +8,13 @@ test_description='--ancestry-path'\n> # / \\\n> # A-------K---------------L--M\n> #\n> -# D..M == E F G H I J K L M\n> -# --ancestry-path D..M == E F H I J L M\n> +# D..M == E F G H I J K L M\n> +# --ancestry-path D..M == E F H I J L M\n> +# --ancestry-path=F D..M == E F J L M\n> +# --ancestry-path=G D..M == G H I J L M\n> +# --ancestry-path=H D..M == E G H I J L M\n> +# --ancestry-path=K D..M == K L M\n> +# --ancestry-path=K --ancestry-path=F D..M == E F J K L M\n\nThese are good examples, because they help clarify what I had initially\nbeen confused about: you include things in _both_ directions. In\nparticular, \"--ancestry-path=K --ancestry-path=f D..M\" you are kind of\ntaking the union of the following queries:\n\n\t--ancestry-path D..K\n\t--ancestry-path K..M\n\t--ancestry-path D..F\n\t--ancestry-path F..M\n\nI did check just in case, but specifying multiple ranges such as\n\n\t--ancestry-path D..K K..M\n\ndoes not do what is expected.\n\n> +test_expect_success 'rev-list --ancestry-path=F D..M' '\n> +\ttest_write_lines E F J L M >expect &&\n> +\tgit rev-list --ancestry-path=F --format=%s D..M |\n> +\tsed -e \"/^commit /d\" |\n> +\tsort >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nThese tests follow the patterns from other tests in this file, but\nit also has bad patterns. Specifically, the 'git rev-list' command\nis fed directly into a pipe. I include a patch below that applies\ndirectly on this one to rewrite these tests. If you want, you could\nrebase to have that test refactor happen before you add your new\n--ancestry-path= option tests.\n\n> +test_expect_success 'rev-list --ancestry-path=G D..M' '\n> +\ttest_write_lines G H I J L M >expect &&\n> +\tgit rev-list --ancestry-path=G --format=%s D..M |\n> +\tsed -e \"/^commit /d\" |\n> +\tsort >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +test_expect_success 'rev-list --ancestry-path=H D..M' '\n\nnit: needs extra whitespace between tests. The above test pair\nneeds one, too. This becomes moot with the patch I provide.\n\nThanks,\n-Stolee\n\n--- >8 ---\n\nFrom 9ac4e81dba0d7801513a09f5fe307d01357123dd Mon Sep 17 00:00:00 2001\nFrom: Derrick Stolee \nDate: Thu, 18 Aug 2022 11:25:04 -0400\nSubject: [PATCH] t6019: modernize tests with helper\n\nThe tests in t6019 are repetive, so create a helper that greatly\nsimplifies the test script.\n\nIn addition, update the common pattern that places 'git rev-list' on the\nleft side of a pipe, which can hide some exit codes. Send the output to\na 'raw' file that is then consumed by other tools so the Git exit code\nis verified as zero.\n\nSigned-off-by: Derrick Stolee \n---\n t/t6019-rev-list-ancestry-path.sh | 131 +++++++-----------------------\n 1 file changed, 31 insertions(+), 100 deletions(-)\n\ndiff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh\nindex d3657737fa60..18941a80ce67 100755\n--- a/t/t6019-rev-list-ancestry-path.sh\n+++ b/t/t6019-rev-list-ancestry-path.sh\n@@ -55,111 +55,42 @@ test_expect_success setup '\n \ttest_commit M\n '\n \n-test_expect_success 'rev-list D..M' '\n-\ttest_write_lines E F G H I J K L M >expect &&\n-\tgit rev-list --format=%s D..M |\n-\tsed -e \"/^commit /d\" |\n-\tsort >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'rev-list --ancestry-path D..M' '\n-\ttest_write_lines E F H I J L M >expect &&\n-\tgit rev-list --ancestry-path --format=%s D..M |\n-\tsed -e \"/^commit /d\" |\n-\tsort >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'rev-list --ancestry-path=F D..M' '\n-\ttest_write_lines E F J L M >expect &&\n-\tgit rev-list --ancestry-path=F --format=%s D..M |\n-\tsed -e \"/^commit /d\" |\n-\tsort >actual &&\n-\ttest_cmp expect actual\n-'\n-test_expect_success 'rev-list --ancestry-path=G D..M' '\n-\ttest_write_lines G H I J L M >expect &&\n-\tgit rev-list --ancestry-path=G --format=%s D..M |\n-\tsed -e \"/^commit /d\" |\n-\tsort >actual &&\n-\ttest_cmp expect actual\n-'\n-test_expect_success 'rev-list --ancestry-path=H D..M' '\n-\ttest_write_lines E G H I J L M >expect &&\n-\tgit rev-list --ancestry-path=H --format=%s D..M |\n-\tsed -e \"/^commit /d\" |\n-\tsort >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'rev-list --ancestry-path=K D..M' '\n-\ttest_write_lines K L M >expect &&\n-\tgit rev-list --ancestry-path=K --format=%s D..M |\n-\tsed -e \"/^commit /d\" |\n-\tsort >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'rev-list --ancestry-path=F --ancestry-path=K D..M' '\n-\ttest_write_lines E F J K L M >expect &&\n-\tgit rev-list --ancestry-path=F --ancestry-path=K --format=%s D..M |\n-\tsed -e \"/^commit /d\" |\n-\tsort >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'rev-list D..M -- M.t' '\n-\techo M >expect &&\n-\tgit rev-list --format=%s D..M -- M.t |\n-\tsed -e \"/^commit /d\" >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'rev-list --ancestry-path D..M -- M.t' '\n-\techo M >expect &&\n-\tgit rev-list --ancestry-path --format=%s D..M -- M.t |\n-\tsed -e \"/^commit /d\" >actual &&\n-\ttest_cmp expect actual\n-'\n+test_ancestry () {\n+\targs=$1\n+\texpected=$2\n+\ttest_expect_success \"rev-list $args\" \"\n+\t\ttest_write_lines $expected >expect &&\n+\t\tgit rev-list --format=%s $args >raw &&\n+\n+\t\tif test -n \\\"$expected\\\"\n+\t\tthen\n+\t\t\tsed -e \\\"/^commit /d\\\" raw | sort >actual &&\n+\t\t\ttest_cmp expect actual || return 1\n+\t\telse\n+\t\t\ttest_must_be_empty raw\n+\t\tfi\n+\t\"\n+}\n \n-test_expect_success 'rev-list F...I' '\n-\ttest_write_lines F G H I >expect &&\n-\tgit rev-list --format=%s F...I |\n-\tsed -e \"/^commit /d\" |\n-\tsort >actual &&\n-\ttest_cmp expect actual\n-'\n+test_ancestry \"D..M\" \"E F G H I J K L M\"\n \n-test_expect_success 'rev-list --ancestry-path F...I' '\n-\ttest_write_lines F H I >expect &&\n-\tgit rev-list --ancestry-path --format=%s F...I |\n-\tsed -e \"/^commit /d\" |\n-\tsort >actual &&\n-\ttest_cmp expect actual\n-'\n+test_ancestry \"--ancestry-path D..M\" \"E F H I J L M\"\n+test_ancestry \"--ancestry-path D..M\" \"E F H I J L M\"\n+test_ancestry \"--ancestry-path=F D..M\" \"E F J L M\"\n+test_ancestry \"--ancestry-path=G D..M\" \"G H I J L M\"\n+test_ancestry \"--ancestry-path=H D..M\" \"E G H I J L M\"\n+test_ancestry \"--ancestry-path=K D..M\" \"K L M\"\n+test_ancestry \"--ancestry-path=K --ancestry-path=F D..M\" \"E F J K L M\"\n \n-# G.t is dropped in an \"-s ours\" merge\n-test_expect_success 'rev-list G..M -- G.t' '\n-\tgit rev-list --format=%s G..M -- G.t |\n-\tsed -e \"/^commit /d\" >actual &&\n-\ttest_must_be_empty actual\n-'\n+test_ancestry \"D..M -- M.t\" \"M\"\n+test_ancestry \"--ancestry-path D..M -- M.t\" \"M\"\n \n-test_expect_success 'rev-list --ancestry-path G..M -- G.t' '\n-\techo L >expect &&\n-\tgit rev-list --ancestry-path --format=%s G..M -- G.t |\n-\tsed -e \"/^commit /d\" >actual &&\n-\ttest_cmp expect actual\n-'\n+test_ancestry \"F...I\" \"F G H I\"\n+test_ancestry \"--ancestry-path F...I\" \"F H I\"\n \n-test_expect_success 'rev-list --ancestry-path --simplify-merges G^..M -- G.t' '\n-\ttest_write_lines G L >expect &&\n-\tgit rev-list --ancestry-path --simplify-merges --format=%s G^..M -- G.t |\n-\tsed -e \"/^commit /d\" |\n-\tsort >actual &&\n-\ttest_cmp expect actual\n-'\n+test_ancestry \"G..M -- G.t\" \"\"\n+test_ancestry \"--ancestry-path G..M -- G.t\" \"L\"\n+test_ancestry \"--ancestry-path --simplify-merges G^..M -- G.t\" \"G L\"\n \n # b---bc\n # / \\ /\n-- \n2.37.1.vfs.0.0.rebase\n\n\n\n\n"},{"id":"461436","messageId":"220818.86ilmp8rzn.gmgdl@evledraar.gmail.com","threadId":"58314","inReplyTo":"ee4a7a7f-d646-6357-233a-1fefde5607bf@github.com","subject":"Re: [PATCH v2 2/2] revision: allow --ancestry-path to take an argument","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-18T15:50:50Z","receivedAt":"2022-08-18T15:57:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Aug 18 2022, Derrick Stolee wrote:\n\n> On 8/18/2022 2:17 AM, Elijah Newren via GitGitGadget wrote:\n>> +test_expect_success 'rev-list --ancestry-path=F D..M' '\n>> +\ttest_write_lines E F J L M >expect &&\n>> +\tgit rev-list --ancestry-path=F --format=%s D..M |\n>> +\tsed -e \"/^commit /d\" |\n>> +\tsort >actual &&\n>> +\ttest_cmp expect actual\n>> +'\n>\n> These tests follow the patterns from other tests in this file, but\n> it also has bad patterns. Specifically, the 'git rev-list' command\n> is fed directly into a pipe. I include a patch below that applies\n> directly on this one to rewrite these tests. If you want, you could\n> rebase to have that test refactor happen before you add your new\n> --ancestry-path= option tests.\n\nThanks, I was going to comment on the same, but your solution is much\nbetter (I was just going to suggest using intermediate files).\n\n> [...]\n> -test_expect_success 'rev-list --ancestry-path D..M -- M.t' '\n> -\techo M >expect &&\n> -\tgit rev-list --ancestry-path --format=%s D..M -- M.t |\n> -\tsed -e \"/^commit /d\" >actual &&\n> -\ttest_cmp expect actual\n> -'\n> +test_ancestry () {\n> +\targs=$1\n> +\texpected=$2\n\nMaybe add &&-chaining here (we do it in some cases, but I'm not sure\nwhen such assignments would ever fail).\n\n> +\ttest_expect_success \"rev-list $args\" \"\n> +\t\ttest_write_lines $expected >expect &&\n> +\t\tgit rev-list --format=%s $args >raw &&\n> +\n> +\t\tif test -n \\\"$expected\\\"\n\nAren't you making things harder for yourself here than required by using\n\"\"-quoting for the body of the test.\n\nWe eval it into existence, so you can use ''-quotes, and then you don't\nneed to escape e.g. the \"\" quotes here for expected, no?\n\n> +\t\tthen\n> +\t\t\tsed -e \\\"/^commit /d\\\" raw | sort >actual &&\n\nnit for debuggability (and not correctness), maybe using intermediate\nfiles here would be nicer? And then perhaps call them \"actual\" and\n\"actual.sorted\".\n\n\n> +\t\t\ttest_cmp expect actual || return 1\n\nNo need for a \"return 1\" here when we're not in a loop. It's redundant,\nand makes the -x output on failure confusing (\"why didn't I fail on the\ntest_cmp, but on this stray return?...\").\n\n...\n\n> +\t\telse\n> +\t\t\ttest_must_be_empty raw\n\n...which would also allow you to extract much of this if/else at the\ncost of not using test_must_be_empty, i.e. just make the \"expected\"\nempty unless \"$expected\" is provided. Just a thought/nit, we could also\nleave this as-is :)\n\nAlso does the \"compare rev\" part of this want test_cmp_rev instead?\n"},{"id":"461438","messageId":"xmqqy1vlqzqp.fsf@gitster.g","threadId":"58314","inReplyTo":"pull.1303.v2.git.1660803467.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] Allow --ancestry-path to take an argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-18T16:32:14Z","receivedAt":"2022-08-18T16:32:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> Changes since v1:\n>\n> * Tweaked the commit message, and incorporated Junio's suggestion to update\n> left_flag and ancestry_flag together.\n\nThanks, will replace and queue.\n"},{"id":"461440","messageId":"59b3af21-0685-1819-38bc-8afd826bee6f@github.com","threadId":"58314","inReplyTo":"220818.86ilmp8rzn.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/2] revision: allow --ancestry-path to take an argument","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-18T16:51:05Z","receivedAt":"2022-08-18T16:51:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/18/2022 11:50 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Aug 18 2022, Derrick Stolee wrote:\n> \n>> On 8/18/2022 2:17 AM, Elijah Newren via GitGitGadget wrote:\n>>> +test_expect_success 'rev-list --ancestry-path=F D..M' '\n>>> +\ttest_write_lines E F J L M >expect &&\n>>> +\tgit rev-list --ancestry-path=F --format=%s D..M |\n>>> +\tsed -e \"/^commit /d\" |\n>>> +\tsort >actual &&\n>>> +\ttest_cmp expect actual\n>>> +'\n>>\n>> These tests follow the patterns from other tests in this file, but\n>> it also has bad patterns. Specifically, the 'git rev-list' command\n>> is fed directly into a pipe. I include a patch below that applies\n>> directly on this one to rewrite these tests. If you want, you could\n>> rebase to have that test refactor happen before you add your new\n>> --ancestry-path= option tests.\n> \n> Thanks, I was going to comment on the same, but your solution is much\n> better (I was just going to suggest using intermediate files).\n> \n>> [...]\n>> -test_expect_success 'rev-list --ancestry-path D..M -- M.t' '\n>> -\techo M >expect &&\n>> -\tgit rev-list --ancestry-path --format=%s D..M -- M.t |\n>> -\tsed -e \"/^commit /d\" >actual &&\n>> -\ttest_cmp expect actual\n>> -'\n>> +test_ancestry () {\n>> +\targs=$1\n>> +\texpected=$2\n> \n> Maybe add &&-chaining here (we do it in some cases, but I'm not sure\n> when such assignments would ever fail).\n\nThese are outside of a test_expect_success, so they are not important.\n \n>> +\ttest_expect_success \"rev-list $args\" \"\n>> +\t\ttest_write_lines $expected >expect &&\n>> +\t\tgit rev-list --format=%s $args >raw &&\n>> +\n>> +\t\tif test -n \\\"$expected\\\"\n> \n> Aren't you making things harder for yourself here than required by using\n> \"\"-quoting for the body of the test.\n> \n> We eval it into existence, so you can use ''-quotes, and then you don't\n> need to escape e.g. the \"\" quotes here for expected, no?\n\nAre \"args\" and \"expected\" in-scope if I use single quotes? I don't think\nthey are unless we export them. I could be wrong.\n\n>> +\t\tthen\n>> +\t\t\tsed -e \\\"/^commit /d\\\" raw | sort >actual &&\n> \n> nit for debuggability (and not correctness), maybe using intermediate\n> files here would be nicer? And then perhaps call them \"actual\" and\n> \"actual.sorted\".\n\nI don't think that level of debuggability is important. We have the\nraw file if we want to see what the Git output was.\n\n> \n>> +\t\t\ttest_cmp expect actual || return 1\n> \n> No need for a \"return 1\" here when we're not in a loop. It's redundant,\n> and makes the -x output on failure confusing (\"why didn't I fail on the\n> test_cmp, but on this stray return?...\").\n\nSure. I did this more out of habit, but it makes sense that we don't\nneed it for an if block.\n\n> ...\n> \n>> +\t\telse\n>> +\t\t\ttest_must_be_empty raw\n> \n> ...which would also allow you to extract much of this if/else at the\n> cost of not using test_must_be_empty, i.e. just make the \"expected\"\n> empty unless \"$expected\" is provided. Just a thought/nit, we could also\n> leave this as-is :)\n\n*shrug* either way is fine by me.\n\n> Also does the \"compare rev\" part of this want test_cmp_rev instead?\n\nI don't know what you mean here. We are not comparing revisions anywhere,\nbut we are using the commit messages to create an easy comparison for the\noutput.\n\nThanks,\n-Stolee\n"},{"id":"461441","messageId":"CAPig+cS4gROePP=acuHwRZ62QfZkwfyD8Owo-C8NAkQ7tUcujA@mail.gmail.com","threadId":"58314","inReplyTo":"ee4a7a7f-d646-6357-233a-1fefde5607bf@github.com","subject":"Re: [PATCH v2 2/2] revision: allow --ancestry-path to take an argument","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-08-18T16:53:49Z","receivedAt":"2022-08-18T16:54:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 18, 2022 at 11:50 AM Derrick Stolee\n wrote:\n> From: Derrick Stolee \n> Subject: [PATCH] t6019: modernize tests with helper\n>\n> The tests in t6019 are repetive, so create a helper that greatly\n> simplifies the test script.\n\ns/repetive/repetitive/\n\n> In addition, update the common pattern that places 'git rev-list' on the\n> left side of a pipe, which can hide some exit codes. Send the output to\n> a 'raw' file that is then consumed by other tools so the Git exit code\n> is verified as zero.\n>\n> Signed-off-by: Derrick Stolee \n"},{"id":"461444","messageId":"CAPig+cSerOZbVUH=Twh_yKiZH7wJdUQ+Wk_sp3pL7bR4rR8f2g@mail.gmail.com","threadId":"58314","inReplyTo":"220818.86ilmp8rzn.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/2] revision: allow --ancestry-path to take an argument","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-08-18T16:56:25Z","receivedAt":"2022-08-18T16:56:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 18, 2022 at 12:01 PM Ævar Arnfjörð Bjarmason\n wrote:\n> On Thu, Aug 18 2022, Derrick Stolee wrote:\n> > +test_ancestry () {\n> > + args=$1\n> > + expected=$2\n>\n> Maybe add &&-chaining here (we do it in some cases, but I'm not sure\n> when such assignments would ever fail).\n\nAssignment shouldn't fail, but keeping the &&-chain intact here\nprotects us against the unlikely event of someone inserting &&-chained\ncode above these assignments and not realizing that the &&-chain is\nnot intact at the assignment lines.\n"},{"id":"461482","messageId":"20220818222416.3567602-1-jonathantanmy@google.com","threadId":"58314","inReplyTo":"f580ec6d06072ea6ed2ecc4f8142b94fccbe4c0f.1660803467.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] revision: allow --ancestry-path to take an argument","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-08-18T22:24:16Z","receivedAt":"2022-08-18T22:24:24Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks - overall this looks good. I only have some minor textual\ncomments and one small code comment.\n\n\"Elijah Newren via GitGitGadget\" writes:\n> From: Elijah Newren \n> \n> We have long allowed users to run e.g.\n> git log --ancestry-path master..seen\n> which shows all commits which satisfy all three of these criteria:\n> * are an ancestor of seen\n> * are not an ancestor master\n\nare not an ancestor *of* master\n\n> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> index 2f85726745a..001e49cee55 100644\n> --- a/Documentation/rev-list-options.txt\n> +++ b/Documentation/rev-list-options.txt\n> @@ -389,12 +389,15 @@ Default mode::\n> \tmerges from the resulting history, as there are no selected\n> \tcommits contributing to this merge.\n> \n> ---ancestry-path::\n> +--ancestry-path[=]::\n> \tWhen given a range of commits to display (e.g. 'commit1..commit2'\n> -\tor 'commit2 {caret}commit1'), only display commits that exist\n> -\tdirectly on the ancestry chain between the 'commit1' and\n> -\t'commit2', i.e. commits that are both descendants of 'commit1',\n> -\tand ancestors of 'commit2'.\n> +\tor 'commit2 {caret}commit1'), only display commits in that\n> +\trange where is part of the ancestry chain. By \"part of\n> +\tthe ancestry chain\", we mean including itself and\n> +\tcommits that are either ancestors or descendants of .\n> +\tIf no commit is specified, use 'commit1' (the excluded part of\n> +\tthe range) as . Can be passed multiple times to look for\n> +\tcommits in the ancestry range of multiple commits.\n\n\"Ancestry chain\" seems to be used multiple times in the Git codebase,\napparently with slightly different definitions, so probably best to\nclear up at least this part by not reusing the term. It's also probably\nnot worth introducing a new term \"ancestry range\". Maybe rewrite to\nsay:\n\n When given a range of commits to display (e.g. 'commit1..commit2'\n or 'commit2 {caret}commit1'), only display commits in that\n range that are ancestors of , descendants of , or\n itself. If no commit is specified, use 'commit1' (the\n excluded part of the range) as . Can be passed multiple times;\n if so, a commit is included if it is any of the commits given or if it\n is an ancestor or descendant of one of them.\n\n> @@ -568,11 +571,10 @@ Note the major differences in `N`, `P`, and `Q` over `--full-history`:\n> \n> There is another simplification mode available:\n> \n> ---ancestry-path::\n> -\tLimit the displayed commits to those directly on the ancestry\n> -\tchain between the ``from'' and ``to'' commits in the given commit\n> -\trange. I.e. only display commits that are ancestor of the ``to''\n> -\tcommit and descendants of the ``from'' commit.\n> +--ancestry-path[=]::\n> +\tLimit the displayed commits to those containing in their\n> +\tancestry path. I.e. only display and commits which have\n> +\t as either a direct ancestor or descendant.\n\nCan we refer back to the documentation of --ancestry-path instead?\n\n> @@ -1304,13 +1304,20 @@ static int still_interesting(struct commit_list *src, timestamp_t date, int slop\n> }\n> \n> /*\n> - * \"rev-list --ancestry-path A..B\" computes commits that are ancestors\n> + * \"rev-list --ancestry-path=C A..B\" computes commits that are ancestors\n> * of B but not ancestors of A but further limits the result to those\n> - * that are descendants of A. This takes the list of bottom commits and\n> - * the result of \"A..B\" without --ancestry-path, and limits the latter\n> - * further to the ones that can reach one of the commits in \"bottom\".\n> + * that have C in their ancestry path (i.e. are either ancestors of C,\n> + * descendants of C, or are C). If multiple --ancestry-path=$COMMITTISH\n> + * arguments are supplied, we limit the result to those that have at\n> + * least one of those COMMITTISH in their ancestry path. If\n> + * --ancestry-path is specified with no commit, we use all bottom\n> + * commits for C.\n> + *\n> + * Before this function is called, ancestors of C will have already been\n> + * marked with ANCESTRY_PATH previously, so we just need to also mark\n> + * the descendants here, then collect both sets of commits.\n> */\n> -static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *list)\n> +static void limit_to_ancestry(struct commit_list *bottoms, struct commit_list *list)\n\nI thought the original description of this function (\"This takes the\nlist...\") to be clear and it would be nice to retain it. So, e.g.:\n\n \"rev-list --ancestry-path=C_0 [--ancestry-path=C_1 ...] A..B\" computes commits\n that are ancestors of B but not ancestors of A but further limits the\n result to those that have any of C in their ancestry path (i.e. are\n either ancestors of any of C, descendants of any of C, or are any of\n C). If --ancestry-path is specified with no commit, we use all bottom\n commits for C.\n \n Before this function is called, ancestors of C will have already been\n marked with ANCESTRY_PATH previously.\n\n This takes the list of bottom commits and the result of \"A..B\" without\n --ancestry-path, and limits the latter further to the ones that have\n any of C in their ancestry path. Since the ancestors of C have already\n been marked (a prerequisite of this function), we just need to mark\n the descendants, then exclude any commit that does not have any of\n these marks.\n\nOptional: Besides that, from what I can tell, sometimes the C commits\nthemselves are marked with ANCESTRY_PATH (when they are explicitly\nspecified) and sometimes they are not (when they are not explicitly\nspecified). It's not a bug here, but it might be worth handling that in\nthe ancestry_path_need_bottoms codepath (instead of explicitly setting\nTMP_MARK on the bottoms in limit_to_ancestry() - that way, I think we\ncan also use ANCESTRY_PATH instead of TMP_MARK throughout the ancestry\npath codepaths, but I haven't tested it), at least to prevent possible\nfuture bugs.\n\n> @@ -2213,7 +2220,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n> \t\t\t const struct setup_revision_opt* opt)\n> {\n> \tconst char *arg = argv[0];\n> -\tconst char *optarg;\n> +\tconst char *optarg = NULL;\n> \tint argcount;\n> \tconst unsigned hexsz = the_hash_algo->hexsz;\n\n[snip]\n\n> @@ -2280,10 +2287,26 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n> \t\trevs->first_parent_only = 1;\n> \t} else if (!strcmp(arg, \"--exclude-first-parent-only\")) {\n> \t\trevs->exclude_first_parent_only = 1;\n> -\t} else if (!strcmp(arg, \"--ancestry-path\")) {\n> +\t} else if (!strcmp(arg, \"--ancestry-path\") ||\n> +\t\t skip_prefix(arg, \"--ancestry-path=\", &optarg)) {\n\nThis and the above hunk might cause bugs if --ancestry-path was first\nspecified with a commit and then specified without. Probably best to\nbreak this up into separate \"else if\" branches, even though there is a\nbit of code duplication (and also remove the \"= NULL\" addition in the\nabove hunk).\n\n> @@ -164,6 +165,7 @@ struct rev_info {\n> \t\t\tcherry_mark:1,\n> \t\t\tbisect:1,\n> \t\t\tancestry_path:1,\n> +\t\t\tancestry_path_need_bottoms:1,\n\nMight be better named as ancestry_path_implicit_bottoms? And probably\nworth documenting, e.g.\n\n True if --ancestry-path was specified without an argument. The bottom\n revisions are implicitly the arguments in this case.\n"},{"id":"461484","messageId":"CABPp-BGNG+E8ALs2rvRtzrw+m-kYYa+8yUuqphf=VJtstWr+zA@mail.gmail.com","threadId":"58314","inReplyTo":"ee4a7a7f-d646-6357-233a-1fefde5607bf@github.com","subject":"Re: [PATCH v2 2/2] revision: allow --ancestry-path to take an argument","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-19T01:01:00Z","receivedAt":"2022-08-19T01:02:43Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Aug 18, 2022 at 8:30 AM Derrick Stolee wrote:\n>\n> On 8/18/2022 2:17 AM, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n>\n> Code looks good!\n>\n> > diff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh\n> > index af57a04b7ff..d3657737fa6 100755\n> > --- a/t/t6019-rev-list-ancestry-path.sh\n> > +++ b/t/t6019-rev-list-ancestry-path.sh\n> > @@ -8,8 +8,13 @@ test_description='--ancestry-path'\n> > # / \\\n> > # A-------K---------------L--M\n> > #\n> > -# D..M == E F G H I J K L M\n> > -# --ancestry-path D..M == E F H I J L M\n> > +# D..M == E F G H I J K L M\n> > +# --ancestry-path D..M == E F H I J L M\n> > +# --ancestry-path=F D..M == E F J L M\n> > +# --ancestry-path=G D..M == G H I J L M\n> > +# --ancestry-path=H D..M == E G H I J L M\n> > +# --ancestry-path=K D..M == K L M\n> > +# --ancestry-path=K --ancestry-path=F D..M == E F J K L M\n>\n> These are good examples, because they help clarify what I had initially\n> been confused about: you include things in _both_ directions. In\n> particular, \"--ancestry-path=K --ancestry-path=f D..M\" you are kind of\n> taking the union of the following queries:\n>\n> --ancestry-path D..K\n> --ancestry-path K..M\n> --ancestry-path D..F\n> --ancestry-path F..M\n>\n> I did check just in case, but specifying multiple ranges such as\n>\n> --ancestry-path D..K K..M\n>\n> does not do what is expected.\n\nRight, because there's no such thing as multiple ranges. Quoting from the docs:\n\n\"\"\"\nCommands that are specifically designed to take two distinct ranges\n(e.g. \"git range-diff R1 R2\" to compare two ranges) do exist, but\nthey are exceptions. Unless otherwise noted, all \"git\" commands\nthat operate on a set of commits work on a single revision range.\nIn other words, writing two \"two-dot range notation\" next to each\nother, e.g.\n\n $ git log A..B C..D\n\ndoes *not* specify two revision ranges for most commands. Instead\nit will name a single connected set of commits, i.e. those that are\nreachable from either B or D but are reachable from neither A or C.\n\"\"\"\n\n>\n> > +test_expect_success 'rev-list --ancestry-path=F D..M' '\n> > + test_write_lines E F J L M >expect &&\n> > + git rev-list --ancestry-path=F --format=%s D..M |\n> > + sed -e \"/^commit /d\" |\n> > + sort >actual &&\n> > + test_cmp expect actual\n> > +'\n>\n> These tests follow the patterns from other tests in this file, but\n> it also has bad patterns. Specifically, the 'git rev-list' command\n> is fed directly into a pipe. I include a patch below that applies\n> directly on this one to rewrite these tests. If you want, you could\n> rebase to have that test refactor happen before you add your new\n> --ancestry-path= option tests.\n\nOoh, I like it. I'll rebase and include this patch earlier in my\nseries. Thanks!\n\n>\n> > +test_expect_success 'rev-list --ancestry-path=G D..M' '\n> > + test_write_lines G H I J L M >expect &&\n> > + git rev-list --ancestry-path=G --format=%s D..M |\n> > + sed -e \"/^commit /d\" |\n> > + sort >actual &&\n> > + test_cmp expect actual\n> > +'\n> > +test_expect_success 'rev-list --ancestry-path=H D..M' '\n>\n> nit: needs extra whitespace between tests. The above test pair\n> needs one, too. This becomes moot with the patch I provide.\n>\n> Thanks,\n> -Stolee\n>\n> --- >8 ---\n>\n> From 9ac4e81dba0d7801513a09f5fe307d01357123dd Mon Sep 17 00:00:00 2001\n> From: Derrick Stolee \n> Date: Thu, 18 Aug 2022 11:25:04 -0400\n> Subject: [PATCH] t6019: modernize tests with helper\n>\n> The tests in t6019 are repetive, so create a helper that greatly\n> simplifies the test script.\n>\n> In addition, update the common pattern that places 'git rev-list' on the\n> left side of a pipe, which can hide some exit codes. Send the output to\n> a 'raw' file that is then consumed by other tools so the Git exit code\n> is verified as zero.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> t/t6019-rev-list-ancestry-path.sh | 131 +++++++-----------------------\n> 1 file changed, 31 insertions(+), 100 deletions(-)\n>\n> diff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh\n> index d3657737fa60..18941a80ce67 100755\n> --- a/t/t6019-rev-list-ancestry-path.sh\n> +++ b/t/t6019-rev-list-ancestry-path.sh\n> @@ -55,111 +55,42 @@ test_expect_success setup '\n> test_commit M\n> '\n>\n> -test_expect_success 'rev-list D..M' '\n> - test_write_lines E F G H I J K L M >expect &&\n> - git rev-list --format=%s D..M |\n> - sed -e \"/^commit /d\" |\n> - sort >actual &&\n> - test_cmp expect actual\n> -'\n> -\n> -test_expect_success 'rev-list --ancestry-path D..M' '\n> - test_write_lines E F H I J L M >expect &&\n> - git rev-list --ancestry-path --format=%s D..M |\n> - sed -e \"/^commit /d\" |\n> - sort >actual &&\n> - test_cmp expect actual\n> -'\n> -\n> -test_expect_success 'rev-list --ancestry-path=F D..M' '\n> - test_write_lines E F J L M >expect &&\n> - git rev-list --ancestry-path=F --format=%s D..M |\n> - sed -e \"/^commit /d\" |\n> - sort >actual &&\n> - test_cmp expect actual\n> -'\n> -test_expect_success 'rev-list --ancestry-path=G D..M' '\n> - test_write_lines G H I J L M >expect &&\n> - git rev-list --ancestry-path=G --format=%s D..M |\n> - sed -e \"/^commit /d\" |\n> - sort >actual &&\n> - test_cmp expect actual\n> -'\n> -test_expect_success 'rev-list --ancestry-path=H D..M' '\n> - test_write_lines E G H I J L M >expect &&\n> - git rev-list --ancestry-path=H --format=%s D..M |\n> - sed -e \"/^commit /d\" |\n> - sort >actual &&\n> - test_cmp expect actual\n> -'\n> -\n> -test_expect_success 'rev-list --ancestry-path=K D..M' '\n> - test_write_lines K L M >expect &&\n> - git rev-list --ancestry-path=K --format=%s D..M |\n> - sed -e \"/^commit /d\" |\n> - sort >actual &&\n> - test_cmp expect actual\n> -'\n> -\n> -test_expect_success 'rev-list --ancestry-path=F --ancestry-path=K D..M' '\n> - test_write_lines E F J K L M >expect &&\n> - git rev-list --ancestry-path=F --ancestry-path=K --format=%s D..M |\n> - sed -e \"/^commit /d\" |\n> - sort >actual &&\n> - test_cmp expect actual\n> -'\n> -\n> -test_expect_success 'rev-list D..M -- M.t' '\n> - echo M >expect &&\n> - git rev-list --format=%s D..M -- M.t |\n> - sed -e \"/^commit /d\" >actual &&\n> - test_cmp expect actual\n> -'\n> -\n> -test_expect_success 'rev-list --ancestry-path D..M -- M.t' '\n> - echo M >expect &&\n> - git rev-list --ancestry-path --format=%s D..M -- M.t |\n> - sed -e \"/^commit /d\" >actual &&\n> - test_cmp expect actual\n> -'\n> +test_ancestry () {\n> + args=$1\n> + expected=$2\n> + test_expect_success \"rev-list $args\" \"\n> + test_write_lines $expected >expect &&\n> + git rev-list --format=%s $args >raw &&\n> +\n> + if test -n \\\"$expected\\\"\n> + then\n> + sed -e \\\"/^commit /d\\\" raw | sort >actual &&\n> + test_cmp expect actual || return 1\n> + else\n> + test_must_be_empty raw\n> + fi\n> + \"\n> +}\n>\n> -test_expect_success 'rev-list F...I' '\n> - test_write_lines F G H I >expect &&\n> - git rev-list --format=%s F...I |\n> - sed -e \"/^commit /d\" |\n> - sort >actual &&\n> - test_cmp expect actual\n> -'\n> +test_ancestry \"D..M\" \"E F G H I J K L M\"\n>\n> -test_expect_success 'rev-list --ancestry-path F...I' '\n> - test_write_lines F H I >expect &&\n> - git rev-list --ancestry-path --format=%s F...I |\n> - sed -e \"/^commit /d\" |\n> - sort >actual &&\n> - test_cmp expect actual\n> -'\n> +test_ancestry \"--ancestry-path D..M\" \"E F H I J L M\"\n> +test_ancestry \"--ancestry-path D..M\" \"E F H I J L M\"\n> +test_ancestry \"--ancestry-path=F D..M\" \"E F J L M\"\n> +test_ancestry \"--ancestry-path=G D..M\" \"G H I J L M\"\n> +test_ancestry \"--ancestry-path=H D..M\" \"E G H I J L M\"\n> +test_ancestry \"--ancestry-path=K D..M\" \"K L M\"\n> +test_ancestry \"--ancestry-path=K --ancestry-path=F D..M\" \"E F J K L M\"\n>\n> -# G.t is dropped in an \"-s ours\" merge\n> -test_expect_success 'rev-list G..M -- G.t' '\n> - git rev-list --format=%s G..M -- G.t |\n> - sed -e \"/^commit /d\" >actual &&\n> - test_must_be_empty actual\n> -'\n> +test_ancestry \"D..M -- M.t\" \"M\"\n> +test_ancestry \"--ancestry-path D..M -- M.t\" \"M\"\n>\n> -test_expect_success 'rev-list --ancestry-path G..M -- G.t' '\n> - echo L >expect &&\n> - git rev-list --ancestry-path --format=%s G..M -- G.t |\n> - sed -e \"/^commit /d\" >actual &&\n> - test_cmp expect actual\n> -'\n> +test_ancestry \"F...I\" \"F G H I\"\n> +test_ancestry \"--ancestry-path F...I\" \"F H I\"\n>\n> -test_expect_success 'rev-list --ancestry-path --simplify-merges G^..M -- G.t' '\n> - test_write_lines G L >expect &&\n> - git rev-list --ancestry-path --simplify-merges --format=%s G^..M -- G.t |\n> - sed -e \"/^commit /d\" |\n> - sort >actual &&\n> - test_cmp expect actual\n> -'\n> +test_ancestry \"G..M -- G.t\" \"\"\n> +test_ancestry \"--ancestry-path G..M -- G.t\" \"L\"\n> +test_ancestry \"--ancestry-path --simplify-merges G^..M -- G.t\" \"G L\"\n>\n> # b---bc\n> # / \\ /\n> --\n> 2.37.1.vfs.0.0.rebase\n>\n>\n>\n>\n"},{"id":"461485","messageId":"CABPp-BFOrKDEnq6R7k5+CxUKHObkfNWDGZghkqvkCnuXQC8rTg@mail.gmail.com","threadId":"58314","inReplyTo":"220818.86ilmp8rzn.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/2] revision: allow --ancestry-path to take an argument","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-19T01:12:00Z","receivedAt":"2022-08-19T01:12:36Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Aug 18, 2022 at 8:57 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Thu, Aug 18 2022, Derrick Stolee wrote:\n>\n> > On 8/18/2022 2:17 AM, Elijah Newren via GitGitGadget wrote:\n> >> +test_expect_success 'rev-list --ancestry-path=F D..M' '\n> >> + test_write_lines E F J L M >expect &&\n> >> + git rev-list --ancestry-path=F --format=%s D..M |\n> >> + sed -e \"/^commit /d\" |\n> >> + sort >actual &&\n> >> + test_cmp expect actual\n> >> +'\n> >\n> > These tests follow the patterns from other tests in this file, but\n> > it also has bad patterns. Specifically, the 'git rev-list' command\n> > is fed directly into a pipe. I include a patch below that applies\n> > directly on this one to rewrite these tests. If you want, you could\n> > rebase to have that test refactor happen before you add your new\n> > --ancestry-path= option tests.\n>\n> Thanks, I was going to comment on the same, but your solution is much\n> better (I was just going to suggest using intermediate files).\n>\n> > [...]\n> > -test_expect_success 'rev-list --ancestry-path D..M -- M.t' '\n> > - echo M >expect &&\n> > - git rev-list --ancestry-path --format=%s D..M -- M.t |\n> > - sed -e \"/^commit /d\" >actual &&\n> > - test_cmp expect actual\n> > -'\n> > +test_ancestry () {\n> > + args=$1\n> > + expected=$2\n>\n> Maybe add &&-chaining here (we do it in some cases, but I'm not sure\n> when such assignments would ever fail).\n>\n> > + test_expect_success \"rev-list $args\" \"\n> > + test_write_lines $expected >expect &&\n> > + git rev-list --format=%s $args >raw &&\n> > +\n> > + if test -n \\\"$expected\\\"\n>\n> Aren't you making things harder for yourself here than required by using\n> \"\"-quoting for the body of the test.\n>\n> We eval it into existence, so you can use ''-quotes, and then you don't\n> need to escape e.g. the \"\" quotes here for expected, no?\n>\n> > + then\n> > + sed -e \\\"/^commit /d\\\" raw | sort >actual &&\n>\n> nit for debuggability (and not correctness), maybe using intermediate\n> files here would be nicer? And then perhaps call them \"actual\" and\n> \"actual.sorted\".\n\nWould be better to just nuke the sed by replacing 'rev-list' with\n'log' (the line already has a --format option, so might as well get\nthe output we want).\n\n> > + test_cmp expect actual || return 1\n>\n> No need for a \"return 1\" here when we're not in a loop. It's redundant,\n> and makes the -x output on failure confusing (\"why didn't I fail on the\n> test_cmp, but on this stray return?...\").\n>\n> ...\n>\n> > + else\n> > + test_must_be_empty raw\n>\n> ...which would also allow you to extract much of this if/else at the\n> cost of not using test_must_be_empty, i.e. just make the \"expected\"\n> empty unless \"$expected\" is provided. Just a thought/nit, we could also\n> leave this as-is :)\n>\n> Also does the \"compare rev\" part of this want test_cmp_rev instead?\n\nUm, I don't see any \"compare rev\" part of this, or any revision\ncomparing. What are you referring to?\n"},{"id":"461486","messageId":"CABPp-BFjxFeGO+NU4HFCGqDe4aRFhqOdOxNYVDf7EJOWdT5RgA@mail.gmail.com","threadId":"58314","inReplyTo":"20220818222416.3567602-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] revision: allow --ancestry-path to take an argument","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-19T01:23:00Z","receivedAt":"2022-08-19T01:24:07Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Aug 18, 2022 at 3:24 PM Jonathan Tan wrote:\n>\n> Thanks - overall this looks good. I only have some minor textual\n> comments and one small code comment.\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n> > From: Elijah Newren \n> >\n> > We have long allowed users to run e.g.\n> > git log --ancestry-path master..seen\n> > which shows all commits which satisfy all three of these criteria:\n> > * are an ancestor of seen\n> > * are not an ancestor master\n>\n> are not an ancestor *of* master\n\nThanks, good catch.\n\n> > diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> > index 2f85726745a..001e49cee55 100644\n> > --- a/Documentation/rev-list-options.txt\n> > +++ b/Documentation/rev-list-options.txt\n> > @@ -389,12 +389,15 @@ Default mode::\n> > merges from the resulting history, as there are no selected\n> > commits contributing to this merge.\n> >\n> > ---ancestry-path::\n> > +--ancestry-path[=]::\n> > When given a range of commits to display (e.g. 'commit1..commit2'\n> > - or 'commit2 {caret}commit1'), only display commits that exist\n> > - directly on the ancestry chain between the 'commit1' and\n> > - 'commit2', i.e. commits that are both descendants of 'commit1',\n> > - and ancestors of 'commit2'.\n> > + or 'commit2 {caret}commit1'), only display commits in that\n> > + range where is part of the ancestry chain. By \"part of\n> > + the ancestry chain\", we mean including itself and\n> > + commits that are either ancestors or descendants of .\n> > + If no commit is specified, use 'commit1' (the excluded part of\n> > + the range) as . Can be passed multiple times to look for\n> > + commits in the ancestry range of multiple commits.\n>\n> \"Ancestry chain\" seems to be used multiple times in the Git codebase,\n> apparently with slightly different definitions, so probably best to\n> clear up at least this part by not reusing the term. It's also probably\n> not worth introducing a new term \"ancestry range\". Maybe rewrite to\n> say:\n>\n> When given a range of commits to display (e.g. 'commit1..commit2'\n> or 'commit2 {caret}commit1'), only display commits in that\n> range that are ancestors of , descendants of , or\n> itself. If no commit is specified, use 'commit1' (the\n> excluded part of the range) as . Can be passed multiple times;\n> if so, a commit is included if it is any of the commits given or if it\n> is an ancestor or descendant of one of them.\n\nWorks for me; thanks.\n\n> > @@ -568,11 +571,10 @@ Note the major differences in `N`, `P`, and `Q` over `--full-history`:\n> >\n> > There is another simplification mode available:\n> >\n> > ---ancestry-path::\n> > - Limit the displayed commits to those directly on the ancestry\n> > - chain between the ``from'' and ``to'' commits in the given commit\n> > - range. I.e. only display commits that are ancestor of the ``to''\n> > - commit and descendants of the ``from'' commit.\n> > +--ancestry-path[=]::\n> > + Limit the displayed commits to those containing in their\n> > + ancestry path. I.e. only display and commits which have\n> > + as either a direct ancestor or descendant.\n>\n> Can we refer back to the documentation of --ancestry-path instead?\n\nI had the same thought, especially since the nearby text also\nduplicates definitions for --dense, --sparse, --simplify-merges, and\n--show-pulls, each of which might also benefit from just referring\nback to previous definitions to avoid drift. I think we should handle\nthis whole \"History Simplification\" section consistently, so if we\njust refer back to the previous definition somehow for this flag then\nwe should also do the same for the others. But some of the others\nappear to intentionally avoid using the same wording in order to draw\ngraphs and pictorially explain it.\n\nSo it feels like a can of worms that I'm not sure how to solve, and\nwhat I currently have is the best solution available.\n\n> > @@ -1304,13 +1304,20 @@ static int still_interesting(struct commit_list *src, timestamp_t date, int slop\n> > }\n> >\n> > /*\n> > - * \"rev-list --ancestry-path A..B\" computes commits that are ancestors\n> > + * \"rev-list --ancestry-path=C A..B\" computes commits that are ancestors\n> > * of B but not ancestors of A but further limits the result to those\n> > - * that are descendants of A. This takes the list of bottom commits and\n> > - * the result of \"A..B\" without --ancestry-path, and limits the latter\n> > - * further to the ones that can reach one of the commits in \"bottom\".\n> > + * that have C in their ancestry path (i.e. are either ancestors of C,\n> > + * descendants of C, or are C). If multiple --ancestry-path=$COMMITTISH\n> > + * arguments are supplied, we limit the result to those that have at\n> > + * least one of those COMMITTISH in their ancestry path. If\n> > + * --ancestry-path is specified with no commit, we use all bottom\n> > + * commits for C.\n> > + *\n> > + * Before this function is called, ancestors of C will have already been\n> > + * marked with ANCESTRY_PATH previously, so we just need to also mark\n> > + * the descendants here, then collect both sets of commits.\n> > */\n> > -static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *list)\n> > +static void limit_to_ancestry(struct commit_list *bottoms, struct commit_list *list)\n>\n> I thought the original description of this function (\"This takes the\n> list...\") to be clear and it would be nice to retain it. So, e.g.:\n>\n> \"rev-list --ancestry-path=C_0 [--ancestry-path=C_1 ...] A..B\" computes commits\n> that are ancestors of B but not ancestors of A but further limits the\n> result to those that have any of C in their ancestry path (i.e. are\n> either ancestors of any of C, descendants of any of C, or are any of\n> C). If --ancestry-path is specified with no commit, we use all bottom\n> commits for C.\n>\n> Before this function is called, ancestors of C will have already been\n> marked with ANCESTRY_PATH previously.\n>\n> This takes the list of bottom commits and the result of \"A..B\" without\n> --ancestry-path, and limits the latter further to the ones that have\n> any of C in their ancestry path. Since the ancestors of C have already\n> been marked (a prerequisite of this function), we just need to mark\n> the descendants, then exclude any commit that does not have any of\n> these marks.\n\nSounds good to me; I'm happy to adopt this wording.\n\n> Optional: Besides that, from what I can tell, sometimes the C commits\n> themselves are marked with ANCESTRY_PATH (when they are explicitly\n> specified) and sometimes they are not (when they are not explicitly\n> specified). It's not a bug here, but it might be worth handling that in\n> the ancestry_path_need_bottoms codepath (instead of explicitly setting\n> TMP_MARK on the bottoms in limit_to_ancestry() - that way, I think we\n> can also use ANCESTRY_PATH instead of TMP_MARK throughout the ancestry\n> path codepaths, but I haven't tested it), at least to prevent possible\n> future bugs.\n\nThat sounds like you're trying to duplicate the bug in my first\nattempt at this patch. If you try to coalesce ANCESTRY_PATH and\nTMP_MARK, then you not only get all descendants of C, you also get all\ndescendants of any ancestor of C, which defeats the whole point of my\nchanges.\n\nIt's true that I don't mark implicit C commits with ANCESTRY_PATH, but\nthose are always bottom commits that are the excluded end of a range\nanyway. While those could be marked without causing problems, it\nwould always be a waste of effort.\n\n> > @@ -2213,7 +2220,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n> > const struct setup_revision_opt* opt)\n> > {\n> > const char *arg = argv[0];\n> > - const char *optarg;\n> > + const char *optarg = NULL;\n> > int argcount;\n> > const unsigned hexsz = the_hash_algo->hexsz;\n>\n> [snip]\n>\n> > @@ -2280,10 +2287,26 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n> > revs->first_parent_only = 1;\n> > } else if (!strcmp(arg, \"--exclude-first-parent-only\")) {\n> > revs->exclude_first_parent_only = 1;\n> > - } else if (!strcmp(arg, \"--ancestry-path\")) {\n> > + } else if (!strcmp(arg, \"--ancestry-path\") ||\n> > + skip_prefix(arg, \"--ancestry-path=\", &optarg)) {\n>\n> This and the above hunk might cause bugs if --ancestry-path was first\n> specified with a commit and then specified without. Probably best to\n> break this up into separate \"else if\" branches, even though there is a\n> bit of code duplication (and also remove the \"= NULL\" addition in the\n> above hunk).\n\nAh, good catch.\n\n> > @@ -164,6 +165,7 @@ struct rev_info {\n> > cherry_mark:1,\n> > bisect:1,\n> > ancestry_path:1,\n> > + ancestry_path_need_bottoms:1,\n>\n> Might be better named as ancestry_path_implicit_bottoms? And probably\n> worth documenting, e.g.\n>\n> True if --ancestry-path was specified without an argument. The bottom\n> revisions are implicitly the arguments in this case.\n\nSure, sounds good.\n\nThanks for the careful review!\n"},{"id":"461488","messageId":"220819.86a6817xyw.gmgdl@evledraar.gmail.com","threadId":"58314","inReplyTo":"CABPp-BFOrKDEnq6R7k5+CxUKHObkfNWDGZghkqvkCnuXQC8rTg@mail.gmail.com","subject":"Re: [PATCH v2 2/2] revision: allow --ancestry-path to take an argument","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-08-19T02:45:08Z","receivedAt":"2022-08-19T02:45:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Aug 18 2022, Elijah Newren wrote:\n\n> On Thu, Aug 18, 2022 at 8:57 AM Ævar Arnfjörð Bjarmason\n> wrote:\n>> [...]\n>> Also does the \"compare rev\" part of this want test_cmp_rev instead?\n>\n> Um, I don't see any \"compare rev\" part of this, or any revision\n> comparing. What are you referring to?\n\nI just misread this part while skimming it, sorry.\n"},{"id":"461501","messageId":"pull.1303.v3.git.1660883290.gitgitgadget@gmail.com","threadId":"58314","inReplyTo":"pull.1303.v2.git.1660803467.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] Allow --ancestry-path to take an argument","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-19T04:28:07Z","receivedAt":"2022-08-19T04:28:24Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Changes since v2:\n\n * Incorporated Stolee's suggested patch as a preliminary cleanup of t6019.\n Slightly modified his patch by:\n * fixing the \"repetitive\" typo pointed out by Eric\n * removed the unnecessary \"return 1\" pointed out by Ævar\n * switched \"rev-list\" to \"log\" since we are using --format anyway, in\n order to remove the need to call \"sed\" afterward\n * lots of wording improvements suggested by Jonathan\n * fixed an issue with argument parsing pointed out by Jonathan\n\nChanges since v1:\n\n * Tweaked the commit message, and incorporated Junio's suggestion to update\n left_flag and ancestry_flag together.\n\nSeries description:\n\nThis came out of a previous thread[1], where I wanted to be able to run\nsomething like\n\ngit log --oneline --ancestry-path=ab/submodule-cleanup main..seen\n\n\nand see the commits in main..seen which contained ab/submodule-cleanup in\ntheir ancestry path. Let me start by defining the terminology \"X is in a\ncommit's ancestry path\". By that, I just mean that either the commit is X,\nthe commit is an ancestor of X, or the commit is a descendant of X. With\nthat definition...\n\nThe command\n\ngit log --ancestry-path A..B\n\n\nmeans find the commits in A..B which contain A in their ancestry path. I\nsometimes still want to use A..B to get the basic range, but would like to\nuse a commit other than A for specifying which ancestry path is of interest.\nSo, for example, I might want to use\n\ngit log --ancestry-path=C A..B\n\n\nto mean find the commits in A..B which contain C in their ancestry path, or\nuse\n\ngit log --ancestry-path=C --ancestry-path=D A..B\n\n\nto mean find the commits in A..B which contain either C or D in their\nancestry path.\n\nThis series implements this request, by allowing --ancestry-path to take an\noptional argument. With it, I can find the answer to my question in the\nthread at [1] within the git.git repository (replacing branch names with\nactual hashes since the branches have since moved on):\n\n$ git log --oneline --ancestry-path=5b893f7d81 8168d5e9c2..ac0248bfba | wc -l\n36\n\n\nThis returns the answer I want, whereas dropping the '=5b893f7d81' from the\ncommand line gives me 192 unwanted commits (228 total), and various other\ncommand line flags (--first-parent, --boundary, etc.) also fail to give me\nthe set of commits I am looking for.\n\n[1]\nhttps://lore.kernel.org/git/CABPp-BF+8aqysioP_e27Q9kJ02rE2SuSqXu+XphzKWnk5a_Q+A@mail.gmail.com/\n\nDerrick Stolee (1):\n t6019: modernize tests with helper\n\nElijah Newren (2):\n rev-list-options.txt: fix simple typo\n revision: allow --ancestry-path to take an argument\n\n Documentation/rev-list-options.txt | 46 +++++++++----\n object.h | 2 +-\n revision.c | 89 ++++++++++++++++---------\n revision.h | 9 +++\n t/t6019-rev-list-ancestry-path.sh | 101 +++++++++++------------------\n 5 files changed, 141 insertions(+), 106 deletions(-)\n\n\nbase-commit: 6a475b71f8c4ce708d69fdc9317aefbde3769e25\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1303%2Fnewren%2Fancestry-path-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1303/newren/ancestry-path-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1303\n\nRange-diff vs v2:\n\n 1: 68ab719d99c = 1: 68ab719d99c rev-list-options.txt: fix simple typo\n -: ----------- > 2: 5226da2123e t6019: modernize tests with helper\n 2: f580ec6d060 ! 3: b810b3c8a2a revision: allow --ancestry-path to take an argument\n @@ Commit message\n git log --ancestry-path master..seen\n which shows all commits which satisfy all three of these criteria:\n * are an ancestor of seen\n - * are not an ancestor master\n + * are not an ancestor of master\n * have master as an ancestor\n \n This commit allows another variant:\n @@ Documentation/rev-list-options.txt: Default mode::\n -\tdirectly on the ancestry chain between the 'commit1' and\n -\t'commit2', i.e. commits that are both descendants of 'commit1',\n -\tand ancestors of 'commit2'.\n -+\tor 'commit2 {caret}commit1'), only display commits in that\n -+\trange where is part of the ancestry chain. By \"part of\n -+\tthe ancestry chain\", we mean including itself and\n -+\tcommits that are either ancestors or descendants of .\n -+\tIf no commit is specified, use 'commit1' (the excluded part of\n -+\tthe range) as . Can be passed multiple times to look for\n -+\tcommits in the ancestry range of multiple commits.\n ++\tor 'commit2 {caret}commit1'), only display commits in that range\n ++\tthat are ancestors of , descendants of , or\n ++\t itself. If no commit is specified, use 'commit1' (the\n ++\texcluded part of the range) as . Can be passed multiple\n ++\ttimes; if so, a commit is included if it is any of the commits\n ++\tgiven or if it is an ancestor or descendant of one of them.\n \n A more detailed explanation follows.\n \n @@ Documentation/rev-list-options.txt: Note the major differences in `N`, `P`, and\n -\trange. I.e. only display commits that are ancestor of the ``to''\n -\tcommit and descendants of the ``from'' commit.\n +--ancestry-path[=]::\n -+\tLimit the displayed commits to those containing in their\n -+\tancestry path. I.e. only display and commits which have\n -+\t as either a direct ancestor or descendant.\n ++\tLimit the displayed commits to those which are an ancestor of\n ++\t, or which are a descendant of , or are \n ++\titself.\n +\n As an example use case, consider the following commit history:\n +\n @@ revision.c: static int still_interesting(struct commit_list *src, timestamp_t da\n \n /*\n - * \"rev-list --ancestry-path A..B\" computes commits that are ancestors\n -+ * \"rev-list --ancestry-path=C A..B\" computes commits that are ancestors\n - * of B but not ancestors of A but further limits the result to those\n +- * of B but not ancestors of A but further limits the result to those\n - * that are descendants of A. This takes the list of bottom commits and\n - * the result of \"A..B\" without --ancestry-path, and limits the latter\n - * further to the ones that can reach one of the commits in \"bottom\".\n -+ * that have C in their ancestry path (i.e. are either ancestors of C,\n -+ * descendants of C, or are C). If multiple --ancestry-path=$COMMITTISH\n -+ * arguments are supplied, we limit the result to those that have at\n -+ * least one of those COMMITTISH in their ancestry path. If\n -+ * --ancestry-path is specified with no commit, we use all bottom\n -+ * commits for C.\n ++ * \"rev-list --ancestry-path=C_0 [--ancestry-path=C_1 ...] A..B\"\n ++ * computes commits that are ancestors of B but not ancestors of A but\n ++ * further limits the result to those that have any of C in their\n ++ * ancestry path (i.e. are either ancestors of any of C, descendants\n ++ * of any of C, or are any of C). If --ancestry-path is specified with\n ++ * no commit, we use all bottom commits for C.\n ++ *\n ++ * Before this function is called, ancestors of C will have already\n ++ * been marked with ANCESTRY_PATH previously.\n + *\n -+ * Before this function is called, ancestors of C will have already been\n -+ * marked with ANCESTRY_PATH previously, so we just need to also mark\n -+ * the descendants here, then collect both sets of commits.\n ++ * This takes the list of bottom commits and the result of \"A..B\"\n ++ * without --ancestry-path, and limits the latter further to the ones\n ++ * that have any of C in their ancestry path. Since the ancestors of C\n ++ * have already been marked (a prerequisite of this function), we just\n ++ * need to mark the descendants, then exclude any commit that does not\n ++ * have any of these marks.\n */\n -static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *list)\n +static void limit_to_ancestry(struct commit_list *bottoms, struct commit_list *list)\n @@ revision.c: static int limit_list(struct rev_info *revs)\n -\tif (revs->ancestry_path) {\n -\t\tbottom = collect_bottom_commits(original_list);\n -\t\tif (!bottom)\n -+\tif (revs->ancestry_path_need_bottoms) {\n ++\tif (revs->ancestry_path_implicit_bottoms) {\n +\t\tcollect_bottom_commits(original_list,\n +\t\t\t\t &revs->ancestry_path_bottoms);\n +\t\tif (!revs->ancestry_path_bottoms)\n @@ revision.c: static int handle_revision_opt(struct rev_info *revs, int argc, cons\n \tconst unsigned hexsz = the_hash_algo->hexsz;\n \n @@ revision.c: static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n - \t\trevs->first_parent_only = 1;\n - \t} else if (!strcmp(arg, \"--exclude-first-parent-only\")) {\n - \t\trevs->exclude_first_parent_only = 1;\n --\t} else if (!strcmp(arg, \"--ancestry-path\")) {\n -+\t} else if (!strcmp(arg, \"--ancestry-path\") ||\n -+\t\t skip_prefix(arg, \"--ancestry-path=\", &optarg)) {\n \t\trevs->ancestry_path = 1;\n \t\trevs->simplify_history = 0;\n \t\trevs->limited = 1;\n -+\t\tif (optarg) {\n -+\t\t\tstruct commit *c;\n -+\t\t\tstruct object_id oid;\n -+\t\t\tconst char *msg = _(\"could not get commit for ancestry-path argument %s\");\n ++\t\trevs->ancestry_path_implicit_bottoms = 1;\n ++\t} else if (skip_prefix(arg, \"--ancestry-path=\", &optarg)) {\n ++\t\tstruct commit *c;\n ++\t\tstruct object_id oid;\n ++\t\tconst char *msg = _(\"could not get commit for ancestry-path argument %s\");\n ++\n ++\t\trevs->ancestry_path = 1;\n ++\t\trevs->simplify_history = 0;\n ++\t\trevs->limited = 1;\n +\n -+\t\t\tif (repo_get_oid_committish(revs->repo, optarg, &oid))\n -+\t\t\t\treturn error(msg, optarg);\n -+\t\t\tget_reference(revs, optarg, &oid, ANCESTRY_PATH);\n -+\t\t\tc = lookup_commit_reference(revs->repo, &oid);\n -+\t\t\tif (!c)\n -+\t\t\t\treturn error(msg, optarg);\n -+\t\t\tcommit_list_insert(c, &revs->ancestry_path_bottoms);\n -+\t\t} else {\n -+\t\t\trevs->ancestry_path_need_bottoms = 1;\n -+\t\t}\n ++\t\tif (repo_get_oid_committish(revs->repo, optarg, &oid))\n ++\t\t\treturn error(msg, optarg);\n ++\t\tget_reference(revs, optarg, &oid, ANCESTRY_PATH);\n ++\t\tc = lookup_commit_reference(revs->repo, &oid);\n ++\t\tif (!c)\n ++\t\t\treturn error(msg, optarg);\n ++\t\tcommit_list_insert(c, &revs->ancestry_path_bottoms);\n \t} else if (!strcmp(arg, \"-g\") || !strcmp(arg, \"--walk-reflogs\")) {\n \t\tinit_reflog_walk(&revs->reflog_info);\n \t} else if (!strcmp(arg, \"--default\")) {\n @@ revision.h: struct rev_info {\n \t\t\tcherry_mark:1,\n \t\t\tbisect:1,\n \t\t\tancestry_path:1,\n -+\t\t\tancestry_path_need_bottoms:1,\n ++\n ++\t\t\t/* True if --ancestry-path was specified without an\n ++\t\t\t * argument. The bottom revisions are implicitly\n ++\t\t\t * the arguments in this case.\n ++\t\t\t */\n ++\t\t\tancestry_path_implicit_bottoms:1,\n ++\n \t\t\tfirst_parent_only:1,\n \t\t\texclude_first_parent_only:1,\n \t\t\tline_level_traverse:1,\n @@ t/t6019-rev-list-ancestry-path.sh: test_description='--ancestry-path'\n #\n # D..M -- M.t == M\n # --ancestry-path D..M -- M.t == M\n -@@ t/t6019-rev-list-ancestry-path.sh: test_expect_success 'rev-list --ancestry-path D..M' '\n - \ttest_cmp expect actual\n - '\n +@@ t/t6019-rev-list-ancestry-path.sh: test_ancestry () {\n + test_ancestry \"D..M\" \"E F G H I J K L M\"\n \n -+test_expect_success 'rev-list --ancestry-path=F D..M' '\n -+\ttest_write_lines E F J L M >expect &&\n -+\tgit rev-list --ancestry-path=F --format=%s D..M |\n -+\tsed -e \"/^commit /d\" |\n -+\tsort >actual &&\n -+\ttest_cmp expect actual\n -+'\n -+test_expect_success 'rev-list --ancestry-path=G D..M' '\n -+\ttest_write_lines G H I J L M >expect &&\n -+\tgit rev-list --ancestry-path=G --format=%s D..M |\n -+\tsed -e \"/^commit /d\" |\n -+\tsort >actual &&\n -+\ttest_cmp expect actual\n -+'\n -+test_expect_success 'rev-list --ancestry-path=H D..M' '\n -+\ttest_write_lines E G H I J L M >expect &&\n -+\tgit rev-list --ancestry-path=H --format=%s D..M |\n -+\tsed -e \"/^commit /d\" |\n -+\tsort >actual &&\n -+\ttest_cmp expect actual\n -+'\n -+\n -+test_expect_success 'rev-list --ancestry-path=K D..M' '\n -+\ttest_write_lines K L M >expect &&\n -+\tgit rev-list --ancestry-path=K --format=%s D..M |\n -+\tsed -e \"/^commit /d\" |\n -+\tsort >actual &&\n -+\ttest_cmp expect actual\n -+'\n -+\n -+test_expect_success 'rev-list --ancestry-path=F --ancestry-path=K D..M' '\n -+\ttest_write_lines E F J K L M >expect &&\n -+\tgit rev-list --ancestry-path=F --ancestry-path=K --format=%s D..M |\n -+\tsed -e \"/^commit /d\" |\n -+\tsort >actual &&\n -+\ttest_cmp expect actual\n -+'\n -+\n - test_expect_success 'rev-list D..M -- M.t' '\n - \techo M >expect &&\n - \tgit rev-list --format=%s D..M -- M.t |\n + test_ancestry \"--ancestry-path D..M\" \"E F H I J L M\"\n ++test_ancestry \"--ancestry-path=F D..M\" \"E F J L M\"\n ++test_ancestry \"--ancestry-path=G D..M\" \"G H I J L M\"\n ++test_ancestry \"--ancestry-path=H D..M\" \"E G H I J L M\"\n ++test_ancestry \"--ancestry-path=K D..M\" \"K L M\"\n ++test_ancestry \"--ancestry-path=F --ancestry-path=K D..M\" \"E F J K L M\"\n + \n + test_ancestry \"D..M -- M.t\" \"M\"\n + test_ancestry \"--ancestry-path D..M -- M.t\" \"M\"\n\n-- \ngitgitgadget\n"},{"id":"461502","messageId":"68ab719d99c68c81159c65ee42930a3023f275db.1660883290.git.gitgitgadget@gmail.com","threadId":"58314","inReplyTo":"pull.1303.v3.git.1660883290.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] rev-list-options.txt: fix simple typo","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-19T04:28:08Z","receivedAt":"2022-08-19T04:28:25Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nSigned-off-by: Elijah Newren \n---\n Documentation/rev-list-options.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 195e74eec63..2f85726745a 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -659,7 +659,7 @@ Here, the merge commits `O` and `P` contribute extra noise, as they did\n not actually contribute a change to `file.txt`. They only merged a topic\n that was based on an older version of `file.txt`. This is a common\n issue in repositories using a workflow where many contributors work in\n-parallel and merge their topic branches along a single trunk: manu\n+parallel and merge their topic branches along a single trunk: many\n unrelated merges appear in the `--full-history` results.\n \n When using the `--simplify-merges` option, the commits `O` and `P`\n-- \ngitgitgadget\n\n"},{"id":"461503","messageId":"5226da2123ee58992b0e723665c8d9c52d3a9544.1660883290.git.gitgitgadget@gmail.com","threadId":"58314","inReplyTo":"pull.1303.v3.git.1660883290.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] t6019: modernize tests with helper","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-19T04:28:09Z","receivedAt":"2022-08-19T04:28:28Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe tests in t6019 are repetitive, so create a helper that greatly\nsimplifies the test script.\n\nIn addition, update the common pattern that places 'git rev-list' on the\nleft side of a pipe, which can hide some exit codes. Send the output to\na 'raw' file that is then consumed by other tools so the Git exit code\nis verified as zero. And since we're using --format anyway, switch to\n`git log`, so that we get the desired format and can avoid using sed.\n\nSigned-off-by: Derrick Stolee \nSigned-off-by: Elijah Newren \n---\n t/t6019-rev-list-ancestry-path.sh | 87 +++++++++----------------------\n 1 file changed, 25 insertions(+), 62 deletions(-)\n\ndiff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh\nindex af57a04b7ff..5bd787a3c0a 100755\n--- a/t/t6019-rev-list-ancestry-path.sh\n+++ b/t/t6019-rev-list-ancestry-path.sh\n@@ -50,73 +50,36 @@ test_expect_success setup '\n \ttest_commit M\n '\n \n-test_expect_success 'rev-list D..M' '\n-\ttest_write_lines E F G H I J K L M >expect &&\n-\tgit rev-list --format=%s D..M |\n-\tsed -e \"/^commit /d\" |\n-\tsort >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'rev-list --ancestry-path D..M' '\n-\ttest_write_lines E F H I J L M >expect &&\n-\tgit rev-list --ancestry-path --format=%s D..M |\n-\tsed -e \"/^commit /d\" |\n-\tsort >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'rev-list D..M -- M.t' '\n-\techo M >expect &&\n-\tgit rev-list --format=%s D..M -- M.t |\n-\tsed -e \"/^commit /d\" >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'rev-list --ancestry-path D..M -- M.t' '\n-\techo M >expect &&\n-\tgit rev-list --ancestry-path --format=%s D..M -- M.t |\n-\tsed -e \"/^commit /d\" >actual &&\n-\ttest_cmp expect actual\n-'\n+test_ancestry () {\n+\targs=$1\n+\texpected=$2\n+\ttest_expect_success \"log $args\" \"\n+\t\ttest_write_lines $expected >expect &&\n+\t\tgit log --format=%s $args >raw &&\n+\n+\t\tif test -n \\\"$expected\\\"\n+\t\tthen\n+\t\t\tsort raw >actual &&\n+\t\t\ttest_cmp expect actual\n+\t\telse\n+\t\t\ttest_must_be_empty raw\n+\t\tfi\n+\t\"\n+}\n \n-test_expect_success 'rev-list F...I' '\n-\ttest_write_lines F G H I >expect &&\n-\tgit rev-list --format=%s F...I |\n-\tsed -e \"/^commit /d\" |\n-\tsort >actual &&\n-\ttest_cmp expect actual\n-'\n+test_ancestry \"D..M\" \"E F G H I J K L M\"\n \n-test_expect_success 'rev-list --ancestry-path F...I' '\n-\ttest_write_lines F H I >expect &&\n-\tgit rev-list --ancestry-path --format=%s F...I |\n-\tsed -e \"/^commit /d\" |\n-\tsort >actual &&\n-\ttest_cmp expect actual\n-'\n+test_ancestry \"--ancestry-path D..M\" \"E F H I J L M\"\n \n-# G.t is dropped in an \"-s ours\" merge\n-test_expect_success 'rev-list G..M -- G.t' '\n-\tgit rev-list --format=%s G..M -- G.t |\n-\tsed -e \"/^commit /d\" >actual &&\n-\ttest_must_be_empty actual\n-'\n+test_ancestry \"D..M -- M.t\" \"M\"\n+test_ancestry \"--ancestry-path D..M -- M.t\" \"M\"\n \n-test_expect_success 'rev-list --ancestry-path G..M -- G.t' '\n-\techo L >expect &&\n-\tgit rev-list --ancestry-path --format=%s G..M -- G.t |\n-\tsed -e \"/^commit /d\" >actual &&\n-\ttest_cmp expect actual\n-'\n+test_ancestry \"F...I\" \"F G H I\"\n+test_ancestry \"--ancestry-path F...I\" \"F H I\"\n \n-test_expect_success 'rev-list --ancestry-path --simplify-merges G^..M -- G.t' '\n-\ttest_write_lines G L >expect &&\n-\tgit rev-list --ancestry-path --simplify-merges --format=%s G^..M -- G.t |\n-\tsed -e \"/^commit /d\" |\n-\tsort >actual &&\n-\ttest_cmp expect actual\n-'\n+test_ancestry \"G..M -- G.t\" \"\"\n+test_ancestry \"--ancestry-path G..M -- G.t\" \"L\"\n+test_ancestry \"--ancestry-path --simplify-merges G^..M -- G.t\" \"G L\"\n \n # b---bc\n # / \\ /\n-- \ngitgitgadget\n\n"},{"id":"461504","messageId":"b810b3c8a2a7a9d121fc7c0438b4e4f3679557da.1660883290.git.gitgitgadget@gmail.com","threadId":"58314","inReplyTo":"pull.1303.v3.git.1660883290.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] revision: allow --ancestry-path to take an argument","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-19T04:28:10Z","receivedAt":"2022-08-19T04:28:30Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWe have long allowed users to run e.g.\n git log --ancestry-path master..seen\nwhich shows all commits which satisfy all three of these criteria:\n * are an ancestor of seen\n * are not an ancestor of master\n * have master as an ancestor\n\nThis commit allows another variant:\n git log --ancestry-path=$TOPIC master..seen\nwhich shows all commits which satisfy all of these criteria:\n * are an ancestor of seen\n * are not an ancestor of master\n * have $TOPIC in their ancestry-path\nthat last bullet can be defined as commits meeting any of these\ncriteria:\n * are an ancestor of $TOPIC\n * have $TOPIC as an ancestor\n * are $TOPIC\n\nThis also allows multiple --ancestry-path arguments, which can be\nused to find commits with any of the given topics in their ancestry\npath.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/rev-list-options.txt | 44 +++++++++++----\n object.h | 2 +-\n revision.c | 89 ++++++++++++++++++++----------\n revision.h | 9 +++\n t/t6019-rev-list-ancestry-path.sh | 14 ++++-\n 5 files changed, 115 insertions(+), 43 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 2f85726745a..aed486dc309 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -389,12 +389,14 @@ Default mode::\n \tmerges from the resulting history, as there are no selected\n \tcommits contributing to this merge.\n \n---ancestry-path::\n+--ancestry-path[=]::\n \tWhen given a range of commits to display (e.g. 'commit1..commit2'\n-\tor 'commit2 {caret}commit1'), only display commits that exist\n-\tdirectly on the ancestry chain between the 'commit1' and\n-\t'commit2', i.e. commits that are both descendants of 'commit1',\n-\tand ancestors of 'commit2'.\n+\tor 'commit2 {caret}commit1'), only display commits in that range\n+\tthat are ancestors of , descendants of , or\n+\t itself. If no commit is specified, use 'commit1' (the\n+\texcluded part of the range) as . Can be passed multiple\n+\ttimes; if so, a commit is included if it is any of the commits\n+\tgiven or if it is an ancestor or descendant of one of them.\n \n A more detailed explanation follows.\n \n@@ -568,11 +570,10 @@ Note the major differences in `N`, `P`, and `Q` over `--full-history`:\n \n There is another simplification mode available:\n \n---ancestry-path::\n-\tLimit the displayed commits to those directly on the ancestry\n-\tchain between the ``from'' and ``to'' commits in the given commit\n-\trange. I.e. only display commits that are ancestor of the ``to''\n-\tcommit and descendants of the ``from'' commit.\n+--ancestry-path[=]::\n+\tLimit the displayed commits to those which are an ancestor of\n+\t, or which are a descendant of , or are \n+\titself.\n +\n As an example use case, consider the following commit history:\n +\n@@ -604,6 +605,29 @@ option does. Applied to the 'D..M' range, it results in:\n \t\t\t \\\n \t\t\t\tL--M\n -----------------------------------------------------------------------\n++\n+We can also use `--ancestry-path=D` instead of `--ancestry-path` which\n+means the same thing when applied to the 'D..M' range but is just more\n+explicit.\n++\n+If we instead are interested in a given topic within this range, and all\n+commits affected by that topic, we may only want to view the subset of\n+`D..M` which contain that topic in their ancestry path. So, using\n+`--ancestry-path=H D..M` for example would result in:\n++\n+-----------------------------------------------------------------------\n+\t\tE\n+\t\t \\\n+\t\t G---H---I---J\n+\t\t\t \\\n+\t\t\t\tL--M\n+-----------------------------------------------------------------------\n++\n+Whereas `--ancestry-path=K D..M` would result in\n++\n+-----------------------------------------------------------------------\n+\t\tK---------------L--M\n+-----------------------------------------------------------------------\n \n Before discussing another option, `--show-pulls`, we need to\n create a new example history.\ndiff --git a/object.h b/object.h\nindex a2219464c2b..9caef89f1f0 100644\n--- a/object.h\n+++ b/object.h\n@@ -59,7 +59,7 @@ struct object_array {\n \n /*\n * object flag allocation:\n- * revision.h: 0---------10 15 23------26\n+ * revision.h: 0---------10 15 23------27\n * fetch-pack.c: 01 67\n * negotiator/default.c: 2--5\n * walker.c: 0-2\ndiff --git a/revision.c b/revision.c\nindex 0c6e26cd9c8..19d76da235c 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1105,7 +1105,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \t\t\t struct commit_list **list, struct prio_queue *queue)\n {\n \tstruct commit_list *parent = commit->parents;\n-\tunsigned left_flag;\n+\tunsigned pass_flags;\n \n \tif (commit->object.flags & ADDED)\n \t\treturn 0;\n@@ -1160,7 +1160,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \tif (revs->no_walk)\n \t\treturn 0;\n \n-\tleft_flag = (commit->object.flags & SYMMETRIC_LEFT);\n+\tpass_flags = (commit->object.flags & (SYMMETRIC_LEFT | ANCESTRY_PATH));\n \n \tfor (parent = commit->parents; parent; parent = parent->next) {\n \t\tstruct commit *p = parent->item;\n@@ -1181,7 +1181,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,\n \t\t\tif (!*slot)\n \t\t\t\t*slot = *revision_sources_at(revs->sources, commit);\n \t\t}\n-\t\tp->object.flags |= left_flag;\n+\t\tp->object.flags |= pass_flags;\n \t\tif (!(p->object.flags & SEEN)) {\n \t\t\tp->object.flags |= (SEEN | NOT_USER_GIVEN);\n \t\t\tif (list)\n@@ -1304,13 +1304,24 @@ static int still_interesting(struct commit_list *src, timestamp_t date, int slop\n }\n \n /*\n- * \"rev-list --ancestry-path A..B\" computes commits that are ancestors\n- * of B but not ancestors of A but further limits the result to those\n- * that are descendants of A. This takes the list of bottom commits and\n- * the result of \"A..B\" without --ancestry-path, and limits the latter\n- * further to the ones that can reach one of the commits in \"bottom\".\n+ * \"rev-list --ancestry-path=C_0 [--ancestry-path=C_1 ...] A..B\"\n+ * computes commits that are ancestors of B but not ancestors of A but\n+ * further limits the result to those that have any of C in their\n+ * ancestry path (i.e. are either ancestors of any of C, descendants\n+ * of any of C, or are any of C). If --ancestry-path is specified with\n+ * no commit, we use all bottom commits for C.\n+ *\n+ * Before this function is called, ancestors of C will have already\n+ * been marked with ANCESTRY_PATH previously.\n+ *\n+ * This takes the list of bottom commits and the result of \"A..B\"\n+ * without --ancestry-path, and limits the latter further to the ones\n+ * that have any of C in their ancestry path. Since the ancestors of C\n+ * have already been marked (a prerequisite of this function), we just\n+ * need to mark the descendants, then exclude any commit that does not\n+ * have any of these marks.\n */\n-static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *list)\n+static void limit_to_ancestry(struct commit_list *bottoms, struct commit_list *list)\n {\n \tstruct commit_list *p;\n \tstruct commit_list *rlist = NULL;\n@@ -1323,7 +1334,7 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li\n \tfor (p = list; p; p = p->next)\n \t\tcommit_list_insert(p->item, &rlist);\n \n-\tfor (p = bottom; p; p = p->next)\n+\tfor (p = bottoms; p; p = p->next)\n \t\tp->item->object.flags |= TMP_MARK;\n \n \t/*\n@@ -1356,38 +1367,39 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li\n \t */\n \n \t/*\n-\t * The ones that are not marked with TMP_MARK are uninteresting\n+\t * The ones that are not marked with either TMP_MARK or\n+\t * ANCESTRY_PATH are uninteresting\n \t */\n \tfor (p = list; p; p = p->next) {\n \t\tstruct commit *c = p->item;\n-\t\tif (c->object.flags & TMP_MARK)\n+\t\tif (c->object.flags & (TMP_MARK | ANCESTRY_PATH))\n \t\t\tcontinue;\n \t\tc->object.flags |= UNINTERESTING;\n \t}\n \n-\t/* We are done with the TMP_MARK */\n+\t/* We are done with TMP_MARK and ANCESTRY_PATH */\n \tfor (p = list; p; p = p->next)\n-\t\tp->item->object.flags &= ~TMP_MARK;\n-\tfor (p = bottom; p; p = p->next)\n-\t\tp->item->object.flags &= ~TMP_MARK;\n+\t\tp->item->object.flags &= ~(TMP_MARK | ANCESTRY_PATH);\n+\tfor (p = bottoms; p; p = p->next)\n+\t\tp->item->object.flags &= ~(TMP_MARK | ANCESTRY_PATH);\n \tfree_commit_list(rlist);\n }\n \n /*\n- * Before walking the history, keep the set of \"negative\" refs the\n- * caller has asked to exclude.\n+ * Before walking the history, add the set of \"negative\" refs the\n+ * caller has asked to exclude to the bottom list.\n *\n * This is used to compute \"rev-list --ancestry-path A..B\", as we need\n * to filter the result of \"A..B\" further to the ones that can actually\n * reach A.\n */\n-static struct commit_list *collect_bottom_commits(struct commit_list *list)\n+static void collect_bottom_commits(struct commit_list *list,\n+\t\t\t\t struct commit_list **bottom)\n {\n-\tstruct commit_list *elem, *bottom = NULL;\n+\tstruct commit_list *elem;\n \tfor (elem = list; elem; elem = elem->next)\n \t\tif (elem->item->object.flags & BOTTOM)\n-\t\t\tcommit_list_insert(elem->item, &bottom);\n-\treturn bottom;\n+\t\t\tcommit_list_insert(elem->item, bottom);\n }\n \n /* Assumes either left_only or right_only is set */\n@@ -1414,12 +1426,12 @@ static int limit_list(struct rev_info *revs)\n \tstruct commit_list *original_list = revs->commits;\n \tstruct commit_list *newlist = NULL;\n \tstruct commit_list **p = &newlist;\n-\tstruct commit_list *bottom = NULL;\n \tstruct commit *interesting_cache = NULL;\n \n-\tif (revs->ancestry_path) {\n-\t\tbottom = collect_bottom_commits(original_list);\n-\t\tif (!bottom)\n+\tif (revs->ancestry_path_implicit_bottoms) {\n+\t\tcollect_bottom_commits(original_list,\n+\t\t\t\t &revs->ancestry_path_bottoms);\n+\t\tif (!revs->ancestry_path_bottoms)\n \t\t\tdie(\"--ancestry-path given but there are no bottom commits\");\n \t}\n \n@@ -1464,9 +1476,8 @@ static int limit_list(struct rev_info *revs)\n \tif (revs->left_only || revs->right_only)\n \t\tlimit_left_right(newlist, revs);\n \n-\tif (bottom)\n-\t\tlimit_to_ancestry(bottom, newlist);\n-\tfree_commit_list(bottom);\n+\tif (revs->ancestry_path)\n+\t\tlimit_to_ancestry(revs->ancestry_path_bottoms, newlist);\n \n \t/*\n \t * Check if any commits have become TREESAME by some of their parents\n@@ -2213,7 +2224,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\t\t const struct setup_revision_opt* opt)\n {\n \tconst char *arg = argv[0];\n-\tconst char *optarg;\n+\tconst char *optarg = NULL;\n \tint argcount;\n \tconst unsigned hexsz = the_hash_algo->hexsz;\n \n@@ -2284,6 +2295,23 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->ancestry_path = 1;\n \t\trevs->simplify_history = 0;\n \t\trevs->limited = 1;\n+\t\trevs->ancestry_path_implicit_bottoms = 1;\n+\t} else if (skip_prefix(arg, \"--ancestry-path=\", &optarg)) {\n+\t\tstruct commit *c;\n+\t\tstruct object_id oid;\n+\t\tconst char *msg = _(\"could not get commit for ancestry-path argument %s\");\n+\n+\t\trevs->ancestry_path = 1;\n+\t\trevs->simplify_history = 0;\n+\t\trevs->limited = 1;\n+\n+\t\tif (repo_get_oid_committish(revs->repo, optarg, &oid))\n+\t\t\treturn error(msg, optarg);\n+\t\tget_reference(revs, optarg, &oid, ANCESTRY_PATH);\n+\t\tc = lookup_commit_reference(revs->repo, &oid);\n+\t\tif (!c)\n+\t\t\treturn error(msg, optarg);\n+\t\tcommit_list_insert(c, &revs->ancestry_path_bottoms);\n \t} else if (!strcmp(arg, \"-g\") || !strcmp(arg, \"--walk-reflogs\")) {\n \t\tinit_reflog_walk(&revs->reflog_info);\n \t} else if (!strcmp(arg, \"--default\")) {\n@@ -2991,6 +3019,7 @@ static void release_revisions_topo_walk_info(struct topo_walk_info *info);\n void release_revisions(struct rev_info *revs)\n {\n \tfree_commit_list(revs->commits);\n+\tfree_commit_list(revs->ancestry_path_bottoms);\n \tobject_array_clear(&revs->pending);\n \tobject_array_clear(&revs->boundary_commits);\n \trelease_revisions_cmdline(&revs->cmdline);\ndiff --git a/revision.h b/revision.h\nindex e576845cdd1..a10520300ce 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -48,6 +48,7 @@\n */\n #define NOT_USER_GIVEN\t(1u<<25)\n #define TRACK_LINEAR\t(1u<<26)\n+#define ANCESTRY_PATH\t(1u<<27)\n #define ALL_REV_FLAGS\t(((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR | PULL_MERGE)\n \n #define DECORATE_SHORT_REFS\t1\n@@ -164,6 +165,13 @@ struct rev_info {\n \t\t\tcherry_mark:1,\n \t\t\tbisect:1,\n \t\t\tancestry_path:1,\n+\n+\t\t\t/* True if --ancestry-path was specified without an\n+\t\t\t * argument. The bottom revisions are implicitly\n+\t\t\t * the arguments in this case.\n+\t\t\t */\n+\t\t\tancestry_path_implicit_bottoms:1,\n+\n \t\t\tfirst_parent_only:1,\n \t\t\texclude_first_parent_only:1,\n \t\t\tline_level_traverse:1,\n@@ -306,6 +314,7 @@ struct rev_info {\n \tstruct saved_parents *saved_parents_slab;\n \n \tstruct commit_list *previous_parents;\n+\tstruct commit_list *ancestry_path_bottoms;\n \tconst char *break_bar;\n \n \tstruct revision_sources *sources;\ndiff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh\nindex 5bd787a3c0a..738da23628b 100755\n--- a/t/t6019-rev-list-ancestry-path.sh\n+++ b/t/t6019-rev-list-ancestry-path.sh\n@@ -8,8 +8,13 @@ test_description='--ancestry-path'\n # / \\\n # A-------K---------------L--M\n #\n-# D..M == E F G H I J K L M\n-# --ancestry-path D..M == E F H I J L M\n+# D..M == E F G H I J K L M\n+# --ancestry-path D..M == E F H I J L M\n+# --ancestry-path=F D..M == E F J L M\n+# --ancestry-path=G D..M == G H I J L M\n+# --ancestry-path=H D..M == E G H I J L M\n+# --ancestry-path=K D..M == K L M\n+# --ancestry-path=K --ancestry-path=F D..M == E F J K L M\n #\n # D..M -- M.t == M\n # --ancestry-path D..M -- M.t == M\n@@ -70,6 +75,11 @@ test_ancestry () {\n test_ancestry \"D..M\" \"E F G H I J K L M\"\n \n test_ancestry \"--ancestry-path D..M\" \"E F H I J L M\"\n+test_ancestry \"--ancestry-path=F D..M\" \"E F J L M\"\n+test_ancestry \"--ancestry-path=G D..M\" \"G H I J L M\"\n+test_ancestry \"--ancestry-path=H D..M\" \"E G H I J L M\"\n+test_ancestry \"--ancestry-path=K D..M\" \"K L M\"\n+test_ancestry \"--ancestry-path=F --ancestry-path=K D..M\" \"E F J K L M\"\n \n test_ancestry \"D..M -- M.t\" \"M\"\n test_ancestry \"--ancestry-path D..M -- M.t\" \"M\"\n-- \ngitgitgadget\n"},{"id":"461547","messageId":"7b7918f8-d997-89e8-2ba0-7bb31beb9335@github.com","threadId":"58314","inReplyTo":"pull.1303.v3.git.1660883290.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/3] Allow --ancestry-path to take an argument","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-19T12:53:55Z","receivedAt":"2022-08-19T12:54:36Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/19/2022 12:28 AM, Elijah Newren via GitGitGadget wrote:\n> Changes since v2:\n> \n> * Incorporated Stolee's suggested patch as a preliminary cleanup of t6019.\n> Slightly modified his patch by:\n> * fixing the \"repetitive\" typo pointed out by Eric\n> * removed the unnecessary \"return 1\" pointed out by Ævar\n> * switched \"rev-list\" to \"log\" since we are using --format anyway, in\n> order to remove the need to call \"sed\" afterward\n\nThanks! I appreciate you fixing all of my mistakes and\nincorporating all of the existing feedback. It also looks\nnicer as your patch 2 than as a third on top.\n\nThis version looks good to me.\n\n-Stolee\n"},{"id":"461582","messageId":"20220819172519.3703282-1-jonathantanmy@google.com","threadId":"58314","inReplyTo":"CABPp-BFjxFeGO+NU4HFCGqDe4aRFhqOdOxNYVDf7EJOWdT5RgA@mail.gmail.com","subject":"Re: [PATCH v2 2/2] revision: allow --ancestry-path to take an argument","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-08-19T17:25:19Z","receivedAt":"2022-08-19T17:52:18Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Elijah Newren writes:\n> > Optional: Besides that, from what I can tell, sometimes the C commits\n> > themselves are marked with ANCESTRY_PATH (when they are explicitly\n> > specified) and sometimes they are not (when they are not explicitly\n> > specified). It's not a bug here, but it might be worth handling that in\n> > the ancestry_path_need_bottoms codepath (instead of explicitly setting\n> > TMP_MARK on the bottoms in limit_to_ancestry() - that way, I think we\n> > can also use ANCESTRY_PATH instead of TMP_MARK throughout the ancestry\n> > path codepaths, but I haven't tested it), at least to prevent possible\n> > future bugs.\n> \n> That sounds like you're trying to duplicate the bug in my first\n> attempt at this patch. If you try to coalesce ANCESTRY_PATH and\n> TMP_MARK, then you not only get all descendants of C, you also get all\n> descendants of any ancestor of C, which defeats the whole point of my\n> changes.\n\nAh, yes you're right.\n\n> It's true that I don't mark implicit C commits with ANCESTRY_PATH, but\n> those are always bottom commits that are the excluded end of a range\n> anyway. While those could be marked without causing problems, it\n> would always be a waste of effort.\n\nYes, that's true.\n"},{"id":"461589","messageId":"xmqq7d34m84p.fsf@gitster.g","threadId":"58314","inReplyTo":"b810b3c8a2a7a9d121fc7c0438b4e4f3679557da.1660883290.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/3] revision: allow --ancestry-path to take an argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-19T17:54:30Z","receivedAt":"2022-08-19T18:07:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> \t\t\tancestry_path:1,\n> +\n> +\t\t\t/* True if --ancestry-path was specified without an\n> +\t\t\t * argument. The bottom revisions are implicitly\n> +\t\t\t * the arguments in this case.\n> +\t\t\t */\n> +\t\t\tancestry_path_implicit_bottoms:1,\n\nThe above comment explains why this is called \"implicit\" very well.\nThe earlier round used \"need\", but the word missed the essense (i.e.\nancestry_path argument is implicit and not given, so we need to\ncompute bottoms and use them instead). The new name is certainly\nbetter.\n\n"},{"id":"461619","messageId":"xmqqtu68j60w.fsf@gitster.g","threadId":"58314","inReplyTo":"7b7918f8-d997-89e8-2ba0-7bb31beb9335@github.com","subject":"Re: [PATCH v3 0/3] Allow --ancestry-path to take an argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-19T21:08:15Z","receivedAt":"2022-08-19T21:08:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 8/19/2022 12:28 AM, Elijah Newren via GitGitGadget wrote:\n>> Changes since v2:\n>> \n>> * Incorporated Stolee's suggested patch as a preliminary cleanup of t6019.\n>> Slightly modified his patch by:\n>> * fixing the \"repetitive\" typo pointed out by Eric\n>> * removed the unnecessary \"return 1\" pointed out by Ævar\n>> * switched \"rev-list\" to \"log\" since we are using --format anyway, in\n>> order to remove the need to call \"sed\" afterward\n>\n> Thanks! I appreciate you fixing all of my mistakes and\n> incorporating all of the existing feedback. It also looks\n> nicer as your patch 2 than as a third on top.\n>\n> This version looks good to me.\n\nYup, it looks good to me, too. Thanks, both.\n\nLet's mark it for 'next' and merge it down soonish.\n"},{"id":"461638","messageId":"CABPp-BEvDjBqPxrAEzUXaLFWcwK+UmVH7bDXZm30RRrjv++Gxw@mail.gmail.com","threadId":"58314","inReplyTo":"xmqq7d34m84p.fsf@gitster.g","subject":"Re: [PATCH v3 3/3] revision: allow --ancestry-path to take an argument","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-20T00:10:50Z","receivedAt":"2022-08-20T00:11:07Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Aug 19, 2022 at 10:54 AM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > ancestry_path:1,\n> > +\n> > + /* True if --ancestry-path was specified without an\n> > + * argument. The bottom revisions are implicitly\n> > + * the arguments in this case.\n> > + */\n> > + ancestry_path_implicit_bottoms:1,\n>\n> The above comment explains why this is called \"implicit\" very well.\n> The earlier round used \"need\", but the word missed the essense (i.e.\n> ancestry_path argument is implicit and not given, so we need to\n> compute bottoms and use them instead). The new name is certainly\n> better.\n\nYep, I agree, but all credit for this improvement goes to Jonathan;\nthis was one of many good suggestions in his careful review.\n"},{"id":"461639","messageId":"CABPp-BFOag0sTO_wa8qM2h-GWigyQdj9ki0Rrd1CCKD39Y96NA@mail.gmail.com","threadId":"58314","inReplyTo":"xmqqtu68j60w.fsf@gitster.g","subject":"Re: [PATCH v3 0/3] Allow --ancestry-path to take an argument","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-20T00:13:13Z","receivedAt":"2022-08-20T00:13:32Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Aug 19, 2022 at 2:08 PM Junio C Hamano wrote:\n>\n> Derrick Stolee writes:\n>\n> > On 8/19/2022 12:28 AM, Elijah Newren via GitGitGadget wrote:\n> >> Changes since v2:\n> >>\n> >> * Incorporated Stolee's suggested patch as a preliminary cleanup of t6019.\n> >> Slightly modified his patch by:\n> >> * fixing the \"repetitive\" typo pointed out by Eric\n> >> * removed the unnecessary \"return 1\" pointed out by Ævar\n> >> * switched \"rev-list\" to \"log\" since we are using --format anyway, in\n> >> order to remove the need to call \"sed\" afterward\n> >\n> > Thanks! I appreciate you fixing all of my mistakes and\n> > incorporating all of the existing feedback. It also looks\n> > nicer as your patch 2 than as a third on top.\n> >\n> > This version looks good to me.\n>\n> Yup, it looks good to me, too. Thanks, both.\n\nAnd thanks to all the other reviewers as well! Good suggestions came\nin from several different folks.\n\n> Let's mark it for 'next' and merge it down soonish.\n\nSounds good.\n"}]}