{"thread":{"id":"52190","subject":"[Outreachy] [PATCH v7 0/1] Revenge of --bisect --first-parent","startedAt":"2019-11-05T05:21:49Z","lastAt":"2019-11-07T18:56:36Z","messageCount":10,"participants":["workingjubilee@gmail.com","Jonathan Tan","Junio C Hamano","Johannes Schindelin","Jubilee Young"],"isPatch":true,"patchVersion":7,"patchTotal":1},"messages":[{"id":"385444","messageId":"20191105052141.15913-1-workingjubilee@gmail.com","threadId":"52190","inReplyTo":null,"subject":"[Outreachy] [PATCH v7 0/1] Revenge of --bisect --first-parent","fromName":"","fromEmail":"workingjubilee@gmail.com","sentAt":"2019-11-05T05:21:40Z","receivedAt":"2019-11-05T05:21:49Z","isPatch":true,"sender":{"key":"workingjubilee@gmail.com","avatar":null},"body":"From: Jubilee Young <workingjubilee@gmail.com>\n\nHello!\n\nI meant to introduce myself quite a bit earlier, but alas, the world is\nthe kind which also results in version control systems being critical\ninstead of merely desirable. At least, for software development. Yet,\ntime marches on. I know you have quite a few applicants to pick between\nfor the Outreachy internship but I would be honored if you would grant\nme your consideration, nonetheless.\n\n...And if you would merge this patch, too, if it's (finally) up to \nstandard! This is the renovation and update of a long-abandoned feature\nbranch begun by Harald Nordgren and extended by Tiago Botelho some time\nback. The world caught up with them, too, but one re(base|animation)\nlater, these works live on, zombie-like. I did my best to understand\nand integrate everything with the code base in spite of the many\nchanges introduced since. There were many recommendations to the last\niteration, and a fair number of changes were integrated essentially as \nthey were given, with the most extensive work being in the department\nof the tests and refactoring them to reuse the originally generated\ncommit graph.\n\nMy cursory experiments seem to yield the desired result, and the tests\n(which have already proven useful, to my dismay) do pass, so it seems\nrelatively sound for a first try. But it's possible I misunderstood\nsomething and it still needs work. If so, please let me know so that I\ncan fix it and at long last, this patch can live again!\n\nBonus reading for context (most recent first):\nhttps://public-inbox.org/git/20180828123234.44582-1-tiagonbotelho@hotmail.com/\nhttps://public-inbox.org/git/20180622123945.68852-1-tiagonbotelho@hotmail.com/\nhttps://public-inbox.org/git/20180528092017.19022-1-tiagonbotelho@hotmail.com/\nhttps://public-inbox.org/git/20180523120028.36427-1-tiagonbotelho@hotmail.com/\nhttps://public-inbox.org/git/20180510191259.78044-1-tiagonbotelho@hotmail.com/\nhttps://public-inbox.org/git/20180509215728.57046-1-tiagonbotelho@hotmail.com/\nhttps://public-inbox.org/git/20180411225534.48658-1-haraldnordgren@gmail.com/\n\nJubilee Young (1):\n  Implement rev-list --bisect* --first-parent\n\n bisect.c                   | 56 ++++++++++++++++++++++++++------------\n bisect.h                   | 17 ++++++++++--\n builtin/rev-list.c         |  9 ++++--\n revision.c                 |  3 --\n t/t6000-rev-list-misc.sh   |  4 +--\n t/t6002-rev-list-bisect.sh | 54 ++++++++++++++++++++++++++++++++++++\n 6 files changed, 116 insertions(+), 27 deletions(-)\n\n-- \n2.24.0.1.gdc0fb1c0fe\n"},{"id":"385445","messageId":"20191105052141.15913-2-workingjubilee@gmail.com","threadId":"52190","inReplyTo":"20191105052141.15913-1-workingjubilee@gmail.com","subject":"[PATCH v7 1/1] Implement rev-list --bisect* --first-parent","fromName":"","fromEmail":"workingjubilee@gmail.com","sentAt":"2019-11-05T05:21:41Z","receivedAt":"2019-11-05T05:21:51Z","isPatch":true,"sender":{"key":"workingjubilee@gmail.com","avatar":null},"body":"From: Jubilee Young <workingjubilee@gmail.com>\n\nNot all repository maintainers expect every commit to pass tests, only\ntesting the merge commits. Currently bisection assumes every commit is\nof interest. The highly-requested --bisect --first-parent feature imbues\ngit with the same indifference to minutiae when the option is set, so\nthat it casually riffles through commits, throwing aside mountains of\nirrelevant data when looking for a breaking change. Further refinement\nof where breaks occurred can be gained by bisecting over the merge's\nrange.\n\nNote, bisecting on --first-parent becomes part of findall's previously\nexisting pass-through as an \"option state\" flag.\n\nIn order to limit possible obfuscation of bisect operations resulting\nfrom the addition of new flags, some extra documentation was folded in\nto the patch.\n\nSigned-off-by: Jubilee Young <workingjubilee@gmail.com>\nBased-on-patch-by: Tiago Botelho <tiagonbotelho@hotmail.com>\nBased-on-patch-by: Harald Nordgren <haraldnordgren@gmail.com>\n---\n bisect.c                   | 56 ++++++++++++++++++++++++++------------\n bisect.h                   | 17 ++++++++++--\n builtin/rev-list.c         |  9 ++++--\n revision.c                 |  3 --\n t/t6000-rev-list-misc.sh   |  4 +--\n t/t6002-rev-list-bisect.sh | 54 ++++++++++++++++++++++++++++++++++++\n 6 files changed, 116 insertions(+), 27 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex e81c91d02c..54129a796b 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -88,15 +88,16 @@ static inline void weight_set(struct commit_list *elem, int weight)\n \t**commit_weight_at(&commit_weight, elem->item) = weight;\n }\n \n-static int count_interesting_parents(struct commit *commit)\n+static int count_interesting_parents(struct commit *commit, unsigned bisect_flags)\n {\n \tstruct commit_list *p;\n \tint count;\n \n \tfor (count = 0, p = commit->parents; p; p = p->next) {\n-\t\tif (p->item->object.flags & UNINTERESTING)\n-\t\t\tcontinue;\n-\t\tcount++;\n+\t\tif (!(p->item->object.flags & UNINTERESTING))\n+\t\t\tcount++;\n+\t\tif (bisect_flags & BISECT_FIRST_PARENT)\n+\t\t\tbreak;\n \t}\n \treturn count;\n }\n@@ -123,7 +124,7 @@ static inline int halfway(struct commit_list *p, int nr)\n }\n \n static void show_list(const char *debug, int counted, int nr,\n-\t\t      struct commit_list *list)\n+\t\t      struct commit_list *list, unsigned bisect_flags)\n {\n \tstruct commit_list *p;\n \n@@ -259,10 +260,12 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n  */\n static struct commit_list *do_find_bisection(struct commit_list *list,\n \t\t\t\t\t     int nr, int *weights,\n-\t\t\t\t\t     int find_all)\n+\t\t\t\t\t     unsigned bisect_flags)\n {\n \tint n, counted;\n \tstruct commit_list *p;\n+\tunsigned first_parent = bisect_flags & BISECT_FIRST_PARENT;\n+\tunsigned find_all = bisect_flags & BISECT_FIND_ALL;\n \n \tcounted = 0;\n \n@@ -271,13 +274,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list,\n \t\tunsigned flags = commit->object.flags;\n \n \t\t*commit_weight_at(&commit_weight, p->item) = &weights[n++];\n-\t\tswitch (count_interesting_parents(commit)) {\n+\t\tswitch (count_interesting_parents(commit, bisect_flags)) {\n \t\tcase 0:\n \t\t\tif (!(flags & TREESAME)) {\n \t\t\t\tweight_set(p, 1);\n \t\t\t\tcounted++;\n \t\t\t\tshow_list(\"bisection 2 count one\",\n-\t\t\t\t\t  counted, nr, list);\n+\t\t\t\t\t  counted, nr, list, bisect_flags);\n \t\t\t}\n \t\t\t/*\n \t\t\t * otherwise, it is known not to reach any\n@@ -293,7 +296,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,\n \t\t}\n \t}\n \n-\tshow_list(\"bisection 2 initialize\", counted, nr, list);\n+\tshow_list(\"bisection 2 initialize\", counted, nr, list, bisect_flags);\n \n \t/*\n \t * If you have only one parent in the resulting set\n@@ -323,7 +326,8 @@ static struct commit_list *do_find_bisection(struct commit_list *list,\n \t\tcounted++;\n \t}\n \n-\tshow_list(\"bisection 2 count_distance\", counted, nr, list);\n+\t/* should match bisection 2 initialize when BISECT_FIRST_PARENT */\n+\tshow_list(\"bisection 2 count_distance\", counted, nr, list, bisect_flags);\n \n \twhile (counted < nr) {\n \t\tfor (p = list; p; p = p->next) {\n@@ -333,6 +337,17 @@ static struct commit_list *do_find_bisection(struct commit_list *list,\n \t\t\tif (0 <= weight(p))\n \t\t\t\tcontinue;\n \t\t\tfor (q = p->item->parents; q; q = q->next) {\n+\t\t\t\t/*\n+\t\t\t\t * first_parent can skip parent nodes, but only when\n+\t\t\t\t * confirmed as being of no interest.\n+\t\t\t\t */\n+\t\t\t\tif (first_parent) {\n+\t\t\t\t\tif ((q->item->object.flags & UNINTERESTING) ||\n+\t\t\t\t\t\t(weight(q) < 0)) {\n+\t\t\t\t\t\tq = NULL;\n+\t\t\t\t\t}\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n \t\t\t\tif (q->item->object.flags & UNINTERESTING)\n \t\t\t\t\tcontinue;\n \t\t\t\tif (0 <= weight(q))\n@@ -350,7 +365,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,\n \t\t\t\tweight_set(p, weight(q)+1);\n \t\t\t\tcounted++;\n \t\t\t\tshow_list(\"bisection 2 count one\",\n-\t\t\t\t\t  counted, nr, list);\n+\t\t\t\t\t  counted, nr, list, bisect_flags);\n \t\t\t}\n \t\t\telse\n \t\t\t\tweight_set(p, weight(q));\n@@ -361,7 +376,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,\n \t\t}\n \t}\n \n-\tshow_list(\"bisection 2 counted all\", counted, nr, list);\n+\tshow_list(\"bisection 2 counted all\", counted, nr, list, bisect_flags);\n \n \tif (!find_all)\n \t\treturn best_bisection(list, nr);\n@@ -370,13 +385,14 @@ static struct commit_list *do_find_bisection(struct commit_list *list,\n }\n \n void find_bisection(struct commit_list **commit_list, int *reaches,\n-\t\t    int *all, int find_all)\n+\t\t    int *all, unsigned bisect_flags)\n {\n \tint nr, on_list;\n \tstruct commit_list *list, *p, *best, *next, *last;\n \tint *weights;\n+\tunsigned find_all = bisect_flags & BISECT_FIND_ALL;\n \n-\tshow_list(\"bisection 2 entry\", 0, 0, *commit_list);\n+\tshow_list(\"bisection 2 entry\", 0, 0, *commit_list, bisect_flags);\n \tinit_commit_weight(&commit_weight);\n \n \t/*\n@@ -400,13 +416,13 @@ void find_bisection(struct commit_list **commit_list, int *reaches,\n \t\ton_list++;\n \t}\n \tlist = last;\n-\tshow_list(\"bisection 2 sorted\", 0, nr, list);\n+\tshow_list(\"bisection 2 sorted\", 0, nr, list, bisect_flags);\n \n \t*all = nr;\n \tweights = xcalloc(on_list, sizeof(*weights));\n \n \t/* Do the real work of finding bisection commit. */\n-\tbest = do_find_bisection(list, nr, weights, find_all);\n+\tbest = do_find_bisection(list, nr, weights, bisect_flags);\n \tif (best) {\n \t\tif (!find_all) {\n \t\t\tlist->item = best->item;\n@@ -950,6 +966,7 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n \tstruct rev_info revs;\n \tstruct commit_list *tried;\n \tint reaches = 0, all = 0, nr, steps;\n+\tunsigned bisect_flags = 0;\n \tstruct object_id *bisect_rev;\n \tchar *steps_msg;\n \n@@ -964,7 +981,12 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n \n \tbisect_common(&revs);\n \n-\tfind_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr);\n+\tif (skipped_revs.nr)\n+\t\tbisect_flags |= BISECT_FIND_ALL;\n+\tif (revs.first_parent_only)\n+\t\tbisect_flags |= BISECT_FIRST_PARENT;\n+\n+\tfind_bisection(&revs.commits, &reaches, &all, bisect_flags);\n \trevs.commits = managed_skipped(revs.commits, &tried);\n \n \tif (!revs.commits) {\ndiff --git a/bisect.h b/bisect.h\nindex 4e69a11ea8..518be01999 100644\n--- a/bisect.h\n+++ b/bisect.h\n@@ -3,16 +3,18 @@\n \n struct commit_list;\n struct repository;\n+#define BISECT_FIND_ALL\t(1U<<0)\n+#define BISECT_FIRST_PARENT\t(1U<<1)\n \n /*\n  * Find bisection. If something is found, `reaches` will be the number of\n  * commits that the best commit reaches. `all` will be the count of\n  * non-SAMETREE commits. If nothing is found, `list` will be NULL.\n  * Otherwise, it will be either all non-SAMETREE commits or the single\n- * best commit, as chosen by `find_all`.\n+ * best commit, as chosen by the flag `BISECT_FIND_ALL`.\n  */\n void find_bisection(struct commit_list **list, int *reaches, int *all,\n-\t\t    int find_all);\n+\t\t    unsigned bisect_flags);\n \n struct commit_list *filter_skipped(struct commit_list *list,\n \t\t\t\t   struct commit_list **tried,\n@@ -31,6 +33,17 @@ struct rev_list_info {\n \tconst char *header_prefix;\n };\n \n+/*\n+ * Coordinates a bisection by examining input made available so far,\n+ * setting up internal variables, and then bisecting with them.\n+ * no_checkout directs this to only update BISECT_HEAD refs.\n+ *\n+ * Exit code 10 on successful bisection, so caller should exit with 0.\n+ * Exit code 4 when no commits were found to bisect through.\n+ * Exit code 1 MAY result from skipping the commit it would report.\n+ *\n+ * Otherwise, returns a call to command handlers which choose an exit.\n+ */\n int bisect_next_all(struct repository *r,\n \t\t    const char *prefix,\n \t\t    int no_checkout);\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex e28d62ec64..759a182ec6 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -374,8 +374,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tint i;\n \tint bisect_list = 0;\n \tint bisect_show_vars = 0;\n-\tint bisect_find_all = 0;\n \tint use_bitmap_index = 0;\n+\tunsigned bisect_flags = 0;\n \tconst char *show_progress = NULL;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n@@ -443,7 +443,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tif (!strcmp(arg, \"--bisect-all\")) {\n \t\t\tbisect_list = 1;\n-\t\t\tbisect_find_all = 1;\n+\t\t\tbisect_flags |= BISECT_FIND_ALL;\n \t\t\tinfo.flags |= BISECT_SHOW_ALL;\n \t\t\trevs.show_decorations = 1;\n \t\t\tcontinue;\n@@ -565,7 +565,10 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tif (bisect_list) {\n \t\tint reaches, all;\n \n-\t\tfind_bisection(&revs.commits, &reaches, &all, bisect_find_all);\n+\t\tif (revs.first_parent_only)\n+\t\t\tbisect_flags |= BISECT_FIRST_PARENT;\n+\n+\t\tfind_bisection(&revs.commits, &reaches, &all, bisect_flags);\n \n \t\tif (bisect_show_vars)\n \t\t\treturn show_bisect_vars(&info, reaches, all);\ndiff --git a/revision.c b/revision.c\nindex 0e39b2b8a5..c6f0a9f213 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2716,9 +2716,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \tif (!revs->reflog_info && revs->grep_filter.use_reflog_filter)\n \t\tdie(\"cannot use --grep-reflog without --walk-reflogs\");\n \n-\tif (revs->first_parent_only && revs->bisect)\n-\t\tdie(_(\"--first-parent is incompatible with --bisect\"));\n-\n \tif (revs->line_level_traverse &&\n \t    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))\n \t\tdie(_(\"-L does not yet support diff formats besides -p and -s\"));\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex b8cf82349b..95949e4ff1 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -122,8 +122,8 @@ test_expect_success 'rev-list can negate index objects' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success '--bisect and --first-parent can not be combined' '\n-\ttest_must_fail git rev-list --bisect --first-parent HEAD\n+test_expect_success '--bisect and --first-parent CAN be combined' '\n+\tgit rev-list --bisect --first-parent HEAD\n '\n \n test_expect_success '--header shows a NUL after each commit' '\ndiff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh\nindex a661408038..6caf2af650 100755\n--- a/t/t6002-rev-list-bisect.sh\n+++ b/t/t6002-rev-list-bisect.sh\n@@ -263,4 +263,58 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' '\n \ttest_cmp expect.sorted actual.sorted\n '\n \n+# --first-parent tests\n+\n+# --bisect --first-parent should pluck out the middle.\n+printf \"%s\\n\" e4 |\n+test_output_expect_success \"--bisect --first-parent\" '\n+\tgit rev-list --bisect --first-parent E ^F\n+'\n+\n+printf \"%s\\n\" E e1 e2 e3 e4 e5 e6 e7 e8 |\n+test_output_expect_success \"--first-parent\" '\n+\tgit rev-list --first-parent E ^F\n+'\n+\n+test_output_expect_success '--bisect-vars --first-parent' '\n+\tgit rev-list --bisect-vars --first-parent E ^F\n+' <<-EOF\n+\tbisect_rev='e5'\n+\tbisect_nr=4\n+\tbisect_good=4\n+\tbisect_bad=3\n+\tbisect_all=9\n+\tbisect_steps=2\n+EOF\n+\n+test_expect_success '--bisect-all --first-parent returns correct order' '\n+\tgit rev-list --bisect-all --first-parent E ^F >actual &&\n+\n+\t# Make sure the entries are sorted in the dist order\n+\tsed -e \"s/.*dist=\\([0-9]\\).*/\\1/\" actual >actual.dists &&\n+\tsort -r -n actual.dists >actual.dists.sorted &&\n+\ttest_cmp actual.dists.sorted actual.dists\n+'\n+\n+# NEEDSWORK: this test could afford being hardened against other\n+# changes in the same file.\n+test_expect_success '--bisect-all --first-parent compares correctly' '\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse tags/e5) (tag: e5, dist=4)\n+\t$(git rev-parse tags/e4) (tag: e4, dist=4)\n+\t$(git rev-parse tags/e6) (tag: e6, dist=3)\n+\t$(git rev-parse tags/e3) (tag: e3, dist=3)\n+\t$(git rev-parse tags/e7) (tag: e7, dist=2)\n+\t$(git rev-parse tags/e2) (tag: e2, dist=2)\n+\t$(git rev-parse tags/e8) (tag: e8, dist=1)\n+\t$(git rev-parse tags/e1) (tag: e1, dist=1)\n+\t$(git rev-parse tags/E) (tag: E, dist=0)\n+EOF\n+\n+git rev-list --bisect-all --first-parent E ^F >actual &&\n+\tsort actual >actual.sorted &&\n+\tsort expect >expect.sorted &&\n+\ttest_cmp expect.sorted actual.sorted\n+'\n+\n test_done\n-- \n2.24.0.1.gdc0fb1c0fe\n"},{"id":"385491","messageId":"20191105230403.5542-1-jonathantanmy@google.com","threadId":"52190","inReplyTo":"20191105052141.15913-2-workingjubilee@gmail.com","subject":"Re: [PATCH v7 1/1] Implement rev-list --bisect* --first-parent","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-11-05T23:04:03Z","receivedAt":"2019-11-05T23:04:10Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"First of all, thanks for taking a year-old patch and updating it.\nOverall, this looks good. I have some minor comments, but it might be\nbest to wait until someone more experienced with bisect.c takes a look\ntoo.\n\nYour commit message title should be of the form \"<component>: <change>\",\ne.g.:\n\n  rev-list: support --first-parent with --bisect*\n\n> Not all repository maintainers expect every commit to pass tests, only\n> testing the merge commits. Currently bisection assumes every commit is\n> of interest. The highly-requested --bisect --first-parent feature imbues\n> git with the same indifference to minutiae when the option is set, so\n> that it casually riffles through commits, throwing aside mountains of\n> irrelevant data when looking for a breaking change. Further refinement\n> of where breaks occurred can be gained by bisecting over the merge's\n> range.\n\nI would be much more laconic (in particular, omitting subjective terms\nlike \"minutiae\" and \"mountains of irrelevant data\"), but perhaps that is\njust a matter of subjective style.\n\n> Note, bisecting on --first-parent becomes part of findall's previously\n> existing pass-through as an \"option state\" flag.\n\nI don't understand this part.\n\n> In order to limit possible obfuscation of bisect operations resulting\n> from the addition of new flags, some extra documentation was folded in\n> to the patch.\n\nWhat is being obfuscated, and what extra documentation was \"folded\"?\n\nAlso, clarify in the commit message somewhere that this commit does not\nchange the behavior of \"git bisect\".\n\nAs for the diff, besides my comments below, a change in the user-facing\ndocumentation of \"rev-list\" is needed, since --bisect and --first-parent\nnow work together.\n\n> -static int count_interesting_parents(struct commit *commit)\n> +static int count_interesting_parents(struct commit *commit, unsigned bisect_flags)\n>  {\n>  \tstruct commit_list *p;\n>  \tint count;\n>  \n>  \tfor (count = 0, p = commit->parents; p; p = p->next) {\n> -\t\tif (p->item->object.flags & UNINTERESTING)\n> -\t\t\tcontinue;\n> -\t\tcount++;\n> +\t\tif (!(p->item->object.flags & UNINTERESTING))\n> +\t\t\tcount++;\n> +\t\tif (bisect_flags & BISECT_FIRST_PARENT)\n> +\t\t\tbreak;\n>  \t}\n>  \treturn count;\n>  }\n\n(Note that I'm writing my thoughts as I go along to aid future\nreviewers, and to show the author (you) how I'm understanding the\npatch.)\n\nWe only take into account the first parent - straightforward enough.\nI'll have to see how this function is used to ensure that this change is\ncorrect.\n\n>  static void show_list(const char *debug, int counted, int nr,\n> -\t\t      struct commit_list *list)\n> +\t\t      struct commit_list *list, unsigned bisect_flags)\n>  {\n>  \tstruct commit_list *p;\n\nWhat is the purpose of this change? bisect_flags is never used anywhere\nin show_list().\n\n> @@ -271,13 +274,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list,\n>  \t\tunsigned flags = commit->object.flags;\n>  \n>  \t\t*commit_weight_at(&commit_weight, p->item) = &weights[n++];\n> -\t\tswitch (count_interesting_parents(commit)) {\n> +\t\tswitch (count_interesting_parents(commit, bisect_flags)) {\n>  \t\tcase 0:\n\n[snip]\n\nNot shown, but this is an iteration over all commits in the parameter\n\"list\". I'll quote the entire loop:\n\n>         for (n = 0, p = list; p; p = p->next) {\n>                 struct commit *commit = p->item;\n>                 unsigned flags = commit->object.flags;\n> \n>                 *commit_weight_at(&commit_weight, p->item) = &weights[n++];\n>                 switch (count_interesting_parents(commit, bisect_flags)) {\n>                 case 0:\n>                         if (!(flags & TREESAME)) {\n>                                 weight_set(p, 1);\n>                                 counted++;\n>                                 show_list(\"bisection 2 count one\",\n>                                           counted, nr, list, bisect_flags);\n>                         }\n>                         /*\n>                          * otherwise, it is known not to reach any\n>                          * tree-changing commit and gets weight 0.\n>                          */\n>                         break;\n>                 case 1:\n>                         weight_set(p, -1);\n>                         break;\n>                 default:\n>                         weight_set(p, -2);\n>                         break;\n>                 }\n>         }\n\nLooks like count_interesting_parents() is correct. After this loop, all\ncommits have weight 1, 0 (weights is xcalloc'ed by the caller), -1, or\n-2 (impossible when BISECT_FIRST_PARENT). We'll have to observe how\nsubsequent code treats the weights 1, 0, and -1.\n\n> -\tshow_list(\"bisection 2 count_distance\", counted, nr, list);\n> +\t/* should match bisection 2 initialize when BISECT_FIRST_PARENT */\n> +\tshow_list(\"bisection 2 count_distance\", counted, nr, list, bisect_flags);\n\nBesides the review comment I made about show_list() earlier, this /* */\ncomment is also unnecessary.\n\n> @@ -333,6 +337,17 @@ static struct commit_list *do_find_bisection(struct commit_list *list,\n>  \t\t\tif (0 <= weight(p))\n>  \t\t\t\tcontinue;\n>  \t\t\tfor (q = p->item->parents; q; q = q->next) {\n> +\t\t\t\t/*\n> +\t\t\t\t * first_parent can skip parent nodes, but only when\n> +\t\t\t\t * confirmed as being of no interest.\n> +\t\t\t\t */\n> +\t\t\t\tif (first_parent) {\n> +\t\t\t\t\tif ((q->item->object.flags & UNINTERESTING) ||\n> +\t\t\t\t\t\t(weight(q) < 0)) {\n> +\t\t\t\t\t\tq = NULL;\n> +\t\t\t\t\t}\n> +\t\t\t\t\tbreak;\n> +\t\t\t\t}\n>  \t\t\t\tif (q->item->object.flags & UNINTERESTING)\n>  \t\t\t\t\tcontinue;\n>  \t\t\t\tif (0 <= weight(q))\n\nThis is also in a loop. As can be seen at the top of the diff (\"if (0 <=\nweight(p))\"), this only operates on commits with negative weights.\n\nOriginally, the inner loop advances until a non-UNINTERESTING parent\nwith a non-negative weight. If no such parent is found, at the end of\nthe loop, q is NULL. The added code effectively replicates what's going\non, but ignoring any parents after the first.\n\nA previous reviewer [1] wanted an explanation for this part, so thanks\nfor attempting to do that. But I don't understand the explanation -\nfirstly, it is not a question of \"can\" (optional) but of \"will\"\n(mandatory), and it is not only UNINTERESTING that determines skipping,\nbut weight as well.\n\nI would write the entire section like this (remember to wrap the lines):\n\n\tif (first_parent) {\n\t\tq = p->item->parents;\n\t\tif (q && ((q->item->object.flags & UNINTERESTING) || weight(q) < 0))\n\t\t\tq = NULL;\n\t} else {\n\t\t/*\n\t\t * Find an interesting parent with non-negative weight.\n\t\t */\n\t\tfor (...) {\n\t\t}\n\t}\n\nLooking at the rest of do_find_bisection():\n\n- I don't see any other parts that would be affected by only calculating\n  weights based on the first parent, so that's fine.\n\n- There are some early returns that assume that \"list\" is generated by\n  iterating only over first parents. do_find_bisection() is called only\n  by find_bisection(), and the latter is called only by cmd_rev_list()\n  and bisect_next_all(). The former is fine, but I will discuss the\n  latter later.\n\n- I do see some unclear parts (in particular, counter might not reach nr\n  if any of the weights are 0 and if the \"weight_set(p, weight(q));\"\n  line is reached, potentially resulting in an infinite loop) but that\n  is unrelated to this patch, so don't worry about it.\n\n[1] https://public-inbox.org/git/nycvar.QRO.7.76.6.1808281512240.73@tvgsbejvaqbjf.bet/\n\n> @@ -964,7 +981,12 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n>  \n>  \tbisect_common(&revs);\n>  \n> -\tfind_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr);\n> +\tif (skipped_revs.nr)\n> +\t\tbisect_flags |= BISECT_FIND_ALL;\n> +\tif (revs.first_parent_only)\n> +\t\tbisect_flags |= BISECT_FIRST_PARENT;\n> +\n> +\tfind_bisection(&revs.commits, &reaches, &all, bisect_flags);\n>  \trevs.commits = managed_skipped(revs.commits, &tried);\n>  \n>  \tif (!revs.commits) {\n\nI don't see how revs.first_parent_only is ever set in this function. If\nit's never set, undo this change, since this code is never executed.\n\n> +/*\n> + * Coordinates a bisection by examining input made available so far,\n> + * setting up internal variables, and then bisecting with them.\n> + * no_checkout directs this to only update BISECT_HEAD refs.\n> + *\n> + * Exit code 10 on successful bisection, so caller should exit with 0.\n> + * Exit code 4 when no commits were found to bisect through.\n> + * Exit code 1 MAY result from skipping the commit it would report.\n> + *\n> + * Otherwise, returns a call to command handlers which choose an exit.\n> + */\n>  int bisect_next_all(struct repository *r,\n>  \t\t    const char *prefix,\n>  \t\t    int no_checkout);\n\nI haven't looked at this function in detail, so I can't verify that this\ncomment is correct. But in any case, if you end up not making any change\nto bisect_next_all(), I think it's best to leave this change out as\nwell.\n\n> diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\n> index b8cf82349b..95949e4ff1 100755\n> --- a/t/t6000-rev-list-misc.sh\n> +++ b/t/t6000-rev-list-misc.sh\n> @@ -122,8 +122,8 @@ test_expect_success 'rev-list can negate index objects' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> -test_expect_success '--bisect and --first-parent can not be combined' '\n> -\ttest_must_fail git rev-list --bisect --first-parent HEAD\n> +test_expect_success '--bisect and --first-parent CAN be combined' '\n> +\tgit rev-list --bisect --first-parent HEAD\n>  '\n>  \n\nI think this test can just be deleted. It is tested in t6002.\n\n> diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh\n> index a661408038..6caf2af650 100755\n> --- a/t/t6002-rev-list-bisect.sh\n> +++ b/t/t6002-rev-list-bisect.sh\n> @@ -263,4 +263,58 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' '\n>  \ttest_cmp expect.sorted actual.sorted\n>  '\n>  \n> +# --first-parent tests\n> +\n> +# --bisect --first-parent should pluck out the middle.\n> +printf \"%s\\n\" e4 |\n> +test_output_expect_success \"--bisect --first-parent\" '\n> +\tgit rev-list --bisect --first-parent E ^F\n> +'\n> +\n> +printf \"%s\\n\" E e1 e2 e3 e4 e5 e6 e7 e8 |\n> +test_output_expect_success \"--first-parent\" '\n> +\tgit rev-list --first-parent E ^F\n> +'\n> +\n> +test_output_expect_success '--bisect-vars --first-parent' '\n> +\tgit rev-list --bisect-vars --first-parent E ^F\n> +' <<-EOF\n> +\tbisect_rev='e5'\n> +\tbisect_nr=4\n> +\tbisect_good=4\n> +\tbisect_bad=3\n> +\tbisect_all=9\n> +\tbisect_steps=2\n> +EOF\n\nLooks good, except for the middle test - that should already be working\neven before the current patch, right? If there's a reason for including\nit anyway, mention it in the commit message.\n\n> +test_expect_success '--bisect-all --first-parent returns correct order' '\n> +\tgit rev-list --bisect-all --first-parent E ^F >actual &&\n> +\n> +\t# Make sure the entries are sorted in the dist order\n> +\tsed -e \"s/.*dist=\\([0-9]\\).*/\\1/\" actual >actual.dists &&\n> +\tsort -r -n actual.dists >actual.dists.sorted &&\n> +\ttest_cmp actual.dists.sorted actual.dists\n> +'\n> +\n> +# NEEDSWORK: this test could afford being hardened against other\n> +# changes in the same file.\n> +test_expect_success '--bisect-all --first-parent compares correctly' '\n> +\tcat >expect <<-EOF &&\n> +\t$(git rev-parse tags/e5) (tag: e5, dist=4)\n> +\t$(git rev-parse tags/e4) (tag: e4, dist=4)\n> +\t$(git rev-parse tags/e6) (tag: e6, dist=3)\n> +\t$(git rev-parse tags/e3) (tag: e3, dist=3)\n> +\t$(git rev-parse tags/e7) (tag: e7, dist=2)\n> +\t$(git rev-parse tags/e2) (tag: e2, dist=2)\n> +\t$(git rev-parse tags/e8) (tag: e8, dist=1)\n> +\t$(git rev-parse tags/e1) (tag: e1, dist=1)\n> +\t$(git rev-parse tags/E) (tag: E, dist=0)\n> +EOF\n> +\n> +git rev-list --bisect-all --first-parent E ^F >actual &&\n> +\tsort actual >actual.sorted &&\n> +\tsort expect >expect.sorted &&\n> +\ttest_cmp expect.sorted actual.sorted\n> +'\n\nI think these 2 tests can be combined, since the latter also checks the\ndists. Also, correct the indentation of the latter test.\n"},{"id":"385519","messageId":"xmqqtv7hvi6r.fsf@gitster-ct.c.googlers.com","threadId":"52190","inReplyTo":"20191105230403.5542-1-jonathantanmy@google.com","subject":"Re: [PATCH v7 1/1] Implement rev-list --bisect* --first-parent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-06T02:42:52Z","receivedAt":"2019-11-06T02:42:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> Your commit message title should be of the form \"<component>: <change>\",\n> e.g.:\n>\n>   rev-list: support --first-parent with --bisect*\n\nGood suggestion.\n\n> I would be much more laconic (in particular, omitting subjective terms\n> like \"minutiae\" and \"mountains of irrelevant data\"), but perhaps that is\n> just a matter of subjective style.\n\nFWIW, I had the same reaction.  That part of the message was too\nnoisy without adding much actual value.\n\n>> Note, bisecting on --first-parent becomes part of findall's previously\n>> existing pass-through as an \"option state\" flag.\n>\n> I don't understand this part.\n\nMe neither.\n\n> Also, clarify in the commit message somewhere that this commit does not\n> change the behavior of \"git bisect\".\n\ns/\\.$/ when used without the \"--first-parent\" option&/; you mean?\n\n> As for the diff, besides my comments below, a change in the user-facing\n> documentation of \"rev-list\" is needed, since --bisect and --first-parent\n> now work together.\n\nTrue.  I too am, like you are, happy to see that these two options\nmade to work well together.\n\nThanks, both, for the patch and useful comments.  My own review on\nit may take a bit more time.\n\n"},{"id":"385584","messageId":"nycvar.QRO.7.76.6.1911061228340.46@tvgsbejvaqbjf.bet","threadId":"52190","inReplyTo":"xmqqtv7hvi6r.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v7 1/1] Implement rev-list --bisect* --first-parent","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-06T11:30:19Z","receivedAt":"2019-11-06T11:30:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 6 Nov 2019, Junio C Hamano wrote:\n\n> Jonathan Tan <jonathantanmy@google.com> writes:\n>\n> > Your commit message title should be of the form \"<component>: <change>\",\n> > e.g.:\n> >\n> >   rev-list: support --first-parent with --bisect*\n>\n> Good suggestion.\n>\n> > I would be much more laconic (in particular, omitting subjective terms\n> > like \"minutiae\" and \"mountains of irrelevant data\"), but perhaps that is\n> > just a matter of subjective style.\n>\n> FWIW, I had the same reaction.  That part of the message was too\n> noisy without adding much actual value.\n>\n> >> Note, bisecting on --first-parent becomes part of findall's previously\n> >> existing pass-through as an \"option state\" flag.\n> >\n> > I don't understand this part.\n>\n> Me neither.\n>\n> > Also, clarify in the commit message somewhere that this commit does not\n> > change the behavior of \"git bisect\".\n>\n> s/\\.$/ when used without the \"--first-parent\" option&/; you mean?\n>\n> > As for the diff, besides my comments below, a change in the user-facing\n> > documentation of \"rev-list\" is needed, since --bisect and --first-parent\n> > now work together.\n>\n> True.  I too am, like you are, happy to see that these two options\n> made to work well together.\n>\n> Thanks, both, for the patch and useful comments.  My own review on\n> it may take a bit more time.\n\nWhile I sadly won't have time to review this patch, let me first state\nthat I am very excited that you revived this stalled patch. Thank you!\n\nIn addition to Jonathan's comments, I would like to add another one: I\nwould have loved for this patch to be split in two, the first one\nintroducing the `bisect_flags` and using it with `BISECT_FIND_ALL`\ninstead of doing the `find_all` thing, the second one building the\nfirst-parent feature on top.\n\nThanks,\nDscho\n"},{"id":"385625","messageId":"20191106213609.57464-1-jonathantanmy@google.com","threadId":"52190","inReplyTo":"xmqqtv7hvi6r.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v7 1/1] Implement rev-list --bisect* --first-parent","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-11-06T21:36:09Z","receivedAt":"2019-11-06T21:36:16Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > Also, clarify in the commit message somewhere that this commit does not\n> > change the behavior of \"git bisect\".\n> \n> s/\\.$/ when used without the \"--first-parent\" option&/; you mean?\n\nAs far as I know, \"git bisect\" doesn't support --first-parent, whether\nbefore or after this patch.\n\nAt first I thought that this patch also teaches \"git bisect\" to support\n\"--first-parent\", but that is not the case. Only \"git rev-list\" learns\nto make \"--bisect\" work with \"--first-parent\". So I wanted the\nclarification. (But if you think that the clarification is unnecessary,\nthat's fine too.)\n"},{"id":"385647","messageId":"xmqqy2wss6j6.fsf@gitster-ct.c.googlers.com","threadId":"52190","inReplyTo":"20191106213609.57464-1-jonathantanmy@google.com","subject":"Re: [PATCH v7 1/1] Implement rev-list --bisect* --first-parent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-07T03:35:09Z","receivedAt":"2019-11-07T03:35:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n>> > Also, clarify in the commit message somewhere that this commit does not\n>> > change the behavior of \"git bisect\".\n>> \n>> s/\\.$/ when used without the \"--first-parent\" option&/; you mean?\n>\n> As far as I know, \"git bisect\" doesn't support --first-parent, whether\n> before or after this patch.\n>\n> At first I thought that this patch also teaches \"git bisect\" to support\n> \"--first-parent\", but that is not the case.\n\nAh, I was not paying enough attention.  So this is only the first\npart of the whole solution that does not have much end-user visible\nbenefit yet.\n\nWith \"yet\" at the end of what you wrote, I do agree with you that\nsuch a clarification would be necessary.\n\nThanks.\n"},{"id":"385652","messageId":"CAPNHn3oCOQRriDLmLFag0DWgvq2t3TLMiu25HyBKcuBuwLgb2g@mail.gmail.com","threadId":"52190","inReplyTo":"20191106213609.57464-1-jonathantanmy@google.com","subject":"Re: [PATCH v7 1/1] Implement rev-list --bisect* --first-parent","fromName":"Jubilee Young","fromEmail":"workingjubilee@gmail.com","sentAt":"2019-11-07T05:07:06Z","receivedAt":"2019-11-07T05:07:26Z","isPatch":true,"sender":{"key":"workingjubilee@gmail.com","avatar":null},"body":"Hi! Thanks for all the feedback.\n\nOn Wed, Nov 6, 2019 at 1:36 PM Jonathan Tan <jonathantanmy@google.com>\nwrote:\n>\n> > > Also, clarify in the commit message somewhere that this commit does\nnot\n> > > change the behavior of \"git bisect\".\n> >\n> > s/\\.$/ when used without the \"--first-parent\" option&/; you mean?\n>\n> As far as I know, \"git bisect\" doesn't support --first-parent, whether\n> before or after this patch.\n\nCorrect, it still provides the usual \"usage: git bisect ...\" error.\nThis provides plumbing but no porcelain right now.\n\n> At first I thought that this patch also teaches \"git bisect\" to support\n> \"--first-parent\", but that is not the case. Only \"git rev-list\" learns\n> to make \"--bisect\" work with \"--first-parent\". So I wanted the\n> clarification. (But if you think that the clarification is unnecessary,\n> that's fine too.)\n\nI had been trying to deduce the best approach for writing the commit\nmessage but in the end I retained a hunch that I didn't yet know enough\nto write the best one so this conversation is quite illuminating re:\ncommit style.\n\nIf it's even slightly confusing I would like to clarify, since bisect.c\nand git bisect are... very different!\n\nI think Dscho's recommendation also will result in the patch overall\nbeing far more lucid.\n"},{"id":"385708","messageId":"CAPNHn3qj7v=xu1ogG4q9NrHvp1zfZFvUWQKJqf0DJcavxgsz6Q@mail.gmail.com","threadId":"52190","inReplyTo":"20191105230403.5542-1-jonathantanmy@google.com","subject":"Re: [PATCH v7 1/1] Implement rev-list --bisect* --first-parent","fromName":"Jubilee Young","fromEmail":"workingjubilee@gmail.com","sentAt":"2019-11-07T16:00:00Z","receivedAt":"2019-11-07T16:00:43Z","isPatch":true,"sender":{"key":"workingjubilee@gmail.com","avatar":null},"body":"On Tue, Nov 5, 2019 at 3:04 PM Jonathan Tan <jonathantanmy@google.com> wrote:\n>\n> First of all, thanks for taking a year-old patch and updating it.\n> Overall, this looks good. I have some minor comments, but it might be\n> best to wait until someone more experienced with bisect.c takes a look\n> too.\n>\n> Your commit message title should be of the form \"<component>: <change>\",\n> e.g.:\n>\n>   rev-list: support --first-parent with --bisect*\n>\n> > Not all repository maintainers expect every commit to pass tests, only\n> > testing the merge commits. Currently bisection assumes every commit is\n> > of interest. The highly-requested --bisect --first-parent feature imbues\n> > git with the same indifference to minutiae when the option is set, so\n> > that it casually riffles through commits, throwing aside mountains of\n> > irrelevant data when looking for a breaking change. Further refinement\n> > of where breaks occurred can be gained by bisecting over the merge's\n> > range.\n>\n> I would be much more laconic (in particular, omitting subjective terms\n> like \"minutiae\" and \"mountains of irrelevant data\"), but perhaps that is\n> just a matter of subjective style.\n>\n> > Note, bisecting on --first-parent becomes part of findall's previously\n> > existing pass-through as an \"option state\" flag.\n>\n> I don't understand this part.\n>\n> > In order to limit possible obfuscation of bisect operations resulting\n> > from the addition of new flags, some extra documentation was folded in\n> > to the patch.\n>\n> What is being obfuscated, and what extra documentation was \"folded\"?\n>\n> Also, clarify in the commit message somewhere that this commit does not\n> change the behavior of \"git bisect\".\n>\n> As for the diff, besides my comments below, a change in the user-facing\n> documentation of \"rev-list\" is needed, since --bisect and --first-parent\n> now work together.\n\nWill do.\n\n>\n> > -static int count_interesting_parents(struct commit *commit)\n> > +static int count_interesting_parents(struct commit *commit, unsigned bisect_flags)\n> >  {\n> >       struct commit_list *p;\n> >       int count;\n> >\n> >       for (count = 0, p = commit->parents; p; p = p->next) {\n> > -             if (p->item->object.flags & UNINTERESTING)\n> > -                     continue;\n> > -             count++;\n> > +             if (!(p->item->object.flags & UNINTERESTING))\n> > +                     count++;\n> > +             if (bisect_flags & BISECT_FIRST_PARENT)\n> > +                     break;\n> >       }\n> >       return count;\n> >  }\n>\n> (Note that I'm writing my thoughts as I go along to aid future\n> reviewers, and to show the author (you) how I'm understanding the\n> patch.)\n>\n> We only take into account the first parent - straightforward enough.\n> I'll have to see how this function is used to ensure that this change is\n> correct.\n>\n> >  static void show_list(const char *debug, int counted, int nr,\n> > -                   struct commit_list *list)\n> > +                   struct commit_list *list, unsigned bisect_flags)\n> >  {\n> >       struct commit_list *p;\n>\n> What is the purpose of this change? bisect_flags is never used anywhere\n> in show_list().\n\nInsufficiently cleaned-up change cruft! Thanks for catching it.\n\n> This is also in a loop. As can be seen at the top of the diff (\"if (0 <=\n> weight(p))\"), this only operates on commits with negative weights.\n>\n> Originally, the inner loop advances until a non-UNINTERESTING parent\n> with a non-negative weight. If no such parent is found, at the end of\n> the loop, q is NULL. The added code effectively replicates what's going\n> on, but ignoring any parents after the first.\n>\n> A previous reviewer [1] wanted an explanation for this part, so thanks\n> for attempting to do that. But I don't understand the explanation -\n> firstly, it is not a question of \"can\" (optional) but of \"will\"\n> (mandatory), and it is not only UNINTERESTING that determines skipping,\n> but weight as well.\n>\n> I would write the entire section like this (remember to wrap the lines):\n>\n>         if (first_parent) {\n>                 q = p->item->parents;\n>                 if (q && ((q->item->object.flags & UNINTERESTING) || weight(q) < 0))\n>                         q = NULL;\n>         } else {\n>                 /*\n>                  * Find an interesting parent with non-negative weight.\n>                  */\n>                 for (...) {\n>                 }\n>         }\n>\n\n\"uninteresting\" was meant in the colloquial sense rather than the\nCONSTANT, but fair, it's probably just confusing.\n\n> Looking at the rest of do_find_bisection():\n>\n> - I don't see any other parts that would be affected by only calculating\n>   weights based on the first parent, so that's fine.\n>\n> - There are some early returns that assume that \"list\" is generated by\n>   iterating only over first parents. do_find_bisection() is called only\n>   by find_bisection(), and the latter is called only by cmd_rev_list()\n>   and bisect_next_all(). The former is fine, but I will discuss the\n>   latter later.\n>\n> - I do see some unclear parts (in particular, counter might not reach nr\n>   if any of the weights are 0 and if the \"weight_set(p, weight(q));\"\n>   line is reached, potentially resulting in an infinite loop) but that\n>   is unrelated to this patch, so don't worry about it.\n>\n> [1] https://public-inbox.org/git/nycvar.QRO.7.76.6.1808281512240.73@tvgsbejvaqbjf.bet/\n>\n> > @@ -964,7 +981,12 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n> >\n> >       bisect_common(&revs);\n> >\n> > -     find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr);\n> > +     if (skipped_revs.nr)\n> > +             bisect_flags |= BISECT_FIND_ALL;\n> > +     if (revs.first_parent_only)\n> > +             bisect_flags |= BISECT_FIRST_PARENT;\n> > +\n> > +     find_bisection(&revs.commits, &reaches, &all, bisect_flags);\n> >       revs.commits = managed_skipped(revs.commits, &tried);\n> >\n> >       if (!revs.commits) {\n>\n> I don't see how revs.first_parent_only is ever set in this function. If\n> it's never set, undo this change, since this code is never executed.\n\nIn this function, we call bisect_rev_setup() using the revs struct we\nmade, which then calls setup_revisions() on the revs, which appears to\ncall handle_revision_opt() with that struct again,which finally is\nallowed to set revs->first_parent_only = 1; in revision.c.\n\nSo unless I am horribly misreading something, we do set it.\n\n> > diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\n> > index b8cf82349b..95949e4ff1 100755\n> > --- a/t/t6000-rev-list-misc.sh\n> > +++ b/t/t6000-rev-list-misc.sh\n> > @@ -122,8 +122,8 @@ test_expect_success 'rev-list can negate index objects' '\n> >       test_cmp expect actual\n> >  '\n> >\n> > -test_expect_success '--bisect and --first-parent can not be combined' '\n> > -     test_must_fail git rev-list --bisect --first-parent HEAD\n> > +test_expect_success '--bisect and --first-parent CAN be combined' '\n> > +     git rev-list --bisect --first-parent HEAD\n> >  '\n> >\n>\n> I think this test can just be deleted. It is tested in t6002.\n\nSure, I'll drop it.\n\n> > diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh\n> > index a661408038..6caf2af650 100755\n> > --- a/t/t6002-rev-list-bisect.sh\n> > +++ b/t/t6002-rev-list-bisect.sh\n> > @@ -263,4 +263,58 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' '\n> >       test_cmp expect.sorted actual.sorted\n> >  '\n> >\n> > +# --first-parent tests\n> > +\n> > +# --bisect --first-parent should pluck out the middle.\n> > +printf \"%s\\n\" e4 |\n> > +test_output_expect_success \"--bisect --first-parent\" '\n> > +     git rev-list --bisect --first-parent E ^F\n> > +'\n> > +\n> > +printf \"%s\\n\" E e1 e2 e3 e4 e5 e6 e7 e8 |\n> > +test_output_expect_success \"--first-parent\" '\n> > +     git rev-list --first-parent E ^F\n> > +'\n> > +\n> > +test_output_expect_success '--bisect-vars --first-parent' '\n> > +     git rev-list --bisect-vars --first-parent E ^F\n> > +' <<-EOF\n> > +     bisect_rev='e5'\n> > +     bisect_nr=4\n> > +     bisect_good=4\n> > +     bisect_bad=3\n> > +     bisect_all=9\n> > +     bisect_steps=2\n> > +EOF\n>\n> Looks good, except for the middle test - that should already be working\n> even before the current patch, right? If there's a reason for including\n> it anyway, mention it in the commit message.\n\nAlso cutting, I think.\n\n> > +test_expect_success '--bisect-all --first-parent returns correct order' '\n> > +     git rev-list --bisect-all --first-parent E ^F >actual &&\n> > +\n> > +     # Make sure the entries are sorted in the dist order\n> > +     sed -e \"s/.*dist=\\([0-9]\\).*/\\1/\" actual >actual.dists &&\n> > +     sort -r -n actual.dists >actual.dists.sorted &&\n> > +     test_cmp actual.dists.sorted actual.dists\n> > +'\n> > +\n> > +# NEEDSWORK: this test could afford being hardened against other\n> > +# changes in the same file.\n> > +test_expect_success '--bisect-all --first-parent compares correctly' '\n> > +     cat >expect <<-EOF &&\n> > +     $(git rev-parse tags/e5) (tag: e5, dist=4)\n> > +     $(git rev-parse tags/e4) (tag: e4, dist=4)\n> > +     $(git rev-parse tags/e6) (tag: e6, dist=3)\n> > +     $(git rev-parse tags/e3) (tag: e3, dist=3)\n> > +     $(git rev-parse tags/e7) (tag: e7, dist=2)\n> > +     $(git rev-parse tags/e2) (tag: e2, dist=2)\n> > +     $(git rev-parse tags/e8) (tag: e8, dist=1)\n> > +     $(git rev-parse tags/e1) (tag: e1, dist=1)\n> > +     $(git rev-parse tags/E) (tag: E, dist=0)\n> > +EOF\n> > +\n> > +git rev-list --bisect-all --first-parent E ^F >actual &&\n> > +     sort actual >actual.sorted &&\n> > +     sort expect >expect.sorted &&\n> > +     test_cmp expect.sorted actual.sorted\n> > +'\n>\n> I think these 2 tests can be combined, since the latter also checks the\n> dists. Also, correct the indentation of the latter test.\n\nI understand they are similar tests, but... Is there a tangible\nreason for combining them? Especially when their logic can\nlive and breathe completely separately, compacting tests\nreduces the resolution of the information we can extract from failure.\n\nI would rather simply drop one and preserve 1 test = 1 data point.\n"},{"id":"385730","messageId":"20191107185630.253388-1-jonathantanmy@google.com","threadId":"52190","inReplyTo":"CAPNHn3qj7v=xu1ogG4q9NrHvp1zfZFvUWQKJqf0DJcavxgsz6Q@mail.gmail.com","subject":"Re: [PATCH v7 1/1] Implement rev-list --bisect* --first-parent","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-11-07T18:56:30Z","receivedAt":"2019-11-07T18:56:36Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > > @@ -964,7 +981,12 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)\n> > >\n> > >       bisect_common(&revs);\n> > >\n> > > -     find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr);\n> > > +     if (skipped_revs.nr)\n> > > +             bisect_flags |= BISECT_FIND_ALL;\n> > > +     if (revs.first_parent_only)\n> > > +             bisect_flags |= BISECT_FIRST_PARENT;\n> > > +\n> > > +     find_bisection(&revs.commits, &reaches, &all, bisect_flags);\n> > >       revs.commits = managed_skipped(revs.commits, &tried);\n> > >\n> > >       if (!revs.commits) {\n> >\n> > I don't see how revs.first_parent_only is ever set in this function. If\n> > it's never set, undo this change, since this code is never executed.\n> \n> In this function, \n\n(1)\n> we call bisect_rev_setup() using the revs struct we\n> made,\n\n(2)\n> which then calls setup_revisions() on the revs,\n\n(3)\n> which appears to\n> call handle_revision_opt() with that struct again,\n\n(4)\n> which finally is\n> allowed to set revs->first_parent_only = 1; in revision.c.\n> \n> So unless I am horribly misreading something, we do set it.\n\nThanks for performing this analysis. Working backwards:\n\n(4) handle_revision_opt() in revision.c sets revs->first_parent_only\nonly if argv[0] is \"--first-parent\".\n\n(3) setup_revisions() calls handle_revision_opt() in a loop over its\nargv parameter.\n\n(2) bisect_rev_setup() initializes rev_argv (with ARGV_ARRAY_INIT, a\nblank array). Then, it pushes \"bisect_rev_setup\", an OID in\n\"bad_format\", some OIDs in \"good_format\", and possibly some paths. Then\nit calls setup_revisions() with rev_argv as the argv parameter. Notice\nthat there is no \"--first-parent\" sent at all.\n\n> > > +test_expect_success '--bisect-all --first-parent returns correct order' '\n> > > +     git rev-list --bisect-all --first-parent E ^F >actual &&\n> > > +\n> > > +     # Make sure the entries are sorted in the dist order\n> > > +     sed -e \"s/.*dist=\\([0-9]\\).*/\\1/\" actual >actual.dists &&\n> > > +     sort -r -n actual.dists >actual.dists.sorted &&\n> > > +     test_cmp actual.dists.sorted actual.dists\n> > > +'\n> > > +\n> > > +# NEEDSWORK: this test could afford being hardened against other\n> > > +# changes in the same file.\n> > > +test_expect_success '--bisect-all --first-parent compares correctly' '\n> > > +     cat >expect <<-EOF &&\n> > > +     $(git rev-parse tags/e5) (tag: e5, dist=4)\n> > > +     $(git rev-parse tags/e4) (tag: e4, dist=4)\n> > > +     $(git rev-parse tags/e6) (tag: e6, dist=3)\n> > > +     $(git rev-parse tags/e3) (tag: e3, dist=3)\n> > > +     $(git rev-parse tags/e7) (tag: e7, dist=2)\n> > > +     $(git rev-parse tags/e2) (tag: e2, dist=2)\n> > > +     $(git rev-parse tags/e8) (tag: e8, dist=1)\n> > > +     $(git rev-parse tags/e1) (tag: e1, dist=1)\n> > > +     $(git rev-parse tags/E) (tag: E, dist=0)\n> > > +EOF\n> > > +\n> > > +git rev-list --bisect-all --first-parent E ^F >actual &&\n> > > +     sort actual >actual.sorted &&\n> > > +     sort expect >expect.sorted &&\n> > > +     test_cmp expect.sorted actual.sorted\n> > > +'\n> >\n> > I think these 2 tests can be combined, since the latter also checks the\n> > dists. Also, correct the indentation of the latter test.\n> \n> I understand they are similar tests, but... Is there a tangible\n> reason for combining them? Especially when their logic can\n> live and breathe completely separately, compacting tests\n> reduces the resolution of the information we can extract from failure.\n> \n> I would rather simply drop one and preserve 1 test = 1 data point.\n\nIn general, I agree that 1 test should represent 1 data point, and the\nreason for that being (as you said) the resolution of the information we\ncan extract from failure. But here, the resolution is diminished if we\ndon't combine the tests. Here, if either test fails, because of the\npostprocessing we've had to do on the output, we lose signal. But if we\nhad the combined test, we wouldn't.\n"}]}