{"thread":{"id":"49235","subject":"[PATCH v6] Implement --first-parent for git rev-list --bisect","startedAt":"2018-08-28T12:33:13Z","lastAt":"2018-09-04T19:32:05Z","messageCount":12,"participants":["Tiago Botelho","Johannes Schindelin","Junio C Hamano","Duy Nguyen","Nguyễn Thái Ngọc Duy","Christian Couder"],"isPatch":true,"patchVersion":6,"patchTotal":null},"messages":[{"id":"356684","messageId":"20180828123234.44582-1-tiagonbotelho@hotmail.com","threadId":"49235","inReplyTo":null,"subject":"[PATCH v6] Implement --first-parent for git rev-list --bisect","fromName":"Tiago Botelho","fromEmail":"tiagonbotelho@gmail.com","sentAt":"2018-08-28T12:32:34Z","receivedAt":"2018-08-28T12:33:13Z","isPatch":true,"sender":{"key":"tiagonbotelho@gmail.com","avatar":"https://gravatar.com/avatar/263ea75fe0d4aa4ac14fa9dd08b82955689b6769cf5d33599914ca4321a4597b?d=mp&s=160"},"body":"This will enable users to implement bisecting on first parents\nwhich can be useful for when the commits from a feature branch\nthat we want to merge are not always tested.\n\nSigned-off-by: Tiago Botelho <tiagonbotelho@hotmail.com>\n---\n\nThis patch refactors **only** the tests according to the suggestions made by Junio in v5\nmore specifically `https://public-inbox.org/git/xmqqa7rhi40f.fsf@gitster-ct.c.googlers.com/`.\n\nThe discussion between Junio, Christian and Johannes got pretty confusing in the sense\nthat I am unsure if this is the actual problem I am supposed to solve in order to get the\npatch accepted, if it is not I will keep iterating on it until it is good enough to be\nmerged\n\n bisect.c                   | 43 ++++++++++++++++++++++------------\n bisect.h                   |  1 +\n builtin/rev-list.c         |  3 +++\n revision.c                 |  3 ---\n t/t6002-rev-list-bisect.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 90 insertions(+), 18 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 4eafc8262..cb80f29c5 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -82,15 +82,16 @@ static inline void weight_set(struct commit_list *elem, int weight)\n \t*((int*)(elem->item->util)) = 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    count++;\n+\t\tif (bisect_flags & BISECT_FIRST_PARENT)\n+\t\t\tbreak;\n \t}\n \treturn count;\n }\n@@ -117,10 +118,10 @@ static inline int halfway(struct commit_list *p, int nr)\n }\n \n #if !DEBUG_BISECT\n-#define show_list(a,b,c,d) do { ; } while (0)\n+#define show_list(a,b,c,d,e) do { ; } while (0)\n #else\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@@ -146,10 +147,14 @@ static void show_list(const char *debug, int counted, int nr,\n \t\telse\n \t\t\tfprintf(stderr, \"---\");\n \t\tfprintf(stderr, \" %.*s\", 8, oid_to_hex(&commit->object.oid));\n-\t\tfor (pp = commit->parents; pp; pp = pp->next)\n+\t\tfor (pp = commit->parents; pp; pp = pp->next) {\n \t\t\tfprintf(stderr, \" %.*s\", 8,\n \t\t\t\toid_to_hex(&pp->item->object.oid));\n \n+\t\t\tif (bisect_flags & BISECT_FIRST_PARENT)\n+\t\t\t\tbreak;\n+\t\t}\n+\n \t\tsubject_len = find_commit_subject(buf, &subject_start);\n \t\tif (subject_len)\n \t\t\tfprintf(stderr, \" %.*s\", subject_len, subject_start);\n@@ -267,13 +272,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list,\n \t\tunsigned flags = commit->object.flags;\n \n \t\tp->item->util = &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@@ -289,7 +294,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@@ -319,7 +324,7 @@ 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+\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@@ -329,6 +334,11 @@ 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\tif ((bisect_flags & BISECT_FIRST_PARENT)) {\n+\t\t\t\t\tif (weight(q) < 0)\n+\t\t\t\t\t\tq = NULL;\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@@ -346,7 +356,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@@ -357,7 +367,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@@ -372,7 +382,7 @@ void find_bisection(struct commit_list **commit_list, int *reaches,\n \tstruct commit_list *list, *p, *best, *next, *last;\n \tint *weights;\n \n-\tshow_list(\"bisection 2 entry\", 0, 0, *commit_list);\n+\tshow_list(\"bisection 2 entry\", 0, 0, *commit_list, bisect_flags);\n \n \t/*\n \t * Count the number of total and tree-changing items on the\n@@ -395,7 +405,7 @@ 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@@ -962,6 +972,9 @@ int bisect_next_all(const char *prefix, int no_checkout)\n \tif (skipped_revs.nr)\n \t\tbisect_flags |= BISECT_FIND_ALL;\n \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 \ndiff --git a/bisect.h b/bisect.h\nindex 1d40a33ad..e445567c2 100644\n--- a/bisect.h\n+++ b/bisect.h\n@@ -2,6 +2,7 @@\n #define BISECT_H\n \n #define BISECT_FIND_ALL\t\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\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 8752f5bbe..66439e1b3 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -538,6 +538,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tif (bisect_list) {\n \t\tint reaches, all;\n \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)\ndiff --git a/revision.c b/revision.c\nindex 4e0e193e5..b9d063805 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2474,9 +2474,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->expand_tabs_in_log < 0)\n \t\trevs->expand_tabs_in_log = revs->expand_tabs_in_log_default;\n \ndiff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh\nindex a66140803..1bc297de5 100755\n--- a/t/t6002-rev-list-bisect.sh\n+++ b/t/t6002-rev-list-bisect.sh\n@@ -263,4 +263,62 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' '\n \ttest_cmp expect.sorted actual.sorted\n '\n \n+# We generate the following commit graph:\n+#\n+#   B ------ C\n+#  /          \\\n+# A            FX\n+#  \\          /\n+#   D - CC - EX\n+\n+test_expect_success 'setup' '\n+  test_commit A &&\n+  test_commit B &&\n+  test_commit C &&\n+  git reset --hard A &&\n+  test_commit D &&\n+  test_commit CC &&\n+  test_commit EX &&\n+  test_merge FX C\n+'\n+\n+test_output_expect_success \"--bisect --first-parent\" 'git rev-list --bisect --first-parent FX ^A' <<EOF\n+$(git rev-parse CC)\n+EOF\n+\n+test_output_expect_success \"--first-parent\" 'git rev-list --first-parent FX ^A' <<EOF\n+$(git rev-parse FX)\n+$(git rev-parse EX)\n+$(git rev-parse CC)\n+$(git rev-parse D)\n+EOF\n+\n+test_output_expect_success \"--bisect-vars --first-parent\" 'git rev-list --bisect-vars --first-parent FX ^A' <<EOF\n+bisect_rev='$(git rev-parse CC)'\n+bisect_nr=1\n+bisect_good=1\n+bisect_bad=1\n+bisect_all=4\n+bisect_steps=1\n+EOF\n+\n+test_expect_success \"--bisect-all --first-parent\" '\n+cat >expect <<EOF &&\n+$(git rev-parse CC) (dist=2)\n+$(git rev-parse EX) (dist=1)\n+$(git rev-parse D) (dist=1)\n+$(git rev-parse FX) (dist=0)\n+EOF\n+\n+# Make sure we have the same entries, nothing more, nothing less\n+git rev-list --bisect-all --first-parent FX ^A >actual &&\n+  sort actual >actual.sorted &&\n+  sort expect >expect.sorted &&\n+  test_cmp expect.sorted actual.sorted &&\n+  # Make sure the entries are sorted in the dist order\n+  sed -e \"s/.*(dist=\\([1-9]*[0-9]\\)).*/\\1/\" actual >actual.dists &&\n+  sort -r actual.dists >actual.dists.sorted &&\n+  test_cmp actual.dists.sorted actual.dists\n+'\n+\n test_done\n-- \n2.16.3\n\n"},{"id":"356690","messageId":"nycvar.QRO.7.76.6.1808281512240.73@tvgsbejvaqbjf.bet","threadId":"49235","inReplyTo":"20180828123234.44582-1-tiagonbotelho@hotmail.com","subject":"Re: [PATCH v6] Implement --first-parent for git rev-list --bisect","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-08-28T13:21:09Z","receivedAt":"2018-08-28T13:21:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Tiago,\n\nOn Tue, 28 Aug 2018, Tiago Botelho wrote:\n\n> This will enable users to implement bisecting on first parents\n> which can be useful for when the commits from a feature branch\n> that we want to merge are not always tested.\n\nThis message is still lacking the explanation I asked for, namely for the\nlines:\n\n\t@@ -329,6 +334,11 @@ static struct commit_list *do_find_bisection(struct commit_list *list,\n\t \tif (0 <= weight(p))\n\t \t\tcontinue;\n\t \tfor (q = p->item->parents; q; q = q->next) {\n\t+\t\tif ((bisect_flags & BISECT_FIRST_PARENT)) {\n\t+\t\t\tif (weight(q) < 0)\n\t+\t\t\t\tq = NULL;\n\t+\t\t\tbreak;\n\t+\t\t}\n\t \tif (q->item->object.flags & UNINTERESTING)\n\t \t\tcontinue;\n\t \tif (0 <= weight(q))\n\n> Signed-off-by: Tiago Botelho <tiagonbotelho@hotmail.com>\n> ---\n> \n> This patch refactors **only** the tests according to the suggestions made by Junio in v5\n> more specifically `https://public-inbox.org/git/xmqqa7rhi40f.fsf@gitster-ct.c.googlers.com/`.\n> \n> The discussion between Junio, Christian and Johannes got pretty confusing in the sense\n> that I am unsure if this is the actual problem I am supposed to solve in order to get the\n> patch accepted, if it is not I will keep iterating on it until it is good enough to be\n> merged\n\nI would have preferred to reuse the already existing commits generated in\nthe `setup` phase rather than generating yet another batch, and to not\nintroduce an inconsistent way to present a commit graph (compare your\ndiagram with the one in\nhttps://github.com/git/git/blob/v2.18.0/t/t6002-rev-list-bisect.sh#L64-L90\ni.e. *in the same file*), and I mentioned this at least twice before, but\nmaybe it got lost (and I could imagine that Christian's insistence on\npushing the patch through with as little changes as he can get away with\ndoes not help), but at least now the added tests look slightly more\nunderstandable than before (despite funny indentation and too-long lines),\nso I'll stop imitating a broken record at this point.\n\nCiao,\nJohannes\n\n> \n>  bisect.c                   | 43 ++++++++++++++++++++++------------\n>  bisect.h                   |  1 +\n>  builtin/rev-list.c         |  3 +++\n>  revision.c                 |  3 ---\n>  t/t6002-rev-list-bisect.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++\n>  5 files changed, 90 insertions(+), 18 deletions(-)\n> \n> diff --git a/bisect.c b/bisect.c\n> index 4eafc8262..cb80f29c5 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -82,15 +82,16 @@ static inline void weight_set(struct commit_list *elem, int weight)\n>  \t*((int*)(elem->item->util)) = 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    count++;\n> +\t\tif (bisect_flags & BISECT_FIRST_PARENT)\n> +\t\t\tbreak;\n>  \t}\n>  \treturn count;\n>  }\n> @@ -117,10 +118,10 @@ static inline int halfway(struct commit_list *p, int nr)\n>  }\n>  \n>  #if !DEBUG_BISECT\n> -#define show_list(a,b,c,d) do { ; } while (0)\n> +#define show_list(a,b,c,d,e) do { ; } while (0)\n>  #else\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> @@ -146,10 +147,14 @@ static void show_list(const char *debug, int counted, int nr,\n>  \t\telse\n>  \t\t\tfprintf(stderr, \"---\");\n>  \t\tfprintf(stderr, \" %.*s\", 8, oid_to_hex(&commit->object.oid));\n> -\t\tfor (pp = commit->parents; pp; pp = pp->next)\n> +\t\tfor (pp = commit->parents; pp; pp = pp->next) {\n>  \t\t\tfprintf(stderr, \" %.*s\", 8,\n>  \t\t\t\toid_to_hex(&pp->item->object.oid));\n>  \n> +\t\t\tif (bisect_flags & BISECT_FIRST_PARENT)\n> +\t\t\t\tbreak;\n> +\t\t}\n> +\n>  \t\tsubject_len = find_commit_subject(buf, &subject_start);\n>  \t\tif (subject_len)\n>  \t\t\tfprintf(stderr, \" %.*s\", subject_len, subject_start);\n> @@ -267,13 +272,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list,\n>  \t\tunsigned flags = commit->object.flags;\n>  \n>  \t\tp->item->util = &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> @@ -289,7 +294,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> @@ -319,7 +324,7 @@ 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> +\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> @@ -329,6 +334,11 @@ 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\tif ((bisect_flags & BISECT_FIRST_PARENT)) {\n> +\t\t\t\t\tif (weight(q) < 0)\n> +\t\t\t\t\t\tq = NULL;\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> @@ -346,7 +356,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> @@ -357,7 +367,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> @@ -372,7 +382,7 @@ void find_bisection(struct commit_list **commit_list, int *reaches,\n>  \tstruct commit_list *list, *p, *best, *next, *last;\n>  \tint *weights;\n>  \n> -\tshow_list(\"bisection 2 entry\", 0, 0, *commit_list);\n> +\tshow_list(\"bisection 2 entry\", 0, 0, *commit_list, bisect_flags);\n>  \n>  \t/*\n>  \t * Count the number of total and tree-changing items on the\n> @@ -395,7 +405,7 @@ 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> @@ -962,6 +972,9 @@ int bisect_next_all(const char *prefix, int no_checkout)\n>  \tif (skipped_revs.nr)\n>  \t\tbisect_flags |= BISECT_FIND_ALL;\n>  \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> diff --git a/bisect.h b/bisect.h\n> index 1d40a33ad..e445567c2 100644\n> --- a/bisect.h\n> +++ b/bisect.h\n> @@ -2,6 +2,7 @@\n>  #define BISECT_H\n>  \n>  #define BISECT_FIND_ALL\t\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> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index 8752f5bbe..66439e1b3 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -538,6 +538,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n>  \tif (bisect_list) {\n>  \t\tint reaches, all;\n>  \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> diff --git a/revision.c b/revision.c\n> index 4e0e193e5..b9d063805 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2474,9 +2474,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->expand_tabs_in_log < 0)\n>  \t\trevs->expand_tabs_in_log = revs->expand_tabs_in_log_default;\n>  \n> diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh\n> index a66140803..1bc297de5 100755\n> --- a/t/t6002-rev-list-bisect.sh\n> +++ b/t/t6002-rev-list-bisect.sh\n> @@ -263,4 +263,62 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' '\n>  \ttest_cmp expect.sorted actual.sorted\n>  '\n>  \n> +# We generate the following commit graph:\n> +#\n> +#   B ------ C\n> +#  /          \\\n> +# A            FX\n> +#  \\          /\n> +#   D - CC - EX\n> +\n> +test_expect_success 'setup' '\n> +  test_commit A &&\n> +  test_commit B &&\n> +  test_commit C &&\n> +  git reset --hard A &&\n> +  test_commit D &&\n> +  test_commit CC &&\n> +  test_commit EX &&\n> +  test_merge FX C\n> +'\n> +\n> +test_output_expect_success \"--bisect --first-parent\" 'git rev-list --bisect --first-parent FX ^A' <<EOF\n> +$(git rev-parse CC)\n> +EOF\n> +\n> +test_output_expect_success \"--first-parent\" 'git rev-list --first-parent FX ^A' <<EOF\n> +$(git rev-parse FX)\n> +$(git rev-parse EX)\n> +$(git rev-parse CC)\n> +$(git rev-parse D)\n> +EOF\n> +\n> +test_output_expect_success \"--bisect-vars --first-parent\" 'git rev-list --bisect-vars --first-parent FX ^A' <<EOF\n> +bisect_rev='$(git rev-parse CC)'\n> +bisect_nr=1\n> +bisect_good=1\n> +bisect_bad=1\n> +bisect_all=4\n> +bisect_steps=1\n> +EOF\n> +\n> +test_expect_success \"--bisect-all --first-parent\" '\n> +cat >expect <<EOF &&\n> +$(git rev-parse CC) (dist=2)\n> +$(git rev-parse EX) (dist=1)\n> +$(git rev-parse D) (dist=1)\n> +$(git rev-parse FX) (dist=0)\n> +EOF\n> +\n> +# Make sure we have the same entries, nothing more, nothing less\n> +git rev-list --bisect-all --first-parent FX ^A >actual &&\n> +  sort actual >actual.sorted &&\n> +  sort expect >expect.sorted &&\n> +  test_cmp expect.sorted actual.sorted &&\n> +  # Make sure the entries are sorted in the dist order\n> +  sed -e \"s/.*(dist=\\([1-9]*[0-9]\\)).*/\\1/\" actual >actual.dists &&\n> +  sort -r actual.dists >actual.dists.sorted &&\n> +  test_cmp actual.dists.sorted actual.dists\n> +'\n> +\n>  test_done\n> -- \n> 2.16.3\n> \n> \n"},{"id":"356707","messageId":"xmqqlg8qh22q.fsf@gitster-ct.c.googlers.com","threadId":"49235","inReplyTo":"20180828123234.44582-1-tiagonbotelho@hotmail.com","subject":"Re: [PATCH v6] Implement --first-parent for git rev-list --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-28T16:45:49Z","receivedAt":"2018-08-28T16:45:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tiago Botelho <tiagonbotelho@gmail.com> writes:\n\n> diff --git a/bisect.c b/bisect.c\n> index 4eafc8262..cb80f29c5 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -82,15 +82,16 @@ static inline void weight_set(struct commit_list *elem, int weight)\n>  \t*((int*)(elem->item->util)) = 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    count++;\n> +\t\tif (bisect_flags & BISECT_FIRST_PARENT)\n> +\t\t\tbreak;\n>  \t}\n>  \treturn count;\n>  }\n\nWe either say 0 (\"the first parent is uninteresting\") or 1 (\"it is\")\nunder BISECT_FIRST_PARENT mode.  OK.\n\n> @@ -117,10 +118,10 @@ static inline int halfway(struct commit_list *p, int nr)\n>  }\n>  \n>  #if !DEBUG_BISECT\n> -#define show_list(a,b,c,d) do { ; } while (0)\n> +#define show_list(a,b,c,d,e) do { ; } while (0)\n>  #else\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> @@ -146,10 +147,14 @@ static void show_list(const char *debug, int counted, int nr,\n\nAn unrelated tangent, but I think I just spotted a bug in the\nexisting code on the line immediately before this hunk, which reads\n\n\t\tif (commit->util)\n\t\t\tfprintf(stderr, \"%3d\", weight(p));\n\nI think this was a bug introduced at bb408ac9 (\"bisect.c: use\ncommit-slab for commit weight instead of commit->util\", 2018-05-19)\nwhere the internal implementation of weight() was changed not to\ntouch commit->util but instead to use a separate commit-slab storage\n\nLooking at the code before that conversion, it seems that we were\nusing ->util to store a pointer to an integer, so we had the ability\nto differenciate non-negative weight (i.e. weight already computed\nfor the commit), negative weight (i.e. not computed yet, but will\nbe), and commits to which the concept of weight is not applicable.\nWhen we went to the commit-slab with the change, we lost the ability\nto represent the third case.  I am offhand not sure what the best\nremedy would be.  Perhaps stuff a so-far unused value like -3 to the\nweight() and use weight(p) == -3 instead of the old !commit->util or\nsomething like that?\n\n(Duy CC'ed to help checking my sanity on this point).\n\nIn any case, this is an existing bug in a debug helper, and the\nfocus of this patch is not about fixing that bug, so you can and\nshould leave it as-is, until this patch successfully adds the\n\"bisection following only the first parent\" feature.\n\n>  \t\telse\n>  \t\t\tfprintf(stderr, \"---\");\n>  \t\tfprintf(stderr, \" %.*s\", 8, oid_to_hex(&commit->object.oid));\n> -\t\tfor (pp = commit->parents; pp; pp = pp->next)\n> +\t\tfor (pp = commit->parents; pp; pp = pp->next) {\n>  \t\t\tfprintf(stderr, \" %.*s\", 8,\n>  \t\t\t\toid_to_hex(&pp->item->object.oid));\n>  \n> +\t\t\tif (bisect_flags & BISECT_FIRST_PARENT)\n> +\t\t\t\tbreak;\n> +\t\t}\n> +\n\nI am not sure if we want to do this.  This is a debugging aid that\nallows us to peek into the internal state of the commit history\ngraph being worked on; don't we rather want to make sure that tips\nof merged side branches are not participating (e.g. commit->util\nthat records \"weight\" is used differently between the first and\nother parent commits)?\n\n>  \t\tsubject_len = find_commit_subject(buf, &subject_start);\n>  \t\tif (subject_len)\n>  \t\t\tfprintf(stderr, \" %.*s\", subject_len, subject_start);\n> @@ -267,13 +272,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list,\n>  \t\tunsigned flags = commit->object.flags;\n>  \n>  \t\tp->item->util = &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\nBecause count_interesting_parents() returns either 0 or 1 under\nBISECT_FIRST_PARENT mode, we will never set -2 in this switch()\nstatement.  We either paint the bottom boundary with the correct\nweight, or -1 (i.e. \"its weight is one plus its first parent's\nweight\" marker).  OK.\n\n> @@ -289,7 +294,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> @@ -319,7 +324,7 @@ 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> +\tshow_list(\"bisection 2 count_distance\", counted, nr, list, bisect_flags);\n\nThe loop before this part (not shown) is about counting weight for\nmerges the hard way, which does not apply to us under the new\nBISECT_FIRST_PARENT mode.  IOW, \"initialize\" and \"count_distance\"\nshould show exactly the same output from show_list().\n\n>  \twhile (counted < nr) {\n>  \t\tfor (p = list; p; p = p->next) {\n> @@ -329,6 +334,11 @@ static struct commit_list *do_find_bisection(struct commit_list *list,\n>  \t\t\tif (0 <= weight(p))\n>  \t\t\t\tcontinue;\n\nIf we know the weight of 'p' already, then we are already done with\nit.  In the normal code, that means we are done with all the merges,\nso the loop immediately below deals only with a single strand of\npearls.  However, under BISECT_FIRST_PARENT mode, that is\ndifferent.  Let's see.\n\n>  \t\t\tfor (q = p->item->parents; q; q = q->next) {\n> +\t\t\t\tif ((bisect_flags & BISECT_FIRST_PARENT)) {\n> +\t\t\t\t\tif (weight(q) < 0)\n> +\t\t\t\t\t\tq = NULL;\n> +\t\t\t\t\tbreak;\n> +\t\t\t\t}\n\nUnder first-parent mode, we uncondtionally break out of the loop\nafter inspecting the first parent, even if it is a merge.  The code\nafter this loop (outside the context) says\n\n\t\t\tif (!q)\n\t\t\t\tcontinue;\n\nwhich is to say \"we are not ready to set the weight of p based on\nthe weight of q, because the latter is unknown\".  So that is done by\nsetting q to NULL, which makes sort of sense.\n\n>  \t\t\t\tif (q->item->object.flags & UNINTERESTING)\n>  \t\t\t\t\tcontinue;\n> ...\n>  \t\t\t\tif (0 <= weight(q))\n\nPost context of this hunk is \"break;\", meaning that if we find an\ninteresting parent \n\nWhat the above original tells us is that when we are doing a full\nbisection, we do not look at weight of uninteresting parent and\ncontinue.  In that case we know p is not a merge so we will exit\nthis for() loop with q set to NULL.\n\nShouldn't we keep that property even under BISECT_FIRST_PARENT mode?\nWhat I am wondering is the block added to the top of this for() loop.\n\nShouldn't it be more like this?\n\n                                if ((bisect_flags & BISECT_FIRST_PARENT)) {\n\t\t\t\t\tif ((q->item->object.flags & UNINTERESTING) ||\n\t\t\t\t\t    (weight(q) < 0))\n\t\t\t\t\t\tq = NULL;\n\t\t\t\t\tbreak;\n\t\t\t\t}\n\nI do not even recall if weight(q) is defined for an uninteresting q\noffhand; your patch requires weight(q) is defined and is negative\nfor such a 'q' to be correct.  Code would be more trivially correct\nwith an explicit check for UNINTERESTING, I would think.\n\nIn any case, after finding a parent (or not finding, signalled by\nbeing NULL) whose weight is either the same or one less than our\nweight, we leave the above for() loop and then ...\n\n> @@ -346,7 +356,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\n... we set our weight based on its weight.  OK.\n\nThe remainder of the change looked OK (I didn't look at the the test).\n\nThanks.\n"},{"id":"356715","messageId":"xmqqy3cqfi8c.fsf@gitster-ct.c.googlers.com","threadId":"49235","inReplyTo":"nycvar.QRO.7.76.6.1808281512240.73@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v6] Implement --first-parent for git rev-list --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-28T18:39:47Z","receivedAt":"2018-08-28T18:39:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi Tiago,\n>\n> On Tue, 28 Aug 2018, Tiago Botelho wrote:\n>\n>> This will enable users to implement bisecting on first parents\n>> which can be useful for when the commits from a feature branch\n>> that we want to merge are not always tested.\n>\n> This message is still lacking the explanation I asked for, namely for the\n> lines:\n>\n> \t@@ -329,6 +334,11 @@ static struct commit_list *do_find_bisection(struct commit_list *list,\n> \t \tif (0 <= weight(p))\n> \t \t\tcontinue;\n> \t \tfor (q = p->item->parents; q; q = q->next) {\n> \t+\t\tif ((bisect_flags & BISECT_FIRST_PARENT)) {\n> \t+\t\t\tif (weight(q) < 0)\n> \t+\t\t\t\tq = NULL;\n> \t+\t\t\tbreak;\n> \t+\t\t}\n> \t \tif (q->item->object.flags & UNINTERESTING)\n> \t \t\tcontinue;\n> \t \tif (0 <= weight(q))\n\nI've just finished scanning the discussion thread on public-inbox\nfor v5, v4, v3, v2 and the initial round of this series, but found\nyour comments only on the tests.  If you have a pointer that would\nbe great; it also is OK to say what kind of explanation is needed\nfor that addition again.  \n\nFWIW I too was puzzled about the correctness of the added logic\nabove, especially the part that reads weight(q) before checking if\nit is not UNINTERESTING, but I covered it on a separate message.\n\n> I would have preferred to reuse the already existing commits generated in\n> the `setup` phase rather than generating yet another batch, and to not\n> introduce an inconsistent way to present a commit graph (compare your\n> diagram with the one in\n> https://github.com/git/git/blob/v2.18.0/t/t6002-rev-list-bisect.sh#L64-L90\n> i.e. *in the same file*)\n\nAs I already said in the previous round, I do agree with these.\nThat is, ...\n\n>> diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh\n>> index a66140803..1bc297de5 100755\n>> --- a/t/t6002-rev-list-bisect.sh\n>> +++ b/t/t6002-rev-list-bisect.sh\n>> @@ -263,4 +263,62 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' '\n>>  \ttest_cmp expect.sorted actual.sorted\n>>  '\n>>  \n>> +# We generate the following commit graph:\n>> +#\n>> +#   B ------ C\n>> +#  /          \\\n>> +# A            FX\n>> +#  \\          /\n>> +#   D - CC - EX\n>> +\n>> +test_expect_success 'setup' '\n>> +  test_commit A &&\n>> +  test_commit B &&\n>> +  test_commit C &&\n>> +  git reset --hard A &&\n>> +  test_commit D &&\n>> +  test_commit CC &&\n>> +  test_commit EX &&\n>> +  test_merge FX C\n>> +'\n\n... the above graph construction should not be necessary.  An\nearlier part of t6002 would have already created a history of\nsuitable shape to use for writing the following tests.\n\n>> +test_output_expect_success \"--bisect --first-parent\" 'git rev-list --bisect --first-parent FX ^A' <<EOF\n>> +$(git rev-parse CC)\n>> +EOF\n>> +\n>> +test_output_expect_success \"--first-parent\" 'git rev-list --first-parent FX ^A' <<EOF\n>> +$(git rev-parse FX)\n>> +$(git rev-parse EX)\n>> +$(git rev-parse CC)\n>> +$(git rev-parse D)\n>> +EOF\n>> +\n>> +test_output_expect_success \"--bisect-vars --first-parent\" 'git rev-list --bisect-vars --first-parent FX ^A' <<EOF\n>> +bisect_rev='$(git rev-parse CC)'\n>> +bisect_nr=1\n>> +bisect_good=1\n>> +bisect_bad=1\n>> +bisect_all=4\n>> +bisect_steps=1\n>> +EOF\n>> +\n>> +test_expect_success \"--bisect-all --first-parent\" '\n>> +cat >expect <<EOF &&\n>> +$(git rev-parse CC) (dist=2)\n>> +$(git rev-parse EX) (dist=1)\n>> +$(git rev-parse D) (dist=1)\n>> +$(git rev-parse FX) (dist=0)\n>> +EOF\n>> +\n>> +# Make sure we have the same entries, nothing more, nothing less\n>> +git rev-list --bisect-all --first-parent FX ^A >actual &&\n>> +  sort actual >actual.sorted &&\n>> +  sort expect >expect.sorted &&\n>> +  test_cmp expect.sorted actual.sorted &&\n>> +  # Make sure the entries are sorted in the dist order\n>> +  sed -e \"s/.*(dist=\\([1-9]*[0-9]\\)).*/\\1/\" actual >actual.dists &&\n>> +  sort -r actual.dists >actual.dists.sorted &&\n>> +  test_cmp actual.dists.sorted actual.dists\n>> +'\n>> +\n>>  test_done\n>> -- \n>> 2.16.3\n>> \n>> \n"},{"id":"356738","messageId":"xmqqbm9mfcf4.fsf@gitster-ct.c.googlers.com","threadId":"49235","inReplyTo":"xmqqy3cqfi8c.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v6] Implement --first-parent for git rev-list --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-28T20:45:19Z","receivedAt":"2018-08-28T20:45:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n>\n>> I would have preferred to reuse the already existing commits generated in\n>> the `setup` phase rather than generating yet another batch, and to not\n>> introduce an inconsistent way to present a commit graph (compare your\n>> diagram with the one in\n>> https://github.com/git/git/blob/v2.18.0/t/t6002-rev-list-bisect.sh#L64-L90\n>> i.e. *in the same file*)\n>\n> As I already said in the previous round, I do agree with these.\n> That is, ...\n>\n>>>  \n>>> +# We generate the following commit graph:\n>>> +#\n>>> +#   B ------ C\n> ... the above graph construction should not be necessary.  An\n> earlier part of t6002 would have already created a history of\n> suitable shape to use for writing the following tests.\n\nSomething like the following, perhaps?  I have a feeling that use of\ntest-output-expect-success is trying to be too cute without making\nthe result easier to read, and also makes it harder to debug the\ntest script when it does not work as expected (e.g. you need to see\nwhere the output from the actual command is stored by going to the\ndefinition of the shell function), and would have preferred to see\nthese three tests written without it (and instead using explicit\n'expect' vs 'actual' comparison), but at least the patch below shows\nwhat I meant when I suggested updating the tests to reuse the\nexisting history (I do not speak for Johannes, but I am guessing we\nare on the same page on this one).\n\nThanks.\n\n t/t6002-rev-list-bisect.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 46 insertions(+)\n\ndiff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh\nindex a661408038..d27d0087d6 100755\n--- a/t/t6002-rev-list-bisect.sh\n+++ b/t/t6002-rev-list-bisect.sh\n@@ -263,4 +263,50 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' '\n \ttest_cmp expect.sorted actual.sorted\n '\n \n+# See the drawing near the top --- e4 is in the middle of the first parent chain\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' '\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse tags/e5) (dist=4)\n+\t$(git rev-parse tags/e4) (dist=4)\n+\t$(git rev-parse tags/e6) (dist=3)\n+\t$(git rev-parse tags/e3) (dist=3)\n+\t$(git rev-parse tags/e7) (dist=2)\n+\t$(git rev-parse tags/e2) (dist=2)\n+\t$(git rev-parse tags/e8) (dist=1)\n+\t$(git rev-parse tags/e1) (dist=1)\n+\t$(git rev-parse tags/E) (dist=0)\n+\tEOF\n+\n+\t# Make sure we have the same entries, nothing more, nothing less\n+\tgit 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+\t# Make sure the entries are sorted in the dist order\n+\tsed -e \"s/.*(dist=\\([1-9]*[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 test_done\n"},{"id":"356761","messageId":"xmqqwosadw0t.fsf@gitster-ct.c.googlers.com","threadId":"49235","inReplyTo":"xmqqbm9mfcf4.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v6] Implement --first-parent for git rev-list --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-28T21:24:50Z","receivedAt":"2018-08-28T21:24:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Something like the following, perhaps?\n\nHaving said all that.\n\n> +# See the drawing near the top --- e4 is in the middle of the first parent chain\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\nThe above two are probably not controversial.\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\nPerhaps this one isn't either.\n\n\n> +test_expect_success '--bisect-all --first-parent' '\n> +\tcat >expect <<-EOF &&\n> +\t$(git rev-parse tags/e5) (dist=4)\n> +\t$(git rev-parse tags/e4) (dist=4)\n> +\t$(git rev-parse tags/e6) (dist=3)\n> +\t$(git rev-parse tags/e3) (dist=3)\n> +\t$(git rev-parse tags/e7) (dist=2)\n> +\t$(git rev-parse tags/e2) (dist=2)\n> +\t$(git rev-parse tags/e8) (dist=1)\n> +\t$(git rev-parse tags/e1) (dist=1)\n> +\t$(git rev-parse tags/E) (dist=0)\n> +\tEOF\n> +\n> +\t# Make sure we have the same entries, nothing more, nothing less\n> +\tgit 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\nBy sorting both, we attempt allow them to come out in any random\norder when sorting done by --bisect-all gets tiebroken differently\nbetween commits with the same dist value.  As Johannes said, this is\na bit too strict to insist that E *must* get dist 0, when the only\nthing we may care about are e2 and e7 get the same dist value, which\nis larger than the dist value given to E, etc.  If we wanted to ease\nthe over-strictness, we could omit \" (dist=N)\" from the 'expect' file,\nand then\n\n\tsed -e 's/ (dist=[0-9]*)$//' actual | sort >actual.sorted &&\n\tsort expect >expect.sorted &&\n\ttest_cmp expect.sorted actual.sorted &&\n\nto compare only the object names.\n\nThis over-strictness would have caught a bug in Git if we reverted\nthis new feature (i.e. \"rev-list --first-parent --bisect-all\" does\nnot refuse to work---it just does not do what we expect), which\ngives distance of -1 in the output (!).  From that point of view, I\nthink it is also OK for the test to be spelling the values out like\nthe above.\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\nI forgot to mention but I added \"-n\" here to make sure we\nnumerically sort the list of distance values.  Also you had a bogus\nregexp to catch digits inherited from \"something like this\" I showed\nin an older discussion (I think it is sufficient to say [0-9]* here).\n\nThe above makes sure that commits that have larger distance number\nare listed earlier in the output, and we do not care if which one of\ne4 or e5, both of which have distant number 4, is shown first---we'd\nbe happy as long as all the ones at distance 4 are shown before the\nothers at smaller distance number.  So with this in place, I think\nwe can omit the exact distance number from the earlier part of this\ntest, but as I said, it would have caught the bug that produces a\nnegative distance value in the output, so I am still on the fence,\nleaning a bit toward being stricter than necessary.\n\n> +\ttest_cmp actual.dists.sorted actual.dists\n> +'\n> +\n>  test_done\n"},{"id":"357179","messageId":"CACsJy8AFo+mb8R-O-JKRPZk__csq6mbVXbnZhSd-nZ08zWfSeg@mail.gmail.com","threadId":"49235","inReplyTo":"xmqqlg8qh22q.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v6] Implement --first-parent for git rev-list --bisect","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-09-02T07:34:08Z","receivedAt":"2018-09-02T07:34:37Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Aug 28, 2018 at 6:45 PM Junio C Hamano <gitster@pobox.com> wrote:\n> > @@ -146,10 +147,14 @@ static void show_list(const char *debug, int counted, int nr,\n>\n> An unrelated tangent, but I think I just spotted a bug in the\n> existing code on the line immediately before this hunk, which reads\n>\n>                 if (commit->util)\n>                         fprintf(stderr, \"%3d\", weight(p));\n>\n> I think this was a bug introduced at bb408ac9 (\"bisect.c: use\n> commit-slab for commit weight instead of commit->util\", 2018-05-19)\n> where the internal implementation of weight() was changed not to\n> touch commit->util but instead to use a separate commit-slab storage\n>\n> Looking at the code before that conversion, it seems that we were\n> using ->util to store a pointer to an integer, so we had the ability\n> to differenciate non-negative weight (i.e. weight already computed\n> for the commit), negative weight (i.e. not computed yet, but will\n> be), and commits to which the concept of weight is not applicable.\n> When we went to the commit-slab with the change, we lost the ability\n> to represent the third case.  I am offhand not sure what the best\n> remedy would be.  Perhaps stuff a so-far unused value like -3 to the\n> weight() and use weight(p) == -3 instead of the old !commit->util or\n> something like that?\n\nHmm.. no? the commit-slab stores the pointer to the weight, not the\nweight itself, so we still have the ability to check the third case, I\nthink.\n\n> (Duy CC'ed to help checking my sanity on this point).\n>\n> In any case, this is an existing bug in a debug helper, and the\n> focus of this patch is not about fixing that bug, so you can and\n> should leave it as-is, until this patch successfully adds the\n> \"bisection following only the first parent\" feature.\n\nYes. I'll post a patch soon to fix this \"commit->util\" leftover.\n-- \nDuy\n"},{"id":"357181","messageId":"20180902074250.6802-1-pclouds@gmail.com","threadId":"49235","inReplyTo":"CACsJy8AFo+mb8R-O-JKRPZk__csq6mbVXbnZhSd-nZ08zWfSeg@mail.gmail.com","subject":"[PATCH] bisect.c: make show_list() build again","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-09-02T07:42:50Z","receivedAt":"2018-09-02T07:43:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This function only compiles when DEBUG_BISECT is 1, which is often not\nthe case. As a result there are two commits [1] [2] that break it but\nthe breakages went unnoticed because the code did not compile by\ndefault. Update the function and include the new header file to make this\nfunction build again.\n\nIn order to stop this from happening again, the function is now\ncompiled unconditionally but exits early unless DEBUG_BISECT is\nnon-zero. A smart compiler generates no extra code (not even a\nfunction call). But even if it does not, this function does not seem\nto be in a hot path that the extra cost becomes a big problem.\n\n[1] bb408ac95d (bisect.c: use commit-slab for commit weight instead of\n    commit->util - 2018-05-19)\n\n[2] cbd53a2193 (object-store: move object access functions to\n    object-store.h - 2018-05-15)\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n bisect.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex e1275ba79e..e65f6821b8 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -13,6 +13,7 @@\n #include \"sha1-array.h\"\n #include \"argv-array.h\"\n #include \"commit-slab.h\"\n+#include \"object-store.h\"\n \n static struct oid_array good_revs;\n static struct oid_array skipped_revs;\n@@ -120,14 +121,14 @@ static inline int halfway(struct commit_list *p, int nr)\n \t}\n }\n \n-#if !DEBUG_BISECT\n-#define show_list(a,b,c,d) do { ; } while (0)\n-#else\n static void show_list(const char *debug, int counted, int nr,\n \t\t      struct commit_list *list)\n {\n \tstruct commit_list *p;\n \n+\tif (!DEBUG_BISECT)\n+\t\treturn;\n+\n \tfprintf(stderr, \"%s (%d/%d)\\n\", debug, counted, nr);\n \n \tfor (p = list; p; p = p->next) {\n@@ -145,7 +146,7 @@ static void show_list(const char *debug, int counted, int nr,\n \t\t\t(flags & TREESAME) ? ' ' : 'T',\n \t\t\t(flags & UNINTERESTING) ? 'U' : ' ',\n \t\t\t(flags & COUNTED) ? 'C' : ' ');\n-\t\tif (commit->util)\n+\t\tif (*commit_weight_at(&commit_weight, p->item))\n \t\t\tfprintf(stderr, \"%3d\", weight(p));\n \t\telse\n \t\t\tfprintf(stderr, \"---\");\n@@ -160,7 +161,6 @@ static void show_list(const char *debug, int counted, int nr,\n \t\tfprintf(stderr, \"\\n\");\n \t}\n }\n-#endif /* DEBUG_BISECT */\n \n static struct commit_list *best_bisection(struct commit_list *list, int nr)\n {\n-- \n2.19.0.rc0.337.ge906d732e7\n\n"},{"id":"357185","messageId":"CAP8UFD0MHUN=auZW9Zu+hnx6oAxjNsZUfQGeJ0HfBCh5RPv6jg@mail.gmail.com","threadId":"49235","inReplyTo":"20180902074250.6802-1-pclouds@gmail.com","subject":"Re: [PATCH] bisect.c: make show_list() build again","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-09-02T07:57:15Z","receivedAt":"2018-09-02T07:57:18Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Sep 2, 2018 at 9:42 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:\n\n> In order to stop this from happening again, the function is now\n> compiled unconditionally but exits early unless DEBUG_BISECT is\n> non-zero.\n\nThanks for going the extra mile and doing this!\n\nI wonder if we should also try to make the show_list() function part\nof the trace_*() functions to make it even more regular. This can be a\nseparate patch or topic though.\n"},{"id":"357217","messageId":"CACsJy8BOZy5MBsSwvHNsWeAf4tqtRJc0SJuBtfwM9wR8Yn_xfw@mail.gmail.com","threadId":"49235","inReplyTo":"CAP8UFD0MHUN=auZW9Zu+hnx6oAxjNsZUfQGeJ0HfBCh5RPv6jg@mail.gmail.com","subject":"Re: [PATCH] bisect.c: make show_list() build again","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-09-03T17:31:54Z","receivedAt":"2018-09-03T17:32:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Sep 2, 2018 at 9:57 AM Christian Couder\n<christian.couder@gmail.com> wrote:\n>\n> On Sun, Sep 2, 2018 at 9:42 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:\n>\n> > In order to stop this from happening again, the function is now\n> > compiled unconditionally but exits early unless DEBUG_BISECT is\n> > non-zero.\n>\n> Thanks for going the extra mile and doing this!\n>\n> I wonder if we should also try to make the show_list() function part\n> of the trace_*() functions to make it even more regular. This can be a\n> separate patch or topic though.\n\nYeah that's probably a good idea (though I'm not familiar with\nbisect.c enough to take that step).\n-- \nDuy\n"},{"id":"357291","messageId":"CAP8UFD135763VCPSzYuir8pR+wC=C7o12E4UcwkpzK-c8yck_Q@mail.gmail.com","threadId":"49235","inReplyTo":"CACsJy8BOZy5MBsSwvHNsWeAf4tqtRJc0SJuBtfwM9wR8Yn_xfw@mail.gmail.com","subject":"Re: [PATCH] bisect.c: make show_list() build again","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-09-04T11:13:33Z","receivedAt":"2018-09-04T11:13:36Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Sep 3, 2018 at 7:31 PM, Duy Nguyen <pclouds@gmail.com> wrote:\n> On Sun, Sep 2, 2018 at 9:57 AM Christian Couder\n> <christian.couder@gmail.com> wrote:\n>>\n>> Thanks for going the extra mile and doing this!\n>>\n>> I wonder if we should also try to make the show_list() function part\n>> of the trace_*() functions to make it even more regular. This can be a\n>> separate patch or topic though.\n>\n> Yeah that's probably a good idea (though I'm not familiar with\n> bisect.c enough to take that step).\n\nMaybe this could be a GSoC micro project or a left over bit.\n"},{"id":"357330","messageId":"xmqqy3ch83f2.fsf@gitster-ct.c.googlers.com","threadId":"49235","inReplyTo":"CACsJy8AFo+mb8R-O-JKRPZk__csq6mbVXbnZhSd-nZ08zWfSeg@mail.gmail.com","subject":"Re: [PATCH v6] Implement --first-parent for git rev-list --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-04T19:32:01Z","receivedAt":"2018-09-04T19:32:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> Hmm.. no? the commit-slab stores the pointer to the weight, not the\n> weight itself, so we still have the ability to check the third case, I\n> think.\n\nGood, thanks.\n"}]}