{"thread":{"id":"38688","subject":"[BUG] Segfault with rev-list --bisect","startedAt":"2015-03-03T14:19:14Z","lastAt":"2015-03-21T22:01:44Z","messageCount":32,"participants":["Troy Moure","Jeff King","Junio C Hamano","Kevin Daudt","Eric Sunshine","Philip Oakley","Christian Couder","Scott Schmit"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"256903","messageId":"CAMo-WNYNeShbbhNfG455o7krGfY7_9zVU3dMpJ7b4Smh_AiATg@mail.gmail.com","threadId":"38688","inReplyTo":null,"subject":"[BUG] Segfault with rev-list --bisect","fromName":"Troy Moure","fromEmail":"troy.moure@gmail.com","sentAt":"2015-03-03T14:19:14Z","receivedAt":"2015-03-03T14:19:14Z","isPatch":false,"sender":{"key":"troy.moure@gmail.com","avatar":null},"body":"Hi,\n\nI've found a case where git rev-list --bisect segfaults reproducibly\n(git version is 2.3.1). This is the commit topology (A2 is the first\nparent of M):\n\nI - A1 - A2\n \\ \\\n - B1 -- M (HEAD)\n\nAnd this is an example of a command that segfaults:\n\ngit rev-list --bisect --first-parent --parents HEAD --not HEAD~1\n\nI tried a couple of variations quickly: It does not segfault if a\nnon-merge commit is made on top of M (so HEAD is no longer pointing\ndirectly to M). It also does not segfault if 'HEAD~1' is changed to\n'HEAD~2'.\n\nThanks,\nTroy\n"},{"id":"256961","messageId":"20150304053333.GA9584@peff.net","threadId":"38688","inReplyTo":"CAMo-WNYNeShbbhNfG455o7krGfY7_9zVU3dMpJ7b4Smh_AiATg@mail.gmail.com","subject":"Re: [BUG] Segfault with rev-list --bisect","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-04T05:33:33Z","receivedAt":"2015-03-04T05:33:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 03, 2015 at 09:19:14AM -0500, Troy Moure wrote:\n\n> I've found a case where git rev-list --bisect segfaults reproducibly\n> (git version is 2.3.1). This is the commit topology (A2 is the first\n> parent of M):\n> \n> I - A1 - A2\n> \\ \\\n> - B1 -- M (HEAD)\n\nThanks for finding a simple history which shows the problem. I recreated\nthis with:\n\n git init repo && cd repo &&\n echo I >I && git add I && git commit -m I &&\n echo A1 >A && git add A && git commit -m A1 &&\n echo A2 >A && git add A && git commit -m A2 &&\n git checkout -b side HEAD~2 &&\n echo B1 >B && git add B && git commit -m B1 &&\n git checkout master &&\n GIT_EDITOR=: git merge side\n\nand was able to reproduce the segfault with:\n\n git rev-list --bisect --first-parent HEAD --not HEAD~1\n\n(it drops --parents from your command, which is not relevant to the\nsegfault). The segfault itself happens because we try to access the\nweight() of B1, even though we never called weight_set() on it.\n\nAnd that, I think, is related to --first-parent. We do not set a weight\nbecause B1 is not an interesting commit to us (it is accessible only as\na second parent). I am not too familiar with the bisect code, but it\nlooks like it is not really ready to handle --first-parent. There are\nseveral spots where it enumerates the parent list, which is going to\nexamine parents other than the first.\n\nBelow is a fairly hacky patch to respect --first-parent through the\nbisection code. Like I said, I'm not very familiar with this code, so I\nbasically just blindly limited any traversal of commit->parents. It does\nsolve this particular segfault, but I have no clue if it is fixing other\nbugs introducing them. :) E.g., it's changing count_distance(), so\nperhaps our bisection counts were all off with --first-parent, even when\nit didn't segfault?\n\ndiff --git a/bisect.c b/bisect.c\nindex 8c6d843..c51f37a 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -31,7 +31,7 @@ static const char *argv_update_ref[] = {\"update-ref\", \"--no-deref\", \"BISECT_HEAD\n * We care just barely enough to avoid recursing for\n * non-merge entries.\n */\n-static int count_distance(struct commit_list *entry)\n+static int count_distance(struct commit_list *entry, int max_parents)\n {\n \tint nr = 0;\n \n@@ -47,9 +47,10 @@ static int count_distance(struct commit_list *entry)\n \t\tp = commit->parents;\n \t\tentry = p;\n \t\tif (p) {\n+\t\t\tint n = max_parents - 1;\n \t\t\tp = p->next;\n-\t\t\twhile (p) {\n-\t\t\t\tnr += count_distance(p);\n+\t\t\twhile (p && n-- > 0) {\n+\t\t\t\tnr += count_distance(p, max_parents);\n \t\t\t\tp = p->next;\n \t\t\t}\n \t\t}\n@@ -79,12 +80,12 @@ 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, int max_parents)\n {\n \tstruct commit_list *p;\n \tint count;\n \n-\tfor (count = 0, p = commit->parents; p; p = p->next) {\n+\tfor (count = 0, p = commit->parents; p && max_parents-- > 0; p = p->next) {\n \t\tif (p->item->object.flags & UNINTERESTING)\n \t\t\tcontinue;\n \t\tcount++;\n@@ -117,7 +118,7 @@ static inline int halfway(struct commit_list *p, int nr)\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+\t\t struct commit_list *list, int max_parents)\n {\n \tstruct commit_list *p;\n \n@@ -132,6 +133,7 @@ static void show_list(const char *debug, int counted, int nr,\n \t\tchar *buf = read_sha1_file(commit->object.sha1, &type, &size);\n \t\tconst char *subject_start;\n \t\tint subject_len;\n+\t\tint n = max_parents;\n \n \t\tfprintf(stderr, \"%c%c%c \",\n \t\t\t(flags & TREESAME) ? ' ' : 'T',\n@@ -142,7 +144,7 @@ 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, sha1_to_hex(commit->object.sha1));\n-\t\tfor (pp = commit->parents; pp; pp = pp->next)\n+\t\tfor (pp = commit->parents; pp && n-- > 0; pp = pp->next)\n \t\t\tfprintf(stderr, \" %.*s\", 8,\n \t\t\t\tsha1_to_hex(pp->item->object.sha1));\n \n@@ -245,7 +247,7 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n\n */\n static struct commit_list *do_find_bisection(struct commit_list *list,\n \t\t\t\t\t int nr, int *weights,\n-\t\t\t\t\t int find_all)\n+\t\t\t\t\t int find_all, int max_parents)\n {\n \tint n, counted;\n \tstruct commit_list *p;\n@@ -257,7 +259,7 @@ 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, max_parents)) {\n \t\tcase 0:\n \t\t\tif (!(flags & TREESAME)) {\n \t\t\t\tweight_set(p, 1);\n@@ -300,7 +302,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,\n \t\t\tcontinue;\n \t\tif (weight(p) != -2)\n \t\t\tcontinue;\n-\t\tweight_set(p, count_distance(p));\n+\t\tweight_set(p, count_distance(p, max_parents));\n \t\tclear_distance(list);\n \n \t\t/* Does it happen to be at exactly half-way? */\n@@ -315,10 +317,11 @@ static struct commit_list *do_find_bisection(struct commit_list *list,\n \t\tfor (p = list; p; p = p->next) {\n \t\t\tstruct commit_list *q;\n \t\t\tunsigned flags = p->item->object.flags;\n+\t\t\tint n = max_parents;\n \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\tfor (q = p->item->parents; q && n-- > 0; q = q->next) {\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@@ -357,11 +360,12 @@ static struct commit_list *do_find_bisection(struct commit_list *list,\n \n struct commit_list *find_bisection(struct commit_list *list,\n \t\t\t\t\t int *reaches, int *all,\n-\t\t\t\t\t int find_all)\n+\t\t\t\t\t int find_all, int first_parent_only)\n {\n \tint nr, on_list;\n \tstruct commit_list *p, *best, *next, *last;\n \tint *weights;\n+\tint max_parents = first_parent_only ? 1 : INT_MAX;\n \n \tshow_list(\"bisection 2 entry\", 0, 0, list);\n \n@@ -390,7 +394,7 @@ struct commit_list *find_bisection(struct commit_list *list,\n \tweights = xcalloc(on_list, sizeof(*weights));\n \n \t/* Do the real work of finding bisection commit. */\n-\tbest = do_find_bisection(list, nr, weights, find_all);\n+\tbest = do_find_bisection(list, nr, weights, find_all, max_parents);\n \tif (best) {\n \t\tif (!find_all)\n \t\t\tbest->next = NULL;\n@@ -916,7 +920,8 @@ int bisect_next_all(const char *prefix, int no_checkout)\n \tbisect_common(&revs);\n \n \trevs.commits = find_bisection(revs.commits, &reaches, &all,\n-\t\t\t\t !!skipped_revs.nr);\n+\t\t\t\t !!skipped_revs.nr,\n+\t\t\t\t revs.first_parent_only);\n \trevs.commits = managed_skipped(revs.commits, &tried);\n \n \tif (!revs.commits) {\ndiff --git a/bisect.h b/bisect.h\nindex 2a6c831..03d04d1 100644\n--- a/bisect.h\n+++ b/bisect.h\n@@ -3,7 +3,7 @@\n \n extern struct commit_list *find_bisection(struct commit_list *list,\n \t\t\t\t\t int *reaches, int *all,\n-\t\t\t\t\t int find_all);\n+\t\t\t\t\t int find_all, int first_parent_only);\n \n extern struct commit_list *filter_skipped(struct commit_list *list,\n \t\t\t\t\t struct commit_list **tried,\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ff84a82..3f531d6 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -380,7 +380,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\tint reaches = reaches, all = all;\n \n \t\trevs.commits = find_bisection(revs.commits, &reaches, &all,\n-\t\t\t\t\t bisect_find_all);\n+\t\t\t\t\t bisect_find_all,\n+\t\t\t\t\t revs.first_parent_only);\n \n \t\tif (bisect_show_vars)\n \t\t\treturn show_bisect_vars(&info, reaches, all);\n"},{"id":"257034","messageId":"xmqq61ag72gc.fsf@gitster.dls.corp.google.com","threadId":"38688","inReplyTo":"CAMo-WNYNeShbbhNfG455o7krGfY7_9zVU3dMpJ7b4Smh_AiATg@mail.gmail.com","subject":"Re: [BUG] Segfault with rev-list --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-04T23:44:19Z","receivedAt":"2015-03-04T23:44:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Troy Moure writes:\n\n> git rev-list --bisect --first-parent --parents HEAD --not HEAD~1\n\nHmm, as \"rev-list --bisect\" is not end-user facing command (it is\npurely an implementation detail for \"git bisect\") and we never call\nit with --first-parent, I am not sure if it is worth labelling it as\na BUG. Surely, the command can refuse to operate when it sees both\noptions given, but that would be a fairly low priority.\n\nOf course, if you are planning to do \"git bisect --first-parent\", it\nis one of the things that needs to be addressed, together with\ncounting the rounds and bisecting the linear set of commits on the\nfirst-parent chain correctly.\n"},{"id":"257050","messageId":"CAMo-WNaS-at4oE2xS-07O=7R4VTXLrPeDJ84c_HbikXhN-W99g@mail.gmail.com","threadId":"38688","inReplyTo":"xmqq61ag72gc.fsf@gitster.dls.corp.google.com","subject":"Re: [BUG] Segfault with rev-list --bisect","fromName":"Troy Moure","fromEmail":"troy.moure@gmail.com","sentAt":"2015-03-05T02:15:31Z","receivedAt":"2015-03-05T02:15:31Z","isPatch":false,"sender":{"key":"troy.moure@gmail.com","avatar":null},"body":"On Wed, Mar 4, 2015 at 6:44 PM, Junio C Hamano wrote:\n> Troy Moure writes:\n>\n>> git rev-list --bisect --first-parent --parents HEAD --not HEAD~1\n>\n> Hmm, as \"rev-list --bisect\" is not end-user facing command (it is\n> purely an implementation detail for \"git bisect\") and we never call\n> it with --first-parent, I am not sure if it is worth labelling it as\n> a BUG. Surely, the command can refuse to operate when it sees both\n> options given, but that would be a fairly low priority.\n\nHrm, ok. I didn't realize \"--bisect\" is only intended to be used by git-bisect\n(although I suppose the fact that it treats ref/bisect/* specially should have\nbeen a hint). If uses of \"--bisect\" other than by git-bisect are considered\nunsupported, IMO it would be good to say that in the documentation - right now\nit looks like just another rev-list parameter. (I realize rev-list itself is\n\"plumbing\", but that's not the same as \"not user facing\", is it?)\n\nIf you're curious, I ran into this because I am working on a script that can be\nrun repeatedly to process commits, and uses git notes to mark commits that have\nbeen processed. Parents are always processed before their children, so if a\ncommit has a note, it means all its ancestors also have notes. I want to\nquickly find the set of commits that have not yet been processed. I am thinking\nof finding the \"boundary\" commits (commits that have a note and at least one\nchild that does not) by using a binary search to find the boundary commit on\nthe first-parent chain, and then recursively doing the same thing starting from\neach non-first parent of each merge commit between the boundary commit and the\nstarting point.\n\nUpon further thought, it's probably better to just read the whole first-parent\nchain and do the binary search in the script, since \"git rev-list --bisect\"\nwould have generate the chain each time it's called. But I'd already run into\nthe segfault, so I thought I'd report it.\n\nOf course, I'd appreciate any thoughts or comments on the problem I'm trying to\nsolve as well.\n\nThanks,\nTroy\n"},{"id":"257238","messageId":"1425763876-15573-1-git-send-email-me@ikke.info","threadId":"38688","inReplyTo":"xmqq61ag72gc.fsf@gitster.dls.corp.google.com","subject":"[PATCH] rev-list: refuse --first-parent combined with --bisect","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2015-03-07T21:31:16Z","receivedAt":"2015-03-07T21:31:16Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"rev-list --bisect is used by git bisect, but never together with\n--first-parent. Because rev-list --bisect together with --first-parent\nis not handled currently, and even leads to segfaults, refuse to use\nboth options together.\n\nSigned-off-by: Kevin Daudt \n---\nThis is my first code patch, and thought this was a nice exercise.\n\n Documentation/rev-list-options.txt | 3 ++-\n builtin/rev-list.c | 3 +++\n t/t6000-rev-list-misc.sh | 4 ++++\n 3 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 4ed8587..05c3f6d 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -123,7 +123,8 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).\n \tbecause merges into a topic branch tend to be only about\n \tadjusting to updated upstream from time to time, and\n \tthis option allows you to ignore the individual commits\n-\tbrought in to your history by such a merge.\n+\tbrought in to your history by such a merge. Cannot be\n+\tcombined with --bisect.\n \n --not::\n \tReverses the meaning of the '{caret}' prefix (or lack thereof)\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ff84a82..c271e15 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -291,6 +291,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tif (revs.bisect)\n \t\tbisect_list = 1;\n \n+\tif(revs.first_parent_only && revs.bisect)\n+\t\tdie(_(\"--first-parent is incompattible with --bisect\"));\n+\n \tif (DIFF_OPT_TST(&revs.diffopt, QUICK))\n \t\tinfo.flags |= REV_LIST_QUIET;\n \tfor (i = 1 ; i < argc; i++) {\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 2602086..1f58b46 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -96,4 +96,8 @@ test_expect_success 'rev-list can show index objects' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--bisect and --first-parent can not be combined' '\n+\ttest_must_fail git rev-list --bisect --first-parent HEAD\n+'\n+\n test_done\n-- \n2.3.1.184.g97c12a8.dirty\n"},{"id":"257243","messageId":"20150307231305.GA15619@vps892.directvps.nl","threadId":"38688","inReplyTo":"1425763876-15573-1-git-send-email-me@ikke.info","subject":"Re: [PATCH] rev-list: refuse --first-parent combined with --bisect","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2015-03-07T23:13:05Z","receivedAt":"2015-03-07T23:13:05Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Sat, Mar 07, 2015 at 10:31:16PM +0100, Kevin Daudt wrote:\n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index ff84a82..c271e15 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -291,6 +291,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n> \tif (revs.bisect)\n> \t\tbisect_list = 1;\n> \n> +\tif(revs.first_parent_only && revs.bisect)\n\nI should have added a space after the if.\n\n> +\t\tdie(_(\"--first-parent is incompattible with --bisect\"));\n> +\n> \tif (DIFF_OPT_TST(&revs.diffopt, QUICK))\n> \t\tinfo.flags |= REV_LIST_QUIET;\n> \tfor (i = 1 ; i < argc; i++) {\n"},{"id":"257277","messageId":"xmqq1tkzudf7.fsf@gitster.dls.corp.google.com","threadId":"38688","inReplyTo":"20150307231305.GA15619@vps892.directvps.nl","subject":"Re: [PATCH] rev-list: refuse --first-parent combined with --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-08T08:00:12Z","receivedAt":"2015-03-08T08:00:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Daudt writes:\n\n> On Sat, Mar 07, 2015 at 10:31:16PM +0100, Kevin Daudt wrote:\n>> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n>> index ff84a82..c271e15 100644\n>> --- a/builtin/rev-list.c\n>> +++ b/builtin/rev-list.c\n>> @@ -291,6 +291,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n>> \tif (revs.bisect)\n>> \t\tbisect_list = 1;\n>> \n>> +\tif(revs.first_parent_only && revs.bisect)\n>\n> I should have added a space after the if.\n\nSince you are practicing, let me say that a better way to do this is\nto reroll the whole patch and have that comment after the three-dash\nline.\n\nThat is, you respond to your message with a new patch that corrects\nthe above, and where you said \"This is my first code patch, and\nthought this was a nice exercise.\" in your first message, you would\nsay\n\n ---\n\n * changes from v1: corrected coding guideline violation that\n missed a SP between \"if(\"\n\nor something like that.\n\nThanks.\n"},{"id":"257322","messageId":"1425824339-8036-1-git-send-email-me@ikke.info","threadId":"38688","inReplyTo":"1425763876-15573-1-git-send-email-me@ikke.info","subject":"[PATCH v2] rev-list: refuse --first-parent combined with --bisect","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2015-03-08T14:18:59Z","receivedAt":"2015-03-08T14:18:59Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"rev-list --bisect is used by git bisect, but never together with\n--first-parent. Because rev-list --bisect together with --first-parent\nis not handled currently, and even leads to segfaults, refuse to use\nboth options together.\n\nSigned-off-by: Kevin Daudt \nSuggested-by: Junio C. Hamano \n---\n* Changes from v1: Added the missing SP between \"if(\",\n as per the code guidelines\n\nThanks for the feedback.\n\n Documentation/rev-list-options.txt | 3 ++-\n builtin/rev-list.c | 3 +++\n t/t6000-rev-list-misc.sh | 4 ++++\n 3 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 4ed8587..05c3f6d 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -123,7 +123,8 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).\n \tbecause merges into a topic branch tend to be only about\n \tadjusting to updated upstream from time to time, and\n \tthis option allows you to ignore the individual commits\n-\tbrought in to your history by such a merge.\n+\tbrought in to your history by such a merge. Cannot be\n+\tcombined with --bisect.\n \n --not::\n \tReverses the meaning of the '{caret}' prefix (or lack thereof)\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ff84a82..c271e15 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -291,6 +291,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tif (revs.bisect)\n \t\tbisect_list = 1;\n \n+\tif(revs.first_parent_only && revs.bisect)\n+\t\tdie(_(\"--first-parent is incompattible with --bisect\"));\n+\n \tif (DIFF_OPT_TST(&revs.diffopt, QUICK))\n \t\tinfo.flags |= REV_LIST_QUIET;\n \tfor (i = 1 ; i < argc; i++) {\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 2602086..1f58b46 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -96,4 +96,8 @@ test_expect_success 'rev-list can show index objects' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--bisect and --first-parent can not be combined' '\n+\ttest_must_fail git rev-list --bisect --first-parent HEAD\n+'\n+\n test_done\n-- \n2.3.1.184.g97c12a8.dirty\n"},{"id":"257326","messageId":"1425826943-9535-1-git-send-email-me@ikke.info","threadId":"38688","inReplyTo":"1425824339-8036-1-git-send-email-me@ikke.info","subject":"[PATCH v3] rev-list: refuse --first-parent combined with --bisect","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2015-03-08T15:02:23Z","receivedAt":"2015-03-08T15:02:23Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"rev-list --bisect is used by git bisect, but never together with\n--first-parent. Because rev-list --bisect together with --first-parent\nis not handled currently, and even leads to segfaults, refuse to use\nboth options together.\n\nSigned-off-by: Kevin Daudt \nSuggested-by: Junio C. Hamano \n---\nSorry for the false fix reroll in v2, forgot to actually commit the change.\n\n* Changes from v2: Added the missing SP between \"if(\",\n as per the code guidelines\")\" (Now with the actual change)\n\n Documentation/rev-list-options.txt | 3 ++-\n builtin/rev-list.c | 3 +++\n t/t6000-rev-list-misc.sh | 4 ++++\n 3 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 4ed8587..05c3f6d 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -123,7 +123,8 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).\n \tbecause merges into a topic branch tend to be only about\n \tadjusting to updated upstream from time to time, and\n \tthis option allows you to ignore the individual commits\n-\tbrought in to your history by such a merge.\n+\tbrought in to your history by such a merge. Cannot be\n+\tcombined with --bisect.\n \n --not::\n \tReverses the meaning of the '{caret}' prefix (or lack thereof)\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ff84a82..f5da2a4 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -291,6 +291,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tif (revs.bisect)\n \t\tbisect_list = 1;\n \n+\tif (revs.first_parent_only && revs.bisect)\n+\t\tdie(_(\"--first-parent is incompattible with --bisect\"));\n+\n \tif (DIFF_OPT_TST(&revs.diffopt, QUICK))\n \t\tinfo.flags |= REV_LIST_QUIET;\n \tfor (i = 1 ; i < argc; i++) {\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 2602086..1f58b46 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -96,4 +96,8 @@ test_expect_success 'rev-list can show index objects' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--bisect and --first-parent can not be combined' '\n+\ttest_must_fail git rev-list --bisect --first-parent HEAD\n+'\n+\n test_done\n-- \n2.3.1.184.g97c12a8.dirty\n"},{"id":"257327","messageId":"1425827005-9602-1-git-send-email-me@ikke.info","threadId":"38688","inReplyTo":"1425824339-8036-1-git-send-email-me@ikke.info","subject":"[PATCH v3] rev-list: refuse --first-parent combined with --bisect","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2015-03-08T15:03:25Z","receivedAt":"2015-03-08T15:03:25Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"rev-list --bisect is used by git bisect, but never together with\n--first-parent. Because rev-list --bisect together with --first-parent\nis not handled currently, and even leads to segfaults, refuse to use\nboth options together.\n\nSigned-off-by: Kevin Daudt \nSuggested-by: Junio C. Hamano \n---\nSorry for the false fix reroll in v2, forgot to actually commit the change.\n\n* Changes from v2: Added the missing SP between \"if(\",\n as per the code guidelines\")\" (Now with the actual change)\n\n Documentation/rev-list-options.txt | 3 ++-\n builtin/rev-list.c | 3 +++\n t/t6000-rev-list-misc.sh | 4 ++++\n 3 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 4ed8587..05c3f6d 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -123,7 +123,8 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).\n \tbecause merges into a topic branch tend to be only about\n \tadjusting to updated upstream from time to time, and\n \tthis option allows you to ignore the individual commits\n-\tbrought in to your history by such a merge.\n+\tbrought in to your history by such a merge. Cannot be\n+\tcombined with --bisect.\n \n --not::\n \tReverses the meaning of the '{caret}' prefix (or lack thereof)\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ff84a82..f5da2a4 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -291,6 +291,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tif (revs.bisect)\n \t\tbisect_list = 1;\n \n+\tif (revs.first_parent_only && revs.bisect)\n+\t\tdie(_(\"--first-parent is incompattible with --bisect\"));\n+\n \tif (DIFF_OPT_TST(&revs.diffopt, QUICK))\n \t\tinfo.flags |= REV_LIST_QUIET;\n \tfor (i = 1 ; i < argc; i++) {\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 2602086..1f58b46 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -96,4 +96,8 @@ test_expect_success 'rev-list can show index objects' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--bisect and --first-parent can not be combined' '\n+\ttest_must_fail git rev-list --bisect --first-parent HEAD\n+'\n+\n test_done\n-- \n2.3.1.184.g97c12a8.dirty\n"},{"id":"257345","messageId":"CAPig+cROEyWvJDW7uf1D7owdL-FwLHMtEBwWSNwS1M=vMcozLQ@mail.gmail.com","threadId":"38688","inReplyTo":"1425827005-9602-1-git-send-email-me@ikke.info","subject":"Re: [PATCH v3] rev-list: refuse --first-parent combined with --bisect","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-08T21:58:24Z","receivedAt":"2015-03-08T21:58:24Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 8, 2015 at 11:03 AM, Kevin Daudt wrote:\n> rev-list --bisect is used by git bisect, but never together with\n> --first-parent. Because rev-list --bisect together with --first-parent\n> is not handled currently, and even leads to segfaults, refuse to use\n> both options together.\n>\n> Signed-off-by: Kevin Daudt \n> Suggested-by: Junio C. Hamano \n\nIt's customary for your sign-off to be last.\n\n> ---\n> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> index 4ed8587..05c3f6d 100644\n> --- a/Documentation/rev-list-options.txt\n> +++ b/Documentation/rev-list-options.txt\n> @@ -123,7 +123,8 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).\n> because merges into a topic branch tend to be only about\n> adjusting to updated upstream from time to time, and\n> this option allows you to ignore the individual commits\n> - brought in to your history by such a merge.\n> + brought in to your history by such a merge. Cannot be\n> + combined with --bisect.\n\nA couple questions:\n\nShould the documentation for ---bisect be updated to mention this\nrestriction also?\n\nShould this change be protected by a \"ifndef::git-rev-list[]\" as are\nall other mentions of \"bisect\" in rev-list-options.txt?\n\n> --not::\n> Reverses the meaning of the '{caret}' prefix (or lack thereof)\n"},{"id":"257394","messageId":"20150309115733.GB6273@vps892.directvps.nl","threadId":"38688","inReplyTo":"CAPig+cROEyWvJDW7uf1D7owdL-FwLHMtEBwWSNwS1M=vMcozLQ@mail.gmail.com","subject":"Re: [PATCH v3] rev-list: refuse --first-parent combined with --bisect","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2015-03-09T11:57:33Z","receivedAt":"2015-03-09T11:57:33Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Sun, Mar 08, 2015 at 05:58:24PM -0400, Eric Sunshine wrote:\n> On Sun, Mar 8, 2015 at 11:03 AM, Kevin Daudt wrote:\n> > rev-list --bisect is used by git bisect, but never together with\n> > --first-parent. Because rev-list --bisect together with --first-parent\n> > is not handled currently, and even leads to segfaults, refuse to use\n> > both options together.\n> >\n> > Signed-off-by: Kevin Daudt \n> > Suggested-by: Junio C. Hamano \n> \n> It's customary for your sign-off to be last.\n> \n\nOk, noted\n\n> > ---\n> > diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> > index 4ed8587..05c3f6d 100644\n> > --- a/Documentation/rev-list-options.txt\n> > +++ b/Documentation/rev-list-options.txt\n> > @@ -123,7 +123,8 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).\n> > because merges into a topic branch tend to be only about\n> > adjusting to updated upstream from time to time, and\n> > this option allows you to ignore the individual commits\n> > - brought in to your history by such a merge.\n> > + brought in to your history by such a merge. Cannot be\n> > + combined with --bisect.\n> \n> A couple questions:\n> \n> Should the documentation for ---bisect be updated to mention this\n> restriction also?\n\nWas doubting whether that was necessary as --bisect can be seen as a\nmode, and --first-parent modifying that mode. But it can make sense to\nalso add it to that section.\n\n> \n> Should this change be protected by a \"ifndef::git-rev-list[]\" as are\n> all other mentions of \"bisect\" in rev-list-options.txt?\n\nYes, I see why. git log also uses rev-list-options.txt and it has a\n--bisect option that is unrelated to this one, so that comment doesn't\nmake sense for git log.\n\nWill reroll this later.\n"},{"id":"257430","messageId":"1425934575-19581-1-git-send-email-me@ikke.info","threadId":"38688","inReplyTo":"1425827005-9602-1-git-send-email-me@ikke.info","subject":"[PATCH v4] rev-list: refuse --first-parent combined with --bisect","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2015-03-09T20:56:15Z","receivedAt":"2015-03-09T20:56:15Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"rev-list --bisect is used by git bisect, but never together with\n--first-parent. Because rev-list --bisect together with --first-parent\nis not handled currently, and even leads to segfaults, refuse to use\nboth options together.\n\nSuggested-by: Junio C. Hamano \nHelped-by: Eric Sunshine \nSigned-off-by: Kevin Daudt \n---\nChanges since v3:\n\n* Added an ifdef::git-rev-list[] guard around the warning in the\n --first-parent section so that it only shows up in `man git-rev-list`\n and not in `man git log`\n\n* Added the warning also to the --bisect section.\n\n Documentation/rev-list-options.txt | 4 ++++\n builtin/rev-list.c | 3 +++\n t/t6000-rev-list-misc.sh | 4 ++++\n 3 files changed, 11 insertions(+)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 4ed8587..a148672 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -124,6 +124,9 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).\n \tadjusting to updated upstream from time to time, and\n \tthis option allows you to ignore the individual commits\n \tbrought in to your history by such a merge.\n+ifdef::git-rev-list[]\n+\tCannot be combined with --bisect.\n+endif::git-rev-list[]\n \n --not::\n \tReverses the meaning of the '{caret}' prefix (or lack thereof)\n@@ -567,6 +570,7 @@ would be of roughly the same length. Finding the change which\n introduces a regression is thus reduced to a binary search: repeatedly\n generate and test new 'midpoint's until the commit chain is of length\n one.\n+Cannot be combined with --first-parent.\n \n --bisect-vars::\n \tThis calculates the same as `--bisect`, except that refs in\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex ff84a82..f5da2a4 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -291,6 +291,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tif (revs.bisect)\n \t\tbisect_list = 1;\n \n+\tif (revs.first_parent_only && revs.bisect)\n+\t\tdie(_(\"--first-parent is incompattible with --bisect\"));\n+\n \tif (DIFF_OPT_TST(&revs.diffopt, QUICK))\n \t\tinfo.flags |= REV_LIST_QUIET;\n \tfor (i = 1 ; i < argc; i++) {\ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 2602086..1f58b46 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -96,4 +96,8 @@ test_expect_success 'rev-list can show index objects' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--bisect and --first-parent can not be combined' '\n+\ttest_must_fail git rev-list --bisect --first-parent HEAD\n+'\n+\n test_done\n-- \n2.3.0\n"},{"id":"257501","messageId":"xmqqa8zkzeq5.fsf@gitster.dls.corp.google.com","threadId":"38688","inReplyTo":"1425934575-19581-1-git-send-email-me@ikke.info","subject":"Re: [PATCH v4] rev-list: refuse --first-parent combined with --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-10T22:09:54Z","receivedAt":"2015-03-10T22:09:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Daudt writes:\n\n> rev-list --bisect is used by git bisect, but never together with\n> --first-parent. Because rev-list --bisect together with --first-parent\n> is not handled currently, and even leads to segfaults, refuse to use\n> both options together.\n>\n> Suggested-by: Junio C. Hamano \n> Helped-by: Eric Sunshine \n> Signed-off-by: Kevin Daudt \n> ---\n> Changes since v3:\n>\n> * Added an ifdef::git-rev-list[] guard around the warning in the\n> --first-parent section so that it only shows up in `man git-rev-list`\n> and not in `man git log`\n>\n> * Added the warning also to the --bisect section.\n\nI wonder what \"git log --first-parent --bisect A..B\" should do,\nthough.\n\nWouldn't the rejection belong to revision.c::setup_revisions(),\nwhere we reject combined use of (--reverse, --walk-reflogs) and\n(--children, --parents), to apply this to all commands in the \"log\"\nfamily that uses the revision walker machinery?\n\n>\n> Documentation/rev-list-options.txt | 4 ++++\n> builtin/rev-list.c | 3 +++\n> t/t6000-rev-list-misc.sh | 4 ++++\n> 3 files changed, 11 insertions(+)\n>\n> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> index 4ed8587..a148672 100644\n> --- a/Documentation/rev-list-options.txt\n> +++ b/Documentation/rev-list-options.txt\n> @@ -124,6 +124,9 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).\n> \tadjusting to updated upstream from time to time, and\n> \tthis option allows you to ignore the individual commits\n> \tbrought in to your history by such a merge.\n> +ifdef::git-rev-list[]\n> +\tCannot be combined with --bisect.\n> +endif::git-rev-list[]\n> \n> --not::\n> \tReverses the meaning of the '{caret}' prefix (or lack thereof)\n> @@ -567,6 +570,7 @@ would be of roughly the same length. Finding the change which\n> introduces a regression is thus reduced to a binary search: repeatedly\n> generate and test new 'midpoint's until the commit chain is of length\n> one.\n> +Cannot be combined with --first-parent.\n> \n> --bisect-vars::\n> \tThis calculates the same as `--bisect`, except that refs in\n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index ff84a82..f5da2a4 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -291,6 +291,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n> \tif (revs.bisect)\n> \t\tbisect_list = 1;\n> \n> +\tif (revs.first_parent_only && revs.bisect)\n> +\t\tdie(_(\"--first-parent is incompattible with --bisect\"));\n> +\n> \tif (DIFF_OPT_TST(&revs.diffopt, QUICK))\n> \t\tinfo.flags |= REV_LIST_QUIET;\n> \tfor (i = 1 ; i < argc; i++) {\n> diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\n> index 2602086..1f58b46 100755\n> --- a/t/t6000-rev-list-misc.sh\n> +++ b/t/t6000-rev-list-misc.sh\n> @@ -96,4 +96,8 @@ test_expect_success 'rev-list can show index objects' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success '--bisect and --first-parent can not be combined' '\n> +\ttest_must_fail git rev-list --bisect --first-parent HEAD\n> +'\n> +\n> test_done\n"},{"id":"257511","messageId":"20150310225509.GA5442@vps892.directvps.nl","threadId":"38688","inReplyTo":"xmqqa8zkzeq5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4] rev-list: refuse --first-parent combined with --bisect","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2015-03-10T22:55:09Z","receivedAt":"2015-03-10T22:55:09Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Tue, Mar 10, 2015 at 03:09:54PM -0700, Junio C Hamano wrote:\n> Kevin Daudt writes:\n> \n> > rev-list --bisect is used by git bisect, but never together with\n> > --first-parent. Because rev-list --bisect together with --first-parent\n> > is not handled currently, and even leads to segfaults, refuse to use\n> > both options together.\n> >\n> > Suggested-by: Junio C. Hamano \n> > Helped-by: Eric Sunshine \n> > Signed-off-by: Kevin Daudt \n> > ---\n> > Changes since v3:\n> >\n> > * Added an ifdef::git-rev-list[] guard around the warning in the\n> > --first-parent section so that it only shows up in `man git-rev-list`\n> > and not in `man git log`\n> >\n> > * Added the warning also to the --bisect section.\n> \n> I wonder what \"git log --first-parent --bisect A..B\" should do,\n> though.\n> \n> Wouldn't the rejection belong to revision.c::setup_revisions(),\n> where we reject combined use of (--reverse, --walk-reflogs) and\n> (--children, --parents), to apply this to all commands in the \"log\"\n> family that uses the revision walker machinery?\n> \n\ngit log --bisect seems to do something different then git rev-list\n--bisect\n\n>From git-log(1):\n\n Pretend as if the bad bisection ref refs/bisect/bad was listed and\n as if it was followed by --not and the good bisection refs\n refs/bisect/good-* on the command line.\n\nThis seems to just add addition refs to the log command, which seems\nunrelated to what rev-list --bisect does.\n\nSo I don't see why git log --bisect --first-parent should be prohibited\n(unless this combination doesn't make sense on itself).\n\nKevin.\n"},{"id":"257514","messageId":"xmqqoao0xx9p.fsf@gitster.dls.corp.google.com","threadId":"38688","inReplyTo":"20150310225509.GA5442@vps892.directvps.nl","subject":"Re: [PATCH v4] rev-list: refuse --first-parent combined with --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-10T23:12:18Z","receivedAt":"2015-03-10T23:12:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Daudt writes:\n\n> git log --bisect seems to do something different then git rev-list\n> --bisect\n>\n> From git-log(1):\n>\n> Pretend as if the bad bisection ref refs/bisect/bad was listed and\n> as if it was followed by --not and the good bisection refs\n> refs/bisect/good-* on the command line.\n>\n> This seems to just add addition refs to the log command, which seems\n> unrelated to what rev-list --bisect does.\n>\n> So I don't see why git log --bisect --first-parent should be prohibited\n> (unless this combination doesn't make sense on itself).\n\nWell, but think if your \"unless\" holds true or not yourself first\nand then say \"I do not think this combination doesn't make sense\",\nif you truly mean \"I don't see why ... should be prohibited\".\n\nWhat does such a command line _mean_? It tells us this:\n\n Define a set by having the \"bad\" ref as a positive end, and\n having all the \"good\" refs as negative (uninteresting) boundary.\n\nThat is a way to show commits that are reachable from the bad one\nand excluding the ones that are reachable from any of the known-good\ncommits. The area of the graph in the current bisection that\ncontains suspect commits.\n\nNow, what does it mean to pull only the first-parent chain starting\nfrom the bad one in such a set in the first place? What does the\nresulting set of commits mean?\n"},{"id":"257562","messageId":"20150311184512.GB5442@vps892.directvps.nl","threadId":"38688","inReplyTo":"xmqqoao0xx9p.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4] rev-list: refuse --first-parent combined with --bisect","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2015-03-11T18:45:12Z","receivedAt":"2015-03-11T18:45:12Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Tue, Mar 10, 2015 at 04:12:18PM -0700, Junio C Hamano wrote:\n> Kevin Daudt writes:\n> \n> > git log --bisect seems to do something different then git rev-list\n> > --bisect\n> >\n> > From git-log(1):\n> >\n> > Pretend as if the bad bisection ref refs/bisect/bad was listed and\n> > as if it was followed by --not and the good bisection refs\n> > refs/bisect/good-* on the command line.\n> >\n> > This seems to just add addition refs to the log command, which seems\n> > unrelated to what rev-list --bisect does.\n> >\n> > So I don't see why git log --bisect --first-parent should be prohibited\n> > (unless this combination doesn't make sense on itself).\n> \n> Well, but think if your \"unless\" holds true or not yourself first\n> and then say \"I do not think this combination doesn't make sense\",\n> if you truly mean \"I don't see why ... should be prohibited\".\n> \n> What does such a command line _mean_? It tells us this:\n> \n> Define a set by having the \"bad\" ref as a positive end, and\n> having all the \"good\" refs as negative (uninteresting) boundary.\n> \n> That is a way to show commits that are reachable from the bad one\n> and excluding the ones that are reachable from any of the known-good\n> commits. The area of the graph in the current bisection that\n> contains suspect commits.\n> \n> Now, what does it mean to pull only the first-parent chain starting\n> from the bad one in such a set in the first place? What does the\n> resulting set of commits mean?\n> \n> \n\nIn that case it will leave out any merged in branches.\n\nI recalled reading something about this. Searching found me the GSoC\nidea:\n\n When your project is strictly \"new features are merged into trunk,\n never the other way around\", it is handy to be able to first find a\n merge on the trunk that merged a topic to point fingers at when a\n bug appears, instead of having to drill down to the individual\n commit on the faulty side branch.\n\nSo there is definitely a use case for --bisect --first-parent, which\nwould show you those commits that would be part of the bisection.\n"},{"id":"257564","messageId":"xmqqsidb5m2r.fsf@gitster.dls.corp.google.com","threadId":"38688","inReplyTo":"20150311184512.GB5442@vps892.directvps.nl","subject":"Re: [PATCH v4] rev-list: refuse --first-parent combined with --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-11T20:13:48Z","receivedAt":"2015-03-11T20:13:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Daudt writes:\n\n> On Tue, Mar 10, 2015 at 04:12:18PM -0700, Junio C Hamano wrote:\n>\n>> What does such a command line _mean_? It tells us this:\n>> \n>> Define a set by having the \"bad\" ref as a positive end, and\n>> having all the \"good\" refs as negative (uninteresting) boundary.\n>> \n>> That is a way to show commits that are reachable from the bad one\n>> and excluding the ones that are reachable from any of the known-good\n>> commits. The area of the graph in the current bisection that\n>> contains suspect commits.\n>> \n>> Now, what does it mean to pull only the first-parent chain starting\n>> from the bad one in such a set in the first place? What does the\n>> resulting set of commits mean?\n>\n> In that case it will leave out any merged in branches.\n\nNeeds a bit more thinking (hint: branches merged into *what*?).\n\n> I recalled reading something about this. Searching found me the GSoC\n> idea:\n>\n> When your project is strictly \"new features are merged into trunk,\n> never the other way around\", it is handy to be able to first find a\n> merge on the trunk that merged a topic to point fingers at when a\n> bug appears, instead of having to drill down to the individual\n> commit on the faulty side branch.\n>\n> So there is definitely a use case for --bisect --first-parent, which\n> would show you those commits that would be part of the bisection.\n\nStep back and think why \"git bisect --first-parent\" is sometimes\ndesired in the first place.\n\nIt is because in the regular bisection, you will almost always end\nup on a commit that is _not_ on the first-parent chain and asked to\ncheck that commit at a random place on a side branch in the first\nplace. And you mark such a commit as \"bad\".\n\nThe thing is, traversing from that \"bad\" commit that is almost\nalways is on a side branch, following the first-parent chain, will\nnot be a useful history that \"leaves out any merged in branches\".\n\nWhen \"git bisect --first-parent\" feature gets implemented, \"do not\nuse --first-parent with --bisect\" limitation has to be lifted\nanyway, but until then, not allowing \"--first-parent --bisect\" for\n\"rev-list\" but allowing it for \"log\" does not buy our users much.\nThe output does not give us a nice \"show me which merges on the\ntrunk may have caused the breakage to be examined with the remainder\nof this bisect session\".\n\nSo, yes, there is a use case for \"log --bisect --first-parent\", once\nthere is a working \"bisect --first-parent\", but not until then, the\ncommand is not useful, I would think.\n"},{"id":"257798","messageId":"20150316163306.GB11832@vps892.directvps.nl","threadId":"38688","inReplyTo":"xmqqsidb5m2r.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4] rev-list: refuse --first-parent combined with --bisect","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2015-03-16T16:33:06Z","receivedAt":"2015-03-16T16:33:06Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Wed, Mar 11, 2015 at 01:13:48PM -0700, Junio C Hamano wrote:\n> Kevin Daudt writes:\n> \n> > On Tue, Mar 10, 2015 at 04:12:18PM -0700, Junio C Hamano wrote:\n> >\n> \n> Step back and think why \"git bisect --first-parent\" is sometimes\n> desired in the first place.\n> \n> It is because in the regular bisection, you will almost always end\n> up on a commit that is _not_ on the first-parent chain and asked to\n> check that commit at a random place on a side branch in the first\n> place. And you mark such a commit as \"bad\".\n> \n> The thing is, traversing from that \"bad\" commit that is almost\n> always is on a side branch, following the first-parent chain, will\n> not be a useful history that \"leaves out any merged in branches\".\n> \n> When \"git bisect --first-parent\" feature gets implemented, \"do not\n> use --first-parent with --bisect\" limitation has to be lifted\n> anyway, but until then, not allowing \"--first-parent --bisect\" for\n> \"rev-list\" but allowing it for \"log\" does not buy our users much.\n> The output does not give us a nice \"show me which merges on the\n> trunk may have caused the breakage to be examined with the remainder\n> of this bisect session\".\n> \n> So, yes, there is a use case for \"log --bisect --first-parent\", once\n> there is a working \"bisect --first-parent\", but not until then, the\n> command is not useful, I would think.\n\nThank you for you explanation. My confusion came from incorrectly\nassuming refs/bisect/bad and refs/bisect/good-* were pointing to the\ninitially specified good and bad commits, in which case the combination\ndoes make sense.\n\nI was looking in the manpages for the meaning of the bisect refs, but\ncould only find something about refs/bisect/bad:\n\ngit-bisect(1):\n> Eventually there will be no more revisions left to bisect, and you\n> will have been left with the first bad kernel revision in\n> \"refs/bisect/bad\n\nSo this ref changes to the bad commit.\n\nFor refs/bisect/good-*, I could only find an example snippet:\n\n> GOOD=$(git for-each-ref \"--format=%(objectname)\" refs/bisect/good-*)\n\nBut it's not really clear what * might be expanded to, nor what they\nmean. I guess this could use some clarrification in the documentation.\n\nKnowing this, I agree that the combination log --bisect --first-parent\ndoesn't make sense either.\n\nI will send in a new patch.\n\nKevin\n"},{"id":"257810","messageId":"xmqqbnjsrcyz.fsf@gitster.dls.corp.google.com","threadId":"38688","inReplyTo":"20150316163306.GB11832@vps892.directvps.nl","subject":"Re: [PATCH v4] rev-list: refuse --first-parent combined with --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-16T18:53:08Z","receivedAt":"2015-03-16T18:53:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Daudt writes:\n\n> So this ref changes to the bad commit.\n>\n> For refs/bisect/good-*, I could only find an example snippet:\n>\n>> GOOD=$(git for-each-ref \"--format=%(objectname)\" refs/bisect/good-*)\n>\n> But it's not really clear what * might be expanded to, nor what they\n> mean. I guess this could use some clarrification in the documentation.\n\nBecause the history is not linear in Git, bisection works by\nshrinking a subgraph of the history DAG that contains \"yet to be\ntested, suspected to have introduced a badness\" commits. The\nsubgraph is defined as anything reachable from _the_ \"bad\" commit\n(initially, the one you give to the command when you start) that are\nnot reachable from any of the \"good\" commits.\n\nSuppose you started from this graph. Time flows left to right as\nusual.\n\n ---0---2---4---6---8---9\n \\ /\n 1---3---5---7\n\nThen mark the initial good and bad commits as G and B.\n\n ---G---2---4---6---8---B\n \\ /\n 1---3---5---7\n\nAnd imagine that you are asked to check 4, which turns out to be\ngood. We do not _move_ G to 4; we mark 4 as good, while keeping\n0 also as good.\n\n ---G---2---G---6---8---B\n \\ /\n 1---3---5---7\n\nAnd if you are next asked to check 5, and mark it as good, the graph\nwill become like this:\n\n ---G---2---G---6---8---B\n \\ /\n 1---3---G---7\n\nOf course, at this point, the subgraph of suspects are 6, 7, 8 and\n9, and the subgraph no longer is affected by the fact that 0 is\ngood. But it is crucial to keep 0 marked as good in the step before\nthis one, before you tested 5, as that is what allows us not having\nto test any ancestors of 0 at all.\n\nNow, one may wonder why we need multiple \"good\" commits but we do\nnot need multiple \"bad\" commits. This comes from the nature of\n\"bisection\", which is a tool to find a _single_ breakage [*1*], and\na fundamental assumption is that a breakage does not fix itself.\n\nHence, if you have a history that looks like this:\n\n\n G...1---2---3---4---6---8---B\n \\\n 5---7---B\n\nit follows that 4 must also be \"bad\". It used to be good long time\nago somewhere before 1, and somewhere along way on the history,\nthere was a single breakage event that we are hunting for. That\nsingle event cannot be 5, 6, 7 or 8 because breakage at say 5 would\nnot explain why the tip of the upper branch is broken---its breakage\nhas no way to propagate there. The breakage must have happened at 4\nor before that commit.\n\nWhich means that if you marked the child of 8 (the tip of the upper\nbranch) as bad, there is no reason for us to even look at the lower\nbranch. As soon as you mark the tip of the upper branch \"bad\", the\nbisection can become\n\n G...1---2---3---4---6---8---B\n\nand without looking at the lower branch, it can find the single\nbreakage.\n\n\n[Footnote]\n\n*1* You may be hunting for a single _fix_, and flipping the meaning\n of \"good\" and \"bad\", say \"It used to be broken but somewhere we\n seem to have fixed that bug. Where did we do that?\", marking\n the ones that still has the bug \"good\" and the ones that no\n longer has the bug \"bad\". In that context, you would be looking\n for a single fix. A more neutral term might be\n\n - we look for a single event that changes some state.\n\n - old state before that single event is spelled G O O D, but it\n is pronounced \"not yet\".\n\n - new state before that single event is spelled B A D, but it is\n pronounced \"already\".\n"},{"id":"257817","messageId":"065AE7977A54488198B39564E3E174E6@PhilipOakley","threadId":"38688","inReplyTo":"xmqqbnjsrcyz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4] rev-list: refuse --first-parent combined with --bisect","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2015-03-16T20:03:53Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Junio C Hamano\" \n> Kevin Daudt writes:\n>\n>> So this ref changes to the bad commit.\n>>\n>> For refs/bisect/good-*, I could only find an example snippet:\n>>\n>>> GOOD=$(git for-each-ref \"--format=%(objectname)\" refs/bisect/good-*)\n>>\n>> But it's not really clear what * might be expanded to, nor what they\n>> mean. I guess this could use some clarrification in the \n>> documentation.\n>\n> Because the history is not linear in Git, bisection works by\n> shrinking a subgraph of the history DAG that contains \"yet to be\n> tested, suspected to have introduced a badness\" commits. The\n> subgraph is defined as anything reachable from _the_ \"bad\" commit\n> (initially, the one you give to the command when you start) that are\n> not reachable from any of the \"good\" commits.\n>\n> Suppose you started from this graph. Time flows left to right as\n> usual.\n>\n> ---0---2---4---6---8---9\n> \\ /\n> 1---3---5---7\n>\n> Then mark the initial good and bad commits as G and B.\n>\n> ---G---2---4---6---8---B\n> \\ /\n> 1---3---5---7\n>\n> And imagine that you are asked to check 4, which turns out to be\n> good. We do not _move_ G to 4; we mark 4 as good, while keeping\n> 0 also as good.\n>\n> ---G---2---G---6---8---B\n> \\ /\n> 1---3---5---7\n>\n> And if you are next asked to check 5, and mark it as good, the graph\n> will become like this:\n>\n> ---G---2---G---6---8---B\n> \\ /\n> 1---3---G---7\n>\n> Of course, at this point, the subgraph of suspects are 6, 7, 8 and\n> 9, and the subgraph no longer is affected by the fact that 0 is\n> good. But it is crucial to keep 0 marked as good in the step before\n> this one, before you tested 5, as that is what allows us not having\n> to test any ancestors of 0 at all.\n>\n> Now, one may wonder why we need multiple \"good\" commits but we do\n> not need multiple \"bad\" commits. This comes from the nature of\n> \"bisection\", which is a tool to find a _single_ breakage [*1*], and\n> a fundamental assumption is that a breakage does not fix itself.\n>\n> Hence, if you have a history that looks like this:\n>\n>\n> G...1---2---3---4---6---8---B\n> \\\n> 5---7---B\n>\n> it follows that 4 must also be \"bad\". It used to be good long time\n> ago somewhere before 1, and somewhere along way on the history,\n> there was a single breakage event that we are hunting for. That\n> single event cannot be 5, 6, 7 or 8 because breakage at say 5 would\n> not explain why the tip of the upper branch is broken---its breakage\n> has no way to propagate there. The breakage must have happened at 4\n> or before that commit.\n\nIs it not worth at least confirming the assertion that 4 is bad before\nproceding, or at least an option to confirm that in complex scenarios\nwhere the fault may be devious.\n[the explicit explanation has been useful for me...]\n\n>\n> Which means that if you marked the child of 8 (the tip of the upper\n> branch) as bad, there is no reason for us to even look at the lower\n> branch. As soon as you mark the tip of the upper branch \"bad\", the\n> bisection can become\n>\n> G...1---2---3---4---6---8---B\n>\n> and without looking at the lower branch, it can find the single\n> breakage.\n>\n>\n> [Footnote]\n>\n> *1* You may be hunting for a single _fix_, and flipping the meaning\n> of \"good\" and \"bad\", say \"It used to be broken but somewhere we\n> seem to have fixed that bug. Where did we do that?\", marking\n> the ones that still has the bug \"good\" and the ones that no\n> longer has the bug \"bad\". In that context, you would be looking\n> for a single fix. A more neutral term might be\n>\n> - we look for a single event that changes some state.\n>\n> - old state before that single event is spelled G O O D, but it\n> is pronounced \"not yet\".\n>\n> - new state before that single event is spelled B A D, but it is\n> pronounced \"already\".\n> --\n> \n"},{"id":"257820","messageId":"xmqqr3sops9f.fsf@gitster.dls.corp.google.com","threadId":"38688","inReplyTo":"065AE7977A54488198B39564E3E174E6@PhilipOakley","subject":"Re: [PATCH v4] rev-list: refuse --first-parent combined with --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-16T21:05:48Z","receivedAt":"2015-03-16T21:05:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philip Oakley\" writes:\n\n> From: \"Junio C Hamano\" \n>\n>> Hence, if you have a history that looks like this:\n>>\n>>\n>> G...1---2---3---4---6---8---B\n>> \\\n>> 5---7---B\n>>\n>> it follows that 4 must also be \"bad\". It used to be good long time\n>> ago somewhere before 1, and somewhere along way on the history,\n>> there was a single breakage event that we are hunting for. That\n>> single event cannot be 5, 6, 7 or 8 because breakage at say 5 would\n>> not explain why the tip of the upper branch is broken---its breakage\n>> has no way to propagate there. The breakage must have happened at 4\n>> or before that commit.\n>\n> Is it not worth at least confirming the assertion that 4 is bad before\n> proceding, or at least an option to confirm that in complex scenarios\n> where the fault may be devious.\n\nThat raises a somewhat interesting tangent.\n\nChristian seems to be forever interested in bisect, so I'll add him\nto the Cc list ;-)\n\nThere is no way to give multiple \"bad\" from the command line. You\ncan say \"git bisect start rev rev rev...\" but that gives only one\nbad and everything else is good. And once you specify one of the\nabove two bad ones (say, the child of 8), then we will not even\noffer the other one (i.e. the child of 7) as a candidate to be\ntested. So in that sense, \"confirm that 4 is bad before proceeding\"\nis a moot point.\n\nHowever, you can say \"git bisect bad \" (and \"git bisect good\n\" for that matter) on a rev that is unrelated to what the\ncurrent bisection state is. E.g. after you mark the child of 8 as\n\"bad\", the bisected graph would become\n\n G...1---2---3---4---6---8---B\n\nand you would be offered to test somewhere in the middle, say, 4.\nBut it is perfectly OK for you to respond with \"git bisect bad 7\",\nif you know 7 is bad.\n\nI _think_ the current code blindly overwrites the \"bad\" pointer,\nmaking the bisection state into this graph if you do so.\n\n G...1---2---3---4\n \\\n 5---B\n\nThis is very suboptimal. The side branch 4-to-7 could be much\nlonger than the original trunk 4-to-the-tip, in which case we would\nhave made the suspect space _larger_, not smaller.\n\nWe certainly should be able to take advantage of the fact that the\ncurrent \"bad\" commit (i.e. the child of 8) and the newly given \"bad\"\ncommit (i.e. 7) are both known to be bad and mark 4 as \"bad\" instead\nwhen that happens, instead of doing the suboptimal thing the code\ncurrently does.\n"},{"id":"257870","messageId":"CAP8UFD12UX+3psD2=9_RsGv8JA2C8N54qAYGydYgr7n5ta7dzw@mail.gmail.com","threadId":"38688","inReplyTo":"xmqqr3sops9f.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4] rev-list: refuse --first-parent combined with --bisect","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-03-17T16:09:53Z","receivedAt":"2015-03-17T16:09:53Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Mar 16, 2015 at 10:05 PM, Junio C Hamano wrote:\n> \"Philip Oakley\" writes:\n>\n>> From: \"Junio C Hamano\" \n>>\n>>> Hence, if you have a history that looks like this:\n>>>\n>>>\n>>> G...1---2---3---4---6---8---B\n>>> \\\n>>> 5---7---B\n>>>\n>>> it follows that 4 must also be \"bad\". It used to be good long time\n>>> ago somewhere before 1, and somewhere along way on the history,\n>>> there was a single breakage event that we are hunting for. That\n>>> single event cannot be 5, 6, 7 or 8 because breakage at say 5 would\n>>> not explain why the tip of the upper branch is broken---its breakage\n>>> has no way to propagate there. The breakage must have happened at 4\n>>> or before that commit.\n>>\n>> Is it not worth at least confirming the assertion that 4 is bad before\n>> proceding, or at least an option to confirm that in complex scenarios\n>> where the fault may be devious.\n>\n> That raises a somewhat interesting tangent.\n>\n> Christian seems to be forever interested in bisect, so I'll add him\n> to the Cc list ;-)\n>\n> There is no way to give multiple \"bad\" from the command line. You\n> can say \"git bisect start rev rev rev...\" but that gives only one\n> bad and everything else is good. And once you specify one of the\n> above two bad ones (say, the child of 8), then we will not even\n> offer the other one (i.e. the child of 7) as a candidate to be\n> tested. So in that sense, \"confirm that 4 is bad before proceeding\"\n> is a moot point.\n>\n> However, you can say \"git bisect bad \" (and \"git bisect good\n> \" for that matter) on a rev that is unrelated to what the\n> current bisection state is. E.g. after you mark the child of 8 as\n> \"bad\", the bisected graph would become\n>\n> G...1---2---3---4---6---8---B\n>\n> and you would be offered to test somewhere in the middle, say, 4.\n> But it is perfectly OK for you to respond with \"git bisect bad 7\",\n> if you know 7 is bad.\n>\n> I _think_ the current code blindly overwrites the \"bad\" pointer,\n> making the bisection state into this graph if you do so.\n>\n> G...1---2---3---4\n> \\\n> 5---B\n\nYes, we keep only one \"bad\" pointer.\n\n> This is very suboptimal. The side branch 4-to-7 could be much\n> longer than the original trunk 4-to-the-tip, in which case we would\n> have made the suspect space _larger_, not smaller.\n\nYes, but the user is supposed to not change the \"bad\" pointer for no\ngood reason. For example maybe a mistake was made and the first commit\nmarked as \"bad\" was not actually bad.\n\n> We certainly should be able to take advantage of the fact that the\n> current \"bad\" commit (i.e. the child of 8) and the newly given \"bad\"\n> commit (i.e. 7) are both known to be bad and mark 4 as \"bad\" instead\n> when that happens, instead of doing the suboptimal thing the code\n> currently does.\n\nYeah, we could do that, but we would have to allow it only if a\nspecial option is passed on the command line, for example:\n\ngit bisect bad --alternate \n\nand/or we could make \"git bisect bad\" accept any number of bad commitishs.\n\nThat could give additional bonus points to the GSoC student who would\nimplement it :-)\n\nThanks,\nChristian.\n"},{"id":"257874","messageId":"xmqqtwxjo4nf.fsf@gitster.dls.corp.google.com","threadId":"38688","inReplyTo":"CAP8UFD12UX+3psD2=9_RsGv8JA2C8N54qAYGydYgr7n5ta7dzw@mail.gmail.com","subject":"Re: [PATCH v4] rev-list: refuse --first-parent combined with --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-17T18:33:24Z","receivedAt":"2015-03-17T18:33:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Mon, Mar 16, 2015 at 10:05 PM, Junio C Hamano wrote:\n>\n>> However, you can say \"git bisect bad \" (and \"git bisect good\n>> \" for that matter) on a rev that is unrelated to what the\n>> current bisection state is. E.g. after you mark the child of 8 as\n>> \"bad\", the bisected graph would become\n>>\n>> G...1---2---3---4---6---8---B\n>>\n>> and you would be offered to test somewhere in the middle, say, 4.\n>> But it is perfectly OK for you to respond with \"git bisect bad 7\",\n>> if you know 7 is bad.\n>>\n>> I _think_ the current code blindly overwrites the \"bad\" pointer,\n>> making the bisection state into this graph if you do so.\n>>\n>> G...1---2---3---4\n>> \\\n>> 5---B\n>\n> Yes, we keep only one \"bad\" pointer.\n>\n>> This is very suboptimal. The side branch 4-to-7 could be much\n>> longer than the original trunk 4-to-the-tip, in which case we would\n>> have made the suspect space _larger_, not smaller.\n>\n> Yes, but the user is supposed to not change the \"bad\" pointer for no\n> good reason.\n\nThat is irrelevant, no? Nobody is questioning that the user is\nsupposed to judge if a commit is \"good\" or \"bad\" correctly.\n\nAnd nobody sane is dreaming that \"Git could do better and detect\nuser's mistakes when the user says 'bad' for a commit that is\nactually 'good'\"; if Git can do that, then it should be able to do\nthe bisect without any user input (including \"bisect run\") at all\n;-).\n\n>> We certainly should be able to take advantage of the fact that the\n>> current \"bad\" commit (i.e. the child of 8) and the newly given \"bad\"\n>> commit (i.e. 7) are both known to be bad and mark 4 as \"bad\" instead\n>> when that happens, instead of doing the suboptimal thing the code\n>> currently does.\n>\n> Yeah, we could do that, but we would have to allow it only if a\n> special option is passed on the command line, for example:\n> git bisect bad --alternate \n\nI am not quite sure if I am correctly getting what you meant to say,\nbut if you meant \"only when --alternate is given, we should do the\nmerge-base thing; we should keep losing the current 'bad' and\nreplace it with the new one without the --alternate option\", I would\nsee that as an exercise of a bad taste.\n\nBecause the merge-base thing is using both the current and the new\none, such a use is not \"alternate\" in the first place.\n\nIf the proposal were \"with a new option, the user can say 'oh, I\nmade a mistake earlier and said that a commit that is not bad as\n'bad'. Let me replace the commit currently marked as 'bad' with\nthis one.\", I would find it very sensible, actually.\n\nI can see that such an operation can be called \"alternate\", but\n\"--fix\" might be shorter-and-sweeter-and-to-the-point.\n\nIn the \"normal\" case, the commit we offer the user to check (and\nrespond with \"git bisect bad\" without any commit parameter) is\nalways an ancestor of the current 'bad', so the merge-base with\n'bad' and the commit that was just checked would always be the\ncurrent commit. Using the merge-base thing will be transparent to\nthe end users in the normal case, and when the user has off-line\nknowledge that some other commit that is not an ancestor of the\ncurrent 'bad' commit is bad, the merge-base thing will give a better\nbehaviour than the current implementation that blindly replaces.\n\n> and/or we could make \"git bisect bad\" accept any number of bad\n> commitishs.\n\nYes, that is exactly what I meant.\n\nThe way I understand the Philip's point is that the user may have\na-priori knowledge that a breakage from the same cause appears in\nboth tips of these branches. In such a case, we can start bisection\nafter marking the merge-base of two 'bad' commits, e.g. 4 in the\nillustration in the message you are responding to, instead of\nincluding 5, 6, and 8 in the suspect set.\n\nYou need to be careful, though. An obvious pitfall is what you\nshould do when there is a criss-cross merge.\n\nThanks.\n"},{"id":"257881","messageId":"CAP8UFD0Mn3SPimYU3fdF5pV1MDAHXhKUVSutfJKrXzPpaXM=bA@mail.gmail.com","threadId":"38688","inReplyTo":"xmqqtwxjo4nf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4] rev-list: refuse --first-parent combined with --bisect","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-03-17T19:49:50Z","receivedAt":"2015-03-17T19:49:50Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Mar 17, 2015 at 7:33 PM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> On Mon, Mar 16, 2015 at 10:05 PM, Junio C Hamano wrote:\n>>\n>>> However, you can say \"git bisect bad \" (and \"git bisect good\n>>> \" for that matter) on a rev that is unrelated to what the\n>>> current bisection state is. E.g. after you mark the child of 8 as\n>>> \"bad\", the bisected graph would become\n>>>\n>>> G...1---2---3---4---6---8---B\n>>>\n>>> and you would be offered to test somewhere in the middle, say, 4.\n>>> But it is perfectly OK for you to respond with \"git bisect bad 7\",\n>>> if you know 7 is bad.\n>>>\n>>> I _think_ the current code blindly overwrites the \"bad\" pointer,\n>>> making the bisection state into this graph if you do so.\n>>>\n>>> G...1---2---3---4\n>>> \\\n>>> 5---B\n>>\n>> Yes, we keep only one \"bad\" pointer.\n>>\n>>> This is very suboptimal. The side branch 4-to-7 could be much\n>>> longer than the original trunk 4-to-the-tip, in which case we would\n>>> have made the suspect space _larger_, not smaller.\n>>\n>> Yes, but the user is supposed to not change the \"bad\" pointer for no\n>> good reason.\n>\n> That is irrelevant, no? Nobody is questioning that the user is\n> supposed to judge if a commit is \"good\" or \"bad\" correctly.\n\nSo if there is already a bad commit and the user gives another\nbad commit, that means that the user knows that it will replace the\nexisting bad commit with the new one and that it's done for this\npurpose.\n\n> And nobody sane is dreaming that \"Git could do better and detect\n> user's mistakes when the user says 'bad' for a commit that is\n> actually 'good'\"; if Git can do that, then it should be able to do\n> the bisect without any user input (including \"bisect run\") at all\n> ;-).\n>\n>>> We certainly should be able to take advantage of the fact that the\n>>> current \"bad\" commit (i.e. the child of 8) and the newly given \"bad\"\n>>> commit (i.e. 7) are both known to be bad and mark 4 as \"bad\" instead\n>>> when that happens, instead of doing the suboptimal thing the code\n>>> currently does.\n>>\n>> Yeah, we could do that, but we would have to allow it only if a\n>> special option is passed on the command line, for example:\n>> git bisect bad --alternate \n>\n> I am not quite sure if I am correctly getting what you meant to say,\n> but if you meant \"only when --alternate is given, we should do the\n> merge-base thing; we should keep losing the current 'bad' and\n> replace it with the new one without the --alternate option\", I would\n> see that as an exercise of a bad taste.\n\nWhat I wanted to say is that if we change \"git bisect bad \",\nso that now it means \"add a new bad commit\" instead of the previous\n\"replace the current bad commit, if any, with this one\", then experienced\nusers might see that change as a regression in the user interface and\nit might even break scripts.\n\nThat's why I suggested to use a new option to mean\n\"add a new bad commit\", though --alternate might not be the best\nname for this option.\n\n> Because the merge-base thing is using both the current and the new\n> one, such a use is not \"alternate\" in the first place.\n>\n> If the proposal were \"with a new option, the user can say 'oh, I\n> made a mistake earlier and said that a commit that is not bad as\n> 'bad'. Let me replace the commit currently marked as 'bad' with\n> this one.\", I would find it very sensible, actually.\n\nWhat I find sensible is to not break the semantics of the current\ninterface.\n\n> I can see that such an operation can be called \"alternate\", but\n> \"--fix\" might be shorter-and-sweeter-and-to-the-point.\n>\n> In the \"normal\" case, the commit we offer the user to check (and\n> respond with \"git bisect bad\" without any commit parameter) is\n> always an ancestor of the current 'bad', so the merge-base with\n> 'bad' and the commit that was just checked would always be the\n> current commit. Using the merge-base thing will be transparent to\n> the end users in the normal case, and when the user has off-line\n> knowledge that some other commit that is not an ancestor of the\n> current 'bad' commit is bad, the merge-base thing will give a better\n> behaviour than the current implementation that blindly replaces.\n\nYes, I agree that it could be an improvement to make it possible for the\nuser to specify another bad commit. I just think it should be done with\na new option if there is already a bad commit...\n\n>> and/or we could make \"git bisect bad\" accept any number of bad\n>> commitishs.\n\n... or by allowing any number of bad commits after \"git bisect bad\".\n\n> Yes, that is exactly what I meant.\n>\n> The way I understand the Philip's point is that the user may have\n> a-priori knowledge that a breakage from the same cause appears in\n> both tips of these branches. In such a case, we can start bisection\n> after marking the merge-base of two 'bad' commits, e.g. 4 in the\n> illustration in the message you are responding to, instead of\n> including 5, 6, and 8 in the suspect set.\n\nYeah, I agree that we can do better in this case.\n\n> You need to be careful, though. An obvious pitfall is what you\n> should do when there is a criss-cross merge.\n\nYeah, it is not as simple as it might look like, neither for the interface\nnor for the behavior.\n"},{"id":"257888","messageId":"xmqq3853nyia.fsf@gitster.dls.corp.google.com","threadId":"38688","inReplyTo":"CAP8UFD0Mn3SPimYU3fdF5pV1MDAHXhKUVSutfJKrXzPpaXM=bA@mail.gmail.com","subject":"Re: [PATCH v4] rev-list: refuse --first-parent combined with --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-17T20:46:05Z","receivedAt":"2015-03-17T20:46:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Tue, Mar 17, 2015 at 7:33 PM, Junio C Hamano wrote:\n>> Christian Couder writes:\n>>\n>>> Yes, but the user is supposed to not change the \"bad\" pointer for no\n>>> good reason.\n>>\n>> That is irrelevant, no? Nobody is questioning that the user is\n>> supposed to judge if a commit is \"good\" or \"bad\" correctly.\n>\n> So if there is already a bad commit and the user gives another\n> bad commit, that means that the user knows that it will replace the\n> existing bad commit with the new one and that it's done for this\n> purpose.\n\nECANNOTQUITEPARSE. The user may say \"git bisect bad $that\" and we\ndo not question $that is bad. Git does not know better than the\nuser.\n\nBut that does not mean Git does not know better than the user how\nthe current bad commit and $that commit are related. The user is\nnot interested in \"replacing\" at all. The user is telling just one\nsingle fact, that is, \"$that is bad\".\n\n>> I am not quite sure if I am correctly getting what you meant to say,\n>> but if you meant \"only when --alternate is given, we should do the\n>> merge-base thing; we should keep losing the current 'bad' and\n>> replace it with the new one without the --alternate option\", I would\n>> see that as an exercise of a bad taste.\n>\n> What I wanted to say is that if we change \"git bisect bad \",\n> so that now it means \"add a new bad commit\" instead of the previous\n> \"replace the current bad commit, if any, with this one\", then experienced\n> users might see that change as a regression in the user interface and\n> it might even break scripts.\n\nHuh? \n\nStep back a bit. The place you need to start from is to admit the\nfact that what \"git bisect bad \" currently does is\nbroken.\n\nTry creating this history yourself\n\n a---b---c---d---e---f\n\nand start bisection this way:\n\n $ git bisect start f c\n $ git bisect bad a\n\nImmediately after the second command, \"git bisect\" moans\n\n Some good revs are not ancestor of the bad rev.\n git bisect cannot work properly in this case.\n Maybe you mistake good and bad revs?\n\nwhen it notices that the good rev (i.e. 'c') is no longer an\nancestor of the 'bad', which now points at 'a'.\n\nBut that is because \"git bisect bad\" _blindly_ moved 'bad' that used\nto point at 'f' to 'a', making a good rev (i.e. 'c') an ancestor of\nthe bad rev, without even bothering to check.\n\nNow, if we fixed this bug and made the bisect_state function more\ncareful (namely, when accepting \"bad\", make sure it is not beyond\nany existing \"good\", or barf like the above, _without_ moving the\nbad pointer), the user interface and behaviour would be changed. Is\nthat a regression? No, it is a usability fix and a progress.\n\nSimply put, bisect_state function can become more careful and\nintelligent to help users.\n\nI view this \"user goes out of way to tell us a commit that is known\nto be bad as bad, even though it is not what we offered to test and\nis not an ancestor of the commit that currently marked as bad\" case\nthe same way. We by now hopefully understand that blindly replacing\nthe current 'bad' is suboptimal. By teaching bisect_state to do the\n\"merge-base thing\", we would be fixing that.\n\nWhy is it a regression?\n"},{"id":"257938","messageId":"CAP8UFD3=w_7Mm-ew6HDWyK-x9uUTwcw=URNp+gNvfFXueAkgBg@mail.gmail.com","threadId":"38688","inReplyTo":"xmqq3853nyia.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4] rev-list: refuse --first-parent combined with --bisect","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-03-18T10:36:26Z","receivedAt":"2015-03-18T10:36:26Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Mar 17, 2015 at 9:46 PM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>> On Tue, Mar 17, 2015 at 7:33 PM, Junio C Hamano wrote:\n>>> Christian Couder writes:\n>>>\n>>>> Yes, but the user is supposed to not change the \"bad\" pointer for no\n>>>> good reason.\n>>>\n>>> That is irrelevant, no? Nobody is questioning that the user is\n>>> supposed to judge if a commit is \"good\" or \"bad\" correctly.\n>>\n>> So if there is already a bad commit and the user gives another\n>> bad commit, that means that the user knows that it will replace the\n>> existing bad commit with the new one and that it's done for this\n>> purpose.\n>\n> ECANNOTQUITEPARSE. The user may say \"git bisect bad $that\" and we\n> do not question $that is bad. Git does not know better than the\n> user.\n>\n> But that does not mean Git does not know better than the user how\n> the current bad commit and $that commit are related. The user is\n> not interested in \"replacing\" at all. The user is telling just one\n> single fact, that is, \"$that is bad\".\n\nThe user may make mistakes and try to fix them, like for example:\n\n$ git checkout master\n$ git bisect bad\n$ git log --oneline --decorate --graph --all\n# Ooops I was not on the right branch\n$ git checkout dev\n$ git bisect bad\n$ git log --oneline --decorate --graph --all\n# Everything looks ok now; the \"bad\" commit is what I expect\n# I can properly continue bisecting using \"git bisect good\"...\n\nIn this case the user, who knows how git bisect works, expected that\nthe second \"git bisect bad\" would fix the previous mistake made using\nthe first \"git bisect bad\".\n\nIf we make \"git bisect bad\" behave in another way we may break an\nadvanced user's expectation.\n\n>>> I am not quite sure if I am correctly getting what you meant to say,\n>>> but if you meant \"only when --alternate is given, we should do the\n>>> merge-base thing; we should keep losing the current 'bad' and\n>>> replace it with the new one without the --alternate option\", I would\n>>> see that as an exercise of a bad taste.\n>>\n>> What I wanted to say is that if we change \"git bisect bad \",\n>> so that now it means \"add a new bad commit\" instead of the previous\n>> \"replace the current bad commit, if any, with this one\", then experienced\n>> users might see that change as a regression in the user interface and\n>> it might even break scripts.\n>\n> Huh?\n>\n> Step back a bit. The place you need to start from is to admit the\n> fact that what \"git bisect bad \" currently does is\n> broken.\n>\n> Try creating this history yourself\n>\n> a---b---c---d---e---f\n>\n> and start bisection this way:\n>\n> $ git bisect start f c\n> $ git bisect bad a\n>\n> Immediately after the second command, \"git bisect\" moans\n>\n> Some good revs are not ancestor of the bad rev.\n> git bisect cannot work properly in this case.\n> Maybe you mistake good and bad revs?\n>\n> when it notices that the good rev (i.e. 'c') is no longer an\n> ancestor of the 'bad', which now points at 'a'.\n>\n> But that is because \"git bisect bad\" _blindly_ moved 'bad' that used\n> to point at 'f' to 'a', making a good rev (i.e. 'c') an ancestor of\n> the bad rev, without even bothering to check.\n\nYeah, \"git bisect bad\" currently does what it is asked and then\ncomplains when it looks like a mistake has been made.\n\nYou might see that as a bug. I am seeing that more as \"git bisect\"\nexpecting users to know what they are doing.\n\nFor example an advanced user might have realized that the first \"git\nbisect start f c\" was completely rubish for some reason, and the \"git\nbisect bad a\" might be a first step to fix that. (The next step might\nthen be deleting the \"good\" pointer...)\n\n> Now, if we fixed this bug and made the bisect_state function more\n> careful (namely, when accepting \"bad\", make sure it is not beyond\n> any existing \"good\", or barf like the above, _without_ moving the\n> bad pointer), the user interface and behaviour would be changed. Is\n> that a regression? No, it is a usability fix and a progress.\n\nYeah, you might see that as a usability fix and a progress.\n\n> Simply put, bisect_state function can become more careful and\n> intelligent to help users.\n\nYeah, we can try to help users more, but doing that we might annoy\nsome advanced users, who are used to the current way \"git bisect\"\nworks, and perhaps break some scripts.\n"},{"id":"258070","messageId":"1426803248-6905-1-git-send-email-me@ikke.info","threadId":"38688","inReplyTo":"1425934575-19581-1-git-send-email-me@ikke.info","subject":"[PATCH v5] rev-list: refuse --first-parent combined with --bisect","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2015-03-19T22:14:08Z","receivedAt":"2015-03-19T22:14:08Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"rev-list --bisect is used by git bisect, but never together with\n--first-parent. Because rev-list --bisect together with --first-parent\nis not handled currently, and even leads to segfaults, refuse to use\nboth options together.\n\nBecause this is not supported, it makes little sense to use git log\n--bisect --first parent either, because refs/heads/bad is not limited to\nthe first parent chain.\n\nHelped-by: Junio C. Hamano \nHelped-by: Eric Sunshine \nSigned-off-by: Kevin Daudt \n---\nUpdates since v4:\n\n* Not only refusing rev-list --bisect --first-parent, but also log --bisect --first-parent\n\n Documentation/rev-list-options.txt | 7 ++++---\n revision.c | 3 +++\n t/t6000-rev-list-misc.sh | 4 ++++\n 3 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 4ed8587..e2de789 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -123,7 +123,8 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).\n \tbecause merges into a topic branch tend to be only about\n \tadjusting to updated upstream from time to time, and\n \tthis option allows you to ignore the individual commits\n-\tbrought in to your history by such a merge.\n+\tbrought in to your history by such a merge. Cannot be\n+\tcombined with --bisect.\n \n --not::\n \tReverses the meaning of the '{caret}' prefix (or lack thereof)\n@@ -185,7 +186,7 @@ ifndef::git-rev-list[]\n \tPretend as if the bad bisection ref `refs/bisect/bad`\n \twas listed and as if it was followed by `--not` and the good\n \tbisection refs `refs/bisect/good-*` on the command\n-\tline.\n+\tline. Cannot be combined with --first-parent.\n endif::git-rev-list[]\n \n --stdin::\n@@ -566,7 +567,7 @@ outputs 'midpoint', the output of the two commands\n would be of roughly the same length. Finding the change which\n introduces a regression is thus reduced to a binary search: repeatedly\n generate and test new 'midpoint's until the commit chain is of length\n-one.\n+one. Cannot be combined with --first-parent.\n \n --bisect-vars::\n \tThis calculates the same as `--bisect`, except that refs in\ndiff --git a/revision.c b/revision.c\nindex 66520c6..ed3f6e9 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2342,6 +2342,9 @@ 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 \treturn left;\n }\n \ndiff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh\nindex 2602086..1f58b46 100755\n--- a/t/t6000-rev-list-misc.sh\n+++ b/t/t6000-rev-list-misc.sh\n@@ -96,4 +96,8 @@ test_expect_success 'rev-list can show index objects' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--bisect and --first-parent can not be combined' '\n+\ttest_must_fail git rev-list --bisect --first-parent HEAD\n+'\n+\n test_done\n-- \n2.3.2\n"},{"id":"258072","messageId":"xmqqh9tghaky.fsf@gitster.dls.corp.google.com","threadId":"38688","inReplyTo":"1426803248-6905-1-git-send-email-me@ikke.info","subject":"Re: [PATCH v5] rev-list: refuse --first-parent combined with --bisect","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-19T22:43:57Z","receivedAt":"2015-03-19T22:43:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Daudt writes:\n\n> rev-list --bisect is used by git bisect, but never together with\n> --first-parent. Because rev-list --bisect together with --first-parent\n> is not handled currently, and even leads to segfaults, refuse to use\n> both options together.\n>\n> Because this is not supported, it makes little sense to use git log\n> --bisect --first parent either, because refs/heads/bad is not limited to\n> the first parent chain.\n>\n> Helped-by: Junio C. Hamano \n> Helped-by: Eric Sunshine \n> Signed-off-by: Kevin Daudt \n> ---\n\nThanks; will queue.\n"},{"id":"258075","messageId":"3FAFDE160E204496A38A4B6C53FB9B32@PhilipOakley","threadId":"38688","inReplyTo":"xmqqtwxjo4nf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4] rev-list: refuse --first-parent combined with --bisect","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2015-03-19T23:03:43Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Junio C Hamano\" \nSent: Tuesday, March 17, 2015 6:33 PM\n> Christian Couder writes:\n>\n>> On Mon, Mar 16, 2015 at 10:05 PM, Junio C Hamano \n>> wrote:\n>>\n>>> However, you can say \"git bisect bad \" (and \"git bisect good\n>>> \" for that matter) on a rev that is unrelated to what the\n>>> current bisection state is. E.g. after you mark the child of 8 as\n>>> \"bad\", the bisected graph would become\n>>>\n>>> G...1---2---3---4---6---8---B\n>>>\n>>> and you would be offered to test somewhere in the middle, say, 4.\n>>> But it is perfectly OK for you to respond with \"git bisect bad 7\",\n>>> if you know 7 is bad.\n>>>\n>>> I _think_ the current code blindly overwrites the \"bad\" pointer,\n>>> making the bisection state into this graph if you do so.\n>>>\n>>> G...1---2---3---4\n>>> \\\n>>> 5---B\n>>\n>> Yes, we keep only one \"bad\" pointer.\n>>\n>>> This is very suboptimal. The side branch 4-to-7 could be much\n>>> longer than the original trunk 4-to-the-tip, in which case we would\n>>> have made the suspect space _larger_, not smaller.\n>>\n>> Yes, but the user is supposed to not change the \"bad\" pointer for no\n>> good reason.\n>\n> That is irrelevant, no? Nobody is questioning that the user is\n> supposed to judge if a commit is \"good\" or \"bad\" correctly.\n[...]\n>> and/or we could make \"git bisect bad\" accept any number of bad\n>> commitishs.\n>\n> Yes, that is exactly what I meant.\n>\n> The way I understand the Philip's point is that the user may have\n> a-priori knowledge that a breakage from the same cause appears in\n> both tips of these branches.\n\nJust to clarify; my initial query followed on from the way Junio had \ndescribed it with having two tips which were known bad. I hadn't been \naware of how the bisect worked on a DAG, so I wanted to fully understand \nJunio's comment regarding the expectation of a clean jump to commit 4 \n(i.e. shouldn't we test commit 4 before assuming it's actually bad). I \nwas quite happy with a bisect of a linear list, but was unsure about how \nGit dissected DAGs.\n\nI can easily see cases in more complicated product branching where users \nreport intermittent operation for various product variants (especially \nif modular) and one wants to seek out those commits that introduced the \nbehavious (which is typically some racy condition - otherwise it would \nbe deterministic).\n\nGiven Junio's explantion with the two bad commits (on different legs) \nI'd sort of assumed it could be both user given, or algorithmically \ndetermined as part of the bisect.\n\n> In such a case, we can start bisection\n> after marking the merge-base of two 'bad' commits, e.g. 4 in the\n> illustration in the message you are responding to, instead of\n> including 5, 6, and 8 in the suspect set.\n>\n> You need to be careful, though. An obvious pitfall is what you\n> should do when there is a criss-cross merge.\n\nYou end up with possibly two (or more) merges being marked as the source \nof the bad behaviour, especially when racy ;-)\n\n>\n> Thanks.\n> --\n\nHope that helps.\nPhilip\n"},{"id":"258126","messageId":"20150320130235.GA18772@odin.ulthar.us","threadId":"38688","inReplyTo":"xmqqbnjsrcyz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4] rev-list: refuse --first-parent combined with --bisect","fromName":"Scott Schmit","fromEmail":"i.grok@comcast.net","sentAt":"2015-03-20T13:02:35Z","receivedAt":"2015-03-20T13:02:35Z","isPatch":true,"sender":{"key":"i.grok@comcast.net","avatar":null},"body":"On Mon, Mar 16, 2015 at 11:53:08AM -0700, Junio C Hamano wrote:\n> Because the history is not linear in Git, bisection works by\n> shrinking a subgraph of the history DAG that contains \"yet to be\n> tested, suspected to have introduced a badness\" commits. The\n> subgraph is defined as anything reachable from _the_ \"bad\" commit\n> (initially, the one you give to the command when you start) that are\n> not reachable from any of the \"good\" commits.\n> \n> Suppose you started from this graph. Time flows left to right as\n> usual.\n> \n> ---0---2---4---6---8---9\n> \\ /\n> 1---3---5---7\n> \n> Then mark the initial good and bad commits as G and B.\n> \n> ---G---2---4---6---8---B\n> \\ /\n> 1---3---5---7\n> \n> And imagine that you are asked to check 4, which turns out to be\n> good. We do not _move_ G to 4; we mark 4 as good, while keeping\n> 0 also as good.\n> \n> ---G---2---G---6---8---B\n> \\ /\n> 1---3---5---7\n> \n> And if you are next asked to check 5, and mark it as good, the graph\n> will become like this:\n> \n> ---G---2---G---6---8---B\n> \\ /\n> 1---3---G---7\n> \n> Of course, at this point, the subgraph of suspects are 6, 7, 8 and\n> 9, and the subgraph no longer is affected by the fact that 0 is\n> good. But it is crucial to keep 0 marked as good in the step before\n> this one, before you tested 5, as that is what allows us not having\n> to test any ancestors of 0 at all.\n> \n> Now, one may wonder why we need multiple \"good\" commits but we do\n> not need multiple \"bad\" commits. This comes from the nature of\n> \"bisection\", which is a tool to find a _single_ breakage [*1*], and\n> a fundamental assumption is that a breakage does not fix itself.\n> \n> Hence, if you have a history that looks like this:\n> \n> \n> G...1---2---3---4---6---8---B\n> \\\n> 5---7---B\n> \n> it follows that 4 must also be \"bad\". It used to be good long time\n> ago somewhere before 1, and somewhere along way on the history,\n> there was a single breakage event that we are hunting for. That\n> single event cannot be 5, 6, 7 or 8 because breakage at say 5 would\n> not explain why the tip of the upper branch is broken---its breakage\n> has no way to propagate there. The breakage must have happened at 4\n> or before that commit.\n\nBut what if 7 & 8 are the same patch, cherry-picked? Or nearly the same\npatch, but with some conflict resolution?\n\nCouldn't that lead to the case that 4, 5, and 6 are good, while 7 & 8\nare bad? Or does that violate the \"single breakage\" rule in a way that\nmight be too subtle for some users?\n\n-- \nScott Schmit\n"},{"id":"258259","messageId":"20150321220144.GG11832@vps892.directvps.nl","threadId":"38688","inReplyTo":"xmqqh9tghaky.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5] rev-list: refuse --first-parent combined with --bisect","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2015-03-21T22:01:44Z","receivedAt":"2015-03-21T22:01:44Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Thu, Mar 19, 2015 at 03:43:57PM -0700, Junio C Hamano wrote:\n> Kevin Daudt writes:\n> \n> > rev-list --bisect is used by git bisect, but never together with\n> > --first-parent. Because rev-list --bisect together with --first-parent\n> > is not handled currently, and even leads to segfaults, refuse to use\n> > both options together.\n> >\n> > Because this is not supported, it makes little sense to use git log\n> > --bisect --first parent either, because refs/heads/bad is not limited to\n> > the first parent chain.\n> >\n> > Helped-by: Junio C. Hamano \n> > Helped-by: Eric Sunshine \n> > Signed-off-by: Kevin Daudt \n> > ---\n> \n> Thanks; will queue.\n\nThank you too, for your thorough explanation in this.\n"}]}