{"thread":{"id":"48763","subject":"[RFC PATCH v5] Implement --first-parent for git rev-list --bisect","startedAt":"2018-06-22T12:40:15Z","lastAt":"2018-07-06T21:43:09Z","messageCount":18,"participants":["Tiago Botelho","Junio C Hamano","Christian Couder","Johannes Schindelin"],"isPatch":true,"patchVersion":5,"patchTotal":null},"messages":[{"id":"350724","messageId":"20180622123945.68852-1-tiagonbotelho@hotmail.com","threadId":"48763","inReplyTo":null,"subject":"[RFC PATCH v5] Implement --first-parent for git rev-list --bisect","fromName":"Tiago Botelho","fromEmail":"tiagonbotelho@gmail.com","sentAt":"2018-06-22T12:39:45Z","receivedAt":"2018-06-22T12:40:15Z","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 resolves every issue with the unit tests. \n\nThe bug detected by Junio regarding do_find_bisection() propagating \na weight of -1 over to the child nodes ended up not being an \nissue since it cannot happen in practice, there will always be at \nleast one UNINTERESTING parent which will propagate either a weight \nof 0 or 1 throughout the child nodes.\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..c9cd349de 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 >expect1 <<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+cat >expect2 <<EOF &&\n+$(git rev-parse CC) (dist=2)\n+$(git rev-parse D) (dist=1)\n+$(git rev-parse EX) (dist=1)\n+$(git rev-parse FX) (dist=0)\n+EOF\n+\n+git rev-list --bisect-all --first-parent FX ^A >actual &&\n+  ( test_cmp expect1 actual || test_cmp expect2 actual )\n+'\n+\n test_done\n-- \n2.16.3\n\n"},{"id":"350865","messageId":"xmqq4lhqpy80.fsf@gitster-ct.c.googlers.com","threadId":"48763","inReplyTo":"20180622123945.68852-1-tiagonbotelho@hotmail.com","subject":"Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-25T17:33:51Z","receivedAt":"2018-06-25T17:33:58Z","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> +test_expect_success \"--bisect-all --first-parent\" '\n> +cat >expect1 <<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> +cat >expect2 <<EOF &&\n> +$(git rev-parse CC) (dist=2)\n> +$(git rev-parse D) (dist=1)\n> +$(git rev-parse EX) (dist=1)\n> +$(git rev-parse FX) (dist=0)\n> +EOF\n> +\n> +git rev-list --bisect-all --first-parent FX ^A >actual &&\n> +  ( test_cmp expect1 actual || test_cmp expect2 actual )\n> +'\n\nI hate to say this, but the above looks like a typical\nunmaintainable mess.\n\nWhat happens when you or somebody else later needs to update the\ngraph to be tested to add one more commit (or even more)?  Would it\nbe enough to add another \"rev-parse\" plus \"dist=X\" line in both\nexpects?  Or do we see a trap for combinatorial explosion that\nrequires us to add new expect$N?\n\n\n\n\n"},{"id":"350964","messageId":"CAP8UFD3oEjW75qsk4d_wqo2V8PmzMvZLshutw20CD7AU4b4ocg@mail.gmail.com","threadId":"48763","inReplyTo":"xmqq4lhqpy80.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-06-26T11:59:32Z","receivedAt":"2018-06-26T11:59:39Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jun 25, 2018 at 7:33 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Tiago Botelho <tiagonbotelho@gmail.com> writes:\n>\n>> +test_expect_success \"--bisect-all --first-parent\" '\n>> +cat >expect1 <<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>> +cat >expect2 <<EOF &&\n>> +$(git rev-parse CC) (dist=2)\n>> +$(git rev-parse D) (dist=1)\n>> +$(git rev-parse EX) (dist=1)\n>> +$(git rev-parse FX) (dist=0)\n>> +EOF\n>> +\n>> +git rev-list --bisect-all --first-parent FX ^A >actual &&\n>> +  ( test_cmp expect1 actual || test_cmp expect2 actual )\n>> +'\n>\n> I hate to say this, but the above looks like a typical\n> unmaintainable mess.\n>\n> What happens when you or somebody else later needs to update the\n> graph to be tested to add one more commit (or even more)?  Would it\n> be enough to add another \"rev-parse\" plus \"dist=X\" line in both\n> expects?  Or do we see a trap for combinatorial explosion that\n> requires us to add new expect$N?\n\nWhat about the following then:\n\ntest_dist_order () {\n    file=\"$1\"\n    n=\"$2\"\n    while read -r hash dist\n    do\n        d=$(echo \"$dist\" | sed -e \"s/(dist=\\(.*\\))/\\1/\")\n        case \"$d\" in\n            ''|*[!0-9]*) return 1 ;;\n            *) ;;\n        esac\n        test \"$d\" -le \"$n\" || return 1\n        n=\"$d\"\n    done <\"$file\"\n}\n\ntest_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)\nEOF\n    sort expect >expect_sorted &&\n    git rev-list --bisect-all --first-parent FX ^A >actual &&\n    sort actual >actual_sorted &&\n    test_cmp expect_sorted actual_sorted &&\n    test_dist_order actual 2\n'\n\nThis feels overkill to me, but it should scale if we ever make more\ncomplex tests.\n"},{"id":"350969","messageId":"nycvar.QRO.7.76.6.1806261540340.21419@tvgsbejvaqbjf.bet","threadId":"48763","inReplyTo":"CAP8UFD3oEjW75qsk4d_wqo2V8PmzMvZLshutw20CD7AU4b4ocg@mail.gmail.com","subject":"Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-06-26T14:10:05Z","receivedAt":"2018-06-26T14:10:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Chris,\n\nOn Tue, 26 Jun 2018, Christian Couder wrote:\n\n> On Mon, Jun 25, 2018 at 7:33 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> > Tiago Botelho <tiagonbotelho@gmail.com> writes:\n> >\n> >> +test_expect_success \"--bisect-all --first-parent\" '\n> >> +cat >expect1 <<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> >> +cat >expect2 <<EOF &&\n> >> +$(git rev-parse CC) (dist=2)\n> >> +$(git rev-parse D) (dist=1)\n> >> +$(git rev-parse EX) (dist=1)\n> >> +$(git rev-parse FX) (dist=0)\n> >> +EOF\n> >> +\n> >> +git rev-list --bisect-all --first-parent FX ^A >actual &&\n> >> +  ( test_cmp expect1 actual || test_cmp expect2 actual )\n> >> +'\n> >\n> > I hate to say this, but the above looks like a typical\n> > unmaintainable mess.\n> >\n> > What happens when you or somebody else later needs to update the\n> > graph to be tested to add one more commit (or even more)?  Would it\n> > be enough to add another \"rev-parse\" plus \"dist=X\" line in both\n> > expects?  Or do we see a trap for combinatorial explosion that\n> > requires us to add new expect$N?\n> \n> What about the following then:\n> \n> test_dist_order () {\n>     file=\"$1\"\n>     n=\"$2\"\n>     while read -r hash dist\n>     do\n>         d=$(echo \"$dist\" | sed -e \"s/(dist=\\(.*\\))/\\1/\")\n>         case \"$d\" in\n>             ''|*[!0-9]*) return 1 ;;\n>             *) ;;\n>         esac\n>         test \"$d\" -le \"$n\" || return 1\n>         n=\"$d\"\n>     done <\"$file\"\n> }\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>     sort expect >expect_sorted &&\n>     git rev-list --bisect-all --first-parent FX ^A >actual &&\n>     sort actual >actual_sorted &&\n>     test_cmp expect_sorted actual_sorted &&\n>     test_dist_order actual 2\n> '\n> \n> This feels overkill to me, but it should scale if we ever make more\n> complex tests.\n\nI *think* that you misunderstand Junio. At least when I read this:\n\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\nI go: Huh?!?!?!?!?!\n\nWhat's CC? Is it Christian Couder? Creative Commons? Crudely Complex?\n\nThe point, for me, is: if this test fails, at some stage in the future,\nfor any reason, it will be a major pain to even dissect what the test is\nsupposed to do. This is no good. And you can do better. A lot better. You\ncan write the test in a way that the head of a reader does not hurt, and\nat the same time it is still obvious what it does, and obvious that it\ndoes the right thing.\n\nOne thing that makes the brain stumble for certain is when you deviate\nfrom the conventions, especially when it is for no good reason at all. In\nthis case (and I am not sure why you, as a long-time contributor, did not\nspot this before public review):\n\n- the titles of the test cases leave a lot of room for improvement,\n\n- the lines are too long,\n\n- the convention to end the `test_expect_success` line with an opening\n  single quote is not heeded,\n\n- the graph is definitely written in an unnecessarily different format\n  than in the same test script!!!\n\n- speaking of the graph: there is a perfectly fine commit graph already.\n  Why on earth is it not reused?\n\nIn this particular case it even feels as if this test is not even testing\nwhat it should test at all:\n\n- it should verify that all of the commits in the first parent lineage are\n  part of the list\n\n- it should verify that none of the other commits are in the list\n\nAnd that is really all there is to test. You can even write that in a much\neasier way:\n\n-- snip --\ntest_expect_success '--first-parent --bisect-all lists correct revs' '\n\tgit rev-list --first-parent --bisect-all F..E >revs &&\n        # E and e1..e8 need to be shown, F and f1..f4 not\n\ttest_line_count = 9 revs &&\n\tfor rev in E e1 e2 e3 e4 e5 e6 e7 e8\n\tdo\n\t\tgrep \"^$(git rev-parse $rev) \" revs || {\n\t\t\techo \"$rev not shown\" >&2 &&\n\t\t\treturn 1\n\t\t}\n\tdone\n'\n-- snap --\n\nTo test more precisely for the order or the distance would be both\noverkill and likely to be unreadable.\n\nTo test `--bisect-vars` here would be excessive, as the part that handles\nthat particular option is not even touched. All that is touched is the\nlogic in the bisect algorithm in conjunction with --first-parent. And that\nis all that should be tested here.\n\nWith a test like the one I outlined above, I only have one more gripe\nabout the patch: the commit message does nothing to explain this part of\nthe diff:\n\n+                               if ((bisect_flags & BISECT_FIRST_PARENT)) {\n+                                       if (weight(q) < 0)\n+                                               q = NULL;\n+                                       break;\n+                               }\n\nAnd I would really, really like that to be explained in the commit\nmessage. Because to me, it is completely opaque why this needs to be here.\nThe rest of the diff is pretty obvious, though.\n\nCiao,\nJohannes\n"},{"id":"350971","messageId":"CAP8UFD1TeC4czp_8HCRw5CtjGO78A8gRezw_xspnm4MXuhQswg@mail.gmail.com","threadId":"48763","inReplyTo":"nycvar.QRO.7.76.6.1806261540340.21419@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-06-26T15:41:44Z","receivedAt":"2018-06-26T15:41:50Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Dscho,\n\nOn Tue, Jun 26, 2018 at 4:10 PM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n>\n> On Tue, 26 Jun 2018, Christian Couder wrote:\n>\n>> On Mon, Jun 25, 2018 at 7:33 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> > I hate to say this, but the above looks like a typical\n>> > unmaintainable mess.\n>> >\n>> > What happens when you or somebody else later needs to update the\n>> > graph to be tested to add one more commit (or even more)?  Would it\n>> > be enough to add another \"rev-parse\" plus \"dist=X\" line in both\n>> > expects?  Or do we see a trap for combinatorial explosion that\n>> > requires us to add new expect$N?\n>>\n>> What about the following then:\n>>\n>> test_dist_order () {\n>>     file=\"$1\"\n>>     n=\"$2\"\n>>     while read -r hash dist\n>>     do\n>>         d=$(echo \"$dist\" | sed -e \"s/(dist=\\(.*\\))/\\1/\")\n>>         case \"$d\" in\n>>             ''|*[!0-9]*) return 1 ;;\n>>             *) ;;\n>>         esac\n>>         test \"$d\" -le \"$n\" || return 1\n>>         n=\"$d\"\n>>     done <\"$file\"\n>> }\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>>     sort expect >expect_sorted &&\n>>     git rev-list --bisect-all --first-parent FX ^A >actual &&\n>>     sort actual >actual_sorted &&\n>>     test_cmp expect_sorted actual_sorted &&\n>>     test_dist_order actual 2\n>> '\n>>\n>> This feels overkill to me, but it should scale if we ever make more\n>> complex tests.\n>\n> I *think* that you misunderstand Junio. At least when I read this:\n>\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>\n> I go: Huh?!?!?!?!?!\n>\n> What's CC? Is it Christian Couder? Creative Commons? Crudely Complex?\n\nI agree that the name of the commit could be improved.\n\n> The point, for me, is: if this test fails, at some stage in the future,\n> for any reason, it will be a major pain to even dissect what the test is\n> supposed to do.\n\nI think the test is quite simple and there is an ascii picture, so it\nis not so difficult to understand.\n\n> This is no good. And you can do better. A lot better. You\n> can write the test in a way that the head of a reader does not hurt, and\n> at the same time it is still obvious what it does, and obvious that it\n> does the right thing.\n\nObviousness is often not the same for everybody.\n\n> One thing that makes the brain stumble for certain is when you deviate\n> from the conventions, especially when it is for no good reason at all. In\n> this case (and I am not sure why you, as a long-time contributor, did not\n> spot this before public review):\n\nPlease note that I never committed myself (like most of us who are not\nmaintainer actually) to reviewing everything bisect related (or\neverything that Tiago works on).\n\n> - the titles of the test cases leave a lot of room for improvement,\n>\n> - the lines are too long,\n>\n> - the convention to end the `test_expect_success` line with an opening\n>   single quote is not heeded,\n\nIf you take a look at the beginning of the script you will see that\nthere are those problems there too.\n\nI know that we should try to do better, but here I am mostly\ninterested in moving forward a feature that people have requested for\nages, not cleaning up those tests. If someone else like you or Junio\nthinks that it would be a good time to clean things up a bit, then I\nam ok with it, but that's not my main goal here.\n\n> - the graph is definitely written in an unnecessarily different format\n>   than in the same test script!!!\n\nJust above you complain about things that are similar to the previous\ntests and now you complain about things that are different...\n\n> - speaking of the graph: there is a perfectly fine commit graph already.\n>   Why on earth is it not reused?\n\nPerhaps because it is more complex than needed to test this feature\nand/or to understand what happens. And I don't think we require\neverywhere only one commit graph per test script.\n\n> In this particular case it even feels as if this test is not even testing\n> what it should test at all:\n>\n> - it should verify that all of the commits in the first parent lineage are\n>   part of the list\n\nIt does that.\n\n> - it should verify that none of the other commits are in the list\n\nIt does that too.\n\n> And that is really all there is to test.\n\nI don't agree. I think that when possible, especially when testing\nplumbing commands like rev-list, it is a good thing to test as many\nthings as possible at once.\n\n> You can even write that in a much\n> easier way:\n>\n> -- snip --\n> test_expect_success '--first-parent --bisect-all lists correct revs' '\n>         git rev-list --first-parent --bisect-all F..E >revs &&\n>         # E and e1..e8 need to be shown, F and f1..f4 not\n>         test_line_count = 9 revs &&\n>         for rev in E e1 e2 e3 e4 e5 e6 e7 e8\n>         do\n>                 grep \"^$(git rev-parse $rev) \" revs || {\n>                         echo \"$rev not shown\" >&2 &&\n>                         return 1\n>                 }\n>         done\n> '\n> -- snap --\n>\n> To test more precisely for the order or the distance would be both\n> overkill and likely to be unreadable.\n\nI don't think the previous version was either overkill or unreadable.\nYeah it had other (potential in my opinion) problems, but I was trying\nwith my suggestion to find a good balance between the different\nrequirements (readability, complexity, maintainability, testing many\nthings).\n\nBy the way with only the 2 requirements you list above, I think the\nsimplest would be sorting things like I do it in my suggestion without\nchecking that the (dist=X) are in the right order with just:\n\ntest_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)\nEOF\n    sort expect >expect_sorted &&\n    git rev-list --bisect-all --first-parent FX ^A >actual &&\n    sort actual >actual_sorted &&\n    test_cmp expect_sorted actual_sorted\n'\n\n> To test `--bisect-vars` here would be excessive, as the part that handles\n> that particular option is not even touched. All that is touched is the\n> logic in the bisect algorithm in conjunction with --first-parent. And that\n> is all that should be tested here.\n\nI don't agree with that. I think that as we now enable --first-parent\nwith all the --bisect* options, we should test at least once all the\n--bisect* options with --first-parent.\n\n> With a test like the one I outlined above, I only have one more gripe\n> about the patch: the commit message does nothing to explain this part of\n> the diff:\n>\n> +                               if ((bisect_flags & BISECT_FIRST_PARENT)) {\n> +                                       if (weight(q) < 0)\n> +                                               q = NULL;\n> +                                       break;\n> +                               }\n>\n> And I would really, really like that to be explained in the commit\n> message. Because to me, it is completely opaque why this needs to be here.\n\nThis was suggested by Junio in a previous iteration and I agree that a\ncomment would be welcome.\n\n> The rest of the diff is pretty obvious, though.\n\nThanks for taking a look anyway,\nChristian.\n"},{"id":"351026","messageId":"nycvar.QRO.7.76.6.1806262256400.21419@tvgsbejvaqbjf.bet","threadId":"48763","inReplyTo":"CAP8UFD1TeC4czp_8HCRw5CtjGO78A8gRezw_xspnm4MXuhQswg@mail.gmail.com","subject":"Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-06-26T21:16:12Z","receivedAt":"2018-06-26T21:16:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Chris,\n\nOn Tue, 26 Jun 2018, Christian Couder wrote:\n\n> On Tue, Jun 26, 2018 at 4:10 PM, Johannes Schindelin\n> <Johannes.Schindelin@gmx.de> wrote:\n> >\n> > The point, for me, is: if this test fails, at some stage in the\n> > future, for any reason, it will be a major pain to even dissect what\n> > the test is supposed to do.\n> \n> I think the test is quite simple and there is an ascii picture, so it is\n> not so difficult to understand.\n\nSo I tell you that I find it hard to understand and you try to refute this\nreview by... saying that it is quite simple to understand?\n\nThat's not really a basis for discussion there, if you do not even\nacknowledge that both Junio and I pointed out a serious flaw in the patch.\nI have no idea how you think we can reach consensus if you just deny that\nthere is a problem *when I just demonstrated that there is a\nproblem*?!?!?!\n\n> I know that we should try to do better, but here I am mostly\n> interested in moving forward a feature that people have requested for\n> ages, not cleaning up those tests. If someone else like you or Junio\n> thinks that it would be a good time to clean things up a bit, then I\n> am ok with it, but that's not my main goal here.\n\nThis test is not good. It is not good because if even a long-time\ncontributor such as myself finds it too hard to understand.\n\nSo you cannot just force it forward.\n\n> > - the graph is definitely written in an unnecessarily different format\n> > than in the same test script!!!\n> \n> Just above you complain about things that are similar to the previous\n> tests and now you complain about things that are different...\n\nYou misunderstood. I complained that things are *not* similar to previous\ntests. I said the exact opposite of what you understood.\n\n> > - speaking of the graph: there is a perfectly fine commit graph already.\n> >   Why on earth is it not reused?\n> \n> Perhaps because it is more complex than needed to test this feature\n> and/or to understand what happens.\n\nWhat? Have you even *bothered* to look at the graphs?\n\nIf at all, the newly introduced is *less* complex (except for the\npointlessly confusing commit names). Both graphs have a branch point, a\ntip commit, and a first-parent and a second-parent lineage.\n\nSo the new test introduces new commits for no good reason whatsoever, with\na diagram that is unnecessarily different from the one that is already in\nthe file (horizontal layout instead of vertical, and the horizontal layout\ndisagrees even with the existing other tests' horizontal graph diagram).\n\n> > In this particular case it even feels as if this test is not even testing\n> > what it should test at all:\n> >\n> > - it should verify that all of the commits in the first parent lineage are\n> >   part of the list\n> \n> It does that.\n\nYes, it does that, and you can verify that it does by, uhm, spending 10\nminutes pouring over the diagram.\n\nIn my proposed test case, you see the same after reading the simple loop\nand from the comment and the line count test, i.e. after spending 10\nseconds.\n\nSo your \"It does that\" is a bit inappropriate. Of course your test does\nthat. In a very convoluted way. And I demonstrated that it does not have\nto be convoluted at all.\n\n> > - it should verify that none of the other commits are in the list\n> \n> It does that too.\n\nSame here. You miss the point. It is obvious in my proposed test case that\nit does that. In your proposed test case it is everything but obvious.\n\n> > And that is really all there is to test.\n> \n> I don't agree. I think that when possible, especially when testing\n> plumbing commands like rev-list, it is a good thing to test as many\n> things as possible at once.\n\nThen you do not understand the main purpose of regression tests, which is\nto not only catch, but also to fix regressions. And your test code seems\nto be designed to make the latter very, very hard. Just like your response\nto my review almost appears to be designed to be defiant rather than\nconstructive.\n\nIf you want to accept my help in making your code better, we can continue.\nOtherwise I am afraid that your patch is stuck in the \"not good enough\"\npile.\n\n> > You can even write that in a much\n> > easier way:\n> >\n> > -- snip --\n> > test_expect_success '--first-parent --bisect-all lists correct revs' '\n> >         git rev-list --first-parent --bisect-all F..E >revs &&\n> >         # E and e1..e8 need to be shown, F and f1..f4 not\n> >         test_line_count = 9 revs &&\n> >         for rev in E e1 e2 e3 e4 e5 e6 e7 e8\n> >         do\n> >                 grep \"^$(git rev-parse $rev) \" revs || {\n> >                         echo \"$rev not shown\" >&2 &&\n> >                         return 1\n> >                 }\n> >         done\n> > '\n> > -- snap --\n> >\n> > To test more precisely for the order or the distance would be both\n> > overkill and likely to be unreadable.\n> \n> I don't think the previous version was either overkill or unreadable.\n\nOkay, this is too tedious. Seriously, why do you make it so unpleasant to\nhelp you improve the patch.\n\n> Yeah it had other (potential in my opinion) problems, but I was trying\n> with my suggestion to find a good balance between the different\n> requirements (readability, complexity, maintainability, testing many\n> things).\n> \n> By the way with only the 2 requirements you list above, I think the\n> simplest would be sorting things like I do it in my suggestion without\n> checking that the (dist=X) are in the right order with just:\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>     sort expect >expect_sorted &&\n>     git rev-list --bisect-all --first-parent FX ^A >actual &&\n>     sort actual >actual_sorted &&\n>     test_cmp expect_sorted actual_sorted\n> '\n> \n> > To test `--bisect-vars` here would be excessive, as the part that handles\n> > that particular option is not even touched. All that is touched is the\n> > logic in the bisect algorithm in conjunction with --first-parent. And that\n> > is all that should be tested here.\n> \n> I don't agree with that. I think that as we now enable --first-parent\n> with all the --bisect* options, we should test at least once all the\n> --bisect* options with --first-parent.\n\nBut why? Why, why, why? Do you touch code that changes all of those code\npaths? No you don't. Not at all. You change a code path that is shared by\nthose options, so if there is a regression, it affects all of those\noptions.\n\nYour proposed test is too extensive and therefore just *wastes space and\ntime for no good reason*.\n\n> > With a test like the one I outlined above, I only have one more gripe\n> > about the patch: the commit message does nothing to explain this part of\n> > the diff:\n> >\n> > +                               if ((bisect_flags & BISECT_FIRST_PARENT)) {\n> > +                                       if (weight(q) < 0)\n> > +                                               q = NULL;\n> > +                                       break;\n> > +                               }\n> >\n> > And I would really, really like that to be explained in the commit\n> > message. Because to me, it is completely opaque why this needs to be here.\n> \n> This was suggested by Junio in a previous iteration and I agree that a\n> comment would be welcome.\n\nThat is a very roundabout way of saying that this explanation is required.\n\n> > The rest of the diff is pretty obvious, though.\n> \n> Thanks for taking a look anyway,\n\nThere is absolutely no need to be flippant and disrespectful about this.\n\nCiao,\nJohannes\n"},{"id":"351051","messageId":"xmqqa7rhi40f.fsf@gitster-ct.c.googlers.com","threadId":"48763","inReplyTo":"CAP8UFD1TeC4czp_8HCRw5CtjGO78A8gRezw_xspnm4MXuhQswg@mail.gmail.com","subject":"Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-26T22:20:32Z","receivedAt":"2018-06-26T22:20:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> Obviousness is often not the same for everybody.\n\n... which you just learned---what you thought obvious turns out to\nbe not so obvious after all, so you adjust to help your readers.\n\n>> In this particular case it even feels as if this test is not even testing\n>> what it should test at all:\n>>\n>> - it should verify that all of the commits in the first parent lineage are\n>>   part of the list\n>\n> It does that.\n>\n>> - it should verify that none of the other commits are in the list\n>\n> It does that too.\n\nBut the point is it does a lot more by insisting exact output.  For\nexample, the version I reviewed had a two \"expected output\", and\nsaid that the actual output must match either one of them.  I guess\nit was because there were two entries with the same distance and we\ncannot rely on which order they appear in the result?  If a test\nhistory gained another entry with the same distance, then would we\nneed 6 possible expected output because we cannot rely on the order\nin which these three come out?\n\nThat was the only thing I was complaining about.  Dscho gave me too\nmuch credit and read a lot more good things than what I actually\nmeant to say ;-).\n\n>> And that is really all there is to test.\n\nAnother is that \"rev-list --bisect-all\" promises that the entries\nare ordered by the distance value.  So taking the above three\npoints, perhaps\n\n\tcat >expect <<EOF &&\n\t... as written in one of the expect list in Tiago's patch\n\tEOF\n\n\t# Make sure we have the same entries, nothing more, nothing less\n\tgit rev-list --bisect-all $other_args >actual &&\n\tsort actual >actual.sorted &&\n\tsort expect >expect.sorted &&\n\ttest_cmp expect.sorted actual.sorted\n\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 actual.dists >actual.dists.sorted &&\n\ttest_cmp actual.dists.sorted actual.dists\n\nis what I would have expected.\n"},{"id":"351085","messageId":"nycvar.QRO.7.76.6.1806271254210.21419@tvgsbejvaqbjf.bet","threadId":"48763","inReplyTo":"xmqqa7rhi40f.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-06-27T11:48:43Z","receivedAt":"2018-06-27T11:48:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 26 Jun 2018, Junio C Hamano wrote:\n\n> Christian Couder <christian.couder@gmail.com> writes:\n> \n> > Obviousness is often not the same for everybody.\n> \n> ... which you just learned---what you thought obvious turns out to\n> be not so obvious after all, so you adjust to help your readers.\n\nIndeed. And trying to tell the reader that they should find it obvious is\nnot exactly productive. It only causes bad feelings and can be easily\navoided.\n\n> >> In this particular case it even feels as if this test is not even\n> >> testing what it should test at all:\n> >>\n> >> - it should verify that all of the commits in the first parent\n> >> lineage are part of the list\n> >\n> > It does that.\n> >\n> >> - it should verify that none of the other commits are in the list\n> >\n> > It does that too.\n> \n> But the point is it does a lot more by insisting exact output.\n\nIndeed. One thing it does in addition is to make the test code a lot less\nobvious for readers in general.\n\nLet me summarize again what good regression tests have to deliver, because\nI think it cannot be stressed enough, especially in this context where we\nrun the danger of adding poor regression tests:\n\n- a regression test needs to catch regressions (d'oh)\n\n- a regression test needs to be *quick*. Otherwise developers will skip\n  running them, which is worse than having no tests at all (because the\n  effort to develop them is wasted)\n\n- a regression test must make it as easy as possible to fix regressions\n\nThere are quite a few corollaries to that last point, some of which are:\n\n- a regression test that fails for anything but an obvious reason is\n  *useless*\n\n- a regression test that tries to test for *everything* (including dogs and\n  cats) not only breaks the quickness requirement, but it also makes it\n  confusing where to start fixing things. \"The test talked about --bisect\n  but now I am stuck somewhere in XYZ.c, what has *that* to do with\n  --bisect?\" is *not* something you want to risk any developer yelling at\n  the test code that you authored\n\n- less is more. If you can use commits that were already generated in the\n  common \"setup\" step, there is literally a negative value in generating a\n  new set of commits instead\n\n- less is more. If you can catch the same regressions in three, concise\n  *and* understandable lines, avoid using thirty lines instead\n\n- regression tests should not need to be adjusted when the logic changes\n  in an intended way. It is a strong sign that a regression test was\n  written badly if it starts failing for any reason other than a\n  regression\n\nThe overzealous \"I want this output to be exactly this\" stance of the\ntests we are discussing here is a very obvious violation here.\n\nRegression tests should fail only to indicate regressions.\n\nReally. Let me repeat that because it is so obvious when you think about\nit, but it is easy to forget when writing regression tests:\n\nRegression tests should fail only to indicate regressions.\n\n> For example, the version I reviewed had a two \"expected output\", and\n> said that the actual output must match either one of them.  I guess it\n> was because there were two entries with the same distance and we cannot\n> rely on which order they appear in the result?  If a test history gained\n> another entry with the same distance, then would we need 6 possible\n> expected output because we cannot rely on the order in which these three\n> come out?\n\nIt is totally unnecessary to go there, as it would make those regression\ntests a lot less valuable than they could otherwise be. Let me elaborate\nfurther below.\n\n> That was the only thing I was complaining about.  Dscho gave me too\n> much credit and read a lot more good things than what I actually\n> meant to say ;-).\n\nWhy don't you just accept my praise gracefully? ;-)\n\nIt's not that I gave you a lot of praise recently, even if you clearly\ndeserve it.\n\n> >> And that is really all there is to test.\n> \n> Another is that \"rev-list --bisect-all\" promises that the entries\n> are ordered by the distance value.\n\nYes! And you know what we can do there? We can test *precisely* that!\n\n\t# verify that the output is sorted by `dist` (descending)\n\tsed \"s/.*dist=\\([0-9]*\\).*/\\1/\" <revs >dist &&\n\tsort -n -r <dist >dist.sorted &&\n\ttest_cmp dist dist.sorted\n\nThis extracts the distance numbers into their own file, then verifies that\nthey were already sorted.\n\nThe really big advantage here is that any future change that might result\nin a different order of entries with the same \"dist\" value *will not cause\nthis regression test to fail*. And for a good reason: because ordering\nidentical \"dist\" values differently is not a regression at all.\n\n> So taking the above three points, perhaps\n> \n> \tcat >expect <<EOF &&\n> \t... as written in one of the expect list in Tiago's patch\n> \tEOF\n\nPlease no. Please let's *not* generate more commits when we already have a\nperfectly fine set of commits generated by the setup phase of the very\nsame script. Please let's not use confusing names for those commits.\nPlease let's not add a diagram whos layout deviates for no good reason\nfrom the layout used by the existing diagram in the same file.\n\n> \t# Make sure we have the same entries, nothing more, nothing less\n> \tgit rev-list --bisect-all $other_args >actual &&\n> \tsort actual >actual.sorted &&\n> \tsort expect >expect.sorted &&\n> \ttest_cmp expect.sorted actual.sorted\n\nWith plenty experience in investigating test failures of Git's test suite\nunder my belt, let me tell you how much faster it is for me to start\ndebugging when reading this code instead:\n\n\tgit rev-list --bisect-all --first-parent F..E >revs &&\n\t# only E, e1..e8 should be listed, nothing else\n\ttest_line_count = 9 revs &&\n\tfor rev in E e1 e2 e3 e4 e5 e6 e7 e8\n\tdo\n\t\tgrep \"^$(git rev-parse $rev) \" revs || return\n\tdone\n\nI am faster by... a lot. Like, seconds instead of minutes.\n\nAs a bonus, it is also shorter. Quicker to read. Easier to grok. Quicker\nto validate manually.\n\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 actual.dists >actual.dists.sorted &&\n> \ttest_cmp actual.dists.sorted actual.dists\n> \n> is what I would have expected.\n\n... but without the [1-9] because the distance can be 0. And with a star\nafter the [0-9] instead.\n\nAnd then you have essentially the very same thing I suggested above. A\nreadable regression test, that only triggers failures upon the regression\nwe want to prevent, is fast to grok, actionable, and leads to fast\ndebugging.\n\nJust to make sure: I am utterly disinterested in pushing \"my\" code\nthrough. I am very much interested in having a good patch with a good\nregression test and a good commit message. Otherwise I would not put up\nwith what it takes to review it.\n\nSo Chris, if you feel that you must \"win\" against me, just take Junio's\ncode already. In that case, however, do change his code to avoid\ngenerating those unnecessary commits.\n\nThanks,\nDscho\n"},{"id":"351117","messageId":"xmqqwoukgpr9.fsf@gitster-ct.c.googlers.com","threadId":"48763","inReplyTo":"nycvar.QRO.7.76.6.1806271254210.21419@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-27T16:26:02Z","receivedAt":"2018-06-27T16:26:09Z","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> \tgit rev-list --bisect-all --first-parent F..E >revs &&\n> \t# only E, e1..e8 should be listed, nothing else\n> \ttest_line_count = 9 revs &&\n> \tfor rev in E e1 e2 e3 e4 e5 e6 e7 e8\n> \tdo\n> \t\tgrep \"^$(git rev-parse $rev) \" revs || return\n> \tdone\n>\n> I am faster by... a lot. Like, seconds instead of minutes.\n\nI'm fine either way.  I just thought you would not want 9 separate\ninvocations of grep ;-)\n"},{"id":"351206","messageId":"nycvar.QRO.7.76.6.1806281505160.73@tvgsbejvaqbjf.bet","threadId":"48763","inReplyTo":"xmqqwoukgpr9.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-06-28T13:08:22Z","receivedAt":"2018-06-28T13:08:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 27 Jun 2018, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > \tgit rev-list --bisect-all --first-parent F..E >revs &&\n> > \t# only E, e1..e8 should be listed, nothing else\n> > \ttest_line_count = 9 revs &&\n> > \tfor rev in E e1 e2 e3 e4 e5 e6 e7 e8\n> > \tdo\n> > \t\tgrep \"^$(git rev-parse $rev) \" revs || return\n> > \tdone\n> >\n> > I am faster by... a lot. Like, seconds instead of minutes.\n> \n> I'm fine either way.  I just thought you would not want 9 separate\n> invocations of grep ;-)\n\nI don't.\n\nI like the unnecessary test_commit calls even less ;-)\n\nAnd yes, we could avoid those `grep` calls, I know. By something as\nconvoluted as\n\n\trevs=\"$(cat revs)\" &&\n\tfor rev in $(git rev-parse E e1 e2 e3 e4 e5 e6 e7 e8)\n\tdo\n\t\tcase \"$LF$revs\" in\n\t\t*\"$LF$rev \"*) ;; # okay\n\t\t*) echo \"Missing: $rev\">&2; return 1;;\n\t\tesac\n\tdone\n\nIs this really what you would call an easy-to-understand test? Maybe for\nyou, a Unix oldtimer and shell scripting king.\n\nCiao,\nDscho\n"},{"id":"351229","messageId":"xmqqvaa2yjo1.fsf@gitster-ct.c.googlers.com","threadId":"48763","inReplyTo":"nycvar.QRO.7.76.6.1806281505160.73@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-28T16:12:30Z","receivedAt":"2018-06-28T16:12:38Z","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> On Wed, 27 Jun 2018, Junio C Hamano wrote:\n>\n>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n>> \n>> > \tgit rev-list --bisect-all --first-parent F..E >revs &&\n>> > \t# only E, e1..e8 should be listed, nothing else\n>> > \ttest_line_count = 9 revs &&\n>> > \tfor rev in E e1 e2 e3 e4 e5 e6 e7 e8\n>> > \tdo\n>> > \t\tgrep \"^$(git rev-parse $rev) \" revs || return\n>> > \tdone\n>> >\n>> > I am faster by... a lot. Like, seconds instead of minutes.\n>> \n>> I'm fine either way.  I just thought you would not want 9 separate\n>> invocations of grep ;-)\n>\n> I don't.\n>\n> I like the unnecessary test_commit calls even less ;-)\n>\n> And yes, we could avoid those `grep` calls, I know. By something as\n> convoluted as\n\nI now think you are reading me wrong ;-)  \n\nI think you already established pretty well that it is a good idea\nto avoid introducing different history to run the new test on, when\nthere is perfectly usable one available.  That part, i.e. test_commit,\nI do not think we have anything to disagree about.\n\nWhat I meant by \"many separte grep calls\" was to contrast these two\napproaches:\n\n * Have one typical output spelled out as \"expect\", take an output\n   from an actual run into \"actual\", make them comparable and then\n   do a compare (which does not use grep; it uses sort in the\n   \"making comparable\" phase)\n\n * Not have any typical output, take an output from an actual run,\n   and have _code_ that inspects the output encode the rule over the\n   output (e.g. \"these must exist\" is inspected with many grep\n   invocations)\n\nTwo things the \"output must have 9 entries, and these 9 must be\nmentioned\" we see at the beginning of this message does not verify\nare (1) exact dist value given to each of these entries and (2) the\norder in which these entries appear in the output.  The latter is\nsomething we document, and the test should cover.  The former does\nnot have to be cast in stone (i.e. I do not think it does not make a\ndifference to label the edge commits with dist=1 or dist=0 as long\nas everything is consistent), but if there is no strong reason to\nkeep it possible for us to later change how the numbers are assigned,\nI am OK if the test cast it in stone.\n\nAnother reason (other than \"many separate invocation of grep\") to\nfavor the former approach (i.e. use real-looking expected output,\nmunge it and the real output into comparable forms and then compare)\nis that it is easier to see what aspect of the output we care (and\nwe do not care) about.\n\nIt is harder to see the omission of exact dist value and ordering of\nentries in the outpu in the latter approach, and more importantly,\nknow if the omission was deliberate (e.g. it was merely an example)\nor a mere mistake.\n\nWith \"using a real-looking expected output, make it and the actual\noutput comparable and then compare\" approach, the aspects in the\noutput we choose not to care about will show in the \"make them\ncomparable\" munging.  If we do not care the exact dist values, there\nwould be something like s/dist=[0-9]*/dist=X/ to mask the exact\nvalue before making the two comparable to see that the expect and\nthe actual files have the same entries.  If we still do care about\nthe entries are ordered by the dist values, there would be something\nthat sorts the entries with the actual dist values before doing that\nmasking to ensure if the order is correct.\n\n\n\n\n   \n\n\n"},{"id":"351328","messageId":"nycvar.QRO.7.76.6.1806291317150.74@tvgsbejvaqbjf.bet","threadId":"48763","inReplyTo":"xmqqvaa2yjo1.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-06-29T11:20:58Z","receivedAt":"2018-06-29T11:21:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 28 Jun 2018, Junio C Hamano wrote:\n\n> What I meant by \"many separte grep calls\" was to contrast these two\n> approaches:\n> \n>  * Have one typical output spelled out as \"expect\", take an output\n>    from an actual run into \"actual\", make them comparable and then\n>    do a compare (which does not use grep; it uses sort in the\n>    \"making comparable\" phase)\n> \n>  * Not have any typical output, take an output from an actual run,\n>    and have _code_ that inspects the output encode the rule over the\n>    output (e.g. \"these must exist\" is inspected with many grep\n>    invocations)\n> \n> Two things the \"output must have 9 entries, and these 9 must be\n> mentioned\" we see at the beginning of this message does not verify\n> are (1) exact dist value given to each of these entries and (2) the\n> order in which these entries appear in the output.  The latter is\n> something we document, and the test should cover.  The former does\n> not have to be cast in stone (i.e. I do not think it does not make a\n> difference to label the edge commits with dist=1 or dist=0 as long\n> as everything is consistent), but if there is no strong reason to\n> keep it possible for us to later change how the numbers are assigned,\n> I am OK if the test cast it in stone.\n> \n> Another reason (other than \"many separate invocation of grep\") to\n> favor the former approach (i.e. use real-looking expected output,\n> munge it and the real output into comparable forms and then compare)\n> is that it is easier to see what aspect of the output we care (and\n> we do not care) about.\n> \n> It is harder to see the omission of exact dist value and ordering of\n> entries in the outpu in the latter approach, and more importantly,\n> know if the omission was deliberate (e.g. it was merely an example)\n> or a mere mistake.\n> \n> With \"using a real-looking expected output, make it and the actual\n> output comparable and then compare\" approach, the aspects in the\n> output we choose not to care about will show in the \"make them\n> comparable\" munging.  If we do not care the exact dist values, there\n> would be something like s/dist=[0-9]*/dist=X/ to mask the exact\n> value before making the two comparable to see that the expect and\n> the actual files have the same entries.  If we still do care about\n> the entries are ordered by the dist values, there would be something\n> that sorts the entries with the actual dist values before doing that\n> masking to ensure if the order is correct.\n\nThe problem here is of course that you *cannot* set the exact output\nin stone, because of sorting instabilities.\n\nSo you have to play tricks to sort (twice, with different keys) the\nexpected output and the actual output, to verify that all the expected\ncommits are listed (and only those) and to verify that they are ordered by\nthe distance, in descending order.\n\nAnd this trick, while it still makes the test correct and stable and yadda\nyadda, *also* makes this trick a lot less readable than my version. And\ntherefore makes it more difficult to verify that your test actually does\nwhat it is supposed to do.\n\nCiao,\nDscho\n"},{"id":"351659","messageId":"CAADF+x3jd5G9+SP3UmhwqrR_T6BuD0PkQJ3x+NLpq2BJ_Ej-Sw@mail.gmail.com","threadId":"48763","inReplyTo":"nycvar.QRO.7.76.6.1806291317150.74@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect","fromName":"Tiago Botelho","fromEmail":"tiagonbotelho@gmail.com","sentAt":"2018-07-03T21:33:10Z","receivedAt":"2018-07-03T21:35:03Z","isPatch":true,"sender":{"key":"tiagonbotelho@gmail.com","avatar":"https://gravatar.com/avatar/263ea75fe0d4aa4ac14fa9dd08b82955689b6769cf5d33599914ca4321a4597b?d=mp&s=160"},"body":"On Fri, 29 Jun 2018 at 12:21, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n>\n> Hi Junio,\n>\n> On Thu, 28 Jun 2018, Junio C Hamano wrote:\n>\n> > What I meant by \"many separte grep calls\" was to contrast these two\n> > approaches:\n> >\n> >  * Have one typical output spelled out as \"expect\", take an output\n> >    from an actual run into \"actual\", make them comparable and then\n> >    do a compare (which does not use grep; it uses sort in the\n> >    \"making comparable\" phase)\n> >\n> >  * Not have any typical output, take an output from an actual run,\n> >    and have _code_ that inspects the output encode the rule over the\n> >    output (e.g. \"these must exist\" is inspected with many grep\n> >    invocations)\n> >\n> > Two things the \"output must have 9 entries, and these 9 must be\n> > mentioned\" we see at the beginning of this message does not verify\n> > are (1) exact dist value given to each of these entries and (2) the\n> > order in which these entries appear in the output.  The latter is\n> > something we document, and the test should cover.  The former does\n> > not have to be cast in stone (i.e. I do not think it does not make a\n> > difference to label the edge commits with dist=1 or dist=0 as long\n> > as everything is consistent), but if there is no strong reason to\n> > keep it possible for us to later change how the numbers are assigned,\n> > I am OK if the test cast it in stone.\n> >\n> > Another reason (other than \"many separate invocation of grep\") to\n> > favor the former approach (i.e. use real-looking expected output,\n> > munge it and the real output into comparable forms and then compare)\n> > is that it is easier to see what aspect of the output we care (and\n> > we do not care) about.\n> >\n> > It is harder to see the omission of exact dist value and ordering of\n> > entries in the outpu in the latter approach, and more importantly,\n> > know if the omission was deliberate (e.g. it was merely an example)\n> > or a mere mistake.\n> >\n> > With \"using a real-looking expected output, make it and the actual\n> > output comparable and then compare\" approach, the aspects in the\n> > output we choose not to care about will show in the \"make them\n> > comparable\" munging.  If we do not care the exact dist values, there\n> > would be something like s/dist=[0-9]*/dist=X/ to mask the exact\n> > value before making the two comparable to see that the expect and\n> > the actual files have the same entries.  If we still do care about\n> > the entries are ordered by the dist values, there would be something\n> > that sorts the entries with the actual dist values before doing that\n> > masking to ensure if the order is correct.\n>\n> The problem here is of course that you *cannot* set the exact output\n> in stone, because of sorting instabilities.\n>\n> So you have to play tricks to sort (twice, with different keys) the\n> expected output and the actual output, to verify that all the expected\n> commits are listed (and only those) and to verify that they are ordered by\n> the distance, in descending order.\n>\n> And this trick, while it still makes the test correct and stable and yadda\n> yadda, *also* makes this trick a lot less readable than my version. And\n> therefore makes it more difficult to verify that your test actually does\n> what it is supposed to do.\n>\n> Ciao,\n> Dscho\n\nHello,\n\nfirst of all I would like to thank all the feedback provided in this patch it\nhas truly helped me progress on my first contribution to git.\n\nAfter looking through Junio's and Johannes's suggestions I believe that\nthe *only* test we should add will be something like:\n\n-- snip --\ntest_expect_success '--first-parent --bisect-all lists correct revs' '\ngit rev-list --first-parent --bisect-all F..E >revs &&\ntest_line_count = 9 revs &&\nfor rev in E e1 e2 e3 e4 e5 e6 e7 e8\ndo\n  grep \"^$(git rev-parse $rev) \" revs ||\n  {\n    echo \"$rev not shown\" >&2 &&\n    return 1\n  }\ndone &&\nsed -e \"s/.*(dist=\\([0-9]*\\)).*/\\1/\" revs >actual.dists &&\nsort -r actual.dists >actual.dists.sorted &&\ntest_cmp actual.dists.sorted actual.dists\n'\n-- snap --\n\nThe only change I had to make was use -r in sort to\nrevert the sorting in `sort` otherwise we get it in\nascending order but the revs are provided in descending order.\n\nKind regards,\nTiago\n"},{"id":"351669","messageId":"xmqq36x0ndza.fsf@gitster-ct.c.googlers.com","threadId":"48763","inReplyTo":"CAADF+x3jd5G9+SP3UmhwqrR_T6BuD0PkQJ3x+NLpq2BJ_Ej-Sw@mail.gmail.com","subject":"Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-03T22:36:41Z","receivedAt":"2018-07-03T22:36:46Z","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> git rev-list --first-parent --bisect-all F..E >revs &&\n> test_line_count = 9 revs &&\n> for rev in E e1 e2 e3 e4 e5 e6 e7 e8\n> do\n>   grep \"^$(git rev-parse $rev) \" revs ||\n>   {\n>     echo \"$rev not shown\" >&2 &&\n>     return 1\n>   }\n> done &&\n> sed -e \"s/.*(dist=\\([0-9]*\\)).*/\\1/\" revs >actual.dists &&\n> sort -r actual.dists >actual.dists.sorted &&\n> test_cmp actual.dists.sorted actual.dists\n\nThe distance in the current graph might be all lower single-digits,\nbut you need \"sort -n -r\" to be future-proof, as dist=10 would sort\nnext to dist=1, perhaps in between dist=1 and dist=2.\n\nAnother thing you missed in my message is that the above does not\nsay what distance value each commit should be assigned in the\nhistory.  Even though the grep loop makes sure that each of E, e1,\n... e8 appears at least once, and line-count before that ensures\nthat the output has 9 entries (and taken together, it guarantees\nthat each of these appears not just \"at least once\", but also\nexactly once), nothing guarantees if they are getting relative\ndistance correctly, as the sed strips a bit too much (and that\nrelates to my earlier point why starting from a concrete expected\noutput and explicitly discard the aspect of the output we do not\ncare about before comparison---that way, we can easily tell when the\ncode is _designed to_ discard too much).\n\n"},{"id":"351687","messageId":"nycvar.QRO.7.76.6.1807041224460.75@tvgsbejvaqbjf.bet","threadId":"48763","inReplyTo":"xmqq36x0ndza.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-04T10:26:32Z","receivedAt":"2018-07-04T10:26:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 3 Jul 2018, Junio C Hamano wrote:\n\n> Tiago Botelho <tiagonbotelho@gmail.com> writes:\n> \n> > git rev-list --first-parent --bisect-all F..E >revs &&\n> > test_line_count = 9 revs &&\n> > for rev in E e1 e2 e3 e4 e5 e6 e7 e8\n> > do\n> >   grep \"^$(git rev-parse $rev) \" revs ||\n> >   {\n> >     echo \"$rev not shown\" >&2 &&\n> >     return 1\n> >   }\n> > done &&\n> > sed -e \"s/.*(dist=\\([0-9]*\\)).*/\\1/\" revs >actual.dists &&\n> > sort -r actual.dists >actual.dists.sorted &&\n> > test_cmp actual.dists.sorted actual.dists\n> \n> The distance in the current graph might be all lower single-digits,\n> but you need \"sort -n -r\" to be future-proof, as dist=10 would sort\n> next to dist=1, perhaps in between dist=1 and dist=2.\n> \n> Another thing you missed in my message is that the above does not\n> say what distance value each commit should be assigned in the\n> history.  Even though the grep loop makes sure that each of E, e1,\n> ... e8 appears at least once, and line-count before that ensures\n> that the output has 9 entries (and taken together, it guarantees\n> that each of these appears not just \"at least once\", but also\n> exactly once), nothing guarantees if they are getting relative\n> distance correctly, as the sed strips a bit too much (and that\n> relates to my earlier point why starting from a concrete expected\n> output and explicitly discard the aspect of the output we do not\n> care about before comparison---that way, we can easily tell when the\n> code is _designed to_ discard too much).\n\nFrom my point of view, this indicates that you want to set those exact\ndist values in stone. I am not sure I follow that reasoning, as this\nparticular test case's purpose is to ensure that the `--first-parent`\noption works with `--bisect`, not that the distance values match a fixed\nexpectation.\n\nIn short: I disagree that the *exact* values (beyond testing the order)\nshould be tested *here*.\n\nCiao,\nDscho\n"},{"id":"351787","messageId":"xmqqwou8kz9b.fsf@gitster-ct.c.googlers.com","threadId":"48763","inReplyTo":"nycvar.QRO.7.76.6.1807041224460.75@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-06T18:14:24Z","receivedAt":"2018-07-06T18:14:30Z","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>> > git rev-list --first-parent --bisect-all F..E >revs &&\n>> > test_line_count = 9 revs &&\n>> > for rev in E e1 e2 e3 e4 e5 e6 e7 e8\n>> > do\n>> >   grep \"^$(git rev-parse $rev) \" revs ||\n>> >   {\n>> >     echo \"$rev not shown\" >&2 &&\n>> >     return 1\n>> >   }\n>> > done &&\n>> > sed -e \"s/.*(dist=\\([0-9]*\\)).*/\\1/\" revs >actual.dists &&\n>> > sort -r actual.dists >actual.dists.sorted &&\n>> > test_cmp actual.dists.sorted actual.dists\n>> \n> From my point of view, this indicates that you want to set those exact\n> dist values in stone.\n\nAs I already said, I do not think it is absolutely necessary to\ndeclare that the minimum dist is 0 or 1, or how big one step of dist\nis.  For those reading from the sidelines, the history we are\ntesting this new feature over looks like this\n\n#     E\t\tdist=0\n#    / \\\n#   e1  |\tdist=1\n#   |   |\n#   e2  |\tdist=2\n#   |   |       ...\n#   |   |       ...\n#   e7  |\tdist=2\n#   |   |\n#   e8  |\tdist=1\n#    \\ /\n#     F\t\tdist=0\n\nCurrent code will say dist=0 for E and F, dist=1 for e1 and e8,\netc., and I am fine if the code suddenly start saying that E and F\n(i.e. those at the boundary of the graph) have dist=1 and one hop\nweighs 10 so dist=11 for e1 and e8 (i.e. those at one hop from the\nboundary).\n\nBut I am not fine if E and F get larger dist than e1 and e8, or e1\nand e8 get different ones.  I do not think the code quoted upfront\nwould catch such future breakages.\n\nAnd I also do not see a reason why somebody wants to make the dist\ncomputation to be 1-based (iow, changing the minimum from 0 to 1) or\none step not to be 1 (iow, giving 11 to e1 and e8), so while I agree\nit is not strictly necessary to cast the concrete distance value in\nstone, I do not see much harm doing so *if* it helps to make it\nsimpler the test that is necessary to make sure relative dist values\nassigned to these commits are in correct order.\n"},{"id":"351810","messageId":"nycvar.QRO.7.76.6.1807062123280.75@tvgsbejvaqbjf.bet","threadId":"48763","inReplyTo":"xmqqwou8kz9b.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-07-06T20:33:56Z","receivedAt":"2018-07-06T20:34:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 6 Jul 2018, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> >> > git rev-list --first-parent --bisect-all F..E >revs &&\n> >> > test_line_count = 9 revs &&\n> >> > for rev in E e1 e2 e3 e4 e5 e6 e7 e8\n> >> > do\n> >> >   grep \"^$(git rev-parse $rev) \" revs ||\n> >> >   {\n> >> >     echo \"$rev not shown\" >&2 &&\n> >> >     return 1\n> >> >   }\n> >> > done &&\n> >> > sed -e \"s/.*(dist=\\([0-9]*\\)).*/\\1/\" revs >actual.dists &&\n> >> > sort -r actual.dists >actual.dists.sorted &&\n> >> > test_cmp actual.dists.sorted actual.dists\n> >> \n> > From my point of view, this indicates that you want to set those exact\n> > dist values in stone.\n> \n> As I already said, I do not think it is absolutely necessary to\n> declare that the minimum dist is 0 or 1, or how big one step of dist\n> is.  For those reading from the sidelines, the history we are\n> testing this new feature over looks like this\n> \n> #     E\t\tdist=0\n> #    / \\\n> #   e1  |\tdist=1\n> #   |   |\n> #   e2  |\tdist=2\n> #   |   |       ...\n> #   |   |       ...\n> #   e7  |\tdist=2\n> #   |   |\n> #   e8  |\tdist=1\n> #    \\ /\n> #     F\t\tdist=0\n> \n> Current code will say dist=0 for E and F, dist=1 for e1 and e8,\n> etc., and I am fine if the code suddenly start saying that E and F\n> (i.e. those at the boundary of the graph) have dist=1 and one hop\n> weighs 10 so dist=11 for e1 and e8 (i.e. those at one hop from the\n> boundary).\n> \n> But I am not fine if E and F get larger dist than e1 and e8, or e1\n> and e8 get different ones.  I do not think the code quoted upfront\n> would catch such future breakages.\n> \n> And I also do not see a reason why somebody wants to make the dist\n> computation to be 1-based (iow, changing the minimum from 0 to 1) or\n> one step not to be 1 (iow, giving 11 to e1 and e8), so while I agree\n> it is not strictly necessary to cast the concrete distance value in\n> stone, I do not see much harm doing so *if* it helps to make it\n> simpler the test that is necessary to make sure relative dist values\n> assigned to these commits are in correct order.\n\nI guess that you still want to misunderstand me.\n\nBecause it is not really hard to understand what I said: a good regression\ntest will verify precisely what you want to ensure, not precisely what the\ncommand's output is.\n\nSo in this case, quite obviously what you want to do is to verify that E\nand F get larger dist than e1 and e8. So that is what you test for. Not\nsome fixed text that might require adjusting in the future for any other\nreason than a real bug.\n\nCiao,\nDscho\n"},{"id":"351823","messageId":"xmqqfu0whwgo.fsf@gitster-ct.c.googlers.com","threadId":"48763","inReplyTo":"nycvar.QRO.7.76.6.1807062123280.75@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-06T21:43:03Z","receivedAt":"2018-07-06T21:43:09Z","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>> And I also do not see a reason why somebody wants to make the dist\n>> computation to be 1-based (iow, changing the minimum from 0 to 1) or\n>> one step not to be 1 (iow, giving 11 to e1 and e8), so while I agree\n>> it is not strictly necessary to cast the concrete distance value in\n>> stone, I do not see much harm doing so *if* it helps to make it\n>> simpler the test that is necessary to make sure relative dist values\n>> assigned to these commits are in correct order.\n>\n> I guess that you still want to misunderstand me.\n\nNot at all.  \n\n> So in this case, quite obviously what you want to do is to verify that E\n> and F get larger dist than e1 and e8. So that is what you test for. Not\n> some fixed text that might require adjusting in the future for any other\n> reason than a real bug.\n\nThe most important part of the above quote is \"*if* it helps...\"\npart, and I do think the downside of insisting on dist being exactly\n0 for E and F is acceptable than having to write the \"E and F must\nget the same dist value, and e1 and e8 must get the same dist value\nthat is larger than the value that E and F gets, ...\" test without\ndoing so *and* still keep the resulting test readable.  It is a\ntradeoff and we are drawing the line differently (I am being more\npractical here than insisting on requiring absolute minimum and\nnothing more).\n\n\n\n"}]}